This is a new-style I2C driver for some common 8 and 16 bit I2C based

GPIO expanders:  pcf8574 and pcf8575.  Since it's a new-style driver,

it's configured as part of board-specific init ... eliminating the

need for error-prone manual configuration of module parameters.
The driver exposes the GPIO signals using the platform-neutral GPIO

programming interface, so they are easily accessed by other kernel

code.  The lack of such a flexible kernel API is what has ensured

the proliferation of board-specific hacks for these chips... stuff

that rarely makes it upstream since it's so ugly.  This driver will

let such board-specific code use standard GPIO calls.
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

---

Note that there's currently a drivers/i2c/chips/pcf8574.c driver.
Key differences include:  this one talks to other kernel code so

it can use the GPIOs "normally", but that one talks to userspace

through sysfs.  Also, this one is a "new style" I2C driver, so it's

smaller and doesn't need all those error-prone module parameters.

Plus, this one handles both 8 and 16 bit chip versions.
 drivers/i2c/chips/Kconfig   |   18 ++

 drivers/i2c/chips/Makefile  |    1

 drivers/i2c/chips/pcf857x.c |  309 ++++++++++++++++++++++++++++++++++++++++++++

 include/linux/pcf857x.h     |   43 ++++++

 4 files changed, 371 insertions(+)
--- 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

+	  Say yes here to provide access to some I2C GPIO expanders which

+	  may be used for additional digital outputs or inputs:

+

+	    - pcf8574, pcf8574a ... 8 bits, from NXP or TI

+	    - pcf8575, pcf8575c ... 16 bits, from NXP or TI

+

+	  Your board setup code will need to declare ...
[ message continues ]

Hi David,
Sorry for the late review.
Note that I can't test your code.
Would make more sense to list setup and teardown before context?
After reading this paragraph I still have no idea what n_latch does.
I recommend spelling out chip names completely, as it lets people grep
I suspect that there will be many more such header files in the future.
!!(value & (1 << offset))

is more efficiently written
It would be more efficient to drop pcf857x_set8 altogether and do

gpio->chip.set = pcf857x_output8.
Might make sense to move this to a common error path, as you do it more
Why %s instead of hard-coding "teardown"?
Why is this an error message while a failing setup() at initialization
No MODULE_AUTHOR? No entry in MAINTAINERS?
--

Jean Delvare

-

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.
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
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.
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
For alphabetical order it would go much sooner.
I was thinking more like <linux/i2c/...> myself.  There are many more
No can do; return types differ, which means that on some platforms
To share (current code) three copies of the "<3>%s %...
[ message continues ]

Here's the current version of this patch ... updated to put the

driver into drivers/gpio (separate patch setting that up) and

the header into <linux/i2c/pcf857x.h>
Note that after looking at the GPIO expanders listed at the NXP

website, I updated this to accept a few more of these chips.

Other than reset pins and addressing options, the key difference

between these seems to be the top I2C clock speed supported:
 pcf857x ...  100 KHz

 pca857x ...  400 KHz

 pca967x ... 1000 KHz
Otherwise they're equivalent at the level of just swapping parts.
- Dave
=============	SNIP!

This is a new-style I2C driver for most common 8 and 16 bit I2C based

"quasi-bidirectional" GPIO expanders:  pcf8574 or pcf8575, and several

compatible models (mostly faster, supporting I2C at up to 1 MHz).
Since it's a new-style driver, these devices must be configured as

part of board-specific init.  That eliminates the need for error-prone

manual configuration of module parameters, and makes compatibility

with legacy drivers (pcf8574.c, pc8575.c)for these chips easier.
The driver exposes the GPIO signals using the platform-neutral GPIO

programming interface, so they are easily accessed by other kernel

code.  The lack of such a flexible kernel API is what has ensured

the proliferation of board-specific drivers for these chips... stuff

that rarely makes it upstream since it's so ugly.  This driver will

let them use standard calls.
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

---

 drivers/gpio/Kconfig        |   23 +++

 drivers/gpio/Makefile       |    2

 drivers/gpio/pcf857x.c      |  331 ++++++++++++++++++++++++++++++++++++++++++++

 include/linux/i2c/pcf857x.h |   45 +++++

 4 files changed, 401 insertions(+)
