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 <su...@vectorindia.org>
> ---
>  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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to