lkml.org 
[lkml]   [2008]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 2.6.26-rc-mm] gpio: max732x driver
On Sun, 13 Jul 2008 19:18:06 -0700 David Brownell <david-b@pacbell.net> wrote:

> From: Eric Miao <eric.miao@marvell.com>
>
> This adds a driver supporting a family of I2C port expanders from
> Maxim, which includes the MAX7319 and MAX7320-7327 chips.
>
> Signed-off-by: Jack Ren <jack.ren@marvell.com>
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> Acked-by: Jean Delvare <khali@linux-fr.org>
> [ dbrownell@users.sourceforge.net: minor fixes ]
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> We're growing quite the collection of dedicated GPIO expander drivers!
> And the multi-function chip support is growing too. This applies after
> the patch adding support for max7301 chips.
>
>
> ...
>
> +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;
> + }

This..

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

and this demonstrate how weird it is that i2c_smbus_write_byte() and
i2c_smbus_read_byte() return an `s32' instead of plain old `int'.


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

strange random unnecessary usage of "1u" in here

> +}
> +
>
> ...
>
> +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) {
> + dev_dbg(&chip->client->dev, "%s port %d is output only\n",
> + chip->client->name, off);
> + return -EACCES;

I don't think that EACCES is a suitable error code here. That's a
security/permissions sort of thing. If userspace is requesting this
driver to do something which the hardware cannot do then probably
EINVAL is the appropriate return code.

> + }
> +
> + 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) {
> + dev_dbg(&chip->client->dev, "%s port %d is input only\n",
> + chip->client->name, off);
> + return -EACCES;

Ditto

> + }
> +
> + max732x_gpio_set_value(gc, off, val);
> + return 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);

<goes back and checks that `chip' is a `struct max732x_chip*'>

I do find

p = kmalloc(sizeof(*p), ...)

to be more conforting to read.

> + if (chip == NULL)
> + return -ENOMEM;
> + chip->client = client;
> +
> + 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;

Multiple assignments get nasty comments from Linus when he's feeling
perky.

> + }
> + break;
> + default:
> + dev_err(&client->dev, "invalid I2C address specified %02x\n",
> + client->addr);
> + ret = -EINVAL;
> + goto out_failed;
> + }
> +
> + 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;
> +}
>
> ...
>

But that's all very minor stuff. Nice-looking driver.



\
 
 \ /
  Last update: 2008-07-22 11:09    [W:0.292 / U:0.792 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site