--- a/drivers/gpio/Kconfig	2007-12-05 15:13:27.000000000 -0800

+++ b/drivers/gpio/Kconfig	2007-12-05 15:14:12.000000000 -0800

@@ -5,4 +5,27 @@

 menu "GPIO Support"

 	depends on GPIO_LIB
+config GPIO_PCF857X

+	tristate "PCF857x, PCA857x, and PCA967x I2C...
[ message continues ]

Hi David,
Missing space after closing parenthesis. Also, I don't quite see what

is supposed to make compatibility with the legacy drivers easier, nor
This is no longer a boolean value, is that OK? I guess that it doesn't

matter but maybe it should be documented (what GPIO drivers are allowed
Shouldn't this be degraded to dev_warn? The probe still succeeds. Or

keep dev_err but make the probe fail (in which case you'll probably
The rest looks fine to me.
--

Jean Delvare

--

Already documented -- as zero/nonzero, the original boolean model for C.
Thanks for the comments.  I'll send this in with the next batch

of gpiolib patches.
- Dave
--

Hi David,
So the user-space interface would be part of the generic GPIO
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
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
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
Ah, right, sorry for missing that. I had only looked at the parameters
Only two copies in the version you posted, but indeed there would be

three if the trick was applied consistently.
--

Jean Delvare

-

I thought that would make sense too! :)  Someone would need to

write the code though.  Having such a mechanism would provide

another "carrot" to migrate folk towards the gpiolib core.
I think adding a gpiochip primitive to mark a (potential) GPIO

as invalid would support the converse of /sys/kernel/debug/gpio.

Invalid GPIOs include pins set up for non-GPIO usage (like being

used for MMC or MII), or not wired up on a given board.  Pins

that were valid as GPIOs and not requested by a kernel driver
I expect it will become a lot more common.  Remember that legacy

I2C drivers *couldn't* get any board-specific config data; that's

been problematic, since it meant the drivers themselves ended up

with lots of board-specific cruft.   That prevented many drivers

from going upstream at all.  (As I mentioned about pcf8574 code,

although in that case the problem was worsened by lack of any
Could be.  Right now we have three "GPIO expander" drivers using

the new "gpiolib" framework:  pcf875x and pca9539 for I2C, and

mcp23s08 for SPI.  There are many more that *could* be used with

Linux boxes.  And there are other drivers/XYZ directories that

are (currently) that small.  Maybe gpiolib should go upstream

like that, and lib/gpiolib should be in drivers/gpio too...
However, keep in mind that lots of chips export a few GPIOs but

don't have that as their core functionality ... one example is

the drivers/i2c/chips/tps65010 driver.  So it'd never be the

case that GPIO drivers only live in that directory.
- Dave

-

Here's some code to do this.  It's not entirely perfect yet, but it is

usable.  I create a directory in sysfs (class/gpio/<name>:<num>) for each

gpio line.  The are files in this directory to allow reading/setting the

gpio value and direction.  One can also see the label associated with the

gpio if gpiolib is keeping track of labels (i.e.  debugfs is on).  sysfs

also creates a gpio directory under the device associated with the gpio

lines (this is a new thing one needs to add) with that device's gpios.

Actually, think these are "real" sysfs directories, and the ones in

/sys/class are copies made by sysfs.
----------------------------------------------------------------------------

From c65d0fb239b79de7f595e47edb2fb641217e7309 Mon Sep 17 00:00:00 2001

From: Trent Piepho <tpiepho@freescale.com>

Date: Thu, 3 Apr 2008 18:37:23 -0700

Subject: GPIO: Create a sysfs gpio class for messing with GPIOs from userspace
struct gpio_chip gets a new field, dev, which points to the struct device

of whatever the gpio lines are part of.  If this is non-NULL, gpio class

entries are created for this device when gpiochip_add() is called, by a new

function gpiochip_classdev_register().
It creates a sysfs directory for each gpio the chip defines.  Each device

has three attributes so far, direction, value, and label.  label is

read-only, and will only be present if DEBUG_FS is on, as without DEBUG_FS

the gpio layer doesn't keep track of any labels.
Maybe the value file should be changed to RO or WO depending on direction?
Setting the direction auto-allocates the gpio, which is not really what I

wanted.  Maybe I should call the chip set method directly?
There are almost certainly a bunch of races with gpio_desc access.
No code has been written yet to remove the devices from the class when the

class is removed.
The GPIO_CLASS define/ifdef code should either be removed, or turned into a

Kconfig variable that can be used turn this feature on and off.
Signed-off-by: Trent Pi...
[ message continues ]

Hi Trent,
Purely technical review (I can't say whether this interface is better
Confusing construct... I suggest using sprintf instead, which will
As far as I know, the string you receive from sysfs is guaranteed to be
This exposes to user-space the so far internal-only decision to encode

output as 1 and input as 0. I don't see much benefit in doing this, in

particular when you don't check for errors so for example an input of

"Out" would result in value 0 i.e. input mode. I'd rather support only
Why not just return gpio_get_value(n) in all cases? User might want to
Why "+ 1"? As far as I know you are not supposed to return a trailing
--

Jean Delvare

--

So, after trimming trailing whitespace:
	if (strcmp(buf, "out") == 0)

		gpio_direction_output(...)

	else if (strcmp(buf, "in") == 0)

		gpio_direction_input(...)
--

I guess I wanted to support the interface of using out or 1 vs in or 0, since

both seemed like the two most obvious ways of setting it.  But direction uses
I thought the gpiolib layer didn't let you read an output, but I see that's

not the case now.  Maybe it's changed since the first revision?  I've changed

this to call gpio_get_value(n) for outputs too.
Though for the MPC8572 GPIO driver I wrote, it doesn't support reading

outputs.  The hardware doesn't allow it (IMHO, a design flaw), and working

around this significantly slows down GPIO functions.  What I'm trying to do

with the GPIO lines is going to be slower than desired no matter how fast I

make the gpio code (which is almost entirely responsible for the final speed),

so slowing down reads by a factor of four or more just to read back outputs
I know, but I'm not using modules for the system this is in, so it will never

get called.  What's the point of writing code I'll never use if this isn't
It would be easier to call gpiochip_classdev_register() earlier when the spin

lock is held, but one can't because it could sleep.  The comment was a warning
Looks like mailer damage from something, it's correct in my patch file.

--

I don't recall ever disallowing reading output values ... the

exact behavior is platform-specific, though it "should" be the

value actually sensed at the pin, which might not be the same
Then don't worry about it.  Any portable code can't rely on

being able to do that.
--

Can you prove that it is actually less efficient, and if so, by how

much? The time spent in this single function if probably insignificant

in comparison to the whole chain from the user-space process to the

GPIO chip.
Not that it really matters anyway, this is in no way a hot path

so clarity and correctness definitely take over efficiency. And the

code above is actually incorrect: as I mentioned elsewhere in this

thread, you aren't support to include trailing \0s in the buffer you

pass back to sysfs. Not all programming languages use \0 for string
Because most certainly your code won't be accepted upstream until this

is fixed, and presumably you posted this patch in the hope that it

would go upstream ;) Just because it isn't useful to you doesn't mean

it won't be useful to others. Otherwise this particular piece of code
Oops, sorry. I totally missed the "can't" in the comment.
--

Jean Delvare

--

I think sprintf will parse the format string, and then end up calling the same

strcpy() call to handle a %s.  Since sprintf() contains the strcpy(), it has

to be slower than strcpy alone.
I fixed all those.  I wasn't clear on that when I wrote this code and forgot I
I guess I was waiting for a "this could go upstream if you fix this" or "this

won't go upstream even if you fix it" so I don't waste time writing code no

one is interested in.

--

I quite like the fact that this easily tracks labels but I like the

interface of simple_gpio posted a few days back:

http://lkml.org/lkml/2008/3/26/87
Either way, anything unified is good.
	--Ben.

--

Always too slow posting my patches.  I wrote this two months ago when there

wasn't anything else.
A char device allows better permissions and could be more efficient, if one

really wants to do extensive control of gpio lines from userspace.  I can see

how it might be preferrable in some instances.
The nice thing about sysfs is that you don't need any extra software to

interact with it.  It's very convienent when you're just trying to debug the

gpio driver you're writing or verify that the gpio lines you just connected

are doing things.  It's also nice to be able to say something like:

# run these commands to un-write protect flash

echo out > /sys/class/gpio/MPC85XX:5/direction

echo 1 > /sys/class/gpio/MPC85XX:5/value
Instead of a complicated process that includes directions for creating the

correct device file, compiling a program that will set gpio lines, downloading

said program's source, and so on.

--

I've got something similar myself; mine's more complex, handles and

simple_gpio allows

echo "O1" > /dev/gpioN
Creating the device file is largely mdev/udev's job, most people don't

have to worry about it.
As I mentioned before (and in my review of simple_gpio) I really like

the idea of having labels attached to the gpio numbers in some way; as

it stands I see that as simple_gpio's only major weakness.
David, you're kinda the gatekeeper here; any input from you on which

approach is to be preferred, essential features etc?
--Ben.
--

I won't much care about /dev/... vs /sys/... though I'd

probably have used sysfs myself (just because it's much

simpler and doesn't need to imply mdev/udev and classes).
The configuration part of each driver bothers me:
 - Mike's simple_gpio requires manual kernel config

   to set up the platform_device nodes ... and thus

   rules out the first usage scenarios I ever heard

   of for such a userspace mechanism.
 - Trent's gpio_class exposes all GPIOs, even ones

   that are claimed by kernel drivers ... and thus

   makes it easy to clobber kernel driver state.

   (Plus it won't work on most built-in GPIOs, since

   they by and large don't have parent devices.)
What I'd like to see is userspace config commands to

cause the gpio_request() ... *maybe* something like
    echo 42 foo 0 > .../gpio_config
	... causing error-checked versions of:
    gpio_request(42, "foo")

    gpio_direction_output(42, 0)
	... then some .../gpio42 file, read/write, appears
and
    echo 84 bar in > .../gpio_config
	... causing error-checked versions of:
    gpio_request(84, "bar")

    gpio_direction_input(84)
	... then some .../gpio84 file, read-only, appears
Though arguably the label could just always be "userspace"

(it's mostly for /sys/kernel/debug/gpio), and the default could

be to configure as an input (unless an output value was given).

Plus, there should be some way to cause gpio_free() too.
A potential advantage of the /dev/... node approach would be

that it's easier to support an IRQ-backed poll() mechanism

for inputs.
- Dave
--

This was intentional.  When you're developing said kernel drivers, or

connecting hardware they're supposed to drive, it's very handy to be able

to set and read the GPIOs from userspace.  At least, when writing a

gpiolib driver and code that used gpiolib, I found this ability very

useful, so I thought other developers might as well.
I suppose one could make the sys files read-only once a kernel driver

allocates a gpio.  But it would be nice to have the ability to make them
Couldn't they always add one?  My GPIO driver is part of the CPU/SoC, and

it has a device node.  It's pretty easy to add a platform device, and

probably cleaner than not associating a device with the gpio driver.

From my understanding of sysfs, it seems any sysfs based approach has to
Suppose I took the code I had, and make the label file writable?  Writing

to it allocates the gpio with the written label?  That would be

relatively simple to add.  Is there any reason why the GPIOs should

appear in sysfs by default?  They are devices, and most other devices
Write a blank label?  Too bad one can't "rm" sysfs files, that would be a

neat way to trigger stuff.  I can see it used to hot-unplug a pci device,

just delete the slot.

--

... but not more efficiently implemented.
Your version requires code to do the shift on live data at runtime.

David's version lets the compiler create the mask once, at compile-time.
b.g.
--

Bill Gatliff

bgat@billgatliff.com
-

Hi Bill,
I don't understand. How could the compiler create the mask at

compile-time when "offset" is only known at runtime?
Anyway, I just checked and gcc (4.1.2 on x86_86) generates the same

code for both expressions, so there's not much to argue about. David,

feel free to ignore my comment if you prefer your implementation :)
--

Jean Delvare

-

Oops.  :)  I was thinking of some different code.  Disregard.
/me got up too early this morning, after working too late last night!
b.g.
--

Bill Gatliff

bgat@billgatliff.com
-