On Mon, May 06, 2019 at 12:23:56PM +0000, Dragan Cvetic wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > Sent: Saturday 4 May 2019 08:55 > > To: Dragan Cvetic <draganc@xxxxxxxxxx> > > Cc: arnd@xxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > > mark.rutland@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Derek Kiernan <dkiernan@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxx] > > > > Sent: Thursday 2 May 2019 18:20 > > > > To: Dragan Cvetic <draganc@xxxxxxxxxx> > > > > Cc: arnd@xxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > > > > mark.rutland@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Derek Kiernan <dkiernan@xxxxxxxxxx> > > > > 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
