[i2c] [PATCH] basic gpio_chip support for 16-bit PCA9539 GPIO expander

eric miao eric.y.miao at gmail.com
Fri Nov 30 01:48:32 CET 2007


On Nov 29, 2007 10:51 PM, Jean Delvare <khali at linux-fr.org> wrote:
> Hi Eric,
>
> On Thu, 29 Nov 2007 10:10:44 +0800, eric miao wrote:
> > Please comment, thanks.
> >
> > From 586db2cb6bf7e0353e07f8c4b648b752cda6ead5 Mon Sep 17 00:00:00 2001
> > From: eric miao <eric.miao at marvell.com>
> > Date: Thu, 29 Nov 2007 09:44:57 +0800
> > Subject: [PATCH] basic gpio_chip support for 16-bit PCA9539 GPIO expander
> >
> > 1. make the original pca9539 driver to use the new style I2C driver
> >    model
> >
> > 2. use "struct gpio_chip" so that PCA9539 can be used by Generic GPIO
> >    API
> >
> > 3. add "struct irq_chip" support for on-chip IRQs, due to the interrupt
> >    based I2C transfer, the IRQ handlers are actually performed within
> >    workqueue
> >
> > 4. remove those sensor attributes (assume that no one is ever using the
> >    GPIOs for sensor ...)
>
> Don't let the attribute names fool yourself: these attributes have
> nothing to do with sensors. The driver author simply found it
> convenient to reuse structures and macros that are commonly used by
> sensor drivers, but that's about it.
>

OK

