Skip to contentSkip to conversation

i2c-core: i2c bitbang gpio structure

PATCH In review Wu, Bryan 5 participants 20 messages Mar 9, 2007 – Mar 12, 2007

Subsystems (2)
lore
Share & tools
lore

Wu, Bryan

author
#0 lore raw

PATCH 1/1 i2c-core: i2c bitbang gpio structure

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.

---
 include/linux/i2c.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
linux/i2c.h +20 -0
Index: include/linux/i2c.h
===================================================================
@@ -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

#1 lore raw

Hi Bryan,

Quoted 18 lines · 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.

---
 include/linux/i2c.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: include/linux/i2c.h
===================================================================

linux/i2c.h +0 -0
@@ -201,6 +201,26 @@ struct i2c_algorithm {
 };

linux/i2c.h struct i2c_algorithm { · line 211

Quoted 10 lines
linux/i2c.h +8 -0
 
 /*
+ * 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.

linux/i2c.h struct i2c_algorithm { · line 221

Quoted 10 lines
linux/i2c.h +10 -0
+
+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?

Signature · 1 line
Jean Delvare
#2 lore raw
Quoted 10 lines · Jean Delvare

linux/i2c.h struct i2c_algorithm { · line 211

Quoted 4 lines
linux/i2c.h +4 -0
+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

#3 lore raw

PATCH 1/1 Bitbanging i2c bus driver using the GPIO API

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 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(-)
drivers/i2c/busses/Kconfig +8 -0
index fb19dbb..52f79d1 100644
@@ -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
drivers/i2c/busses/Makefile +1 -0
index 290b540..68f2b05 100644
@@ -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
drivers/i2c/busses/i2c-gpio.c new file +164 -0

164 lines not on this page · raw message

include/linux/i2c-gpio.h new file +18 -0
new file mode 100644
index 0000000..57f0a0d
@@ -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 */
include/linux/i2c-id.h +1 -0
index 9c21dc7..34230a6 100644
@@ -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 */
Signature · 1 line
1.4.4.4
#4 lore raw

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

Quoted 3 lines · 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.

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.

drivers/i2c/busses/i2c-gpio.c line 82

Quoted 4 lines
drivers/i2c/busses/i2c-gpio.c new file +4 -0
+	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. ;)

drivers/i2c/busses/i2c-gpio.c line 104

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c new file +2 -0
+	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.

include/linux/i2c-id.h line 197

Quoted 7 lines
include/linux/i2c-id.h +1 -0
@@ -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

#5 lore raw

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

Quoted 5 lines · David Brownell
Quoted 3 lines · 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.

Quoted 7 lines

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.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
#6 lore raw

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

Quoted 14 lines · Russell King
Quoted 5 lines · David Brownell
Quoted 3 lines · 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.

Quoted 7 lines

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.

Quoted 4 lines

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:

Quoted 3 lines
Quoted 3 lines

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.

Quoted 7 lines

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

#7 lore raw

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

In reply to David Brownell
Quoted 5 lines · David Brownell
Quoted 3 lines · 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.

Quoted 7 lines

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?

Quoted 3 lines

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?

Quoted 7 lines

drivers/i2c/busses/i2c-gpio.c line 82

Quoted 4 lines
drivers/i2c/busses/i2c-gpio.c new file +4 -0
+	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 ;-)

Quoted 5 lines

drivers/i2c/busses/i2c-gpio.c line 104

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c new file +2 -0
+	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?

Quoted 11 lines
Quoted 8 lines
include/linux/i2c-id.h +0 -0
@@ -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

#8 lore raw

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

Quoted 9 lines · Håvard Skinnemoen
Quoted 7 lines · David Brownell
Quoted 3 lines · 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.

That's the right idea! But remember that not all GPIOs support reading back the actual value on SCL ...

Quoted 6 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

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

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.

Quoted 8 lines
Quoted 5 lines

drivers/i2c/busses/i2c-gpio.c line 104

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c new file +2 -0
+	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

#9 lore raw

PATCH v2 1/1 Bitbanging i2c bus driver using the GPIO API

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.

---
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(-)
drivers/i2c/busses/Kconfig +8 -0
index fb19dbb..52f79d1 100644
@@ -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
drivers/i2c/busses/Makefile +1 -0
index 290b540..68f2b05 100644
@@ -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
drivers/i2c/busses/i2c-gpio.c new file +211 -0

211 lines not on this page · raw message

include/linux/i2c-gpio.h new file +30 -0
new file mode 100644
index 0000000..1e8fa29
@@ -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 */
Signature · 1 line
1.4.4.4
#10 lore raw

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

Hi Haavard,

Quoted 19 lines · 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,
	},
