[PATCH v4 1/4] staging: unisys: remove unneeded comparison

[PATCH v4 1/4] staging: unisys: remove unneeded comparison

[Sudip Mukherjee]

visorchipset_file_cleanup() is called from visorchipset_exit() which
is the module_exit function and this function is executing means
module_init succeeded, so registered will always be true at this time.
and majordev has also been initialized in the init function. hence
these comparisons will always be true.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v4: messed up the subject in v3
v3: broke the previous patch in this series

 drivers/staging/unisys/visorchipset/file.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index 9ca7f1e..cbed1ba2 100644
--- a/drivers/staging/unisys/visorchipset/file.c
+++ b/drivers/staging/unisys/visorchipset/file.c
@@ -81,13 +81,9 @@ visorchipset_file_cleanup(void)
 	if (file_cdev.ops != NULL)
 		cdev_del(&file_cdev);
 	file_cdev.ops = NULL;
-	if (registered) {
-		if (MAJOR(majordev) >= 0) {
-			unregister_chrdev_region(majordev, 1);
-			majordev = MKDEV(0, 0);
-		}
-		registered = FALSE;
-	}
+	unregister_chrdev_region(majordev, 1);
+	majordev = MKDEV(0, 0);
+	registered = FALSE;
 }
 
 static int
-- 
1.8.1.2

[PATCH v4 2/4] staging: unisys: use local dev_t instead of global

[Sudip Mukherjee]

the dev_t is stored in visorchipset_platform_device.dev.devt, so we
can pass that value as an argument to visorchipset_file_cleanup()
instead of using the global variable in file.c

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v4: messed up the subject in v3
v3: broke the previous patch in this series

 drivers/staging/unisys/visorchipset/file.c              | 3 +--
 drivers/staging/unisys/visorchipset/file.h              | 2 +-
 drivers/staging/unisys/visorchipset/visorchipset_main.c | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index cbed1ba2..d62908d 100644
--- a/drivers/staging/unisys/visorchipset/file.c
+++ b/drivers/staging/unisys/visorchipset/file.c
@@ -76,13 +76,12 @@ visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
 }
 
 void
-visorchipset_file_cleanup(void)
+visorchipset_file_cleanup(dev_t majordev)
 {
 	if (file_cdev.ops != NULL)
 		cdev_del(&file_cdev);
 	file_cdev.ops = NULL;
 	unregister_chrdev_region(majordev, 1);
-	majordev = MKDEV(0, 0);
 	registered = FALSE;
 }
 
diff --git a/drivers/staging/unisys/visorchipset/file.h b/drivers/staging/unisys/visorchipset/file.h
index dc7a195..b32a472 100644
--- a/drivers/staging/unisys/visorchipset/file.h
+++ b/drivers/staging/unisys/visorchipset/file.h
@@ -22,6 +22,6 @@
 
 int visorchipset_file_init(dev_t majorDev,
 			   struct visorchannel **pControlVm_channel);
-void visorchipset_file_cleanup(void);
+void visorchipset_file_cleanup(dev_t majordev);
 
 #endif
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 9c8605d..f2663d2c 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -2278,7 +2278,7 @@ visorchipset_exit(void)
 
 	visorchannel_destroy(controlvm_channel);
 
-	visorchipset_file_cleanup();
+	visorchipset_file_cleanup(visorchipset_platform_device.dev.devt);
 	POSTCODE_LINUX_2(DRIVER_EXIT_PC, POSTCODE_SEVERITY_INFO);
 }
 
-- 
1.8.1.2

[PATCH v4 3/4] staging: unisys: use local variable instead of global

[Sudip Mukherjee]

dev_t was passed as an argument to visorchipset_file_init() which was
storing that value in a global variable so that it can be used while
cleanup. now we are no longer using the global variable in the cleanup
so we donot need to keep the global variable anymore.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v4: messed up the subject in v3
v3: broke the previous patch in this series

 drivers/staging/unisys/visorchipset/file.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index d62908d..a52939a4 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);
@@ -55,21 +54,20 @@ visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
 	int rc = 0;
 
 	file_controlvm_channel = controlvm_channel;
-	majordev = major_dev;
 	cdev_init(&file_cdev, &visorchipset_fops);
 	file_cdev.owner = THIS_MODULE;
-	if (MAJOR(majordev) == 0) {
+	if (MAJOR(major_dev) == 0) {
 		/* dynamic major device number registration required */
-		if (alloc_chrdev_region(&majordev, 0, 1, MYDRVNAME) < 0)
+		if (alloc_chrdev_region(&major_dev, 0, 1, MYDRVNAME) < 0)
 			return -1;
 		registered = TRUE;
 	} else {
 		/* static major device number registration required */
-		if (register_chrdev_region(majordev, 1, MYDRVNAME) < 0)
+		if (register_chrdev_region(major_dev, 1, MYDRVNAME) < 0)
 			return -1;
 		registered = TRUE;
 	}
-	rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
+	rc = cdev_add(&file_cdev, major_dev, 1);
 	if (rc  < 0)
 		return -1;
 	return 0;
-- 
1.8.1.2

Re: [PATCH v4 3/4] staging: unisys: use local variable instead of global

[Dan Carpenter]

I don't like how these are broken up at all.  It's random grab bag of
renaming and deleting.

Do it like this, perhaps:

[patch 1] delete registered

Leave the if (MAJOR(majordev) >= 0) for now, because it just
confuses issues to mix it in.  Delete all references to registered.

[patch 1] use local variable in visorchipset_file_init()
Don't delete anything.

[patch 2] pass major_dev to visorchipset_file_cleanup()

This deletes the "if (MAJOR(majordev) >= 0)" condition because it's
obvious that can't be true.  This deletes all references to the
"majordev" global because that isn't needed any more.  The whole reason
it was needed originally was because of visorchipset_file_cleanup() so
it's wrong to do the deleting in this patch which is about
visorchipset_file_init().

regards,
dan carpenter

[PATCH v4 4/4] staging: unisys: remove unused variable

[Sudip Mukherjee]

after the first patch of this series the registered variable was
no longer required.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v4: messed up the subject in v3
v3: broke the previous patch in this series

 drivers/staging/unisys/visorchipset/file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index a52939a4..24cac93 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 BOOL registered = FALSE;
 
 static int visorchipset_open(struct inode *inode, struct file *file);
 static int visorchipset_release(struct inode *inode, struct file *file);
@@ -60,12 +59,10 @@ visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
 		/* dynamic major device number registration required */
 		if (alloc_chrdev_region(&major_dev, 0, 1, MYDRVNAME) < 0)
 			return -1;
-		registered = TRUE;
 	} else {
 		/* static major device number registration required */
 		if (register_chrdev_region(major_dev, 1, MYDRVNAME) < 0)
 			return -1;
-		registered = TRUE;
 	}
 	rc = cdev_add(&file_cdev, major_dev, 1);
 	if (rc  < 0)
@@ -80,7 +77,6 @@ visorchipset_file_cleanup(dev_t majordev)
 		cdev_del(&file_cdev);
 	file_cdev.ops = NULL;
 	unregister_chrdev_region(majordev, 1);
-	registered = FALSE;
 }
 
 static int
-- 
1.8.1.2

Re: [PATCH v4 1/4] staging: unisys: remove unneeded comparison

[Dan Carpenter]

On Tue, Mar 24, 2015 at 05:47:34PM +0530, Sudip Mukherjee wrote:
> visorchipset_file_cleanup() is called from visorchipset_exit() which
> is the module_exit function and this function is executing means
> module_init succeeded, so registered will always be true at this time.
> and majordev has also been initialized in the init function. hence
> these comparisons will always be true.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

This is a partial patch.  The "one thing per patch" means don't do half
a thing per patch.

We now set "registered" but there are no users.

regards,
dan carpenter

Re: [PATCH v4 1/4] staging: unisys: remove unneeded comparison

[Sudip Mukherjee]

On Tue, Mar 24, 2015 at 03:30:21PM +0300, Dan Carpenter wrote:
> On Tue, Mar 24, 2015 at 05:47:34PM +0530, Sudip Mukherjee wrote:
> > visorchipset_file_cleanup() is called from visorchipset_exit() which
> > is the module_exit function and this function is executing means
> > module_init succeeded, so registered will always be true at this time.
> > and majordev has also been initialized in the init function. hence
> > these comparisons will always be true.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> 
> This is a partial patch.  The "one thing per patch" means don't do half
> a thing per patch.
> 
> We now set "registered" but there are no users.
it is being removed separately in the 4/4 patch.
do you want that to be combined here in this patch?
but then won't that become 2 different changes in a single patch?
my message says we are removing that as the comparison is always
true.

regards
sudip
> 
> regards,
> dan carpenter
>