[i2c] [patch 2.6.23-rc5] i2c_msg kerneldoc

[David Brownell]

> Thanks for contributing i2c documentation, this is appreciated.

Someone's gotta do it.  ;)

Unless I respond to it below, you should assume I just accepted
your feedback as-is.


> > + * @values: byte array into which data will be read; big enough to hold
> > + *	the data returned by the slave.
>
> Where "big enough" is I2C_SMBUS_BLOCK_MAX. It might be worth insisting
> on the fact that to be on the safe side, the buffer should always be
> I2C_SMBUS_BLOCK_MAX bytes wide, even if the caller expects less bytes
> to be returned by the slave.

Actually as I commented earlier, I think that the arbitrary limitation
that the buffers be "fixed/small size" is problematic and error prone.

That's experience talking.  When implementations have such limits, it's
only a matter of time before those limits are found to be too small.
(Unfortunately, with this I2C stack removing that limit looks tricky.)


> This suggests that our interface is less than optimal and we should let
> the caller pass the buffer size? If we want to do this, now is probably
> the right time, as there is a single user of this function in-tree.

Ideally we'd have both TX and RX block paths able to take buffers of
any size physically possible with the encoding (1-255), and to report
just how many bytes were transferred.  Like most I/O interfaces ...

Agreed that callers should be able to specify how many bytes they
will accept; that fixes one of the problems in this area.  As part
of this patch??


> > + * Returns the number of bytes read in the slave's response, else a
> > + * negative number to indicate some kind of error.

Actually that irks me.  The kernel convention is "returns negative
errno", and that's what should be done here and elsewhere in I2C.
Without some convention for error codes, callers can only guess
about what went wrong and what recovery is appropriate.


> > +/**
> > + * struct i2c_msg - Represents an I2C transaction beginning with START

Relabled as "an I2C transaction segment beginning with START".

Turns out this struct deserves much doc attention, since it's part
of the userspace interface (an ABI).


> > + *	For read transactions where I2C_M_RECV_LEN is set, the caller
> > + *	guarantees that this buffer can hold up to 32 bytes in addition
> > + *	to the initial length byte sent by the slave (plus, if used,
> > + *	the SMBus PEC); and this value will be incremented by the number
> > + *	of block data bytes received.
>
> This is true, but not necessarily very interesting: the only caller
> that will ever pass I2C_M_RECV_LEN is i2c_smbus_xfer_emulated().

But ... if the I2C adapter doesn't do that, checking PEC will fail!
It's included (despite the ugliness!) since that's part of how the
adapter must implement RECV_LEN.

By the way, did you consider how this affects I2C_RDWR from userspace?
I took my first glance at that code.  The bounce buffers should probably
use kzalloc(); not copy from userspace when it's a read buffer; and write
back i2c_msg.len for this I2C_M_RECV_LEN case...


> > + * Alternatively, adapters supporting I2C_FUNC_PROTOCOL_MANGLING accept
>
> Additionally, not alternatively.

I think "alternatively" is correct.  It's paired up front with an
"Except when protocol mangling...".  Either the standard protocol
operations apply, or else "alternatively" some protocol manglation
is happening.  I tweaked the presentation a bit though.

Presenting this as either/or should clarify things; most people can
just ignore the manglation paths.


> >  	__u16 flags;
> ...
> > +#define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
> > +#define I2C_M_RD		0x0001
> > +#define I2C_M_NOSTART		0x4000	/* iff I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_REV_DIR_ADDR	0x2000	/* iff I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_IGNORE_NAK	0x1000	/* iff I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_NO_RD_ACK		0x0800	/* iff I2C_FUNC_PROTOCOL_MANGLING */
> > +#define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
> >  	__u16 len;		/* msg length				*/
> >  	__u8 *buf;		/* pointer to msg data			*/
> >  };
>
> While you're here: could you please move these defines outside of the
> structure definition? This is so ugly :(

Actually, this way they aren't stripped out by kerneldoc ... and 
there's no real ambiguity about which field they apply to!


> 	 Your "iffs" above are not really correct,

OK; "if", so as not to imply that all protocol-mangling adapters
support all four mangle-modes.  I also updated the description
of such manglation to highlight the driver portability issue.

Updated patch follows.

- Dave