Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare <khali@linux-fr.org>]

Hi Mark,

Answering your questions before I forget about them and let a week pass
again...

Anything I don't comment on, means that you were right and I agree with
you.

> > May I ask why you define separate types for the M41T81 and the M41T85,
> > when you handle both exactly the same way?
> 
> The only reason is so that its obvious that both the t81 and t85 are
> supported.  If I make it M41T81 only then I can easily see someone
> grep'ing around looking for M41T85, not finding it, and writing their
> own driver.  I don't see any harm in having both, do you?

It wastes some memory, and you may later fix something for the M41T81
and forget to fix it for the M41T85.

If your only concern is to help grepers, you can add a clear list of
supported chips either as a comment at the top of the source file, or
as Documentation/i2c/chips/m41t00. That's what we do for hardware
monitoring chips.

No big deal anyway, so the decision is up to you.

> > What you do here are basically SMBus Read Byte and SMBus Write Byte
> > transactions. The code would be much more simple if you were using the
> > i2c_smbus_read_byte_data and i2c_smbus_write_byte_data functions, which
> > take care of all the technical details.
> 
> That's true but I assumed that since I was using i2c_transfer
> earlier, I should use it here.  Is that a bad assumption?
> I do see that ds1337.c uses both types.

Bad assumption, indeed. Nothing prevents you from using the smbus
functions and the i2c functions in the same driver, as long as your
call to i2c_check_functionality covers every function you use. So,
just use whatever makes your driver easier to write.

Thanks,
-- 
Jean Delvare