[PATCH] i2c-core: i2c bitbang gpio structure

[PATCH] i2c-core: i2c bitbang gpio structure

[Wu, Bryan]

Hi folks,

A new structure is added to i2c-core for GPIO-based I2C interface
adapter. My latest GPIO based I2C adapter driver for Blackfin system
will use this stuff. And also IXP4XX GPIO based I2C driver can also be
moved to this.

Signed-off-by: Bryan Wu <bryan.wu@analog.com> 
---
 include/linux/i2c.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: include/linux/i2c.h
===================================================================
--- include/linux/i2c.h	(revision 2813)
+++ include/linux/i2c.h	(working copy)
@@ -201,6 +201,26 @@ struct i2c_algorithm {
 };
 
 /*
+ * Some chips do not have an I2C unit, so GPIO lines are just used to 
+ * Used as platform_data to provide GPIO pin information to this kind GPIO 
+ * based I2C driver.
+ */
+struct i2c_bitbang_gpio {
+	int sda;
+	int scl;
+};
+
+static inline int i2c_bitbang_gpio_sda(struct i2c_bitbang_gpio *gpio)
+{
+	return (gpio->sda);
+}
+
+static inline int i2c_bitbang_gpio_scl(struct i2c_bitbang_gpio *gpio)
+{
+	return (gpio->scl);
+}
+
+/*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.
  */
_

Thanks,
-Bryan Wu

Re: [PATCH] i2c-core: i2c bitbang gpio structure

[Jean Delvare]

Hi Bryan,

On Fri, 09 Mar 2007 18:13:21 +0800, Wu, Bryan wrote:
> Hi folks,
> 
> A new structure is added to i2c-core for GPIO-based I2C interface
> adapter. My latest GPIO based I2C adapter driver for Blackfin system
> will use this stuff. And also IXP4XX GPIO based I2C driver can also be
> moved to this.
>
> Signed-off-by: Bryan Wu <bryan.wu@analog.com> 
> ---
>  include/linux/i2c.h |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> Index: include/linux/i2c.h
> ===================================================================
> --- include/linux/i2c.h	(revision 2813)
> +++ include/linux/i2c.h	(working copy)
> @@ -201,6 +201,26 @@ struct i2c_algorithm {
>  };
>  
>  /*
> + * Some chips do not have an I2C unit, so GPIO lines are just used to 
> + * Used as platform_data to provide GPIO pin information to this kind GPIO 
> + * based I2C driver.
> + */
> +struct i2c_bitbang_gpio {
> +	int sda;
> +	int scl;
> +};

Why would this be included in the generic i2c.h header file? As far as
I can see this structure only makes sense for bit-banged I2C busses, so
this structure should be declared in i2c-algo-bit.h.

Also, this structure alone isn't very useful. I'm waiting to see
drivers actually making use of it before I will consider merging this
patch at all.

> +
> +static inline int i2c_bitbang_gpio_sda(struct i2c_bitbang_gpio *gpio)
> +{
> +	return (gpio->sda);
> +}
> +
> +static inline int i2c_bitbang_gpio_scl(struct i2c_bitbang_gpio *gpio)
> +{
> +	return (gpio->scl);
> +}

What's the point of these?

-- 
Jean Delvare

Re: [PATCH] i2c-core: i2c bitbang gpio structure

[David Brownell]

On Friday 09 March 2007 8:55 am, Jean Delvare wrote:

> > +struct i2c_bitbang_gpio {
> > +	int sda;
> > +	int scl;
> > +};
> 
> ...
> 
> Also, this structure alone isn't very useful. I'm waiting to see
> drivers actually making use of it before I will consider merging this
> patch at all.

The notion would be that we could have one i2c bitbanger using
the CONFIG_GENERIC_GPIO <asm/gpio.h> interfaces that could work
on most platforms, using that struct for platform_data and the
usual convention for platform device naming.

I'd expect that struct would be merged as part of such a generic
GPIO bitbang driver, and would only be used by that one driver.

SPI could use such a generic bitbanger too.  Until 2.6.21 it's
been missing that last step:  it's needed platform-specific
GPIO calls, so the bitbangers were generic except for those
lowest-level hooks.

- Dave

[PATCH] Bitbanging i2c bus driver using the GPIO API

[Haavard Skinnemoen]

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.

To use, include something similar to the following in the
board-specific setup code:

  #include <linux/i2c-gpio.h>

  static struct i2c_gpio_platform_data i2c_gpio_data = {
  	.sda_pin	= GPIO_PIN_FOO,
	.scl_pin	= GPIO_PIN_BAR,
  };
  static struct platform_device i2c_gpio_device = {
  	.name		= "i2c-gpio",
	.id		= 0,
	.dev		= {
		.platform_data	= &i2c_gpio_data,
	},
  };

Register this platform_device, set up the i2c pins as GPIO if
required and you're ready to go.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
I wrote this driver for testing purposes a couple of weeks ago.
Figured I might as well post it since it looks like something like
this is needed.

This driver hasn't yet been updated for the latest change to the GPIO
API. I'll update the patch when the GPIO change makes it into
mainline.

Haavard

 drivers/i2c/busses/Kconfig    |    8 ++
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-gpio.c |  164 +++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-gpio.h      |   18 +++++
 include/linux/i2c-id.h        |    1 +
 5 files changed, 192 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fb19dbb..52f79d1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -102,6 +102,14 @@ config I2C_ELEKTOR
 	  This support is also available as a module.  If so, the module 
 	  will be called i2c-elektor.
 
