[PATCH][I2C] ST M41T00 I2C RTC chip driver

[Mark A. Greer]

Thank you for your comments, Jean.

Jean Delvare wrote:

>Hi Mark,
>
>  
>
>>+	  This driver can also be built as a module.  If so, the module
>>+	  will be called i2c-m41t00.
>>    
>>
>
>It'll actually be called m41t00, according to the Makefile.
>

Good catch.

>  
>
>>+struct m41t00_data {
>>+	struct i2c_client client;
>>+};
>>    
>>
>
>You don't have to do that. Including the i2c_client stucture in the data
>structure is a trick which let us get both allocated with a single
>kmalloc (and freed with a single kfree) while still respecting the
>arch-dependent alignment requirements. If you have no private data to
>carry around, you can do the kmalloc on the i2c_client structure
>directly, and have client->data point to NULL (which it actually already
>does thanks to memset). This will save some code in both the detection
>and the detach functions.
>
>However, if you know that, in a future update of this driver, you *will*
>have to store client-private data, then I guess you can keep it this way.
>

Its gone.

>>+	i2c_detach_client(client);
>>    
>>
>
>This one supposedly can fail.
>

Okay, I'll check.

>  
>
>>+	.name		= "M41T00",
>>    
>>
>
>No caps in name please (will be used in sysfs).
>

Done.

>
>  
>
>>+static int __devinit
>>+m41t00_init(void)
>>+{
>>+	return i2c_add_driver(&m41t00_driver);
>>+}
>>
>>+static void __devexit
>>+m41t00_exit(void)
>>+{
>>+	i2c_del_driver(&m41t00_driver);
>>+	return;
>>+}
>>    
>>
>
>Should be __init and __exit, respectively, unless I am mistaken. And the
>last return is usless.
>

I thought the __devxxx ones were the best ones to use but I don't know 
for sure.  I'll change them to __init/__exit.

>
>I'm also suspicious about the other __devexit and __devinit you used. No
>other i2c chip drivers has them.
>  
>

Done.

Here is a replacement patch that should address Jean Delvare and Dick 
Johnson's issues.  Please let me know if there are any more issues.

Thanks,

Mark

Signed-off-by: Mark A. Greer <mgreer at mvista.com>
--
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: m41t00_2.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050201/a855c9c6/attachment.pl