This patch also doesn't introduce bugs but it's sort of whacky and hard
to understand. Also the subject and description imply or say "fix" but
it's just a cleanup. The original code was also proper but just messy.
On Tue, Mar 17, 2015 at 08:31:24PM +0530, Sudip Mukherjee wrote:
> fixed the handling of dev_t and the major number.
> now the major and minor number is passed to the init function.
> similarly in the cleanup function dev_t is passed to unregister it.
>
> Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> ---
> drivers/staging/unisys/visorchipset/file.c | 18 ++++++++----------
> drivers/staging/unisys/visorchipset/file.h | 4 ++--
> .../staging/unisys/visorchipset/visorchipset_main.c | 10 +++-------
> 3 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
> index 9ca7f1e..224e214 100644
> --- a/drivers/staging/unisys/visorchipset/file.c
> +++ b/drivers/staging/unisys/visorchipset/file.c
> @@ -30,7 +30,6 @@
>
> static struct cdev file_cdev;
> static struct visorchannel **file_controlvm_channel;
> -static dev_t majordev = -1; /**< indicates major num for device */
> static BOOL registered = FALSE;
>
> static int visorchipset_open(struct inode *inode, struct file *file);
> @@ -50,15 +49,17 @@ static const struct file_operations visorchipset_fops = {
> };
>
> int
> -visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
> +visorchipset_file_init(int major, int minor,
> + struct visorchannel **controlvm_channel)
Pass the dev_t majordev;
1) Then it's consistent with visorchipset_file_cleanup()
2) You need majordev anyway.
Don't save majordev as a global because globals are bad and you already
have a copy in Visorchipset_platform_device.dev.devt.
> {
> int rc = 0;
> + dev_t majordev;
>
> file_controlvm_channel = controlvm_channel;
> - majordev = major_dev;
> cdev_init(&file_cdev, &visorchipset_fops);
> file_cdev.owner = THIS_MODULE;
> - if (MAJOR(majordev) == 0) {
> + majordev = MKDEV(major, minor);
> + if (major == 0) {
> /* dynamic major device number registration required */
> if (alloc_chrdev_region(&majordev, 0, 1, MYDRVNAME) < 0)
> return -1;
> @@ -69,23 +70,20 @@ visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
> return -1;
> registered = TRUE;
> }
> - rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
> + rc = cdev_add(&file_cdev, MKDEV(major, 0), 1);
This should just be:
rc = cdev_add(&file_cdev, majordev, 1);
So here is my suggestion:
[patch 1] delete dead code I mentioned in my previous email.
This deletes "registered" and the (MAJOR(majordev) >= 0) check.
[patch 2] pass majordev to visorchipset_file_cleanup()
This lets you delete the "majordev" global.
[patch 3] small cleanup in visorchipset_file_init()
- rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
+ rc = cdev_add(&file_cdev, majordev, 1);
There are several other ways you could break it up but do something like
that.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