+config I2C_GPIO
+	tristate "GPIO-based bitbanging i2c driver"
+	depends on I2C && GENERIC_GPIO
+	select I2C_ALGOBIT
+	help
+	  This is a very simple bitbanging i2c driver utilizing the
+	  arch-neutral GPIO API to control the SCL and SDA lines.
+
 config I2C_HYDRA
 	tristate "CHRP Apple Hydra Mac I/O I2C interface"
 	depends on I2C && PCI && PPC_CHRP && EXPERIMENTAL
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 290b540..68f2b05 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
+obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
 obj-$(CONFIG_I2C_I810)		+= i2c-i810.o
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
new file mode 100644
index 0000000..f5ed64e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -0,0 +1,164 @@
+/*
+ * Bitbanging i2c bus driver using the GPIO API
+ *
+ * Copyright (C) 2006 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
+#include <linux/i2c-gpio.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <asm/gpio.h>
+
+void i2c_gpio_setsda(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	if (state)
+		gpio_direction_input(pdata->sda_pin);
+	else
+		gpio_direction_output(pdata->sda_pin);
+}
+
+void i2c_gpio_setscl(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	if (state)
+		gpio_direction_input(pdata->scl_pin);
+	else
+		gpio_direction_output(pdata->scl_pin);
+}
+
+int i2c_gpio_getsda(void *data)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	return gpio_get_value(pdata->sda_pin);
+}
+
+int i2c_gpio_getscl(void *data)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	return gpio_get_value(pdata->scl_pin);
+}
+
+static int __init i2c_gpio_probe(struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata;
+	struct i2c_algo_bit_data *bit_data;
+	struct i2c_adapter *adap;
+	int ret;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata)
+		return -ENXIO;
+
+	ret = -ENOMEM;
+	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
+	if (!adap)
+		goto err_alloc_adap;
+	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
+	if (!bit_data)
+		goto err_alloc_bit_data;
+
+	ret = gpio_request(pdata->sda_pin, "sda");
+	if (ret)
+		goto err_request_sda;
+	ret = gpio_request(pdata->scl_pin, "scl");
+	if (ret)
+		goto err_request_scl;
+
+	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);
+
+	bit_data->setsda	= i2c_gpio_setsda,
+	bit_data->setscl	= i2c_gpio_setscl,
+	bit_data->getsda	= i2c_gpio_getsda,
+	bit_data->getscl	= i2c_gpio_getscl,
+	bit_data->udelay	= 5,			/* 100 kHz */
+	bit_data->timeout	= HZ / 10,		/* 100 ms */
+	bit_data->data		= pdata;
+
+	adap->owner = THIS_MODULE;
+	snprintf(adap->name, I2C_NAME_SIZE, "i2c-gpio%d", pdev->id);
+	adap->algo_data = bit_data;
+	adap->dev.parent = &pdev->dev;
+
+	ret = i2c_bit_add_bus(adap);
+	if (ret)
+		goto err_add_bus;
+
+	platform_set_drvdata(pdev, adap);
+
+	printk(KERN_INFO "i2c-gpio: using pins 0x%x (sda) 0x%x (scl)\n",
+	       pdata->sda_pin, pdata->scl_pin);
+
+	return 0;
+
+err_add_bus:
+	gpio_free(pdata->scl_pin);
+err_request_scl:
+	gpio_free(pdata->sda_pin);
+err_request_sda:
+	kfree(bit_data);
+err_alloc_bit_data:
+	kfree(adap);
+err_alloc_adap:
+	return ret;
+}
+
+static int __exit i2c_gpio_remove(struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata;
+	struct i2c_adapter *adap;
+
+	adap = platform_get_drvdata(pdev);
+	pdata = pdev->dev.platform_data;
+
+	i2c_del_adapter(adap);
+	gpio_free(pdata->scl_pin);
+	gpio_free(pdata->sda_pin);
+	kfree(adap);
+
+	return 0;
+}
+
+static struct platform_driver i2c_gpio_driver = {
+	.driver		= {
+		.name	= "i2c-gpio",
+		.owner	= THIS_MODULE,
+	},
+	.remove		= __exit_p(i2c_gpio_remove),
+};
+
+static int __init i2c_gpio_init(void)
+{
+	int ret;
+
+	ret = platform_driver_probe(&i2c_gpio_driver, i2c_gpio_probe);
+	if (ret)
+		printk("i2c-gpio: probe failed: %d\n", ret);
+
+	return ret;
+}
+module_init(i2c_gpio_init);
+
+static void __exit i2c_gpio_exit(void)
+{
+	platform_driver_unregister(&i2c_gpio_driver);
+}
+module_exit(i2c_gpio_exit);
+
+MODULE_AUTHOR("Haavard Skinnemoen <hskinnemoen@atmel.com>");
+MODULE_DESCRIPTION("Platform-independent bitbanging i2c driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c-gpio.h b/include/linux/i2c-gpio.h
new file mode 100644
index 0000000..57f0a0d
--- /dev/null
+++ b/include/linux/i2c-gpio.h
@@ -0,0 +1,18 @@
+/*
+ * i2c-gpio interface to platform code
+ *
+ * Copyright (C) 2007 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _LINUX_I2C_GPIO_H
+#define _LINUX_I2C_GPIO_H
+
+struct i2c_gpio_platform_data {
+	unsigned int sda_pin;
+	unsigned int scl_pin;
+};
+
+#endif /* _LINUX_I2C_GPIO_H */
diff --git a/include/linux/i2c-id.h b/include/linux/i2c-id.h
index 9c21dc7..34230a6 100644
--- 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 */
 
 /* --- PCF 8584 based algorithms					*/
 #define I2C_HW_P_LP		0x020000 /* Parallel port interface */
-- 
1.4.4.4

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

[David Brownell]

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

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

[Russell King]

On Fri, Mar 09, 2007 at 11:30:12AM -0800, 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.

That's a more correct I2C implementation.  If you read the specs, the
SDA and SCL signals are supposed to be driven by open-collector or
open-drain drivers, such that devices only pull the bus low.  Pull-up
resistors pull the signals high when undriven.

This avoids the possibility of damage caused when one device drives
a signal low and another device tries to drive it high.

Therefore, the correct I2C GPIO implementation is one where you drive
both SDA and SCL low by using a combination of the data direction
register and the output level register, but avoid driving the output
high.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

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

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

[David Brownell]

On Friday 09 March 2007 12:08 pm, Russell King wrote:
> On Fri, Mar 09, 2007 at 11:30:12AM -0800, 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.
> 
> That's a more correct I2C implementation.  If you read the specs, the
> SDA and SCL signals are supposed to be driven by open-collector or
> open-drain drivers, such that devices only pull the bus low.  Pull-up
> resistors pull the signals high when undriven.

Exactly as I had mentioned:

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

Where "multi-drive" == open-drain.  You're saying it should also be
used on SDA too ... OK, I was focussing on clock stretching but it
applies there too.

 
> This avoids the possibility of damage caused when one device drives
> a signal low and another device tries to drive it high.
> 
> Therefore, the correct I2C GPIO implementation is one where you drive
> both SDA and SCL low by using a combination of the data direction
> register and the output level register, but avoid driving the output
> high.

I see your point -- it does answer my question -- but you seem to have
overlooked a highly relevant one of mine.  :)

