[lkml]   [2008]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [i2c] [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,
    Hi David,

    On Sat, 10 May 2008 09:36:50 -0700, David Brownell wrote:
    > > It's not that easy. There are some drivers which are both in-tree and
    > > out-of-tree, for which such a change means adding ifdefs.
    > Actually, I thought it *WAS* that easy. See the appended patch,
    > which goes on top of the various doc and (mostly SMBus) fault
    > handling patches I've sent ... the old names are still available
    > (only needed by out-of-tree drivers), but marked as __deprecated.
    > So: no #ifdefs required.

    No #ifdefs required until we drop the deprecated helpers, which will
    inevitably happen, even if only in several years. So in practice,
    #ifdefs would be required for out-of-tree drivers (or semi-out-of-tree
    drivers such as v4l/dvb drivers).

    > I agree it's not worth while for *all* the SMBus functions that
    > use names-made-up-for-Linux. But for the two which are really
    > badly misnamed (after *different* SMBus operations), I suggest
    > fixing would be a reasonable thing.

    That's tempting, but see below.

    > > And there is
    > > i2c-dev.h (the user-space one) which has similar functions,
    > That's problematic in its own right though. Not only that
    > the *kernel* file Documentation/i2c/dev-interface referring
    > to those functions, which are unavailable from the header
    > provided by the kernel. Or that there's no relationship
    > between the kernel and userspace files of the same name.
    > But also that those functions are actually a bit too large
    > to be appropriate as inlines, even once you manage to track
    > it down (as part of a "tools" package, not a "library").

    I agree that the i2c-dev.h mess needs some love and this is on my to-do
    list for this year. That's not necessarily related to the problem at
    hand though.

    > > if we
    > > rename only the kernel variants, there will be some confusion. But if
    > > we rename also the user-space variants, then it's up to 2.4 kernel
    > > users to have different names for kernel-space and user-space functions.
    > True, but that would be a question for a "libsmbus" or somesuch
    > to deal with. Not a kernel issue.

    Whether it's in the kernel or outside the kernel is irrelevant. The
    developers are likely to be the same, and the person who will have to
    deal with the confusion is the same (i.e.: me.)

    > ===========
    > Two of the SMBus operations are using confusingly inappropriate names:
    > i2c_smbus_read_byte() does not execute the SMBus "Read Byte" protocol
    > ... it implements the SMBus "Receive Byte" protocol instead!!
    > i2c_smbus_write_byte() does not execute the SMBus "Write Byte" protocol
    > ... it implements the SMBus "Send Byte" protocol instead!!
    > This patch changes the names of those functions, so they no longer
    > use names of different operations (which they do not implement).
    > ---
    > Documentation/i2c/chips/max6875 | 4 ++--
    > Documentation/i2c/smbus-protocol | 12 +++++-------
    > Documentation/i2c/writing-clients | 4 ++--
    > drivers/gpio/pcf857x.c | 8 ++++----
    > drivers/hwmon/ds1621.c | 2 +-
    > drivers/hwmon/lm90.c | 2 +-
    > drivers/i2c/chips/eeprom.c | 10 +++++-----
    > drivers/i2c/chips/max6875.c | 2 +-
    > drivers/i2c/chips/pcf8574.c | 4 ++--
    > drivers/i2c/chips/pcf8591.c | 10 +++++-----
    > drivers/i2c/chips/tsl2550.c | 16 ++++++++--------
    > drivers/i2c/i2c-core.c | 12 ++++++------
    > drivers/media/video/saa7110.c | 2 +-
    > drivers/media/video/saa7185.c | 2 +-
    > drivers/media/video/tda9840.c | 8 ++++----
    > drivers/media/video/tea6415c.c | 4 ++--
    > drivers/media/video/tea6420.c | 4 ++--
    > drivers/w1/masters/ds2482.c | 10 +++++-----
    > include/linux/i2c.h | 20 ++++++++++++++++++--
    > 19 files changed, 75 insertions(+), 61 deletions(-)

    I'm not going to take this, sorry. I still believe that it will cause
    more trouble than is worth at this point in time. For example, after
    applying your patch, we'd have functionality defines no longer matching
    the SMBus helper names (I2C_FUNC_SMBUS_READ_BYTE,
    I2C_FUNC_SMBUS_WRITE_BYTE). And you can't easily rename these because
    they are part of the i2c-dev API.

    I'm not saying that the suggested cleanup shouldn't be done, nor that
    it cannot be done. All I'm saying is that it will be a lot of work if
    we do it properly, and potentially a lot of trouble for many developers
    out there, all for a marginal benefit, and I think that our time can be
    better used to solve other, more important problems. And you've done a
    very good job at documenting the current implementation, so I hope that
    it is clear enough for everyone now.

    Jean Delvare

     \ /
      Last update: 2008-05-20 11:23    [W:0.029 / U:78.468 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site