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

Signed-off-by Jack Ren <[email protected]>

Signed-off-by Eric Miao <[email protected]>

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

drivers/gpio/Kconfig +0 -0

index bbd2834..9f8d030 100644

drivers/gpio/Kconfig +13 -0

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

drivers/gpio/Makefile +0 -0

index fdde992..814698c 100644

drivers/gpio/max732x.c new file +8 -0

new file mode 100644
index 0000000..b1b9b62
@@ -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 <[email protected]>
+ *  Copyright (C) 2008 Eric Miao <[email protected]>
+ *
+ *  Derived from drivers/gpio/pca953x.c

drivers/gpio/max732x.c new file +5 -0

+
+#include <asm/gpio.h>
+
+/* Each port of MAX732x (including MAX7319) falls into one of the
+ * following three types:

drivers/gpio/max732x.c new file +4 -0

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

drivers/gpio/max732x.c new file +23 -0

+#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;

drivers/gpio/max732x.c new file +6 -0

+
+	ret = i2c_smbus_write_byte(client, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed writing\n");
+		return -EIO;
+	}

drivers/gpio/max732x.c new file +102 -0

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

drivers/gpio/max732x.c new file +23 -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;

drivers/gpio/max732x.c new file +17 -0

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

drivers/gpio/max732x.c new file +46 -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 <[email protected]>");
+MODULE_DESCRIPTION("GPIO expander driver for MAX732X");
+MODULE_LICENSE("GPL");

include/linux/i2c/max732x.h new file +19 -0

new file mode 100644
index 0000000..e103366
@@ -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 */

Hi Eric, David,

Quoted 3 lines · Eric Miao

Signed-off-by Jack Ren <[email protected]>

Signed-off-by Eric Miao <[email protected]>

David, maybe you should create an entry for the gpio subsystem in MAINTAINERS?

drivers/gpio/Kconfig config GPIO_PCF857X · line 69

Quoted 6 lines

drivers/gpio/Kconfig +6 -0

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

drivers/gpio/Kconfig config GPIO_PCF857X · line 72

Quoted 4 lines

drivers/gpio/Kconfig +3 -0

+       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! :(

drivers/gpio/Makefile obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o · line 10

Quoted 5 lines

drivers/gpio/Makefile +1 -0

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

drivers/gpio/max732x.c line 18

Quoted 4 lines

drivers/gpio/max732x.c new file +4 -0

+#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.

Quoted 2 lines

+ * 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"?

Quoted 6 lines

+ *
+ * - 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)

Quoted 13 lines

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+#define PORT_NONE    0x0     /* '/' No Port */

You don't use this define anywhere.

drivers/gpio/max732x.c line 63

Quoted 12 lines

drivers/gpio/max732x.c new file +11 -0

+#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.

Quoted 3 lines

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

drivers/gpio/max732x.c line 102

Quoted 6 lines

drivers/gpio/max732x.c new file +6 -0

+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

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+     nr_port = port;

Why do you need 2 variables for that?

drivers/gpio/max732x.c line 236

Quoted 7 lines

drivers/gpio/max732x.c new file +6 -0

+
+     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]);

?

drivers/gpio/max732x.c line 269

Quoted 3 lines

drivers/gpio/max732x.c new file +3 -0

+     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.)

drivers/gpio/max732x.c line 296

Quoted 5 lines

drivers/gpio/max732x.c new file +5 -0

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

Quoted 7 lines · Jean Delvare

Quoted 4 lines · Eric Miao

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

Quoted 4 lines

Quoted 2 lines

(...)
+#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

Quoted 4 lines

Quoted 2 lines

(...)
+ nr_port = port;

Why do you need 2 variables for that?

Again, better readability :-)

I can remove that if you mind.

Oh, and one more thing as I just notice it:

Review

Quoted 4 lines

(quoted diff) +4 -0

+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

From 18d910f69c5408340661326f1ad1bcbb2c5bc6f6 Mon Sep 17 00:00:00 2001

drivers/gpio/max732x.c new file +26 -0

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

drivers/gpio/max732x.c new file +15 -0

+
+	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);
+	}

drivers/gpio/max732x.c new file +28 -0

+{
+	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;
+}

patch updated:

Quoted 1 line

From 18d910f69c5408340661326f1ad1bcbb2c5bc6f6 Mon Sep 17 00:00:00 2001

From: Eric Miao <[email protected]>
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 <[email protected]>

Signed-off-by Eric Miao <[email protected]>

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

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

+ * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574,
+ * they are not supported by this driver.

drivers/gpio/max732x.c new file +20 -0

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

drivers/gpio/max732x.c line 21

Quoted 11 lines · eric miao

drivers/gpio/max732x.c new file +7 -0

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

Quoted 26 lines

(...)

drivers/gpio/max732x.c new file +25 -0

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

drivers/gpio/max732x.c line 276

Quoted 5 lines

drivers/gpio/max732x.c new file +5 -0

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

Quoted 8 lines

drivers/gpio/max732x.c line 21

Quoted 4 lines

drivers/gpio/max732x.c new file +4 -0

+#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.

Quoted 10 lines

Quoted 10 lines

drivers/gpio/max732x.c new file +10 -0

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

Quoted 8 lines

drivers/gpio/max732x.c line 276

Quoted 5 lines

drivers/gpio/max732x.c new file +5 -0

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

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.

Quoted 5 lines · Jean Delvare

Quoted 2 lines

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.

Quoted 4 lines · David Brownell

Quoted 3 lines · Jean Delvare

Quoted 2 lines

Why not just insist the 0x5x address be registered/probed? This extra stuff is needless and confusing.

Quoted 5 lines

Quoted 2 lines

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

Quoted 11 lines · Jean Delvare

Quoted 9 lines · David Brownell

Quoted 5 lines · Jean Delvare

Quoted 2 lines

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.

Quoted 2 lines

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

From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001

From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001

From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001
From: Eric Miao <[email protected]>
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 <[email protected]>

Signed-off-by Eric Miao <[email protected]>

---

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/max732x.c new file +57 -0

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

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

Anyway, here's the one for i2c_new_dummy().
(...)

Looks OK to me.

--
Jean Delvare

Quoted 11 lines

Quoted 6 lines

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.

So , do you see any chance of this getting merged during this window? If that's possible, please consider merge, thanks.

Quoted 12 lines · eric miao

Quoted 11 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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

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?

Quoted 4 lines · eric miao

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.

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.

Quoted 3 lines

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.

From 562e78eec627092610db817e28c4ff9be6af13e1 Mon Sep 17 00:00:00 2001
From: Eric Miao <[email protected]>
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 <[email protected]>

Signed-off-by Eric Miao <[email protected]>

(...)

 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

(...)

Quoted 15 lines

Oh, and one more thing as I just notice it:

Review

Quoted 4 lines

(quoted diff) +4 -0

+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);
}

Quoted 3 lines

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

Quoted 19 lines · eric miao

Quoted 18 lines

Oh, and one more thing as I just notice it:

Review

Quoted 4 lines

(quoted diff) +4 -0

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

Quoted 2 lines

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