Regardless, those nuances should be captured in comments in that
driver:  that for GPIOs that don't support open drain output (only
push/pull drivers), that it's faked by otherwise-unnecessary
tweaking of GPIO direction.

- Dave

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

[David Brownell]

On Friday 09 March 2007 12:43 pm, Håvard Skinnemoen wrote:
> 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 ...
> 
> 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?

Not strictly; but folk do so, and the results are compatible-enough
for many purposes.  Certainly a quick glance at i2c-algo-bit tells
me that it knows how to cope with output-only SCL.  A generic GPIO
driver "should" be able to at least act sanely given such a pin.


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

Not necessarily ... reading the GPIO pad state works regardless of what
direction was configured on most chips (including AT91 and AVR32), but
not all of them.

Certainly multi-drive/open-drain outputs get the electical stuff right
without that.  Russell's explanation says the reason to switch direction
is to disable the non-open-drain output drivers.

There are several options lurking, that a generic I2C bitbanger "ought"
to handle.  Existence of open-drain outputs is one issue.  Ability to
change direction is another; as is ability to read the value on output
pads.  Your code assumed one set of answers; others are possible.


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

As I said in a different way above!  If open drain outputs are available,
and the actual values can be read on output pins, then I think both pins
can be configured as outputs and left that way.

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

Reading the code, I see it's automatic if you don't provide getscl()...

- Dave

[PATCH v2] Bitbanging i2c bus driver using the GPIO API

[Haavard Skinnemoen]

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.

To use, include something similar to the following in the
board-specific setup code:

  #include <linux/i2c-gpio.h>

  static struct i2c_gpio_platform_data i2c_gpio_data = {
	.sda_pin	= GPIO_PIN_FOO,
	.scl_pin	= GPIO_PIN_BAR,
  };
  static struct platform_device i2c_gpio_device = {
	.name		= "i2c-gpio",
	.id		= 0,
	.dev		= {
		.platform_data	= &i2c_gpio_data,
	},
  };

Register this platform_device, set up the i2c pins as GPIO if
required and you're ready to go.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
This patch is different from the first patch in the following ways:
  * Handles pins set up as open drain (aka multidrive) by toggling
    the output value instead of the direction
  * Handles output-only SCL pins the same way, and also does not
    install a getscl() callback for such pins
  * Does not add anything to include/linux/i2c-ids.h
  * Sets the output value explicitly after changing the direction to
    output.
  * Plugs a memory leak in remove() -- algo_data wasn't freed.
  * Prints out the pin IDs in decimal, with an extra note when clock
    stretching isn't supported

This version has been compile-tested only. I'll give it a spin when I
get back to work on monday.

Dave, does this address your concerns?

Haavard   

 drivers/i2c/busses/Kconfig    |    8 ++
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-gpio.c |  211 +++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-gpio.h      |   30 ++++++
 4 files changed, 250 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fb19dbb..52f79d1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -102,6 +102,14 @@ config I2C_ELEKTOR
 	  This support is also available as a module.  If so, the module 
 	  will be called i2c-elektor.
 