> >
> > 5. platform code is required to setup
> >    a) the numbering of GPIO/IRQ for PCA9539
> >    b) the mapping between these GPIOs and IRQs
> >       i.e. keep correct gpio_to_irq()/irq_to_gpio()
> >    c) pass the start of gpio through i2c_board_info.platform_data
> >       by casting (so to not introduce another structure)
>
> Your driver offers an interface which is totally different from what the
> original driver did. Before you do that change, you need to figure out
> who is using the current interface and whether this is an option for
> them to switch to the new interface. Ben (Cc'd) should know.
>

OK, actually I was aware of that and sent this patch to Ben in another
mail. Thanks anyway.

> If you actually do that change, please update
> Documentation/i2c/chips/pca9539 accordingly.
>

OK

> >
> > Signed-off-by: eric miao <eric.miao at marvell.com>
> > ---
> >  drivers/i2c/chips/pca9539.c |  448 ++++++++++++++++++++++++++++++-------------
> >  1 files changed, 319 insertions(+), 129 deletions(-)
> >
> > diff --git a/drivers/i2c/chips/pca9539.c b/drivers/i2c/chips/pca9539.c
> > index f43c4e7..fc730ab 100644
> > --- a/drivers/i2c/chips/pca9539.c
> > +++ b/drivers/i2c/chips/pca9539.c
> > @@ -6,177 +6,368 @@
> >      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; version 2 of the License.
> > +
> > +    Copyright (C) 2007 Marvell Internation Ltd.
>
> Please move this next to the other copyright statement this driver
> already carries.
>

OK

> > +
> > +    2007-11-29 eric miao <eric.miao at marvell.com>
> > +               modified to use new style I2C driver model and support
> > +               gpio_chip & IRQs
>
> We have git for the changelogs.
>

OK. I was originally wondering the change is so big that I want people
to notice this. Anyway, I'll remove this.

>
> >  */
> >
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > -#include <linux/slab.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/workqueue.h>
> >  #include <linux/i2c.h>
> > -#include <linux/hwmon-sysfs.h>
> >
> > -/* Addresses to scan */
> > -static unsigned short normal_i2c[] = {0x74, 0x75, 0x76, 0x77, I2C_CLIENT_END};
> > +#include <asm/gpio.h>
> >
> > -/* Insmod parameters */
> > -I2C_CLIENT_INSMOD_1(pca9539);
> > +#define NR_PCA9539_GPIOS     16
> >
> > -enum pca9539_cmd
> > -{
> > -     PCA9539_INPUT_0         = 0,
> > -     PCA9539_INPUT_1         = 1,
> > -     PCA9539_OUTPUT_0        = 2,
> > -     PCA9539_OUTPUT_1        = 3,
> > -     PCA9539_INVERT_0        = 4,
> > -     PCA9539_INVERT_1        = 5,
> > -     PCA9539_DIRECTION_0     = 6,
> > -     PCA9539_DIRECTION_1     = 7,
> > -};
> > +#define PCA9539_INPUT                0
> > +#define PCA9539_OUTPUT               2
> > +#define PCA9539_INVERT               4
> > +#define PCA9539_DIRECTION    6
> >
> > -static int pca9539_attach_adapter(struct i2c_adapter *adapter);
> > -static int pca9539_detect(struct i2c_adapter *adapter, int address, int kind);
> > -static int pca9539_detach_client(struct i2c_client *client);
> > +struct pca9539_chip {
> > +     int irq;
> >
> > -/* This is the driver that will be inserted */
> > -static struct i2c_driver pca9539_driver = {
> > -     .driver = {
> > -             .name   = "pca9539",
> > -     },
> > -     .attach_adapter = pca9539_attach_adapter,
> > -     .detach_client  = pca9539_detach_client,
> > -};
> > +     unsigned gpio_start;
> > +     int irq_start;
> > +
> > +     uint16_t reg_output;
> > +     uint16_t reg_direction;
> > +
> > +     uint16_t last_input;
> >
> > -struct pca9539_data {
> > -     struct i2c_client client;
> > +     uint16_t irq_mask;
> > +     uint16_t irq_falling_edge;
> > +     uint16_t irq_rising_edge;
> > +
> > +     struct i2c_client *client;
> > +     struct gpio_chip gpio_chip;
> > +     struct irq_chip  irq_chip;
> > +
> > +     struct work_struct irq_work;
> >  };
> >
> > -/* following are the sysfs callback functions */
> > -static ssize_t pca9539_show(struct device *dev, struct device_attribute *attr,
> > -                         char *buf)
> > +static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
> >  {
> > -     struct sensor_device_attribute *psa = to_sensor_dev_attr(attr);
> > -     struct i2c_client *client = to_i2c_client(dev);
> > -     return sprintf(buf, "%d\n", i2c_smbus_read_byte_data(client,
> > -                                                          psa->index));
> > +     return i2c_smbus_write_word_data(chip->client, reg, val);
> >  }
> >
> > -static ssize_t pca9539_store(struct device *dev, struct device_attribute *attr,
> > -                          const char *buf, size_t count)
> > +static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
> >  {
> > -     struct sensor_device_attribute *psa = to_sensor_dev_attr(attr);
> > -     struct i2c_client *client = to_i2c_client(dev);
> > -     unsigned long val = simple_strtoul(buf, NULL, 0);
> > -     if (val > 0xff)
> > -             return -EINVAL;
> > -     i2c_smbus_write_byte_data(client, psa->index, val);
> > -     return count;
> > -}
> > -
> > -/* Define the device attributes */
> > -
> > -#define PCA9539_ENTRY_RO(name, cmd_idx) \
> > -     static SENSOR_DEVICE_ATTR(name, S_IRUGO, pca9539_show, NULL, cmd_idx)
> > -
> > -#define PCA9539_ENTRY_RW(name, cmd_idx) \
> > -     static SENSOR_DEVICE_ATTR(name, S_IRUGO | S_IWUSR, pca9539_show, \
> > -                               pca9539_store, cmd_idx)
> > -
> > -PCA9539_ENTRY_RO(input0, PCA9539_INPUT_0);
> > -PCA9539_ENTRY_RO(input1, PCA9539_INPUT_1);
> > -PCA9539_ENTRY_RW(output0, PCA9539_OUTPUT_0);
> > -PCA9539_ENTRY_RW(output1, PCA9539_OUTPUT_1);
> > -PCA9539_ENTRY_RW(invert0, PCA9539_INVERT_0);
> > -PCA9539_ENTRY_RW(invert1, PCA9539_INVERT_1);
> > -PCA9539_ENTRY_RW(direction0, PCA9539_DIRECTION_0);
> > -PCA9539_ENTRY_RW(direction1, PCA9539_DIRECTION_1);
> > -
> > -static struct attribute *pca9539_attributes[] = {
> > -     &sensor_dev_attr_input0.dev_attr.attr,
> > -     &sensor_dev_attr_input1.dev_attr.attr,
> > -     &sensor_dev_attr_output0.dev_attr.attr,
> > -     &sensor_dev_attr_output1.dev_attr.attr,
> > -     &sensor_dev_attr_invert0.dev_attr.attr,
> > -     &sensor_dev_attr_invert1.dev_attr.attr,
> > -     &sensor_dev_attr_direction0.dev_attr.attr,
> > -     &sensor_dev_attr_direction1.dev_attr.attr,
> > -     NULL
> > -};
> > +     int ret;
> >
> > -static struct attribute_group pca9539_defattr_group = {
> > -     .attrs = pca9539_attributes,
> > -};
> > +     ret = i2c_smbus_read_word_data(chip->client, reg);
> > +     if (ret < 0) {
> > +             printk(KERN_ERR "%s: failed to read\n", __FUNCTION__);
> > +             return ret;
> > +     }
> > +
> > +     *val = (uint16_t)ret;
> > +     return 0;
> > +}
> > +
> > +static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
> > +{
> > +     struct pca9539_chip *chip;
> > +     uint16_t reg_val;
> > +     int ret;
> > +
> > +     chip = container_of(gc, struct pca9539_chip, gpio_chip);
> > +
> > +     reg_val = chip->reg_direction | (1u << off);
> > +     ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     chip->reg_direction = reg_val;
> > +     return 0;
> > +}
> >
> > -static int pca9539_attach_adapter(struct i2c_adapter *adapter)
> > +static int pca9539_gpio_direction_output(struct gpio_chip *gc,
> > +             unsigned off, int val)
> >  {
> > -     return i2c_probe(adapter, &addr_data, pca9539_detect);
> > +     struct pca9539_chip *chip;
> > +     uint16_t reg_val;
> > +     int ret;
> > +
> > +     chip = container_of(gc, struct pca9539_chip, gpio_chip);
> > +
> > +     /* set output level */
> > +     if (val)
> > +             reg_val = chip->reg_output | (1u << off);
> > +     else
> > +             reg_val = chip->reg_output & ~(1u << off);
> > +
> > +     ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     chip->reg_output = reg_val;
> > +
> > +     /* then direction */
> > +     reg_val = chip->reg_direction & ~(1u << off);
> > +     ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     chip->reg_direction = reg_val;
> > +     return 0;
> > +}
> > +
> > +static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
> > +{
> > +     struct pca9539_chip *chip;
> > +     uint16_t reg_val;
> > +     int ret;
> > +
> > +     chip = container_of(gc, struct pca9539_chip, gpio_chip);
> > +
> > +     ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return (reg_val & (1u << off)) ? 1 : 0;
> > +}
> > +
> > +static void pca9539_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> > +{
> > +     struct pca9539_chip *chip;
> > +     uint16_t reg_val;
> > +     int ret;
> > +
> > +     chip = container_of(gc, struct pca9539_chip, gpio_chip);
> > +
> > +     if (val)
> > +             reg_val = chip->reg_output | (1u << off);
> > +     else
> > +             reg_val = chip->reg_output & ~(1u << off);
> > +
> > +     ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
> > +     if (ret)
> > +             return;
> > +
> > +     chip->reg_output = reg_val;
> > +}
> > +
> > +static int pca9539_init_gpio(struct pca9539_chip *chip)
> > +{
> > +     struct gpio_chip *gc;
> > +
> > +     gc = &chip->gpio_chip;
> > +
> > +     gc->direction_input  = pca9539_gpio_direction_input;
> > +     gc->direction_output = pca9539_gpio_direction_output;
> > +     gc->get = pca9539_gpio_get_value;
> > +     gc->set = pca9539_gpio_set_value;
> > +
> > +     gc->base = chip->gpio_start;
> > +     gc->ngpio = NR_PCA9539_GPIOS;
> > +
> > +     return gpiochip_add(gc);
> >  }
> >
> > -/* This function is called by i2c_probe */
> > -static int pca9539_detect(struct i2c_adapter *adapter, int address, int kind)
> > +/* FIXME: change to schedule_delayed_work() here if reading out of
> > + * registers does not reflect the actual pin levels
> > + */
> > +
> > +static void pca9539_irq_work(struct work_struct *work)
> >  {
> > -     struct i2c_client *new_client;
> > -     struct pca9539_data *data;
> > -     int err = 0;
> > +     struct pca9539_chip *chip;
> > +     uint16_t input, mask, rising, falling;
> > +     int ret, i;
> > +
> > +     chip = container_of(work, struct pca9539_chip, irq_work);
> > +
> > +     ret = pca9539_read_reg(chip, PCA9539_INPUT, &input);
> > +     if (ret < 0)
> > +             return;
> >
> > -     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > -             goto exit;
> > +     mask = (input ^ chip->last_input) & chip->irq_mask;
> > +     rising = (input & mask) & chip->irq_rising_edge;
> > +     falling = (~input & mask) & chip->irq_falling_edge;
> >
> > -     /* OK. For now, we presume we have a valid client. We now create the
> > -        client structure, even though we cannot fill it completely yet. */
> > -     if (!(data = kzalloc(sizeof(struct pca9539_data), GFP_KERNEL))) {
> > -             err = -ENOMEM;
> > -             goto exit;
> > +     irq_enter();
> > +
> > +     for (i = 0; i < NR_PCA9539_GPIOS; i++) {
> > +             if ((rising | falling) & (1u << i)) {
> > +                     int irq = chip->irq_start + i;
> > +                     struct irq_desc *desc;
> > +
> > +                     desc = irq_desc + irq;
> > +                     desc_handle_irq(irq, desc);
> > +             }
> >       }
> >
> > -     new_client = &data->client;
> > -     i2c_set_clientdata(new_client, data);
> > -     new_client->addr = address;
> > -     new_client->adapter = adapter;
> > -     new_client->driver = &pca9539_driver;
> > -     new_client->flags = 0;
> > -
> > -     if (kind < 0) {
> > -             /* Detection: the pca9539 only has 8 registers (0-7).
> > -                A read of 7 should succeed, but a read of 8 should fail. */
> > -             if ((i2c_smbus_read_byte_data(new_client, 7) < 0) ||
> > -                 (i2c_smbus_read_byte_data(new_client, 8) >= 0))
> > -                     goto exit_kfree;
> > +     irq_exit();
> > +
> > +     chip->last_input = input;
> > +}
> > +
> > +static void fastcall
> > +pca9539_irq_demux(unsigned int irq, struct irq_desc *desc)
> > +{
> > +     struct pca9539_chip *chip = desc->handler_data;
> > +
> > +     desc->chip->mask(chip->irq);
> > +     desc->chip->ack(chip->irq);
> > +     schedule_work(&chip->irq_work);
> > +     desc->chip->unmask(chip->irq);
> > +}
> > +
> > +static void pca9539_irq_mask(unsigned int irq)
> > +{
> > +     struct irq_desc *desc = irq_desc + irq;
> > +     struct pca9539_chip *chip = desc->chip_data;
> > +
> > +     chip->irq_mask &= ~(1u << (irq - chip->irq_start));
> > +}
> > +
> > +static void pca9539_irq_unmask(unsigned int irq)
> > +{
> > +     struct irq_desc *desc = irq_desc + irq;
> > +     struct pca9539_chip *chip = desc->chip_data;
> > +
> > +     chip->irq_mask |= 1u << (irq - chip->irq_start);
> > +}
> > +
> > +static void pca9539_irq_ack(unsigned int irq)
> > +{
> > +     /* unfortunately, we have to provide an empty irq_chip.ack even
> > +      * if we do nothing here, Generic IRQ will complain otherwise
> > +      */
> > +}
> > +
> > +static int pca9539_irq_set_type(unsigned int irq, unsigned int type)
> > +{
> > +     struct irq_desc *desc = irq_desc + irq;
> > +     struct pca9539_chip *chip = desc->chip_data;
> > +     uint16_t mask = 1u << (irq - chip->irq_start);
> > +
> > +     if (type == IRQT_PROBE) {
> > +             if ((mask & chip->irq_rising_edge) ||
> > +                 (mask & chip->irq_falling_edge) ||
> > +                 (mask & ~chip->reg_direction))
> > +                     return 0;
> > +
> > +             type = __IRQT_RISEDGE | __IRQT_FALEDGE;
> >       }
> >
> > -     strlcpy(new_client->name, "pca9539", I2C_NAME_SIZE);
> > +     gpio_direction_input(irq_to_gpio(irq));
> >
> > -     /* Tell the I2C layer a new client has arrived */
> > -     if ((err = i2c_attach_client(new_client)))
> > -             goto exit_kfree;
> > +     if (type & __IRQT_RISEDGE)
> > +             chip->irq_rising_edge |= mask;
> > +     else
> > +             chip->irq_rising_edge &= ~mask;
> >
> > -     /* Register sysfs hooks */
> > -     err = sysfs_create_group(&new_client->dev.kobj,
> > -                              &pca9539_defattr_group);
> > -     if (err)
> > -             goto exit_detach;
> > +     if (type & __IRQT_FALEDGE)
> > +             chip->irq_falling_edge |= mask;
> > +     else
> > +             chip->irq_falling_edge &= ~mask;
> >
> >       return 0;
> > +}
> > +
> > +static int pca9539_init_irq(struct pca9539_chip *chip)
> > +{
> > +     struct irq_chip *ic = &chip->irq_chip;
> > +     int irq, irq_start = chip->irq_start;
> > +
> > +     /* do not install GPIO interrupts for the chip if
> > +      * 1. the PCA9539 interrupt line is not used
> > +      * 2. or the GPIO interrupt number exceeds NR_IRQS
> > +      */
> > +     if (chip->irq <= 0 || irq_start + NR_PCA9539_GPIOS >= NR_IRQS)
> > +             return -EINVAL;
> >
> > -exit_detach:
> > -     i2c_detach_client(new_client);
> > -exit_kfree:
> > -     kfree(data);
> > -exit:
> > -     return err;
> > +     chip->irq_mask  = 0;
> > +     chip->irq_rising_edge  = 0;
> > +     chip->irq_falling_edge = 0;
> > +
> > +     ic->ack         = pca9539_irq_ack;
> > +     ic->mask        = pca9539_irq_mask;
> > +     ic->unmask      = pca9539_irq_unmask;
> > +     ic->set_type    = pca9539_irq_set_type;
>
> No alignments in code please.
>
>
> > +
> > +     for (irq = irq_start; irq < irq_start + NR_PCA9539_GPIOS; irq++) {
> > +             set_irq_chip(irq, ic);
> > +             set_irq_chip_data(irq, chip);
> > +             set_irq_handler(irq, handle_edge_irq);
> > +             set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> > +     }
> > +
> > +     set_irq_type(chip->irq, IRQT_FALLING);
> > +     set_irq_data(chip->irq, chip);
> > +     set_irq_chained_handler(chip->irq, pca9539_irq_demux);
> > +
> > +     INIT_WORK(&chip->irq_work, pca9539_irq_work);
> > +     return 0;
> >  }
> >
> > -static int pca9539_detach_client(struct i2c_client *client)
> > +static int __devinit pca9539_probe(struct i2c_client *client)
> >  {
> > -     int err;
> > +     struct pca9539_chip *chip;
> > +     unsigned gpio_start;
> > +     int ret;
> > +
> > +     /* NOTE: treat *platform_data as a cast of unsigned number of
> > +      * start GPIO, so to avoid defining a "pca9539_platform_data"
> > +      */
> > +     gpio_start = (unsigned)(client->dev.platform_data);
> > +
> > +     chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
> > +     if (chip == NULL)
> > +             return -ENOMEM;
> > +
> > +     chip->client = client;
> > +     chip->irq = client->irq;
> >
> > -     sysfs_remove_group(&client->dev.kobj, &pca9539_defattr_group);
> > +     chip->gpio_start = gpio_start;
> > +     chip->irq_start  = gpio_to_irq(gpio_start);
> >
> > -     if ((err = i2c_detach_client(client)))
> > -             return err;
> > +     /* read init register values */
> > +     ret = pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);
> > +     if (ret)
> > +             goto out_failed;
> >
> > -     kfree(i2c_get_clientdata(client));
> > +     /* initialize registers */
> > +     chip->reg_output = 0xffff;
> > +     chip->reg_direction = 0xffff;
> > +
> > +     pca9539_write_reg(chip, PCA9539_OUTPUT, chip->reg_output);
> > +     pca9539_write_reg(chip, PCA9539_OUTPUT, chip->reg_direction);
> > +
> > +     /* no polarity inversion */
> > +     pca9539_write_reg(chip, PCA9539_INVERT, 0);
>
> What if some of these GPIOs were in output mode and actually inverted,
> e.g. by some firmware/BIOS code? Your code would arbitrarily change
> their output value, right? Doesn't look good.
>

I was thinking of keeping the logic simple, default to no polarity inversion
looks like sane. We don't yet have standard interface to control the GPIO
polarity, however I guess adding a platform specific field for this would
be OK. That way, one can assign the initial polarity inversion settings,
though he can still not change it on-the-fly.

>
> > +
> > +     ret = pca9539_init_gpio(chip);
> > +     if (ret)
> > +             goto out_failed;
> > +
> > +     ret = pca9539_init_irq(chip);
> > +     if (ret)
> > +             goto out_failed;
> > +
> > +     i2c_set_clientdata(client, chip);
> >       return 0;
> > +
> > +out_failed:
> > +     kfree(chip);
> > +     return ret;
> >  }
> >
> > +/* FIXME: PCA9539 is not supposed to be removed for now */
> > +static struct i2c_driver pca9539_driver = {
> > +     .driver = {
> > +             .name   = "pca9539",
> > +     },
> > +     .probe          = pca9539_probe,
> > +};
> > +
> >  static int __init pca9539_init(void)
> >  {
> >       return i2c_add_driver(&pca9539_driver);
> > @@ -193,4 +384,3 @@ MODULE_LICENSE("GPL");
> >
> >  module_init(pca9539_init);
> >  module_exit(pca9539_exit);
>
> --
> Jean Delvare
>

Thanks Jean for your comments

-- 
Cheers
- eric



More information about the i2c mailing list