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

[David Brownell]

On Monday 03 December 2007, eric miao wrote:
> Attached are the patches updated (sorry for the inconvenience). As suggested,
> divided into two patches, and introduce a CONFIG_PCA9539_GENERIC_IRQ
> option to be selected _only_ when the driver is selected to be built-in, thus
> avoid ugly #ifdef MODULE in driver, and solve the dependency issue
> in a cleaner way.

Comments here on just the first one ... I got build errors with the second.

One generic comment:  this is kernel code, so use "u16" not "uint16_t".


> From 061f7affff96879e13fc66db534890dd4e8ac7b0 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] pxa: 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

Well, by/with the new "gpiolib" infrastructure of which "gpio_chip" is part.


> 3. remove sysfs entries to avoid register access conflict with Generic
>    GPIO support (assuming that no one is ever using these entries)
>
> 4. use 2 x 8-bit register access to simplify the logic, cache OUTPUT
>    and DIRECTION registers for fast access
>
> 5. platform code is required to setup
>    a) the numbering of GPIO for PCA9539
>    b) a correct gpio_to_irq()/irq_to_gpio()

(5b) is not required for this particular patch, since those routines
should be ignoring these chips for now...


>    c) pass "pca9539_platform_data" within "i2c_board_info"
>
> Signed-off-by: eric miao <eric.miao at marvell.com>
> Acked-by: Ben Gardner <bgardner at wabtec.com>
> ---
>  Documentation/i2c/chips/pca9539 |   30 +---
>  drivers/i2c/chips/Kconfig       |    2 +-
>  drivers/i2c/chips/pca9539.c     |  322 +++++++++++++++++++++++----------------
>  include/linux/i2c/pca9539.h     |   18 +++
>  4 files changed, 217 insertions(+), 155 deletions(-)
>  create mode 100644 include/linux/i2c/pca9539.h
>
> diff --git a/Documentation/i2c/chips/pca9539
> b/Documentation/i2c/chips/pca9539 index c4fce6a..1b5fd41 100644
> --- a/Documentation/i2c/chips/pca9539
> +++ b/Documentation/i2c/chips/pca9539
> @@ -19,29 +19,15 @@ All 16 lines can be individually configured as an input
> or output. The input sense can also be inverted.
>  The 16 lines are split between two bytes.
>
> +Generic GPIO support
> +--------------------
> +
> +The extended I/O ports are now fully covered by Generic GPIO API with
> +GPIO lib support. The "struct pca9539_platform_data" has to be filled
> +by platform specific code to properly initialize the PCA9539.

Specifically, gpio_base is mandatory.  The rest is optional.


>
>  Sysfs entries
>  -------------
>
> -Each is a byte that maps to the 8 I/O bits.
> -A '0' suffix is for bits 0-7, while '1' is for bits 8-15.
> -
> -input[01]     - read the current value
> -output[01]    - sets the output value
> -direction[01] - direction of each bit: 1=input, 0=output
> -invert[01]    - toggle the input bit sense
> -
> -input reads the actual state of the line and is always available.
> -The direction defaults to input for all channels.
> -
> -
> -General Remarks
> ----------------
> -
> -Note that each output, direction, and invert entry controls 8 lines.
> -You should use the read, modify, write sequence.
> -For example. to set output bit 0 of 1.
> -  val=$(cat output0)
> -  val=$(( $val | 1 ))
> -  echo $val > output0
> -
> +The original sysfs entries are removed to avoid register access conflict
> +with the Generic GPIO support.

> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig

Jean suggets moving this to drivers/gpio ... I'd suggest doing that as
a separate patch, assuming we go with a drivers/gpio directory.


> @@ -67,7 +67,7 @@ config SENSORS_PCF8574
>
>  config SENSORS_PCA9539
>  	tristate "Philips PCA9539 16-bit I/O port"
> -	depends on EXPERIMENTAL
> +	depends on EXPERIMENTAL && GPIO_LIB

What's EXPERIMENTAL about it though?

>  	help
>  	  If you say yes here you get support for the Philips PCA9539
>  	  16-bit I/O port.


