| Message ID | 1556402706-176271-3-git-send-email-dragan.cvetic@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: xilinx sd-fec drive | expand |
On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > +#define DRIVER_NAME "xilinx_sdfec" > +#define DRIVER_VERSION "0.3" Version means nothing with the driver in the kernel tree, please remove it. > +#define DRIVER_MAX_DEV BIT(MINORBITS) Why this number? Why limit yourself to any number? > + > +static struct class *xsdfec_class; Do you really need your own class? > +static atomic_t xsdfec_ndevs = ATOMIC_INIT(0); Why? > +static dev_t xsdfec_devt; Why? Why not use misc_device for this? Why do you need your own major with a bunch of minor devices reserved ahead of time? Why not just create a new misc device for every individual device that happens to be found in the system? That will make the code a lot simpler and smaller and easier. > + > +/** > + * struct xsdfec_dev - Driver data for SDFEC > + * @regs: device physical base address > + * @dev: pointer to device struct > + * @config: Configuration of the SDFEC device > + * @open_count: Count of char device being opened > + * @xsdfec_cdev: Character device handle > + * @irq_lock: Driver spinlock > + * > + * This structure contains necessary state for SDFEC driver to operate > + */ > +struct xsdfec_dev { > + void __iomem *regs; > + struct device *dev; > + struct xsdfec_config config; > + atomic_t open_count; > + struct cdev xsdfec_cdev; > + /* Spinlock to protect state_updated and stats_updated */ > + spinlock_t irq_lock; > +}; > + > +static const struct file_operations xsdfec_fops = { > + .owner = THIS_MODULE, > +}; No operations at all? That's an easy driver :) thanks, greg k-h
Hi Greg, Please find my inline comments below, Regards Dragan > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Thursday 2 May 2019 18:20 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > +#define DRIVER_NAME "xilinx_sdfec" > > +#define DRIVER_VERSION "0.3" > > Version means nothing with the driver in the kernel tree, please remove > it. Will be removed. Thank you. > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > Why this number? Why limit yourself to any number? > There can be max 8 devices for this driver. I'll change to 8. > > + > > +static struct class *xsdfec_class; > > Do you really need your own class? When writing a character device driver, my goal is to create and register an instance of that structure associated with a struct file_operations, exposing a set of operations to the user-space. One of the steps to make this goal is Create a class for a devices, visible in /sys/class/. > > > +static atomic_t xsdfec_ndevs = ATOMIC_INIT(0); > > Why? At the end this become a minor number. It is not needed, will be removed. Thanks. > > > +static dev_t xsdfec_devt; > > Why? > > Why not use misc_device for this? Why do you need your own major with a > bunch of minor devices reserved ahead of time? Why not just create a > new misc device for every individual device that happens to be found in > the system? That will make the code a lot simpler and smaller and > easier. > > > > > + > > +/** > > + * struct xsdfec_dev - Driver data for SDFEC > > + * @regs: device physical base address > > + * @dev: pointer to device struct > > + * @config: Configuration of the SDFEC device > > + * @open_count: Count of char device being opened > > + * @xsdfec_cdev: Character device handle > > + * @irq_lock: Driver spinlock > > + * > > + * This structure contains necessary state for SDFEC driver to operate > > + */ > > +struct xsdfec_dev { > > + void __iomem *regs; > > + struct device *dev; > > + struct xsdfec_config config; > > + atomic_t open_count; > > + struct cdev xsdfec_cdev; > > + /* Spinlock to protect state_updated and stats_updated */ > > + spinlock_t irq_lock; > > +}; > > + > > +static const struct file_operations xsdfec_fops = { > > + .owner = THIS_MODULE, > > +}; > > No operations at all? That's an easy driver :) The operations are implemented in the later patches. > > thanks, > > greg k-h
On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > Hi Greg, > > Please find my inline comments below, > > Regards > Dragan > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Thursday 2 May 2019 18:20 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > +#define DRIVER_NAME "xilinx_sdfec" > > > +#define DRIVER_VERSION "0.3" > > > > Version means nothing with the driver in the kernel tree, please remove > > it. > > Will be removed. Thank you. > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > Why this number? Why limit yourself to any number? > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > + > > > +static struct class *xsdfec_class; > > > > Do you really need your own class? > > When writing a character device driver, my goal is to create and register an instance > of that structure associated with a struct file_operations, exposing a set of operations > to the user-space. One of the steps to make this goal is Create a class for a devices, > visible in /sys/class/. Why do you need a class? Again, why not just use the misc_device api, that seems much more relevant here and will make the code a lot simpler. thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Saturday 4 May 2019 08:55 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > Hi Greg, > > > > Please find my inline comments below, > > > > Regards > > Dragan > > > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > Sent: Thursday 2 May 2019 18:20 > > > To: Dragan Cvetic <draganc@xilinx.com> > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > +#define DRIVER_VERSION "0.3" > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > it. > > > > Will be removed. Thank you. > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > Why this number? Why limit yourself to any number? > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > + > > > > +static struct class *xsdfec_class; > > > > > > Do you really need your own class? > > > > When writing a character device driver, my goal is to create and register an instance > > of that structure associated with a struct file_operations, exposing a set of operations > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > visible in /sys/class/. > > Why do you need a class? Again, why not just use the misc_device api, > that seems much more relevant here and will make the code a lot simpler. > The driver can have 8 devices in SoC plus more in Programming Logic. It looked logical to group them under the same MAJOR, although they are independent of each other. Is this argument strong enough to use class? > thanks, > > greg k-h
On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Saturday 4 May 2019 08:55 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > Hi Greg, > > > > > > Please find my inline comments below, > > > > > > Regards > > > Dragan > > > > > > > -----Original Message----- > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > Sent: Thursday 2 May 2019 18:20 > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > it. > > > > > > Will be removed. Thank you. > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > + > > > > > +static struct class *xsdfec_class; > > > > > > > > Do you really need your own class? > > > > > > When writing a character device driver, my goal is to create and register an instance > > > of that structure associated with a struct file_operations, exposing a set of operations > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > visible in /sys/class/. > > > > Why do you need a class? Again, why not just use the misc_device api, > > that seems much more relevant here and will make the code a lot simpler. > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > It looked logical to group them under the same MAJOR, although they > are independent of each other. Is this argument strong enough to use > class? Not really :) 8 devices is pretty small. What tool will be trying to talk to all of these devices and how was it going to find out what devices were in the system? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Monday 6 May 2019 13:34 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > Sent: Saturday 4 May 2019 08:55 > > > To: Dragan Cvetic <draganc@xilinx.com> > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > > Hi Greg, > > > > > > > > Please find my inline comments below, > > > > > > > > Regards > > > > Dragan > > > > > > > > > -----Original Message----- > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > Sent: Thursday 2 May 2019 18:20 > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > > it. > > > > > > > > Will be removed. Thank you. > > > > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > > > + > > > > > > +static struct class *xsdfec_class; > > > > > > > > > > Do you really need your own class? > > > > > > > > When writing a character device driver, my goal is to create and register an instance > > > > of that structure associated with a struct file_operations, exposing a set of operations > > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > > visible in /sys/class/. > > > > > > Why do you need a class? Again, why not just use the misc_device api, > > > that seems much more relevant here and will make the code a lot simpler. > > > > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > > It looked logical to group them under the same MAJOR, although they > > are independent of each other. Is this argument strong enough to use > > class? > > Not really :) > > 8 devices is pretty small. What tool will be trying to talk to all of > these devices and how was it going to find out what devices were in the > system? > These devices are Forward Error Correction encoder/decoder and will be part of the RF communication chain. They will be included in the system through DT. Also, described in DT. > thanks, > > greg k-h
On Tue, May 07, 2019 at 08:48:41AM +0000, Dragan Cvetic wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Monday 6 May 2019 13:34 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > > > > > > > -----Original Message----- > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > Sent: Saturday 4 May 2019 08:55 > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > > > Hi Greg, > > > > > > > > > > Please find my inline comments below, > > > > > > > > > > Regards > > > > > Dragan > > > > > > > > > > > -----Original Message----- > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > Sent: Thursday 2 May 2019 18:20 > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > > > it. > > > > > > > > > > Will be removed. Thank you. > > > > > > > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > > > > > + > > > > > > > +static struct class *xsdfec_class; > > > > > > > > > > > > Do you really need your own class? > > > > > > > > > > When writing a character device driver, my goal is to create and register an instance > > > > > of that structure associated with a struct file_operations, exposing a set of operations > > > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > > > visible in /sys/class/. > > > > > > > > Why do you need a class? Again, why not just use the misc_device api, > > > > that seems much more relevant here and will make the code a lot simpler. > > > > > > > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > > > It looked logical to group them under the same MAJOR, although they > > > are independent of each other. Is this argument strong enough to use > > > class? > > > > Not really :) > > > > 8 devices is pretty small. What tool will be trying to talk to all of > > these devices and how was it going to find out what devices were in the > > system? > > > > These devices are Forward Error Correction encoder/decoder > and will be part of the RF communication chain. They will be included > in the system through DT. Also, described in DT. Userspace doesn't mess with DT. I am asking what userspace tool/program is going to be interacting with these devices through your now-custom api you are creating. Do you have a link to that software, and how is that code doing the "determine what device nodes are associated with what devices" logic? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday 7 May 2019 10:40 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > On Tue, May 07, 2019 at 08:48:41AM +0000, Dragan Cvetic wrote: > > > > > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > Sent: Monday 6 May 2019 13:34 > > > To: Dragan Cvetic <draganc@xilinx.com> > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > Sent: Saturday 4 May 2019 08:55 > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > > > > Hi Greg, > > > > > > > > > > > > Please find my inline comments below, > > > > > > > > > > > > Regards > > > > > > Dragan > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > > Sent: Thursday 2 May 2019 18:20 > > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > > > > it. > > > > > > > > > > > > Will be removed. Thank you. > > > > > > > > > > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > > > > > > > + > > > > > > > > +static struct class *xsdfec_class; > > > > > > > > > > > > > > Do you really need your own class? > > > > > > > > > > > > When writing a character device driver, my goal is to create and register an instance > > > > > > of that structure associated with a struct file_operations, exposing a set of operations > > > > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > > > > visible in /sys/class/. > > > > > > > > > > Why do you need a class? Again, why not just use the misc_device api, > > > > > that seems much more relevant here and will make the code a lot simpler. > > > > > > > > > > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > > > > It looked logical to group them under the same MAJOR, although they > > > > are independent of each other. Is this argument strong enough to use > > > > class? > > > > > > Not really :) > > > > > > 8 devices is pretty small. What tool will be trying to talk to all of > > > these devices and how was it going to find out what devices were in the > > > system? > > > > > > > These devices are Forward Error Correction encoder/decoder > > and will be part of the RF communication chain. They will be included > > in the system through DT. Also, described in DT. > > Userspace doesn't mess with DT. > > I am asking what userspace tool/program is going to be interacting with > these devices through your now-custom api you are creating. Do you have > a link to that software, and how is that code doing the "determine what > device nodes are associated with what devices" logic? > Example code is not public yet, sorry. The index number in the device name is a link to device, see snippet from the example code: #define FEC_DEC "/dev/xsdfec0" dec_fd = open_xsdfec(FEC_DEC); The index number corresponds to the device order in DT. > thanks, > > greg k-h
On Tue, May 07, 2019 at 11:55:42AM +0000, Dragan Cvetic wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Tuesday 7 May 2019 10:40 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > On Tue, May 07, 2019 at 08:48:41AM +0000, Dragan Cvetic wrote: > > > > > > > > > > -----Original Message----- > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > Sent: Monday 6 May 2019 13:34 > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > Sent: Saturday 4 May 2019 08:55 > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > > > > > Hi Greg, > > > > > > > > > > > > > > Please find my inline comments below, > > > > > > > > > > > > > > Regards > > > > > > > Dragan > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > > > Sent: Thursday 2 May 2019 18:20 > > > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > > > > > it. > > > > > > > > > > > > > > Will be removed. Thank you. > > > > > > > > > > > > > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > > > > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > > > > > > > > > + > > > > > > > > > +static struct class *xsdfec_class; > > > > > > > > > > > > > > > > Do you really need your own class? > > > > > > > > > > > > > > When writing a character device driver, my goal is to create and register an instance > > > > > > > of that structure associated with a struct file_operations, exposing a set of operations > > > > > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > > > > > visible in /sys/class/. > > > > > > > > > > > > Why do you need a class? Again, why not just use the misc_device api, > > > > > > that seems much more relevant here and will make the code a lot simpler. > > > > > > > > > > > > > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > > > > > It looked logical to group them under the same MAJOR, although they > > > > > are independent of each other. Is this argument strong enough to use > > > > > class? > > > > > > > > Not really :) > > > > > > > > 8 devices is pretty small. What tool will be trying to talk to all of > > > > these devices and how was it going to find out what devices were in the > > > > system? > > > > > > > > > > These devices are Forward Error Correction encoder/decoder > > > and will be part of the RF communication chain. They will be included > > > in the system through DT. Also, described in DT. > > > > Userspace doesn't mess with DT. > > > > I am asking what userspace tool/program is going to be interacting with > > these devices through your now-custom api you are creating. Do you have > > a link to that software, and how is that code doing the "determine what > > device nodes are associated with what devices" logic? > > > > Example code is not public yet, sorry. Ok, then I think we need to wait for that to get this merged at the minimum, don't you agree? Otherwise how do we even know that any of these codepaths are tested? > The index number in the device name > is a link to device, see snippet from the example code: > > #define FEC_DEC "/dev/xsdfec0" > dec_fd = open_xsdfec(FEC_DEC); > > The index number corresponds to the device order in DT. So that implies you don't need a class at all, right? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday 7 May 2019 13:21 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > On Tue, May 07, 2019 at 11:55:42AM +0000, Dragan Cvetic wrote: > > > > > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > Sent: Tuesday 7 May 2019 10:40 > > > To: Dragan Cvetic <draganc@xilinx.com> > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > On Tue, May 07, 2019 at 08:48:41AM +0000, Dragan Cvetic wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > Sent: Monday 6 May 2019 13:34 > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > > Sent: Saturday 4 May 2019 08:55 > > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > > > On Fri, May 03, 2019 at 04:41:21PM +0000, Dragan Cvetic wrote: > > > > > > > > Hi Greg, > > > > > > > > > > > > > > > > Please find my inline comments below, > > > > > > > > > > > > > > > > Regards > > > > > > > > Dragan > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > > > > > > Sent: Thursday 2 May 2019 18:20 > > > > > > > > > To: Dragan Cvetic <draganc@xilinx.com> > > > > > > > > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > > > > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan > <dkiernan@xilinx.com> > > > > > > > > > Subject: Re: [PATCH V3 02/12] misc: xilinx-sdfec: add core driver > > > > > > > > > > > > > > > > > > On Sat, Apr 27, 2019 at 11:04:56PM +0100, Dragan Cvetic wrote: > > > > > > > > > > +#define DRIVER_NAME "xilinx_sdfec" > > > > > > > > > > +#define DRIVER_VERSION "0.3" > > > > > > > > > > > > > > > > > > Version means nothing with the driver in the kernel tree, please remove > > > > > > > > > it. > > > > > > > > > > > > > > > > Will be removed. Thank you. > > > > > > > > > > > > > > > > > > > > > > > > > > > +#define DRIVER_MAX_DEV BIT(MINORBITS) > > > > > > > > > > > > > > > > > > Why this number? Why limit yourself to any number? > > > > > > > > > > > > > > > > > > > > > > > > > There can be max 8 devices for this driver. I'll change to 8. > > > > > > > > > > > > > > > > > > + > > > > > > > > > > +static struct class *xsdfec_class; > > > > > > > > > > > > > > > > > > Do you really need your own class? > > > > > > > > > > > > > > > > When writing a character device driver, my goal is to create and register an instance > > > > > > > > of that structure associated with a struct file_operations, exposing a set of operations > > > > > > > > to the user-space. One of the steps to make this goal is Create a class for a devices, > > > > > > > > visible in /sys/class/. > > > > > > > > > > > > > > Why do you need a class? Again, why not just use the misc_device api, > > > > > > > that seems much more relevant here and will make the code a lot simpler. > > > > > > > > > > > > > > > > > > > The driver can have 8 devices in SoC plus more in Programming Logic. > > > > > > It looked logical to group them under the same MAJOR, although they > > > > > > are independent of each other. Is this argument strong enough to use > > > > > > class? > > > > > > > > > > Not really :) > > > > > > > > > > 8 devices is pretty small. What tool will be trying to talk to all of > > > > > these devices and how was it going to find out what devices were in the > > > > > system? > > > > > > > > > > > > > These devices are Forward Error Correction encoder/decoder > > > > and will be part of the RF communication chain. They will be included > > > > in the system through DT. Also, described in DT. > > > > > > Userspace doesn't mess with DT. > > > > > > I am asking what userspace tool/program is going to be interacting with > > > these devices through your now-custom api you are creating. Do you have > > > a link to that software, and how is that code doing the "determine what > > > device nodes are associated with what devices" logic? > > > > > > > Example code is not public yet, sorry. > > Ok, then I think we need to wait for that to get this merged at the > minimum, don't you agree? Otherwise how do we even know that any of > these codepaths are tested? > > > The index number in the device name > > is a link to device, see snippet from the example code: > > > > #define FEC_DEC "/dev/xsdfec0" > > dec_fd = open_xsdfec(FEC_DEC); > > > > The index number corresponds to the device order in DT. > > So that implies you don't need a class at all, right? > Greg, you won:( Thanks for patience, I appreciate it very much. Dragan > thanks, > > greg k-h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 3209ee0..1a1fe9c 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -528,6 +528,18 @@ config PCI_ENDPOINT_TEST Enable this configuration option to enable the host side test driver for PCI Endpoint. +config XILINX_SDFEC + tristate "Xilinx SDFEC 16" + help + This option enables support for the Xilinx SDFEC (Soft Decision + Forward Error Correction) driver. This enables a char driver + for the SDFEC. + + You may select this driver if your design instantiates the + SDFEC(16nm) hardened block. To compile this as a module choose M. + + If unsure, say N. + config MISC_RTSX tristate default MISC_RTSX_PCI || MISC_RTSX_USB diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index c362395..ee31140c 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -49,6 +49,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/ obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o obj-$(CONFIG_SRAM) += sram.o obj-$(CONFIG_SRAM_EXEC) += sram-exec.o +obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o obj-y += mic/ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c new file mode 100644 index 0000000..278754b --- /dev/null +++ b/drivers/misc/xilinx_sdfec.c @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Xilinx SDFEC + * + * Copyright (C) 2016 - 2017 Xilinx, Inc. + * + * Description: + * This driver is developed for SDFEC16 (Soft Decision FEC 16nm) + * IP. It exposes a char device interface in sysfs and supports file + * operations like open(), close() and ioctl(). + */ + +#include <linux/cdev.h> +#include <linux/device.h> +#include <linux/fs.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/poll.h> +#include <linux/slab.h> +#include <linux/uaccess.h> +#include <linux/spinlock.h> +#include <linux/clk.h> + +#include <uapi/misc/xilinx_sdfec.h> + +#define DRIVER_NAME "xilinx_sdfec" +#define DRIVER_VERSION "0.3" +#define DRIVER_MAX_DEV BIT(MINORBITS) + +static struct class *xsdfec_class; +static atomic_t xsdfec_ndevs = ATOMIC_INIT(0); +static dev_t xsdfec_devt; + +/** + * struct xsdfec_dev - Driver data for SDFEC + * @regs: device physical base address + * @dev: pointer to device struct + * @config: Configuration of the SDFEC device + * @open_count: Count of char device being opened + * @xsdfec_cdev: Character device handle + * @irq_lock: Driver spinlock + * + * This structure contains necessary state for SDFEC driver to operate + */ +struct xsdfec_dev { + void __iomem *regs; + struct device *dev; + struct xsdfec_config config; + atomic_t open_count; + struct cdev xsdfec_cdev; + /* Spinlock to protect state_updated and stats_updated */ + spinlock_t irq_lock; +}; + +static const struct file_operations xsdfec_fops = { + .owner = THIS_MODULE, +}; + +static int xsdfec_probe(struct platform_device *pdev) +{ + struct xsdfec_dev *xsdfec; + struct device *dev; + struct device *dev_create; + struct resource *res; + int err; + + xsdfec = devm_kzalloc(&pdev->dev, sizeof(*xsdfec), GFP_KERNEL); + if (!xsdfec) + return -ENOMEM; + + xsdfec->dev = &pdev->dev; + xsdfec->config.fec_id = atomic_read(&xsdfec_ndevs); + spin_lock_init(&xsdfec->irq_lock); + + dev = xsdfec->dev; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + xsdfec->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(xsdfec->regs)) { + dev_err(dev, "Unable to map resource"); + err = PTR_ERR(xsdfec->regs); + goto err_xsdfec_dev; + } + + /* Save driver private data */ + platform_set_drvdata(pdev, xsdfec); + + cdev_init(&xsdfec->xsdfec_cdev, &xsdfec_fops); + xsdfec->xsdfec_cdev.owner = THIS_MODULE; + err = cdev_add(&xsdfec->xsdfec_cdev, + MKDEV(MAJOR(xsdfec_devt), xsdfec->config.fec_id), 1); + if (err < 0) { + dev_err(dev, "cdev_add failed"); + err = -EIO; + goto err_xsdfec_dev; + } + + if (!xsdfec_class) { + err = -EIO; + dev_err(dev, "xsdfec class not created correctly"); + goto err_xsdfec_cdev; + } + + dev_create = + device_create(xsdfec_class, dev, + MKDEV(MAJOR(xsdfec_devt), xsdfec->config.fec_id), + xsdfec, "xsdfec%d", xsdfec->config.fec_id); + if (IS_ERR(dev_create)) { + dev_err(dev, "unable to create device"); + err = PTR_ERR(dev_create); + goto err_xsdfec_cdev; + } + + atomic_set(&xsdfec->open_count, 1); + dev_info(dev, "XSDFEC%d Probe Successful", xsdfec->config.fec_id); + atomic_inc(&xsdfec_ndevs); + return 0; + + /* Failure cleanup */ +err_xsdfec_cdev: + cdev_del(&xsdfec->xsdfec_cdev); +err_xsdfec_dev: + return err; +} + +static int xsdfec_remove(struct platform_device *pdev) +{ + struct xsdfec_dev *xsdfec; + struct device *dev = &pdev->dev; + + xsdfec = platform_get_drvdata(pdev); + if (!xsdfec) + return -ENODEV; + + if (!xsdfec_class) { + dev_err(dev, "xsdfec_class is NULL"); + return -EIO; + } + + device_destroy(xsdfec_class, + MKDEV(MAJOR(xsdfec_devt), xsdfec->config.fec_id)); + cdev_del(&xsdfec->xsdfec_cdev); + atomic_dec(&xsdfec_ndevs); + return 0; +} + +static const struct of_device_id xsdfec_of_match[] = { + { + .compatible = "xlnx,sd-fec-1.1", + }, + { /* end of table */ } +}; +MODULE_DEVICE_TABLE(of, xsdfec_of_match); + +static struct platform_driver xsdfec_driver = { + .driver = { + .name = "xilinx-sdfec", + .of_match_table = xsdfec_of_match, + }, + .probe = xsdfec_probe, + .remove = xsdfec_remove, +}; + +static int __init xsdfec_init_mod(void) +{ + int err; + + xsdfec_class = class_create(THIS_MODULE, DRIVER_NAME); + if (IS_ERR(xsdfec_class)) { + err = PTR_ERR(xsdfec_class); + pr_err("%s : Unable to register xsdfec class", __func__); + return err; + } + + err = alloc_chrdev_region(&xsdfec_devt, 0, DRIVER_MAX_DEV, DRIVER_NAME); + if (err < 0) { + pr_err("%s : Unable to get major number", __func__); + goto err_xsdfec_class; + } + + err = platform_driver_register(&xsdfec_driver); + if (err < 0) { + pr_err("%s Unabled to register %s driver", __func__, + DRIVER_NAME); + goto err_xsdfec_drv; + } + return 0; + + /* Error Path */ +err_xsdfec_drv: + unregister_chrdev_region(xsdfec_devt, DRIVER_MAX_DEV); +err_xsdfec_class: + class_destroy(xsdfec_class); + return err; +} + +static void __exit xsdfec_cleanup_mod(void) +{ + platform_driver_unregister(&xsdfec_driver); + unregister_chrdev_region(xsdfec_devt, DRIVER_MAX_DEV); + class_destroy(xsdfec_class); + xsdfec_class = NULL; +} + +module_init(xsdfec_init_mod); +module_exit(xsdfec_cleanup_mod); + +MODULE_AUTHOR("Xilinx, Inc"); +MODULE_DESCRIPTION("Xilinx SD-FEC16 Driver"); +MODULE_LICENSE("GPL"); +MODULE_VERSION(DRIVER_VERSION); diff --git a/include/uapi/misc/xilinx_sdfec.h b/include/uapi/misc/xilinx_sdfec.h new file mode 100644 index 0000000..ba577fa --- /dev/null +++ b/include/uapi/misc/xilinx_sdfec.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ +/* + * Xilinx SD-FEC + * + * Copyright (C) 2016 - 2017 Xilinx, Inc. + * + * Description: + * This driver is developed for SDFEC16 IP. It provides a char device + * in sysfs and supports file operations like open(), close() and ioctl(). + */ +#ifndef __XILINX_SDFEC_H__ +#define __XILINX_SDFEC_H__ + +/** + * enum xsdfec_state - State. + * @XSDFEC_INIT: Driver is initialized. + * @XSDFEC_STARTED: Driver is started. + * @XSDFEC_STOPPED: Driver is stopped. + * @XSDFEC_NEEDS_RESET: Driver needs to be reset. + * @XSDFEC_PL_RECONFIGURE: Programmable Logic needs to be recofigured. + * + * This enum is used to indicate the state of the driver. + */ +enum xsdfec_state { + XSDFEC_INIT = 0, + XSDFEC_STARTED, + XSDFEC_STOPPED, + XSDFEC_NEEDS_RESET, + XSDFEC_PL_RECONFIGURE, +}; + +/** + * struct xsdfec_config - Configuration of SD-FEC core. + * @fec_id: ID of SD-FEC instance. ID is limited to the number of active + * SD-FEC's in the FPGA and is related to the driver instance + * Minor number. + */ +struct xsdfec_config { + __s32 fec_id; +}; + +#endif /* __XILINX_SDFEC_H__ */