* [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders
@ 2008-07-10 6:13 Eric Miao
[not found] ` <4875A893.3090402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 25+ messages in thread
From: Eric Miao @ 2008-07-10 6:13 UTC (permalink / raw)
To: i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel; +Cc: David Brownell, Jack Ren
Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
drivers/gpio/Kconfig | 22 +++
drivers/gpio/Makefile | 1 +
drivers/gpio/max732x.c | 342 +++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c/max732x.h | 19 +++
4 files changed, 384 insertions(+), 0 deletions(-)
create mode 100644 drivers/gpio/max732x.c
create mode 100644 include/linux/i2c/max732x.h
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index bbd2834..9f8d030 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -61,6 +61,28 @@ config GPIO_PCF857X
This driver provides an in-kernel interface to those GPIOs using
platform-neutral GPIO calls.
+config GPIO_MAX732X
+ tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders"
+ depends on I2C
+ help
+ Say yes here to support the MAX7319, MAX7320-7327 series of I2C
+ Port Expanders. Each IO port on these chips has a fixed role of
+ Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain
+ Input and Output (designed by 'P'). The combinations are listed
+ below:
+
+ MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O),
+ MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O)
+ MAX7326 (4I12O), MAX7327 (4P12O)
+
+ The board code has to specify the model to use, and the start
+ number for these GPIOs. Model specific information is calculated
+ automatically. Due to the fixed role of each port, configuration
+ of the port direction will be ignored and a warning be issued if
+ the corresponding port isn't applicable. And gpio_get_value() to
+ those output only ports will return the configured output status
+ instead of a real input.
+
comment "SPI GPIO expanders:"
config GPIO_MCP23S08
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index fdde992..814698c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o
obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o
obj-$(CONFIG_GPIO_PCA953X) += pca953x.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
+obj-$(CONFIG_GPIO_MAX732X) += max732x.o
diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c
new file mode 100644
index 0000000..b1b9b62
--- /dev/null
+++ b/drivers/gpio/max732x.c
@@ -0,0 +1,342 @@
+/*
+ * max732x.c - I2C Port Expander with 8/16 I/O
+ *
+ * Copyright (C) 2007 Marvell International Ltd.
+ * Copyright (C) 2008 Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
+ * Copyright (C) 2008 Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * Derived from drivers/gpio/pca953x.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/max732x.h>
+
+#include <asm/gpio.h>
+
+/* Each port of MAX732x (including MAX7319) falls into one of the
+ * following three types:
+ *
+ * - Push Pull Output
+ * - Input
+ * - Open Drain I/O
+ *
+ * designated by 'O', 'I' and 'P' individually according to MAXIM's
+ * datasheets.
+ *
+ * There are two groups of I/O ports, each group usually includes
+ * up to 8 I/O ports, and is accessed by different I2C address:
+ *
+ * - Group A : by I2C address 0b'110xxxx
+ * - Group B : by I2C address 0b'101xxxx
+ *
+ * where 'xxxx' is decided by the connections of pin AD2/AD0.
+ *
+ * Within each group of ports, there are five known combinations of
+ * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for
+ * the detailed organization of these ports.
+ *
+ * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16',
+ * and GPIOs from GROUP_A are numbered before those from GROUP_B
+ * (if there are two groups).
+ *
+ * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574,
+ * they are not supported by this driver.
+ */
+
+#define PORT_NONE 0x0 /* '/' No Port */
+#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */
+#define PORT_INPUT 0x2 /* 'I' Input Only */
+#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */
+
+#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */
+#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */
+#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */
+#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */
+#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */
+
+#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */
+#define GROUP_B(x) ((x) << 16) /* I2C Addr: 0b'101xxxx */
+
+static const struct i2c_device_id max732x_id[] = {
+ { "max7319", GROUP_A(IO_8I) },
+ { "max7320", GROUP_B(IO_8O) },
+ { "max7321", GROUP_A(IO_8P) },
+ { "max7322", GROUP_A(IO_4I4O) },
+ { "max7323", GROUP_A(IO_4P4O) },
+ { "max7324", GROUP_A(IO_8I) | GROUP_B(IO_8O) },
+ { "max7325", GROUP_A(IO_8P) | GROUP_B(IO_8O) },
+ { "max7326", GROUP_A(IO_4I4O) | GROUP_B(IO_8O) },
+ { "max7327", GROUP_A(IO_4P4O) | GROUP_B(IO_8O) },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, max732x_id);
+
+struct max732x_chip {
+ struct i2c_client *client;
+ struct gpio_chip gpio_chip;
+
+ unsigned gpio_start;
+ unsigned short addr_group_a;
+ unsigned short addr_group_b;
+
+ unsigned int mask_group_a;
+ unsigned int dir_input;
+ unsigned int dir_output;
+ uint8_t reg_out[2];
+};
+
+/* 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 max732x_write(struct max732x_chip *chip, int group_a, uint8_t val)
+{
+ struct i2c_client *client = chip->client;
+ int ret;
+
+ client->addr = group_a ? chip->addr_group_a : chip->addr_group_b;
+
+ ret = i2c_smbus_write_byte(client, val);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed writing\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int max732x_read(struct max732x_chip *chip, int group_a, uint8_t *val)
+{
+ struct i2c_client *client = chip->client;
+ int ret;
+
+ client->addr = group_a ? chip->addr_group_a : chip->addr_group_b;
+
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed reading\n");
+ return -EIO;
+ }
+
+ *val = (uint8_t)ret;
+ return 0;
+}
+
+static inline int is_group_a(struct max732x_chip *chip, unsigned off)
+{
+ return (1u << off) & chip->mask_group_a;
+}
+
+static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+ struct max732x_chip *chip;
+ uint8_t reg_val;
+ int ret;
+
+ chip = container_of(gc, struct max732x_chip, gpio_chip);
+
+ ret = max732x_read(chip, is_group_a(chip, off), ®_val);
+ if (ret < 0)
+ return 0;
+
+ return (reg_val & (1u << (off & 0x7))) ? 1 : 0;
+}
+
+static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
+{
+ struct max732x_chip *chip;
+ uint8_t reg_out, mask = 1u << (off & 0x7);
+ int ret;
+
+ chip = container_of(gc, struct max732x_chip, gpio_chip);
+
+ reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0];
+ reg_out = (val) ? reg_out | mask : reg_out & ~mask;
+
+ ret = max732x_write(chip, is_group_a(chip, off), reg_out);
+ if (ret < 0)
+ return;
+
+ /* update the shadow register then */
+ if (off > 7)
+ chip->reg_out[1] = reg_out;
+ else
+ chip->reg_out[0] = reg_out;
+}
+
+static int max732x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ struct max732x_chip *chip;
+ unsigned int mask = 1u << off;
+
+ chip = container_of(gc, struct max732x_chip, gpio_chip);
+
+ if ((mask & chip->dir_input) == 0)
+ pr_warning("%s: port %d is output only\n", __func__, off);
+
+ return 0;
+}
+
+static int max732x_gpio_direction_output(struct gpio_chip *gc,
+ unsigned off, int val)
+{
+ struct max732x_chip *chip;
+ unsigned int mask = 1u << off;
+
+ chip = container_of(gc, struct max732x_chip, gpio_chip);
+
+ if ((mask & chip->dir_output) == 0) {
+ pr_warning("%s: port %d is input only\n", __func__, off);
+ return 0;
+ }
+
+ max732x_gpio_set_value(gc, off, val);
+ return 0;
+}
+
+static void max732x_setup_gpio(struct max732x_chip *chip, uint32_t id_data)
+{
+ struct gpio_chip *gc = &chip->gpio_chip;
+ int i, port = 0, nr_port;
+
+ for (i = 0; i < 16; i++, id_data >>= 2) {
+ unsigned int mask = 1 << port;
+
+ switch (id_data & 0x3) {
+ case PORT_OUTPUT:
+ chip->dir_output |= mask;
+ break;
+ case PORT_INPUT:
+ chip->dir_input |= mask;
+ break;
+ case PORT_OPENDRAIN:
+ chip->dir_output |= mask;
+ chip->dir_input |= mask;
+ break;
+ default:
+ continue;
+ }
+
+ if (i < 8)
+ chip->mask_group_a |= mask;
+ port++;
+ }
+
+ nr_port = port;
+
+ if (nr_port > 7) {
+ max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]);
+ max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]);
+ } else
+ max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]);
+
+ gc->direction_input = max732x_gpio_direction_input;
+ gc->direction_output = max732x_gpio_direction_output;
+ gc->get = max732x_gpio_get_value;
+ gc->set = max732x_gpio_set_value;
+ gc->can_sleep = 1;
+
+ gc->base = chip->gpio_start;
+ gc->ngpio = port;
+ gc->label = chip->client->name;
+ gc->owner = THIS_MODULE;
+}
+
+static int __devinit max732x_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct max732x_platform_data *pdata;
+ struct max732x_chip *chip;
+ int ret;
+
+ pdata = client->dev.platform_data;
+ if (pdata == NULL)
+ return -ENODEV;
+
+ chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL);
+ if (chip == NULL)
+ return -ENOMEM;
+
+ chip->client = client;
+
+ chip->gpio_start = pdata->gpio_base;
+ chip->addr_group_a = (client->addr & 0x0f) | 0x60;
+ chip->addr_group_b = (client->addr & 0x0f) | 0x50;
+
+ max732x_setup_gpio(chip, id->driver_data);
+
+ ret = gpiochip_add(&chip->gpio_chip);
+ if (ret)
+ goto out_failed;
+
+ if (pdata->setup) {
+ ret = pdata->setup(client, chip->gpio_chip.base,
+ chip->gpio_chip.ngpio, pdata->context);
+ if (ret < 0)
+ dev_warn(&client->dev, "setup failed, %d\n", ret);
+ }
+
+ i2c_set_clientdata(client, chip);
+ return 0;
+
+out_failed:
+ kfree(chip);
+ return ret;
+}
+
+static int max732x_remove(struct i2c_client *client)
+{
+ struct max732x_platform_data *pdata = client->dev.platform_data;
+ struct max732x_chip *chip = i2c_get_clientdata(client);
+ int ret = 0;
+
+ if (pdata->teardown) {
+ ret = pdata->teardown(client, chip->gpio_chip.base,
+ chip->gpio_chip.ngpio, pdata->context);
+ 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, "%s failed, %d\n",
+ "gpiochip_remove()", ret);
+ return ret;
+ }
+
+ kfree(chip);
+ return 0;
+}
+
+static struct i2c_driver max732x_driver = {
+ .driver = {
+ .name = "max732x",
+ },
+ .probe = max732x_probe,
+ .remove = max732x_remove,
+ .id_table = max732x_id,
+};
+
+static int __init max732x_init(void)
+{
+ return i2c_add_driver(&max732x_driver);
+}
+module_init(max732x_init);
+
+static void __exit max732x_exit(void)
+{
+ i2c_del_driver(&max732x_driver);
+}
+module_exit(max732x_exit);
+
+MODULE_AUTHOR("Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>");
+MODULE_DESCRIPTION("GPIO expander driver for MAX732X");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c/max732x.h b/include/linux/i2c/max732x.h
new file mode 100644
index 0000000..e103366
--- /dev/null
+++ b/include/linux/i2c/max732x.h
@@ -0,0 +1,19 @@
+#ifndef __LINUX_I2C_MAX732X_H
+#define __LINUX_I2C_MAX732X_H
+
+/* platform data for the MAX732x 8/16-bit I/O expander driver */
+
+struct max732x_platform_data {
+ /* number of the first GPIO */
+ unsigned gpio_base;
+
+ void *context; /* param to setup/teardown */
+
+ int (*setup)(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *context);
+ int (*teardown)(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *context);
+};
+#endif /* __LINUX_I2C_MAX732X_H */
--
1.5.4.3
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply related [flat|nested] 25+ messages in thread[parent not found: <4875A893.3090402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <4875A893.3090402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2008-07-11 8:29 ` Jean Delvare [not found] ` <20080711102952.31d2d943-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-11 8:29 UTC (permalink / raw) To: Eric Miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel Hi Eric, David, On Thu, 10 Jul 2008 14:13:39 +0800, Eric Miao wrote: > > Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> David, maybe you should create an entry for the gpio subsystem in MAINTAINERS? > --- > drivers/gpio/Kconfig | 22 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/max732x.c | 342 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c/max732x.h | 19 +++ > 4 files changed, 384 insertions(+), 0 deletions(-) > create mode 100644 drivers/gpio/max732x.c > create mode 100644 include/linux/i2c/max732x.h > Quick review, essentially on the i2c side of things as I don't know much about the gpio subsystem: > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index bbd2834..9f8d030 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -61,6 +61,28 @@ config GPIO_PCF857X > This driver provides an in-kernel interface to those GPIOs using > platform-neutral GPIO calls. > > +config GPIO_MAX732X > + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" > + depends on I2C > + help > + Say yes here to support the MAX7319, MAX7320-7327 series of I2C > + Port Expanders. Each IO port on these chips has a fixed role of I have a personal preference for "I/O" over "IO", but maybe that's just me. > + Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain > + Input and Output (designed by 'P'). The combinations are listed > + below: 'O' for push-pull and 'P' for open-drain, this is brilliant! :( > + > + MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O), > + MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O) > + MAX7326 (4I12O), MAX7327 (4P12O) > + > + The board code has to specify the model to use, and the start > + number for these GPIOs. Model specific information is calculated > + automatically. Due to the fixed role of each port, configuration > + of the port direction will be ignored and a warning be issued if > + the corresponding port isn't applicable. And gpio_get_value() to > + those output only ports will return the configured output status > + instead of a real input. > + > comment "SPI GPIO expanders:" > > config GPIO_MCP23S08 > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index fdde992..814698c 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o > obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o > obj-$(CONFIG_GPIO_PCA953X) += pca953x.o > obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o > +obj-$(CONFIG_GPIO_MAX732X) += max732x.o Alphabetical order? > diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c > new file mode 100644 > index 0000000..b1b9b62 > --- /dev/null > +++ b/drivers/gpio/max732x.c > @@ -0,0 +1,342 @@ > +/* > + * max732x.c - I2C Port Expander with 8/16 I/O > + * > + * Copyright (C) 2007 Marvell International Ltd. > + * Copyright (C) 2008 Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > + * Copyright (C) 2008 Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > + * > + * Derived from drivers/gpio/pca953x.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/max732x.h> You also need to include <linux/slab.h> for kzalloc and kfree. > + > +#include <asm/gpio.h> > + > +/* Each port of MAX732x (including MAX7319) falls into one of the > + * following three types: > + * > + * - Push Pull Output > + * - Input > + * - Open Drain I/O > + * > + * designated by 'O', 'I' and 'P' individually according to MAXIM's > + * datasheets. > + * > + * There are two groups of I/O ports, each group usually includes > + * up to 8 I/O ports, and is accessed by different I2C address: "by a specific I2C address"? > + * > + * - Group A : by I2C address 0b'110xxxx > + * - Group B : by I2C address 0b'101xxxx > + * > + * where 'xxxx' is decided by the connections of pin AD2/AD0. AD2-AD0 (assuming there there is an AD1 pin) > + * > + * Within each group of ports, there are five known combinations of > + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for > + * the detailed organization of these ports. > + * > + * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16', > + * and GPIOs from GROUP_A are numbered before those from GROUP_B > + * (if there are two groups). > + * > + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, > + * they are not supported by this driver. > + */ > + > +#define PORT_NONE 0x0 /* '/' No Port */ You don't use this define anywhere. > +#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */ > +#define PORT_INPUT 0x2 /* 'I' Input Only */ > +#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */ > + > +#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */ > +#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */ > +#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */ > +#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */ > +#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */ > + > +#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */ The masking doesn't seem necessary. > +#define GROUP_B(x) ((x) << 16) /* I2C Addr: 0b'101xxxx */ > + > +static const struct i2c_device_id max732x_id[] = { > + { "max7319", GROUP_A(IO_8I) }, > + { "max7320", GROUP_B(IO_8O) }, > + { "max7321", GROUP_A(IO_8P) }, > + { "max7322", GROUP_A(IO_4I4O) }, > + { "max7323", GROUP_A(IO_4P4O) }, > + { "max7324", GROUP_A(IO_8I) | GROUP_B(IO_8O) }, > + { "max7325", GROUP_A(IO_8P) | GROUP_B(IO_8O) }, > + { "max7326", GROUP_A(IO_4I4O) | GROUP_B(IO_8O) }, > + { "max7327", GROUP_A(IO_4P4O) | GROUP_B(IO_8O) }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, max732x_id); > + > +struct max732x_chip { > + struct i2c_client *client; > + struct gpio_chip gpio_chip; > + > + unsigned gpio_start; > + unsigned short addr_group_a; > + unsigned short addr_group_b; > + > + unsigned int mask_group_a; > + unsigned int dir_input; > + unsigned int dir_output; > + uint8_t reg_out[2]; > +}; > + > +/* 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. > + */ With David's patches which will be merged in 2.6.27, you actually can in most cases: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-adapters-return-proper-error-codes.patch http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-core-return-proper-error-codes.patch So I would suggest that you do not overwrite the error codes in your driver. If you notice problems with the error codes that come from a specific i2c bus driver, please fix that bus driver instead. > +static int max732x_write(struct max732x_chip *chip, int group_a, uint8_t val) > +{ > + struct i2c_client *client = chip->client; > + int ret; > + > + client->addr = group_a ? chip->addr_group_a : chip->addr_group_b; This is prohibited. The i2c client is registered with i2c-core with a given address, which it marks as busy so that other drivers can't attach. Changing the client address on the fly defeats these safety checks. If you need more than one i2c_client, you can get the extra ones using i2c_new_dummy() at device probe time. The new at24 eeprom driver is doing this very well: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-at24-new-eeprom-driver.patch > + > + ret = i2c_smbus_write_byte(client, val); > + if (ret < 0) { > + dev_err(&client->dev, "failed writing\n"); > + return -EIO; > + } > + > + return 0; > +} > + > +static int max732x_read(struct max732x_chip *chip, int group_a, uint8_t *val) > +{ > + struct i2c_client *client = chip->client; > + int ret; > + > + client->addr = group_a ? chip->addr_group_a : chip->addr_group_b; Same problem here. > + > + ret = i2c_smbus_read_byte(client); > + if (ret < 0) { > + dev_err(&client->dev, "failed reading\n"); > + return -EIO; > + } > + > + *val = (uint8_t)ret; > + return 0; > +} > + > +static inline int is_group_a(struct max732x_chip *chip, unsigned off) > +{ > + return (1u << off) & chip->mask_group_a; > +} > + > +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off) > +{ > + struct max732x_chip *chip; > + uint8_t reg_val; > + int ret; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + ret = max732x_read(chip, is_group_a(chip, off), ®_val); > + if (ret < 0) > + return 0; > + > + return (reg_val & (1u << (off & 0x7))) ? 1 : 0; > +} > + > +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) > +{ > + struct max732x_chip *chip; > + uint8_t reg_out, mask = 1u << (off & 0x7); > + int ret; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0]; > + reg_out = (val) ? reg_out | mask : reg_out & ~mask; > + > + ret = max732x_write(chip, is_group_a(chip, off), reg_out); > + if (ret < 0) > + return; > + > + /* update the shadow register then */ > + if (off > 7) > + chip->reg_out[1] = reg_out; > + else > + chip->reg_out[0] = reg_out; > +} > + > +static int max732x_gpio_direction_input(struct gpio_chip *gc, unsigned off) > +{ > + struct max732x_chip *chip; > + unsigned int mask = 1u << off; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + if ((mask & chip->dir_input) == 0) > + pr_warning("%s: port %d is output only\n", __func__, off); > + > + return 0; > +} > + > +static int max732x_gpio_direction_output(struct gpio_chip *gc, > + unsigned off, int val) > +{ > + struct max732x_chip *chip; > + unsigned int mask = 1u << off; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + if ((mask & chip->dir_output) == 0) { > + pr_warning("%s: port %d is input only\n", __func__, off); > + return 0; > + } > + > + max732x_gpio_set_value(gc, off, val); > + return 0; > +} > + > +static void max732x_setup_gpio(struct max732x_chip *chip, uint32_t id_data) > +{ > + struct gpio_chip *gc = &chip->gpio_chip; > + int i, port = 0, nr_port; > + > + for (i = 0; i < 16; i++, id_data >>= 2) { > + unsigned int mask = 1 << port; > + > + switch (id_data & 0x3) { > + case PORT_OUTPUT: > + chip->dir_output |= mask; > + break; > + case PORT_INPUT: > + chip->dir_input |= mask; > + break; > + case PORT_OPENDRAIN: > + chip->dir_output |= mask; > + chip->dir_input |= mask; > + break; > + default: > + continue; > + } > + > + if (i < 8) > + chip->mask_group_a |= mask; > + port++; > + } > + > + nr_port = port; Why do you need 2 variables for that? > + > + if (nr_port > 7) { > + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); > + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); > + } else > + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); Isn't this better written: max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); if (nr_port > 7) max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); ? > + > + gc->direction_input = max732x_gpio_direction_input; > + gc->direction_output = max732x_gpio_direction_output; > + gc->get = max732x_gpio_get_value; > + gc->set = max732x_gpio_set_value; > + gc->can_sleep = 1; > + > + gc->base = chip->gpio_start; > + gc->ngpio = port; > + gc->label = chip->client->name; > + gc->owner = THIS_MODULE; > +} > + > +static int __devinit max732x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct max732x_platform_data *pdata; > + struct max732x_chip *chip; > + int ret; > + > + pdata = client->dev.platform_data; > + if (pdata == NULL) > + return -ENODEV; > + > + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + chip->client = client; > + > + chip->gpio_start = pdata->gpio_base; > + chip->addr_group_a = (client->addr & 0x0f) | 0x60; > + chip->addr_group_b = (client->addr & 0x0f) | 0x50; As written above, this needs some rework. For one thing, you should check that client->addr actually matches either chip->addr_group_a or chip->addr_group_b. For another, for chips which use 2 addresses, you must get a client for the second one by calling i2c_new_dummy (and i2c_unregister_device at remove time.) > + > + max732x_setup_gpio(chip, id->driver_data); > + > + ret = gpiochip_add(&chip->gpio_chip); > + if (ret) > + goto out_failed; > + > + if (pdata->setup) { > + ret = pdata->setup(client, chip->gpio_chip.base, > + chip->gpio_chip.ngpio, pdata->context); > + if (ret < 0) > + dev_warn(&client->dev, "setup failed, %d\n", ret); > + } > + > + i2c_set_clientdata(client, chip); > + return 0; > + > +out_failed: > + kfree(chip); > + return ret; > +} > + > +static int max732x_remove(struct i2c_client *client) > +{ > + struct max732x_platform_data *pdata = client->dev.platform_data; > + struct max732x_chip *chip = i2c_get_clientdata(client); > + int ret = 0; Initialization of ret isn't needed as far as I can tell. > + > + if (pdata->teardown) { > + ret = pdata->teardown(client, chip->gpio_chip.base, > + chip->gpio_chip.ngpio, pdata->context); > + 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, "%s failed, %d\n", > + "gpiochip_remove()", ret); > + return ret; > + } > + > + kfree(chip); > + return 0; > +} > + > +static struct i2c_driver max732x_driver = { > + .driver = { > + .name = "max732x", > + }, > + .probe = max732x_probe, > + .remove = max732x_remove, > + .id_table = max732x_id, > +}; > + > +static int __init max732x_init(void) > +{ > + return i2c_add_driver(&max732x_driver); > +} > +module_init(max732x_init); > + > +static void __exit max732x_exit(void) > +{ > + i2c_del_driver(&max732x_driver); > +} > +module_exit(max732x_exit); > + > +MODULE_AUTHOR("Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>"); > +MODULE_DESCRIPTION("GPIO expander driver for MAX732X"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c/max732x.h b/include/linux/i2c/max732x.h > new file mode 100644 > index 0000000..e103366 > --- /dev/null > +++ b/include/linux/i2c/max732x.h > @@ -0,0 +1,19 @@ > +#ifndef __LINUX_I2C_MAX732X_H > +#define __LINUX_I2C_MAX732X_H > + > +/* platform data for the MAX732x 8/16-bit I/O expander driver */ > + > +struct max732x_platform_data { > + /* number of the first GPIO */ > + unsigned gpio_base; > + > + void *context; /* param to setup/teardown */ > + > + int (*setup)(struct i2c_client *client, > + unsigned gpio, unsigned ngpio, > + void *context); > + int (*teardown)(struct i2c_client *client, > + unsigned gpio, unsigned ngpio, > + void *context); > +}; > +#endif /* __LINUX_I2C_MAX732X_H */ -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080711102952.31d2d943-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080711102952.31d2d943-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-11 8:57 ` eric miao [not found] ` <f17812d70807110157p5e421222sc9ef420ceb80970c-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-11 8:57 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Fri, Jul 11, 2008 at 4:29 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > Hi Eric, David, > > On Thu, 10 Jul 2008 14:13:39 +0800, Eric Miao wrote: >> >> Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> >> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > > David, maybe you should create an entry for the gpio subsystem in > MAINTAINERS? I don't mind, this patch actually requires joint review :-) >> +config GPIO_MAX732X >> + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" >> + depends on I2C >> + help >> + Say yes here to support the MAX7319, MAX7320-7327 series of I2C >> + Port Expanders. Each IO port on these chips has a fixed role of > > I have a personal preference for "I/O" over "IO", but maybe that's just > me. OK > >> + Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain >> + Input and Output (designed by 'P'). The combinations are listed >> + below: > > 'O' for push-pull and 'P' for open-drain, this is brilliant! :( This is how MAXIM named those pins in their datasheets, I think it's acceptable, at least to make thing clear in a simple way. >> @@ -7,3 +7,4 @@ obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o >> obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o >> obj-$(CONFIG_GPIO_PCA953X) += pca953x.o >> obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o >> +obj-$(CONFIG_GPIO_MAX732X) += max732x.o > > Alphabetical order? OK >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/i2c.h> >> +#include <linux/i2c/max732x.h> > > You also need to include <linux/slab.h> for kzalloc and kfree. OK >> + * There are two groups of I/O ports, each group usually includes >> + * up to 8 I/O ports, and is accessed by different I2C address: > > "by a specific I2C address"? OK > >> + * >> + * - Group A : by I2C address 0b'110xxxx >> + * - Group B : by I2C address 0b'101xxxx >> + * >> + * where 'xxxx' is decided by the connections of pin AD2/AD0. > > AD2-AD0 (assuming there there is an AD1 pin) Unfortunately, no AD1 pin > >> + * >> + * Within each group of ports, there are five known combinations of >> + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for >> + * the detailed organization of these ports. >> + * >> + * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16', >> + * and GPIOs from GROUP_A are numbered before those from GROUP_B >> + * (if there are two groups). >> + * >> + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, >> + * they are not supported by this driver. >> + */ >> + >> +#define PORT_NONE 0x0 /* '/' No Port */ > > You don't use this define anywhere. Just defined here to illustrate the purpose of the "0" here means NO PORT exist in that bit position, it helps people to better understand the port organization and initialization sequence > >> +#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */ >> +#define PORT_INPUT 0x2 /* 'I' Input Only */ >> +#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */ >> + >> +#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */ >> +#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */ >> +#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */ >> +#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */ >> +#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */ >> + >> +#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */ > > The masking doesn't seem necessary. > No it doesn't. But to keep here for the readability, i.e. to make things clear what upper/lower 16bit means >> +/* 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. >> + */ > > With David's patches which will be merged in 2.6.27, you actually can > in most cases: > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-adapters-return-proper-error-codes.patch > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-core-return-proper-error-codes.patch > > So I would suggest that you do not overwrite the error codes in your > driver. If you notice problems with the error codes that come from a > specific i2c bus driver, please fix that bus driver instead. > OK >> +static int max732x_write(struct max732x_chip *chip, int group_a, uint8_t val) >> +{ >> + struct i2c_client *client = chip->client; >> + int ret; >> + >> + client->addr = group_a ? chip->addr_group_a : chip->addr_group_b; > > This is prohibited. The i2c client is registered with i2c-core with a > given address, which it marks as busy so that other drivers can't > attach. Changing the client address on the fly defeats these safety > checks. > > If you need more than one i2c_client, you can get the extra ones using > i2c_new_dummy() at device probe time. The new at24 eeprom driver is > doing this very well: > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-at24-new-eeprom-driver.patch > OK >> + >> + nr_port = port; > > Why do you need 2 variables for that? Again, better readability :-) I can remove that if you mind. > >> + >> + if (nr_port > 7) { >> + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); >> + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); >> + } else >> + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); > > Isn't this better written: > > max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); > if (nr_port > 7) > max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); > > ? OK >> + chip->gpio_start = pdata->gpio_base; >> + chip->addr_group_a = (client->addr & 0x0f) | 0x60; >> + chip->addr_group_b = (client->addr & 0x0f) | 0x50; > > As written above, this needs some rework. For one thing, you should > check that client->addr actually matches either chip->addr_group_a or > chip->addr_group_b. For another, for chips which use 2 addresses, you > must get a client for the second one by calling i2c_new_dummy (and > i2c_unregister_device at remove time.) OK >> +static int max732x_remove(struct i2c_client *client) >> +{ >> + struct max732x_platform_data *pdata = client->dev.platform_data; >> + struct max732x_chip *chip = i2c_get_clientdata(client); >> + int ret = 0; > > Initialization of ret isn't needed as far as I can tell. OK Thanks, Jean. I'll come up with a patch updated soon. -- Cheers - eric _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807110157p5e421222sc9ef420ceb80970c-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807110157p5e421222sc9ef420ceb80970c-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-11 9:31 ` Jean Delvare [not found] ` <20080711113114.79d80212-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-11 9:31 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Fri, 11 Jul 2008 16:57:36 +0800, eric miao wrote: > On Fri, Jul 11, 2008 at 4:29 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > > On Thu, 10 Jul 2008 14:13:39 +0800, Eric Miao wrote: > >> + * - Group A : by I2C address 0b'110xxxx > >> + * - Group B : by I2C address 0b'101xxxx > >> + * > >> + * where 'xxxx' is decided by the connections of pin AD2/AD0. > > > > AD2-AD0 (assuming there there is an AD1 pin) > > Unfortunately, no AD1 pin Ah, these are 2 four-state address pins, I get it now. > >> (...) > >> +#define PORT_NONE 0x0 /* '/' No Port */ > > > > You don't use this define anywhere. > > Just defined here to illustrate the purpose of the "0" here means > NO PORT exist in that bit position, it helps people to better > understand the port organization and initialization sequence Could be a comment rather than a define, but up to you. > >> (...) > >> + nr_port = port; > > > > Why do you need 2 variables for that? > > Again, better readability :-) > > I can remove that if you mind. Personally I tend to think that it makes the readability worse not better. Looking at just the end of the function, I see: if (nr_port > 7) { (...) gc->ngpio = port; And I have to scroll up a bit to find out that "nr_port" and "port" always have the same value by construction. So indeed I would suggest to drop "port" and use "nr_port" everywhere for clarity. Oh, and one more thing as I just notice it: > +static inline int is_group_a(struct max732x_chip *chip, unsigned off) > +{ > + return (1u << off) & chip->mask_group_a; > +} Given the way you use it, can't you just define this function as: static inline int is_group_a(struct max732x_chip *chip, unsigned off) { return (off < 8); } ? As this is the only place where you use chip->mask_group_a, you would be able to get rid of it. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080711113114.79d80212-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080711113114.79d80212-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-11 9:39 ` eric miao [not found] ` <f17812d70807110239q6c175f5cn70db966681ae387d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-11 9:39 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel > > Oh, and one more thing as I just notice it: > >> +static inline int is_group_a(struct max732x_chip *chip, unsigned off) >> +{ >> + return (1u << off) & chip->mask_group_a; >> +} > > Given the way you use it, can't you just define this function as: > > static inline int is_group_a(struct max732x_chip *chip, unsigned off) > { > return (off < 8); > } > > ? As this is the only place where you use chip->mask_group_a, you would > be able to get rid of it. > I want to get rid of it either but I'm afraid not. (off < 8) doesn't necessarily mean it's in group_a, max7320 is an exception. > -- > Jean Delvare > -- Cheers - eric _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807110239q6c175f5cn70db966681ae387d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807110239q6c175f5cn70db966681ae387d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-11 9:48 ` eric miao [not found] ` <f17812d70807110248y7fab3328q59ea41084c491df-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-07-11 9:54 ` Jean Delvare 1 sibling, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-11 9:48 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel patch updated: >From 18d910f69c5408340661326f1ad1bcbb2c5bc6f6 Mon Sep 17 00:00:00 2001 From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Date: Thu, 10 Jul 2008 13:37:59 +0800 Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> --- drivers/gpio/Kconfig | 22 +++ drivers/gpio/Makefile | 1 + drivers/gpio/max732x.c | 361 +++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c/max732x.h | 19 +++ 4 files changed, 403 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/max732x.c create mode 100644 include/linux/i2c/max732x.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bbd2834..9f8d030 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -61,6 +61,28 @@ config GPIO_PCF857X This driver provides an in-kernel interface to those GPIOs using platform-neutral GPIO calls. +config GPIO_MAX732X + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" + depends on I2C + help + Say yes here to support the MAX7319, MAX7320-7327 series of I2C + Port Expanders. Each IO port on these chips has a fixed role of + Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain + Input and Output (designed by 'P'). The combinations are listed + below: + + MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O), + MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O) + MAX7326 (4I12O), MAX7327 (4P12O) + + The board code has to specify the model to use, and the start + number for these GPIOs. Model specific information is calculated + automatically. Due to the fixed role of each port, configuration + of the port direction will be ignored and a warning be issued if + the corresponding port isn't applicable. And gpio_get_value() to + those output only ports will return the configured output status + instead of a real input. + comment "SPI GPIO expanders:" config GPIO_MCP23S08 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fdde992..421a11b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o +obj-$(CONFIG_GPIO_MAX732X) += max732x.o obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o obj-$(CONFIG_GPIO_PCA953X) += pca953x.o obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c new file mode 100644 index 0000000..0f103f2 --- /dev/null +++ b/drivers/gpio/max732x.c @@ -0,0 +1,361 @@ +/* + * max732x.c - I2C Port Expander with 8/16 I/O + * + * Copyright (C) 2007 Marvell International Ltd. + * Copyright (C) 2008 Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * Copyright (C) 2008 Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * + * Derived from drivers/gpio/pca953x.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/slab.h> +#include <linux/i2c.h> +#include <linux/i2c/max732x.h> + +#include <asm/gpio.h> + +/* Each port of MAX732x (including MAX7319) falls into one of the + * following three types: + * + * - Push Pull Output + * - Input + * - Open Drain I/O + * + * designated by 'O', 'I' and 'P' individually according to MAXIM's + * datasheets. + * + * There are two groups of I/O ports, each group usually includes + * up to 8 I/O ports, and is accessed by a specific I2C address: + * + * - Group A : by I2C address 0b'110xxxx + * - Group B : by I2C address 0b'101xxxx + * + * where 'xxxx' is decided by the connections of pin AD2/AD0. + * + * Within each group of ports, there are five known combinations of + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for + * the detailed organization of these ports. + * + * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16', + * and GPIOs from GROUP_A are numbered before those from GROUP_B + * (if there are two groups). + * + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, + * they are not supported by this driver. + */ + +#define PORT_NONE 0x0 /* '/' No Port */ +#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */ +#define PORT_INPUT 0x2 /* 'I' Input Only */ +#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */ + +#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */ +#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */ +#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */ +#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */ +#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */ + +#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */ +#define GROUP_B(x) ((x) << 16) /* I2C Addr: 0b'101xxxx */ + +static const struct i2c_device_id max732x_id[] = { + { "max7319", GROUP_A(IO_8I) }, + { "max7320", GROUP_B(IO_8O) }, + { "max7321", GROUP_A(IO_8P) }, + { "max7322", GROUP_A(IO_4I4O) }, + { "max7323", GROUP_A(IO_4P4O) }, + { "max7324", GROUP_A(IO_8I) | GROUP_B(IO_8O) }, + { "max7325", GROUP_A(IO_8P) | GROUP_B(IO_8O) }, + { "max7326", GROUP_A(IO_4I4O) | GROUP_B(IO_8O) }, + { "max7327", GROUP_A(IO_4P4O) | GROUP_B(IO_8O) }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, max732x_id); + +struct max732x_chip { + struct gpio_chip gpio_chip; + + struct i2c_client *client[2]; + struct i2c_client *client_group_a; + struct i2c_client *client_group_b; + + unsigned gpio_start; + unsigned short addr_group_a; + unsigned short addr_group_b; + + unsigned int mask_group_a; + unsigned int dir_input; + unsigned int dir_output; + uint8_t reg_out[2]; +}; + +static int max732x_write(struct max732x_chip *chip, int group_a, uint8_t val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + ret = i2c_smbus_write_byte(client, val); + if (ret < 0) { + dev_err(&client->dev, "failed writing\n"); + return ret; + } + + return 0; +} + +static int max732x_read(struct max732x_chip *chip, int group_a, uint8_t *val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + ret = i2c_smbus_read_byte(client); + if (ret < 0) { + dev_err(&client->dev, "failed reading\n"); + return ret; + } + + *val = (uint8_t)ret; + return 0; +} + +static inline int is_group_a(struct max732x_chip *chip, unsigned off) +{ + return (1u << off) & chip->mask_group_a; +} + +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + uint8_t reg_val; + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + ret = max732x_read(chip, is_group_a(chip, off), ®_val); + if (ret < 0) + return 0; + + return (reg_val & (1u << (off & 0x7))) ? 1 : 0; +} + +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) +{ + struct max732x_chip *chip; + uint8_t reg_out, mask = 1u << (off & 0x7); + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0]; + reg_out = (val) ? reg_out | mask : reg_out & ~mask; + + ret = max732x_write(chip, is_group_a(chip, off), reg_out); + if (ret < 0) + return; + + /* update the shadow register then */ + if (off > 7) + chip->reg_out[1] = reg_out; + else + chip->reg_out[0] = reg_out; +} + +static int max732x_gpio_direction_input(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_input) == 0) + pr_warning("%s: port %d is output only\n", __func__, off); + + return 0; +} + +static int max732x_gpio_direction_output(struct gpio_chip *gc, + unsigned off, int val) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_output) == 0) { + pr_warning("%s: port %d is input only\n", __func__, off); + return 0; + } + + max732x_gpio_set_value(gc, off, val); + return 0; +} + +static void max732x_setup_gpio(struct max732x_chip *chip, uint32_t id_data) +{ + struct gpio_chip *gc = &chip->gpio_chip; + int i, port = 0; + + for (i = 0; i < 16; i++, id_data >>= 2) { + unsigned int mask = 1 << port; + + switch (id_data & 0x3) { + case PORT_OUTPUT: + chip->dir_output |= mask; + break; + case PORT_INPUT: + chip->dir_input |= mask; + break; + case PORT_OPENDRAIN: + chip->dir_output |= mask; + chip->dir_input |= mask; + break; + default: + continue; + } + + if (i < 8) + chip->mask_group_a |= mask; + port++; + } + + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); + if (port > 7) + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); + + gc->direction_input = max732x_gpio_direction_input; + gc->direction_output = max732x_gpio_direction_output; + gc->get = max732x_gpio_get_value; + gc->set = max732x_gpio_set_value; + gc->can_sleep = 1; + + gc->base = chip->gpio_start; + gc->ngpio = port; + gc->label = chip->client[0]->name; + gc->owner = THIS_MODULE; +} + +static int __devinit max732x_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct max732x_platform_data *pdata; + struct max732x_chip *chip; + int ret; + + pdata = client->dev.platform_data; + if (pdata == NULL) + return -ENODEV; + + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); + if (chip == NULL) + return -ENOMEM; + + chip->client[0] = client; + + switch (client->addr & 0x70) { + case 0x60: + chip->client[1] = i2c_new_dummy(client->adapter, + (client->addr & 0x0f) | 0x50); + chip->client_group_a = chip->client[0]; + chip->client_group_b = chip->client[1]; + break; + case 0x50: + chip->client[1] = i2c_new_dummy(client->adapter, + (client->addr & 0x0f) | 0x60); + chip->client_group_a = chip->client[1]; + chip->client_group_b = chip->client[0]; + break; + default: + dev_err(&client->dev, "invalid I2C address specified %02x\n", + client->addr); + kfree(chip); + return -EINVAL; + } + + chip->gpio_start = pdata->gpio_base; + + max732x_setup_gpio(chip, id->driver_data); + + ret = gpiochip_add(&chip->gpio_chip); + if (ret) + goto out_failed; + + if (pdata->setup) { + ret = pdata->setup(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + if (ret < 0) + dev_warn(&client->dev, "setup failed, %d\n", ret); + } + + i2c_set_clientdata(client, chip); + return 0; + +out_failed: + kfree(chip); + return ret; +} + +static int max732x_remove(struct i2c_client *client) +{ + struct max732x_platform_data *pdata = client->dev.platform_data; + struct max732x_chip *chip = i2c_get_clientdata(client); + int ret; + + if (pdata->teardown) { + ret = pdata->teardown(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + 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, "%s failed, %d\n", + "gpiochip_remove()", ret); + return ret; + } + + /* unregister the dummy i2c_client */ + i2c_unregister_device(chip->client[1]); + + kfree(chip); + return 0; +} + +static struct i2c_driver max732x_driver = { + .driver = { + .name = "max732x", + .owner = THIS_MODULE, + }, + .probe = max732x_probe, + .remove = max732x_remove, + .id_table = max732x_id, +}; + +static int __init max732x_init(void) +{ + return i2c_add_driver(&max732x_driver); +} +module_init(max732x_init); + +static void __exit max732x_exit(void) +{ + i2c_del_driver(&max732x_driver); +} +module_exit(max732x_exit); + +MODULE_AUTHOR("Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>"); +MODULE_DESCRIPTION("GPIO expander driver for MAX732X"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c/max732x.h b/include/linux/i2c/max732x.h new file mode 100644 index 0000000..e103366 --- /dev/null +++ b/include/linux/i2c/max732x.h @@ -0,0 +1,19 @@ +#ifndef __LINUX_I2C_MAX732X_H +#define __LINUX_I2C_MAX732X_H + +/* platform data for the MAX732x 8/16-bit I/O expander driver */ + +struct max732x_platform_data { + /* number of the first GPIO */ + unsigned gpio_base; + + void *context; /* param to setup/teardown */ + + int (*setup)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); + int (*teardown)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); +}; +#endif /* __LINUX_I2C_MAX732X_H */ -- 1.5.4.3 _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807110248y7fab3328q59ea41084c491df-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807110248y7fab3328q59ea41084c491df-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-11 11:15 ` Jean Delvare 2008-07-11 21:25 ` David Brownell 1 sibling, 0 replies; 25+ messages in thread From: Jean Delvare @ 2008-07-11 11:15 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel Hi Eric, On Fri, 11 Jul 2008 17:48:10 +0800, eric miao wrote: > +static int __devinit max732x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct max732x_platform_data *pdata; > + struct max732x_chip *chip; > + int ret; > + > + pdata = client->dev.platform_data; > + if (pdata == NULL) > + return -ENODEV; > + > + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + chip->client[0] = client; > + > + switch (client->addr & 0x70) { > + case 0x60: > + chip->client[1] = i2c_new_dummy(client->adapter, > + (client->addr & 0x0f) | 0x50); > + chip->client_group_a = chip->client[0]; > + chip->client_group_b = chip->client[1]; > + break; > + case 0x50: > + chip->client[1] = i2c_new_dummy(client->adapter, > + (client->addr & 0x0f) | 0x60); > + chip->client_group_a = chip->client[1]; > + chip->client_group_b = chip->client[0]; > + break; > + default: > + dev_err(&client->dev, "invalid I2C address specified %02x\n", > + client->addr); > + kfree(chip); > + return -EINVAL; > + } Not all chips use 2 I2C addresses. You must only call i2c_new_dummy for those which do. Otherwise you may prevent another driver from binding to its device. Also note that you don't use chip->client[0] except in this function, so I'm not sure why you store it. Having just chip->dummy_client to remember on which client to call i2c_unregister_device would be enough. > + > + chip->gpio_start = pdata->gpio_base; > + > + max732x_setup_gpio(chip, id->driver_data); > + > + ret = gpiochip_add(&chip->gpio_chip); > + if (ret) > + goto out_failed; > + > + if (pdata->setup) { > + ret = pdata->setup(client, chip->gpio_chip.base, > + chip->gpio_chip.ngpio, pdata->context); > + if (ret < 0) > + dev_warn(&client->dev, "setup failed, %d\n", ret); > + } > + > + i2c_set_clientdata(client, chip); > + return 0; > + > +out_failed: > + kfree(chip); > + return ret; > +} > + > +static int max732x_remove(struct i2c_client *client) max732x_probe is __devinit but this one isn't __devexit? > +{ > + struct max732x_platform_data *pdata = client->dev.platform_data; > + struct max732x_chip *chip = i2c_get_clientdata(client); > + int ret; > + > + if (pdata->teardown) { > + ret = pdata->teardown(client, chip->gpio_chip.base, > + chip->gpio_chip.ngpio, pdata->context); > + 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, "%s failed, %d\n", > + "gpiochip_remove()", ret); > + return ret; > + } > + > + /* unregister the dummy i2c_client */ > + i2c_unregister_device(chip->client[1]); > + > + kfree(chip); > + return 0; > +} -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807110248y7fab3328q59ea41084c491df-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-07-11 11:15 ` Jean Delvare @ 2008-07-11 21:25 ` David Brownell [not found] ` <200807111425.00961.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 1 sibling, 1 reply; 25+ messages in thread From: David Brownell @ 2008-07-11 21:25 UTC (permalink / raw) To: eric miao; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Friday 11 July 2008, eric miao wrote: > patch updated: > > >From 18d910f69c5408340661326f1ad1bcbb2c5bc6f6 Mon Sep 17 00:00:00 2001 > From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Date: Thu, 10 Jul 2008 13:37:59 +0800 > Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 > I2C Port Expanders > > Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > --- > drivers/gpio/Kconfig | 22 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/max732x.c | 361 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c/max732x.h | 19 +++ > 4 files changed, 403 insertions(+), 0 deletions(-) > create mode 100644 drivers/gpio/max732x.c > create mode 100644 include/linux/i2c/max732x.h > > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -61,6 +61,28 @@ config GPIO_PCF857X > This driver provides an in-kernel interface to those GPIOs using > platform-neutral GPIO calls. > > +config GPIO_MAX732X > + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" "MAX" should still precede "PCF" in the Kconfig. As it says: put drivers in the right section, and in alphabetical order. > --- /dev/null > +++ b/drivers/gpio/max732x.c > @@ -0,0 +1,361 @@ > ... > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/i2c.h> > +#include <linux/i2c/max732x.h> > + > +#include <asm/gpio.h> For consistency, make that <linux/gpio.h> and put it up above. I like to see blank lines delineating groups, like <linux/*> and <linux/i2c/*>, too. > + * Within each group of ports, there are five known combinations of > + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for > + * the detailed organization of these ports. Quirky little buggers, they are!! > + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, > + * they are not supported by this driver. In fact the data sheet for those parts says they're second sources for those PCF chips. Feel free to submit a patch updating Kconfig and the pcf857x driver accordingly ... :) > +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off) > +{ > + struct max732x_chip *chip; > + uint8_t reg_val; > + int ret; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + ret = max732x_read(chip, is_group_a(chip, off), ®_val); > + if (ret < 0) > + return 0; > + > + return (reg_val & (1u << (off & 0x7))) ? 1 : 0; Simpler: "reg_val & (the_mask)". The values here are zero/nonzero, not zero/one/undefined. > +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) > +{ > + struct max732x_chip *chip; > + uint8_t reg_out, mask = 1u << (off & 0x7); > + int ret; > + > + chip = container_of(gc, struct max732x_chip, gpio_chip); > + > + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0]; > + reg_out = (val) ? reg_out | mask : reg_out & ~mask; You should have some kind of locking to protect chip->reg_out[]. Hmm, I notice the pcf857x driver doesn't ... sigh. I can't quite see a way around a mutex or rwlock. The set_bit() clear_bit() calls would atomic, but you need the assignment to be atomic at the I2C chip, not just on the host side. > + > + ret = max732x_write(chip, is_group_a(chip, off), reg_out); > + if (ret < 0) > + return; > + > + /* update the shadow register then */ > + if (off > 7) > + chip->reg_out[1] = reg_out; > + else > + chip->reg_out[0] = reg_out; > +} Notice how two tasks setting direction for different GPIOs will both feel free to clobber the cached chip->reg_out[] value ... > + > + if ((mask & chip->dir_input) == 0) > + pr_warning("%s: port %d is output only\n", __func__, off); First problem: if it's an output-only signal, you should return a negative errno to anyone trying to use it as an input. (Likewise for the converse, trying to set an output-only pin as an input.) EACCES would seem appropriate; it's what open(2) returns when you try to write a read-only file, and these modes can't be written. Seocnd: you shouldn't use pr_*() calls when there's a relevant driver model node. Of course, in this case once you properly return a fault code, this merits at most a dev_dbg() call, since the caller can be expected to handle such errors somehow. > +static int __devinit max732x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct max732x_platform_data *pdata; > + struct max732x_chip *chip; > + int ret; > + > + pdata = client->dev.platform_data; > + if (pdata == NULL) > + return -ENODEV; > + > + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + chip->client[0] = client; > + > + switch (client->addr & 0x70) { > + case 0x60: > + chip->client[1] = i2c_new_dummy(client->adapter, > + (client->addr & 0x0f) | 0x50); > + chip->client_group_a = chip->client[0]; > + chip->client_group_b = chip->client[1]; > + break; > + case 0x50: > + chip->client[1] = i2c_new_dummy(client->adapter, > + (client->addr & 0x0f) | 0x60); > + chip->client_group_a = chip->client[1]; > + chip->client_group_b = chip->client[0]; > + break; Why not just insist the 0x5x address be registered/probed? This extra stuff is needless and confusing. _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <200807111425.00961.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <200807111425.00961.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-07-12 7:16 ` Jean Delvare [not found] ` <20080712091610.4ec242c3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-12 7:16 UTC (permalink / raw) To: David Brownell; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel Hi David, On Fri, 11 Jul 2008 14:25:00 -0700, David Brownell wrote: > On Friday 11 July 2008, eric miao wrote: > > --- /dev/null > > +++ b/drivers/gpio/max732x.c > > @@ -0,0 +1,361 @@ > > ... > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/i2c.h> > > +#include <linux/i2c/max732x.h> > > + > > +#include <asm/gpio.h> > > For consistency, make that <linux/gpio.h> and put it up above. > I like to see blank lines delineating groups, like <linux/*> > and <linux/i2c/*>, too. You probably want to fix all gpio drivers to do the same then, otherwise contributors will keep copying the wrong practice. > > (...) > > +static int __devinit max732x_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct max732x_platform_data *pdata; > > + struct max732x_chip *chip; > > + int ret; > > + > > + pdata = client->dev.platform_data; > > + if (pdata == NULL) > > + return -ENODEV; > > + > > + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); > > + if (chip == NULL) > > + return -ENOMEM; > > + > > + chip->client[0] = client; > > + > > + switch (client->addr & 0x70) { > > + case 0x60: > > + chip->client[1] = i2c_new_dummy(client->adapter, > > + (client->addr & 0x0f) | 0x50); > > + chip->client_group_a = chip->client[0]; > > + chip->client_group_b = chip->client[1]; > > + break; > > + case 0x50: > > + chip->client[1] = i2c_new_dummy(client->adapter, > > + (client->addr & 0x0f) | 0x60); > > + chip->client_group_a = chip->client[1]; > > + chip->client_group_b = chip->client[0]; > > + break; > > Why not just insist the 0x5x address be registered/probed? This > extra stuff is needless and confusing. Because some of the chips supported by this driver (max7319, max7321, max7322 and max7323) only use address 0x6x. Forcing 0x6x doesn't work either, because the max7320 only uses address 0x5x. I agree that the way the two clients are handled seems to be more complex than it needs to be, but the diversity of supported chips is such that even simplifying it as much as possible will still result in non-trivial code. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080712091610.4ec242c3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080712091610.4ec242c3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-12 7:46 ` David Brownell [not found] ` <200807120046.29389.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: David Brownell @ 2008-07-12 7:46 UTC (permalink / raw) To: Jean Delvare; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Saturday 12 July 2008, Jean Delvare wrote: > > > +#include <linux/i2c.h> > > > +#include <linux/i2c/max732x.h> > > > + > > > +#include <asm/gpio.h> > > > > For consistency, make that <linux/gpio.h> and put it up above. > > I like to see blank lines delineating groups, like <linux/*> > > and <linux/i2c/*>, too. > > You probably want to fix all gpio drivers to do the same then, > otherwise contributors will keep copying the wrong practice. When I get time. ;) > > > + chip->client[0] = client; > > > + > > > + switch (client->addr & 0x70) { > > > + case 0x60: > > > + chip->client[1] = i2c_new_dummy(client->adapter, > > > + (client->addr & 0x0f) | 0x50); > > > + chip->client_group_a = chip->client[0]; > > > + chip->client_group_b = chip->client[1]; > > > + break; > > > + case 0x50: > > > + chip->client[1] = i2c_new_dummy(client->adapter, > > > + (client->addr & 0x0f) | 0x60); > > > + chip->client_group_a = chip->client[1]; > > > + chip->client_group_b = chip->client[0]; > > > + break; > > > > Why not just insist the 0x5x address be registered/probed? This > > extra stuff is needless and confusing. > > Because some of the chips supported by this driver (max7319, max7321, > max7322 and max7323) only use address 0x6x. That's not the behavior implemented here though ... it's always creating a dummy, even when it's not needed. > I agree that the way the two clients are handled seems to be more > complex than it needs to be, but the diversity of supported chips is > such that even simplifying it as much as possible will still result in > non-trivial code. For what it *does* it **IS** more complex than it needs to be. There's no "seems" about it. For what was *intended*, it's evidently not yet complex enough. :( - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <200807120046.29389.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <200807120046.29389.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-07-12 7:53 ` Jean Delvare [not found] ` <20080712095300.1ba4b3a7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2008-07-13 6:04 ` eric miao 1 sibling, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-12 7:53 UTC (permalink / raw) To: David Brownell; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Sat, 12 Jul 2008 00:46:29 -0700, David Brownell wrote: > On Saturday 12 July 2008, Jean Delvare wrote: > > > Why not just insist the 0x5x address be registered/probed? This > > > extra stuff is needless and confusing. > > > > Because some of the chips supported by this driver (max7319, max7321, > > max7322 and max7323) only use address 0x6x. > > That's not the behavior implemented here though ... it's always > creating a dummy, even when it's not needed. Did you miss this comment of mine maybe? http://lists.lm-sensors.org/pipermail/i2c/2008-July/004267.html -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080712095300.1ba4b3a7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080712095300.1ba4b3a7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-12 21:42 ` David Brownell 2008-07-13 6:55 ` Jean Delvare 0 siblings, 1 reply; 25+ messages in thread From: David Brownell @ 2008-07-12 21:42 UTC (permalink / raw) To: Jean Delvare; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Saturday 12 July 2008, Jean Delvare wrote: > On Sat, 12 Jul 2008 00:46:29 -0700, David Brownell wrote: > > On Saturday 12 July 2008, Jean Delvare wrote: > > > > Why not just insist the 0x5x address be registered/probed? This > > > > extra stuff is needless and confusing. > > > > > > Because some of the chips supported by this driver (max7319, max7321, > > > max7322 and max7323) only use address 0x6x. > > > > That's not the behavior implemented here though ... it's always > > creating a dummy, even when it's not needed. > > Did you miss this comment of mine maybe? > http://lists.lm-sensors.org/pipermail/i2c/2008-July/004267.html Server not responding; can't tell. Using the MARC.info archive [1] what message is that? Are you disagreeing with what I said? [1] http://marc.info/?t=121567059700001&r=1&w=2 _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders 2008-07-12 21:42 ` David Brownell @ 2008-07-13 6:55 ` Jean Delvare 0 siblings, 0 replies; 25+ messages in thread From: Jean Delvare @ 2008-07-13 6:55 UTC (permalink / raw) To: David Brownell; +Cc: Jack Ren, i2c, linux-arm-kernel On Sat, 12 Jul 2008 14:42:30 -0700, David Brownell wrote: > On Saturday 12 July 2008, Jean Delvare wrote: > > On Sat, 12 Jul 2008 00:46:29 -0700, David Brownell wrote: > > > On Saturday 12 July 2008, Jean Delvare wrote: > > > > > Why not just insist the 0x5x address be registered/probed? This > > > > > extra stuff is needless and confusing. > > > > > > > > Because some of the chips supported by this driver (max7319, max7321, > > > > max7322 and max7323) only use address 0x6x. > > > > > > That's not the behavior implemented here though ... it's always > > > creating a dummy, even when it's not needed. > > > > Did you miss this comment of mine maybe? > > http://lists.lm-sensors.org/pipermail/i2c/2008-July/004267.html > > Server not responding; can't tell. Using the MARC.info archive [1] > what message is that? Are you disagreeing with what I said? > > [1] http://marc.info/?t=121567059700001&r=1&w=2 http://marc.info/?l=i2c&m=121577504211597&w=2 No, I didn't disagree. In fact you were merely repeating what I had already written ;) so I wanted you to read my comment so you knew you didn't have to convince me. -- Jean Delvare ------------------------------------------------------------------- List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <200807120046.29389.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-07-12 7:53 ` Jean Delvare @ 2008-07-13 6:04 ` eric miao [not found] ` <f17812d70807122304o533d8af9k1384db653b264912-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-13 6:04 UTC (permalink / raw) To: David Brownell; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel >From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001 From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Date: Thu, 10 Jul 2008 13:37:59 +0800 Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> --- Updated as below. i2c_new_dummy() is called _only_ when necessary (there's 8-bit IO expander). mutex is used to protect the update to chip->reg_out[]. Actually, I don't quite like the idea of i2c_new_dummy(), which creates several i2c_client for one device. Due to: 1. i2c_client should really be 1:1 with the device 2. a dummy i2c_client just wastes another several bytes 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be monitored by the I2C chips at startup to decide the connections of AD2/AD0 pins to GND/VCC/SCL/SDA, so actually, even if the chip is finally decided at, say 0x56, no sane hardware designers will put another chip whose address falls between 0x50-0x5F together with such a max732x chip, ugly, but true. One i2c_client requesting a group of address could somehow be more reasonable, and requested address can be changed at run-time. Anyway, here's the one for i2c_new_dummy(). drivers/gpio/Kconfig | 22 +++ drivers/gpio/Makefile | 1 + drivers/gpio/max732x.c | 380 +++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c/max732x.h | 19 ++ 4 files changed, 422 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/max732x.c create mode 100644 include/linux/i2c/max732x.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bbd2834..b8a60fb 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -27,6 +27,28 @@ config DEBUG_GPIO comment "I2C GPIO expanders:" +config GPIO_MAX732X + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" + depends on I2C + help + Say yes here to support the MAX7319, MAX7320-7327 series of I2C + Port Expanders. Each IO port on these chips has a fixed role of + Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain + Input and Output (designed by 'P'). The combinations are listed + below: + + MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O), + MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O) + MAX7326 (4I12O), MAX7327 (4P12O) + + The board code has to specify the model to use, and the start + number for these GPIOs. Model specific information is calculated + automatically. Due to the fixed role of each port, configuration + of the port direction will be ignored and a warning be issued if + the corresponding port isn't applicable. And gpio_get_value() to + those output only ports will return the configured output status + instead of a real input. + config GPIO_PCA953X tristate "PCA953x I/O ports" depends on I2C diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fdde992..421a11b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o +obj-$(CONFIG_GPIO_MAX732X) += max732x.o obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o obj-$(CONFIG_GPIO_PCA953X) += pca953x.o obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c new file mode 100644 index 0000000..187511e --- /dev/null +++ b/drivers/gpio/max732x.c @@ -0,0 +1,380 @@ +/* + * max732x.c - I2C Port Expander with 8/16 I/O + * + * Copyright (C) 2007 Marvell International Ltd. + * Copyright (C) 2008 Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * Copyright (C) 2008 Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * + * Derived from drivers/gpio/pca953x.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/slab.h> +#include <linux/string.h> +#include <linux/gpio.h> + +#include <linux/i2c.h> +#include <linux/i2c/max732x.h> + +/* Each port of MAX732x (including MAX7319) falls into one of the + * following three types: + * + * - Push Pull Output + * - Input + * - Open Drain I/O + * + * designated by 'O', 'I' and 'P' individually according to MAXIM's + * datasheets. + * + * There are two groups of I/O ports, each group usually includes + * up to 8 I/O ports, and is accessed by a specific I2C address: + * + * - Group A : by I2C address 0b'110xxxx + * - Group B : by I2C address 0b'101xxxx + * + * where 'xxxx' is decided by the connections of pin AD2/AD0. + * + * Within each group of ports, there are five known combinations of + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for + * the detailed organization of these ports. + * + * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16', + * and GPIOs from GROUP_A are numbered before those from GROUP_B + * (if there are two groups). + * + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, + * they are not supported by this driver. + */ + +#define PORT_NONE 0x0 /* '/' No Port */ +#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */ +#define PORT_INPUT 0x2 /* 'I' Input Only */ +#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */ + +#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */ +#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */ +#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */ +#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */ +#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */ + +#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */ +#define GROUP_B(x) ((x) << 16) /* I2C Addr: 0b'101xxxx */ + +static const struct i2c_device_id max732x_id[] = { + { "max7319", GROUP_A(IO_8I) }, + { "max7320", GROUP_B(IO_8O) }, + { "max7321", GROUP_A(IO_8P) }, + { "max7322", GROUP_A(IO_4I4O) }, + { "max7323", GROUP_A(IO_4P4O) }, + { "max7324", GROUP_A(IO_8I) | GROUP_B(IO_8O) }, + { "max7325", GROUP_A(IO_8P) | GROUP_B(IO_8O) }, + { "max7326", GROUP_A(IO_4I4O) | GROUP_B(IO_8O) }, + { "max7327", GROUP_A(IO_4P4O) | GROUP_B(IO_8O) }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, max732x_id); + +struct max732x_chip { + struct gpio_chip gpio_chip; + + struct i2c_client *client_dummy; + struct i2c_client *client_group_a; + struct i2c_client *client_group_b; + + unsigned int mask_group_a; + unsigned int dir_input; + unsigned int dir_output; + + struct mutex lock; + uint8_t reg_out[2]; +}; + +static int max732x_write(struct max732x_chip *chip, int group_a, uint8_t val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + BUG_ON(client == NULL); + + ret = i2c_smbus_write_byte(client, val); + if (ret < 0) { + dev_err(&client->dev, "failed writing\n"); + return ret; + } + + return 0; +} + +static int max732x_read(struct max732x_chip *chip, int group_a, uint8_t *val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + BUG_ON(client == NULL); + + ret = i2c_smbus_read_byte(client); + if (ret < 0) { + dev_err(&client->dev, "failed reading\n"); + return ret; + } + + *val = (uint8_t)ret; + return 0; +} + +static inline int is_group_a(struct max732x_chip *chip, unsigned off) +{ + return (1u << off) & chip->mask_group_a; +} + +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + uint8_t reg_val; + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + ret = max732x_read(chip, is_group_a(chip, off), ®_val); + if (ret < 0) + return 0; + + return reg_val & (1u << (off & 0x7)); +} + +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) +{ + struct max732x_chip *chip; + uint8_t reg_out, mask = 1u << (off & 0x7); + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + mutex_lock(&chip->lock); + + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0]; + reg_out = (val) ? reg_out | mask : reg_out & ~mask; + + ret = max732x_write(chip, is_group_a(chip, off), reg_out); + if (ret < 0) + goto out; + + /* update the shadow register then */ + if (off > 7) + chip->reg_out[1] = reg_out; + else + chip->reg_out[0] = reg_out; +out: + mutex_unlock(&chip->lock); +} + +static int max732x_gpio_direction_input(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_input) == 0) { + pr_warning("%s: port %d is output only\n", __func__, off); + return -EACCES; + } + + return 0; +} + +static int max732x_gpio_direction_output(struct gpio_chip *gc, + unsigned off, int val) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_output) == 0) { + pr_warning("%s: port %d is input only\n", __func__, off); + return -EACCES; + } + + max732x_gpio_set_value(gc, off, val); + return 0; +} + +static int __devinit max732x_setup_gpio(struct max732x_chip *chip, + const struct i2c_device_id *id, + unsigned gpio_start) +{ + struct gpio_chip *gc = &chip->gpio_chip; + uint32_t id_data = id->driver_data; + int i, port = 0; + + for (i = 0; i < 16; i++, id_data >>= 2) { + unsigned int mask = 1 << port; + + switch (id_data & 0x3) { + case PORT_OUTPUT: + chip->dir_output |= mask; + break; + case PORT_INPUT: + chip->dir_input |= mask; + break; + case PORT_OPENDRAIN: + chip->dir_output |= mask; + chip->dir_input |= mask; + break; + default: + continue; + } + + if (i < 8) + chip->mask_group_a |= mask; + port++; + } + + gc->direction_input = max732x_gpio_direction_input; + gc->direction_output = max732x_gpio_direction_output; + gc->get = max732x_gpio_get_value; + gc->set = max732x_gpio_set_value; + gc->can_sleep = 1; + + gc->base = gpio_start; + gc->ngpio = port; + gc->label = "max732x"; + gc->owner = THIS_MODULE; + + return port; +} + +static int __devinit max732x_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct max732x_platform_data *pdata; + struct max732x_chip *chip; + struct i2c_client *c; + uint16_t addr_a, addr_b; + int ret, nr_port; + + pdata = client->dev.platform_data; + if (pdata == NULL) + return -ENODEV; + + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); + if (chip == NULL) + return -ENOMEM; + + nr_port = max732x_setup_gpio(chip, id, pdata->gpio_base); + + addr_a = (client->addr & 0x0f) | 0x60; + addr_b = (client->addr & 0x0f) | 0x50; + + switch (client->addr & 0x70) { + case 0x60: + chip->client_group_a = client; + if (nr_port > 7) { + c = i2c_new_dummy(client->adapter, addr_b); + chip->client_group_b = chip->client_dummy = c; + } + break; + case 0x50: + chip->client_group_b = client; + if (nr_port > 7) { + c = i2c_new_dummy(client->adapter, addr_a); + chip->client_group_a = chip->client_dummy = c; + } + break; + default: + dev_err(&client->dev, "invalid I2C address specified %02x\n", + client->addr); + return -EINVAL; + } + + mutex_init(&chip->lock); + + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); + if (nr_port > 7) + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); + + ret = gpiochip_add(&chip->gpio_chip); + if (ret) + goto out_failed; + + if (pdata->setup) { + ret = pdata->setup(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + if (ret < 0) + dev_warn(&client->dev, "setup failed, %d\n", ret); + } + + i2c_set_clientdata(client, chip); + return 0; + +out_failed: + kfree(chip); + return ret; +} + +static int __devexit max732x_remove(struct i2c_client *client) +{ + struct max732x_platform_data *pdata = client->dev.platform_data; + struct max732x_chip *chip = i2c_get_clientdata(client); + int ret; + + if (pdata->teardown) { + ret = pdata->teardown(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + 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, "%s failed, %d\n", + "gpiochip_remove()", ret); + return ret; + } + + /* unregister the dummy i2c_client */ + if (chip->client_dummy) + i2c_unregister_device(chip->client_dummy); + + kfree(chip); + return 0; +} + +static struct i2c_driver max732x_driver = { + .driver = { + .name = "max732x", + .owner = THIS_MODULE, + }, + .probe = max732x_probe, + .remove = __devexit_p(max732x_remove), + .id_table = max732x_id, +}; + +static int __init max732x_init(void) +{ + return i2c_add_driver(&max732x_driver); +} +module_init(max732x_init); + +static void __exit max732x_exit(void) +{ + i2c_del_driver(&max732x_driver); +} +module_exit(max732x_exit); + +MODULE_AUTHOR("Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>"); +MODULE_DESCRIPTION("GPIO expander driver for MAX732X"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c/max732x.h b/include/linux/i2c/max732x.h new file mode 100644 index 0000000..e103366 --- /dev/null +++ b/include/linux/i2c/max732x.h @@ -0,0 +1,19 @@ +#ifndef __LINUX_I2C_MAX732X_H +#define __LINUX_I2C_MAX732X_H + +/* platform data for the MAX732x 8/16-bit I/O expander driver */ + +struct max732x_platform_data { + /* number of the first GPIO */ + unsigned gpio_base; + + void *context; /* param to setup/teardown */ + + int (*setup)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); + int (*teardown)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); +}; +#endif /* __LINUX_I2C_MAX732X_H */ -- 1.5.4.3 >From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001 From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Date: Thu, 10 Jul 2008 13:37:59 +0800 Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> --- drivers/gpio/Kconfig | 22 +++ drivers/gpio/Makefile | 1 + drivers/gpio/max732x.c | 380 +++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c/max732x.h | 19 ++ 4 files changed, 422 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/max732x.c create mode 100644 include/linux/i2c/max732x.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bbd2834..b8a60fb 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -27,6 +27,28 @@ config DEBUG_GPIO comment "I2C GPIO expanders:" +config GPIO_MAX732X + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" + depends on I2C + help + Say yes here to support the MAX7319, MAX7320-7327 series of I2C + Port Expanders. Each IO port on these chips has a fixed role of + Input (designated by 'I'), Push-Pull Output ('O'), or Open-Drain + Input and Output (designed by 'P'). The combinations are listed + below: + + MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O), + MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O) + MAX7326 (4I12O), MAX7327 (4P12O) + + The board code has to specify the model to use, and the start + number for these GPIOs. Model specific information is calculated + automatically. Due to the fixed role of each port, configuration + of the port direction will be ignored and a warning be issued if + the corresponding port isn't applicable. And gpio_get_value() to + those output only ports will return the configured output status + instead of a real input. + config GPIO_PCA953X tristate "PCA953x I/O ports" depends on I2C diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fdde992..421a11b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o +obj-$(CONFIG_GPIO_MAX732X) += max732x.o obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o obj-$(CONFIG_GPIO_PCA953X) += pca953x.o obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c new file mode 100644 index 0000000..187511e --- /dev/null +++ b/drivers/gpio/max732x.c @@ -0,0 +1,380 @@ +/* + * max732x.c - I2C Port Expander with 8/16 I/O + * + * Copyright (C) 2007 Marvell International Ltd. + * Copyright (C) 2008 Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * Copyright (C) 2008 Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> + * + * Derived from drivers/gpio/pca953x.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/slab.h> +#include <linux/string.h> +#include <linux/gpio.h> + +#include <linux/i2c.h> +#include <linux/i2c/max732x.h> + +/* Each port of MAX732x (including MAX7319) falls into one of the + * following three types: + * + * - Push Pull Output + * - Input + * - Open Drain I/O + * + * designated by 'O', 'I' and 'P' individually according to MAXIM's + * datasheets. + * + * There are two groups of I/O ports, each group usually includes + * up to 8 I/O ports, and is accessed by a specific I2C address: + * + * - Group A : by I2C address 0b'110xxxx + * - Group B : by I2C address 0b'101xxxx + * + * where 'xxxx' is decided by the connections of pin AD2/AD0. + * + * Within each group of ports, there are five known combinations of + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for + * the detailed organization of these ports. + * + * GPIO numbers start from 'gpio_base + 0' to 'gpio_base + 8/16', + * and GPIOs from GROUP_A are numbered before those from GROUP_B + * (if there are two groups). + * + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, + * they are not supported by this driver. + */ + +#define PORT_NONE 0x0 /* '/' No Port */ +#define PORT_OUTPUT 0x1 /* 'O' Push-Pull, Output Only */ +#define PORT_INPUT 0x2 /* 'I' Input Only */ +#define PORT_OPENDRAIN 0x3 /* 'P' Open-Drain, I/O */ + +#define IO_4I4O 0x5AA5 /* O7 O6 I5 I4 I3 I2 O1 O0 */ +#define IO_4P4O 0x5FF5 /* O7 O6 P5 P4 P3 P2 O1 O0 */ +#define IO_8I 0xAAAA /* I7 I6 I5 I4 I3 I2 I1 I0 */ +#define IO_8P 0xFFFF /* P7 P6 P5 P4 P3 P2 P1 P0 */ +#define IO_8O 0x5555 /* O7 O6 O5 O4 O3 O2 O1 O0 */ + +#define GROUP_A(x) ((x) & 0xffff) /* I2C Addr: 0b'110xxxx */ +#define GROUP_B(x) ((x) << 16) /* I2C Addr: 0b'101xxxx */ + +static const struct i2c_device_id max732x_id[] = { + { "max7319", GROUP_A(IO_8I) }, + { "max7320", GROUP_B(IO_8O) }, + { "max7321", GROUP_A(IO_8P) }, + { "max7322", GROUP_A(IO_4I4O) }, + { "max7323", GROUP_A(IO_4P4O) }, + { "max7324", GROUP_A(IO_8I) | GROUP_B(IO_8O) }, + { "max7325", GROUP_A(IO_8P) | GROUP_B(IO_8O) }, + { "max7326", GROUP_A(IO_4I4O) | GROUP_B(IO_8O) }, + { "max7327", GROUP_A(IO_4P4O) | GROUP_B(IO_8O) }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, max732x_id); + +struct max732x_chip { + struct gpio_chip gpio_chip; + + struct i2c_client *client_dummy; + struct i2c_client *client_group_a; + struct i2c_client *client_group_b; + + unsigned int mask_group_a; + unsigned int dir_input; + unsigned int dir_output; + + struct mutex lock; + uint8_t reg_out[2]; +}; + +static int max732x_write(struct max732x_chip *chip, int group_a, uint8_t val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + BUG_ON(client == NULL); + + ret = i2c_smbus_write_byte(client, val); + if (ret < 0) { + dev_err(&client->dev, "failed writing\n"); + return ret; + } + + return 0; +} + +static int max732x_read(struct max732x_chip *chip, int group_a, uint8_t *val) +{ + struct i2c_client *client; + int ret; + + client = group_a ? chip->client_group_a : chip->client_group_b; + + BUG_ON(client == NULL); + + ret = i2c_smbus_read_byte(client); + if (ret < 0) { + dev_err(&client->dev, "failed reading\n"); + return ret; + } + + *val = (uint8_t)ret; + return 0; +} + +static inline int is_group_a(struct max732x_chip *chip, unsigned off) +{ + return (1u << off) & chip->mask_group_a; +} + +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + uint8_t reg_val; + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + ret = max732x_read(chip, is_group_a(chip, off), ®_val); + if (ret < 0) + return 0; + + return reg_val & (1u << (off & 0x7)); +} + +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) +{ + struct max732x_chip *chip; + uint8_t reg_out, mask = 1u << (off & 0x7); + int ret; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + mutex_lock(&chip->lock); + + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0]; + reg_out = (val) ? reg_out | mask : reg_out & ~mask; + + ret = max732x_write(chip, is_group_a(chip, off), reg_out); + if (ret < 0) + goto out; + + /* update the shadow register then */ + if (off > 7) + chip->reg_out[1] = reg_out; + else + chip->reg_out[0] = reg_out; +out: + mutex_unlock(&chip->lock); +} + +static int max732x_gpio_direction_input(struct gpio_chip *gc, unsigned off) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_input) == 0) { + pr_warning("%s: port %d is output only\n", __func__, off); + return -EACCES; + } + + return 0; +} + +static int max732x_gpio_direction_output(struct gpio_chip *gc, + unsigned off, int val) +{ + struct max732x_chip *chip; + unsigned int mask = 1u << off; + + chip = container_of(gc, struct max732x_chip, gpio_chip); + + if ((mask & chip->dir_output) == 0) { + pr_warning("%s: port %d is input only\n", __func__, off); + return -EACCES; + } + + max732x_gpio_set_value(gc, off, val); + return 0; +} + +static int __devinit max732x_setup_gpio(struct max732x_chip *chip, + const struct i2c_device_id *id, + unsigned gpio_start) +{ + struct gpio_chip *gc = &chip->gpio_chip; + uint32_t id_data = id->driver_data; + int i, port = 0; + + for (i = 0; i < 16; i++, id_data >>= 2) { + unsigned int mask = 1 << port; + + switch (id_data & 0x3) { + case PORT_OUTPUT: + chip->dir_output |= mask; + break; + case PORT_INPUT: + chip->dir_input |= mask; + break; + case PORT_OPENDRAIN: + chip->dir_output |= mask; + chip->dir_input |= mask; + break; + default: + continue; + } + + if (i < 8) + chip->mask_group_a |= mask; + port++; + } + + gc->direction_input = max732x_gpio_direction_input; + gc->direction_output = max732x_gpio_direction_output; + gc->get = max732x_gpio_get_value; + gc->set = max732x_gpio_set_value; + gc->can_sleep = 1; + + gc->base = gpio_start; + gc->ngpio = port; + gc->label = "max732x"; + gc->owner = THIS_MODULE; + + return port; +} + +static int __devinit max732x_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct max732x_platform_data *pdata; + struct max732x_chip *chip; + struct i2c_client *c; + uint16_t addr_a, addr_b; + int ret, nr_port; + + pdata = client->dev.platform_data; + if (pdata == NULL) + return -ENODEV; + + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); + if (chip == NULL) + return -ENOMEM; + + nr_port = max732x_setup_gpio(chip, id, pdata->gpio_base); + + addr_a = (client->addr & 0x0f) | 0x60; + addr_b = (client->addr & 0x0f) | 0x50; + + switch (client->addr & 0x70) { + case 0x60: + chip->client_group_a = client; + if (nr_port > 7) { + c = i2c_new_dummy(client->adapter, addr_b); + chip->client_group_b = chip->client_dummy = c; + } + break; + case 0x50: + chip->client_group_b = client; + if (nr_port > 7) { + c = i2c_new_dummy(client->adapter, addr_a); + chip->client_group_a = chip->client_dummy = c; + } + break; + default: + dev_err(&client->dev, "invalid I2C address specified %02x\n", + client->addr); + return -EINVAL; + } + + mutex_init(&chip->lock); + + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); + if (nr_port > 7) + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); + + ret = gpiochip_add(&chip->gpio_chip); + if (ret) + goto out_failed; + + if (pdata->setup) { + ret = pdata->setup(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + if (ret < 0) + dev_warn(&client->dev, "setup failed, %d\n", ret); + } + + i2c_set_clientdata(client, chip); + return 0; + +out_failed: + kfree(chip); + return ret; +} + +static int __devexit max732x_remove(struct i2c_client *client) +{ + struct max732x_platform_data *pdata = client->dev.platform_data; + struct max732x_chip *chip = i2c_get_clientdata(client); + int ret; + + if (pdata->teardown) { + ret = pdata->teardown(client, chip->gpio_chip.base, + chip->gpio_chip.ngpio, pdata->context); + 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, "%s failed, %d\n", + "gpiochip_remove()", ret); + return ret; + } + + /* unregister the dummy i2c_client */ + if (chip->client_dummy) + i2c_unregister_device(chip->client_dummy); + + kfree(chip); + return 0; +} + +static struct i2c_driver max732x_driver = { + .driver = { + .name = "max732x", + .owner = THIS_MODULE, + }, + .probe = max732x_probe, + .remove = __devexit_p(max732x_remove), + .id_table = max732x_id, +}; + +static int __init max732x_init(void) +{ + return i2c_add_driver(&max732x_driver); +} +module_init(max732x_init); + +static void __exit max732x_exit(void) +{ + i2c_del_driver(&max732x_driver); +} +module_exit(max732x_exit); + +MODULE_AUTHOR("Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>"); +MODULE_DESCRIPTION("GPIO expander driver for MAX732X"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c/max732x.h b/include/linux/i2c/max732x.h new file mode 100644 index 0000000..e103366 --- /dev/null +++ b/include/linux/i2c/max732x.h @@ -0,0 +1,19 @@ +#ifndef __LINUX_I2C_MAX732X_H +#define __LINUX_I2C_MAX732X_H + +/* platform data for the MAX732x 8/16-bit I/O expander driver */ + +struct max732x_platform_data { + /* number of the first GPIO */ + unsigned gpio_base; + + void *context; /* param to setup/teardown */ + + int (*setup)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); + int (*teardown)(struct i2c_client *client, + unsigned gpio, unsigned ngpio, + void *context); +}; +#endif /* __LINUX_I2C_MAX732X_H */ -- 1.5.4.3 _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807122304o533d8af9k1384db653b264912-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807122304o533d8af9k1384db653b264912-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-13 7:20 ` Jean Delvare [not found] ` <20080713092050.6dffd8d3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2008-07-13 14:37 ` Jean Delvare 1 sibling, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-13 7:20 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel Hi Eric, On Sun, 13 Jul 2008 14:04:28 +0800, eric miao wrote: > From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001 > From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Date: Thu, 10 Jul 2008 13:37:59 +0800 > Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 > I2C Port Expanders > > Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > --- > > Updated as below. i2c_new_dummy() is called _only_ when necessary > (there's 8-bit IO expander). mutex is used to protect the update to > chip->reg_out[]. > > Actually, I don't quite like the idea of i2c_new_dummy(), which creates > several i2c_client for one device. Due to: > > 1. i2c_client should really be 1:1 with the device > 2. a dummy i2c_client just wastes another several bytes I don't like i2c_new_dummy much either, and I agree with the 2 points above. However... > 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be > monitored by the I2C chips at startup to decide the connections of > AD2/AD0 pins to GND/VCC/SCL/SDA, so actually, even if the chip > is finally decided at, say 0x56, no sane hardware designers will put > another chip whose address falls between 0x50-0x5F together with > such a max732x chip, ugly, but true. Why do these chips have a selectable address at all then, if they virtually use all the range of possible addresses? I don't buy this point at all. I see no reason why putting another chip in the same range would cause any problem. > One i2c_client requesting a group of address could somehow be > more reasonable, and requested address can be changed at > run-time. It would be more reasonable, but it would be racy (as your original driver was, now that I come to think of it.) Without a mutex (or equivalent) to protect the i2c_client structure, two concurrent accesses at different addresses would break. And if you add a mutex to the i2c_client, and take it for every access, then you have a performance degradation. I definitely prefer to waste a few hundreds bytes in memory. One possible solution to this problem would be to split the few fields needed by i2c_smbus_* and friends, to a separate structure (say, i2c_handle). One i2c_client structure would include one i2c_handle, and could have an optional array of extra ones for multi-address chips. This would however require that we change the prototype of all i2c_smbus_* helper functions, this is a big change, so it better be well considered and deemed worth the effort. > > Anyway, here's the one for i2c_new_dummy(). > (...) > +static int __devinit max732x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct max732x_platform_data *pdata; > + struct max732x_chip *chip; > + struct i2c_client *c; > + uint16_t addr_a, addr_b; > + int ret, nr_port; > + > + pdata = client->dev.platform_data; > + if (pdata == NULL) > + return -ENODEV; > + > + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + nr_port = max732x_setup_gpio(chip, id, pdata->gpio_base); > + > + addr_a = (client->addr & 0x0f) | 0x60; > + addr_b = (client->addr & 0x0f) | 0x50; > + > + switch (client->addr & 0x70) { > + case 0x60: > + chip->client_group_a = client; > + if (nr_port > 7) { > + c = i2c_new_dummy(client->adapter, addr_b); > + chip->client_group_b = chip->client_dummy = c; > + } > + break; > + case 0x50: > + chip->client_group_b = client; > + if (nr_port > 7) { > + c = i2c_new_dummy(client->adapter, addr_a); > + chip->client_group_a = chip->client_dummy = c; > + } > + break; > + default: > + dev_err(&client->dev, "invalid I2C address specified %02x\n", > + client->addr); > + return -EINVAL; > + } > + > + mutex_init(&chip->lock); > + > + max732x_read(chip, is_group_a(chip, 0), &chip->reg_out[0]); > + if (nr_port > 7) > + max732x_read(chip, is_group_a(chip, 8), &chip->reg_out[1]); > + > + ret = gpiochip_add(&chip->gpio_chip); > + if (ret) > + goto out_failed; > + > + if (pdata->setup) { > + ret = pdata->setup(client, chip->gpio_chip.base, > + chip->gpio_chip.ngpio, pdata->context); > + if (ret < 0) > + dev_warn(&client->dev, "setup failed, %d\n", ret); > + } > + > + i2c_set_clientdata(client, chip); > + return 0; > + > +out_failed: > + kfree(chip); > + return ret; > +} Looks OK to me. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080713092050.6dffd8d3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080713092050.6dffd8d3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-13 8:53 ` eric miao [not found] ` <f17812d70807130153g290e17ecq10ab12529effc8d4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-07-13 9:12 ` David Brownell 1 sibling, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-13 8:53 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel >> 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be >> monitored by the I2C chips at startup to decide the connections of >> AD2/AD0 pins to GND/VCC/SCL/SDA, so actually, even if the chip >> is finally decided at, say 0x56, no sane hardware designers will put >> another chip whose address falls between 0x50-0x5F together with >> such a max732x chip, ugly, but true. > > Why do these chips have a selectable address at all then, if they > virtually use all the range of possible addresses? I don't buy this > point at all. I see no reason why putting another chip in the same > range would cause any problem. Selectable address is used to decide the initial state of each pin (especially the output level). Due to the fact that SCL/SDA are initially held high, the chip has to monitor the first I2C activity to decide if AD2/AD0 is tied to one of GND/VCC/SCL/SDA. > >> One i2c_client requesting a group of address could somehow be >> more reasonable, and requested address can be changed at >> run-time. > > It would be more reasonable, but it would be racy (as your original > driver was, now that I come to think of it.) Without a mutex (or > equivalent) to protect the i2c_client structure, two concurrent > accesses at different addresses would break. And if you add a mutex to > the i2c_client, and take it for every access, then you have a > performance degradation. I definitely prefer to waste a few hundreds > bytes in memory. > > One possible solution to this problem would be to split the few fields > needed by i2c_smbus_* and friends, to a separate structure (say, > i2c_handle). One i2c_client structure would include one i2c_handle, and > could have an optional array of extra ones for multi-address chips. > This would however require that we change the prototype of all > i2c_smbus_* helper functions, this is a big change, so it better be > well considered and deemed worth the effort. Sounds good. > >> >> Anyway, here's the one for i2c_new_dummy(). >> (...) ... > Looks OK to me. > So , do you see any chance of this getting merged during this window? If that's possible, please consider merge, thanks. > -- > Jean Delvare > -- Cheers - eric _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807130153g290e17ecq10ab12529effc8d4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807130153g290e17ecq10ab12529effc8d4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-13 9:13 ` Jean Delvare [not found] ` <20080713111306.791bbc41-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-13 9:13 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Sun, 13 Jul 2008 16:53:19 +0800, eric miao wrote: > >> 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be > >> monitored by the I2C chips at startup to decide the connections of > >> AD2/AD0 pins to GND/VCC/SCL/SDA, so actually, even if the chip > >> is finally decided at, say 0x56, no sane hardware designers will put > >> another chip whose address falls between 0x50-0x5F together with > >> such a max732x chip, ugly, but true. > > > > Why do these chips have a selectable address at all then, if they > > virtually use all the range of possible addresses? I don't buy this > > point at all. I see no reason why putting another chip in the same > > range would cause any problem. > > Selectable address is used to decide the initial state of each > pin (especially the output level). Due to the fact that SCL/SDA > are initially held high, the chip has to monitor the first I2C activity > to decide if AD2/AD0 is tied to one of GND/VCC/SCL/SDA. Ah, OK. I had missed the fact that the address had an impact on the initial pin states. This sounds a bit weird to me, BTW, as the chip doesn't know its address before the first transaction on the I2C bus, it also can't set the pins to their "initial" state before the first transaction on the I2C bus. This is a strange definition of "initial state" if you ask me. Anyway, this still doesn't prevent other I2C chips from using other addresses in the 0x50-0x6f range on the same bus. > So , do you see any chance of this getting merged during this > window? If that's possible, please consider merge, thanks. You probably want to read MAINTAINERS again, to find out that I am not the maintainer of the gpio subsystem. Ask David Brownell, or Andrew Morton. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080713111306.791bbc41-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080713111306.791bbc41-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-13 13:45 ` eric miao [not found] ` <f17812d70807130645i755c1c62la30d5c515c2d2080-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: eric miao @ 2008-07-13 13:45 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Sun, Jul 13, 2008 at 5:13 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > On Sun, 13 Jul 2008 16:53:19 +0800, eric miao wrote: >> >> 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be >> >> monitored by the I2C chips at startup to decide the connections of >> >> AD2/AD0 pins to GND/VCC/SCL/SDA, so actually, even if the chip >> >> is finally decided at, say 0x56, no sane hardware designers will put >> >> another chip whose address falls between 0x50-0x5F together with >> >> such a max732x chip, ugly, but true. >> > >> > Why do these chips have a selectable address at all then, if they >> > virtually use all the range of possible addresses? I don't buy this >> > point at all. I see no reason why putting another chip in the same >> > range would cause any problem. >> >> Selectable address is used to decide the initial state of each >> pin (especially the output level). Due to the fact that SCL/SDA >> are initially held high, the chip has to monitor the first I2C activity >> to decide if AD2/AD0 is tied to one of GND/VCC/SCL/SDA. > > Ah, OK. I had missed the fact that the address had an impact on the > initial pin states. This sounds a bit weird to me, BTW, as the chip > doesn't know its address before the first transaction on the I2C bus, > it also can't set the pins to their "initial" state before the first > transaction on the I2C bus. This is a strange definition of "initial > state" if you ask me. > > Anyway, this still doesn't prevent other I2C chips from using other > addresses in the 0x50-0x6f range on the same bus. OK, I see. > >> So , do you see any chance of this getting merged during this >> window? If that's possible, please consider merge, thanks. > > You probably want to read MAINTAINERS again, to find out that I am not > the maintainer of the gpio subsystem. Ask David Brownell, or Andrew > Morton. Well, I don't mind your optional Acked-by to the I2C part :-) And David, apart from my misunderstanding of the I2C address, which I don't think have impact to the patch itself, can I have you Acked-by so I'll mail Andrew Morton to see his willingness to include this into -mm tree? (Or you have a plan to have a dedicated gpiolib tree, which I don't mind either) > > -- > Jean Delvare > -- Cheers - eric _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <f17812d70807130645i755c1c62la30d5c515c2d2080-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807130645i755c1c62la30d5c515c2d2080-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-07-13 19:18 ` David Brownell [not found] ` <200807131218.29575.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 25+ messages in thread From: David Brownell @ 2008-07-13 19:18 UTC (permalink / raw) To: eric miao; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Sunday 13 July 2008, eric miao wrote: > And David, apart from my misunderstanding of the I2C address, which > I don't think have impact to the patch itself, can I have you Acked-by so > I'll mail Andrew Morton to see his willingness to include this into -mm > tree? I'll forward it with my signed-off-by, if the appended updates are OK. Notice the memory leak fix, removing the pr_warning(), and other minor tweaks. The Kconfig text now matches other similar drivers; and also doesn't explain twice about some ports being single-direction. This doesn't apply quite as-is to with the current pending GPIO patches (found in the MM tree) FWIW; the Makefile conflict doesn't show up in the diffs below. - Dave --- g26.orig/drivers/gpio/max732x.c 2008-07-13 11:56:29.000000000 -0700 +++ g26/drivers/gpio/max732x.c 2008-07-13 11:56:22.000000000 -0700 @@ -21,7 +21,9 @@ #include <linux/i2c.h> #include <linux/i2c/max732x.h> -/* Each port of MAX732x (including MAX7319) falls into one of the + +/* + * Each port of MAX732x (including MAX7319) falls into one of the * following three types: * * - Push Pull Output @@ -37,7 +39,8 @@ * - Group A : by I2C address 0b'110xxxx * - Group B : by I2C address 0b'101xxxx * - * where 'xxxx' is decided by the connections of pin AD2/AD0. + * where 'xxxx' is decided by the connections of pin AD2/AD0. The + * address used also affects the initial state of output signals. * * Within each group of ports, there are five known combinations of * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for @@ -47,7 +50,7 @@ * and GPIOs from GROUP_A are numbered before those from GROUP_B * (if there are two groups). * - * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574, + * NOTE: MAX7328/MAX7329 are drop-in replacements for PCF8574/a, so * they are not supported by this driver. */ @@ -82,6 +85,7 @@ MODULE_DEVICE_TABLE(i2c, max732x_id); struct max732x_chip { struct gpio_chip gpio_chip; + struct i2c_client *client; /* "main" client */ struct i2c_client *client_dummy; struct i2c_client *client_group_a; struct i2c_client *client_group_b; @@ -185,7 +189,8 @@ static int max732x_gpio_direction_input( chip = container_of(gc, struct max732x_chip, gpio_chip); if ((mask & chip->dir_input) == 0) { - pr_warning("%s: port %d is output only\n", __func__, off); + dev_dbg(&chip->client->dev, "%s port %d is output only\n", + chip->client->name, off); return -EACCES; } @@ -201,7 +206,8 @@ static int max732x_gpio_direction_output chip = container_of(gc, struct max732x_chip, gpio_chip); if ((mask & chip->dir_output) == 0) { - pr_warning("%s: port %d is input only\n", __func__, off); + dev_dbg(&chip->client->dev, "%s port %d is input only\n", + chip->client->name, off); return -EACCES; } @@ -240,15 +246,18 @@ static int __devinit max732x_setup_gpio( port++; } - gc->direction_input = max732x_gpio_direction_input; - gc->direction_output = max732x_gpio_direction_output; + if (chip->dir_input) + gc->direction_input = max732x_gpio_direction_input; + if (chip->dir_output) { + gc->direction_output = max732x_gpio_direction_output; + gc->set = max732x_gpio_set_value; + } gc->get = max732x_gpio_get_value; - gc->set = max732x_gpio_set_value; gc->can_sleep = 1; gc->base = gpio_start; gc->ngpio = port; - gc->label = "max732x"; + gc->label = chip->client->name; gc->owner = THIS_MODULE; return port; @@ -270,6 +279,7 @@ static int __devinit max732x_probe(struc chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL); if (chip == NULL) return -ENOMEM; + chip->client = client; nr_port = max732x_setup_gpio(chip, id, pdata->gpio_base); @@ -294,7 +304,8 @@ static int __devinit max732x_probe(struc default: dev_err(&client->dev, "invalid I2C address specified %02x\n", client->addr); - return -EINVAL; + ret = -EINVAL; + goto out_failed; } mutex_init(&chip->lock); @@ -345,7 +356,7 @@ static int __devexit max732x_remove(stru return ret; } - /* unregister the dummy i2c_client */ + /* unregister any dummy i2c_client */ if (chip->client_dummy) i2c_unregister_device(chip->client_dummy); --- g26.orig/drivers/gpio/Kconfig 2008-07-13 11:56:29.000000000 -0700 +++ g26/drivers/gpio/Kconfig 2008-07-13 10:10:35.000000000 -0700 @@ -43,7 +43,7 @@ config GPIO_SYSFS comment "I2C GPIO expanders:" config GPIO_MAX732X - tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders" + tristate "MAX7319, MAX7320-7327 I2C Port Expanders" depends on I2C help Say yes here to support the MAX7319, MAX7320-7327 series of I2C @@ -52,17 +52,14 @@ config GPIO_MAX732X Input and Output (designed by 'P'). The combinations are listed below: - MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), MAX7322 (4I4O), - MAX7323 (4P4O), MAX7324 (8I8O), MAX7325 (8P8O) - MAX7326 (4I12O), MAX7327 (4P12O) + 8 bits: MAX7319 (8I), MAX7320 (8O), MAX7321 (8P), + MAX7322 (4I4O), MAX7323 (4P4O) - The board code has to specify the model to use, and the start - number for these GPIOs. Model specific information is calculated - automatically. Due to the fixed role of each port, configuration - of the port direction will be ignored and a warning be issued if - the corresponding port isn't applicable. And gpio_get_value() to - those output only ports will return the configured output status - instead of a real input. + 16 bits: MAX7324 (8I8O), MAX7325 (8P8O), + MAX7326 (4I12O), MAX7327 (4P12O) + + Board setup code must specify the model to use, and the start + number for these GPIOs. config GPIO_PCA953X tristate "PCA953x, PCA955x, and MAX7310 I/O ports" _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <200807131218.29575.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <200807131218.29575.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-07-14 1:35 ` Eric Miao 0 siblings, 0 replies; 25+ messages in thread From: Eric Miao @ 2008-07-14 1:35 UTC (permalink / raw) To: David Brownell; +Cc: Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel David Brownell wrote: > On Sunday 13 July 2008, eric miao wrote: >> And David, apart from my misunderstanding of the I2C address, which >> I don't think have impact to the patch itself, can I have you Acked-by so >> I'll mail Andrew Morton to see his willingness to include this into -mm >> tree? > > I'll forward it with my signed-off-by, if the appended updates are OK. I'm OK with the updates, thanks. _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080713092050.6dffd8d3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2008-07-13 8:53 ` eric miao @ 2008-07-13 9:12 ` David Brownell [not found] ` <20080713091236.015E920ABDD-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> 1 sibling, 1 reply; 25+ messages in thread From: David Brownell @ 2008-07-13 9:12 UTC (permalink / raw) To: eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w Cc: jack.ren-eYqpPyKDWXRBDgjK7y7TUQ, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW, i2c-GZX6beZjE8VD60Wz+7aTrA > 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be > monitored by the I2C chips at startup to decide the connections of > AD2/AD0 pins to GND/VCC/SCL/SDA, There's no need to monitor addresses ... before the first data bit is sent -- whatever value! -- it's known how those pins are wired: - START bit ... SCL is high, SDA falls. Any pin that stayed low is wired to ground. Any pin that changed high-to-low is thus connected to SDA. - Prepare to send first bit ... SDA still low, SCL falls. Any pin that stayed high is wired to Vcc. Any pin that changed high-to-low is connected to SCL. And then the master updates SDA to match the first (address) bit, and lets SCL be pulled high ... the i2c slave has all the data it needs to be able to determine its address. > so actually, even if the chip > is finally decided at, say 0x56, no sane hardware designers will put > another chip whose address falls between 0x50-0x5F together with > such a max732x chip, ugly, but true. As Jean said: I don't buy this point at all. This isn't the only chip to use that GND/VCC/SCL/SDA scheme, and all data sheets I've seen talk about how it lets lots of distinct chips be used. They can't all be wrong... I think someone was leading you astray! - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080713091236.015E920ABDD-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080713091236.015E920ABDD-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> @ 2008-07-13 9:18 ` Jean Delvare 0 siblings, 0 replies; 25+ messages in thread From: Jean Delvare @ 2008-07-13 9:18 UTC (permalink / raw) To: David Brownell Cc: jack.ren-eYqpPyKDWXRBDgjK7y7TUQ, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW, i2c-GZX6beZjE8VD60Wz+7aTrA On Sun, 13 Jul 2008 02:12:36 -0700, David Brownell wrote: > > 3. for chips like max732x, actually, the range of 0x50 - 0x5F will be > > monitored by the I2C chips at startup to decide the connections of > > AD2/AD0 pins to GND/VCC/SCL/SDA, > > There's no need to monitor addresses ... before the first data bit > is sent -- whatever value! -- it's known how those pins are wired: > > - START bit ... SCL is high, SDA falls. Any pin that stayed > low is wired to ground. Any pin that changed high-to-low is > thus connected to SDA. > > - Prepare to send first bit ... SDA still low, SCL falls. Any > pin that stayed high is wired to Vcc. Any pin that changed > high-to-low is connected to SCL. > > And then the master updates SDA to match the first (address) bit, > and lets SCL be pulled high ... the i2c slave has all the data it > needs to be able to determine its address. Totally true. Add to this that the first 3 address bits can't change, this gives the chip some more time to setup its internal logic to match the 4 LSBs by the time they come. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807122304o533d8af9k1384db653b264912-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-07-13 7:20 ` Jean Delvare @ 2008-07-13 14:37 ` Jean Delvare 1 sibling, 0 replies; 25+ messages in thread From: Jean Delvare @ 2008-07-13 14:37 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Sun, 13 Jul 2008 14:04:28 +0800, eric miao wrote: > From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001 > From: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Date: Thu, 10 Jul 2008 13:37:59 +0800 > Subject: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 > I2C Port Expanders > > Signed-off-by: Jack Ren <jack.ren-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> > (...) > > drivers/gpio/Kconfig | 22 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/max732x.c | 380 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c/max732x.h | 19 ++ > 4 files changed, 422 insertions(+), 0 deletions(-) > create mode 100644 drivers/gpio/max732x.c > create mode 100644 include/linux/i2c/max732x.h > (...) Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <f17812d70807110239q6c175f5cn70db966681ae387d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-07-11 9:48 ` eric miao @ 2008-07-11 9:54 ` Jean Delvare [not found] ` <20080711115436.22134ce7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 1 sibling, 1 reply; 25+ messages in thread From: Jean Delvare @ 2008-07-11 9:54 UTC (permalink / raw) To: eric miao Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Fri, 11 Jul 2008 17:39:50 +0800, eric miao wrote: > > > > Oh, and one more thing as I just notice it: > > > >> +static inline int is_group_a(struct max732x_chip *chip, unsigned off) > >> +{ > >> + return (1u << off) & chip->mask_group_a; > >> +} > > > > Given the way you use it, can't you just define this function as: > > > > static inline int is_group_a(struct max732x_chip *chip, unsigned off) > > { > > return (off < 8); > > } > > > > ? As this is the only place where you use chip->mask_group_a, you would > > be able to get rid of it. > > > > I want to get rid of it either but I'm afraid not. (off < 8) doesn't necessarily > mean it's in group_a, max7320 is an exception. I didn't look into the details (and won't have the time to) but my feeling is that it only depends on how you decide to handle the max7320. If you want to make it fit with the rest of the supported chips, I see no reason why it wouldn't work. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20080711115436.22134ce7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders [not found] ` <20080711115436.22134ce7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-07-11 10:04 ` eric miao 0 siblings, 0 replies; 25+ messages in thread From: eric miao @ 2008-07-11 10:04 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, Jack Ren, i2c-GZX6beZjE8VD60Wz+7aTrA, linux-arm-kernel On Fri, Jul 11, 2008 at 5:54 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > On Fri, 11 Jul 2008 17:39:50 +0800, eric miao wrote: >> > >> > Oh, and one more thing as I just notice it: >> > >> >> +static inline int is_group_a(struct max732x_chip *chip, unsigned off) >> >> +{ >> >> + return (1u << off) & chip->mask_group_a; >> >> +} >> > >> > Given the way you use it, can't you just define this function as: >> > >> > static inline int is_group_a(struct max732x_chip *chip, unsigned off) >> > { >> > return (off < 8); >> > } >> > >> > ? As this is the only place where you use chip->mask_group_a, you would >> > be able to get rid of it. >> > >> >> I want to get rid of it either but I'm afraid not. (off < 8) doesn't necessarily >> mean it's in group_a, max7320 is an exception. > > I didn't look into the details (and won't have the time to) but my > feeling is that it only depends on how you decide to handle the > max7320. If you want to make it fit with the rest of the supported > chips, I see no reason why it wouldn't work. That's possible, but I really want to keep it generic enough, so that user has the freedom to define arbitrary combinitions of GROUP_A(XXX) | GROUP_B(XXX). > > -- > Jean Delvare > -- Cheers - eric _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2008-07-14 1:35 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-10 6:13 [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders Eric Miao
[not found] ` <4875A893.3090402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2008-07-11 8:29 ` Jean Delvare
[not found] ` <20080711102952.31d2d943-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-11 8:57 ` eric miao
[not found] ` <f17812d70807110157p5e421222sc9ef420ceb80970c-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-11 9:31 ` Jean Delvare
[not found] ` <20080711113114.79d80212-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-11 9:39 ` eric miao
[not found] ` <f17812d70807110239q6c175f5cn70db966681ae387d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-11 9:48 ` eric miao
[not found] ` <f17812d70807110248y7fab3328q59ea41084c491df-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-11 11:15 ` Jean Delvare
2008-07-11 21:25 ` David Brownell
[not found] ` <200807111425.00961.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-07-12 7:16 ` Jean Delvare
[not found] ` <20080712091610.4ec242c3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-12 7:46 ` David Brownell
[not found] ` <200807120046.29389.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-07-12 7:53 ` Jean Delvare
[not found] ` <20080712095300.1ba4b3a7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-12 21:42 ` David Brownell
2008-07-13 6:55 ` Jean Delvare
2008-07-13 6:04 ` eric miao
[not found] ` <f17812d70807122304o533d8af9k1384db653b264912-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-13 7:20 ` Jean Delvare
[not found] ` <20080713092050.6dffd8d3-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-13 8:53 ` eric miao
[not found] ` <f17812d70807130153g290e17ecq10ab12529effc8d4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-13 9:13 ` Jean Delvare
[not found] ` <20080713111306.791bbc41-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-13 13:45 ` eric miao
[not found] ` <f17812d70807130645i755c1c62la30d5c515c2d2080-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-07-13 19:18 ` David Brownell
[not found] ` <200807131218.29575.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-07-14 1:35 ` Eric Miao
2008-07-13 9:12 ` David Brownell
[not found] ` <20080713091236.015E920ABDD-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>
2008-07-13 9:18 ` Jean Delvare
2008-07-13 14:37 ` Jean Delvare
2008-07-11 9:54 ` Jean Delvare
[not found] ` <20080711115436.22134ce7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-07-11 10:04 ` eric miao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox