[i2c] i2c-tiny-usb review

[Jean Delvare]

Hi Till,

On Wed, 18 Apr 2007 22:37:01 +0200, Till Harbaum / Lists wrote:
> Am Mittwoch 18 April 2007 schrieb Jean Delvare:
> > First of all: please provide your driver as a patch against a recent
> > kernel tree. Something that I can easily apply and test-compile.
>
> Find the new version attached. I was done against an ubuntu patched
> 2.6.20, but it has been verified to apply to 2.6.21-rc7 as well with only
> minor offsets.

Yes, it applies fine. See my comments below:

> I have tried to fix everything you asked for. Thanks for that valuable
> input. And my offer is still valid: Do you want one of those modules?
> You can e.g. use it to easily test client drivers.

Yes, I would love to have such a thing for testing purposes. However,
beware I don't have any electronics stuff here, so for example I won't
be able to program the chip myself. You'll have to send me a chip which
is pre-programmed. Unless it can be reprogrammed over USB directly?

Ideally, if you can add some I2C device on it so that I can actually use
it, even better.

Second-pass review:

> /* commands via USB, must match command ids in the firmware */
> #define CMD_ECHO       0
> #define CMD_GET_FUNC   1
> #define CMD_SET_DELAY  2
> #define CMD_GET_STATUS 3
> 
> #define CMD_I2C_IO        4
> #define CMD_I2C_IO_BEGIN  (1<<0)
> #define CMD_I2C_IO_END    (1<<1)

These are still aligned using spaces and not tabs. This is a no go,
sorry.

> MODULE_PARM_DESC(delay, "bit delay in microseconds, e.g. 10 for 100kHz");

10 for 100 kHz is not just an example, it's the default value. I
believe this information is valuable for the user.

> static inline int usb_read(struct i2c_adapter *i2c_adap, int cmd,
> 			   int value, int index, void *data, int len);
> 
> static inline int usb_write(struct i2c_adapter *adapter, int cmd,
> 			    int value, int index, void *data, int len);
> 

One i2c_adapter is called i2c_adap, the other is called adapter; this
is inconsistent ;)

But more importantly, you cannot forward-declare inline functions. It
breaks on gcc 3. If you want to inline the functions, you have to
define them completely before they are ever used. You have to fix this.

> --- linux-source-2.6.20.orig/drivers/i2c/busses/Kconfig	2007-01-06 19:45:52.000000000 +0100
> +++ linux-source-2.6.20/drivers/i2c/busses/Kconfig	2007-04-18 20:17:39.000000000 +0200
> @@ -476,6 +476,17 @@
>  
>  	  If you don't know what to do here, definitely say N.
>  
> +config I2C_TINY_USB
> +	tristate "I2C-TINY-USB"

All caps give me headaches. Please try to find a more user-friendly
string.

> +	depends on I2C && USB
> +	help
> +	  If you say yes to this option, support will be included for the 
> +          I2C-TINY-USB I2C interface. See 

Broken indentation. Here again it would be more friendly to explain
that the device is a simple I2C-over-USB module that can be used for
various experimental purposes.

> +	  http://www.harbaum.org/till/i2c_tiny_usb for hardware details.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-tiny-usb.
> +
>  config I2C_VERSATILE
>  	tristate "ARM Versatile/Realview I2C bus support"
>  	depends on I2C && (ARCH_VERSATILE || ARCH_REALVIEW)

Additionally, can you please add yourself to MAINTAINERS as the
maintainer for this driver?

Thanks,
-- 
Jean Delvare