Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
On Sat, 5 Jan 2008 11:40:55 -0800, David Brownell wrote: > From: eric miao <[EMAIL PROTECTED]> > Subject: [PATCH] gpiolib: support PCA9539 GPIO expander > > This adds a new-style I2C driver with basic support for the sixteen > bit PCA9539 GPIO expanders. These chips have multiple registers, > push-pull output drivers, and (not supported in this patch) pin > change interrupts. > > Board-specific code must provide "pca9539_platform_data" with each > chip's "i2c_board_info". That provides the GPIO numbers to be used > by that chip, and callbacks for board-specific setup/teardown logic. > > Derived from drivers/i2c/chips/pca9539.c (which has no current known > users). This is faster and simpler; it uses 16-bit register access, > and cache the OUTPUT and DIRECTION registers for fast access > > Signed-off-by: eric miao <[EMAIL PROTECTED]> > Signed-off-by: David Brownell <[EMAIL PROTECTED]> > --- > Incorporates cleanups noted by Jean Delvare. > > drivers/gpio/Kconfig| 10 + > drivers/gpio/Makefile |1 > drivers/gpio/pca9539.c | 271 > > include/linux/i2c/pca9539.h | 18 ++ > 4 files changed, 300 insertions(+) Acked-by: Jean Delvare <[EMAIL PROTECTED]> -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
From: eric miao <[EMAIL PROTECTED]>
Subject: [PATCH] gpiolib: support PCA9539 GPIO expander
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders. These chips have multiple registers,
push-pull output drivers, and (not supported in this patch) pin
change interrupts.
Board-specific code must provide "pca9539_platform_data" with each
chip's "i2c_board_info". That provides the GPIO numbers to be used
by that chip, and callbacks for board-specific setup/teardown logic.
Derived from drivers/i2c/chips/pca9539.c (which has no current known
users). This is faster and simpler; it uses 16-bit register access,
and cache the OUTPUT and DIRECTION registers for fast access
Signed-off-by: eric miao <[EMAIL PROTECTED]>
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
Incorporates cleanups noted by Jean Delvare.
drivers/gpio/Kconfig| 10 +
drivers/gpio/Makefile |1
drivers/gpio/pca9539.c | 271
include/linux/i2c/pca9539.h | 18 ++
4 files changed, 300 insertions(+)
--- at91.orig/drivers/gpio/Kconfig 2008-01-05 11:27:12.0 -0800
+++ at91/drivers/gpio/Kconfig 2008-01-05 11:27:12.0 -0800
@@ -27,6 +27,16 @@ config DEBUG_GPIO
comment "I2C GPIO expanders:"
+config GPIO_PCA9539
+ tristate "PCA9539 16-bit I/O port"
+ depends on I2C
+ help
+ Say yes here to support the PCA9539 16-bit I/O port. These
+ parts are made by NXP and TI.
+
+ This driver can also be built as a module. If so, the module
+ will be called pca9539.
+
config GPIO_PCF857X
tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
depends on I2C
--- at91.orig/drivers/gpio/Makefile 2008-01-05 11:27:12.0 -0800
+++ at91/drivers/gpio/Makefile 2008-01-05 11:27:12.0 -0800
@@ -1,6 +1,7 @@
# gpio support: dedicated expander chips, etc
obj-$(CONFIG_GPIO_MCP23S08)+= mcp23s08.o
+obj-$(CONFIG_GPIO_PCA9539) += pca9539.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
ifeq ($(CONFIG_DEBUG_GPIO),y)
--- /dev/null 1970-01-01 00:00:00.0 +
+++ at91/drivers/gpio/pca9539.c 2008-01-05 11:32:06.0 -0800
@@ -0,0 +1,271 @@
+/*
+ * pca9539.c - 16-bit I/O port with interrupt and reset
+ *
+ * Copyright (C) 2005 Ben Gardner <[EMAIL PROTECTED]>
+ * Copyright (C) 2007 Marvell International Ltd.
+ *
+ * Derived from drivers/i2c/chips/pca9539.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+
+#define NR_PCA9539_GPIOS 16
+
+#define PCA9539_INPUT 0
+#define PCA9539_OUTPUT 2
+#define PCA9539_INVERT 4
+#define PCA9539_DIRECTION 6
+
+struct pca9539_chip {
+ unsigned gpio_start;
+ uint16_t reg_output;
+ uint16_t reg_direction;
+
+ struct i2c_client *client;
+ struct gpio_chip gpio_chip;
+};
+
+/* NOTE: we can't currently rely on fault codes to come from SMBus
+ * calls, so we map all errors to EIO here and return zero otherwise.
+ */
+static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
+{
+ if (i2c_smbus_write_word_data(chip->client, reg, val) < 0)
+ return -EIO;
+ else
+ return 0;
+}
+
+static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(chip->client, reg);
+ if (ret < 0) {
+ dev_err(>client->dev, "failed reading register\n");
+ return -EIO;
+ }
+
+ *val = (uint16_t)ret;
+ return 0;
+}
+
+static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ reg_val = chip->reg_direction | (1u << off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip->reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_direction_output(struct gpio_chip *gc,
+ unsigned off, int val)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ /* set output level */
+ if (val)
+ reg_val = chip->reg_output | (1u << off);
+ else
+ reg_val = chip->reg_output & ~(1u << off);
+
+ ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
+ if (ret)
+ return ret;
+
+ chip->reg_output = reg_val;
+
+ /* then direction */
+ reg_val = chip->reg_direction & ~(1u << off);
+ ret =
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
From: eric miao [EMAIL PROTECTED]
Subject: [PATCH] gpiolib: support PCA9539 GPIO expander
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders. These chips have multiple registers,
push-pull output drivers, and (not supported in this patch) pin
change interrupts.
Board-specific code must provide pca9539_platform_data with each
chip's i2c_board_info. That provides the GPIO numbers to be used
by that chip, and callbacks for board-specific setup/teardown logic.
Derived from drivers/i2c/chips/pca9539.c (which has no current known
users). This is faster and simpler; it uses 16-bit register access,
and cache the OUTPUT and DIRECTION registers for fast access
Signed-off-by: eric miao [EMAIL PROTECTED]
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
Incorporates cleanups noted by Jean Delvare.
drivers/gpio/Kconfig| 10 +
drivers/gpio/Makefile |1
drivers/gpio/pca9539.c | 271
include/linux/i2c/pca9539.h | 18 ++
4 files changed, 300 insertions(+)
--- at91.orig/drivers/gpio/Kconfig 2008-01-05 11:27:12.0 -0800
+++ at91/drivers/gpio/Kconfig 2008-01-05 11:27:12.0 -0800
@@ -27,6 +27,16 @@ config DEBUG_GPIO
comment I2C GPIO expanders:
+config GPIO_PCA9539
+ tristate PCA9539 16-bit I/O port
+ depends on I2C
+ help
+ Say yes here to support the PCA9539 16-bit I/O port. These
+ parts are made by NXP and TI.
+
+ This driver can also be built as a module. If so, the module
+ will be called pca9539.
+
config GPIO_PCF857X
tristate PCF857x, PCA857x, and PCA967x I2C GPIO expanders
depends on I2C
--- at91.orig/drivers/gpio/Makefile 2008-01-05 11:27:12.0 -0800
+++ at91/drivers/gpio/Makefile 2008-01-05 11:27:12.0 -0800
@@ -1,6 +1,7 @@
# gpio support: dedicated expander chips, etc
obj-$(CONFIG_GPIO_MCP23S08)+= mcp23s08.o
+obj-$(CONFIG_GPIO_PCA9539) += pca9539.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
ifeq ($(CONFIG_DEBUG_GPIO),y)
--- /dev/null 1970-01-01 00:00:00.0 +
+++ at91/drivers/gpio/pca9539.c 2008-01-05 11:32:06.0 -0800
@@ -0,0 +1,271 @@
+/*
+ * pca9539.c - 16-bit I/O port with interrupt and reset
+ *
+ * Copyright (C) 2005 Ben Gardner [EMAIL PROTECTED]
+ * Copyright (C) 2007 Marvell International Ltd.
+ *
+ * Derived from drivers/i2c/chips/pca9539.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include linux/module.h
+#include linux/init.h
+#include linux/i2c.h
+#include linux/i2c/pca9539.h
+
+#include asm/gpio.h
+
+
+#define NR_PCA9539_GPIOS 16
+
+#define PCA9539_INPUT 0
+#define PCA9539_OUTPUT 2
+#define PCA9539_INVERT 4
+#define PCA9539_DIRECTION 6
+
+struct pca9539_chip {
+ unsigned gpio_start;
+ uint16_t reg_output;
+ uint16_t reg_direction;
+
+ struct i2c_client *client;
+ struct gpio_chip gpio_chip;
+};
+
+/* NOTE: we can't currently rely on fault codes to come from SMBus
+ * calls, so we map all errors to EIO here and return zero otherwise.
+ */
+static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
+{
+ if (i2c_smbus_write_word_data(chip-client, reg, val) 0)
+ return -EIO;
+ else
+ return 0;
+}
+
+static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(chip-client, reg);
+ if (ret 0) {
+ dev_err(chip-client-dev, failed reading register\n);
+ return -EIO;
+ }
+
+ *val = (uint16_t)ret;
+ return 0;
+}
+
+static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ reg_val = chip-reg_direction | (1u off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip-reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_direction_output(struct gpio_chip *gc,
+ unsigned off, int val)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ /* set output level */
+ if (val)
+ reg_val = chip-reg_output | (1u off);
+ else
+ reg_val = chip-reg_output ~(1u off);
+
+ ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
+ if (ret)
+ return ret;
+
+ chip-reg_output = reg_val;
+
+ /* then direction */
+ reg_val = chip-reg_direction ~(1u off);
+
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
Le 02 Janvier 2008, Jean Delvare a écrit:
>
> Hi David, hi Eric,
>
> Le 29/12/2007, "David Brownell" <[EMAIL PROTECTED]> a écrit:
> >From: eric miao <[EMAIL PROTECTED]>
> >
> >This adds a new-style I2C driver with basic support for the sixteen
> >bit PCA9539 GPIO expanders.
> >
> > ...
>
> Random comments:
>
> >+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
> >+{
> >+...
> >+
> >+ret = pca9539_read_reg(chip, PCA9539_INPUT, _val);
> >+if (ret < 0) {
> >+/* NOTE: diagnostic already omitted; that's the
> >+ * best we can do here.
> >+ */
> >+return 0;
> >+}
>
> I guess that you really mean "emitted" here, not "omitted"?
Yeah, typo.
> More importantly, I don't agree that it's the best we can do here.
> Maybe it was already discussed before and there's a good reason to not
> report errors from "get" functions at the gpio-core level,
Yes there is. It's by explicit request. Expecting drivers to cope
with per-bit errors is at best unrealistic. This was decided well
over a year ago ... nobody wants to see bit-banging code that spends
more time trying to figure out how to recover from "can't happen"
errors than getting real work done. (None of the SOC-specific GPIO
interfaces being replaced by this generic one returned errors either.)
That said, with things like I2C there actually *could* be errors;
which are impossible with valid parameters to SOC-level GPIOs.
That might argue for gpio_{get,set}_value_cansleep() calls being
able to return fault codes that would be nonsense on the more
widely used gpio_{get,set}_value() alls.
But such a change would be for a different set of patches. This
set does not change *any* driver programming interface. At all.
> >+static int __devinit pca9539_probe(struct i2c_client *client)
> >+{
> >+ (...)
> >+if (pdata->setup) {
> >+ret = pdata->setup(client, chip->gpio_chip.base,
> >+chip->gpio_chip.ngpio, pdata->context);
> >+if (ret < 0)
> >+dev_dbg(>dev, "setup failed, %d\n", ret);
>
> Should be at least dev_warn() and maybe even dev_err().
It's not treated as an error (i.e. abort the probe); warning
is right.
Hmm, I thought both this issue and the previous one had been
fixed already ... oh, it was the pcf857x driver that fixed that.
Never mind. ;)
> >+}
> >+ (...)
> >+}
> >+
> >+static int pca9539_remove(struct i2c_client *client)
> >+{
> >+ (...)
> >+if (pdata->teardown) {
> >+ret = pdata->teardown(client, chip->gpio_chip.base,
> >+chip->gpio_chip.ngpio, pdata->context);
> >+if (ret < 0)
> >+dev_dbg(>dev, "teardown failed, %d\n", ret);
>
> Same thing here.
That was supposed to be dev_err() then "return ret" !
> >+}
> >+
> >+ret = gpiochip_remove(>gpio_chip);
> >+if (ret) {
> >+dev_err(>dev, "failed remove gpio_chip\n");
>
> This error message could certainly be reworded to sound better. Also, for
> consistency you should include the value of "ret" in the message.
Right. So, pretty much like the appended. (Which I'll merge into
refreshed version of this patch.)
--- a/drivers/gpio/pca9539.c
+++ b/drivers/gpio/pca9539.c
@@ -118,7 +118,7 @@ static int pca9539_gpio_get_value(struct
ret = pca9539_read_reg(chip, PCA9539_INPUT, _val);
if (ret < 0) {
- /* NOTE: diagnostic already omitted; that's the
+ /* NOTE: diagnostic already emitted; that's the
* best we can do here.
*/
return 0;
@@ -205,7 +205,7 @@ static int __devinit pca9539_probe(struc
ret = pdata->setup(client, chip->gpio_chip.base,
chip->gpio_chip.ngpio, pdata->context);
if (ret < 0)
- dev_dbg(>dev, "setup failed, %d\n", ret);
+ dev_warn(>dev, "setup failed, %d\n", ret);
}
i2c_set_clientdata(client, chip);
@@ -225,13 +225,17 @@ static int pca9539_remove(struct i2c_cli
if (pdata->teardown) {
ret = pdata->teardown(client, chip->gpio_chip.base,
chip->gpio_chip.ngpio, pdata->context);
- if (ret < 0)
- dev_dbg(>dev, "teardown failed, %d\n", ret);
+ if (ret < 0) {
+ dev_err(>dev, "%s failed, %d\n",
+ "teardown", ret);
+ return ret;
+ }
}
ret = gpiochip_remove(>gpio_chip);
if (ret) {
- dev_err(>dev, "failed remove gpio_chip\n");
+ dev_err(>dev, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
return ret;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
Le 02 Janvier 2008, Jean Delvare a écrit:
Hi David, hi Eric,
Le 29/12/2007, David Brownell [EMAIL PROTECTED] a écrit:
From: eric miao [EMAIL PROTECTED]
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders.
...
Random comments:
+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+...
+
+ret = pca9539_read_reg(chip, PCA9539_INPUT, reg_val);
+if (ret 0) {
+/* NOTE: diagnostic already omitted; that's the
+ * best we can do here.
+ */
+return 0;
+}
I guess that you really mean emitted here, not omitted?
Yeah, typo.
More importantly, I don't agree that it's the best we can do here.
Maybe it was already discussed before and there's a good reason to not
report errors from get functions at the gpio-core level,
Yes there is. It's by explicit request. Expecting drivers to cope
with per-bit errors is at best unrealistic. This was decided well
over a year ago ... nobody wants to see bit-banging code that spends
more time trying to figure out how to recover from can't happen
errors than getting real work done. (None of the SOC-specific GPIO
interfaces being replaced by this generic one returned errors either.)
That said, with things like I2C there actually *could* be errors;
which are impossible with valid parameters to SOC-level GPIOs.
That might argue for gpio_{get,set}_value_cansleep() calls being
able to return fault codes that would be nonsense on the more
widely used gpio_{get,set}_value() alls.
But such a change would be for a different set of patches. This
set does not change *any* driver programming interface. At all.
+static int __devinit pca9539_probe(struct i2c_client *client)
+{
+ (...)
+if (pdata-setup) {
+ret = pdata-setup(client, chip-gpio_chip.base,
+chip-gpio_chip.ngpio, pdata-context);
+if (ret 0)
+dev_dbg(client-dev, setup failed, %d\n, ret);
Should be at least dev_warn() and maybe even dev_err().
It's not treated as an error (i.e. abort the probe); warning
is right.
Hmm, I thought both this issue and the previous one had been
fixed already ... oh, it was the pcf857x driver that fixed that.
Never mind. ;)
+}
+ (...)
+}
+
+static int pca9539_remove(struct i2c_client *client)
+{
+ (...)
+if (pdata-teardown) {
+ret = pdata-teardown(client, chip-gpio_chip.base,
+chip-gpio_chip.ngpio, pdata-context);
+if (ret 0)
+dev_dbg(client-dev, teardown failed, %d\n, ret);
Same thing here.
That was supposed to be dev_err() then return ret !
+}
+
+ret = gpiochip_remove(chip-gpio_chip);
+if (ret) {
+dev_err(client-dev, failed remove gpio_chip\n);
This error message could certainly be reworded to sound better. Also, for
consistency you should include the value of ret in the message.
Right. So, pretty much like the appended. (Which I'll merge into
refreshed version of this patch.)
--- a/drivers/gpio/pca9539.c
+++ b/drivers/gpio/pca9539.c
@@ -118,7 +118,7 @@ static int pca9539_gpio_get_value(struct
ret = pca9539_read_reg(chip, PCA9539_INPUT, reg_val);
if (ret 0) {
- /* NOTE: diagnostic already omitted; that's the
+ /* NOTE: diagnostic already emitted; that's the
* best we can do here.
*/
return 0;
@@ -205,7 +205,7 @@ static int __devinit pca9539_probe(struc
ret = pdata-setup(client, chip-gpio_chip.base,
chip-gpio_chip.ngpio, pdata-context);
if (ret 0)
- dev_dbg(client-dev, setup failed, %d\n, ret);
+ dev_warn(client-dev, setup failed, %d\n, ret);
}
i2c_set_clientdata(client, chip);
@@ -225,13 +225,17 @@ static int pca9539_remove(struct i2c_cli
if (pdata-teardown) {
ret = pdata-teardown(client, chip-gpio_chip.base,
chip-gpio_chip.ngpio, pdata-context);
- if (ret 0)
- dev_dbg(client-dev, teardown failed, %d\n, ret);
+ if (ret 0) {
+ dev_err(client-dev, %s failed, %d\n,
+ teardown, ret);
+ return ret;
+ }
}
ret = gpiochip_remove(chip-gpio_chip);
if (ret) {
- dev_err(client-dev, failed remove gpio_chip\n);
+ dev_err(client-dev, %s failed, %d\n,
+ gpiochip_remove(), ret);
return ret;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
Hi David, hi Eric,
Le 29/12/2007, "David Brownell" <[EMAIL PROTECTED]> a écrit:
>From: eric miao <[EMAIL PROTECTED]>
>
>This adds a new-style I2C driver with basic support for the sixteen
>bit PCA9539 GPIO expanders. These chips have multiple registers,
>push-pull output drivers, and (not supported by this patch) pin
>change interrupts.
>
>Board-specific code must provide "pca9539_platform_data" with each
>chip's "i2c_board_info". That provides the GPIO numbers to be used
>by that chip, and callbacks for board-specific setup/teardown logic.
>
>Derived from drivers/i2c/chips/pca9539.c (which has no current known
>users). This is faster and simpler; it uses 16-bit register access,
>and caches the OUTPUT and DIRECTION registers for fast access.
>
>Signed-off-by: eric miao <[EMAIL PROTECTED]>
>Signed-off-by: David Brownell <[EMAIL PROTECTED]>
>---
> drivers/gpio/Kconfig| 10 +
> drivers/gpio/Makefile |1
> drivers/gpio/pca9539.c | 264 ++
> include/linux/i2c/pca9539.h | 18 +++
> 4 files changed, 293 insertions(+)
Random comments:
>+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
>+{
>+ struct pca9539_chip *chip;
>+ uint16_t reg_val;
>+ int ret;
>+
>+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
>+
>+ ret = pca9539_read_reg(chip, PCA9539_INPUT, _val);
>+ if (ret < 0) {
>+ /* NOTE: diagnostic already omitted; that's the
>+ * best we can do here.
>+ */
>+ return 0;
>+ }
I guess that you really mean "emitted" here, not "omitted"?
More importantly, I don't agree that it's the best we can do here.
Maybe it was already discussed before and there's a good reason to not
report errors from "get" functions at the gpio-core level, but I
can't see it. Whether a read error should be considered as "0" or
"1" (or neither) should be a decision left to the user of the GPIO
chip, rather than to each GPIO driver.
>+
>+ return (reg_val & (1u << off)) ? 1 : 0;
>+}
>+static int __devinit pca9539_probe(struct i2c_client *client)
>+{
>+ (...)
>+ if (pdata->setup) {
>+ ret = pdata->setup(client, chip->gpio_chip.base,
>+ chip->gpio_chip.ngpio, pdata->context);
>+ if (ret < 0)
>+ dev_dbg(>dev, "setup failed, %d\n", ret);
Should be at least dev_warn() and maybe even dev_err().
>+ }
>+ (...)
>+}
>+
>+static int pca9539_remove(struct i2c_client *client)
>+{
>+ (...)
>+ if (pdata->teardown) {
>+ ret = pdata->teardown(client, chip->gpio_chip.base,
>+ chip->gpio_chip.ngpio, pdata->context);
>+ if (ret < 0)
>+ dev_dbg(>dev, "teardown failed, %d\n", ret);
Same thing here.
>+ }
>+
>+ ret = gpiochip_remove(>gpio_chip);
>+ if (ret) {
>+ dev_err(>dev, "failed remove gpio_chip\n");
This error message could certainly be reworded to sound better. Also, for
consistency you should include the value of "ret" in the message.
>+ return ret;
>+ }
>+
>+ kfree(chip);
>+ return 0;
>+}
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
Hi David, hi Eric,
Le 29/12/2007, David Brownell [EMAIL PROTECTED] a écrit:
From: eric miao [EMAIL PROTECTED]
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders. These chips have multiple registers,
push-pull output drivers, and (not supported by this patch) pin
change interrupts.
Board-specific code must provide pca9539_platform_data with each
chip's i2c_board_info. That provides the GPIO numbers to be used
by that chip, and callbacks for board-specific setup/teardown logic.
Derived from drivers/i2c/chips/pca9539.c (which has no current known
users). This is faster and simpler; it uses 16-bit register access,
and caches the OUTPUT and DIRECTION registers for fast access.
Signed-off-by: eric miao [EMAIL PROTECTED]
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
drivers/gpio/Kconfig| 10 +
drivers/gpio/Makefile |1
drivers/gpio/pca9539.c | 264 ++
include/linux/i2c/pca9539.h | 18 +++
4 files changed, 293 insertions(+)
Random comments:
+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ ret = pca9539_read_reg(chip, PCA9539_INPUT, reg_val);
+ if (ret 0) {
+ /* NOTE: diagnostic already omitted; that's the
+ * best we can do here.
+ */
+ return 0;
+ }
I guess that you really mean emitted here, not omitted?
More importantly, I don't agree that it's the best we can do here.
Maybe it was already discussed before and there's a good reason to not
report errors from get functions at the gpio-core level, but I
can't see it. Whether a read error should be considered as 0 or
1 (or neither) should be a decision left to the user of the GPIO
chip, rather than to each GPIO driver.
+
+ return (reg_val (1u off)) ? 1 : 0;
+}
+static int __devinit pca9539_probe(struct i2c_client *client)
+{
+ (...)
+ if (pdata-setup) {
+ ret = pdata-setup(client, chip-gpio_chip.base,
+ chip-gpio_chip.ngpio, pdata-context);
+ if (ret 0)
+ dev_dbg(client-dev, setup failed, %d\n, ret);
Should be at least dev_warn() and maybe even dev_err().
+ }
+ (...)
+}
+
+static int pca9539_remove(struct i2c_client *client)
+{
+ (...)
+ if (pdata-teardown) {
+ ret = pdata-teardown(client, chip-gpio_chip.base,
+ chip-gpio_chip.ngpio, pdata-context);
+ if (ret 0)
+ dev_dbg(client-dev, teardown failed, %d\n, ret);
Same thing here.
+ }
+
+ ret = gpiochip_remove(chip-gpio_chip);
+ if (ret) {
+ dev_err(client-dev, failed remove gpio_chip\n);
This error message could certainly be reworded to sound better. Also, for
consistency you should include the value of ret in the message.
+ return ret;
+ }
+
+ kfree(chip);
+ return 0;
+}
--
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
From: eric miao <[EMAIL PROTECTED]>
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders. These chips have multiple registers,
push-pull output drivers, and (not supported by this patch) pin
change interrupts.
Board-specific code must provide "pca9539_platform_data" with each
chip's "i2c_board_info". That provides the GPIO numbers to be used
by that chip, and callbacks for board-specific setup/teardown logic.
Derived from drivers/i2c/chips/pca9539.c (which has no current known
users). This is faster and simpler; it uses 16-bit register access,
and caches the OUTPUT and DIRECTION registers for fast access.
Signed-off-by: eric miao <[EMAIL PROTECTED]>
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
drivers/gpio/Kconfig| 10 +
drivers/gpio/Makefile |1
drivers/gpio/pca9539.c | 264
include/linux/i2c/pca9539.h | 18 +++
4 files changed, 293 insertions(+)
--- at91.orig/drivers/gpio/Kconfig
+++ at91/drivers/gpio/Kconfig
@@ -27,6 +27,16 @@ config DEBUG_GPIO
comment "I2C GPIO expanders:"
+config GPIO_PCA9539
+ tristate "PCA9539 16-bit I/O port"
+ depends on I2C
+ help
+ Say yes here to support the PCA9539 16-bit I/O port. These
+ parts are made by NXP and TI.
+
+ This driver can also be built as a module. If so, the module
+ will be called pca9539.
+
config GPIO_PCF857X
tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
depends on I2C
--- at91.orig/drivers/gpio/Makefile
+++ at91/drivers/gpio/Makefile
@@ -1,6 +1,7 @@
# gpio support: dedicated expander chips, etc
obj-$(CONFIG_GPIO_MCP23S08)+= mcp23s08.o
+obj-$(CONFIG_GPIO_PCA9539) += pca9539.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
ifeq ($(CONFIG_DEBUG_GPIO),y)
--- /dev/null
+++ at91/drivers/gpio/pca9539.c
@@ -0,0 +1,264 @@
+/*
+ * pca9539.c - 16-bit I/O port with interrupt and reset
+ *
+ * Copyright (C) 2005 Ben Gardner <[EMAIL PROTECTED]>
+ * Copyright (C) 2007 Marvell International Ltd.
+ *
+ * Derived from drivers/i2c/chips/pca9539.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+
+#define NR_PCA9539_GPIOS 16
+
+#define PCA9539_INPUT 0
+#define PCA9539_OUTPUT 2
+#define PCA9539_INVERT 4
+#define PCA9539_DIRECTION 6
+
+struct pca9539_chip {
+ unsigned gpio_start;
+ uint16_t reg_output;
+ uint16_t reg_direction;
+
+ struct i2c_client *client;
+ struct gpio_chip gpio_chip;
+};
+
+/* NOTE: we can't currently rely on fault codes to come from SMBus
+ * calls, so we map all errors to EIO here and return zero otherwise.
+ */
+static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
+{
+ if (i2c_smbus_write_word_data(chip->client, reg, val) < 0)
+ return -EIO;
+ else
+ return 0;
+}
+
+static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(chip->client, reg);
+ if (ret < 0) {
+ dev_err(>client->dev, "failed reading register\n");
+ return -EIO;
+ }
+
+ *val = (uint16_t)ret;
+ return 0;
+}
+
+static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ reg_val = chip->reg_direction | (1u << off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip->reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_direction_output(struct gpio_chip *gc,
+ unsigned off, int val)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ /* set output level */
+ if (val)
+ reg_val = chip->reg_output | (1u << off);
+ else
+ reg_val = chip->reg_output & ~(1u << off);
+
+ ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
+ if (ret)
+ return ret;
+
+ chip->reg_output = reg_val;
+
+ /* then direction */
+ reg_val = chip->reg_direction & ~(1u << off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip->reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc,
[patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support
From: eric miao [EMAIL PROTECTED]
This adds a new-style I2C driver with basic support for the sixteen
bit PCA9539 GPIO expanders. These chips have multiple registers,
push-pull output drivers, and (not supported by this patch) pin
change interrupts.
Board-specific code must provide pca9539_platform_data with each
chip's i2c_board_info. That provides the GPIO numbers to be used
by that chip, and callbacks for board-specific setup/teardown logic.
Derived from drivers/i2c/chips/pca9539.c (which has no current known
users). This is faster and simpler; it uses 16-bit register access,
and caches the OUTPUT and DIRECTION registers for fast access.
Signed-off-by: eric miao [EMAIL PROTECTED]
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
drivers/gpio/Kconfig| 10 +
drivers/gpio/Makefile |1
drivers/gpio/pca9539.c | 264
include/linux/i2c/pca9539.h | 18 +++
4 files changed, 293 insertions(+)
--- at91.orig/drivers/gpio/Kconfig
+++ at91/drivers/gpio/Kconfig
@@ -27,6 +27,16 @@ config DEBUG_GPIO
comment I2C GPIO expanders:
+config GPIO_PCA9539
+ tristate PCA9539 16-bit I/O port
+ depends on I2C
+ help
+ Say yes here to support the PCA9539 16-bit I/O port. These
+ parts are made by NXP and TI.
+
+ This driver can also be built as a module. If so, the module
+ will be called pca9539.
+
config GPIO_PCF857X
tristate PCF857x, PCA857x, and PCA967x I2C GPIO expanders
depends on I2C
--- at91.orig/drivers/gpio/Makefile
+++ at91/drivers/gpio/Makefile
@@ -1,6 +1,7 @@
# gpio support: dedicated expander chips, etc
obj-$(CONFIG_GPIO_MCP23S08)+= mcp23s08.o
+obj-$(CONFIG_GPIO_PCA9539) += pca9539.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
ifeq ($(CONFIG_DEBUG_GPIO),y)
--- /dev/null
+++ at91/drivers/gpio/pca9539.c
@@ -0,0 +1,264 @@
+/*
+ * pca9539.c - 16-bit I/O port with interrupt and reset
+ *
+ * Copyright (C) 2005 Ben Gardner [EMAIL PROTECTED]
+ * Copyright (C) 2007 Marvell International Ltd.
+ *
+ * Derived from drivers/i2c/chips/pca9539.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include linux/module.h
+#include linux/init.h
+#include linux/i2c.h
+#include linux/i2c/pca9539.h
+
+#include asm/gpio.h
+
+
+#define NR_PCA9539_GPIOS 16
+
+#define PCA9539_INPUT 0
+#define PCA9539_OUTPUT 2
+#define PCA9539_INVERT 4
+#define PCA9539_DIRECTION 6
+
+struct pca9539_chip {
+ unsigned gpio_start;
+ uint16_t reg_output;
+ uint16_t reg_direction;
+
+ struct i2c_client *client;
+ struct gpio_chip gpio_chip;
+};
+
+/* NOTE: we can't currently rely on fault codes to come from SMBus
+ * calls, so we map all errors to EIO here and return zero otherwise.
+ */
+static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
+{
+ if (i2c_smbus_write_word_data(chip-client, reg, val) 0)
+ return -EIO;
+ else
+ return 0;
+}
+
+static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(chip-client, reg);
+ if (ret 0) {
+ dev_err(chip-client-dev, failed reading register\n);
+ return -EIO;
+ }
+
+ *val = (uint16_t)ret;
+ return 0;
+}
+
+static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ reg_val = chip-reg_direction | (1u off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip-reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_direction_output(struct gpio_chip *gc,
+ unsigned off, int val)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct pca9539_chip, gpio_chip);
+
+ /* set output level */
+ if (val)
+ reg_val = chip-reg_output | (1u off);
+ else
+ reg_val = chip-reg_output ~(1u off);
+
+ ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
+ if (ret)
+ return ret;
+
+ chip-reg_output = reg_val;
+
+ /* then direction */
+ reg_val = chip-reg_direction ~(1u off);
+ ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+ if (ret)
+ return ret;
+
+ chip-reg_direction = reg_val;
+ return 0;
+}
+
+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+ struct pca9539_chip *chip;
+ uint16_t reg_val;
+ int ret;
+
+
