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

[Håvard Skinnemoen]

On Fri, March 9, 2007 20:30, David Brownell wrote:
> 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.

The idea is to keep the output value at 0 and switch the output driver on
and off. I assumed that changing the direction was the easiest way to
achieve this.

I never really thought about output-only pins. Is it actually possible to
implement i2c properly on such hardware?

> 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.

Isn't it better to switch the direction to input since the driver needs to
watch the pin state in order to support clock stretching and multi-master?

>> +	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.  ;)

No I really do mean input, as I want to put the bus into an idle state
initially, which means no output drivers and passive pullup on both lines.

The gpio_set_value() calls should go away and the output value should be
explicitly set to 0 when turning on the output drivers. In its present
form, the driver happens to work on my hardware, which is all I really
cared about when writing it ;-)

>> +	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.

Ok, but how do I inform i2c-algo-bit about whether or not 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!

Sure.

Haavard

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/