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/. > > > +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
