| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20080722020542.d180162d.akpm@linux-foundation.org>
Date: Tue, 22 Jul 2008 02:05:42 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: David Brownell <david-b@...bell.net>
Cc: lkml <linux-kernel@...r.kernel.org>,
Eric Miao <eric.y.miao@...il.com>,
Jean Delvare <khali@...ux-fr.org>,
Jack Ren <jack.ren@...vell.com>
Subject: Re: [patch 2.6.26-rc-mm] gpio: max732x driver
On Sun, 13 Jul 2008 19:18:06 -0700 David Brownell <david-b@...bell.net> wrote:
> From: Eric Miao <eric.miao@...vell.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@...vell.com>
> Signed-off-by: Eric Miao <eric.miao@...vell.com>
> Acked-by: Jean Delvare <khali@...ux-fr.org>
> [ dbrownell@...rs.sourceforge.net: minor fixes ]
> Signed-off-by: David Brownell <dbrownell@...rs.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists