[patch/rfc 2/4] pcf875x I2C GPIO expander driver - Kernel

This is a discussion on [patch/rfc 2/4] pcf875x I2C GPIO expander driver - Kernel ; 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 ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 23

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

  1. [patch/rfc 2/4] pcf875x I2C GPIO expander driver

    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
    ---
    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 the expanders in
    + use, and assign numbers to the GPIOs they expose. Those GPIOs
    + can then be used from drivers and other kernel code, just like
    + other GPIOs, but only accessible from task contexts.
    +
    + This driver provides only an in-kernel interface to those GPIOs.
    + Any sysfs interface to userspace would be provided separately.
    +
    config SENSORS_PCF8574
    tristate "Philips PCF8574 and PCF8574A"
    depends on EXPERIMENTAL
    --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    +++ b/include/linux/pcf857x.h 2007-10-28 21:09:49.000000000 -0700
    @@ -0,0 +1,43 @@
    +#ifndef __LINUX_PCF857X_H
    +#define __LINUX_PCF857X_H
    +
    +/**
    + * 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
    + * @context: optional parameter passed to setup() and teardown()
    + * @setup: optional callback issued once the GPIOs are valid
    + * @teardown: optional callback issued before the GPIOs are invvalidated
    + *
    + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
    + * the i2c_board_info used with the pcf875x driver must provide the
    + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
    + * platform_data (pointer to one of these structures) with at least
    + * the gpio_base value initialized.
    + *
    + * The @setup callback may be used with the kind of board-specific glue
    + * which hands the (now-valid) GPIOs to other drivers, or which puts
    + * devices in their initial states using these GPIOs.
    + *
    + * Since these GPIO chips don't have conventional registers recording
    + * whether a pin is used for input or output, or an output latch to
    + * record the values being driven, the n_latch value may be used to
    + * avoid intialization glitches. Its inverted value initializes the
    + * value into which bits are masked before they're written to the PCF
    + * chip. That means that if it's left at zero, the chip is treated as
    + * if it came from power-up reset.
    + */
    +struct pcf857x_platform_data {
    + unsigned gpio_base;
    + unsigned n_latch;
    +
    + void *context;
    + int (*setup)(struct i2c_client *client,
    + int gpio, unsigned ngpio,
    + void *context);
    + int (*teardown)(struct i2c_client *client,
    + int gpio, unsigned ngpio,
    + void *context);
    +};
    +
    +#endif /* __LINUX_PCF857X_H */
    --- 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
    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
    + *
    + * Copyright (C) 2007 David Brownell
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License as published by
    + * the Free Software Foundation; either version 2 of the License, or
    + * (at your option) any later version.
    + *
    + * This program is distributed in the hope that it will be useful,
    + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    + * GNU General Public License for more details.
    + *
    + * You should have received a copy of the GNU General Public License
    + * along with this program; if not, write to the Free Software
    + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
    + */
    +
    +#include
    +#include
    +#include
    +
    +#include
    +
    +
    +/*
    + * The pcf857x chips only expose a one read register and one write register.
    + * Writing a "one" bit (to match the reset state) lets that pin be used as
    + * an input; it's not an open-drain model, but it acts a bit like that.
    + *
    + * Some other I2C GPIO expander chips (like the pca9534, pca9535, pca9555,
    + * pca9539, mcp23008, and mc23017) have a more complex register model with
    + * more conventional input circuitry, often using 0x20..0x27 addresses.
    + */
    +struct pcf857x {
    + struct gpio_chip chip;
    + struct i2c_client *client;
    + unsigned out; /* software latch */
    +};
    +
    +/*-------------------------------------------------------------------------*/
    +
    +/* Talk to 8-bit I/O expander */
    +
    +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    +
    + gpio->out |= (1 << offset);
    + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
    +}
    +
    +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + s32 value;
    +
    + value = i2c_smbus_read_byte(gpio->client);
    + return (value < 0) ? 0 : !!(value & (1 << offset));
    +}
    +
    +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + unsigned bit = 1 << offset;
    +
    + if (value)
    + gpio->out |= bit;
    + else
    + gpio->out &= ~bit;
    + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);
    +}
    +
    +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + pcf857x_output8(chip, offset, value);
    +}
    +
    +/*-------------------------------------------------------------------------*/
    +
    +/* Talk to 16-bit I/O expander */
    +
    +static int i2c_write_le16(struct i2c_client *client, u16 word)
    +{
    + u8 buf[2] = { word & 0xff, word >> 8, };
    + int status;
    +
    + status = i2c_master_send(client, buf, 2);
    + return (status < 0) ? status : 0;
    +}
    +
    +static int i2c_read_le16(struct i2c_client *client)
    +{
    + u8 buf[2];
    + int status;
    +
    + status = i2c_master_recv(client, buf, 2);
    + if (status < 0)
    + return status;
    + return (buf[1] << 8) | buf[0];
    +}
    +
    +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    +
    + gpio->out |= (1 << offset);
    + return i2c_write_le16(gpio->client, (u16) gpio->out);
    +}
    +
    +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + int value;
    +
    + value = i2c_read_le16(gpio->client);
    + return (value < 0) ? 0 : !!(value & (1 << offset));
    +}
    +
    +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + unsigned bit = 1 << offset;
    +
    + if (value)
    + gpio->out |= bit;
    + else
    + gpio->out &= ~bit;
    + return i2c_write_le16(gpio->client, (u16) gpio->out);
    +}
    +
    +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + pcf857x_output16(chip, offset, value);
    +}
    +
    +/*-------------------------------------------------------------------------*/
    +
    +static int pcf857x_probe(struct i2c_client *client)
    +{
    + struct pcf857x_platform_data *pdata;
    + struct pcf857x *gpio;
    + int status = 0;
    +
    + pdata = client->dev.platform_data;
    + if (!pdata)
    + return -ENODEV;
    +
    + /* Allocate, initialize, and register this gpio_chip. */
    + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
    + if (!gpio)
    + return -ENOMEM;
    +
    + gpio->chip.base = pdata->gpio_base;
    + gpio->chip.can_sleep = 1;
    +
    + /* NOTE: the OnSemi jlc1562b is also largely compatible with
    + * these parts, notably for output. It has a low-resolution
    + * DAC instead of pin change IRQs; and its inputs can be the
    + * result of comparators.
    + */
    +
    + /* '74a addresses are 0x38..0x3f; '74 uses 0x20..0x27 */
    + if (strcmp(client->name, "pcf8574a") == 0
    + || strcmp(client->name, "pcf8574") == 0) {
    + gpio->chip.ngpio = 8;
    + gpio->chip.direction_input = pcf857x_input8;
    + gpio->chip.get = pcf857x_get8;
    + gpio->chip.direction_output = pcf857x_output8;
    + gpio->chip.set = pcf857x_set8;
    +
    + if (!i2c_check_functionality(client->adapter,
    + I2C_FUNC_SMBUS_BYTE))
    + status = -EIO;
    +
    + /* fail if there's no chip present */
    + status = i2c_smbus_read_byte(client);
    +
    + /* '75/'75c addresses are 0x20..0x27, just like the '74;
    + * the '75c doesn't have a current source pulling high.
    + */
    + } else if (strcmp(client->name, "pcf8575c") == 0
    + || strcmp(client->name, "pcf8575") == 0) {
    + gpio->chip.ngpio = 16;
    + gpio->chip.direction_input = pcf857x_input16;
    + gpio->chip.get = pcf857x_get16;
    + gpio->chip.direction_output = pcf857x_output16;
    + gpio->chip.set = pcf857x_set16;
    +
    + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
    + status = -EIO;
    +
    + /* fail if there's no chip present */
    + status = i2c_read_le16(client);
    +
    + } else
    + status = -ENODEV;
    +
    + if (status < 0) {
    + dev_dbg(&client->dev, "probe error %d for '%s'\n",
    + status, client->name);
    + kfree(gpio);
    + return status;
    + }
    +
    + gpio->chip.label = client->name;
    +
    + gpio->client = client;
    + i2c_set_clientdata(client, gpio);
    +
    + /* NOTE: these chips have strange "pseudo-bidirectional" I/O pins.
    + * We can't actually know whether a pin is configured (a) as output
    + * and driving the signal low, or (b) as input and reporting a low
    + * value ... without knowing the last value written since the chip
    + * came out of reset (if any). We can't read the latched output.
    + *
    + * In short, the only reliable solution for setting up pin direction
    + * is to do it explicitly. The setup() method can do that.
    + *
    + * We use pdata->n_latch to avoid trouble. In the typical case it's
    + * left initialized to zero; our software copy of the "latch" then
    + * matches the chip's reset state. But there may be cases where a
    + * system must drive some pins low, without transient glitching.
    + * Handle those cases by assigning n_latch to a nonzero value.
    + */
    + gpio->out = ~pdata->n_latch;
    +
    + status = gpiochip_add(&gpio->chip);
    + if (status < 0) {
    + kfree(gpio);
    + return status;
    + }
    +
    + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
    + * don't yet even try to use. Among other issues, the relevant
    + * genirq state isn't available to modular drivers; and most irq
    + * methods can't be called from sleeping contexts.
    + */
    +
    + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
    + gpio->chip.base,
    + gpio->chip.base + gpio->chip.ngpio - 1,
    + client->name,
    + client->irq ? " (irq ignored)" : "");
    +
    + /* Let platform code set up the GPIOs and their users.
    + * Now is the first time anyone can use them.
    + */
    + if (pdata->setup) {
    + status = pdata->setup(client,
    + gpio->chip.base, gpio->chip.ngpio,
    + pdata->context);
    + if (status < 0)
    + dev_dbg(&client->dev, "setup --> %d\n", status);
    + }
    +
    + return 0;
    +}
    +
    +static int pcf857x_remove(struct i2c_client *client)
    +{
    + struct pcf857x_platform_data *pdata = client->dev.platform_data;
    + struct pcf857x *gpio = i2c_get_clientdata(client);
    + int status = 0;
    +
    + if (pdata->teardown) {
    + status = pdata->teardown(client,
    + gpio->chip.base, gpio->chip.ngpio,
    + pdata->context);
    + if (status < 0) {
    + dev_err(&client->dev, "%s --> %d\n",
    + "teardown", status);
    + return status;
    + }
    + }
    +
    + status = gpiochip_remove(&gpio->chip);
    + if (status == 0)
    + kfree(gpio);
    + else
    + dev_err(&client->dev, "%s --> %d\n", "remove", status);
    + return status;
    +}
    +
    +static struct i2c_driver pcf857x_driver = {
    + .driver = {
    + .name = "pcf857x",
    + .owner = THIS_MODULE,
    + },
    + .probe = pcf857x_probe,
    + .remove = pcf857x_remove,
    +};
    +
    +static int __init pcf857x_init(void)
    +{
    + return i2c_add_driver(&pcf857x_driver);
    +}
    +/* we want GPIOs to be ready at device_initcall() time */
    +subsys_initcall(pcf857x_init);
    +
    +static void __exit pcf857x_exit(void)
    +{
    + i2c_del_driver(&pcf857x_driver);
    +}
    +module_exit(pcf857x_exit);
    +
    +MODULE_LICENSE("GPL");
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Hi David,

    Sorry for the late review.

    Note that I can't test your code.

    On Mon, 29 Oct 2007 18:51:48 -0700, David Brownell wrote:
    > 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
    > ---
    > Note that there's currently a drivers/i2c/chips/pcf8574.c driver.


    By now there's also a drivers/i2c/chips/pcf8575.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 the expanders in
    > + use, and assign numbers to the GPIOs they expose. Those GPIOs
    > + can then be used from drivers and other kernel code, just like
    > + other GPIOs, but only accessible from task contexts.
    > +
    > + This driver provides only an in-kernel interface to those GPIOs.
    > + Any sysfs interface to userspace would be provided separately.


    How?

    > +
    > config SENSORS_PCF8574
    > tristate "Philips PCF8574 and PCF8574A"
    > depends on EXPERIMENTAL
    > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    > +++ b/include/linux/pcf857x.h 2007-10-28 21:09:49.000000000 -0700
    > @@ -0,0 +1,43 @@
    > +#ifndef __LINUX_PCF857X_H
    > +#define __LINUX_PCF857X_H
    > +
    > +/**
    > + * 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.

    > + * @context: optional parameter passed to setup() and teardown()
    > + * @setup: optional callback issued once the GPIOs are valid
    > + * @teardown: optional callback issued before the GPIOs are invvalidated


    Would make more sense to list setup and teardown before context?

    Typo: invalidated.

    > + *
    > + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
    > + * the i2c_board_info used with the pcf875x driver must provide the
    > + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
    > + * platform_data (pointer to one of these structures) with at least
    > + * the gpio_base value initialized.
    > + *
    > + * The @setup callback may be used with the kind of board-specific glue
    > + * which hands the (now-valid) GPIOs to other drivers, or which puts
    > + * devices in their initial states using these GPIOs.
    > + *
    > + * Since these GPIO chips don't have conventional registers recording
    > + * whether a pin is used for input or output, or an output latch to
    > + * record the values being driven, the n_latch value may be used to
    > + * avoid intialization glitches. Its inverted value initializes the


    Typo: initialization.

    > + * value into which bits are masked before they're written to the PCF
    > + * chip. That means that if it's left at zero, the chip is treated as
    > + * if it came from power-up reset.


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

    > + */
    > +struct pcf857x_platform_data {
    > + unsigned gpio_base;
    > + unsigned n_latch;
    > +
    > + void *context;
    > + int (*setup)(struct i2c_client *client,
    > + int gpio, unsigned ngpio,
    > + void *context);
    > + int (*teardown)(struct i2c_client *client,
    > + int gpio, unsigned ngpio,
    > + void *context);
    > +};
    > +
    > +#endif /* __LINUX_PCF857X_H */
    > --- 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.

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

    > + *
    > + * Copyright (C) 2007 David Brownell
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License as published by
    > + * the Free Software Foundation; either version 2 of the License, or
    > + * (at your option) any later version.
    > + *
    > + * This program is distributed in the hope that it will be useful,
    > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + *
    > + * You should have received a copy of the GNU General Public License
    > + * along with this program; if not, write to the Free Software
    > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
    > + */
    > +
    > +#include
    > +#include
    > +#include


    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?

    > +
    > +#include
    > +
    > +
    > +/*
    > + * The pcf857x chips only expose a one read register and one write register.


    Typo: extra "a".

    > + * Writing a "one" bit (to match the reset state) lets that pin be used as
    > + * an input; it's not an open-drain model, but it acts a bit like that.
    > + *
    > + * Some other I2C GPIO expander chips (like the pca9534, pca9535, pca9555,
    > + * pca9539, mcp23008, and mc23017) have a more complex register model with
    > + * more conventional input circuitry, often using 0x20..0x27 addresses.
    > + */
    > +struct pcf857x {
    > + struct gpio_chip chip;
    > + struct i2c_client *client;
    > + unsigned out; /* software latch */
    > +};
    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +/* Talk to 8-bit I/O expander */
    > +
    > +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > +
    > + gpio->out |= (1 << offset);
    > + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);


    Useless cast.

    > +}
    > +
    > +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + s32 value;
    > +
    > + value = i2c_smbus_read_byte(gpio->client);
    > + return (value < 0) ? 0 : !!(value & (1 << offset));


    !!(value & (1 << offset))
    is more efficiently written
    (value >> offset) & 1

    > +}
    > +
    > +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + unsigned bit = 1 << offset;
    > +
    > + if (value)
    > + gpio->out |= bit;
    > + else
    > + gpio->out &= ~bit;
    > + return i2c_smbus_write_byte(gpio->client, (u8) gpio->out);


    Useless cast.

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

    Many of the comments above apply to the 16-bit functions below as well.

    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +/* Talk to 16-bit I/O expander */
    > +
    > +static int i2c_write_le16(struct i2c_client *client, u16 word)
    > +{
    > + u8 buf[2] = { word & 0xff, word >> 8, };


    Stray comma.

    > + int status;
    > +
    > + status = i2c_master_send(client, buf, 2);
    > + return (status < 0) ? status : 0;
    > +}
    > +
    > +static int i2c_read_le16(struct i2c_client *client)
    > +{
    > + u8 buf[2];
    > + int status;
    > +
    > + status = i2c_master_recv(client, buf, 2);
    > + if (status < 0)
    > + return status;
    > + return (buf[1] << 8) | buf[0];
    > +}
    > +
    > +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > +
    > + gpio->out |= (1 << offset);
    > + return i2c_write_le16(gpio->client, (u16) gpio->out);
    > +}
    > +
    > +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + int value;
    > +
    > + value = i2c_read_le16(gpio->client);
    > + return (value < 0) ? 0 : !!(value & (1 << offset));
    > +}
    > +
    > +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + unsigned bit = 1 << offset;
    > +
    > + if (value)
    > + gpio->out |= bit;
    > + else
    > + gpio->out &= ~bit;
    > + return i2c_write_le16(gpio->client, (u16) gpio->out);
    > +}
    > +
    > +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + pcf857x_output16(chip, offset, value);
    > +}
    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +static int pcf857x_probe(struct i2c_client *client)
    > +{
    > + struct pcf857x_platform_data *pdata;
    > + struct pcf857x *gpio;
    > + int status = 0;


    Useless initialization.

    > +
    > + pdata = client->dev.platform_data;
    > + if (!pdata)
    > + return -ENODEV;
    > +
    > + /* Allocate, initialize, and register this gpio_chip. */
    > + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);


    You need to include to have access to this function.

    > + if (!gpio)
    > + return -ENOMEM;
    > +
    > + gpio->chip.base = pdata->gpio_base;
    > + gpio->chip.can_sleep = 1;
    > +
    > + /* NOTE: the OnSemi jlc1562b is also largely compatible with
    > + * these parts, notably for output. It has a low-resolution
    > + * DAC instead of pin change IRQs; and its inputs can be the
    > + * result of comparators.
    > + */
    > +
    > + /* '74a addresses are 0x38..0x3f; '74 uses 0x20..0x27 */
    > + if (strcmp(client->name, "pcf8574a") == 0
    > + || strcmp(client->name, "pcf8574") == 0) {
    > + gpio->chip.ngpio = 8;
    > + gpio->chip.direction_input = pcf857x_input8;
    > + gpio->chip.get = pcf857x_get8;
    > + gpio->chip.direction_output = pcf857x_output8;
    > + gpio->chip.set = pcf857x_set8;
    > +
    > + if (!i2c_check_functionality(client->adapter,
    > + I2C_FUNC_SMBUS_BYTE))
    > + status = -EIO;


    You set status here...

    > +
    > + /* fail if there's no chip present */
    > + status = i2c_smbus_read_byte(client);


    .... but overwrite it immediately.

    > +
    > + /* '75/'75c addresses are 0x20..0x27, just like the '74;
    > + * the '75c doesn't have a current source pulling high.
    > + */
    > + } else if (strcmp(client->name, "pcf8575c") == 0
    > + || strcmp(client->name, "pcf8575") == 0) {
    > + gpio->chip.ngpio = 16;
    > + gpio->chip.direction_input = pcf857x_input16;
    > + gpio->chip.get = pcf857x_get16;
    > + gpio->chip.direction_output = pcf857x_output16;
    > + gpio->chip.set = pcf857x_set16;
    > +
    > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
    > + status = -EIO;
    > +
    > + /* fail if there's no chip present */
    > + status = i2c_read_le16(client);


    Same problem here.

    > +
    > + } else
    > + status = -ENODEV;
    > +
    > + if (status < 0) {
    > + dev_dbg(&client->dev, "probe error %d for '%s'\n",
    > + status, client->name);
    > + kfree(gpio);
    > + return status;


    Might make sense to move this to a common error path, as you do it more
    than once.

    > + }
    > +
    > + gpio->chip.label = client->name;
    > +
    > + gpio->client = client;
    > + i2c_set_clientdata(client, gpio);
    > +
    > + /* NOTE: these chips have strange "pseudo-bidirectional" I/O pins.
    > + * We can't actually know whether a pin is configured (a) as output
    > + * and driving the signal low, or (b) as input and reporting a low
    > + * value ... without knowing the last value written since the chip
    > + * came out of reset (if any). We can't read the latched output.
    > + *
    > + * In short, the only reliable solution for setting up pin direction
    > + * is to do it explicitly. The setup() method can do that.
    > + *
    > + * We use pdata->n_latch to avoid trouble. In the typical case it's
    > + * left initialized to zero; our software copy of the "latch" then
    > + * matches the chip's reset state. But there may be cases where a
    > + * system must drive some pins low, without transient glitching.
    > + * Handle those cases by assigning n_latch to a nonzero value.
    > + */
    > + gpio->out = ~pdata->n_latch;
    > +
    > + status = gpiochip_add(&gpio->chip);
    > + if (status < 0) {
    > + kfree(gpio);
    > + return status;
    > + }
    > +
    > + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
    > + * don't yet even try to use. Among other issues, the relevant
    > + * genirq state isn't available to modular drivers; and most irq
    > + * methods can't be called from sleeping contexts.
    > + */
    > +
    > + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
    > + gpio->chip.base,
    > + gpio->chip.base + gpio->chip.ngpio - 1,
    > + client->name,
    > + client->irq ? " (irq ignored)" : "");
    > +
    > + /* Let platform code set up the GPIOs and their users.
    > + * Now is the first time anyone can use them.
    > + */
    > + if (pdata->setup) {
    > + status = pdata->setup(client,
    > + gpio->chip.base, gpio->chip.ngpio,
    > + pdata->context);
    > + if (status < 0)
    > + dev_dbg(&client->dev, "setup --> %d\n", status);
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static int pcf857x_remove(struct i2c_client *client)
    > +{
    > + struct pcf857x_platform_data *pdata = client->dev.platform_data;
    > + struct pcf857x *gpio = i2c_get_clientdata(client);
    > + int status = 0;
    > +
    > + if (pdata->teardown) {
    > + status = pdata->teardown(client,
    > + gpio->chip.base, gpio->chip.ngpio,
    > + pdata->context);
    > + if (status < 0) {
    > + dev_err(&client->dev, "%s --> %d\n",
    > + "teardown", status);


    Why %s instead of hard-coding "teardown"?

    Why is this an error message while a failing setup() at initialization
    time only deserves a debug message?

    > + return status;
    > + }
    > + }
    > +
    > + status = gpiochip_remove(&gpio->chip);
    > + if (status == 0)
    > + kfree(gpio);
    > + else
    > + dev_err(&client->dev, "%s --> %d\n", "remove", status);
    > + return status;
    > +}
    > +
    > +static struct i2c_driver pcf857x_driver = {
    > + .driver = {
    > + .name = "pcf857x",
    > + .owner = THIS_MODULE,
    > + },
    > + .probe = pcf857x_probe,
    > + .remove = pcf857x_remove,
    > +};
    > +
    > +static int __init pcf857x_init(void)
    > +{
    > + return i2c_add_driver(&pcf857x_driver);
    > +}
    > +/* we want GPIOs to be ready at device_initcall() time */
    > +subsys_initcall(pcf857x_init);
    > +
    > +static void __exit pcf857x_exit(void)
    > +{
    > + i2c_del_driver(&pcf857x_driver);
    > +}
    > +module_exit(pcf857x_exit);
    > +
    > +MODULE_LICENSE("GPL");


    No MODULE_AUTHOR? No entry in MAINTAINERS?

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

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

    Jean Delvare wrote:
    > !!(value & (1 << offset))
    > is more efficiently written
    > (value >> offset) & 1
    >


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

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

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

    Hi Bill,

    On Fri, 30 Nov 2007 07:04:10 -0600, Bill Gatliff wrote:
    > Jean Delvare wrote:
    > > !!(value & (1 << offset))
    > > is more efficiently written
    > > (value >> offset) & 1

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


    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
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Jean Delvare wrote:
    > Hi Bill,
    >
    > On Fri, 30 Nov 2007 07:04:10 -0600, Bill Gatliff wrote:
    >
    >> Jean Delvare wrote:
    >>
    >>> !!(value & (1 << offset))
    >>> is more efficiently written
    >>> (value >> offset) & 1
    >>>

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

    >
    > I don't understand. How could the compiler create the mask at
    > compile-time when "offset" is only known at runtime?
    >


    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

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

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

    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

    >
    > 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 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.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > >
    > > 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 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Friday 30 November 2007, Jean Delvare wrote:
    >
    > So the user-space interface would be part of the generic GPIO
    > infrastructure? I like the idea.


    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
    might reasonably be managed by userspace code.


    > > > > +#include
    > > >
    > > > 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 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?


    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
    reusable kernel interface for such GPIO signals.)


    > Along the same line, I am wondering if it would make sense to put the
    > various GPIO drivers in drivers/gpio.


    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
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [patch/rfc 2/4] pcf857x I2C GPIO expander driver

    On Friday 30 November 2007, David Brownell wrote:
    > Thanks for the review. *I'll snip out typos and similar trivial
    > comments (and fix them!), using responses here for more the
    > substantive feedback.


    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

    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
    ---
    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 GPIO expanders"
    + depends on I2C
    + help
    + Say yes here to provide access to most "quasi-bidirectional" I2C
    + GPIO expanders used for additional digital outputs or inputs.
    + Most of these parts are from NXP, though TI is a second source for
    + some of them. Compatible models include:
    +
    + 8 bits: pcf8574, pcf8574a, pca8574, pca8574a,
    + pca9670, pca9672, pca9674, pca9674a
    +
    + 16 bits: pcf8575, pcf8575c, pca8575,
    + pca9671, pca9673, pca9675
    +
    + Your board setup code will need to declare the expanders in
    + use, and assign numbers to the GPIOs they expose. Those GPIOs
    + can then be used from drivers and other kernel code, just like
    + other GPIOs, but only accessible from task contexts.
    +
    + This driver provides an in-kernel interface to those GPIOs using
    + platform-neutral GPIO calls.
    +
    endmenu
    --- a/drivers/gpio/Makefile 2007-12-05 15:14:03.000000000 -0800
    +++ b/drivers/gpio/Makefile 2007-12-05 15:14:12.000000000 -0800
    @@ -1 +1,3 @@
    # gpio support: dedicated expander chips, etc
    +
    +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
    --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    +++ b/drivers/gpio/pcf857x.c 2007-12-05 15:15:18.000000000 -0800
    @@ -0,0 +1,331 @@
    +/*
    + * pcf857x - driver for pcf857x, pca857x, and pca967x I2C GPIO expanders
    + *
    + * Copyright (C) 2007 David Brownell
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License as published by
    + * the Free Software Foundation; either version 2 of the License, or
    + * (at your option) any later version.
    + *
    + * This program is distributed in the hope that it will be useful,
    + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    + * GNU General Public License for more details.
    + *
    + * You should have received a copy of the GNU General Public License
    + * along with this program; if not, write to the Free Software
    + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
    + */
    +
    +#include
    +#include
    +#include
    +#include
    +
    +#include
    +
    +
    +/*
    + * The pcf857x, pca857x, and pca967x chips only expose one read and one
    + * write register. Writing a "one" bit (to match the reset state) lets
    + * that pin be used as an input; it's not an open-drain model, but acts
    + * a bit like one. This is described as "quasi-bidirectional"; read the
    + * chip documentation for details.
    + *
    + * Some other I2C GPIO expander chips (like the pca953{4,5,6,7,9}, pca9555,
    + * pca9698, mcp23008, and mc23017) have more complex register models with
    + * more conventional input circuitry, often using 0x20..0x27 addresses.
    + */
    +struct pcf857x {
    + struct gpio_chip chip;
    + struct i2c_client *client;
    + unsigned out; /* software latch */
    +};
    +
    +/*-------------------------------------------------------------------------*/
    +
    +/* Talk to 8-bit I/O expander */
    +
    +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    +
    + gpio->out |= (1 << offset);
    + return i2c_smbus_write_byte(gpio->client, gpio->out);
    +}
    +
    +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + s32 value;
    +
    + value = i2c_smbus_read_byte(gpio->client);
    + return (value < 0) ? 0 : (value & (1 << offset));
    +}
    +
    +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + unsigned bit = 1 << offset;
    +
    + if (value)
    + gpio->out |= bit;
    + else
    + gpio->out &= ~bit;
    + return i2c_smbus_write_byte(gpio->client, gpio->out);
    +}
    +
    +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + pcf857x_output8(chip, offset, value);
    +}
    +
    +/*-------------------------------------------------------------------------*/
    +
    +/* Talk to 16-bit I/O expander */
    +
    +static int i2c_write_le16(struct i2c_client *client, u16 word)
    +{
    + u8 buf[2] = { word & 0xff, word >> 8, };
    + int status;
    +
    + status = i2c_master_send(client, buf, 2);
    + return (status < 0) ? status : 0;
    +}
    +
    +static int i2c_read_le16(struct i2c_client *client)
    +{
    + u8 buf[2];
    + int status;
    +
    + status = i2c_master_recv(client, buf, 2);
    + if (status < 0)
    + return status;
    + return (buf[1] << 8) | buf[0];
    +}
    +
    +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    +
    + gpio->out |= (1 << offset);
    + return i2c_write_le16(gpio->client, gpio->out);
    +}
    +
    +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + int value;
    +
    + value = i2c_read_le16(gpio->client);
    + return (value < 0) ? 0 : (value & (1 << offset));
    +}
    +
    +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    + unsigned bit = 1 << offset;
    +
    + if (value)
    + gpio->out |= bit;
    + else
    + gpio->out &= ~bit;
    + return i2c_write_le16(gpio->client, gpio->out);
    +}
    +
    +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
    +{
    + pcf857x_output16(chip, offset, value);
    +}
    +
    +/*-------------------------------------------------------------------------*/
    +
    +static int pcf857x_probe(struct i2c_client *client)
    +{
    + struct pcf857x_platform_data *pdata;
    + struct pcf857x *gpio;
    + int status;
    +
    + pdata = client->dev.platform_data;
    + if (!pdata)
    + return -ENODEV;
    +
    + /* Allocate, initialize, and register this gpio_chip. */
    + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
    + if (!gpio)
    + return -ENOMEM;
    +
    + gpio->chip.base = pdata->gpio_base;
    + gpio->chip.can_sleep = 1;
    +
    + /* NOTE: the OnSemi jlc1562b is also largely compatible with
    + * these parts, notably for output. It has a low-resolution
    + * DAC instead of pin change IRQs; and its inputs can be the
    + * result of comparators.
    + */
    +
    + /* 8574 addresses are 0x20..0x27; 8574a uses 0x38..0x3f;
    + * 9670, 9672, 9764, and 9764a use quite a variety.
    + *
    + * NOTE: we dont distinguish here between *4 and *4a parts.
    + */
    + if (strcmp(client->name, "pcf8574") == 0
    + || strcmp(client->name, "pca8574") == 0
    + || strcmp(client->name, "pca9670") == 0
    + || strcmp(client->name, "pca9672") == 0
    + || strcmp(client->name, "pca9674") == 0
    + ) {
    + gpio->chip.ngpio = 8;
    + gpio->chip.direction_input = pcf857x_input8;
    + gpio->chip.get = pcf857x_get8;
    + gpio->chip.direction_output = pcf857x_output8;
    + gpio->chip.set = pcf857x_set8;
    +
    + if (!i2c_check_functionality(client->adapter,
    + I2C_FUNC_SMBUS_BYTE))
    + status = -EIO;
    +
    + /* fail if there's no chip present */
    + else
    + status = i2c_smbus_read_byte(client);
    +
    + /* '75/'75c addresses are 0x20..0x27, just like the '74;
    + * the '75c doesn't have a current source pulling high.
    + * 9671, 9673, and 9765 use quite a variety of addresses.
    + *
    + * NOTE: we dont distinguish here between 8575/8575a parts.
    + */
    + } else if (strcmp(client->name, "pcf8575") == 0
    + || strcmp(client->name, "pca8575") == 0
    + || strcmp(client->name, "pca9671") == 0
    + || strcmp(client->name, "pca9673") == 0
    + || strcmp(client->name, "pca9675") == 0
    + ) {
    + gpio->chip.ngpio = 16;
    + gpio->chip.direction_input = pcf857x_input16;
    + gpio->chip.get = pcf857x_get16;
    + gpio->chip.direction_output = pcf857x_output16;
    + gpio->chip.set = pcf857x_set16;
    +
    + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
    + status = -EIO;
    +
    + /* fail if there's no chip present */
    + else
    + status = i2c_read_le16(client);
    +
    + } else
    + status = -ENODEV;
    +
    + if (status < 0)
    + goto fail;
    +
    + gpio->chip.label = client->name;
    +
    + gpio->client = client;
    + i2c_set_clientdata(client, gpio);
    +
    + /* NOTE: these chips have strange "quasi-bidirectional" I/O pins.
    + * We can't actually know whether a pin is configured (a) as output
    + * and driving the signal low, or (b) as input and reporting a low
    + * value ... without knowing the last value written since the chip
    + * came out of reset (if any). We can't read the latched output.
    + *
    + * In short, the only reliable solution for setting up pin direction
    + * is to do it explicitly. The setup() method can do that.
    + *
    + * We use pdata->n_latch to avoid trouble. In the typical case it's
    + * left initialized to zero; our software copy of the "latch" then
    + * matches the chip's all-ones reset state. But some systems will
    + * need to drive some pins low, while avoiding transient glitches.
    + * Handle those cases by assigning n_latch to a nonzero value.
    + */
    + gpio->out = ~pdata->n_latch;
    +
    + status = gpiochip_add(&gpio->chip);
    + if (status < 0)
    + goto fail;
    +
    + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
    + * don't yet even try to use. Among other issues, the relevant
    + * genirq state isn't available to modular drivers; and most irq
    + * methods can't be called from sleeping contexts.
    + */
    +
    + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
    + gpio->chip.base,
    + gpio->chip.base + gpio->chip.ngpio - 1,
    + client->name,
    + client->irq ? " (irq ignored)" : "");
    +
    + /* Let platform code set up the GPIOs and their users.
    + * Now is the first time anyone can use them.
    + */
    + if (pdata->setup) {
    + status = pdata->setup(client,
    + gpio->chip.base, gpio->chip.ngpio,
    + pdata->context);
    + if (status < 0)
    + dev_err(&client->dev, "%s --> %d\n",
    + "setup", status);
    + }
    +
    + return 0;
    +
    +fail:
    + dev_dbg(&client->dev, "probe error %d for '%s'\n",
    + status, client->name);
    + kfree(gpio);
    + return status;
    +}
    +
    +static int pcf857x_remove(struct i2c_client *client)
    +{
    + struct pcf857x_platform_data *pdata = client->dev.platform_data;
    + struct pcf857x *gpio = i2c_get_clientdata(client);
    + int status = 0;
    +
    + if (pdata->teardown) {
    + status = pdata->teardown(client,
    + gpio->chip.base, gpio->chip.ngpio,
    + pdata->context);
    + if (status < 0) {
    + dev_err(&client->dev, "%s --> %d\n",
    + "teardown", status);
    + return status;
    + }
    + }
    +
    + status = gpiochip_remove(&gpio->chip);
    + if (status == 0)
    + kfree(gpio);
    + else
    + dev_err(&client->dev, "%s --> %d\n", "remove", status);
    + return status;
    +}
    +
    +static struct i2c_driver pcf857x_driver = {
    + .driver = {
    + .name = "pcf857x",
    + .owner = THIS_MODULE,
    + },
    + .probe = pcf857x_probe,
    + .remove = pcf857x_remove,
    +};
    +
    +static int __init pcf857x_init(void)
    +{
    + return i2c_add_driver(&pcf857x_driver);
    +}
    +/* we want GPIOs to be ready at device_initcall() time */
    +subsys_initcall(pcf857x_init);
    +
    +static void __exit pcf857x_exit(void)
    +{
    + i2c_del_driver(&pcf857x_driver);
    +}
    +module_exit(pcf857x_exit);
    +
    +MODULE_LICENSE("GPL");
    +MODULE_AUTHOR("David Brownell");
    --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    +++ b/include/linux/i2c/pcf857x.h 2007-12-05 15:14:12.000000000 -0800
    @@ -0,0 +1,45 @@
    +#ifndef __LINUX_PCF857X_H
    +#define __LINUX_PCF857X_H
    +
    +/**
    + * 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 register value; if
    + * you leave this initialized to zero the driver will act
    + * like the chip was just reset
    + * @setup: optional callback issued once the GPIOs are valid
    + * @teardown: optional callback issued before the GPIOs are invalidated
    + * @context: optional parameter passed to setup() and teardown()
    + *
    + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
    + * the i2c_board_info used with the pcf875x driver must provide the
    + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
    + * platform_data (pointer to one of these structures) with at least
    + * the gpio_base value initialized.
    + *
    + * The @setup callback may be used with the kind of board-specific glue
    + * which hands the (now-valid) GPIOs to other drivers, or which puts
    + * devices in their initial states using these GPIOs.
    + *
    + * These GPIO chips are only "quasi-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 be
    + * inferred by reading the chip's value and knowing the last value written
    + * to it. If you leave n_latch initialized to zero, that last written
    + * value is presumed to be all ones (as if the chip were just reset).
    + */
    +struct pcf857x_platform_data {
    + unsigned gpio_base;
    + unsigned n_latch;
    +
    + int (*setup)(struct i2c_client *client,
    + int gpio, unsigned ngpio,
    + void *context);
    + int (*teardown)(struct i2c_client *client,
    + int gpio, unsigned ngpio,
    + void *context);
    + void *context;
    +};
    +
    +#endif /* __LINUX_PCF857X_H */
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [patch/rfc 2/4] pcf857x I2C GPIO expander driver

    Hi David,

    On Wed, 5 Dec 2007 19:03:12 -0800, David Brownell wrote:
    > On Friday 30 November 2007, David Brownell wrote:
    > > Thanks for the review. *I'll snip out typos and similar trivial
    > > comments (and fix them!), using responses here for more the
    > > substantive feedback.

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


    Missing space after closing parenthesis. Also, I don't quite see what
    is supposed to make compatibility with the legacy drivers easier, nor
    how, not why it matters in the first place.

    >
    > 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
    > ---
    > 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 GPIO expanders"
    > + depends on I2C
    > + help
    > + Say yes here to provide access to most "quasi-bidirectional" I2C
    > + GPIO expanders used for additional digital outputs or inputs.
    > + Most of these parts are from NXP, though TI is a second source for
    > + some of them. Compatible models include:
    > +
    > + 8 bits: pcf8574, pcf8574a, pca8574, pca8574a,
    > + pca9670, pca9672, pca9674, pca9674a
    > +
    > + 16 bits: pcf8575, pcf8575c, pca8575,
    > + pca9671, pca9673, pca9675
    > +
    > + Your board setup code will need to declare the expanders in
    > + use, and assign numbers to the GPIOs they expose. Those GPIOs
    > + can then be used from drivers and other kernel code, just like
    > + other GPIOs, but only accessible from task contexts.
    > +
    > + This driver provides an in-kernel interface to those GPIOs using
    > + platform-neutral GPIO calls.
    > +
    > endmenu
    > --- a/drivers/gpio/Makefile 2007-12-05 15:14:03.000000000 -0800
    > +++ b/drivers/gpio/Makefile 2007-12-05 15:14:12.000000000 -0800
    > @@ -1 +1,3 @@
    > # gpio support: dedicated expander chips, etc
    > +
    > +obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
    > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    > +++ b/drivers/gpio/pcf857x.c 2007-12-05 15:15:18.000000000 -0800
    > @@ -0,0 +1,331 @@
    > +/*
    > + * pcf857x - driver for pcf857x, pca857x, and pca967x I2C GPIO expanders
    > + *
    > + * Copyright (C) 2007 David Brownell
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License as published by
    > + * the Free Software Foundation; either version 2 of the License, or
    > + * (at your option) any later version.
    > + *
    > + * This program is distributed in the hope that it will be useful,
    > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + *
    > + * You should have received a copy of the GNU General Public License
    > + * along with this program; if not, write to the Free Software
    > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
    > + */
    > +
    > +#include
    > +#include
    > +#include
    > +#include
    > +
    > +#include
    > +
    > +
    > +/*
    > + * The pcf857x, pca857x, and pca967x chips only expose one read and one
    > + * write register. Writing a "one" bit (to match the reset state) lets
    > + * that pin be used as an input; it's not an open-drain model, but acts
    > + * a bit like one. This is described as "quasi-bidirectional"; read the
    > + * chip documentation for details.
    > + *
    > + * Some other I2C GPIO expander chips (like the pca953{4,5,6,7,9}, pca9555,
    > + * pca9698, mcp23008, and mc23017) have more complex register models with


    mc_p_23017?

    > + * more conventional input circuitry, often using 0x20..0x27 addresses.
    > + */
    > +struct pcf857x {
    > + struct gpio_chip chip;
    > + struct i2c_client *client;
    > + unsigned out; /* software latch */
    > +};
    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +/* Talk to 8-bit I/O expander */
    > +
    > +static int pcf857x_input8(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > +
    > + gpio->out |= (1 << offset);
    > + return i2c_smbus_write_byte(gpio->client, gpio->out);
    > +}
    > +
    > +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + s32 value;
    > +
    > + value = i2c_smbus_read_byte(gpio->client);
    > + return (value < 0) ? 0 : (value & (1 << offset));


    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
    to return in these callback functions.)

    > +}
    > +
    > +static int pcf857x_output8(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + unsigned bit = 1 << offset;
    > +
    > + if (value)
    > + gpio->out |= bit;
    > + else
    > + gpio->out &= ~bit;
    > + return i2c_smbus_write_byte(gpio->client, gpio->out);
    > +}
    > +
    > +static void pcf857x_set8(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + pcf857x_output8(chip, offset, value);
    > +}
    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +/* Talk to 16-bit I/O expander */
    > +
    > +static int i2c_write_le16(struct i2c_client *client, u16 word)
    > +{
    > + u8 buf[2] = { word & 0xff, word >> 8, };


    Stray comma.

    > + int status;
    > +
    > + status = i2c_master_send(client, buf, 2);
    > + return (status < 0) ? status : 0;
    > +}
    > +
    > +static int i2c_read_le16(struct i2c_client *client)
    > +{
    > + u8 buf[2];
    > + int status;
    > +
    > + status = i2c_master_recv(client, buf, 2);
    > + if (status < 0)
    > + return status;
    > + return (buf[1] << 8) | buf[0];
    > +}
    > +
    > +static int pcf857x_input16(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > +
    > + gpio->out |= (1 << offset);
    > + return i2c_write_le16(gpio->client, gpio->out);
    > +}
    > +
    > +static int pcf857x_get16(struct gpio_chip *chip, unsigned offset)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + int value;
    > +
    > + value = i2c_read_le16(gpio->client);
    > + return (value < 0) ? 0 : (value & (1 << offset));
    > +}
    > +
    > +static int pcf857x_output16(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > + unsigned bit = 1 << offset;
    > +
    > + if (value)
    > + gpio->out |= bit;
    > + else
    > + gpio->out &= ~bit;
    > + return i2c_write_le16(gpio->client, gpio->out);
    > +}
    > +
    > +static void pcf857x_set16(struct gpio_chip *chip, unsigned offset, int value)
    > +{
    > + pcf857x_output16(chip, offset, value);
    > +}
    > +
    > +/*-------------------------------------------------------------------------*/
    > +
    > +static int pcf857x_probe(struct i2c_client *client)
    > +{
    > + struct pcf857x_platform_data *pdata;
    > + struct pcf857x *gpio;
    > + int status;
    > +
    > + pdata = client->dev.platform_data;
    > + if (!pdata)
    > + return -ENODEV;
    > +
    > + /* Allocate, initialize, and register this gpio_chip. */
    > + gpio = kzalloc(sizeof *gpio, GFP_KERNEL);
    > + if (!gpio)
    > + return -ENOMEM;
    > +
    > + gpio->chip.base = pdata->gpio_base;
    > + gpio->chip.can_sleep = 1;
    > +
    > + /* NOTE: the OnSemi jlc1562b is also largely compatible with
    > + * these parts, notably for output. It has a low-resolution
    > + * DAC instead of pin change IRQs; and its inputs can be the
    > + * result of comparators.
    > + */
    > +
    > + /* 8574 addresses are 0x20..0x27; 8574a uses 0x38..0x3f;
    > + * 9670, 9672, 9764, and 9764a use quite a variety.
    > + *
    > + * NOTE: we dont distinguish here between *4 and *4a parts.


    Typo: don't.

    > + */
    > + if (strcmp(client->name, "pcf8574") == 0
    > + || strcmp(client->name, "pca8574") == 0
    > + || strcmp(client->name, "pca9670") == 0
    > + || strcmp(client->name, "pca9672") == 0
    > + || strcmp(client->name, "pca9674") == 0
    > + ) {
    > + gpio->chip.ngpio = 8;
    > + gpio->chip.direction_input = pcf857x_input8;
    > + gpio->chip.get = pcf857x_get8;
    > + gpio->chip.direction_output = pcf857x_output8;
    > + gpio->chip.set = pcf857x_set8;
    > +
    > + if (!i2c_check_functionality(client->adapter,
    > + I2C_FUNC_SMBUS_BYTE))
    > + status = -EIO;
    > +
    > + /* fail if there's no chip present */
    > + else
    > + status = i2c_smbus_read_byte(client);
    > +
    > + /* '75/'75c addresses are 0x20..0x27, just like the '74;
    > + * the '75c doesn't have a current source pulling high.
    > + * 9671, 9673, and 9765 use quite a variety of addresses.
    > + *
    > + * NOTE: we dont distinguish here between 8575/8575a parts.
    > + */
    > + } else if (strcmp(client->name, "pcf8575") == 0
    > + || strcmp(client->name, "pca8575") == 0
    > + || strcmp(client->name, "pca9671") == 0
    > + || strcmp(client->name, "pca9673") == 0
    > + || strcmp(client->name, "pca9675") == 0
    > + ) {
    > + gpio->chip.ngpio = 16;
    > + gpio->chip.direction_input = pcf857x_input16;
    > + gpio->chip.get = pcf857x_get16;
    > + gpio->chip.direction_output = pcf857x_output16;
    > + gpio->chip.set = pcf857x_set16;
    > +
    > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
    > + status = -EIO;
    > +
    > + /* fail if there's no chip present */
    > + else
    > + status = i2c_read_le16(client);
    > +
    > + } else
    > + status = -ENODEV;
    > +
    > + if (status < 0)
    > + goto fail;
    > +
    > + gpio->chip.label = client->name;
    > +
    > + gpio->client = client;
    > + i2c_set_clientdata(client, gpio);
    > +
    > + /* NOTE: these chips have strange "quasi-bidirectional" I/O pins.
    > + * We can't actually know whether a pin is configured (a) as output
    > + * and driving the signal low, or (b) as input and reporting a low
    > + * value ... without knowing the last value written since the chip
    > + * came out of reset (if any). We can't read the latched output.
    > + *
    > + * In short, the only reliable solution for setting up pin direction
    > + * is to do it explicitly. The setup() method can do that.
    > + *
    > + * We use pdata->n_latch to avoid trouble. In the typical case it's
    > + * left initialized to zero; our software copy of the "latch" then
    > + * matches the chip's all-ones reset state. But some systems will
    > + * need to drive some pins low, while avoiding transient glitches.
    > + * Handle those cases by assigning n_latch to a nonzero value.
    > + */
    > + gpio->out = ~pdata->n_latch;
    > +
    > + status = gpiochip_add(&gpio->chip);
    > + if (status < 0)
    > + goto fail;
    > +
    > + /* NOTE: these chips can issue "some pin-changed" IRQs, which we
    > + * don't yet even try to use. Among other issues, the relevant
    > + * genirq state isn't available to modular drivers; and most irq
    > + * methods can't be called from sleeping contexts.
    > + */
    > +
    > + dev_info(&client->dev, "gpios %d..%d on a %s%s\n",
    > + gpio->chip.base,
    > + gpio->chip.base + gpio->chip.ngpio - 1,
    > + client->name,
    > + client->irq ? " (irq ignored)" : "");
    > +
    > + /* Let platform code set up the GPIOs and their users.
    > + * Now is the first time anyone can use them.
    > + */
    > + if (pdata->setup) {
    > + status = pdata->setup(client,
    > + gpio->chip.base, gpio->chip.ngpio,
    > + pdata->context);
    > + if (status < 0)
    > + dev_err(&client->dev, "%s --> %d\n",
    > + "setup", status);


    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
    want to swap this block of code with the dev_info above.)

    > + }
    > +
    > + return 0;
    > +
    > +fail:
    > + dev_dbg(&client->dev, "probe error %d for '%s'\n",
    > + status, client->name);
    > + kfree(gpio);
    > + return status;
    > +}
    > +
    > +static int pcf857x_remove(struct i2c_client *client)
    > +{
    > + struct pcf857x_platform_data *pdata = client->dev.platform_data;
    > + struct pcf857x *gpio = i2c_get_clientdata(client);
    > + int status = 0;
    > +
    > + if (pdata->teardown) {
    > + status = pdata->teardown(client,
    > + gpio->chip.base, gpio->chip.ngpio,
    > + pdata->context);
    > + if (status < 0) {
    > + dev_err(&client->dev, "%s --> %d\n",
    > + "teardown", status);
    > + return status;
    > + }
    > + }
    > +
    > + status = gpiochip_remove(&gpio->chip);
    > + if (status == 0)
    > + kfree(gpio);
    > + else
    > + dev_err(&client->dev, "%s --> %d\n", "remove", status);
    > + return status;
    > +}
    > +
    > +static struct i2c_driver pcf857x_driver = {
    > + .driver = {
    > + .name = "pcf857x",
    > + .owner = THIS_MODULE,
    > + },
    > + .probe = pcf857x_probe,
    > + .remove = pcf857x_remove,
    > +};
    > +
    > +static int __init pcf857x_init(void)
    > +{
    > + return i2c_add_driver(&pcf857x_driver);
    > +}
    > +/* we want GPIOs to be ready at device_initcall() time */
    > +subsys_initcall(pcf857x_init);
    > +
    > +static void __exit pcf857x_exit(void)
    > +{
    > + i2c_del_driver(&pcf857x_driver);
    > +}
    > +module_exit(pcf857x_exit);
    > +
    > +MODULE_LICENSE("GPL");
    > +MODULE_AUTHOR("David Brownell");
    > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
    > +++ b/include/linux/i2c/pcf857x.h 2007-12-05 15:14:12.000000000 -0800
    > @@ -0,0 +1,45 @@
    > +#ifndef __LINUX_PCF857X_H
    > +#define __LINUX_PCF857X_H
    > +
    > +/**
    > + * 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 register value; if
    > + * you leave this initialized to zero the driver will act
    > + * like the chip was just reset
    > + * @setup: optional callback issued once the GPIOs are valid
    > + * @teardown: optional callback issued before the GPIOs are invalidated
    > + * @context: optional parameter passed to setup() and teardown()
    > + *
    > + * In addition to the I2C_BOARD_INFO() state appropriate to each chip,
    > + * the i2c_board_info used with the pcf875x driver must provide the
    > + * chip "type" ("pcf8574", "pcf8574a", "pcf8575", "pcf8575c") and its
    > + * platform_data (pointer to one of these structures) with at least
    > + * the gpio_base value initialized.
    > + *
    > + * The @setup callback may be used with the kind of board-specific glue
    > + * which hands the (now-valid) GPIOs to other drivers, or which puts
    > + * devices in their initial states using these GPIOs.
    > + *
    > + * These GPIO chips are only "quasi-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 be
    > + * inferred by reading the chip's value and knowing the last value written
    > + * to it. If you leave n_latch initialized to zero, that last written
    > + * value is presumed to be all ones (as if the chip were just reset).
    > + */
    > +struct pcf857x_platform_data {
    > + unsigned gpio_base;
    > + unsigned n_latch;
    > +
    > + int (*setup)(struct i2c_client *client,
    > + int gpio, unsigned ngpio,
    > + void *context);
    > + int (*teardown)(struct i2c_client *client,
    > + int gpio, unsigned ngpio,
    > + void *context);
    > + void *context;
    > +};
    > +
    > +#endif /* __LINUX_PCF857X_H */


    The rest looks fine to me.

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

  11. Re: [patch/rfc 2/4] pcf857x I2C GPIO expander driver

    On Thursday 06 December 2007, Jean Delvare wrote:

    > Also, I don't quite see what
    > is supposed to make compatibility with the legacy drivers easier, nor
    > how, not why it matters in the first place.


    There's a clear either/or disjunction. No fuzzy/confusing middle ground.


    > > +static int pcf857x_get8(struct gpio_chip *chip, unsigned offset)
    > > +{
    > > + struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
    > > + s32 value;
    > > +
    > > + value = i2c_smbus_read_byte(gpio->client);
    > > + return (value < 0) ? 0 : (value & (1 << offset));

    >
    > 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
    > to return in these callback functions.)


    Already documented -- as zero/nonzero, the original boolean model for C.
    Anything else would be at least tristate, not boolean.


    > > + /* Let platform code set up the GPIOs and their users.
    > > + * Now is the first time anyone can use them.
    > > + */
    > > + if (pdata->setup) {
    > > + status = pdata->setup(client,
    > > + gpio->chip.base, gpio->chip.ngpio,
    > > + pdata->context);
    > > + if (status < 0)
    > > + dev_err(&client->dev, "%s --> %d\n",
    > > + "setup", status);

    >
    > 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
    > want to swap this block of code with the dev_info above.)


    Good point.


    > The rest looks fine to me.


    Thanks for the comments. I'll send this in with the next batch
    of gpiolib patches.

    - Dave

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

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

    On Fri, 30 Nov 2007, David Brownell wrote:
    > On Friday 30 November 2007, Jean Delvare wrote:
    >>
    >> So the user-space interface would be part of the generic GPIO
    >> infrastructure? I like the idea.

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


    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/:) 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
    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 Piepho
    ---
    drivers/gpio/gpiolib.c | 188 ++++++++++++++++++++++++++++++++++++++++++++
    include/asm-generic/gpio.h | 2 +
    2 files changed, 190 insertions(+), 0 deletions(-)

    diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
    index d8db2f8..db0677d 100644
    --- a/drivers/gpio/gpiolib.c
    +++ b/drivers/gpio/gpiolib.c
    @@ -18,6 +18,7 @@
    * only an instruction or two per bit.
    */

    +#define GPIO_CLASS 1

    /* When debugging, extend minimal trust to callers and platform code.
    * Also emit diagnostic messages that may help initial bringup, when
    @@ -47,6 +48,9 @@ struct gpio_desc {
    #ifdef CONFIG_DEBUG_FS
    const char *label;
    #endif
    +#if GPIO_CLASS
    + struct device *dev;
    +#endif
    };
    static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];

    @@ -57,6 +61,174 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
    #endif
    }

    +#if GPIO_CLASS
    +#include
    +#include
    +#include
    +#include
    +
    +static struct class *gpio_class;
    +
    +static ssize_t gpio_direction_show(struct device *dev,
    + struct device_attribute *attr, char *buf)
    +{
    + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    +
    + strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
    +
    + return 5;
    +}
    +
    +static ssize_t gpio_direction_store(struct device *dev,
    + struct device_attribute *attr, const char *buf, size_t size)
    +{
    + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    + int d, n = gdesc - gpio_desc;
    +
    + if (size >= 3 && !strncmp(buf, "out", 3)) {
    + d = 1;
    + } else if (size >= 2 && !strncmp(buf, "in", 2)) {
    + d = 0;
    + } else {
    + d = simple_strtoul(buf, NULL, 0);
    + }
    +
    + if (d)
    + gpio_direction_output(n, 0);
    + else
    + gpio_direction_input(n);
    +
    + return size;
    +}
    +
    +static DEVICE_ATTR(direction, 0644, gpio_direction_show, gpio_direction_store);
    +
    +static ssize_t gpio_value_show(struct device *dev,
    + struct device_attribute *attr, char *buf)
    +{
    + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    + int n = gdesc - gpio_desc;
    +
    + if (test_bit(FLAG_IS_OUT, &gdesc->flags)) {
    + return -EINVAL;
    + /* strcpy(buf, "-1\n"); return 4; */ /* Or this? */
    + }
    + return sprintf(buf, "%d\n", gpio_get_value(n)) + 1;
    +}
    +
    +static ssize_t gpio_value_store(struct device *dev,
    + struct device_attribute *attr, const char *buf, size_t size)
    +{
    + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    + int v, n = gdesc - gpio_desc;
    +
    + if (!test_bit(FLAG_IS_OUT, &gdesc->flags))
    + return -EINVAL;
    +
    + v = simple_strtoul(buf, NULL, 0);
    + gpio_set_value(n, v);
    +
    + return size;
    +}
    +
    +static DEVICE_ATTR(value, 0644, gpio_value_show, gpio_value_store);
    +
    +#ifdef CONFIG_DEBUG_FS
    +static ssize_t gpio_label_show(struct device *dev,
    + struct device_attribute *attr, char *buf)
    +{
    + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    +
    + if (!gdesc->label) {
    + strcpy(buf, "free\n");
    + return 6;
    + } else
    + return sprintf(buf, "%s\n", gdesc->label) + 1;
    +}
    +
    +static DEVICE_ATTR(label, 0444, gpio_label_show, NULL);
    +#endif
    +
    +static int gpiochip_classdev_register(const struct gpio_chip *chip)
    +{
    + int ret, i;
    + struct gpio_desc *gdesc;
    +
    + BUG_ON(!chip->dev);
    +
    + for (i = chip->base; i < chip->base + chip->ngpio; i++) {
    + gdesc = gpio_desc + i;
    + gdesc->dev = device_create(gpio_class, chip->dev, 0, "%s:%d",
    + chip->label, i);
    + if (IS_ERR(gdesc->dev)) {
    + ret = PTR_ERR(gdesc->dev);
    + i--;
    + goto fail;
    + }
    +
    + dev_set_drvdata(gdesc->dev, gdesc);
    +
    + ret = device_create_file(gdesc->dev, &dev_attr_direction);
    + if (ret)
    + goto fail_dev;
    + ret = device_create_file(gdesc->dev, &dev_attr_value);
    + if (ret)
    + goto fail_dir;
    +#ifdef CONFIG_DEBUG_FS
    + ret = device_create_file(gdesc->dev, &dev_attr_label);
    + if (ret)
    + goto fail_value;
    +#endif
    + }
    + return 0;
    +
    +fail:
    + for (; i >= chip->base; i--) {
    + gdesc = gpio_desc + i;
    +
    +#ifdef CONFIG_DEBUG_FS
    + device_remove_file(gdesc->dev, &dev_attr_label);
    +
    +fail_value:
    +#endif
    + device_remove_file(gdesc->dev, &dev_attr_value);
    +
    +fail_dir:
    + device_remove_file(gdesc->dev, &dev_attr_direction);
    +
    +fail_dev:
    + device_unregister(gdesc->dev);
    + gdesc->dev = NULL;
    + }
    + return ret;
    +}
    +
    +static int __init gpio_class_init(void)
    +{
    + gpio_class = class_create(THIS_MODULE, "gpio");
    + if (IS_ERR(gpio_class))
    + return PTR_ERR(gpio_class);
    + return 0;
    +}
    +
    +static void __exit gpio_class_exit(void)
    +{
    + /* FIXME: Code to remove all the sysfs devices and files created
    + * should go here */
    + class_destroy(gpio_class);
    +}
    +subsys_initcall(gpio_class_init);
    +module_exit(gpio_class_exit);
    +
    +#else /* no class */
    +
    +/* I coulda been a contender, I coulda had class... */
    +static inline int gpiochip_classdev_register(const struct gpio_chip *chip)
    +{ return 0; }
    +
    +#endif
    +
    +
    /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
    * when setting direction, and otherwise illegal. Until board setup code
    * and drivers use explicit requests everywhere (which won't happen when
    @@ -118,6 +290,22 @@ int gpiochip_add(struct gpio_chip *chip)
    }

    spin_unlock_irqrestore(&gpio_lock, flags);
    +
    + if (status)
    + goto fail;
    +
    + if (chip->dev) {
    + /* can sleep, so can't call with the spinlock held */
    + status = gpiochip_classdev_register(chip);
    + if (status) {
    + /* De-allocate GPIOs */
    + spin_lock_irqsave(&gpio_lock, flags);
    + for (id = chip->base; id < chip->base + chip->ngpio; id++)
    + gpio_desc[id].chip = NULL;
    + spin_unlock_irqrestore(&gpio_lock, flags);
    + }
    + }
    +
    fail:
    /* failures here can mean systems won't boot... */
    if (status)
    diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
    index f29a502..b2a5262 100644
    --- a/include/asm-generic/gpio.h
    +++ b/include/asm-generic/gpio.h
    @@ -29,6 +29,7 @@ struct seq_file;
    * @dbg_show: optional routine to show contents in debugfs; default code
    * will be used when this is omitted, but custom code can show extra
    * state (such as pullup/pulldown configuration).
    + * @dev: optional device for the GPIO chip. Used to create sysfs files.
    * @base: identifies the first GPIO number handled by this chip; or, if
    * negative during registration, requests dynamic ID allocation.
    * @ngpio: the number of GPIOs handled by this controller; the last GPIO
    @@ -59,6 +60,7 @@ struct gpio_chip {
    unsigned offset, int value);
    void (*dbg_show)(struct seq_file *s,
    struct gpio_chip *chip);
    + struct device *dev;
    int base;
    u16 ngpio;
    unsigned can_sleep:1;
    --
    1.5.4.1

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

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


    On Thu, 2008-04-03 at 19:06 -0700, Trent Piepho wrote:
    > On Fri, 30 Nov 2007, David Brownell wrote:
    > > On Friday 30 November 2007, Jean Delvare wrote:
    > >>
    > >> So the user-space interface would be part of the generic GPIO
    > >> infrastructure? I like the idea.

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

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


    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 4 Apr 2008, Ben Nizette wrote:
    > On Thu, 2008-04-03 at 19:06 -0700, Trent Piepho wrote:
    >> On Fri, 30 Nov 2007, David Brownell wrote:
    >>> On Friday 30 November 2007, Jean Delvare wrote:
    >>>>
    >>>> So the user-space interface would be part of the generic GPIO
    >>>> infrastructure? I like the idea.
    >>>
    >>> 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.

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

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


    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    On Thu, 2008-04-03 at 20:33 -0700, Trent Piepho wrote:

    > Always too slow posting my patches. I wrote this two months ago when there
    > wasn't anything else.


    I've got something similar myself; mine's more complex, handles and
    reports IRQs but isn't near releasable. Happens :-)

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

    simple_gpio allows
    echo "O1" > /dev/gpioN
    to do what you've got above, no extra points there ;-)

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


    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.

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

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

    Hi Trent,

    On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:
    > On Fri, 30 Nov 2007, David Brownell wrote:
    > > On Friday 30 November 2007, Jean Delvare wrote:
    > >>
    > >> So the user-space interface would be part of the generic GPIO
    > >> infrastructure? I like the idea.

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

    >
    > 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/:) 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
    > 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.


    Purely technical review (I can't say whether this interface is better
    than what has been proposed elsewhere or not):

    >
    > Signed-off-by: Trent Piepho
    > ---
    > drivers/gpio/gpiolib.c | 188 ++++++++++++++++++++++++++++++++++++++++++++
    > include/asm-generic/gpio.h | 2 +
    > 2 files changed, 190 insertions(+), 0 deletions(-)
    >
    > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
    > index d8db2f8..db0677d 100644
    > --- a/drivers/gpio/gpiolib.c
    > +++ b/drivers/gpio/gpiolib.c
    > @@ -18,6 +18,7 @@
    > * only an instruction or two per bit.
    > */
    >
    > +#define GPIO_CLASS 1
    >
    > /* When debugging, extend minimal trust to callers and platform code.
    > * Also emit diagnostic messages that may help initial bringup, when
    > @@ -47,6 +48,9 @@ struct gpio_desc {
    > #ifdef CONFIG_DEBUG_FS
    > const char *label;
    > #endif
    > +#if GPIO_CLASS
    > + struct device *dev;
    > +#endif
    > };
    > static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
    >
    > @@ -57,6 +61,174 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
    > #endif
    > }
    >
    > +#if GPIO_CLASS
    > +#include
    > +#include
    > +#include
    > +#include
    > +
    > +static struct class *gpio_class;
    > +
    > +static ssize_t gpio_direction_show(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > +
    > + strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
    > +
    > + return 5;


    Confusing construct... I suggest using sprintf instead, which will
    automatically return the correct number of bytes for you.

    > +}
    > +
    > +static ssize_t gpio_direction_store(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t size)
    > +{
    > + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > + int d, n = gdesc - gpio_desc;
    > +
    > + if (size >= 3 && !strncmp(buf, "out", 3)) {
    > + d = 1;
    > + } else if (size >= 2 && !strncmp(buf, "in", 2)) {
    > + d = 0;


    As far as I know, the string you receive from sysfs is guaranteed to be
    NUL-terminated, so the size checks are not needed.

    > + } else {
    > + d = simple_strtoul(buf, NULL, 0);


    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
    "in" and "out" as valid values and return -EINVAL otherwise.

    > + }
    > +
    > + if (d)
    > + gpio_direction_output(n, 0);
    > + else
    > + gpio_direction_input(n);
    > +
    > + return size;
    > +}
    > +
    > +static DEVICE_ATTR(direction, 0644, gpio_direction_show, gpio_direction_store);
    > +
    > +static ssize_t gpio_value_show(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > + int n = gdesc - gpio_desc;
    > +
    > + if (test_bit(FLAG_IS_OUT, &gdesc->flags)) {
    > + return -EINVAL;
    > + /* strcpy(buf, "-1\n"); return 4; */ /* Or this? */


    Why not just return gpio_get_value(n) in all cases? User might want to
    know which value is currently being output.

    > + }
    > + return sprintf(buf, "%d\n", gpio_get_value(n)) + 1;


    Why "+ 1"? As far as I know you are not supposed to return a trailing
    NUL to user-space.

    > +}
    > +
    > +static ssize_t gpio_value_store(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t size)
    > +{
    > + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > + int v, n = gdesc - gpio_desc;
    > +
    > + if (!test_bit(FLAG_IS_OUT, &gdesc->flags))
    > + return -EINVAL;
    > +
    > + v = simple_strtoul(buf, NULL, 0);


    We have strict_strtoul() now.

    > + gpio_set_value(n, v);
    > +
    > + return size;
    > +}
    > +
    > +static DEVICE_ATTR(value, 0644, gpio_value_show, gpio_value_store);
    > +
    > +#ifdef CONFIG_DEBUG_FS
    > +static ssize_t gpio_label_show(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > +
    > + if (!gdesc->label) {
    > + strcpy(buf, "free\n");
    > + return 6;


    Again sprintf would seem safer than a hard-coded length.

    > + } else
    > + return sprintf(buf, "%s\n", gdesc->label) + 1;
    > +}
    > +
    > +static DEVICE_ATTR(label, 0444, gpio_label_show, NULL);
    > +#endif
    > +
    > +static int gpiochip_classdev_register(const struct gpio_chip *chip)
    > +{
    > + int ret, i;
    > + struct gpio_desc *gdesc;
    > +
    > + BUG_ON(!chip->dev);
    > +
    > + for (i = chip->base; i < chip->base + chip->ngpio; i++) {
    > + gdesc = gpio_desc + i;
    > + gdesc->dev = device_create(gpio_class, chip->dev, 0, "%s:%d",
    > + chip->label, i);
    > + if (IS_ERR(gdesc->dev)) {
    > + ret = PTR_ERR(gdesc->dev);
    > + i--;
    > + goto fail;
    > + }
    > +
    > + dev_set_drvdata(gdesc->dev, gdesc);
    > +
    > + ret = device_create_file(gdesc->dev, &dev_attr_direction);
    > + if (ret)
    > + goto fail_dev;
    > + ret = device_create_file(gdesc->dev, &dev_attr_value);
    > + if (ret)
    > + goto fail_dir;
    > +#ifdef CONFIG_DEBUG_FS
    > + ret = device_create_file(gdesc->dev, &dev_attr_label);
    > + if (ret)
    > + goto fail_value;
    > +#endif
    > + }
    > + return 0;
    > +
    > +fail:
    > + for (; i >= chip->base; i--) {
    > + gdesc = gpio_desc + i;
    > +
    > +#ifdef CONFIG_DEBUG_FS
    > + device_remove_file(gdesc->dev, &dev_attr_label);
    > +
    > +fail_value:
    > +#endif
    > + device_remove_file(gdesc->dev, &dev_attr_value);
    > +
    > +fail_dir:
    > + device_remove_file(gdesc->dev, &dev_attr_direction);
    > +
    > +fail_dev:
    > + device_unregister(gdesc->dev);
    > + gdesc->dev = NULL;
    > + }
    > + return ret;
    > +}
    > +
    > +static int __init gpio_class_init(void)
    > +{
    > + gpio_class = class_create(THIS_MODULE, "gpio");
    > + if (IS_ERR(gpio_class))
    > + return PTR_ERR(gpio_class);
    > + return 0;
    > +}
    > +
    > +static void __exit gpio_class_exit(void)
    > +{
    > + /* FIXME: Code to remove all the sysfs devices and files created
    > + * should go here */


    Oh yes it really should

    > + class_destroy(gpio_class);
    > +}
    > +subsys_initcall(gpio_class_init);
    > +module_exit(gpio_class_exit);
    > +
    > +#else /* no class */
    > +
    > +/* I coulda been a contender, I coulda had class... */
    > +static inline int gpiochip_classdev_register(const struct gpio_chip *chip)
    > +{ return 0; }
    > +
    > +#endif
    > +
    > +
    > /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
    > * when setting direction, and otherwise illegal. Until board setup code
    > * and drivers use explicit requests everywhere (which won't happen when
    > @@ -118,6 +290,22 @@ int gpiochip_add(struct gpio_chip *chip)
    > }
    >
    > spin_unlock_irqrestore(&gpio_lock, flags);
    > +
    > + if (status)
    > + goto fail;
    > +
    > + if (chip->dev) {
    > + /* can sleep, so can't call with the spinlock held */


    You don't actually hold the spinlock at this point.

    > + status = gpiochip_classdev_register(chip);
    > + if (status) {
    > + /* De-allocate GPIOs */
    > + spin_lock_irqsave(&gpio_lock, flags);
    > + for (id = chip->base; id < chip->base + chip->ngpio; id++)
    > + gpio_desc[id].chip = NULL;
    > + spin_unlock_irqrestore(&gpio_lock, flags);
    > + }
    > + }
    > +
    > fail:
    > /* failures here can mean systems won't boot... */
    > if (status)
    > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
    > index f29a502..b2a5262 100644
    > --- a/include/asm-generic/gpio.h
    > +++ b/include/asm-generic/gpio.h
    > @@ -29,6 +29,7 @@ struct seq_file;
    > * @dbg_show: optional routine to show contents in debugfs; default code
    > * will be used when this is omitted, but custom code can show extra
    > * state (such as pullup/pulldown configuration).
    > + * @dev: optional device for the GPIO chip. Used to create sysfs files.


    Broken indentation.

    > * @base: identifies the first GPIO number handled by this chip; or, if
    > * negative during registration, requests dynamic ID allocation.
    > * @ngpio: the number of GPIOs handled by this controller; the last GPIO
    > @@ -59,6 +60,7 @@ struct gpio_chip {
    > unsigned offset, int value);
    > void (*dbg_show)(struct seq_file *s,
    > struct gpio_chip *chip);
    > + struct device *dev;
    > int base;
    > u16 ngpio;
    > unsigned can_sleep:1;



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

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

    On Fri, 4 Apr 2008, Jean Delvare wrote:
    > On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:
    >> From c65d0fb239b79de7f595e47edb2fb641217e7309 Mon Sep 17 00:00:00 2001
    >> From: Trent Piepho
    >> 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.
    >>

    >
    > Purely technical review (I can't say whether this interface is better
    > than what has been proposed elsewhere or not):
    >
    >> + strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
    >> +
    >> + return 5;

    >
    > Confusing construct... I suggest using sprintf instead, which will
    > automatically return the correct number of bytes for you.


    But it's less efficient! Will nobody think of the wasted cycles?

    >> + } else {
    >> + d = simple_strtoul(buf, NULL, 0);

    >
    > 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
    > "in" and "out" as valid values and return -EINVAL otherwise.


    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
    in/out for output, so that's probably the most obvious single interface.

    >> + const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    >> + int n = gdesc - gpio_desc;
    >> +
    >> + if (test_bit(FLAG_IS_OUT, &gdesc->flags)) {
    >> + return -EINVAL;
    >> + /* strcpy(buf, "-1\n"); return 4; */ /* Or this? */

    >
    > Why not just return gpio_get_value(n) in all cases? User might want to
    > know which value is currently being output.


    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
    isn't desirable.

    >> +static void __exit gpio_class_exit(void)
    >> +{
    >> + /* FIXME: Code to remove all the sysfs devices and files created
    >> + * should go here */

    >
    > Oh yes it really should


    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
    useful for the kernel?

    >> @@ -118,6 +290,22 @@ int gpiochip_add(struct gpio_chip *chip)
    >> }
    >>
    >> spin_unlock_irqrestore(&gpio_lock, flags);
    >> +
    >> + if (status)
    >> + goto fail;
    >> +
    >> + if (chip->dev) {
    >> + /* can sleep, so can't call with the spinlock held */

    >
    > You don't actually hold the spinlock at this point.


    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
    to anyone who thought they could simplify the logic.

    >> * state (such as pullup/pulldown configuration).
    >> + * @dev: optional device for the GPIO chip. Used to create sysfs files.

    >
    > Broken indentation.


    Looks like mailer damage from something, it's correct in my patch file.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 4 Apr 2008 12:07:12 -0700 (PDT), Trent Piepho wrote:
    > On Fri, 4 Apr 2008, Jean Delvare wrote:
    > > On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:
    > >> + strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
    > >> +
    > >> + return 5;

    > >
    > > Confusing construct... I suggest using sprintf instead, which will
    > > automatically return the correct number of bytes for you.

    >
    > But it's less efficient! Will nobody think of the wasted cycles?


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

    > (...)
    > >> +static void __exit gpio_class_exit(void)
    > >> +{
    > >> + /* FIXME: Code to remove all the sysfs devices and files created
    > >> + * should go here */

    > >
    > > Oh yes it really should

    >
    > 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
    > useful for the kernel?


    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
    couldn't be built as a module at all.

    > (...)
    > >> @@ -118,6 +290,22 @@ int gpiochip_add(struct gpio_chip *chip)
    > >> }
    > >>
    > >> spin_unlock_irqrestore(&gpio_lock, flags);
    > >> +
    > >> + if (status)
    > >> + goto fail;
    > >> +
    > >> + if (chip->dev) {
    > >> + /* can sleep, so can't call with the spinlock held */

    > >
    > > You don't actually hold the spinlock at this point.

    >
    > 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
    > to anyone who thought they could simplify the logic.


    Oops, sorry. I totally missed the "can't" in the comment.

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

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

    On Fri, 4 Apr 2008, Jean Delvare wrote:
    > On Fri, 4 Apr 2008 12:07:12 -0700 (PDT), Trent Piepho wrote:
    >> On Fri, 4 Apr 2008, Jean Delvare wrote:
    >>> On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:
    >>>> + strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
    >>>> +
    >>>> + return 5;
    >>>
    >>> Confusing construct... I suggest using sprintf instead, which will
    >>> automatically return the correct number of bytes for you.

    >>
    >> But it's less efficient! Will nobody think of the wasted cycles?

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


    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.

    But I should have put a in there, as I changed this code.

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


    I fixed all those. I wasn't clear on that when I wrote this code and forgot I
    had done it incorrectly.

    >>>> + /* FIXME: Code to remove all the sysfs devices and files created
    >>>> + * should go here */
    >>>
    >>> Oh yes it really should

    >>
    >> 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
    >> useful for the kernel?

    >
    > 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
    > couldn't be built as a module at all.


    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Friday 04 April 2008, Trent Piepho wrote:
    > >> +****if (test_bit(FLAG_IS_OUT, &gdesc->flags)) {
    > >> +************return -EINVAL;
    > >> +************/* strcpy(buf, "-1\n"); return 4; */ /* Or this? */

    > >
    > > Why not just return gpio_get_value(n) in all cases?


    That's the right answer...


    > > User might want to
    > > know which value is currently being output.

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


    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
    as what's being driven.


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


    Then don't worry about it. Any portable code can't rely on
    being able to do that.

    - Dave


    > 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
    > isn't desirable.
    >



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

+ Reply to Thread
Page 1 of 2 1 2 LastLast