Quoted 4 lines

};

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?

Quoted 43 lines
 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(-)
drivers/i2c/busses/Kconfig +8 -0
index fb19dbb..52f79d1 100644
@@ -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
drivers/i2c/busses/Makefile +1 -0
index 290b540..68f2b05 100644
@@ -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
drivers/i2c/busses/i2c-gpio.c new file +0 -0
new file mode 100644
index 0000000..423db0a

drivers/i2c/busses/i2c-gpio.c line 4

Quoted 4 lines
drivers/i2c/busses/i2c-gpio.c new file +4 -0
@@ -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 ;)

Quoted 61 lines
drivers/i2c/busses/i2c-gpio.c new file +61 -0
+ *
+ * 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;

drivers/i2c/busses/i2c-gpio.c line 75

Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c new file +10 -0
+
+	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?

Quoted 47 lines
drivers/i2c/busses/i2c-gpio.c new file +47 -0
+
+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;

drivers/i2c/busses/i2c-gpio.c line 132

Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c new file +10 -0
+	} 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.

Quoted 55 lines
drivers/i2c/busses/i2c-gpio.c new file +55 -0
+	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,
+	},

drivers/i2c/busses/i2c-gpio.c line 197

Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c new file +10 -0
+	.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.

Quoted 40 lines
drivers/i2c/busses/i2c-gpio.c new file +14 -0
+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Platform-independent bitbanging i2c driver");
+MODULE_LICENSE("GPL");
include/linux/i2c-gpio.h new file +20 -0
new file mode 100644
index 0000000..1e8fa29
@@ -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.

include/linux/i2c-gpio.h line 30

Quoted 10 lines
include/linux/i2c-gpio.h new file +10 -0
+ */
+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,

Signature · 1 line
Jean Delvare
#11 lore raw

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

Quoted 6 lines · Jean Delvare

Hi Haavard,

Quoted 3 lines · 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.

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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

#12 lore raw

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

In reply to Jean Delvare
Quoted 2 lines · On Sat, 10 Mar 2007 21:15:50 +0100 Jean Delvare <[email protected]> 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.

Quoted 6 lines
Quoted 4 lines

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

Quoted 11 lines

drivers/i2c/busses/i2c-gpio.c line 75

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c new file +6 -0
+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.

Quoted 5 lines

drivers/i2c/busses/i2c-gpio.c line 132

Quoted 1 line
drivers/i2c/busses/i2c-gpio.c new file +1 -0
+	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.

Quoted 4 lines

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.

Quoted 5 lines

drivers/i2c/busses/i2c-gpio.c line 197

Quoted 3 lines
drivers/i2c/busses/i2c-gpio.c new file +3 -0
+	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.

Quoted 2 lines

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

#13 lore raw

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

In reply to Haavard Skinnemoen
Quoted 15 lines · Haavard Skinnemoen

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

Quoted 4 lines

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

author
#14 lore raw

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

In reply to Haavard Skinnemoen
Quoted 4 lines · 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.

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.

Quoted 9 lines

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;
};
Quoted 25 lines
  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.

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

Quoted 6 lines

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.

drivers/i2c/busses/i2c-gpio.c line 133

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c new file +6 -0
+	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

#15 lore raw

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

Quoted 8 lines · Bryan
Quoted 4 lines
  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:

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

Quoted 2 lines

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

I hope it works for you.

Quoted 10 lines

drivers/i2c/busses/i2c-gpio.c line 133

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c new file +6 -0
+	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

#16 lore raw

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

Quoted 11 lines · On Mon, 12 Mar 2007 15:34:57 +0100 Haavard Skinnemoen <[email protected]>…
Quoted 6 lines

drivers/i2c/busses/i2c-gpio.c line 133

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c new file +2 -0
+	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

#17 lore raw

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

Hi Haavard,

Quoted 15 lines · Haavard Skinnemoen
Quoted 11 lines · On Mon, 12 Mar 2007 15:34:57 +0100 Haavard Skinnemoen <[email protected]>…
Quoted 6 lines

drivers/i2c/busses/i2c-gpio.c line 133

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c new file +2 -0
+	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.

Quoted 4 lines

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.

Signature · 1 line
Jean Delvare
#18 lore raw

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

Quoted 9 lines · On Mon, 12 Mar 2007 16:11:09 +0100 Jean Delvare <[email protected]> wrote:
Quoted 4 lines

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

#19 lore raw

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

In reply to David Brownell
Quoted 10 lines · David Brownell

include/linux/i2c-id.h line 197

Quoted 7 lines
include/linux/i2c-id.h +1 -0
@@ -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.

Signature · 1 line
Jean Delvare