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

Jean Delvare
2008-05-09T21:21:46Z

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