Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

On Wednesday 30 January 2008, eric miao wrote:
> I like this patch, overall looks ok. See my comments below.

I basically like it, but I'd still like to see some changes.  :)

I'd split this patch into two parts (feature addition, and renaming),
and use simpler chip type handling.  Re that latter, in 2.6.26 there
will be an i2c_device_id struct:

  struct i2c_device_id {
        char name[I2C_NAME_SIZE];
        kernel_ulong_t driver_data;     /* Data private to the driver */
  };

A little planning done now will make it easy to use that scheme.
First, don't add a type field to the platform_data; that's what
the i2c_client.name is for.  Then just map those strings to the
number of GPIOs (eventually just use driver_data), and everything
else follows from there.


> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -27,15 +27,15 @@ config DEBUG_GPIO
> >
> >  comment "I2C GPIO expanders:"
> >
> > -config GPIO_PCA9539
> > -       tristate "PCA9539 16-bit I/O port"
> > +config GPIO_PCA953X
> > +       tristate "PCA953X I/O port"

"Ports"

> >         depends on I2C
> >         help
> > -         Say yes here to support the PCA9539 16-bit I/O port. These
> > -         parts are made by NXP and TI.
> > +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.

This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
Same register layout as the '36 (4-bit).  Otherwise the "x" seems
misleading...


> > diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca953x.c
> > similarity index 67%
> > rename from drivers/gpio/pca9539.c
> > rename to drivers/gpio/pca953x.c

I'd prefer to see the new feature and the renaming be separate patches.


> > -#define NR_PCA9539_GPIOS       16
> > +struct pca953x_desc {
> > +       unsigned int gpios;
> > +       unsigned int input;
> > +       unsigned int output;
> > +       unsigned int invert;
> > +       unsigned int direction;
> > +};
> 
> I guess the register address can be inferred either from the number
> of GPIOs or a single member field of "shift" may be sufficient.

Yes, inferring that would be simpler.  The number of GPIOs is a
function of the particular chip, and register offsets are 0/1/2/3
except for the 16-GPIO part where they're twice that.

What I'd do is save those four offsets directly in pca953x_chip,
initialized near when gpio_chip.ngpio is set up and with no need
for a separate "desc" type or table.  Eventually there'd be:

	struct i2c_device_id chips [] = {
		{ "pca9536", 4, },
		{ "pca9537", 4, },
		{ "pca9538", 8, },
		{ "pca9539", 16, },
	};
	MODULE_DEVICE_TABLE(i2c, chips);

Meanwhile, the equivalent of that table can come from a few strcmp()
tests in the probe() logic -- like what you already have, except
not using a "desc" type.


> > -static int pca9539_init_gpio(struct pca9539_chip *chip)
> > +static int pca953x_init_gpio(struct pca953x_chip *chip)
> >  {
> >         struct gpio_chip *gc;
> >
> >         gc = &chip->gpio_chip;
> >
> > -       gc->direction_input  = pca9539_gpio_direction_input;
> > -       gc->direction_output = pca9539_gpio_direction_output;
> > -       gc->get = pca9539_gpio_get_value;
> > -       gc->set = pca9539_gpio_set_value;
> > +       gc->direction_input  = pca953x_gpio_direction_input;
> > +       gc->direction_output = pca953x_gpio_direction_output;
> > +       gc->get = pca953x_gpio_get_value;
> > +       gc->set = pca953x_gpio_set_value;
> >
> >         gc->base = chip->gpio_start;
> > -       gc->ngpio = NR_PCA9539_GPIOS;
> > -       gc->label = "pca9539";
> > +       gc->ngpio = chip->desc->gpios;

I'd expect "desc" to vanish, and ngpio to be an input to this function
(see below).  Then ngpio could be used to choose the values to stuff
into chip->{input,output,invert,direction} register offsets.


> > +       gc->label = "pca953x";

Why not just set gc->label to be chip->client->name?

Might as well have /sys/kernel/debug/gpio output be
that little bit more useful.  And save the space that
would otherwise be wasted by that string.  :)


> >
> >         return gpiochip_add(gc);
> >  }
> >
> > -static int __devinit pca9539_probe(struct i2c_client *client)
> > +static int __devinit pca953x_probe(struct i2c_client *client)
> >  {
> > -       struct pca9539_platform_data *pdata;
> > -       struct pca9539_chip *chip;
> > +       struct pca953x_platform_data *pdata;
> > +       struct pca953x_chip *chip;
> >         int ret;
> > +       int variant;
> >
> >         pdata = client->dev.platform_data;
> >         if (pdata == NULL)
> >                 return -ENODEV;
> >
> > -       chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
> > +       if (!strcmp("pca9539", pdata->name))

Use client->name instead; there's no need to change pdata.


> > +               variant = PCA9539;
> > +       else if (!strcmp("pca9536", pdata->name))
> > +               variant = PCA9536;
> > +       else
> > +               return -ENODEV;
> > +
> 
> I prefer integer here more than string, for simplicity and efficiency.

Actually I'd use strings right now, and not change pdata at all.  But
compare client->name, using that to figure out how many GPIOs.

When, later, an i2c_device_id is passed in, the driver can get the
number of GPIOs handed to it directly ... and won't need to worry
about that ENODEV case.



> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -16,6 +16,10 @@
> >  #define ARCH_NR_GPIOS          256
> >  #endif
> >
> > +#ifndef NO_GPIO
> > +#define NO_GPIO                        ((unsigned int)-1)
> > +#endif
> > +
> 
> I don't understand this.

Me either; *ANY* negative number is invalid as a GPIO number,
not just "-1"...


> > --- a/include/linux/i2c/pca9539.h
> > +++ b/include/linux/i2c/pca953x.h
> > @@ -1,9 +1,12 @@
> > -/* platform data for the PCA9539 16-bit I/O expander driver */
> > +/* platform data for the PCA953x I/O expander driver */
> >
> > -struct pca9539_platform_data {
> > +struct pca953x_platform_data {
> >         /* number of the first GPIO */
> >         unsigned        gpio_base;
> >
> > +       /* chip variant. Currently supported "pca9536" and "pca9539" */
> > +       const char      *name;
> > +

That's not needed; i2c_client.name should identify the chip.

> >         /* initial polarity inversion setting */
> >         uint16_t        invert;
> >
> > 

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c