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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