+config I2C_GPIO
+	tristate "GPIO-based bitbanging i2c driver"
+	depends on I2C && GENERIC_GPIO
+	select I2C_ALGOBIT
+	help
+	  This is a very simple bitbanging i2c driver utilizing the
+	  arch-neutral GPIO API to control the SCL and SDA lines.
+
 config I2C_HYDRA
 	tristate "CHRP Apple Hydra Mac I/O I2C interface"
 	depends on I2C && PCI && PPC_CHRP && EXPERIMENTAL
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 290b540..68f2b05 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
+obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
 obj-$(CONFIG_I2C_I810)		+= i2c-i810.o
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
new file mode 100644
index 0000000..423db0a
--- /dev/null
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -0,0 +1,211 @@
+/*
+ * Bitbanging i2c bus driver using the GPIO API
+ *
+ * Copyright (C) 2006 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
+#include <linux/i2c-gpio.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <asm/gpio.h>
+
+/* Toggle SDA by changing the direction of the pin */
+static void i2c_gpio_setsda_dir(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	if (state)
+		gpio_direction_input(pdata->sda_pin);
+	else {
+		gpio_direction_output(pdata->sda_pin);
+		gpio_set_value(pdata->sda_pin, 0);
+	}
+}
+
+/*
+ * Toggle SDA by changing the output value of the pin. This is only
+ * valid for pins configured as open drain (i.e. setting the value
+ * high effectively turns off the output driver.)
+ */
+static void i2c_gpio_setsda_val(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	gpio_set_value(pdata->sda_pin, state);
+}
+
+/* Toggle SCL by changing the direction of the pin. */
+static void i2c_gpio_setscl_dir(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	if (state)
+		gpio_direction_input(pdata->scl_pin);
+	else {
+		gpio_direction_output(pdata->scl_pin);
+		gpio_set_value(pdata->scl_pin, 0);
+	}
+}
+
+/*
+ * Toggle SCL by changing the output value of the pin. This is used
+ * for pins that are configured as open drain and for output-only
+ * pins. The latter case will break the i2c protocol, but it will
+ * often work in practice.
+ */
+static void i2c_gpio_setscl_val(void *data, int state)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	gpio_set_value(pdata->scl_pin, state);
+}
+
+int i2c_gpio_getsda(void *data)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	return gpio_get_value(pdata->sda_pin);
+}
+
+int i2c_gpio_getscl(void *data)
+{
+	struct i2c_gpio_platform_data *pdata = data;
+
+	return gpio_get_value(pdata->scl_pin);
+}
+
+static int __init i2c_gpio_probe(struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata;
+	struct i2c_algo_bit_data *bit_data;
+	struct i2c_adapter *adap;
+	int ret;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata)
+		return -ENXIO;
+
+	ret = -ENOMEM;
+	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
+	if (!adap)
+		goto err_alloc_adap;
+	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
+	if (!bit_data)
+		goto err_alloc_bit_data;
+
+	ret = gpio_request(pdata->sda_pin, "sda");
+	if (ret)
+		goto err_request_sda;
+	ret = gpio_request(pdata->scl_pin, "scl");
+	if (ret)
+		goto err_request_scl;
+
+	if (pdata->sda_is_open_drain) {
+		gpio_direction_output(pdata->sda_pin);
+		gpio_set_value(pdata->sda_pin, 1);
+		bit_data->setsda = i2c_gpio_setsda_val;
+	} else {
+		gpio_direction_input(pdata->sda_pin);
+		bit_data->setsda = i2c_gpio_setsda_dir;
+	}
+
+	if (pdata->scl_is_open_drain || pdata->scl_is_output_only) {
+		gpio_direction_output(pdata->scl_pin);
+		gpio_set_value(pdata->scl_pin, 1);
+		bit_data->setscl = i2c_gpio_setscl_val;
+	} else {
+		gpio_direction_input(pdata->scl_pin);
+		bit_data->setscl = i2c_gpio_setscl_dir;
+	}
+
+	if (!pdata->scl_is_output_only)
+		bit_data->getscl = i2c_gpio_getscl,
+
+	bit_data->getsda	= i2c_gpio_getsda,
+	bit_data->udelay	= 5,			/* 100 kHz */
+	bit_data->timeout	= HZ / 10,		/* 100 ms */
+	bit_data->data		= pdata;
+
+	adap->owner = THIS_MODULE;
+	snprintf(adap->name, I2C_NAME_SIZE, "i2c-gpio%d", pdev->id);
+	adap->algo_data = bit_data;
+	adap->dev.parent = &pdev->dev;
+
+	ret = i2c_bit_add_bus(adap);
+	if (ret)
+		goto err_add_bus;
+
+	platform_set_drvdata(pdev, adap);
+
+	dev_info(&pdev->dev, "using pins %u (sda) and %u (scl%s)\n",
+		 pdata->sda_pin, pdata->scl_pin,
+		 pdata->scl_is_output_only
+		 ? ", no clock stretching" : "");
+
+	return 0;
+
+err_add_bus:
+	gpio_free(pdata->scl_pin);
+err_request_scl:
+	gpio_free(pdata->sda_pin);
+err_request_sda:
+	kfree(bit_data);
+err_alloc_bit_data:
+	kfree(adap);
+err_alloc_adap:
+	return ret;
+}
+
+static int __exit i2c_gpio_remove(struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata;
+	struct i2c_adapter *adap;
+
+	adap = platform_get_drvdata(pdev);
+	pdata = pdev->dev.platform_data;
+
+	i2c_del_adapter(adap);
+	gpio_free(pdata->scl_pin);
+	gpio_free(pdata->sda_pin);
+	kfree(adap->algo_data);
+	kfree(adap);
+
+	return 0;
+}
+
+static struct platform_driver i2c_gpio_driver = {
+	.driver		= {
+		.name	= "i2c-gpio",
+		.owner	= THIS_MODULE,
+	},
+	.remove		= __exit_p(i2c_gpio_remove),
+};
+
+static int __init i2c_gpio_init(void)
+{
+	int ret;
+
+	ret = platform_driver_probe(&i2c_gpio_driver, i2c_gpio_probe);
+	if (ret)
+		printk("i2c-gpio: probe failed: %d\n", ret);
+
+	return ret;
+}
+module_init(i2c_gpio_init);
+
+static void __exit i2c_gpio_exit(void)
+{
+	platform_driver_unregister(&i2c_gpio_driver);
+}
+module_exit(i2c_gpio_exit);
+
+MODULE_AUTHOR("Haavard Skinnemoen <hskinnemoen@atmel.com>");
+MODULE_DESCRIPTION("Platform-independent bitbanging i2c driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c-gpio.h b/include/linux/i2c-gpio.h
new file mode 100644
index 0000000..1e8fa29
--- /dev/null
+++ b/include/linux/i2c-gpio.h
@@ -0,0 +1,30 @@
+/*
+ * i2c-gpio interface to platform code
+ *
+ * Copyright (C) 2007 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _LINUX_I2C_GPIO_H
+#define _LINUX_I2C_GPIO_H
+
+/**
+ * struct i2c_gpio_platform_data - Platform-dependent data for i2c-gpio
+ * @sda_pin: GPIO pin ID to use for SDA
+ * @scl_pin: GPIO pin ID to use for SCL
+ * @sda_is_open_drain: SDA is configured as open drain, i.e. the pin
+ *	isn't actively driven high when setting the output value high.
+ * @scl_is_open_drain: SCL is set up as open drain.
+ * @scl_is_output_only: SCL output drivers cannot be turned off.
+ */
+struct i2c_gpio_platform_data {
+	unsigned int sda_pin;
+	unsigned int scl_pin;
+	unsigned int sda_is_open_drain:1;
+	unsigned int scl_is_open_drain:1;
+	unsigned int scl_is_output_only:1;
+};
+
+#endif /* _LINUX_I2C_GPIO_H */
-- 
1.4.4.4

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

[Jean Delvare]

On Fri, 9 Mar 2007 11:30:12 -0800, David Brownell wrote:
> > --- 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!

I'm confident that we'll be able to get rid of the I2C device driver
IDs pretty soon thanks to your new i2c-core model, but I'm not sure if
we'll be able to get rid of I2C adapter drivers IDs that quickly. But I
agree with you, please let's not add new unused IDs.

-- 
Jean Delvare

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

[Jean Delvare]

Hi Haavard,

On Sat, 10 Mar 2007 14:13:28 +0100, 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.
> 
> To use, include something similar to the following in the
> board-specific setup code:
> 
>   #include <linux/i2c-gpio.h>
> 
>   static struct i2c_gpio_platform_data i2c_gpio_data = {
> 	.sda_pin	= GPIO_PIN_FOO,
> 	.scl_pin	= GPIO_PIN_BAR,
>   };
>   static struct platform_device i2c_gpio_device = {
> 	.name		= "i2c-gpio",
> 	.id		= 0,
> 	.dev		= {
> 		.platform_data	= &i2c_gpio_data,
> 	},
>   };
> 
> Register this platform_device, set up the i2c pins as GPIO if
> required and you're ready to go.

I like the idea very much. Would this let us get rid of i2c-ixp2000?
i2c-ixp4xx? scx200_i2c? Other drivers?

>  drivers/i2c/busses/Kconfig    |    8 ++
>  drivers/i2c/busses/Makefile   |    1 +
>  drivers/i2c/busses/i2c-gpio.c |  211 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-gpio.h      |   30 ++++++
>  4 files changed, 250 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fb19dbb..52f79d1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -102,6 +102,14 @@ config I2C_ELEKTOR
>  	  This support is also available as a module.  If so, the module 
>  	  will be called i2c-elektor.
>  
> +config I2C_GPIO
> +	tristate "GPIO-based bitbanging i2c driver"
> +	depends on I2C && GENERIC_GPIO
> +	select I2C_ALGOBIT
> +	help
> +	  This is a very simple bitbanging i2c driver utilizing the
> +	  arch-neutral GPIO API to control the SCL and SDA lines.
> +
>  config I2C_HYDRA
>  	tristate "CHRP Apple Hydra Mac I/O I2C interface"
>  	depends on I2C && PCI && PPC_CHRP && EXPERIMENTAL
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 290b540..68f2b05 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
>  obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
>  obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
>  obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
> +obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
>  obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
>  obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
>  obj-$(CONFIG_I2C_I810)		+= i2c-i810.o
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> new file mode 100644
> index 0000000..423db0a
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -0,0 +1,211 @@
> +/*
> + * Bitbanging i2c bus driver using the GPIO API
> + *
> + * Copyright (C) 2006 Atmel Corporation

I'm told we're in year 2007 ;)

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-bit.h>
> +#include <linux/i2c-gpio.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/gpio.h>
> +
> +/* Toggle SDA by changing the direction of the pin */
> +static void i2c_gpio_setsda_dir(void *data, int state)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	if (state)
> +		gpio_direction_input(pdata->sda_pin);
> +	else {
> +		gpio_direction_output(pdata->sda_pin);
> +		gpio_set_value(pdata->sda_pin, 0);
> +	}
> +}
> +
> +/*
> + * Toggle SDA by changing the output value of the pin. This is only
> + * valid for pins configured as open drain (i.e. setting the value
> + * high effectively turns off the output driver.)
> + */
> +static void i2c_gpio_setsda_val(void *data, int state)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	gpio_set_value(pdata->sda_pin, state);
> +}
> +
> +/* Toggle SCL by changing the direction of the pin. */
> +static void i2c_gpio_setscl_dir(void *data, int state)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	if (state)
> +		gpio_direction_input(pdata->scl_pin);
> +	else {
> +		gpio_direction_output(pdata->scl_pin);
> +		gpio_set_value(pdata->scl_pin, 0);
> +	}
> +}
> +
> +/*
> + * Toggle SCL by changing the output value of the pin. This is used
> + * for pins that are configured as open drain and for output-only
> + * pins. The latter case will break the i2c protocol, but it will
> + * often work in practice.
> + */
> +static void i2c_gpio_setscl_val(void *data, int state)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	gpio_set_value(pdata->scl_pin, state);
> +}
> +
> +int i2c_gpio_getsda(void *data)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	return gpio_get_value(pdata->sda_pin);
> +}


