thanks for the contribution.
the ST M41xx product line is a good one, lots of variants,
I've used the 41T80 which adds an interrupt output but
unfortunately has a slightly different register layout.
The reason for using smbus calls instead of i2c_transfer is that
now you will work with any bus master rather than just
the pure i2c bit-bangers. So no matter what the hardware setup
the driver will work. That's why the smbus calls are "preferred".
You could use smbus_i2c_read_i2c_block_data() to get the efficiency back,
but most smbus drivers don't support that. So you did it right to
design for maximum flexibility.
mds
Mark A. Greer wrote:
> 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
