Re: [PATCH] Bitbanging i2c bus driver using the GPIO API

[David Brownell <david-b@pacbell.net>]

On Friday 09 March 2007 10:48 am, Haavard Skinnemoen wrote:
> This is a very simple bitbanging i2c bus driver utilizing the new
> arch-neutral GPIO API. Useful for chips that don't have a built-in
> i2c controller, additional i2c busses, or testing purposes.

That's the right idea!  But remember that not all GPIOs support
reading back the actual value on SCL (it's an OUT pin, so lacking
multidrive capability the values "should" be what you wrote), so
getscl() support should depend on a flag in platform data.  In
the same vein, if SCL is an output-only pin, you won't be able
to change its direction ... but then, I'm not sure why you were
changing its direction in setscl() rather than just its value.

I2C has another interesting special case.  at91_set_multi_drive()
would be appropriate (yes?) for ARCH_AT91 to use on SCL, to best
support both clock stretching and multi-master configurations.


> +	gpio_direction_input(pdata->sda_pin);
> +	gpio_direction_input(pdata->scl_pin);
> +	gpio_set_value(pdata->sda_pin, 0);
> +	gpio_set_value(pdata->scl_pin, 0);

Surely you mean "output" in both cases.  So you can set the
value.  Setting the value on an input pin is undefined.  ;)


> +	printk(KERN_INFO "i2c-gpio: using pins 0x%x (sda) 0x%x (scl)\n",
> +	       pdata->sda_pin, pdata->scl_pin);

Please, no hex there.  I think dev_info() would be better; and it
might be nice to report whether clock stretching is supported.


> --- a/include/linux/i2c-id.h
> +++ b/include/linux/i2c-id.h
> @@ -194,6 +194,7 @@
>  #define I2C_HW_B_EM28XX		0x01001f /* em28xx video capture cards */
>  #define I2C_HW_B_CX2341X	0x010020 /* Conexant CX2341X MPEG encoder cards */
>  #define I2C_HW_B_INTELFB	0x010021 /* intel framebuffer driver */
> +#define I2C_HW_B_GPIO		0x010022 /* Generic GPIO-based driver */

It'd be nice to completely abolish those IDs, starting by not
adding new ones.  Especially, not adding unused ones!

- Dave