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

Jean Delvare khali at linux-fr.org
Thu Nov 29 15:51:56 CET 2007


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.

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

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

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

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

>  */
> 
>  #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.

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



More information about the i2c mailing list