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
