[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, ®_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