What value will you get if the SDA pin is open-drain and currently in
output mode? Are such GPIO pins actually able to detect that the pin is
low while they are not themselves driving it low?

> +
> +int i2c_gpio_getscl(void *data)
> +{
> +	struct i2c_gpio_platform_data *pdata = data;
> +
> +	return gpio_get_value(pdata->scl_pin);
> +}
> +
> +static int __init i2c_gpio_probe(struct platform_device *pdev)
> +{
> +	struct i2c_gpio_platform_data *pdata;
> +	struct i2c_algo_bit_data *bit_data;
> +	struct i2c_adapter *adap;
> +	int ret;
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata)
> +		return -ENXIO;
> +
> +	ret = -ENOMEM;
> +	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> +	if (!adap)
> +		goto err_alloc_adap;
> +	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
> +	if (!bit_data)
> +		goto err_alloc_bit_data;
> +
> +	ret = gpio_request(pdata->sda_pin, "sda");
> +	if (ret)
> +		goto err_request_sda;
> +	ret = gpio_request(pdata->scl_pin, "scl");
> +	if (ret)
> +		goto err_request_scl;
> +
> +	if (pdata->sda_is_open_drain) {
> +		gpio_direction_output(pdata->sda_pin);
> +		gpio_set_value(pdata->sda_pin, 1);
> +		bit_data->setsda = i2c_gpio_setsda_val;
> +	} else {
> +		gpio_direction_input(pdata->sda_pin);
> +		bit_data->setsda = i2c_gpio_setsda_dir;
> +	}
> +
> +	if (pdata->scl_is_open_drain || pdata->scl_is_output_only) {
> +		gpio_direction_output(pdata->scl_pin);
> +		gpio_set_value(pdata->scl_pin, 1);
> +		bit_data->setscl = i2c_gpio_setscl_val;
> +	} else {
> +		gpio_direction_input(pdata->scl_pin);
> +		bit_data->setscl = i2c_gpio_setscl_dir;
> +	}
> +
> +	if (!pdata->scl_is_output_only)
> +		bit_data->getscl = i2c_gpio_getscl,
> +
> +	bit_data->getsda	= i2c_gpio_getsda,
> +	bit_data->udelay	= 5,			/* 100 kHz */

