Re: [patch/rfc 2/4] pcf875x I2C GPIO expander driver

[David Brownell <david-b@pacbell.net>]

On Friday 30 November 2007, Jean Delvare wrote:
> Hi David,
> 
> Sorry for the late review.

I've got patches in my queue too, sigh ...

Thanks for the review.  I'll snip out typos and similar trivial
comments (and fix them!), using responses here for more the
substantive feedback.

- Dave


> > --- a/drivers/i2c/chips/Kconfig	2007-10-28 21:04:06.000000000 -0700
> > +++ b/drivers/i2c/chips/Kconfig	2007-10-29 14:16:01.000000000 -0700
> > @@ -51,6 +51,24 @@ config SENSORS_EEPROM
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called eeprom.
> >  
> > +config GPIO_PCF857X
> > +	tristate "PCF875x GPIO expanders"
> > +	depends on GPIO_LIB
> > +	help
> > +	  ...
> > +
> > +	  This driver provides only an in-kernel interface to those GPIOs.
> > +	  Any sysfs interface to userspace would be provided separately.
> 
> How?

I'll take that out, to avoid the question.  The answer is still mostly
TBD, but the gpiolib infrastructure provides a number of the hooks
that such a userspace interface would need.


> > +/**
> > + * struct pcf857x_platform_data - data to set up pcf857x driver
> > + * @gpio_base: number of the chip's first GPIO
> > + * @n_latch: optional bit-inverse of initial output state
> 
> Strange name, and I can't make much sense of the description either.

Updated description:

 * @n_latch: optional bit-inverse of initial register value; if
 *      you leave this initialized to zero, the driver will treat
 *      all bits as inputs as if the chip was just reset

This chip is documented as being "pseudo-bidirectional", which is
a sign that there are some confusing mechanisms lurking...


Conventions for naming negative-true signals include a "#" suffix
(illegal for C), a overbar (not expressible in ASCII), and prefixes
including "/" (illegal for C) and "n" (aha!).  I morphed the latter
into "n_" since it's often paired with all-caps signal names, as
in "nRESET", which are bad kernel coding style.

Latches hold values; http://en.wikipedia.org/wiki/Latch_%28electronics%29
talks about bit-level latching, but GPIO controllers use register-wide
latches to record the value that should be driven on output pins.
(As opposed to input pins, whose values are read without latching.)


> After reading this paragraph I still have no idea what n_latch does.
> But maybe that's just me.

It's a wierd little arrangement, maybe you have a better explanation.
I tried hitting the confusing points more directly:

 * These GPIO chips are only "pseudo-bidirectional"; read the chip specs
 * to understand the behavior.  They don't have separate registers to
 * record which pins are used for input or output, record which output
 * values are driven, or provide access to input values.  That must all
 * be inferred by reading the chip's value and knowing the last value
 * written to it.  If you don't initialize n_latch, that last written
 * value is presumed to be all ones (as if the chip were just reset).


> > --- a/drivers/i2c/chips/Makefile	2007-10-28 21:04:06.000000000 -0700
> > +++ b/drivers/i2c/chips/Makefile	2007-10-28 21:09:49.000000000 -0700
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
> >  obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
> >  obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
> >  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> > +obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
> 
> For alphabetical order, it would go one line above.

For alphabetical order it would go much sooner.
GPIO precedes SENSOR.  ;)


> >  obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
> >  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >  obj-$(CONFIG_SENSORS_TLV320AIC23) += tlv320aic23.o
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ b/drivers/i2c/chips/pcf857x.c	2007-10-29 14:12:21.000000000 -0700
> > @@ -0,0 +1,309 @@
> > +/*
> > + * pcf857x - driver for pcf857{4,4a,5,5c} I2C GPIO expanders
> 
> I recommend spelling out chip names completely, as it lets people grep
> the kernel tree for chip names when they look for support.

I'll do that -- but note that the names *are* spelled out later.


> > +#include <linux/pcf857x.h>
> 
> I suspect that there will be many more such header files in the future.
> Would it make sense to move them to include/linux/gpio?

I was thinking more like <linux/i2c/...> myself.  There are many more
I2C chips than GPIO expanders.


> > +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
> > +	...
> > +
> > +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
> > +{
> > +	pcf857x_output8(chip, offset, value);
> > +}
> 
> It would be more efficient to drop pcf857x_set8 altogether and do
> gpio->chip.set = pcf857x_output8.

No can do; return types differ, which means that on some platforms
the calling conventions have significant differences.


> > +                     dev_err(&client->dev, "%s --> %d\n",
> > +                                     "teardown", status);
>
> Why %s instead of hard-coding "teardown"?

To share (current code) three copies of the "<3>%s %s: %s --> %d\n"
string.  Every little bit of kernel bloat prevention helps.  ;)