> --- a/drivers/i2c/chips/pca9539.c
> +++ b/drivers/i2c/chips/pca9539.c
> @@ -2,6 +2,7 @@
>      pca9539.c - 16-bit I/O port with interrupt and reset
>
>      Copyright (C) 2005 Ben Gardner <bgardner at wabtec.com>
> +    Copyright (C) 2007 Marvell Internation Ltd.
>
>      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
> @@ -10,173 +11,231 @@
>
>  #include <linux/module.h>
>  #include <linux/init.h>
> -#include <linux/slab.h>
>  #include <linux/i2c.h>
> -#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c/pca9539.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 {
> +	unsigned gpio_start;
> +	uint16_t reg_output;
> +	uint16_t reg_direction;
> +	uint16_t last_input;

There's no point in remembering "last_input"; you read its value once
and then never touch it again.

>
> -/* 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,
> +	struct i2c_client *client;
> +	struct gpio_chip gpio_chip;
>  };
>
> -struct pca9539_data {
> -	struct i2c_client client;
> -};
> +static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t
> val) +{
> +	return i2c_smbus_write_word_data(chip->client, reg, val);
> +}
>
> -/* following are the sysfs callback functions */
> -static ssize_t pca9539_show(struct device *dev, struct device_attribute
> *attr, -			    char *buf)
> +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);
> -	return sprintf(buf, "%d\n", i2c_smbus_read_byte_data(client,
> -							     psa->index));
> +	int ret;
> +
> +	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 ssize_t pca9539_store(struct device *dev, struct device_attribute
> *attr, -			     const char *buf, size_t count)
> +static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned
> off) {
> -	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;
> +	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;
>  }
>
> -/* 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
> -};
> +static int pca9539_gpio_direction_output(struct gpio_chip *gc,
> +		unsigned off, int val)
> +{
> +	struct pca9539_chip *chip;
> +	uint16_t reg_val;
> +	int ret;
>
> -static struct attribute_group pca9539_defattr_group = {
> -	.attrs = pca9539_attributes,
> -};
> +	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;
>
> -static int pca9539_attach_adapter(struct i2c_adapter *adapter)
> +	chip->reg_direction = reg_val;
> +	return 0;
> +}
> +
> +static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
>  {
> -	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);
> +
> +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (reg_val & (1u << off)) ? 1 : 0;
>  }
>
> -/* This function is called by i2c_probe */
> -static int pca9539_detect(struct i2c_adapter *adapter, int address, int
> kind) +static void pca9539_gpio_set_value(struct gpio_chip *gc, unsigned
> off, int val) {
> -	struct i2c_client *new_client;
> -	struct pca9539_data *data;
> -	int err = 0;
> -
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> -		goto exit;
> -
> -	/* 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;
> -	}
> +	struct pca9539_chip *chip;
> +	uint16_t reg_val;
> +	int ret;
>
> -	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;
> -	}
> +	chip = container_of(gc, struct pca9539_chip, gpio_chip);
>
> -	strlcpy(new_client->name, "pca9539", I2C_NAME_SIZE);
> +	if (val)
> +		reg_val = chip->reg_output | (1u << off);
> +	else
> +		reg_val = chip->reg_output & ~(1u << off);
>
> -	/* Tell the I2C layer a new client has arrived */
> -	if ((err = i2c_attach_client(new_client)))
> -		goto exit_kfree;
> +	ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
> +	if (ret)
> +		return;
>
> -	/* Register sysfs hooks */
> -	err = sysfs_create_group(&new_client->dev.kobj,
> -				 &pca9539_defattr_group);
> -	if (err)
> -		goto exit_detach;
> +	chip->reg_output = reg_val;
> +}
>
> -	return 0;
> +static int pca9539_init_gpio(struct pca9539_chip *chip)
> +{
> +	struct gpio_chip *gc;
> +
> +	gc = &chip->gpio_chip;
>
> -exit_detach:
> -	i2c_detach_client(new_client);
> -exit_kfree:
> -	kfree(data);
> -exit:
> -	return err;
> +	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;
> +	gc->label = "pca9539";
> +
> +	return gpiochip_add(gc);
>  }
>
> -static int pca9539_detach_client(struct i2c_client *client)
> +static int __devinit pca9539_probe(struct i2c_client *client)
>  {
> -	int err;
> +	struct pca9539_platform_data *pdata;
> +	struct pca9539_chip *chip;
> +	int ret;
> +
> +	pdata = client->dev.platform_data;
> +	if (pdata == NULL)
> +		return -ENODEV;
> +
> +	chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
> +	if (chip == NULL)
> +		return -ENOMEM;
>
> -	sysfs_remove_group(&client->dev.kobj, &pca9539_defattr_group);
> +	chip->client = client;
>
> -	if ((err = i2c_detach_client(client)))
> -		return err;
> +	chip->gpio_start = pdata->gpio_base;
>
> -	kfree(i2c_get_clientdata(client));
> +	/* read init register values */
> +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);

