Re: [PATCH 02/16] fpga: add FPGA device framework

[Greg KH]

On Thu, Mar 30, 2017 at 08:08:02PM +0800, Wu Hao wrote:
> During FPGA device (e.g PCI-based) discovery, platform devices are
> registered for different FPGA function units. But the device node path
> isn't quite friendly to applications.
> 
> Consider this case, applications want to access child device's sysfs file
> for some information.
> 
> 1) Access using bus-based path (e.g PCI)
> 
>   /sys/bus/pci/devices/xxxxx/fpga_func_a.0/sysfs_file
> 
>   From the path, it's clear which PCI device is the parent, but not perfect
>   solution for applications. PCI device BDF is not fixed, application may
>   need to search all PCI device to find the actual FPGA Device.
> 
> 2) Or access using platform device path
> 
>   /sys/bus/platform/devices/fpga_func_a.0/sysfs_file
> 
>   Applications find the actual function by name easily, but no information
>   about which fpga device it belongs to. It's quite confusing if multiple
>   FPGA devices are in one system.
> 
> 'FPGA Device' class is introduced to resolve this problem. Each node under
> this class represents a fpga device, which may have one or more child
> devices. Applications only need to search under this FPGA Device class
> folder to find the child device node it needs.
> 
> For example, for the platform has 2 fpga devices, each fpga device has
> 3 child devices, the hierarchy looks like this.
> 
> Two nodes are under /sys/class/fpga/:
> /sys/class/fpga/fpga.0
> /sys/class/fpga/fpga.1
> 
> Each node has 1 function A device and 2 function B devices:
> /sys/class/fpga/fpga.0/func_a.0
> /sys/class/fpga/fpga.0/func_b.0
> /sys/class/fpga/fpga.0/func_b.1
> 
> /sys/class/fpga/fpga.1/func_a.1
> /sys/class/fpga/fpga.1/func_b.2
> /sys/class/fpga/fpga.1/func_b.3
> 
> This following APIs are provided by FPGA device framework:
> * fpga_dev_create
>   Create fpga device under the given parent device.
> * fpga_dev_destroy
>   Destroy fpga device
> 
> The following sysfs files are created:
> * /sys/class/fpga/<fpga.x>/name
>   Name of the fpga device.
> 
> Signed-off-by: Tim Whisonant <tim.whisonant@xxxxxxxxx>
> Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
> Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
> Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> ---
>  drivers/fpga/Kconfig          |   6 +++
>  drivers/fpga/Makefile         |   3 ++
>  drivers/fpga/fpga-dev.c       | 120 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-dev.h |  34 ++++++++++++
>  4 files changed, 163 insertions(+)
>  create mode 100644 drivers/fpga/fpga-dev.c
>  create mode 100644 include/linux/fpga/fpga-dev.h
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index ce861a2..d99b640 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -12,6 +12,12 @@ config FPGA
>  	  manager drivers.
>  
>  if FPGA
> +config FPGA_DEVICE
> +	tristate "FPGA Device Framework"
> +	help
> +	  Say Y here if you want support for FPGA Devices from the kernel.
> +	  The FPGA Device Framework adds a FPGA device class and provide
> +	  interfaces to create FPGA devices.
>  
>  config FPGA_REGION
>  	tristate "FPGA Region"
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8df07bc..53a41d2 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -5,6 +5,9 @@
>  # Core FPGA Manager Framework
>  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>  
> +# FPGA Device Framework
> +obj-$(CONFIG_FPGA_DEVICE)		+= fpga-dev.o
> +
>  # FPGA Manager Drivers
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
> diff --git a/drivers/fpga/fpga-dev.c b/drivers/fpga/fpga-dev.c
> new file mode 100644
> index 0000000..0f4c0ed
> --- /dev/null
> +++ b/drivers/fpga/fpga-dev.c
> @@ -0,0 +1,120 @@
> +/*
> + * FPGA Device Framework Driver
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * This work is licensed under a dual BSD/GPLv2 license. When using or
> + * redistributing this file, you may do so under either license. See the
> + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and see
> + * the COPYING file in the top-level directory for the GPLv2 license.

Really?  A BSD licened file that does EXPORT_SYMBOL_GPL and interacts
directly with the driver core?  Please go talk to some of your lawyers
about this before you resubmit this correctly...


> + */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/fpga/fpga-dev.h>
> +
> +static DEFINE_IDA(fpga_dev_ida);
> +static struct class *fpga_dev_class;
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_dev *fdev = to_fpga_dev(dev);
> +
> +	return sprintf(buf, "%s\n", fdev->name);
> +}
> +static DEVICE_ATTR_RO(name);