Actually, no, i2c-algo-bit has a 1/3-2/3 duty cycle, so a complete
cycle is 3 times the udelay value. So udelay=5 gives you 66 kHz. If
someone wants to fix that...

Also, I wouldn't recommend such a low value when SCL cannot be sensed,
if a slave stretches the line even very briefly, you won't notice and
havoc will ensue. udelay=50 sounds more reasonable for such half-baked
busses.

> +	bit_data->timeout	= HZ / 10,		/* 100 ms */
> +	bit_data->data		= pdata;
> +
> +	adap->owner = THIS_MODULE;
> +	snprintf(adap->name, I2C_NAME_SIZE, "i2c-gpio%d", pdev->id);
> +	adap->algo_data = bit_data;
> +	adap->dev.parent = &pdev->dev;
> +
> +	ret = i2c_bit_add_bus(adap);
> +	if (ret)
> +		goto err_add_bus;
> +
> +	platform_set_drvdata(pdev, adap);
> +
> +	dev_info(&pdev->dev, "using pins %u (sda) and %u (scl%s)\n",
> +		 pdata->sda_pin, pdata->scl_pin,
> +		 pdata->scl_is_output_only
> +		 ? ", no clock stretching" : "");
> +
> +	return 0;
> +
> +err_add_bus:
> +	gpio_free(pdata->scl_pin);
> +err_request_scl:
> +	gpio_free(pdata->sda_pin);
> +err_request_sda:
> +	kfree(bit_data);
> +err_alloc_bit_data:
> +	kfree(adap);
> +err_alloc_adap:
> +	return ret;
> +}
> +
> +static int __exit i2c_gpio_remove(struct platform_device *pdev)
> +{
> +	struct i2c_gpio_platform_data *pdata;
> +	struct i2c_adapter *adap;
> +
> +	adap = platform_get_drvdata(pdev);
> +	pdata = pdev->dev.platform_data;
> +
> +	i2c_del_adapter(adap);
> +	gpio_free(pdata->scl_pin);
> +	gpio_free(pdata->sda_pin);
> +	kfree(adap->algo_data);
> +	kfree(adap);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver i2c_gpio_driver = {
> +	.driver		= {
> +		.name	= "i2c-gpio",
> +		.owner	= THIS_MODULE,
> +	},
> +	.remove		= __exit_p(i2c_gpio_remove),
> +};
> +
> +static int __init i2c_gpio_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_probe(&i2c_gpio_driver, i2c_gpio_probe);
> +	if (ret)
> +		printk("i2c-gpio: probe failed: %d\n", ret);

Add KERN_ERR or similar.

> +
> +	return ret;
> +}
> +module_init(i2c_gpio_init);
> +
> +static void __exit i2c_gpio_exit(void)
> +{
> +	platform_driver_unregister(&i2c_gpio_driver);
> +}
> +module_exit(i2c_gpio_exit);
> +
> +MODULE_AUTHOR("Haavard Skinnemoen <hskinnemoen@atmel.com>");
> +MODULE_DESCRIPTION("Platform-independent bitbanging i2c driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/i2c-gpio.h b/include/linux/i2c-gpio.h
> new file mode 100644
> index 0000000..1e8fa29
> --- /dev/null
> +++ b/include/linux/i2c-gpio.h
> @@ -0,0 +1,30 @@
> +/*
> + * i2c-gpio interface to platform code
> + *
> + * Copyright (C) 2007 Atmel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_I2C_GPIO_H
> +#define _LINUX_I2C_GPIO_H
> +
> +/**
> + * struct i2c_gpio_platform_data - Platform-dependent data for i2c-gpio
> + * @sda_pin: GPIO pin ID to use for SDA
> + * @scl_pin: GPIO pin ID to use for SCL
> + * @sda_is_open_drain: SDA is configured as open drain, i.e. the pin
> + *	isn't actively driven high when setting the output value high.
> + * @scl_is_open_drain: SCL is set up as open drain.
> + * @scl_is_output_only: SCL output drivers cannot be turned off.
> + */
> +struct i2c_gpio_platform_data {
> +	unsigned int sda_pin;
> +	unsigned int scl_pin;
> +	unsigned int sda_is_open_drain:1;
> +	unsigned int scl_is_open_drain:1;
> +	unsigned int scl_is_output_only:1;
> +};
> +
> +#endif /* _LINUX_I2C_GPIO_H */

Would you mind also adding yourself to MAINTAINERS for this driver? I
would appreciate it.

Thanks,
-- 
Jean Delvare

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

[David Brownell]

On Saturday 10 March 2007 5:13 am, Haavard Skinnemoen wrote:
> This is a very simple bitbanging i2c bus driver utilizing the new
> arch-neutral GPIO API. ...
> ---
> This patch is different from the first patch in the following ways:
>   * Handles pins set up as open drain (aka multidrive) by toggling
>     the output value instead of the direction
>   * Handles output-only SCL pins the same way, and also does not
>     install a getscl() callback for such pins
>   * Does not add anything to include/linux/i2c-ids.h
>   * Sets the output value explicitly after changing the direction to
>     output.
>   * Plugs a memory leak in remove() -- algo_data wasn't freed.
>   * Prints out the pin IDs in decimal, with an extra note when clock
>     stretching isn't supported
> 
> This version has been compile-tested only. I'll give it a spin when I
> get back to work on monday.
> 
> Dave, does this address your concerns?

Yes, though see my followup to Jean's note.  Unless I make time
to test this out on some system, the issues seem to be:

 (a) will need to change once gpio_direction_output() gains
     that second argument;

 (b) i2c-gpio.h could stand one minor comment addition to highlight
     an assumption.

Looking good!

- Dave

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

[David Brownell]

On Saturday 10 March 2007 12:15 pm, Jean Delvare wrote:
> Hi Haavard,
> 
> On Sat, 10 Mar 2007 14:13:28 +0100, 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.

This updated version looks a lot better.  However it doesn't address
the API change -- gpio_direction_output(gpio, initial_value) -- which
is understandable since that patch hasn't yet merged.


> I like the idea very much. Would this let us get rid of i2c-ixp2000?
> i2c-ixp4xx? scx200_i2c? Other drivers?

There's CONFIG_GENERIC_GPIO support for ixp4xx (nyet upstream, ISTR it's
waiting on the gpio_direction_output update), so that one should be
particularly easy to replace.  Presumably some other bitbang drivers
could vanish before long too.


