On Friday 11 July 2008, eric miao wrote:
> patch updated:
>
> >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
>
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -61,6 +61,28 @@ config GPIO_PCF857X
> This driver provides an in-kernel interface to those GPIOs using
> platform-neutral GPIO calls.
>
> +config GPIO_MAX732X
> + tristate "MAX7319, MAX7320-7327 8/16-bit I2C Port Expanders"
"MAX" should still precede "PCF" in the Kconfig.
As it says: put drivers in the right section,
and in alphabetical order.
> --- /dev/null
> +++ b/drivers/gpio/max732x.c
> @@ -0,0 +1,361 @@
> ...
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/max732x.h>
> +
> +#include <asm/gpio.h>
For consistency, make that <linux/gpio.h> and put it up above.
I like to see blank lines delineating groups, like <linux/*>
and <linux/i2c/*>, too.
> + * Within each group of ports, there are five known combinations of
> + * I/O ports: 4I4O, 4P4O, 8I, 8P, 8O, see the definitions below for
> + * the detailed organization of these ports.
Quirky little buggers, they are!!
> + * NOTE: MAX7328/MAX7329, however, resembles much closer to PCF8574,
> + * they are not supported by this driver.
In fact the data sheet for those parts says they're second sources
for those PCF chips. Feel free to submit a patch updating Kconfig
and the pcf857x driver accordingly ... :)
> +static int max732x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> +{
> + struct max732x_chip *chip;
> + uint8_t reg_val;
> + int ret;
> +
> + chip = container_of(gc, struct max732x_chip, gpio_chip);
> +
> + ret = max732x_read(chip, is_group_a(chip, off), ®_val);
> + if (ret < 0)
> + return 0;
> +
> + return (reg_val & (1u << (off & 0x7))) ? 1 : 0;
Simpler: "reg_val & (the_mask)". The values here are
zero/nonzero, not zero/one/undefined.
> +static void max732x_gpio_set_value(struct gpio_chip *gc, unsigned off, int
> val)
> +{
> + struct max732x_chip *chip;
> + uint8_t reg_out, mask = 1u << (off & 0x7);
> + int ret;
> +
> + chip = container_of(gc, struct max732x_chip, gpio_chip);
> +
> + reg_out = (off > 7) ? chip->reg_out[1] : chip->reg_out[0];
> + reg_out = (val) ? reg_out | mask : reg_out & ~mask;
You should have some kind of locking to protect chip->reg_out[].
Hmm, I notice the pcf857x driver doesn't ... sigh.
I can't quite see a way around a mutex or rwlock. The set_bit()
clear_bit() calls would atomic, but you need the assignment to be
atomic at the I2C chip, not just on the host side.
> +
> + ret = max732x_write(chip, is_group_a(chip, off), reg_out);
> + if (ret < 0)
> + return;
> +
> + /* update the shadow register then */
> + if (off > 7)
> + chip->reg_out[1] = reg_out;
> + else
> + chip->reg_out[0] = reg_out;
> +}
Notice how two tasks setting direction for different GPIOs will
both feel free to clobber the cached chip->reg_out[] value ...
> +
> + if ((mask & chip->dir_input) == 0)
> + pr_warning("%s: port %d is output only\n", __func__, off);
First problem: if it's an output-only signal, you should return a
negative errno to anyone trying to use it as an input. (Likewise
for the converse, trying to set an output-only pin as an input.)
EACCES would seem appropriate; it's what open(2) returns when you
try to write a read-only file, and these modes can't be written.
Seocnd: you shouldn't use pr_*() calls when there's a relevant
driver model node. Of course, in this case once you properly
return a fault code, this merits at most a dev_dbg() call, since
the caller can be expected to handle such errors somehow.
> +static int __devinit max732x_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct max732x_platform_data *pdata;
> + struct max732x_chip *chip;
> + int ret;
> +
> + pdata = client->dev.platform_data;
> + if (pdata == NULL)
> + return -ENODEV;
> +
> + chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL);
> + if (chip == NULL)
> + return -ENOMEM;
> +
> + chip->client[0] = client;
> +
> + switch (client->addr & 0x70) {
> + case 0x60:
> + chip->client[1] = i2c_new_dummy(client->adapter,
> + (client->addr & 0x0f) | 0x50);
> + chip->client_group_a = chip->client[0];
> + chip->client_group_b = chip->client[1];
> + break;
> + case 0x50:
> + chip->client[1] = i2c_new_dummy(client->adapter,
> + (client->addr & 0x0f) | 0x60);
> + chip->client_group_a = chip->client[1];
> + chip->client_group_b = chip->client[0];
> + break;
Why not just insist the 0x5x address be registered/probed? This
extra stuff is needless and confusing.
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