As noted above:  this is the only use of "last_input", so it's not
necessary.


> +	if (ret)
> +		goto out_failed;
> +
> +	/* initialize registers */
> +	chip->reg_output = 0xffff;
> +	chip->reg_direction = 0xffff;

Why don't you read those two values -- output, direction -- from the
chip instead of defaulting them to something that may be wrong?  If
some earlier code (bootloader, etc) initialized those values, it would
be better not to change them.  Any setup() call can change them, of
course.


> +
> +	pca9539_write_reg(chip, PCA9539_OUTPUT, chip->reg_output);
> +	pca9539_write_reg(chip, PCA9539_DIRECTION, chip->reg_direction);
> +
> +	/* set platform specific polarity inversion */
> +	pca9539_write_reg(chip, PCA9539_INVERT, pdata->invert);

In this case it's probably less harmful to override whatever the
bootloader may have assigned to the register.


> +
> +	ret = pca9539_init_gpio(chip);
> +	if (ret)
> +		goto out_failed;
> +
> +	if (pdata->setup) {
> +		ret = pdata->setup(client, chip->gpio_chip.base,
> +				chip->gpio_chip.ngpio, pdata->context);
> +		if (ret < 0)
> +			dev_dbg(&client->dev, "setup failed, %d\n", ret);
> +	}
> +
> +	i2c_set_clientdata(client, chip);
>  	return 0;
> +
> +out_failed:
> +	kfree(chip);
> +	return ret;
>  }
>
> +static int pca9539_remove(struct i2c_client *client)
> +{
> +	struct pca9539_platform_data *pdata = client->dev.platform_data;
> +	struct pca9539_chip *chip = i2c_get_clientdata(client);
> +	int ret = 0;
> +
> +	if (pdata->teardown) {
> +		ret = pdata->teardown(client, chip->gpio_chip.base,
> +				chip->gpio_chip.ngpio, pdata->context);
> +		if (ret < 0)
> +			dev_dbg(&client->dev, "teardown failed, %d\n", ret);

In the pcf857x driver I made teardown() failure abort the remove(),
on the grounds that it probably means the chip is still busy...


> +	}
> +
> +	ret = gpiochip_remove(&chip->gpio_chip);
> +	if (ret) {
> +		dev_err(&client->dev, "failed remove gpio_chip\n");
> +		return ret;
> +	}
> +
> +	kfree(chip);
> +	return 0;
> +}
> +
> +static struct i2c_driver pca9539_driver = {
> +	.driver = {
> +		.name	= "pca9539",
> +	},
> +	.probe		= pca9539_probe,
> +	.remove		= pca9539_remove,
> +};
> +
>  static int __init pca9539_init(void)
>  {
>  	return i2c_add_driver(&pca9539_driver);
> @@ -193,4 +252,3 @@ MODULE_LICENSE("GPL");
>
>  module_init(pca9539_init);
>  module_exit(pca9539_exit);
> -

> --- /dev/null
> +++ b/include/linux/i2c/pca9539.h
> @@ -0,0 +1,18 @@
> +/* platform data for the PCA9539 16-bit I/O expander driver */
> +
> +struct pca9539_platform_data {
> +	/* number of the first GPIO */
> +	unsigned	gpio_base;
> +
> +	/* initial polarity inversion setting */
> +	uint16_t	invert;
> +
> +	void		*context;	/* param to setup/teardown */
> +
> +	int		(*setup)(struct i2c_client *client,
> +				unsigned gpio, unsigned ngpio,
> +				void *context);
> +	int		(*teardown)(struct i2c_client *client,
> +				unsigned gpio, unsigned ngpio,
> +				void *context);
> +};