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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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