This is a new-style I2C driver for some common 8 and 16 bit I2C based
GPIO expanders: pcf8574 and pcf8575. Since it's a new-style driver,
it's configured as part of board-specific init ... eliminating the
need for error-prone manual configuration of module parameters.
The driver exposes the GPIO signals using the platform-neutral GPIO
programming interface, so they are easily accessed by other kernel
code. The lack of such a flexible kernel API is what has ensured
the proliferation of board-specific hacks for these chips... stuff
that rarely makes it upstream since it's so ugly. This driver will
let such board-specific code use standard GPIO calls.
Signed-off-by: David Brownell <[email protected]>
---
Note that there's currently a drivers/i2c/chips/pcf8574.c driver.
Key differences include: this one talks to other kernel code so
it can use the GPIOs "normally", but that one talks to userspace
through sysfs. Also, this one is a "new style" I2C driver, so it's
smaller and doesn't need all those error-prone module parameters.
Plus, this one handles both 8 and 16 bit chip versions.
drivers/i2c/chips/Kconfig | 18 ++
drivers/i2c/chips/Makefile | 1
drivers/i2c/chips/pcf857x.c | 309 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pcf857x.h | 43 ++++++
4 files changed, 371 insertions(+)
--- a/drivers/i2c/chips/Kconfig 2007-10-28 21:04:06.000000000 -0700
+++ b/drivers/i2c/chips/Kconfig 2007-10-29 14:16:01.000000000 -0700
@@ -51,6 +51,24 @@ config SENSORS_EEPROM
This driver can also be built as a module. If so, the module
will be called eeprom.
+config GPIO_PCF857X
+ tristate "PCF875x GPIO expanders"
+ depends on GPIO_LIB
+ help
+ Say yes here to provide access to some I2C GPIO expanders which
+ may be used for additional digital outputs or inputs:
+
+ - pcf8574, pcf8574a ... 8 bits, from NXP or TI
+ - pcf8575, pcf8575c ... 16 bits, from NXP or TI
+
+ Your board setup code will need to declare the expanders in
+ use, and assign numbers to the GPIOs they expose. Those GPIOs
+ can then be used from drivers and other kernel code, just like
+ other GPIOs, but only accessible from task contexts.
+
+ This driver provides only an in-kernel interface to those GPIOs.
+ Any sysfs interface to userspace would be provided separately.
+
config SENSORS_PCF8574
tristate "Philips PCF8574 and PCF8574A"
depends on EXPERIMENTAL
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/include/linux/pcf857x.h 2007-10-28 21:09:49.000000000 -0700
@@ -0,0 +1,43 @@
+#ifndef __LINUX_PCF857X_H
+#define __LINUX_PCF857X_H
+
+/**
+ * struct pcf857x_platform_data - data to set up pcf857x driver
+ * @gpio_base: number of the chip's first GPIO
+ * @n_latch: optional bit-inverse of initial output state
+ * @context: optional parameter passed to setup() and teardown()
+ * @setup: optional callback issued once the GPIOs are valid
+ * @teardown: optional callback issued before the GPIOs are invvalidated
+ *
+ * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
+ * the i2c_board_info used with the pcf875x driver must provide the
+ * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
+ * platform_data (pointer to one of these structures) with at least
+ * the gpio_base value initialized.
+ *
+ * The @setup callback may be used with the kind of board-specific glue
+ * which hands the (now-valid) GPIOs to other drivers, or which puts
+ * devices in their initial states using these GPIOs.
+ *
+ * Since these GPIO chips don't have conventional registers recording
+ * whether a pin is used for input or output, or an output latch to
+ * record the values being driven, the n_latch value may be used to
+ * avoid intialization glitches. Its inverted value initializes the
+ * value into which bits are masked before they're written to the PCF
+ * chip. That means that if it's left at zero, the chip is treated as
+ * if it came from power-up reset.
+ */
+struct pcf857x_platform_data {
+ unsigned gpio_base;
+ unsigned n_latch;
+
+ void *context;
+ int (*setup)(struct i2c_client *client,
+ int gpio, unsigned ngpio,
+ void *context);
+ int (*teardown)(struct i2c_client *client,
+ int gpio, unsigned ngpio,
+ void *context);
+};
+
+#endif /* __LINUX_PCF857X_H */
--- a/drivers/i2c/chips/Makefile 2007-10-28 21:04:06.000000000 -0700
+++ b/drivers/i2c/chips/Makefile 2007-10-28 21:09:49.000000000 -0700
@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00) += m41t00.o
obj-$(CONFIG_SENSORS_PCA9539) += pca9539.o
obj-$(CONFIG_SENSORS_PCF8574) += pcf8574.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
+obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
obj-$(CONFIG_ISP1301_OMAP) += isp1301_omap.o
obj-$(CONFIG_TPS65010) += tps65010.o
obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/drivers/i2c/chips/pcf857x.c 2007-10-29 14:12:21.000000000 -0700
@@ -0,0 +1,309 @@
+/*
+ * pcf857x - driver for pcf857{4,4a,5,5c} I2C GPIO expanders
+ *
+ * Copyright (C) 2007 David Brownell
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/pcf857x.h>
+
+#include <asm/gpio.h>
+
+
+/*
+ * The pcf857x chips only expose a one read register and one write register.
+ * Writing a "one" bit (to match the reset state) lets that pin be used as
+ * an input; it's not an open-drain model, but it acts a bit like that.
+ *
+ * Some other I2C GPIO expander chips (like the pca9534, pca9535, pca9555,
+ * pca9539, mcp23008, and mc23017) have a more complex register model with
+ * more conventional input circuitry, often using 0x20..0x27 addresses.
+ */
+struct pcf857x {
+ struct gpio_chip chip;
+ struct i2c_client *client;
+ unsigned out; /* software latch */
+};
+
+/*-------------------------------------------------------------------------*/
+
+/* Talk to 8-bit I/O expander */
+
+static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+
+ gpio->out |= (1 << offset);
+ return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
+}
+
+static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ s32 value;
+
+ value = i2c_smbus_read_byte(gpio->client);
+ return (value < 0) ? 0 : !!(value & (1 << offset));
+}
+
+static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ unsigned bit = 1 << offset;
+
+ if (value)
+ gpio->out |= bit;
+ else
+ gpio->out &= ~bit;
+ return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
+}
+
+static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
+{
+ pcf857x_output8(chip, offset, value);
+}
+
+/*-------------------------------------------------------------------------*/
+
+/* Talk to 16-bit I/O expander */
+
+static int i2c_write_le16(struct i2c_client *client, u16 word)
+{
+ u8 buf[2] = { word & 0xff, word >> 8, };
+ int status;
+
+ status = i2c_master_send(client, buf, 2);
+ return (status < 0) ? status : 0;
+}
+
+static int i2c_read_le16(struct i2c_client *client)
+{
+ u8 buf[2];
+ int status;
+
+ status = i2c_master_recv(client, buf, 2);
+ if (status < 0)
+ return status;
+ return (buf[1] << 8) | buf[0];
+}
+
+static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+
+ gpio->out |= (1 << offset);
+ return i2c_write_le16(gpio->client, (u16) gpio->out);
+}
+
+static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ int value;
+
+ value = i2c_read_le16(gpio->client);
+ return (value < 0) ? 0 : !!(value & (1 << offset));
+}
+
+static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ unsigned bit = 1 << offset;
+
+ if (value)
+ gpio->out |= bit;
+ else
+ gpio->out &= ~bit;
+ return i2c_write_le16(gpio->client, (u16) gpio->out);
+}
+
+static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
+{
+ pcf857x_output16(chip, offset, value);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int pcf857x_probe(struct i2c_client *client)
+{
+ struct pcf857x_platform_data *pdata;
+ struct pcf857x *gpio;
+ int status = 0;
+
+ pdata = client->dev.platform_data;
+ if (!pdata)
+ return -ENODEV;
+
+ /* Allocate, initialize, and register this gpio_chip. */
+ gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->chip.base = pdata->gpio_base;
+ gpio->chip.can_sleep = 1;
+
+ /* NOTE: the OnSemi jlc1562b is also largely compatible with
+ * these parts, notably for output. It has a low-resolution
+ * DAC instead of pin change IRQs; and its inputs can be the
+ * result of comparators.
+ */
+
+ /* '74a addresses are 0x38..0x3f; '74 uses 0x20..0x27 */
+ if (strcmp(client->name, "pcf8574a") == 0
+ || strcmp(client->name, "pcf8574") == 0) {
+ gpio->chip.ngpio = 8;
+ gpio->chip.direction_input = pcf857x_input8;
+ gpio->chip.get = pcf857x_get8;
+ gpio->chip.direction_output = pcf857x_output8;
+ gpio->chip.set = pcf857x_set8;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BYTE))
+ status = -EIO;
+
+ /* fail if there's no chip present */
+ status = i2c_smbus_read_byte(client);
+
+ /* '75/'75c addresses are 0x20..0x27, just like the '74;
+ * the '75c doesn't have a current source pulling high.
+ */
+ } else if (strcmp(client->name, "pcf8575c") == 0
+ || strcmp(client->name, "pcf8575") == 0) {
+ gpio->chip.ngpio = 16;
+ gpio->chip.direction_input = pcf857x_input16;
+ gpio->chip.get = pcf857x_get16;
+ gpio->chip.direction_output = pcf857x_output16;
+ gpio->chip.set = pcf857x_set16;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ status = -EIO;
+
+ /* fail if there's no chip present */
+ status = i2c_read_le16(client);
+
+ } else
+ status = -ENODEV;
+
+ if (status < 0) {
+ dev_dbg(&client->dev, "probe error %d for '%s'\n",
+ status, client->name);
+ kfree(gpio);
+ return status;
+ }
+
+ gpio->chip.label = client->name;
+
+ gpio->client = client;
+ i2c_set_clientdata(client, gpio);
+
+ /* NOTE: these chips have strange "pseudo-bidirectional" I/O pins.
+ * We can't actually know whether a pin is configured (a) as output
+ * and driving the signal low, or (b) as input and reporting a low
+ * value ... without knowing the last value written since the chip
+ * came out of reset (if any). We can't read the latched output.
+ *
+ * In short, the only reliable solution for setting up pin direction
+ * is to do it explicitly. The setup() method can do that.
+ *
+ * We use pdata->n_latch to avoid trouble. In the typical case it's
+ * left initialized to zero; our software copy of the "latch" then
+ * matches the chip's reset state. But there may be cases where a
+ * system must drive some pins low, without transient glitching.
+ * Handle those cases by assigning n_latch to a nonzero value.
+ */
+ gpio->out = ~pdata->n_latch;
+
+ status = gpiochip_add(&gpio->chip);
+ if (status < 0) {
+ kfree(gpio);
+ return status;
+ }
+
+ /* NOTE: these chips can issue "some pin-changed" IRQs, which we
+ * don't yet even try to use. Among other issues, the relevant
+ * genirq state isn't available to modular drivers; and most irq
+ * methods can't be called from sleeping contexts.
+ */
+
+ dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
+ gpio->chip.base,
+ gpio->chip.base + gpio->chip.ngpio - 1,
+ client->name,
+ client->irq ? " (irq ignored)" : "");
+
+ /* Let platform code set up the GPIOs and their users.
+ * Now is the first time anyone can use them.
+ */
+ if (pdata->setup) {
+ status = pdata->setup(client,
+ gpio->chip.base, gpio->chip.ngpio,
+ pdata->context);
+ if (status < 0)
+ dev_dbg(&client->dev, "setup --> %d\n", status);
+ }
+
+ return 0;
+}
+
+static int pcf857x_remove(struct i2c_client *client)
+{
+ struct pcf857x_platform_data *pdata = client->dev.platform_data;
+ struct pcf857x *gpio = i2c_get_clientdata(client);
+ int status = 0;
+
+ if (pdata->teardown) {
+ status = pdata->teardown(client,
+ gpio->chip.base, gpio->chip.ngpio,
+ pdata->context);
+ if (status < 0) {
+ dev_err(&client->dev, "%s --> %d\n",
+ "teardown", status);
+ return status;
+ }
+ }
+
+ status = gpiochip_remove(&gpio->chip);
+ if (status == 0)
+ kfree(gpio);
+ else
+ dev_err(&client->dev, "%s --> %d\n", "remove", status);
+ return status;
+}
+
+static struct i2c_driver pcf857x_driver = {
+ .driver = {
+ .name = "pcf857x",
+ .owner = THIS_MODULE,
+ },
+ .probe = pcf857x_probe,
+ .remove = pcf857x_remove,
+};
+
+static int __init pcf857x_init(void)
+{
+ return i2c_add_driver(&pcf857x_driver);
+}
+/* we want GPIOs to be ready at device_initcall() time */
+subsys_initcall(pcf857x_init);
+
+static void __exit pcf857x_exit(void)
+{
+ i2c_del_driver(&pcf857x_driver);
+}
+module_exit(pcf857x_exit);
+
+MODULE_LICENSE("GPL");
Hi David,
Sorry for the late review.
Note that I can't test your code.
On Mon, 29 Oct 2007 18:51:48 -0700, David Brownell wrote:
> This is a new-style I2C driver for some common 8 and 16 bit I2C based
> GPIO expanders: pcf8574 and pcf8575. Since it's a new-style driver,
> it's configured as part of board-specific init ... eliminating the
> need for error-prone manual configuration of module parameters.
>
> The driver exposes the GPIO signals using the platform-neutral GPIO
> programming interface, so they are easily accessed by other kernel
> code. The lack of such a flexible kernel API is what has ensured
> the proliferation of board-specific hacks for these chips... stuff
> that rarely makes it upstream since it's so ugly. This driver will
> let such board-specific code use standard GPIO calls.
>
> Signed-off-by: David Brownell <[email protected]>
> ---
> Note that there's currently a drivers/i2c/chips/pcf8574.c driver.
By now there's also a drivers/i2c/chips/pcf8575.c driver.
>
> Key differences include: this one talks to other kernel code so
> it can use the GPIOs "normally", but that one talks to userspace
> through sysfs. Also, this one is a "new style" I2C driver, so it's
> smaller and doesn't need all those error-prone module parameters.
> Plus, this one handles both 8 and 16 bit chip versions.
>
> drivers/i2c/chips/Kconfig | 18 ++
> drivers/i2c/chips/Makefile | 1
> drivers/i2c/chips/pcf857x.c | 309 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pcf857x.h | 43 ++++++
> 4 files changed, 371 insertions(+)
>
> --- a/drivers/i2c/chips/Kconfig 2007-10-28 21:04:06.000000000 -0700
> +++ b/drivers/i2c/chips/Kconfig 2007-10-29 14:16:01.000000000 -0700
> @@ -51,6 +51,24 @@ config SENSORS_EEPROM
> This driver can also be built as a module. If so, the module
> will be called eeprom.
>
> +config GPIO_PCF857X
> + tristate "PCF875x GPIO expanders"
> + depends on GPIO_LIB
> + help
> + Say yes here to provide access to some I2C GPIO expanders which
> + may be used for additional digital outputs or inputs:
> +
> + - pcf8574, pcf8574a ... 8 bits, from NXP or TI
> + - pcf8575, pcf8575c ... 16 bits, from NXP or TI
> +
> + Your board setup code will need to declare the expanders in
> + use, and assign numbers to the GPIOs they expose. Those GPIOs
> + can then be used from drivers and other kernel code, just like
> + other GPIOs, but only accessible from task contexts.
> +
> + This driver provides only an in-kernel interface to those GPIOs.
> + Any sysfs interface to userspace would be provided separately.
How?
> +
> config SENSORS_PCF8574
> tristate "Philips PCF8574 and PCF8574A"
> depends on EXPERIMENTAL
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ b/include/linux/pcf857x.h 2007-10-28 21:09:49.000000000 -0700
> @@ -0,0 +1,43 @@
> +#ifndef __LINUX_PCF857X_H
> +#define __LINUX_PCF857X_H
> +
> +/**
> + * struct pcf857x_platform_data - data to set up pcf857x driver
> + * @gpio_base: number of the chip's first GPIO
> + * @n_latch: optional bit-inverse of initial output state
Strange name, and I can't make much sense of the description either.
> + * @context: optional parameter passed to setup() and teardown()
> + * @setup: optional callback issued once the GPIOs are valid
> + * @teardown: optional callback issued before the GPIOs are invvalidated
Would make more sense to list setup and teardown before context?
Typo: invalidated.
> + *
> + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
> + * the i2c_board_info used with the pcf875x driver must provide the
> + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
> + * platform_data (pointer to one of these structures) with at least
> + * the gpio_base value initialized.
> + *
> + * The @setup callback may be used with the kind of board-specific glue
> + * which hands the (now-valid) GPIOs to other drivers, or which puts
> + * devices in their initial states using these GPIOs.
> + *
> + * Since these GPIO chips don't have conventional registers recording
> + * whether a pin is used for input or output, or an output latch to
> + * record the values being driven, the n_latch value may be used to
> + * avoid intialization glitches. Its inverted value initializes the
Typo: initialization.
> + * value into which bits are masked before they're written to the PCF
> + * chip. That means that if it's left at zero, the chip is treated as
> + * if it came from power-up reset.
After reading this paragraph I still have no idea what n_latch does.
But maybe that's just me.
> + */
> +struct pcf857x_platform_data {
> + unsigned gpio_base;
> + unsigned n_latch;
> +
> + void *context;
> + int (*setup)(struct i2c_client *client,
> + int gpio, unsigned ngpio,
> + void *context);
> + int (*teardown)(struct i2c_client *client,
> + int gpio, unsigned ngpio,
> + void *context);
> +};
> +
> +#endif /* __LINUX_PCF857X_H */
> --- a/drivers/i2c/chips/Makefile 2007-10-28 21:04:06.000000000 -0700
> +++ b/drivers/i2c/chips/Makefile 2007-10-28 21:09:49.000000000 -0700
> @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00) += m41t00.o
> obj-$(CONFIG_SENSORS_PCA9539) += pca9539.o
> obj-$(CONFIG_SENSORS_PCF8574) += pcf8574.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
For alphabetical order, it would go one line above.
> obj-$(CONFIG_ISP1301_OMAP) += isp1301_omap.o
> obj-$(CONFIG_TPS65010) += tps65010.o
> obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ b/drivers/i2c/chips/pcf857x.c 2007-10-29 14:12:21.000000000 -0700
> @@ -0,0 +1,309 @@
> +/*
> + * pcf857x - driver for pcf857{4,4a,5,5c} I2C GPIO expanders
I recommend spelling out chip names completely, as it lets people grep
the kernel tree for chip names when they look for support.
> + *
> + * Copyright (C) 2007 David Brownell
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/pcf857x.h>
I suspect that there will be many more such header files in the future.
Would it make sense to move them to include/linux/gpio?
> +
> +#include <asm/gpio.h>
> +
> +
> +/*
> + * The pcf857x chips only expose a one read register and one write register.
Typo: extra "a".
> + * Writing a "one" bit (to match the reset state) lets that pin be used as
> + * an input; it's not an open-drain model, but it acts a bit like that.
> + *
> + * Some other I2C GPIO expander chips (like the pca9534, pca9535, pca9555,
> + * pca9539, mcp23008, and mc23017) have a more complex register model with
> + * more conventional input circuitry, often using 0x20..0x27 addresses.
> + */
> +struct pcf857x {
> + struct gpio_chip chip;
> + struct i2c_client *client;
> + unsigned out; /* software latch */
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* Talk to 8-bit I/O expander */
> +
> +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> +
> + gpio->out |= (1 << offset);
> + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
Useless cast.
> +}
> +
> +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + s32 value;
> +
> + value = i2c_smbus_read_byte(gpio->client);
> + return (value < 0) ? 0 : !!(value & (1 << offset));
!!(value & (1 << offset))
is more efficiently written
(value >> offset) & 1
> +}
> +
> +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + unsigned bit = 1 << offset;
> +
> + if (value)
> + gpio->out |= bit;
> + else
> + gpio->out &= ~bit;
> + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
Useless cast.
> +}
> +
> +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + pcf857x_output8(chip, offset, value);
> +}
It would be more efficient to drop pcf857x_set8 altogether and do
gpio->chip.set = pcf857x_output8.
Many of the comments above apply to the 16-bit functions below as well.
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* Talk to 16-bit I/O expander */
> +
> +static int i2c_write_le16(struct i2c_client *client, u16 word)
> +{
> + u8 buf[2] = { word & 0xff, word >> 8, };
Stray comma.
> + int status;
> +
> + status = i2c_master_send(client, buf, 2);
> + return (status < 0) ? status : 0;
> +}
> +
> +static int i2c_read_le16(struct i2c_client *client)
> +{
> + u8 buf[2];
> + int status;
> +
> + status = i2c_master_recv(client, buf, 2);
> + if (status < 0)
> + return status;
> + return (buf[1] << 8) | buf[0];
> +}
> +
> +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> +
> + gpio->out |= (1 << offset);
> + return i2c_write_le16(gpio->client, (u16) gpio->out);
> +}
> +
> +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + int value;
> +
> + value = i2c_read_le16(gpio->client);
> + return (value < 0) ? 0 : !!(value & (1 << offset));
> +}
> +
> +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + unsigned bit = 1 << offset;
> +
> + if (value)
> + gpio->out |= bit;
> + else
> + gpio->out &= ~bit;
> + return i2c_write_le16(gpio->client, (u16) gpio->out);
> +}
> +
> +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + pcf857x_output16(chip, offset, value);
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int pcf857x_probe(struct i2c_client *client)
> +{
> + struct pcf857x_platform_data *pdata;
> + struct pcf857x *gpio;
> + int status = 0;
Useless initialization.
> +
> + pdata = client->dev.platform_data;
> + if (!pdata)
> + return -ENODEV;
> +
> + /* Allocate, initialize, and register this gpio_chip. */
> + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
You need to include <linux/slab.h> to have access to this function.
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->chip.base = pdata->gpio_base;
> + gpio->chip.can_sleep = 1;
> +
> + /* NOTE: the OnSemi jlc1562b is also largely compatible with
> + * these parts, notably for output. It has a low-resolution
> + * DAC instead of pin change IRQs; and its inputs can be the
> + * result of comparators.
> + */
> +
> + /* '74a addresses are 0x38..0x3f; '74 uses 0x20..0x27 */
> + if (strcmp(client->name, "pcf8574a") == 0
> + || strcmp(client->name, "pcf8574") == 0) {
> + gpio->chip.ngpio = 8;
> + gpio->chip.direction_input = pcf857x_input8;
> + gpio->chip.get = pcf857x_get8;
> + gpio->chip.direction_output = pcf857x_output8;
> + gpio->chip.set = pcf857x_set8;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE))
> + status = -EIO;
You set status here...
> +
> + /* fail if there's no chip present */
> + status = i2c_smbus_read_byte(client);
... but overwrite it immediately.
> +
> + /* '75/'75c addresses are 0x20..0x27, just like the '74;
> + * the '75c doesn't have a current source pulling high.
> + */
> + } else if (strcmp(client->name, "pcf8575c") == 0
> + || strcmp(client->name, "pcf8575") == 0) {
> + gpio->chip.ngpio = 16;
> + gpio->chip.direction_input = pcf857x_input16;
> + gpio->chip.get = pcf857x_get16;
> + gpio->chip.direction_output = pcf857x_output16;
> + gpio->chip.set = pcf857x_set16;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + status = -EIO;
> +
> + /* fail if there's no chip present */
> + status = i2c_read_le16(client);
Same problem here.
> +
> + } else
> + status = -ENODEV;
> +
> + if (status < 0) {
> + dev_dbg(&client->dev, "probe error %d for '%s'\n",
> + status, client->name);
> + kfree(gpio);
> + return status;
Might make sense to move this to a common error path, as you do it more
than once.
> + }
> +
> + gpio->chip.label = client->name;
> +
> + gpio->client = client;
> + i2c_set_clientdata(client, gpio);
> +
> + /* NOTE: these chips have strange "pseudo-bidirectional" I/O pins.
> + * We can't actually know whether a pin is configured (a) as output
> + * and driving the signal low, or (b) as input and reporting a low
> + * value ... without knowing the last value written since the chip
> + * came out of reset (if any). We can't read the latched output.
> + *
> + * In short, the only reliable solution for setting up pin direction
> + * is to do it explicitly. The setup() method can do that.
> + *
> + * We use pdata->n_latch to avoid trouble. In the typical case it's
> + * left initialized to zero; our software copy of the "latch" then
> + * matches the chip's reset state. But there may be cases where a
> + * system must drive some pins low, without transient glitching.
> + * Handle those cases by assigning n_latch to a nonzero value.
> + */
> + gpio->out = ~pdata->n_latch;
> +
> + status = gpiochip_add(&gpio->chip);
> + if (status < 0) {
> + kfree(gpio);
> + return status;
> + }
> +
> + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
> + * don't yet even try to use. Among other issues, the relevant
> + * genirq state isn't available to modular drivers; and most irq
> + * methods can't be called from sleeping contexts.
> + */
> +
> + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
> + gpio->chip.base,
> + gpio->chip.base + gpio->chip.ngpio - 1,
> + client->name,
> + client->irq ? " (irq ignored)" : "");
> +
> + /* Let platform code set up the GPIOs and their users.
> + * Now is the first time anyone can use them.
> + */
> + if (pdata->setup) {
> + status = pdata->setup(client,
> + gpio->chip.base, gpio->chip.ngpio,
> + pdata->context);
> + if (status < 0)
> + dev_dbg(&client->dev, "setup --> %d\n", status);
> + }
> +
> + return 0;
> +}
> +
> +static int pcf857x_remove(struct i2c_client *client)
> +{
> + struct pcf857x_platform_data *pdata = client->dev.platform_data;
> + struct pcf857x *gpio = i2c_get_clientdata(client);
> + int status = 0;
> +
> + if (pdata->teardown) {
> + status = pdata->teardown(client,
> + gpio->chip.base, gpio->chip.ngpio,
> + pdata->context);
> + if (status < 0) {
> + dev_err(&client->dev, "%s --> %d\n",
> + "teardown", status);
Why %s instead of hard-coding "teardown"?
Why is this an error message while a failing setup() at initialization
time only deserves a debug message?
> + return status;
> + }
> + }
> +
> + status = gpiochip_remove(&gpio->chip);
> + if (status == 0)
> + kfree(gpio);
> + else
> + dev_err(&client->dev, "%s --> %d\n", "remove", status);
> + return status;
> +}
> +
> +static struct i2c_driver pcf857x_driver = {
> + .driver = {
> + .name = "pcf857x",
> + .owner = THIS_MODULE,
> + },
> + .probe = pcf857x_probe,
> + .remove = pcf857x_remove,
> +};
> +
> +static int __init pcf857x_init(void)
> +{
> + return i2c_add_driver(&pcf857x_driver);
> +}
> +/* we want GPIOs to be ready at device_initcall() time */
> +subsys_initcall(pcf857x_init);
> +
> +static void __exit pcf857x_exit(void)
> +{
> + i2c_del_driver(&pcf857x_driver);
> +}
> +module_exit(pcf857x_exit);
> +
> +MODULE_LICENSE("GPL");
No MODULE_AUTHOR? No entry in MAINTAINERS?
--
Jean Delvare
Jean Delvare wrote:
> !!(value & (1 << offset))
> is more efficiently written
> (value >> offset) & 1
>
... but not more efficiently implemented.
Your version requires code to do the shift on live data at runtime.
David's version lets the compiler create the mask once, at compile-time.
b.g.
--
Bill Gatliff
[email protected]
Hi Bill,
On Fri, 30 Nov 2007 07:04:10 -0600, Bill Gatliff wrote:
> Jean Delvare wrote:
> > !!(value & (1 << offset))
> > is more efficiently written
> > (value >> offset) & 1
>
> ... but not more efficiently implemented.
>
> Your version requires code to do the shift on live data at runtime.
> David's version lets the compiler create the mask once, at compile-time.
I don't understand. How could the compiler create the mask at
compile-time when "offset" is only known at runtime?
Anyway, I just checked and gcc (4.1.2 on x86_86) generates the same
code for both expressions, so there's not much to argue about. David,
feel free to ignore my comment if you prefer your implementation :)
--
Jean Delvare
Jean Delvare wrote:
> Hi Bill,
>
> On Fri, 30 Nov 2007 07:04:10 -0600, Bill Gatliff wrote:
>
>> Jean Delvare wrote:
>>
>>> !!(value & (1 << offset))
>>> is more efficiently written
>>> (value >> offset) & 1
>>>
>> ... but not more efficiently implemented.
>>
>> Your version requires code to do the shift on live data at runtime.
>> David's version lets the compiler create the mask once, at compile-time.
>>
>
> I don't understand. How could the compiler create the mask at
> compile-time when "offset" is only known at runtime?
>
Oops. :) I was thinking of some different code. Disregard.
/me got up too early this morning, after working too late last night!
b.g.
--
Bill Gatliff
[email protected]
On Friday 30 November 2007, Jean Delvare wrote:
> Hi David,
>
> Sorry for the late review.
I've got patches in my queue too, sigh ...
Thanks for the review. I'll snip out typos and similar trivial
comments (and fix them!), using responses here for more the
substantive feedback.
- Dave
> > --- a/drivers/i2c/chips/Kconfig 2007-10-28 21:04:06.000000000 -0700
> > +++ b/drivers/i2c/chips/Kconfig 2007-10-29 14:16:01.000000000 -0700
> > @@ -51,6 +51,24 @@ config SENSORS_EEPROM
> > This driver can also be built as a module. If so, the module
> > will be called eeprom.
> >
> > +config GPIO_PCF857X
> > + tristate "PCF875x GPIO expanders"
> > + depends on GPIO_LIB
> > + help
> > + ...
> > +
> > + This driver provides only an in-kernel interface to those GPIOs.
> > + Any sysfs interface to userspace would be provided separately.
>
> How?
I'll take that out, to avoid the question. The answer is still mostly
TBD, but the gpiolib infrastructure provides a number of the hooks
that such a userspace interface would need.
> > +/**
> > + * struct pcf857x_platform_data - data to set up pcf857x driver
> > + * @gpio_base: number of the chip's first GPIO
> > + * @n_latch: optional bit-inverse of initial output state
>
> Strange name, and I can't make much sense of the description either.
Updated description:
* @n_latch: optional bit-inverse of initial register value; if
* you leave this initialized to zero, the driver will treat
* all bits as inputs as if the chip was just reset
This chip is documented as being "pseudo-bidirectional", which is
a sign that there are some confusing mechanisms lurking...
Conventions for naming negative-true signals include a "#" suffix
(illegal for C), a overbar (not expressible in ASCII), and prefixes
including "/" (illegal for C) and "n" (aha!). I morphed the latter
into "n_" since it's often paired with all-caps signal names, as
in "nRESET", which are bad kernel coding style.
Latches hold values; http://en.wikipedia.org/wiki/Latch_%28electronics%29
talks about bit-level latching, but GPIO controllers use register-wide
latches to record the value that should be driven on output pins.
(As opposed to input pins, whose values are read without latching.)
> After reading this paragraph I still have no idea what n_latch does.
> But maybe that's just me.
It's a wierd little arrangement, maybe you have a better explanation.
I tried hitting the confusing points more directly:
* These GPIO chips are only "pseudo-bidirectional"; read the chip specs
* to understand the behavior. They don't have separate registers to
* record which pins are used for input or output, record which output
* values are driven, or provide access to input values. That must all
* be inferred by reading the chip's value and knowing the last value
* written to it. If you don't initialize n_latch, that last written
* value is presumed to be all ones (as if the chip were just reset).
> > --- a/drivers/i2c/chips/Makefile 2007-10-28 21:04:06.000000000 -0700
> > +++ b/drivers/i2c/chips/Makefile 2007-10-28 21:09:49.000000000 -0700
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00) += m41t00.o
> > obj-$(CONFIG_SENSORS_PCA9539) += pca9539.o
> > obj-$(CONFIG_SENSORS_PCF8574) += pcf8574.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
>
> For alphabetical order, it would go one line above.
For alphabetical order it would go much sooner.
GPIO precedes SENSOR. ;)
> > obj-$(CONFIG_ISP1301_OMAP) += isp1301_omap.o
> > obj-$(CONFIG_TPS65010) += tps65010.o
> > obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ b/drivers/i2c/chips/pcf857x.c 2007-10-29 14:12:21.000000000 -0700
> > @@ -0,0 +1,309 @@
> > +/*
> > + * pcf857x - driver for pcf857{4,4a,5,5c} I2C GPIO expanders
>
> I recommend spelling out chip names completely, as it lets people grep
> the kernel tree for chip names when they look for support.
I'll do that -- but note that the names *are* spelled out later.
> > +#include <linux/pcf857x.h>
>
> I suspect that there will be many more such header files in the future.
> Would it make sense to move them to include/linux/gpio?
I was thinking more like <linux/i2c/...> myself. There are many more
I2C chips than GPIO expanders.
> > +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
> > + ...
> > +
> > +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
> > +{
> > + pcf857x_output8(chip, offset, value);
> > +}
>
> It would be more efficient to drop pcf857x_set8 altogether and do
> gpio->chip.set = pcf857x_output8.
No can do; return types differ, which means that on some platforms
the calling conventions have significant differences.
> > +?????????????????????dev_err(&client->dev, "%s --> %d\n",
> > +?????????????????????????????????????"teardown", status);
>
> Why %s instead of hard-coding "teardown"?
To share (current code) three copies of the "<3>%s %s: %s --> %d\n"
string. Every little bit of kernel bloat prevention helps. ;)
Hi David,
On Fri, 30 Nov 2007 10:40:54 -0800, David Brownell wrote:
> On Friday 30 November 2007, Jean Delvare wrote:
> > > --- a/drivers/i2c/chips/Kconfig 2007-10-28 21:04:06.000000000 -0700
> > > +++ b/drivers/i2c/chips/Kconfig 2007-10-29 14:16:01.000000000 -0700
> > > @@ -51,6 +51,24 @@ config SENSORS_EEPROM
> > > This driver can also be built as a module. If so, the module
> > > will be called eeprom.
> > >
> > > +config GPIO_PCF857X
> > > + tristate "PCF875x GPIO expanders"
> > > + depends on GPIO_LIB
> > > + help
> > > + ...
> > > +
> > > + This driver provides only an in-kernel interface to those GPIOs.
> > > + Any sysfs interface to userspace would be provided separately.
> >
> > How?
>
> I'll take that out, to avoid the question. The answer is still mostly
> TBD, but the gpiolib infrastructure provides a number of the hooks
> that such a userspace interface would need.
So the user-space interface would be part of the generic GPIO
infrastructure? I like the idea.
> > > +/**
> > > + * struct pcf857x_platform_data - data to set up pcf857x driver
> > > + * @gpio_base: number of the chip's first GPIO
> > > + * @n_latch: optional bit-inverse of initial output state
> >
> > Strange name, and I can't make much sense of the description either.
>
> Updated description:
>
> * @n_latch: optional bit-inverse of initial register value; if
> * you leave this initialized to zero, the driver will treat
> * all bits as inputs as if the chip was just reset
>
> This chip is documented as being "pseudo-bidirectional", which is
> a sign that there are some confusing mechanisms lurking...
>
>
> Conventions for naming negative-true signals include a "#" suffix
> (illegal for C), a overbar (not expressible in ASCII), and prefixes
> including "/" (illegal for C) and "n" (aha!). I morphed the latter
> into "n_" since it's often paired with all-caps signal names, as
> in "nRESET", which are bad kernel coding style.
>
> Latches hold values; http://en.wikipedia.org/wiki/Latch_%28electronics%29
> talks about bit-level latching, but GPIO controllers use register-wide
> latches to record the value that should be driven on output pins.
> (As opposed to input pins, whose values are read without latching.)
>
>
> > After reading this paragraph I still have no idea what n_latch does.
> > But maybe that's just me.
>
> It's a wierd little arrangement, maybe you have a better explanation.
> I tried hitting the confusing points more directly:
>
> * These GPIO chips are only "pseudo-bidirectional"; read the chip specs
> * to understand the behavior. They don't have separate registers to
> * record which pins are used for input or output, record which output
> * values are driven, or provide access to input values. That must all
> * be inferred by reading the chip's value and knowing the last value
> * written to it. If you don't initialize n_latch, that last written
> * value is presumed to be all ones (as if the chip were just reset).
Much clearer now, thanks. I know what a latch is, I just couldn't get
how latching (or lack thereof) was related with an initial register
value. With the explanation above, I get it.
> > > --- a/drivers/i2c/chips/Makefile 2007-10-28 21:04:06.000000000 -0700
> > > +++ b/drivers/i2c/chips/Makefile 2007-10-28 21:09:49.000000000 -0700
> > > @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00) += m41t00.o
> > > obj-$(CONFIG_SENSORS_PCA9539) += pca9539.o
> > > obj-$(CONFIG_SENSORS_PCF8574) += pcf8574.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
> >
> > For alphabetical order, it would go one line above.
>
> For alphabetical order it would go much sooner.
> GPIO precedes SENSOR. ;)
We apply the alphabetical order to driver names, not configuration
symbols, as far as I know. Though for most directories it probably
doesn't make a difference; drivers/i2c/chips is admittedly a bit messy
in this respect.
Note that at some point I will attempt to get rid of the "SENSORS" part
of configuration options that have nothing to do with sensors, that
should help a bit.
> > > +#include <linux/pcf857x.h>
> >
> > I suspect that there will be many more such header files in the future.
> > Would it make sense to move them to include/linux/gpio?
>
> I was thinking more like <linux/i2c/...> myself. There are many more
> I2C chips than GPIO expanders.
But most i2c chip drivers don't need a header file. Or is this going to
change with the new-style i2c drivers?
Along the same line, I am wondering if it would make sense to put the
various GPIO drivers in drivers/gpio. It's a much better practice to
group the drivers according to the functionality they provide than the
way they are connected to the system. drivers/i2c/chips is an exception
in this respect, it's meant for i2c drivers that have no obvious place
to live in. That's why there aren't many drivers there, and I hope it
will stay this way. In an ideal world we could even get rid of this
directory and move the remaining drivers to drivers/misc.
> > > +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
> > > + ...
> > > +
> > > +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
> > > +{
> > > + pcf857x_output8(chip, offset, value);
> > > +}
> >
> > It would be more efficient to drop pcf857x_set8 altogether and do
> > gpio->chip.set = pcf857x_output8.
>
> No can do; return types differ, which means that on some platforms
> the calling conventions have significant differences.
Ah, right, sorry for missing that. I had only looked at the parameters
and forgot the return type.
> > > +?????????????????????dev_err(&client->dev, "%s --> %d\n",
> > > +?????????????????????????????????????"teardown", status);
> >
> > Why %s instead of hard-coding "teardown"?
>
> To share (current code) three copies of the "<3>%s %s: %s --> %d\n"
> string. Every little bit of kernel bloat prevention helps. ;)
Only two copies in the version you posted, but indeed there would be
three if the trick was applied consistently.
--
Jean Delvare
On Friday 30 November 2007, Jean Delvare wrote:
>
> So the user-space interface would be part of the generic GPIO
> infrastructure? I like the idea.
I thought that would make sense too! :) Someone would need to
write the code though. Having such a mechanism would provide
another "carrot" to migrate folk towards the gpiolib core.
I think adding a gpiochip primitive to mark a (potential) GPIO
as invalid would support the converse of /sys/kernel/debug/gpio.
Invalid GPIOs include pins set up for non-GPIO usage (like being
used for MMC or MII), or not wired up on a given board. Pins
that were valid as GPIOs and not requested by a kernel driver
might reasonably be managed by userspace code.
> > > > +#include <linux/pcf857x.h>
> > >
> > > I suspect that there will be many more such header files in the future.
> > > Would it make sense to move them to include/linux/gpio?
> >
> > I was thinking more like <linux/i2c/...> myself. There are many more
> > I2C chips than GPIO expanders.
>
> But most i2c chip drivers don't need a header file. Or is this going to
> change with the new-style i2c drivers?
I expect it will become a lot more common. Remember that legacy
I2C drivers *couldn't* get any board-specific config data; that's
been problematic, since it meant the drivers themselves ended up
with lots of board-specific cruft. That prevented many drivers
from going upstream at all. (As I mentioned about pcf8574 code,
although in that case the problem was worsened by lack of any
reusable kernel interface for such GPIO signals.)
> Along the same line, I am wondering if it would make sense to put the
> various GPIO drivers in drivers/gpio.
Could be. Right now we have three "GPIO expander" drivers using
the new "gpiolib" framework: pcf875x and pca9539 for I2C, and
mcp23s08 for SPI. There are many more that *could* be used with
Linux boxes. And there are other drivers/XYZ directories that
are (currently) that small. Maybe gpiolib should go upstream
like that, and lib/gpiolib should be in drivers/gpio too...
However, keep in mind that lots of chips export a few GPIOs but
don't have that as their core functionality ... one example is
the drivers/i2c/chips/tps65010 driver. So it'd never be the
case that GPIO drivers only live in that directory.
- Dave
On Friday 30 November 2007, David Brownell wrote:
> Thanks for the review. ?I'll snip out typos and similar trivial
> comments (and fix them!), using responses here for more the
> substantive feedback.
Here's the current version of this patch ... updated to put the
driver into drivers/gpio (separate patch setting that up) and
the header into <linux/i2c/pcf857x.h>
Note that after looking at the GPIO expanders listed at the NXP
website, I updated this to accept a few more of these chips.
Other than reset pins and addressing options, the key difference
between these seems to be the top I2C clock speed supported:
pcf857x ... 100 KHz
pca857x ... 400 KHz
pca967x ... 1000 KHz
Otherwise they're equivalent at the level of just swapping parts.
- Dave
============= SNIP!
This is a new-style I2C driver for most common 8 and 16 bit I2C based
"quasi-bidirectional" GPIO expanders: pcf8574 or pcf8575, and several
compatible models (mostly faster, supporting I2C at up to 1 MHz).
Since it's a new-style driver, these devices must be configured as
part of board-specific init. That eliminates the need for error-prone
manual configuration of module parameters, and makes compatibility
with legacy drivers (pcf8574.c, pc8575.c)for these chips easier.
The driver exposes the GPIO signals using the platform-neutral GPIO
programming interface, so they are easily accessed by other kernel
code. The lack of such a flexible kernel API is what has ensured
the proliferation of board-specific drivers for these chips... stuff
that rarely makes it upstream since it's so ugly. This driver will
let them use standard calls.
Signed-off-by: David Brownell <[email protected]>
---
drivers/gpio/Kconfig | 23 +++
drivers/gpio/Makefile | 2
drivers/gpio/pcf857x.c | 331 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c/pcf857x.h | 45 +++++
4 files changed, 401 insertions(+)
--- a/drivers/gpio/Kconfig 2007-12-05 15:13:27.000000000 -0800
+++ b/drivers/gpio/Kconfig 2007-12-05 15:14:12.000000000 -0800
@@ -5,4 +5,27 @@
menu "GPIO Support"
depends on GPIO_LIB
+config GPIO_PCF857X
+ tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
+ depends on I2C
+ help
+ Say yes here to provide access to most "quasi-bidirectional" I2C
+ GPIO expanders used for additional digital outputs or inputs.
+ Most of these parts are from NXP, though TI is a second source for
+ some of them. Compatible models include:
+
+ 8 bits: pcf8574, pcf8574a, pca8574, pca8574a,
+ pca9670, pca9672, pca9674, pca9674a
+
+ 16 bits: pcf8575, pcf8575c, pca8575,
+ pca9671, pca9673, pca9675
+
+ Your board setup code will need to declare the expanders in
+ use, and assign numbers to the GPIOs they expose. Those GPIOs
+ can then be used from drivers and other kernel code, just like
+ other GPIOs, but only accessible from task contexts.
+
+ This driver provides an in-kernel interface to those GPIOs using
+ platform-neutral GPIO calls.
+
endmenu
--- a/drivers/gpio/Makefile 2007-12-05 15:14:03.000000000 -0800
+++ b/drivers/gpio/Makefile 2007-12-05 15:14:12.000000000 -0800
@@ -1 +1,3 @@
# gpio support: dedicated expander chips, etc
+
+obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/drivers/gpio/pcf857x.c 2007-12-05 15:15:18.000000000 -0800
@@ -0,0 +1,331 @@
+/*
+ * pcf857x - driver for pcf857x, pca857x, and pca967x I2C GPIO expanders
+ *
+ * Copyright (C) 2007 David Brownell
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c/pcf857x.h>
+
+#include <asm/gpio.h>
+
+
+/*
+ * The pcf857x, pca857x, and pca967x chips only expose one read and one
+ * write register. Writing a "one" bit (to match the reset state) lets
+ * that pin be used as an input; it's not an open-drain model, but acts
+ * a bit like one. This is described as "quasi-bidirectional"; read the
+ * chip documentation for details.
+ *
+ * Some other I2C GPIO expander chips (like the pca953{4,5,6,7,9}, pca9555,
+ * pca9698, mcp23008, and mc23017) have more complex register models with
+ * more conventional input circuitry, often using 0x20..0x27 addresses.
+ */
+struct pcf857x {
+ struct gpio_chip chip;
+ struct i2c_client *client;
+ unsigned out; /* software latch */
+};
+
+/*-------------------------------------------------------------------------*/
+
+/* Talk to 8-bit I/O expander */
+
+static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+
+ gpio->out |= (1 << offset);
+ return i2c_smbus_write_byte(gpio->client, gpio->out);
+}
+
+static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ s32 value;
+
+ value = i2c_smbus_read_byte(gpio->client);
+ return (value < 0) ? 0 : (value & (1 << offset));
+}
+
+static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ unsigned bit = 1 << offset;
+
+ if (value)
+ gpio->out |= bit;
+ else
+ gpio->out &= ~bit;
+ return i2c_smbus_write_byte(gpio->client, gpio->out);
+}
+
+static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
+{
+ pcf857x_output8(chip, offset, value);
+}
+
+/*-------------------------------------------------------------------------*/
+
+/* Talk to 16-bit I/O expander */
+
+static int i2c_write_le16(struct i2c_client *client, u16 word)
+{
+ u8 buf[2] = { word & 0xff, word >> 8, };
+ int status;
+
+ status = i2c_master_send(client, buf, 2);
+ return (status < 0) ? status : 0;
+}
+
+static int i2c_read_le16(struct i2c_client *client)
+{
+ u8 buf[2];
+ int status;
+
+ status = i2c_master_recv(client, buf, 2);
+ if (status < 0)
+ return status;
+ return (buf[1] << 8) | buf[0];
+}
+
+static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+
+ gpio->out |= (1 << offset);
+ return i2c_write_le16(gpio->client, gpio->out);
+}
+
+static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ int value;
+
+ value = i2c_read_le16(gpio->client);
+ return (value < 0) ? 0 : (value & (1 << offset));
+}
+
+static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+ unsigned bit = 1 << offset;
+
+ if (value)
+ gpio->out |= bit;
+ else
+ gpio->out &= ~bit;
+ return i2c_write_le16(gpio->client, gpio->out);
+}
+
+static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
+{
+ pcf857x_output16(chip, offset, value);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int pcf857x_probe(struct i2c_client *client)
+{
+ struct pcf857x_platform_data *pdata;
+ struct pcf857x *gpio;
+ int status;
+
+ pdata = client->dev.platform_data;
+ if (!pdata)
+ return -ENODEV;
+
+ /* Allocate, initialize, and register this gpio_chip. */
+ gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->chip.base = pdata->gpio_base;
+ gpio->chip.can_sleep = 1;
+
+ /* NOTE: the OnSemi jlc1562b is also largely compatible with
+ * these parts, notably for output. It has a low-resolution
+ * DAC instead of pin change IRQs; and its inputs can be the
+ * result of comparators.
+ */
+
+ /* 8574 addresses are 0x20..0x27; 8574a uses 0x38..0x3f;
+ * 9670, 9672, 9764, and 9764a use quite a variety.
+ *
+ * NOTE: we dont distinguish here between *4 and *4a parts.
+ */
+ if (strcmp(client->name, "pcf8574") == 0
+ || strcmp(client->name, "pca8574") == 0
+ || strcmp(client->name, "pca9670") == 0
+ || strcmp(client->name, "pca9672") == 0
+ || strcmp(client->name, "pca9674") == 0
+ ) {
+ gpio->chip.ngpio = 8;
+ gpio->chip.direction_input = pcf857x_input8;
+ gpio->chip.get = pcf857x_get8;
+ gpio->chip.direction_output = pcf857x_output8;
+ gpio->chip.set = pcf857x_set8;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BYTE))
+ status = -EIO;
+
+ /* fail if there's no chip present */
+ else
+ status = i2c_smbus_read_byte(client);
+
+ /* '75/'75c addresses are 0x20..0x27, just like the '74;
+ * the '75c doesn't have a current source pulling high.
+ * 9671, 9673, and 9765 use quite a variety of addresses.
+ *
+ * NOTE: we dont distinguish here between 8575/8575a parts.
+ */
+ } else if (strcmp(client->name, "pcf8575") == 0
+ || strcmp(client->name, "pca8575") == 0
+ || strcmp(client->name, "pca9671") == 0
+ || strcmp(client->name, "pca9673") == 0
+ || strcmp(client->name, "pca9675") == 0
+ ) {
+ gpio->chip.ngpio = 16;
+ gpio->chip.direction_input = pcf857x_input16;
+ gpio->chip.get = pcf857x_get16;
+ gpio->chip.direction_output = pcf857x_output16;
+ gpio->chip.set = pcf857x_set16;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ status = -EIO;
+
+ /* fail if there's no chip present */
+ else
+ status = i2c_read_le16(client);
+
+ } else
+ status = -ENODEV;
+
+ if (status < 0)
+ goto fail;
+
+ gpio->chip.label = client->name;
+
+ gpio->client = client;
+ i2c_set_clientdata(client, gpio);
+
+ /* NOTE: these chips have strange "quasi-bidirectional" I/O pins.
+ * We can't actually know whether a pin is configured (a) as output
+ * and driving the signal low, or (b) as input and reporting a low
+ * value ... without knowing the last value written since the chip
+ * came out of reset (if any). We can't read the latched output.
+ *
+ * In short, the only reliable solution for setting up pin direction
+ * is to do it explicitly. The setup() method can do that.
+ *
+ * We use pdata->n_latch to avoid trouble. In the typical case it's
+ * left initialized to zero; our software copy of the "latch" then
+ * matches the chip's all-ones reset state. But some systems will
+ * need to drive some pins low, while avoiding transient glitches.
+ * Handle those cases by assigning n_latch to a nonzero value.
+ */
+ gpio->out = ~pdata->n_latch;
+
+ status = gpiochip_add(&gpio->chip);
+ if (status < 0)
+ goto fail;
+
+ /* NOTE: these chips can issue "some pin-changed" IRQs, which we
+ * don't yet even try to use. Among other issues, the relevant
+ * genirq state isn't available to modular drivers; and most irq
+ * methods can't be called from sleeping contexts.
+ */
+
+ dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
+ gpio->chip.base,
+ gpio->chip.base + gpio->chip.ngpio - 1,
+ client->name,
+ client->irq ? " (irq ignored)" : "");
+
+ /* Let platform code set up the GPIOs and their users.
+ * Now is the first time anyone can use them.
+ */
+ if (pdata->setup) {
+ status = pdata->setup(client,
+ gpio->chip.base, gpio->chip.ngpio,
+ pdata->context);
+ if (status < 0)
+ dev_err(&client->dev, "%s --> %d\n",
+ "setup", status);
+ }
+
+ return 0;
+
+fail:
+ dev_dbg(&client->dev, "probe error %d for '%s'\n",
+ status, client->name);
+ kfree(gpio);
+ return status;
+}
+
+static int pcf857x_remove(struct i2c_client *client)
+{
+ struct pcf857x_platform_data *pdata = client->dev.platform_data;
+ struct pcf857x *gpio = i2c_get_clientdata(client);
+ int status = 0;
+
+ if (pdata->teardown) {
+ status = pdata->teardown(client,
+ gpio->chip.base, gpio->chip.ngpio,
+ pdata->context);
+ if (status < 0) {
+ dev_err(&client->dev, "%s --> %d\n",
+ "teardown", status);
+ return status;
+ }
+ }
+
+ status = gpiochip_remove(&gpio->chip);
+ if (status == 0)
+ kfree(gpio);
+ else
+ dev_err(&client->dev, "%s --> %d\n", "remove", status);
+ return status;
+}
+
+static struct i2c_driver pcf857x_driver = {
+ .driver = {
+ .name = "pcf857x",
+ .owner = THIS_MODULE,
+ },
+ .probe = pcf857x_probe,
+ .remove = pcf857x_remove,
+};
+
+static int __init pcf857x_init(void)
+{
+ return i2c_add_driver(&pcf857x_driver);
+}
+/* we want GPIOs to be ready at device_initcall() time */
+subsys_initcall(pcf857x_init);
+
+static void __exit pcf857x_exit(void)
+{
+ i2c_del_driver(&pcf857x_driver);
+}
+module_exit(pcf857x_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Brownell");
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ b/include/linux/i2c/pcf857x.h 2007-12-05 15:14:12.000000000 -0800
@@ -0,0 +1,45 @@
+#ifndef __LINUX_PCF857X_H
+#define __LINUX_PCF857X_H
+
+/**
+ * struct pcf857x_platform_data - data to set up pcf857x driver
+ * @gpio_base: number of the chip's first GPIO
+ * @n_latch: optional bit-inverse of initial register value; if
+ * you leave this initialized to zero the driver will act
+ * like the chip was just reset
+ * @setup: optional callback issued once the GPIOs are valid
+ * @teardown: optional callback issued before the GPIOs are invalidated
+ * @context: optional parameter passed to setup() and teardown()
+ *
+ * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
+ * the i2c_board_info used with the pcf875x driver must provide the
+ * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
+ * platform_data (pointer to one of these structures) with at least
+ * the gpio_base value initialized.
+ *
+ * The @setup callback may be used with the kind of board-specific glue
+ * which hands the (now-valid) GPIOs to other drivers, or which puts
+ * devices in their initial states using these GPIOs.
+ *
+ * These GPIO chips are only "quasi-bidirectional"; read the chip specs
+ * to understand the behavior. They don't have separate registers to
+ * record which pins are used for input or output, record which output
+ * values are driven, or provide access to input values. That must be
+ * inferred by reading the chip's value and knowing the last value written
+ * to it. If you leave n_latch initialized to zero, that last written
+ * value is presumed to be all ones (as if the chip were just reset).
+ */
+struct pcf857x_platform_data {
+ unsigned gpio_base;
+ unsigned n_latch;
+
+ int (*setup)(struct i2c_client *client,
+ int gpio, unsigned ngpio,
+ void *context);
+ int (*teardown)(struct i2c_client *client,
+ int gpio, unsigned ngpio,
+ void *context);
+ void *context;
+};
+
+#endif /* __LINUX_PCF857X_H */
Hi David,
On Wed, 5 Dec 2007 19:03:12 -0800, David Brownell wrote:
> On Friday 30 November 2007, David Brownell wrote:
> > Thanks for the review. ?I'll snip out typos and similar trivial
> > comments (and fix them!), using responses here for more the
> > substantive feedback.
>
> Here's the current version of this patch ... updated to put the
> driver into drivers/gpio (separate patch setting that up) and
> the header into <linux/i2c/pcf857x.h>
>
> Note that after looking at the GPIO expanders listed at the NXP
> website, I updated this to accept a few more of these chips.
> Other than reset pins and addressing options, the key difference
> between these seems to be the top I2C clock speed supported:
>
> pcf857x ... 100 KHz
> pca857x ... 400 KHz
> pca967x ... 1000 KHz
>
> Otherwise they're equivalent at the level of just swapping parts.
>
> - Dave
>
> ============= SNIP!
> This is a new-style I2C driver for most common 8 and 16 bit I2C based
> "quasi-bidirectional" GPIO expanders: pcf8574 or pcf8575, and several
> compatible models (mostly faster, supporting I2C at up to 1 MHz).
>
> Since it's a new-style driver, these devices must be configured as
> part of board-specific init. That eliminates the need for error-prone
> manual configuration of module parameters, and makes compatibility
> with legacy drivers (pcf8574.c, pc8575.c)for these chips easier.
Missing space after closing parenthesis. Also, I don't quite see what
is supposed to make compatibility with the legacy drivers easier, nor
how, not why it matters in the first place.
>
> The driver exposes the GPIO signals using the platform-neutral GPIO
> programming interface, so they are easily accessed by other kernel
> code. The lack of such a flexible kernel API is what has ensured
> the proliferation of board-specific drivers for these chips... stuff
> that rarely makes it upstream since it's so ugly. This driver will
> let them use standard calls.
>
> Signed-off-by: David Brownell <[email protected]>
> ---
> drivers/gpio/Kconfig | 23 +++
> drivers/gpio/Makefile | 2
> drivers/gpio/pcf857x.c | 331 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/i2c/pcf857x.h | 45 +++++
> 4 files changed, 401 insertions(+)
>
> --- a/drivers/gpio/Kconfig 2007-12-05 15:13:27.000000000 -0800
> +++ b/drivers/gpio/Kconfig 2007-12-05 15:14:12.000000000 -0800
> @@ -5,4 +5,27 @@
> menu "GPIO Support"
> depends on GPIO_LIB
>
> +config GPIO_PCF857X
> + tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
> + depends on I2C
> + help
> + Say yes here to provide access to most "quasi-bidirectional" I2C
> + GPIO expanders used for additional digital outputs or inputs.
> + Most of these parts are from NXP, though TI is a second source for
> + some of them. Compatible models include:
> +
> + 8 bits: pcf8574, pcf8574a, pca8574, pca8574a,
> + pca9670, pca9672, pca9674, pca9674a
> +
> + 16 bits: pcf8575, pcf8575c, pca8575,
> + pca9671, pca9673, pca9675
> +
> + Your board setup code will need to declare the expanders in
> + use, and assign numbers to the GPIOs they expose. Those GPIOs
> + can then be used from drivers and other kernel code, just like
> + other GPIOs, but only accessible from task contexts.
> +
> + This driver provides an in-kernel interface to those GPIOs using
> + platform-neutral GPIO calls.
> +
> endmenu
> --- a/drivers/gpio/Makefile 2007-12-05 15:14:03.000000000 -0800
> +++ b/drivers/gpio/Makefile 2007-12-05 15:14:12.000000000 -0800
> @@ -1 +1,3 @@
> # gpio support: dedicated expander chips, etc
> +
> +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ b/drivers/gpio/pcf857x.c 2007-12-05 15:15:18.000000000 -0800
> @@ -0,0 +1,331 @@
> +/*
> + * pcf857x - driver for pcf857x, pca857x, and pca967x I2C GPIO expanders
> + *
> + * Copyright (C) 2007 David Brownell
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/pcf857x.h>
> +
> +#include <asm/gpio.h>
> +
> +
> +/*
> + * The pcf857x, pca857x, and pca967x chips only expose one read and one
> + * write register. Writing a "one" bit (to match the reset state) lets
> + * that pin be used as an input; it's not an open-drain model, but acts
> + * a bit like one. This is described as "quasi-bidirectional"; read the
> + * chip documentation for details.
> + *
> + * Some other I2C GPIO expander chips (like the pca953{4,5,6,7,9}, pca9555,
> + * pca9698, mcp23008, and mc23017) have more complex register models with
mc_p_23017?
> + * more conventional input circuitry, often using 0x20..0x27 addresses.
> + */
> +struct pcf857x {
> + struct gpio_chip chip;
> + struct i2c_client *client;
> + unsigned out; /* software latch */
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* Talk to 8-bit I/O expander */
> +
> +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> +
> + gpio->out |= (1 << offset);
> + return i2c_smbus_write_byte(gpio->client, gpio->out);
> +}
> +
> +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + s32 value;
> +
> + value = i2c_smbus_read_byte(gpio->client);
> + return (value < 0) ? 0 : (value & (1 << offset));
This is no longer a boolean value, is that OK? I guess that it doesn't
matter but maybe it should be documented (what GPIO drivers are allowed
to return in these callback functions.)
> +}
> +
> +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + unsigned bit = 1 << offset;
> +
> + if (value)
> + gpio->out |= bit;
> + else
> + gpio->out &= ~bit;
> + return i2c_smbus_write_byte(gpio->client, gpio->out);
> +}
> +
> +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + pcf857x_output8(chip, offset, value);
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* Talk to 16-bit I/O expander */
> +
> +static int i2c_write_le16(struct i2c_client *client, u16 word)
> +{
> + u8 buf[2] = { word & 0xff, word >> 8, };
Stray comma.
> + int status;
> +
> + status = i2c_master_send(client, buf, 2);
> + return (status < 0) ? status : 0;
> +}
> +
> +static int i2c_read_le16(struct i2c_client *client)
> +{
> + u8 buf[2];
> + int status;
> +
> + status = i2c_master_recv(client, buf, 2);
> + if (status < 0)
> + return status;
> + return (buf[1] << 8) | buf[0];
> +}
> +
> +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> +
> + gpio->out |= (1 << offset);
> + return i2c_write_le16(gpio->client, gpio->out);
> +}
> +
> +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + int value;
> +
> + value = i2c_read_le16(gpio->client);
> + return (value < 0) ? 0 : (value & (1 << offset));
> +}
> +
> +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> + unsigned bit = 1 << offset;
> +
> + if (value)
> + gpio->out |= bit;
> + else
> + gpio->out &= ~bit;
> + return i2c_write_le16(gpio->client, gpio->out);
> +}
> +
> +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + pcf857x_output16(chip, offset, value);
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int pcf857x_probe(struct i2c_client *client)
> +{
> + struct pcf857x_platform_data *pdata;
> + struct pcf857x *gpio;
> + int status;
> +
> + pdata = client->dev.platform_data;
> + if (!pdata)
> + return -ENODEV;
> +
> + /* Allocate, initialize, and register this gpio_chip. */
> + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->chip.base = pdata->gpio_base;
> + gpio->chip.can_sleep = 1;
> +
> + /* NOTE: the OnSemi jlc1562b is also largely compatible with
> + * these parts, notably for output. It has a low-resolution
> + * DAC instead of pin change IRQs; and its inputs can be the
> + * result of comparators.
> + */
> +
> + /* 8574 addresses are 0x20..0x27; 8574a uses 0x38..0x3f;
> + * 9670, 9672, 9764, and 9764a use quite a variety.
> + *
> + * NOTE: we dont distinguish here between *4 and *4a parts.
Typo: don't.
> + */
> + if (strcmp(client->name, "pcf8574") == 0
> + || strcmp(client->name, "pca8574") == 0
> + || strcmp(client->name, "pca9670") == 0
> + || strcmp(client->name, "pca9672") == 0
> + || strcmp(client->name, "pca9674") == 0
> + ) {
> + gpio->chip.ngpio = 8;
> + gpio->chip.direction_input = pcf857x_input8;
> + gpio->chip.get = pcf857x_get8;
> + gpio->chip.direction_output = pcf857x_output8;
> + gpio->chip.set = pcf857x_set8;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE))
> + status = -EIO;
> +
> + /* fail if there's no chip present */
> + else
> + status = i2c_smbus_read_byte(client);
> +
> + /* '75/'75c addresses are 0x20..0x27, just like the '74;
> + * the '75c doesn't have a current source pulling high.
> + * 9671, 9673, and 9765 use quite a variety of addresses.
> + *
> + * NOTE: we dont distinguish here between 8575/8575a parts.
> + */
> + } else if (strcmp(client->name, "pcf8575") == 0
> + || strcmp(client->name, "pca8575") == 0
> + || strcmp(client->name, "pca9671") == 0
> + || strcmp(client->name, "pca9673") == 0
> + || strcmp(client->name, "pca9675") == 0
> + ) {
> + gpio->chip.ngpio = 16;
> + gpio->chip.direction_input = pcf857x_input16;
> + gpio->chip.get = pcf857x_get16;
> + gpio->chip.direction_output = pcf857x_output16;
> + gpio->chip.set = pcf857x_set16;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + status = -EIO;
> +
> + /* fail if there's no chip present */
> + else
> + status = i2c_read_le16(client);
> +
> + } else
> + status = -ENODEV;
> +
> + if (status < 0)
> + goto fail;
> +
> + gpio->chip.label = client->name;
> +
> + gpio->client = client;
> + i2c_set_clientdata(client, gpio);
> +
> + /* NOTE: these chips have strange "quasi-bidirectional" I/O pins.
> + * We can't actually know whether a pin is configured (a) as output
> + * and driving the signal low, or (b) as input and reporting a low
> + * value ... without knowing the last value written since the chip
> + * came out of reset (if any). We can't read the latched output.
> + *
> + * In short, the only reliable solution for setting up pin direction
> + * is to do it explicitly. The setup() method can do that.
> + *
> + * We use pdata->n_latch to avoid trouble. In the typical case it's
> + * left initialized to zero; our software copy of the "latch" then
> + * matches the chip's all-ones reset state. But some systems will
> + * need to drive some pins low, while avoiding transient glitches.
> + * Handle those cases by assigning n_latch to a nonzero value.
> + */
> + gpio->out = ~pdata->n_latch;
> +
> + status = gpiochip_add(&gpio->chip);
> + if (status < 0)
> + goto fail;
> +
> + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
> + * don't yet even try to use. Among other issues, the relevant
> + * genirq state isn't available to modular drivers; and most irq
> + * methods can't be called from sleeping contexts.
> + */
> +
> + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
> + gpio->chip.base,
> + gpio->chip.base + gpio->chip.ngpio - 1,
> + client->name,
> + client->irq ? " (irq ignored)" : "");
> +
> + /* Let platform code set up the GPIOs and their users.
> + * Now is the first time anyone can use them.
> + */
> + if (pdata->setup) {
> + status = pdata->setup(client,
> + gpio->chip.base, gpio->chip.ngpio,
> + pdata->context);
> + if (status < 0)
> + dev_err(&client->dev, "%s --> %d\n",
> + "setup", status);
Shouldn't this be degraded to dev_warn? The probe still succeeds. Or
keep dev_err but make the probe fail (in which case you'll probably
want to swap this block of code with the dev_info above.)
> + }
> +
> + return 0;
> +
> +fail:
> + dev_dbg(&client->dev, "probe error %d for '%s'\n",
> + status, client->name);
> + kfree(gpio);
> + return status;
> +}
> +
> +static int pcf857x_remove(struct i2c_client *client)
> +{
> + struct pcf857x_platform_data *pdata = client->dev.platform_data;
> + struct pcf857x *gpio = i2c_get_clientdata(client);
> + int status = 0;
> +
> + if (pdata->teardown) {
> + status = pdata->teardown(client,
> + gpio->chip.base, gpio->chip.ngpio,
> + pdata->context);
> + if (status < 0) {
> + dev_err(&client->dev, "%s --> %d\n",
> + "teardown", status);
> + return status;
> + }
> + }
> +
> + status = gpiochip_remove(&gpio->chip);
> + if (status == 0)
> + kfree(gpio);
> + else
> + dev_err(&client->dev, "%s --> %d\n", "remove", status);
> + return status;
> +}
> +
> +static struct i2c_driver pcf857x_driver = {
> + .driver = {
> + .name = "pcf857x",
> + .owner = THIS_MODULE,
> + },
> + .probe = pcf857x_probe,
> + .remove = pcf857x_remove,
> +};
> +
> +static int __init pcf857x_init(void)
> +{
> + return i2c_add_driver(&pcf857x_driver);
> +}
> +/* we want GPIOs to be ready at device_initcall() time */
> +subsys_initcall(pcf857x_init);
> +
> +static void __exit pcf857x_exit(void)
> +{
> + i2c_del_driver(&pcf857x_driver);
> +}
> +module_exit(pcf857x_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("David Brownell");
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ b/include/linux/i2c/pcf857x.h 2007-12-05 15:14:12.000000000 -0800
> @@ -0,0 +1,45 @@
> +#ifndef __LINUX_PCF857X_H
> +#define __LINUX_PCF857X_H
> +
> +/**
> + * struct pcf857x_platform_data - data to set up pcf857x driver
> + * @gpio_base: number of the chip's first GPIO
> + * @n_latch: optional bit-inverse of initial register value; if
> + * you leave this initialized to zero the driver will act
> + * like the chip was just reset
> + * @setup: optional callback issued once the GPIOs are valid
> + * @teardown: optional callback issued before the GPIOs are invalidated
> + * @context: optional parameter passed to setup() and teardown()
> + *
> + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
> + * the i2c_board_info used with the pcf875x driver must provide the
> + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
> + * platform_data (pointer to one of these structures) with at least
> + * the gpio_base value initialized.
> + *
> + * The @setup callback may be used with the kind of board-specific glue
> + * which hands the (now-valid) GPIOs to other drivers, or which puts
> + * devices in their initial states using these GPIOs.
> + *
> + * These GPIO chips are only "quasi-bidirectional"; read the chip specs
> + * to understand the behavior. They don't have separate registers to
> + * record which pins are used for input or output, record which output
> + * values are driven, or provide access to input values. That must be
> + * inferred by reading the chip's value and knowing the last value written
> + * to it. If you leave n_latch initialized to zero, that last written
> + * value is presumed to be all ones (as if the chip were just reset).
> + */
> +struct pcf857x_platform_data {
> + unsigned gpio_base;
> + unsigned n_latch;
> +
> + int (*setup)(struct i2c_client *client,
> + int gpio, unsigned ngpio,
> + void *context);
> + int (*teardown)(struct i2c_client *client,
> + int gpio, unsigned ngpio,
> + void *context);
> + void *context;
> +};
> +
> +#endif /* __LINUX_PCF857X_H */
The rest looks fine to me.
--
Jean Delvare
On Thursday 06 December 2007, Jean Delvare wrote:
> Also, I don't quite see what
> is supposed to make compatibility with the legacy drivers easier, nor
> how, not why it matters in the first place.
There's a clear either/or disjunction. No fuzzy/confusing middle ground.
> > +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
> > +{
> > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
> > + s32 value;
> > +
> > + value = i2c_smbus_read_byte(gpio->client);
> > + return (value < 0) ? 0 : (value & (1 << offset));
>
> This is no longer a boolean value, is that OK? I guess that it doesn't
> matter but maybe it should be documented (what GPIO drivers are allowed
> to return in these callback functions.)
Already documented -- as zero/nonzero, the original boolean model for C.
Anything else would be at least tristate, not boolean. :)
> > + /* Let platform code set up the GPIOs and their users.
> > + * Now is the first time anyone can use them.
> > + */
> > + if (pdata->setup) {
> > + status = pdata->setup(client,
> > + gpio->chip.base, gpio->chip.ngpio,
> > + pdata->context);
> > + if (status < 0)
> > + dev_err(&client->dev, "%s --> %d\n",
> > + "setup", status);
>
> Shouldn't this be degraded to dev_warn? The probe still succeeds. Or
> keep dev_err but make the probe fail (in which case you'll probably
> want to swap this block of code with the dev_info above.)
Good point.
> The rest looks fine to me.
Thanks for the comments. I'll send this in with the next batch
of gpiolib patches.
- Dave