From: Jean Delvare <khali@linux-fr.org>
To: David Brownell <david-b@pacbell.net>
Cc: Linux Kernel list <linux-kernel@vger.kernel.org>,
Felipe Balbi <felipebalbi@users.sourceforge.net>,
Bill Gatliff <bgat@billgatliff.com>,
Haavard Skinnemoen <hskinnemoen@atmel.com>,
Andrew Victor <andrew@sanpeople.com>,
Tony Lindgren <tony@atomide.com>,
"eric miao" <eric.y.miao@gmail.com>,
Kevin Hilman <khilman@mvista.com>,
Paul Mundt <lethal@linux-sh.org>,
Ben Dooks <ben@trinity.fluff.org>
Subject: Re: [patch/rfc 2/4] pcf875x I2C GPIO expander driver
Date: Fri, 30 Nov 2007 21:13:32 +0100 [thread overview]
Message-ID: <20071130211332.49a21a6b@hyperion.delvare> (raw)
In-Reply-To: <200711301040.54777.david-b@pacbell.net>
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
next prev parent reply other threads:[~2007-11-30 20:13 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <200710291809.29936.david-b@pacbell.net>
2007-10-30 1:51 ` [patch/rfc 1/4] GPIO implementation framework David Brownell
2007-11-05 21:05 ` David Brownell
2007-11-13 2:28 ` eric miao
2007-11-13 19:06 ` David Brownell
2007-11-14 0:57 ` eric miao
2007-11-14 1:00 ` eric miao
2007-11-14 1:02 ` eric miao
2007-11-14 1:03 ` eric miao
2007-11-14 1:04 ` eric miao
2007-11-14 1:04 ` eric miao
2007-11-14 4:36 ` David Brownell
2007-11-14 6:51 ` eric miao
2007-11-14 7:19 ` David Brownell
2007-11-14 7:36 ` eric miao
2007-11-17 10:38 ` Jean Delvare
2007-11-17 17:36 ` David Brownell
2007-11-20 15:20 ` Jean Delvare
2007-11-14 4:18 ` David Brownell
2007-11-14 6:46 ` eric miao
2007-11-14 3:28 ` David Brownell
2007-11-14 3:25 ` David Brownell
2007-11-14 3:53 ` David Brownell
2007-11-14 6:37 ` eric miao
2007-11-14 3:30 ` David Brownell
2007-11-14 6:40 ` eric miao
2007-11-14 7:08 ` David Brownell
2007-11-27 1:46 ` David Brownell
2007-11-27 10:58 ` eric miao
2007-11-27 17:26 ` David Brownell
2007-11-27 19:03 ` David Brownell
2007-11-27 19:29 ` David Brownell
2007-11-28 5:11 ` eric miao
2007-11-28 3:15 ` [patch/rfc 2.6.24-rc3-mm] gpiolib grows a gpio_desc David Brownell
2007-11-28 9:10 ` eric miao
2007-11-28 9:53 ` David Brownell
2007-10-30 1:51 ` [patch/rfc 2/4] pcf875x I2C GPIO expander driver David Brownell
2007-11-30 12:32 ` Jean Delvare
2007-11-30 13:04 ` Bill Gatliff
2007-11-30 13:36 ` Jean Delvare
2007-11-30 14:09 ` Bill Gatliff
2007-11-30 18:40 ` David Brownell
2007-11-30 20:13 ` Jean Delvare [this message]
2007-11-30 20:59 ` David Brownell
2008-04-04 2:06 ` Trent Piepho
2008-04-04 2:45 ` Ben Nizette
2008-04-04 3:33 ` Trent Piepho
2008-04-04 4:57 ` Ben Nizette
2008-04-05 4:05 ` userspace GPIO access (WAS: [patch/rfc 2/4] pcf875x ...) David Brownell
2008-04-07 17:56 ` Trent Piepho
2008-04-04 8:09 ` [patch/rfc 2/4] pcf875x I2C GPIO expander driver Jean Delvare
2008-04-04 19:07 ` Trent Piepho
2008-04-04 19:36 ` Jean Delvare
2008-04-04 20:18 ` Trent Piepho
2008-04-05 2:51 ` David Brownell
2008-04-05 2:53 ` David Brownell
2007-12-06 3:03 ` [patch/rfc 2/4] pcf857x " David Brownell
2007-12-06 23:17 ` Jean Delvare
2007-12-07 4:02 ` David Brownell
2007-10-30 1:53 ` [patch/rfc 3/4] DaVinci platform uses new GPIOLIB David Brownell
2007-10-30 1:54 ` [patch/rfc 4/4] DaVinci EVM uses pcf857x GPIO driver David Brownell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20071130211332.49a21a6b@hyperion.delvare \
--to=khali@linux-fr.org \
--cc=andrew@sanpeople.com \
--cc=ben@trinity.fluff.org \
--cc=bgat@billgatliff.com \
--cc=david-b@pacbell.net \
--cc=eric.y.miao@gmail.com \
--cc=felipebalbi@users.sourceforge.net \
--cc=hskinnemoen@atmel.com \
--cc=khilman@mvista.com \
--cc=lethal@linux-sh.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).