> What value will you get if the SDA pin is open-drain and currently in
> output mode?

For output GPIOs, gpio_get_value() is specified to either return the
actual value at the pin ... or zero, if the hardware can't do that.
Most GPIO pins *can* do that.  (Specifically, that's how AT91 GPIOs
work, open drain or otherwise.)

(However, there can be various latencies involved.  On one chip
when I wrote the output value, then immediately read it back, I
got the old value.  Reason:  the GPIO controller clock needed
to tick first in order to latch the new input value!  It was only
about 30 MHz, so the back-to-back instructions were too fast.  You
can also sometimes notice capacitance causing similar delays.
Of course those latencies apply regardless of pin direction.)

I think Haavard is assuming the GPIO actually returns that value,
since otherwise there'd be no point in trying to use the open drain
mode.  It'd be worth capturing that in the i2c-gpio.h definition
for that struct.


> Are such GPIO pins actually able to detect that the pin is 
> low while they are not themselves driving it low?

Given a "yes" to the above, then clearly "yes" here too.  As
I noted, if it can't actually sense the value at the pin, that
function should always return zero.

- Dave

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

[Wu, Bryan]

On Sat, 2007-03-10 at 14:13 +0100, 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.
> 

Sorry for missing this hot discussion. Your idea is exactly what I want.
So many arch specific GPIO based I2C adapter implementation will benefit
from this.

> To use, include something similar to the following in the
> board-specific setup code:
> 
>   #include <linux/i2c-gpio.h>
> 
>   static struct i2c_gpio_platform_data i2c_gpio_data = {
> 	.sda_pin	= GPIO_PIN_FOO,
> 	.scl_pin	= GPIO_PIN_BAR,
>   };

Is this usage right, because 3 flags are added to this structure as
below:

struct i2c_gpio_platform_data {
	unsigned int sda_pin;
	unsigned int scl_pin;
	unsigned int sda_is_open_drain:1;
	unsigned int scl_is_open_drain:1;
	unsigned int scl_is_output_only:1;
};

>   static struct platform_device i2c_gpio_device = {
> 	.name		= "i2c-gpio",
> 	.id		= 0,
> 	.dev		= {
> 		.platform_data	= &i2c_gpio_data,
> 	},
>   };
> 
> Register this platform_device, set up the i2c pins as GPIO if
> required and you're ready to go.
> 
> Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
> ---
> This patch is different from the first patch in the following ways:
>   * Handles pins set up as open drain (aka multidrive) by toggling
>     the output value instead of the direction
>   * Handles output-only SCL pins the same way, and also does not
>     install a getscl() callback for such pins
>   * Does not add anything to include/linux/i2c-ids.h
>   * Sets the output value explicitly after changing the direction to
>     output.
>   * Plugs a memory leak in remove() -- algo_data wasn't freed.
>   * Prints out the pin IDs in decimal, with an extra note when clock
>     stretching isn't supported
> 
> This version has been compile-tested only. I'll give it a spin when I
> get back to work on monday.
> 
> Dave, does this address your concerns?
> 
> Haavard   

Thanks a lot,  I will drop our GPIO based I2C driver and try this one on
our platform.

> +	if (!pdata->scl_is_output_only)
> +		bit_data->getscl = i2c_gpio_getscl,
> +
> +	bit_data->getsda	= i2c_gpio_getsda,
> +	bit_data->udelay	= 5,			/* 100 kHz */
> +	bit_data->timeout	= HZ / 10,		/* 100 ms */

Can we add these udelay/timeout to struct i2c_gpio_platform_data? And
let customer to choose these according their specific requirement. We
use Kconfig to do this, but Jean and David don't like the idea, -:(

Regards,
-Bryan Wu

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

[Haavard Skinnemoen]

On Sat, 10 Mar 2007 21:15:50 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> I like the idea very much. Would this let us get rid of i2c-ixp2000?
> i2c-ixp4xx? scx200_i2c? Other drivers?

Any platform that implements the generic gpio api should be able to use
this driver. So yes, I hope we might be able to get rid of a few
existing bitbanging drivers.

> > +/*
> > + * Bitbanging i2c bus driver using the GPIO API
> > + *
> > + * Copyright (C) 2006 Atmel Corporation
> 
> I'm told we're in year 2007 ;)

I'm also told that copyright protection lasts infinitely long in
practice ;)

I'll update it. I probably just copied it blindly from a different
driver.

> > +int i2c_gpio_getsda(void *data)
> > +{
> > +	struct i2c_gpio_platform_data *pdata = data;
> > +
> > +	return gpio_get_value(pdata->sda_pin);
> > +}
> 
> 
> What value will you get if the SDA pin is open-drain and currently in
> output mode? Are such GPIO pins actually able to detect that the pin is
> low while they are not themselves driving it low?

I guess that depends on the GPIO controller. But being able to read the
pin state even when the pin is configured as an output is a
prerequisite for using this driver with "open drain" pins, so if the
hardware doesn't support this, the board code should just set
{sda,scl}_is_opendrain to zero.

> > +	bit_data->udelay	= 5,			/* 100 kHz */
> 
> Actually, no, i2c-algo-bit has a 1/3-2/3 duty cycle, so a complete
> cycle is 3 times the udelay value. So udelay=5 gives you 66 kHz. If
> someone wants to fix that...

