Re: [i2c] [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,

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

Hi Maciej,

On Fri, 9 May 2008 21:55:38 +0100 (BST), Maciej W. Rozycki wrote:
> > This is reimplementing i2c_smbus_write_i2c_block_data().
> 
>  Where does it come from?  I fail to see this type of transfer being 
> defined anywhere in the SMBus spec.

It is indeed not.

>                                      I checked the spec before I referred 
> to the implementation in our I2C core and I hope you agree I may not have 
> expected any extensions beyond what the SMBus spec defines.

The "smbus" in these function names are more about "what SMBus
controllers usually can do" than about the SMBus specification. But I
admit you couldn't guess.

>  That written, you are of course correct WRT the reimplementation and I am 
> eager to remove it -- thanks for the point.  I'll skip all your other 
> comments related as obviously implied by this change.
> 
>  Given the function and friends make use of apparently a non-standard
> SMBus transfer, I think they should be called differently, perhaps
> i2c_smbusext_write_i2c_block_data(), etc. or suchlike.

This was an option when the functions where introduced 9 years ago.
But now that it was done, renaming them would cause even more
confusion, I think. I would be fine with adding comments in i2c-core.c
or improving Documentation/i2c/smbus-protocol to make it more obious,
though.

On a related note, you will notice that the other i2c_smbus_* functions
do not follow the naming of SMBus transactions. Again that's something
I regret but I feel that changing the names now would cause a lot of
confusion amongst developers, so I'm not doing it.

> > Mixing code cleanups with functional changes is a Bad Idea (TM).
> 
>  I am happy to bother you with a separate patch including style fixes.  I
> can even create a handful of them, grouping functionally consistent
> changes.

Just one patch should be enough, if I agree with all the changes. You
might make a separate patch with the things I may not agree with, so
that you don't have to cherry-revert them if I indeed don't agree, and
we just merge them if I do agree.

> > >  	dev_info(&client->dev,
> > > -		 "chip found, driver version " DRV_VERSION "\n");
> > > +		 "%s chip found, driver version " DRV_VERSION "\n",
> > > +		 client->name);
> > 
> > Incorrect change, dev_info() already includes the chip name.
> 
>  My system must be a notable exception then, as this change modifies 
> output:
> 
> rtc-m41t80 1-0068: chip found, driver version 0.05
> 
> to:
> 
> rtc-m41t80 1-0068: m41t81 chip found, driver version 0.05
> 
> here.

My bad, for some reason I thought that dev_printk() included the device
name but it in fact includes the driver name. I was wrong, just ignore
me.

-- 
Jean Delvare