[PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Eric Miao]

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), &reg_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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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), &reg_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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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), &reg_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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[David Brownell]

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), &reg_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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[David Brownell]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[David Brownell]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

>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), &reg_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), &reg_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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[David Brownell]

> 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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[eric miao]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Jean Delvare]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[David Brownell]

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

Re: [PATCH] gpio: max732x: add support for MAX7319, MAX7320-7327 I2C Port Expanders

[Eric Miao]

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