[PATCH]I2C device - release cleanup

[PATCH]I2C device - release cleanup

[Jean-Michel Hautbois]

Hi there,

Here is a little patch which aims to cleanup the release function in i2c-dev.c.
This is only a call to single_release, instead of kfree and several things.

Signed-off-by: Jean-Michel Hautbois <jhautbois-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/i2c-dev.c

--- linux-2.6.34-rc2/drivers/i2c/i2c-dev.c.orig	2010-03-23
10:19:26.000000000 +0100
+++ linux-2.6.34-rc2/drivers/i2c/i2c-dev.c	2010-03-23 10:55:54.000000000 +0100
@@ -36,6 +36,7 @@
 #include <linux/i2c-dev.h>
 #include <linux/jiffies.h>
 #include <asm/uaccess.h>
+#include <linux/seq_file.h>

 static struct i2c_driver i2cdev_driver;

@@ -477,12 +478,10 @@ static int i2cdev_open(struct inode *ino
 static int i2cdev_release(struct inode *inode, struct file *file)
 {
 	struct i2c_client *client = file->private_data;
-
+	
 	i2c_put_adapter(client->adapter);
-	kfree(client);
-	file->private_data = NULL;
-
-	return 0;
+	
+	return single_release(inode, file);
 }

 static const struct file_operations i2cdev_fops = {

Re: [PATCH]I2C device - release cleanup

[Jean Delvare]

Hi Jean-Michel,

On Tue, 23 Mar 2010 11:01:28 +0100, Jean-Michel Hautbois wrote:
> Hi there,
> 
> Here is a little patch which aims to cleanup the release function in i2c-dev.c.
> This is only a call to single_release, instead of kfree and several things.
> 
> Signed-off-by: Jean-Michel Hautbois <jhautbois-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/i2c/i2c-dev.c
> 
> --- linux-2.6.34-rc2/drivers/i2c/i2c-dev.c.orig	2010-03-23
> 10:19:26.000000000 +0100
> +++ linux-2.6.34-rc2/drivers/i2c/i2c-dev.c	2010-03-23 10:55:54.000000000 +0100
> @@ -36,6 +36,7 @@
>  #include <linux/i2c-dev.h>
>  #include <linux/jiffies.h>
>  #include <asm/uaccess.h>
> +#include <linux/seq_file.h>
> 
>  static struct i2c_driver i2cdev_driver;
> 
> @@ -477,12 +478,10 @@ static int i2cdev_open(struct inode *ino
>  static int i2cdev_release(struct inode *inode, struct file *file)
>  {
>  	struct i2c_client *client = file->private_data;
> -
> +	

This is adding trailing white-space. Obviously you did not review your
own patch before sending it. And you did not run it through
scripts/checkpatch.pl either.

>  	i2c_put_adapter(client->adapter);
> -	kfree(client);
> -	file->private_data = NULL;
> -
> -	return 0;
> +	
> +	return single_release(inode, file);
>  }
> 
>  static const struct file_operations i2cdev_fops = {

Did you test your patch? I am very skeptical that calling
single_release() out of the blue is the right thing to do. My instinct
tells me that single_release() is only meant for callers of
single_open().

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

Re: [PATCH]I2C device - release cleanup

[Jean Delvare]

On Tue, 23 Mar 2010 13:02:41 +0000, jhautbois-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> Hi Jean,
> 
> < snip >
> > Did you test your patch? I am very skeptical that calling
> 
> > single_release() out of the blue is the right thing to do. My instinct
> 
> > tells me that single_release() is only meant for callers of
> 
> > single_open().
> 
> Well, using this call works fine with my hardware.
> I would say, as before :).
> Looking at the source code of single_release, this is very similar to what  
> is done today.
> 
> But yes, it would also be interesting to use single_open in the open()  
> syscall.
> I think this would be nicer to use only single_*() functions.
> 
> Maybe is it interesting to submit a patch that does a cleanup for all the  
> i2c-dev file ? And not only the release function ?

Yes, that would be better. If you do that, please make sure to run your
patch through scripts/checkpatch.pl before sending it.

-- 
Jean Delvare

Re: [PATCH]I2C device - release cleanup

[Jean Delvare]

On Tue, 23 Mar 2010 19:11:26 +0000, jhautbois-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > Yes, that would be better. If you do that, please make sure to run your
> 
> > patch through scripts/checkpatch.pl before sending it.
> 
> I will take the time to do it. I can't say when :p.
> I did run checkpatch.pl, and I can't remove the 3 "trailing whitespace"  
> errors...

You'll have to, otherwise I won't apply your patches (nobody will).

> Plus the fact that gmail is changing tabs and spaces...

Check the options. At least attaching the patch should work. Or you can
use a "real" e-mail client for sending patches.

-- 
Jean Delvare