Ok. I guess we should move this parameter into the platform data struct
anyway.

> Also, I wouldn't recommend such a low value when SCL cannot be sensed,
> if a slave stretches the line even very briefly, you won't notice and
> havoc will ensue. udelay=50 sounds more reasonable for such half-baked
> busses.

Makes sense.

> > +	ret = platform_driver_probe(&i2c_gpio_driver, i2c_gpio_probe);
> > +	if (ret)
> > +		printk("i2c-gpio: probe failed: %d\n", ret);
> 
> Add KERN_ERR or similar.

Will do.

> Would you mind also adding yourself to MAINTAINERS for this driver? I
> would appreciate it.

Sure. I'm hoping this driver won't cause that much maintenance overhead
anyway since all the complicated stuff is in i2c-algo-bit. But I agree
it needs a maintainer.

Haavard

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

[Haavard Skinnemoen]

On Mon, 12 Mar 2007 18:07:59 +0800
"Wu, Bryan" <bryan.wu@analog.com> wrote:
> >   static struct i2c_gpio_platform_data i2c_gpio_data = {
> > 	.sda_pin	= GPIO_PIN_FOO,
> > 	.scl_pin	= GPIO_PIN_BAR,
> >   };
> 
> Is this usage right, because 3 flags are added to this structure as
> below:
> 
> struct i2c_gpio_platform_data {
> 	unsigned int sda_pin;
> 	unsigned int scl_pin;
> 	unsigned int sda_is_open_drain:1;
> 	unsigned int scl_is_open_drain:1;
> 	unsigned int scl_is_output_only:1;
> };

Well, it is the simplest possible example. The last 3 fields will be 0,
which is a valid configuration.

> Thanks a lot,  I will drop our GPIO based I2C driver and try this one on
> our platform.

I hope it works for you.

> > +	if (!pdata->scl_is_output_only)
> > +		bit_data->getscl = i2c_gpio_getscl,
> > +
> > +	bit_data->getsda	= i2c_gpio_getsda,
> > +	bit_data->udelay	= 5,			/* 100 kHz */
> > +	bit_data->timeout	= HZ / 10,		/* 100 ms */
> 
> Can we add these udelay/timeout to struct i2c_gpio_platform_data? And
> let customer to choose these according their specific requirement. We
> use Kconfig to do this, but Jean and David don't like the idea, -:(

Yeah, they need to be a bit more configurable than they currently are.
And I think it makes sense to pass them from the board setup code, since
this is where things depending on board-specific details (signal quality
issues, pullup resistor values, etc.) are supposed to go.

Haavard

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

[Haavard Skinnemoen]

On Mon, 12 Mar 2007 15:34:57 +0100
Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:

> > > +	bit_data->udelay	= 5,			/* 100 kHz */
> > > +	bit_data->timeout	= HZ / 10,		/* 100 ms */  
> > 
> > Can we add these udelay/timeout to struct i2c_gpio_platform_data? And
> > let customer to choose these according their specific requirement. We
> > use Kconfig to do this, but Jean and David don't like the idea, -:(  
> 
> Yeah, they need to be a bit more configurable than they currently are.
> And I think it makes sense to pass them from the board setup code, since
> this is where things depending on board-specific details (signal quality
> issues, pullup resistor values, etc.) are supposed to go.

By the way, timeout seems to be hardcoded to 100 jiffies in the
i2c-algo-bit driver, so there's probably not much point passing it from
the board code when it's going to be overridden anyway. I'll add just a
udelay parameter to the platform struct for  now.

Haavard

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

[Jean Delvare]

Hi Haavard,

On Mon, 12 Mar 2007 15:53:59 +0100, Haavard Skinnemoen wrote:
> On Mon, 12 Mar 2007 15:34:57 +0100
> Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> 
> > > > +	bit_data->udelay	= 5,			/* 100 kHz */
> > > > +	bit_data->timeout	= HZ / 10,		/* 100 ms */  
> > > 
> > > Can we add these udelay/timeout to struct i2c_gpio_platform_data? And
> > > let customer to choose these according their specific requirement. We
> > > use Kconfig to do this, but Jean and David don't like the idea, -:(  
> > 
> > Yeah, they need to be a bit more configurable than they currently are.
> > And I think it makes sense to pass them from the board setup code, since
> > this is where things depending on board-specific details (signal quality
> > issues, pullup resistor values, etc.) are supposed to go.
> 
> By the way, timeout seems to be hardcoded to 100 jiffies in the
> i2c-algo-bit driver, so there's probably not much point passing it from
> the board code when it's going to be overridden anyway. I'll add just a
> udelay parameter to the platform struct for  now.

No, it's not hardcoded. I know it looks confusing. struct i2c_adapter
has a timeout field, that's the one being set to 100 in i2c-algo-bit,
but i2c-algo-bit uses the i2c_algo_bit_data timeout field. The
i2c_adapter timeout field is unused.

This is clearly calling for a cleanup but I don't have time for this
right now.

-- 
Jean Delvare

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

[Haavard Skinnemoen]

On Mon, 12 Mar 2007 16:11:09 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> > By the way, timeout seems to be hardcoded to 100 jiffies in the
> > i2c-algo-bit driver, so there's probably not much point passing it from
> > the board code when it's going to be overridden anyway. I'll add just a
> > udelay parameter to the platform struct for  now.  
> 
> No, it's not hardcoded. I know it looks confusing. struct i2c_adapter
> has a timeout field, that's the one being set to 100 in i2c-algo-bit,
> but i2c-algo-bit uses the i2c_algo_bit_data timeout field. The
> i2c_adapter timeout field is unused.

Ah, I see. Now that you mention it, I seem to recall I came to that
conclusion last time I looked at the code, but I've apparently forgotten
it since then ;)

I'll add a timeout field to the platform struct as well, then.

Haavard