Re: [PATCH] i2c: Split I2C_M_NOSTART support out of I2C_FUNC_PROTOCOL_MANGLING

Hi Mark,

On Thu,  3 May 2012 11:53:36 +0100, Mark Brown wrote:

Since there are uses for I2C_M_NOSTART which are much more sensible and
standard than most of the protocol mangling functionality (the main one
being gather writes to devices where something like a register address
needs to be inserted before a block of data) create a new I2C_FUNC_NOSTART
for this feature and update all the users to use it.

This is all correct, but it should be documented in
Documentation/i2c/i2c-protocol. At the moment documentation still says
that I2C_M_NOSTART is a weird protocol quirk nobody should be using.

When you update the documentation, I think it is important to stress
that there are now two use cases of I2C_M_NOSTART. If direction
changes, it is a rarely needed protocol quirk. If direction doesn't
change, it is used for buffer gathering.

In the case of regmap-i2c we remove the requirement for mangling as
I2C_M_NOSTART is the only mangling feature which is being used.

Signed-off-by: Mark Brown <redacted>
---
 Documentation/i2c/functionality       |    1 +
 drivers/base/regmap/regmap-i2c.c      |    2 +-
 drivers/i2c/algos/i2c-algo-bit.c      |    2 +-
 drivers/i2c/algos/i2c-algo-pcf.c      |    2 +-
 drivers/i2c/busses/i2c-nuc900.c       |    3 ++-
 drivers/i2c/busses/i2c-s3c2410.c      |    3 ++-
 drivers/input/joystick/as5011.c       |    1 +
 drivers/video/matrox/matroxfb_maven.c |    1 +
 include/linux/i2c.h                   |    5 +++--
 9 files changed, 13 insertions(+), 7 deletions(-)

Review:

You must also update the description of I2C_FUNC_PROTOCOL_MANGLING to
no longer mention I2C_M_NOSTART.

Not true. Transfers always begin with a Start, even when I2C_M_NOSTART
is used, because it is never set for the first message.
I2C_FUNC_NOSTART means that messages other than the first may not get
their usual Repeated Start and address prefix. So a better working IMHO
would be:

+  I2C_FUNC_NOSTART                Can skip repeated start sequence

2 more quoted lines

A quick grep suggests that i2c-algo-pcf doesn't support I2C_M_NOSTART,
only I2C_M_REV_DIR_ADDR. So you don't want to add I2C_FUNC_NOSTART to
the functionality mask.

39 more quoted lines

Sorry for nitpicking but wouldn't I2C_FUNC_GATHER be a better name?
NOSTART is an implementation detail now, the high level feature is the
ability to gather multiple messages into one.

Also I think I would prefer if you used 0x00000010 for the new
functionality, so that it isn't too far away from
I2C_FUNC_PROTOCOL_MANGLING.

 
 #define I2C_FUNC_SMBUS_BYTE		(I2C_FUNC_SMBUS_READ_BYTE | \
 					 I2C_FUNC_SMBUS_WRITE_BYTE)

Thanks,
-- 
Jean Delvare

On Thu, May 03, 2012 at 08:36:17PM +0200, Jean Delvare wrote:

You must also update the description of I2C_FUNC_PROTOCOL_MANGLING to
no longer mention I2C_M_NOSTART.

For backwards ABI compatibility _PROTCOL_MANGING still has to imply
_NOSTART, though it's unclear if that should be documented here.  For
drivers it should for the most part flow naturally as I'd expect they'll
end up implementing one or more of the mangling flags anyway.  For
applications I guess it means that they should fall back to checking for
_PROTOCOL_MANGLING if _NOSTART isn't there.

quoted

+#define I2C_FUNC_NOSTART		0x10000000 /* I2C_M_NOSTART */

Sorry for nitpicking but wouldn't I2C_FUNC_GATHER be a better name?
NOSTART is an implementation detail now, the high level feature is the
ability to gather multiple messages into one.

I kept it this way mostly because it means that the capability flag has
the same name as the feature flag which seemed helpful from a usability
point of view.

There is also the fact you could in theory also implement gather support
in other ways (eg, using DMA) though at present there's no API level
support for this and the users do have to code this specific
implementation.

On Thu, May 03, 2012 at 08:36:17PM +0200, Jean Delvare wrote:

This is all correct, but it should be documented in
Documentation/i2c/i2c-protocol. At the moment documentation still says
that I2C_M_NOSTART is a weird protocol quirk nobody should be using.

When you update the documentation, I think it is important to stress
that there are now two use cases of I2C_M_NOSTART. If direction
changes, it is a rarely needed protocol quirk. If direction doesn't
change, it is used for buffer gathering.

Hrm, actually rereading the documentation it wasn't all that clear that
these were particularly disrecommended - it just mentioned that the
flags existed and might be needed.  I strengthened the documentation
here as well.