There already is a name for the device, it's the directory name.

> +
> +static struct attribute *fpga_dev_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_dev);
> +
> +/**
> + * fpga_dev_create - create a fpga device
> + * @parent: parent device
> + * @name: fpga device name
> + *
> + * Return fpga_dev struct for success, error code otherwise.
> + */
> +struct fpga_dev *fpga_dev_create(struct device *parent, const char *name)
> +{
> +	struct fpga_dev *fdev;
> +	int id, ret = 0;
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(parent, "Attempt to register with no name!\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	fdev = kzalloc(sizeof(*fdev), GFP_KERNEL);
> +	if (!fdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	id = ida_simple_get(&fpga_dev_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto error_kfree;
> +	}
> +
> +	fdev->name = name;
> +
> +	device_initialize(&fdev->dev);
> +	fdev->dev.class = fpga_dev_class;
> +	fdev->dev.parent = parent;
> +	fdev->dev.id = id;
> +
> +	ret = dev_set_name(&fdev->dev, "fpga.%d", id);
> +	if (ret)
> +		goto error_device;
> +
> +	ret = device_add(&fdev->dev);
> +	if (ret)
> +		goto error_device;
> +
> +	dev_dbg(fdev->dev.parent, "fpga device [%s] created\n", fdev->name);
> +
> +	return fdev;
> +
> +error_device:
> +	ida_simple_remove(&fpga_dev_ida, id);
> +error_kfree:
> +	kfree(fdev);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(fpga_dev_create);
> +
> +static void fpga_dev_release(struct device *dev)
> +{
> +	struct fpga_dev *fdev = to_fpga_dev(dev);
> +
> +	ida_simple_remove(&fpga_dev_ida, fdev->dev.id);
> +	kfree(fdev);
> +}
> +
> +static int __init fpga_dev_class_init(void)
> +{
> +	pr_info("FPGA Device framework\n");
> +
> +	fpga_dev_class = class_create(THIS_MODULE, "fpga");
> +	if (IS_ERR(fpga_dev_class))
> +		return PTR_ERR(fpga_dev_class);
> +
> +	fpga_dev_class->dev_groups = fpga_dev_groups;
> +	fpga_dev_class->dev_release = fpga_dev_release;
> +
> +	return 0;
> +}
> +
> +static void __exit fpga_dev_class_exit(void)
> +{
> +	class_destroy(fpga_dev_class);
> +}
> +
> +MODULE_DESCRIPTION("FPGA Device framework");
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +subsys_initcall(fpga_dev_class_init);
> +module_exit(fpga_dev_class_exit);
> diff --git a/include/linux/fpga/fpga-dev.h b/include/linux/fpga/fpga-dev.h
> new file mode 100644
> index 0000000..7b58356
> --- /dev/null
> +++ b/include/linux/fpga/fpga-dev.h
> @@ -0,0 +1,34 @@
> +/*
> + * FPGA Device Driver Header
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * This work is licensed under a dual BSD/GPLv2 license. When using or
> + * redistributing this file, you may do so under either license. See the
> + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and see
> + * the COPYING file in the top-level directory for the GPLv2 license.

Again with the dual license, please fix up.

> + *
> + */
> +#ifndef _LINUX_FPGA_DEV_H
> +#define _LINUX_FPGA_DEV_H
> +
> +/**
> + * struct fpga_dev - fpga device structure
> + * @name: name of fpga device
> + * @dev: fpga device
> + */
> +struct fpga_dev {
> +	const char *name;
> +	struct device dev;

struct device already has a name, why duplicate it here?

thanks,

greg k-h