[i2c] [patch 2.6.23-rc5] i2c_msg kerneldoc

[Jean Delvare]

Hi David,

On Fri, 07 Sep 2007 14:10:29 -0700, David Brownell wrote:
> Clarify use of the I2C_M_* flags by highlighting the fact that
> most of them depend on I2C_FUNC_PROTOCOL_MANGLING.
> 
> Also provide kerneldoc for i2c_smbus_read_block_data() and also
> for "struct i2c_msg".

Thanks for contributing i2c documentation, this is appreciated.

Review:

> --- at91.orig/drivers/i2c/i2c-core.c	2007-09-07 11:14:16.000000000 -0700
> +++ at91/drivers/i2c/i2c-core.c	2007-09-07 13:15:06.000000000 -0700
> @@ -1457,7 +1457,22 @@ s32 i2c_smbus_write_word_data(struct i2c
>  }
>  EXPORT_SYMBOL(i2c_smbus_write_word_data);
>  
> -/* Returns the number of read bytes */
> +/**
> + * i2c_smbus_read_block_data - SMBus "read block data" request

In SMBus terminology this is simply named an "SMBus block read".

> + * @client: handle to slave device
> + * @command: command byte issued to let the slave know what data should
> + *	be returned
> + * @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.

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.

> + *
> + * Returns the number of bytes read in the slave's response, else a negative
> + * number to indicate some kind of error.
> + *
> + * Note that using this function requires the client's adapter support the
> + * I2C_FUNC_SMBUS_READ_BLOCK_DATA functionality.  Not all adapter drivers

... requires THAT the client's adapter supportS ...?

> + * support this; its emulation through I2C messaging relies on a specific
> + * mechanism which may not be implemented.

               ^-- Maybe insert (I2C_M_RECV_LEN) for completeness?

> + */
>  s32 i2c_smbus_read_block_data(struct i2c_client *client, u8 command,
>  			      u8 *values)
>  {
> --- at91.orig/include/linux/i2c.h	2007-09-07 10:15:54.000000000 -0700
> +++ at91/include/linux/i2c.h	2007-09-07 13:39:40.000000000 -0700
> @@ -456,19 +456,48 @@ static inline int i2c_adapter_id(struct 
>  }
>  #endif /* __KERNEL__ */
>  
> -/*
> - * I2C Message - used for pure i2c transaction, also from /dev interface
> +/**
> + * struct i2c_msg - Represents an I2C transaction beginning with START
> + * @addr: Slave address, either seven or ten bits.  When this is a ten
> + *	bit address, I2C_M_TEN must be set in @flags and the adapter
> + *	must support I2C_FUNC_10BIT_ADDR.
> + * @flags: I2C_M_RD is handled by all adapters.  No other flags may be
> + *	provided unless the adapter exported the relevant I2C_FUNC__*

Doubled underscore.

> + *	flags through i2c_check_functionality().
> + * @len: Length of the buffer being read or written to the I2C address.

... read FROM or written to ...

BTW, len, isn't necessarily the length of the buffer. It is the number
of bytes to read from or write to the slave, that's different.

> + *	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().

> + * @buf: The buffer into which data is read, or from which it's written.
> + *
> + * An i2c_msg is the lowest level representation of an i2c transaction.

It actually represents only a part of a transaction.

> + * It is exposed to drivers by @i2c_transfer(), to userspace by i2c-dev, and
> + * to I2C adapter drivers through the @i2c_adapter. at master_xfer() method.

"Exposed"? This sentence doesn't make much sense to me, sorry. Can you
try to reformulate it?

> + *
> + * Except when I2C "protocol mangling" is used, all i2c adapters implement
> + * the standard rules for I2C transactions.  Each transaction begins with a
> + * START.  That is followed by the slave address, and a bit encoding read
> + * versus write.  Then follow all the data bytes.  The transfer terminates
> + * with a NAK, or when all specified bytes have been transferred and ACKed.
> + * If this is the last message in a group, it is followed by a STOP; otherwise
> + * it is followed by the (repeated) START of the next @i2c_msg.
> + *
> + * Alternatively, adapters supporting I2C_FUNC_PROTOCOL_MANGLING accept

Additionally, not alternatively.

> + * some flags which change the standard protocol behavior described above.
> + * Those flags are defined only for use with broken/nonconformant slaves.

nonconforming?

>   */
>  struct i2c_msg {
>  	__u16 addr;	/* slave address			*/
>  	__u16 flags;
> -#define I2C_M_TEN	0x10	/* we have a ten bit chip address	*/
> -#define I2C_M_RD	0x01
> -#define I2C_M_NOSTART	0x4000
> -#define I2C_M_REV_DIR_ADDR	0x2000
> -#define I2C_M_IGNORE_NAK	0x1000
> -#define I2C_M_NO_RD_ACK		0x0800
> -#define I2C_M_RECV_LEN		0x0400 /* length will be first received byte */
> +#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 :(

I think there is some confusion with regards to the protocol mangling
flags (admittedly explainable by the fact that the implementation
itself is pretty lame). Your "iffs" above are not really correct,
because we have a single functionality bit for 4 different types of
protocol mangling. Adapter drivers which support one protocol mangling
type are not required to support all the other types. Quite to the
contrary: as I said before, I want to limit these implementations to
the strict minimum.

Thus I2C_FUNC_PROTOCOL_MANGLING means that the adapter supports _some_
type of protocol mangling. In other words, chip drivers using any of
the four I2C_M_* mangling options should first check for
I2C_FUNC_PROTOCOL_MANGLING, but a success is no guarantee that the
adapter actually supports the mangling type in question. That makes
I2C_FUNC_PROTOCOL_MANGLING pretty useless IMHO. But OTOH I don't think
that we want to waste more functionality bits for these hacks. If it
were only me, I2C_FUNC_PROTOCOL_MANGLING wouldn't even exist. Whoever
makes use of these funky flags has to know exactly what the underlying
i2c adapter is anyway, and should not let their device driver attach to
other adapters. So checking for the adapter ID instead is probably the
right thing to do.

-- 
Jean Delvare