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

Hi David,

On Fri, 30 Nov 2007 10:40:54 -0800, David Brownell wrote:
On Friday 30 November 2007, Jean Delvare wrote:
--- 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.

So the user-space interface would be part of the generic GPIO
infrastructure? I like the idea.

+/**
+ * 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).

Much clearer now, thanks. I know what a latch is, I just couldn't get
how latching (or lack thereof) was related with an initial register
value. With the explanation above, I get it.

--- 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. ;)

We apply the alphabetical order to driver names, not configuration
symbols, as far as I know. Though for most directories it probably
doesn't make a difference; drivers/i2c/chips is admittedly a bit messy
in this respect.

Note that at some point I will attempt to get rid of the "SENSORS" part
of configuration options that have nothing to do with sensors, that
should help a bit.

+#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.

But most i2c chip drivers don't need a header file. Or is this going to
change with the new-style i2c drivers?

Along the same line, I am wondering if it would make sense to put the
various GPIO drivers in drivers/gpio. It's a much better practice to
group the drivers according to the functionality they provide than the
way they are connected to the system. drivers/i2c/chips is an exception
in this respect, it's meant for i2c drivers that have no obvious place
to live in. That's why there aren't many drivers there, and I hope it
will stay this way. In an ideal world we could even get rid of this
directory and move the remaining drivers to drivers/misc.

+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.

Ah, right, sorry for missing that. I had only looked at the parameters
and forgot the return type.

+                     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. ;)

Only two copies in the version you posted, but indeed there would be
three if the trick was applied consistently.

--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [RFC 2.6.27 1/1] gpiolib: add support for batch set of pins
    ... kind of faults in the middle of bitbanging loops ... ... it's extremely cheap to "return 0" in GPIO drivers which can't error. ... a GPIO-fanout chip over I2C/SPI, where that chip is itself feeding a ...
    (Linux-Kernel)
  • Re: [PATCH] ALS: TSL2550 driver move from i2c/chips
    ... with als. ... apparently doesn't allow for simultaneous probing of multiple drivers ... This only becomes and issue if we have two sensors measuring illuminance ... (even if chips are only fairly coarsely approximating it. ...
    (Linux-Kernel)
  • Re: [PATCH] ALS: TSL2550 driver move from i2c/chips
    ... with als. ... apparently doesn't allow for simultaneous probing of multiple drivers ... This only becomes and issue if we have two sensors measuring illuminance ... (even if chips are only fairly coarsely approximating it. ...
    (Linux-Kernel)
  • Re: Issues with LED grid driving
    ... I have seen 24-ch led drivers along ... Think of the grid as a led matrix display panel as essentially it is ... then the remaining supply voltage is dropped across the chip. ... Unless the chip is in a special thermal package, most of the heat ...
    (sci.electronics.design)
  • Re: Issues with LED grid driving
    ... I have seen 24-ch led drivers along ... Some claim the eye retains the peak value, ... Market forces being what they are, you make a chip that multiplexes ...
    (sci.electronics.design)