[RFC PATCH 3/8] Philips PCA9536 4 bit I2C GPIO extender driver

[RFC PATCH 3/8] Philips PCA9536 4 bit I2C GPIO extender driver

[Guennadi Liakhovetski]

This driver will be used by the mt9m001 and the mt9v022 camera drivers,
that can use a pca9536 to switch between 8 and 10 bit modes.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>
---
 drivers/i2c/chips/Kconfig   |   10 +++
 drivers/i2c/chips/Makefile  |    1 +
 drivers/i2c/chips/pca9536.c |  153 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/pca9536.c

diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
index 2e1c24f..6f492f1 100644
--- a/drivers/i2c/chips/Kconfig
+++ b/drivers/i2c/chips/Kconfig
@@ -65,6 +65,16 @@ config SENSORS_PCF8574
 	  These devices are hard to detect and rarely found on mainstream
 	  hardware.  If unsure, say N.
 
+config SENSORS_PCA9536
+	tristate "Philips PCA9536 4-bit I/O port"
+	depends on EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Philips PCA9539
+	  4-bit I/O port.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called pca9536.
+
 config SENSORS_PCA9539
 	tristate "Philips PCA9539 16-bit I/O port"
 	depends on EXPERIMENTAL
diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
index ca924e1..e24c582 100644
--- a/drivers/i2c/chips/Makefile
+++ b/drivers/i2c/chips/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
+obj-$(CONFIG_SENSORS_PCA9536)	+= pca9536.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
diff --git a/drivers/i2c/chips/pca9536.c b/drivers/i2c/chips/pca9536.c
new file mode 100644
index 0000000..08e5d8b
--- /dev/null
+++ b/drivers/i2c/chips/pca9536.c
@@ -0,0 +1,152 @@
+/*
+ * Driver for PCA9536 I/O Expander
+ *
+ * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/videodev.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <media/v4l2-common.h>
+#include <media/v4l2-chip-ident.h>
+
+#define PCA9536_INPUT 0
+#define PCA9536_OUTPUT 1
+#define PCA9536_INVERT 2
+#define PCA9536_DIRECTION 3
+
+/* following are the sysfs callback functions */
+static ssize_t pca9536_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	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));
+}
+
+static ssize_t pca9536_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	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;
+}
+
+int pca9536_set_level(struct i2c_client *client, u8 pin, u8 level)
+{
+	s32 data;
+
+	if (pin > 3)
+		return -EINVAL;
+
+	/* Switch to output */
+	data = i2c_smbus_read_byte_data(client, PCA9536_DIRECTION);
+	if (data < 0)
+		return data;
+	data = i2c_smbus_write_byte_data(client, PCA9536_DIRECTION,
+					 data & ~(1 << pin));
+	if (data < 0)
+		return data;
+
+	/* Set level */
+	data = i2c_smbus_read_byte_data(client, PCA9536_OUTPUT);
+	if (data < 0)
+		return data;
+	if (level)
+		data |= (1 << pin);
+	else
+		data &= ~(1 << pin);
+	data = i2c_smbus_write_byte_data(client, PCA9536_OUTPUT, data);
+
+	return data;
+}
+EXPORT_SYMBOL(pca9536_set_level);
+
+/* Define the device attributes */
+
+#define PCA9536_ENTRY_RO(name, reg) \
+	static SENSOR_DEVICE_ATTR(name, S_IRUGO, pca9536_show, NULL, reg)
+
+#define PCA9536_ENTRY_RW(name, reg) \
+	static SENSOR_DEVICE_ATTR(name, S_IRUGO | S_IWUSR, pca9536_show, \
+				  pca9536_store, reg)
+
+PCA9536_ENTRY_RO(input, PCA9536_INPUT);
+PCA9536_ENTRY_RW(output, PCA9536_OUTPUT);
+PCA9536_ENTRY_RW(invert, PCA9536_INVERT);
+PCA9536_ENTRY_RW(direction, PCA9536_DIRECTION);
+
+static struct attribute *pca9536_attributes[] = {
+	&sensor_dev_attr_input.dev_attr.attr,
+	&sensor_dev_attr_output.dev_attr.attr,
+	&sensor_dev_attr_invert.dev_attr.attr,
+	&sensor_dev_attr_direction.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group pca9536_defattr_group = {
+	.attrs = pca9536_attributes,
+};
+
+static int pca9536_probe(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		pr_debug("%s: I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n", __FUNCTION__);
+		return -EIO;
+	}
+
+	if (client->dev.platform_data)
+		/* This chip is going to be used from the kernel */
+		*(struct i2c_client **)client->dev.platform_data = client;
+
+	/* Register sysfs hooks */
+	return sysfs_create_group(&client->dev.kobj,
+				  &pca9536_defattr_group);
+}
+
+static int pca9536_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &pca9536_defattr_group);
+	return 0;
+}
+
+static struct i2c_driver pca9536_i2c_driver = {
+	.driver = {
+		.name = "pca9536",
+	},
+	.probe		= pca9536_probe,
+	.remove		= pca9536_remove,
+};
+
+static int __init pca9536_mod_init(void)
+{
+	return i2c_add_driver(&pca9536_i2c_driver);
+}
+
+static void __exit pca9536_mod_exit(void)
+{
+	i2c_del_driver(&pca9536_i2c_driver);
+}
+
+module_init(pca9536_mod_init);
+module_exit(pca9536_mod_exit);
+
+MODULE_DESCRIPTION("PCA9536 driver");
+MODULE_AUTHOR("Guennadi Liakhovetski <kernel@pengutronix.de>");
+MODULE_LICENSE("GPL");
-- 
1.5.3.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Philips PCA9536 4 bit I2C GPIO extender driver

[Jean Delvare]

Hi Guennadi,

On Wed, 23 Jan 2008 18:41:39 +0100 (CET), Guennadi Liakhovetski wrote:
> This driver will be used by the mt9m001 and the mt9v022 camera drivers,
> that can use a pca9536 to switch between 8 and 10 bit modes.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/chips/Kconfig   |   10 +++
>  drivers/i2c/chips/Makefile  |    1 +
>  drivers/i2c/chips/pca9536.c |  153 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 164 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/chips/pca9536.c

There is a new GPIO subsystem (under driver/gpio), that's where you
should add this driver.

> 
> diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> index 2e1c24f..6f492f1 100644
> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig
> @@ -65,6 +65,16 @@ config SENSORS_PCF8574
>  	  These devices are hard to detect and rarely found on mainstream
>  	  hardware.  If unsure, say N.
>  
> +config SENSORS_PCA9536
> +	tristate "Philips PCA9536 4-bit I/O port"
> +	depends on EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Philips PCA9539
> +	  4-bit I/O port.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called pca9536.
> +
>  config SENSORS_PCA9539
>  	tristate "Philips PCA9539 16-bit I/O port"
>  	depends on EXPERIMENTAL
> diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> index ca924e1..e24c582 100644
> --- a/drivers/i2c/chips/Makefile
> +++ b/drivers/i2c/chips/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
>  obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
>  obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
>  obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
> +obj-$(CONFIG_SENSORS_PCA9536)	+= pca9536.o
>  obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
>  obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> diff --git a/drivers/i2c/chips/pca9536.c b/drivers/i2c/chips/pca9536.c
> new file mode 100644
> index 0000000..08e5d8b
> --- /dev/null
> +++ b/drivers/i2c/chips/pca9536.c
> @@ -0,0 +1,152 @@
> +/*
> + * Driver for PCA9536 I/O Expander
> + *
> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/videodev.h>
> +#include <linux/slab.h>

I don't think you need slab.h.

> +#include <linux/i2c.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-chip-ident.h>

A GPIO driver shouldn't include video-specific header files.

> +
> +#define PCA9536_INPUT 0
> +#define PCA9536_OUTPUT 1
> +#define PCA9536_INVERT 2
> +#define PCA9536_DIRECTION 3
> +
> +/* following are the sysfs callback functions */
> +static ssize_t pca9536_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	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));
> +}
> +
> +static ssize_t pca9536_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	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;
> +}
> +
> +int pca9536_set_level(struct i2c_client *client, u8 pin, u8 level)
> +{
> +	s32 data;
> +
> +	if (pin > 3)
> +		return -EINVAL;
> +
> +	/* Switch to output */
> +	data = i2c_smbus_read_byte_data(client, PCA9536_DIRECTION);
> +	if (data < 0)
> +		return data;
> +	data = i2c_smbus_write_byte_data(client, PCA9536_DIRECTION,
> +					 data & ~(1 << pin));
> +	if (data < 0)
> +		return data;
> +
> +	/* Set level */
> +	data = i2c_smbus_read_byte_data(client, PCA9536_OUTPUT);
> +	if (data < 0)
> +		return data;
> +	if (level)
> +		data |= (1 << pin);
> +	else
> +		data &= ~(1 << pin);
> +	data = i2c_smbus_write_byte_data(client, PCA9536_OUTPUT, data);
> +
> +	return data;
> +}
> +EXPORT_SYMBOL(pca9536_set_level);
> +
> +/* Define the device attributes */
> +
> +#define PCA9536_ENTRY_RO(name, reg) \
> +	static SENSOR_DEVICE_ATTR(name, S_IRUGO, pca9536_show, NULL, reg)
> +
> +#define PCA9536_ENTRY_RW(name, reg) \
> +	static SENSOR_DEVICE_ATTR(name, S_IRUGO | S_IWUSR, pca9536_show, \
> +				  pca9536_store, reg)
> +
> +PCA9536_ENTRY_RO(input, PCA9536_INPUT);
> +PCA9536_ENTRY_RW(output, PCA9536_OUTPUT);
> +PCA9536_ENTRY_RW(invert, PCA9536_INVERT);
> +PCA9536_ENTRY_RW(direction, PCA9536_DIRECTION);
> +
> +static struct attribute *pca9536_attributes[] = {
> +	&sensor_dev_attr_input.dev_attr.attr,
> +	&sensor_dev_attr_output.dev_attr.attr,
> +	&sensor_dev_attr_invert.dev_attr.attr,
> +	&sensor_dev_attr_direction.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group pca9536_defattr_group = {
> +	.attrs = pca9536_attributes,
> +};

I'm not sure what this sysfs interface is good for given the use case
of this driver. It might even be dangerous to have, users may damage
their hardware?

> +
> +static int pca9536_probe(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		pr_debug("%s: I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n", __FUNCTION__);

Please use dev_dbg().

> +		return -EIO;
> +	}
> +
> +	if (client->dev.platform_data)
> +		/* This chip is going to be used from the kernel */
> +		*(struct i2c_client **)client->dev.platform_data = client;

This doesn't make any sense to me - what are you doing?

> +
> +	/* Register sysfs hooks */
> +	return sysfs_create_group(&client->dev.kobj,
> +				  &pca9536_defattr_group);
> +}
> +
> +static int pca9536_remove(struct i2c_client *client)
> +{
> +	sysfs_remove_group(&client->dev.kobj, &pca9536_defattr_group);
> +	return 0;
> +}
> +
> +static struct i2c_driver pca9536_i2c_driver = {
> +	.driver = {
> +		.name = "pca9536",
> +	},
> +	.probe		= pca9536_probe,
> +	.remove		= pca9536_remove,
> +};
> +
> +static int __init pca9536_mod_init(void)
> +{
> +	return i2c_add_driver(&pca9536_i2c_driver);
> +}
> +
> +static void __exit pca9536_mod_exit(void)
> +{
> +	i2c_del_driver(&pca9536_i2c_driver);
> +}
> +
> +module_init(pca9536_mod_init);
> +module_exit(pca9536_mod_exit);
> +
> +MODULE_DESCRIPTION("PCA9536 driver");
> +MODULE_AUTHOR("Guennadi Liakhovetski <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> +MODULE_LICENSE("GPL");


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [RFC PATCH 3/8] Philips PCA9536 4 bit I2C GPIO extender driver

[Guennadi Liakhovetski]

On Sun, 27 Jan 2008, Jean Delvare wrote:

> Hi Guennadi,
> 
> On Wed, 23 Jan 2008 18:41:39 +0100 (CET), Guennadi Liakhovetski wrote:
> > This driver will be used by the mt9m001 and the mt9v022 camera drivers,
> > that can use a pca9536 to switch between 8 and 10 bit modes.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>
> > ---
> >  drivers/i2c/chips/Kconfig   |   10 +++
> >  drivers/i2c/chips/Makefile  |    1 +
> >  drivers/i2c/chips/pca9536.c |  153 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 164 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/i2c/chips/pca9536.c
> 
> There is a new GPIO subsystem (under driver/gpio), that's where you
> should add this driver.

Right, thanks, will move there, as well as will fix all your clean up 
suggestions - unneeded headers, dev_dbg, actually, might even remove 
sysfs attributes - they were there only for debugging.

> > +	if (client->dev.platform_data)
> > +		/* This chip is going to be used from the kernel */
> > +		*(struct i2c_client **)client->dev.platform_data = client;
> 
> This doesn't make any sense to me - what are you doing?

This was my way to call 

int pca9536_set_level(struct i2c_client *client, u8 pin, u8 level)
{
...
}
EXPORT_SYMBOL(pca9536_set_level);

from video drivers. But, if I make it a GPIO driver, I'll get a gpio API 
too, so, will not have to export a new function, right (haven't looked at 
gpio API yet)? But, probably, I still will need to somehow pass a handle 
to this gpio-controller to the video driver. The idea is:

in platform file we know, that a specific GPIO controller is attached to a 
specific camera. So, I do

static struct i2c_client *camera_data_bus_switch;

static struct soc_camera_link iclink[] = {
	{
		.bus_id		= 0, /* Must match with the camera ID above */
		.extender	= &camera_data_bus_switch,
	}, {
		.bus_id		= 0, /* Must match with the camera ID above */
	}
};

/* Board I2C devices. */
static struct i2c_board_info __initdata pcm027_i2c_devices[] = {
	{
		/* Must initialize before the camera(s) */
		I2C_BOARD_INFO("pca9536", 0x41),
		.type = "pca9536",
		.platform_data = &camera_data_bus_switch,
	}, {
		I2C_BOARD_INFO("mt9v022", 0x48),
		.type = "mt9v022",
		.platform_data = &iclink[0], /* With extender */
	}, {
		I2C_BOARD_INFO("mt9m001", 0x5d),
		.type = "mt9m001",
		.platform_data = &iclink[0], /* With extender */
	},
};

this way I tell, that the pca9536 extender with i2c address 0x41 is used 
to switch both cameras. And in the pca9536 driver I fill in the 
camera_data_bus_switch variable, so that cameras can later find and use 
the switch.

Well, I haven't find a better way to do this.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Philips PCA9536 4 bit I2C GPIO extender driver

[David Brownell]

On Sunday 27 January 2008, Jean Delvare wrote:
> 
> There is a new GPIO subsystem (under driver/gpio), that's where you
> should add this driver.

That should merge soonish into kernel GIT -- Linus is merging up a
storm before LCA starts, but Andrew's not sent it yet -- but it's
easy to grab from current MM.

There's a pca9539 driver there, and ISTR the 9536 is very similar
except it has no IRQ support and only has 8-bit registers.  You
should be able to reuse its platform_data structure.  It has a
registration callback that should let you establish the linkage to
the camera(s) you want to control.

I suspect you'll come across one other issue... one potential fix
being to add a mechanism matching a NOTE in the gpiolib sources.

I think you should consider making one driver that can handle three
chips: the 4-bit pca9536 (8 pins) and pca9537 (10 pins -- add irq and
reset), plus their 8-bit pca9538 sibling (16 pins -- four more GPIOs,
and two address bits).  Their register model is *identical* ... all
that would mean is understanding that for some chips the high nibble
is actually valid.  (And eventually, maybe, that some chips support
input-changed IRQs that can help avoid synchronous register reads.
If the i2c_client doesn't list an IRQ that's of course not an issue.)

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[Guennadi Liakhovetski]

PCA9539 and PCA9536 have identical register layout, and differ only in the 
number of GPIOs and, respectively, register size. Extend the pca9539 
driver to also support PCA9536. The driver can further be extended in the 
future to also support PCA953[4578] chips.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>

---

On Sun, 27 Jan 2008, David Brownell wrote:

> On Sunday 27 January 2008, Jean Delvare wrote:
> > 
> > There is a new GPIO subsystem (under driver/gpio), that's where you
> > should add this driver.
> 
> That should merge soonish into kernel GIT -- Linus is merging up a
> storm before LCA starts, but Andrew's not sent it yet -- but it's
> easy to grab from current MM.

Hope it does, as now my video patches depend on those.

> There's a pca9539 driver there, and ISTR the 9536 is very similar
> except it has no IRQ support and only has 8-bit registers.  You
> should be able to reuse its platform_data structure.  It has a
> registration callback that should let you establish the linkage to
> the camera(s) you want to control.

I didn't need anything like that. I just used the platform knowledge of 
the GPIO number. This way the camera driver just obtains the GPIO, 
requests and uses it.

> I suspect you'll come across one other issue... one potential fix
> being to add a mechanism matching a NOTE in the gpiolib sources.

You mean gpio numbers? See above - platform code knows, that it's GPIO 1 
on the extender, that the cameras need.

> I think you should consider making one driver that can handle three
> chips: the 4-bit pca9536 (8 pins) and pca9537 (10 pins -- add irq and
> reset), plus their 8-bit pca9538 sibling (16 pins -- four more GPIOs,
> and two address bits).  Their register model is *identical* ... all
> that would mean is understanding that for some chips the high nibble
> is actually valid.  (And eventually, maybe, that some chips support
> input-changed IRQs that can help avoid synchronous register reads.
> If the i2c_client doesn't list an IRQ that's of course not an issue.)

I think, there are more than 3. But I'd prefer staying with the two at the 
moment - others can be added later. And I haven't found in the driver 
support for IRQ and reset pins, so, no differentiation is made for those 
either.

Note, the patch below uses git-specific "renamed from/to" tags, so, it 
won't work with GNU patch. But it is easier to review it this way.

Thanks
Guennadi


diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 74fac0f..713c0fe 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -27,15 +27,15 @@ config DEBUG_GPIO
 
 comment "I2C GPIO expanders:"
 
-config GPIO_PCA9539
-	tristate "PCA9539 16-bit I/O port"
+config GPIO_PCA953X
+	tristate "PCA953X I/O port"
 	depends on I2C
 	help
-	  Say yes here to support the PCA9539 16-bit I/O port. These
-	  parts are made by NXP and TI.
+	  Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.
+	  These parts are made by NXP and TI.
 
 	  This driver can also be built as a module.  If so, the module
-	  will be called pca9539.
+	  will be called pca953x.
 
 config GPIO_PCF857X
 	tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 470ecd6..fdde992 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -5,5 +5,5 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 obj-$(CONFIG_HAVE_GPIO_LIB)	+= gpiolib.o
 
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
-obj-$(CONFIG_GPIO_PCA9539)	+= pca9539.o
+obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca953x.c
similarity index 67%
rename from drivers/gpio/pca9539.c
rename to drivers/gpio/pca953x.c
index 3e85c92..3e9d650 100644
@@ -1,5 +1,5 @@
 /*
- *  pca9539.c - 16-bit I/O port with interrupt and reset
+ *  pca953x.c - I/O port with interrupt and reset
  *
  *  Copyright (C) 2005 Ben Gardner <bgardner@wabtec.com>
  *  Copyright (C) 2007 Marvell International Ltd.
@@ -14,19 +14,42 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
-#include <linux/i2c/pca9539.h>
+#include <linux/i2c/pca953x.h>
 
 #include <asm/gpio.h>
 
+enum {
+	PCA9536,
+	PCA9539,
+};
 
-#define NR_PCA9539_GPIOS	16
+struct pca953x_desc {
+	unsigned int gpios;
+	unsigned int input;
+	unsigned int output;
+	unsigned int invert;
+	unsigned int direction;
+};
 
-#define PCA9539_INPUT		0
-#define PCA9539_OUTPUT		2
-#define PCA9539_INVERT		4
-#define PCA9539_DIRECTION	6
+static const struct pca953x_desc pca953x_descs[] = {
+	[PCA9536] = {
+		.gpios		= 4,
+		.input		= 0,
+		.output		= 1,
+		.invert		= 2,
+		.direction	= 3,
+	},
+	[PCA9539] = {
+		.gpios		= 16,
+		.input		= 0,
+		.output		= 2,
+		.invert		= 4,
+		.direction	= 6,
+	},
+};
 
-struct pca9539_chip {
+struct pca953x_chip {
+	struct pca953x_desc const *desc;
 	unsigned gpio_start;
 	uint16_t reg_output;
 	uint16_t reg_direction;
@@ -38,19 +61,32 @@ struct pca9539_chip {
 /* NOTE:  we can't currently rely on fault codes to come from SMBus
  * calls, so we map all errors to EIO here and return zero otherwise.
  */
-static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
+static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint16_t val)
 {
-	if (i2c_smbus_write_word_data(chip->client, reg, val) < 0)
-		return -EIO;
+	int ret;
+
+	if (chip->desc->gpios <= 8)
+		ret = i2c_smbus_write_byte_data(chip->client, reg, val);
 	else
-		return 0;
+		ret = i2c_smbus_write_word_data(chip->client, reg, val);
+
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "failed writing register\n");
+		return -EIO;
+	}
+
+	return 0;
 }
 
-static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
+static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint16_t *val)
 {
 	int ret;
 
-	ret = i2c_smbus_read_word_data(chip->client, reg);
+	if (chip->desc->gpios <= 8)
+		ret = i2c_smbus_read_byte_data(chip->client, reg);
+	else
+		ret = i2c_smbus_read_word_data(chip->client, reg);
+
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return -EIO;
@@ -60,16 +96,16 @@ static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
 	return 0;
 }
 
-static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
-	struct pca9539_chip *chip;
+	struct pca953x_chip *chip;
 	uint16_t reg_val;
 	int ret;
 
-	chip = container_of(gc, struct pca9539_chip, gpio_chip);
+	chip = container_of(gc, struct pca953x_chip, gpio_chip);
 
 	reg_val = chip->reg_direction | (1u << off);
-	ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+	ret = pca953x_write_reg(chip, chip->desc->direction, reg_val);
 	if (ret)
 		return ret;
 
@@ -77,14 +113,14 @@ static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 	return 0;
 }
 
-static int pca9539_gpio_direction_output(struct gpio_chip *gc,
+static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 		unsigned off, int val)
 {
-	struct pca9539_chip *chip;
+	struct pca953x_chip *chip;
 	uint16_t reg_val;
 	int ret;
 
-	chip = container_of(gc, struct pca9539_chip, gpio_chip);
+	chip = container_of(gc, struct pca953x_chip, gpio_chip);
 
 	/* set output level */
 	if (val)
@@ -92,7 +128,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
 	else
 		reg_val = chip->reg_output & ~(1u << off);
 
-	ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
+	ret = pca953x_write_reg(chip, chip->desc->output, reg_val);
 	if (ret)
 		return ret;
 
@@ -100,7 +136,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
 
 	/* then direction */
 	reg_val = chip->reg_direction & ~(1u << off);
-	ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
+	ret = pca953x_write_reg(chip, chip->desc->direction, reg_val);
 	if (ret)
 		return ret;
 
@@ -108,15 +144,15 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
 	return 0;
 }
 
-static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
+static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 {
-	struct pca9539_chip *chip;
+	struct pca953x_chip *chip;
 	uint16_t reg_val;
 	int ret;
 
-	chip = container_of(gc, struct pca9539_chip, gpio_chip);
+	chip = container_of(gc, struct pca953x_chip, gpio_chip);
 
-	ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
+	ret = pca953x_read_reg(chip, chip->desc->input, &reg_val);
 	if (ret < 0) {
 		/* NOTE:  diagnostic already emitted; that's all we should
 		 * do unless gpio_*_value_cansleep() calls become different
@@ -128,58 +164,67 @@ static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
 	return (reg_val & (1u << off)) ? 1 : 0;
 }
 
-static void pca9539_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
+static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 {
-	struct pca9539_chip *chip;
+	struct pca953x_chip *chip;
 	uint16_t reg_val;
 	int ret;
 
-	chip = container_of(gc, struct pca9539_chip, gpio_chip);
+	chip = container_of(gc, struct pca953x_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);
+	ret = pca953x_write_reg(chip, chip->desc->output, reg_val);
 	if (ret)
 		return;
 
 	chip->reg_output = reg_val;
 }
 
-static int pca9539_init_gpio(struct pca9539_chip *chip)
+static int pca953x_init_gpio(struct pca953x_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->direction_input  = pca953x_gpio_direction_input;
+	gc->direction_output = pca953x_gpio_direction_output;
+	gc->get = pca953x_gpio_get_value;
+	gc->set = pca953x_gpio_set_value;
 
 	gc->base = chip->gpio_start;
-	gc->ngpio = NR_PCA9539_GPIOS;
-	gc->label = "pca9539";
+	gc->ngpio = chip->desc->gpios;
+	gc->label = "pca953x";
 
 	return gpiochip_add(gc);
 }
 
-static int __devinit pca9539_probe(struct i2c_client *client)
+static int __devinit pca953x_probe(struct i2c_client *client)
 {
-	struct pca9539_platform_data *pdata;
-	struct pca9539_chip *chip;
+	struct pca953x_platform_data *pdata;
+	struct pca953x_chip *chip;
 	int ret;
+	int variant;
 
 	pdata = client->dev.platform_data;
 	if (pdata == NULL)
 		return -ENODEV;
 
-	chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
+	if (!strcmp("pca9539", pdata->name))
+		variant = PCA9539;
+	else if (!strcmp("pca9536", pdata->name))
+		variant = PCA9536;
+	else
+		return -ENODEV;
+
+	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
 	if (chip == NULL)
 		return -ENOMEM;
 
+	chip->desc = &pca953x_descs[variant];
 	chip->client = client;
 
 	chip->gpio_start = pdata->gpio_base;
@@ -187,20 +232,20 @@ static int __devinit pca9539_probe(struct i2c_client *client)
 	/* initialize cached registers from their original values.
 	 * we can't share this chip with another i2c master.
 	 */
-	ret = pca9539_read_reg(chip, PCA9539_OUTPUT, &chip->reg_output);
+	ret = pca953x_read_reg(chip, chip->desc->output, &chip->reg_output);
 	if (ret)
 		goto out_failed;
 
-	ret = pca9539_read_reg(chip, PCA9539_DIRECTION, &chip->reg_direction);
+	ret = pca953x_read_reg(chip, chip->desc->direction, &chip->reg_direction);
 	if (ret)
 		goto out_failed;
 
 	/* set platform specific polarity inversion */
-	ret = pca9539_write_reg(chip, PCA9539_INVERT, pdata->invert);
+	ret = pca953x_write_reg(chip, chip->desc->invert, pdata->invert);
 	if (ret)
 		goto out_failed;
 
-	ret = pca9539_init_gpio(chip);
+	ret = pca953x_init_gpio(chip);
 	if (ret)
 		goto out_failed;
 
@@ -219,10 +264,10 @@ out_failed:
 	return ret;
 }
 
-static int pca9539_remove(struct i2c_client *client)
+static int pca953x_remove(struct i2c_client *client)
 {
-	struct pca9539_platform_data *pdata = client->dev.platform_data;
-	struct pca9539_chip *chip = i2c_get_clientdata(client);
+	struct pca953x_platform_data *pdata = client->dev.platform_data;
+	struct pca953x_chip *chip = i2c_get_clientdata(client);
 	int ret = 0;
 
 	if (pdata->teardown) {
@@ -246,26 +291,26 @@ static int pca9539_remove(struct i2c_client *client)
 	return 0;
 }
 
-static struct i2c_driver pca9539_driver = {
+static struct i2c_driver pca953x_driver = {
 	.driver = {
-		.name	= "pca9539",
+		.name	= "pca953x",
 	},
-	.probe		= pca9539_probe,
-	.remove		= pca9539_remove,
+	.probe		= pca953x_probe,
+	.remove		= pca953x_remove,
 };
 
-static int __init pca9539_init(void)
+static int __init pca953x_init(void)
 {
-	return i2c_add_driver(&pca9539_driver);
+	return i2c_add_driver(&pca953x_driver);
 }
-module_init(pca9539_init);
+module_init(pca953x_init);
 
-static void __exit pca9539_exit(void)
+static void __exit pca953x_exit(void)
 {
-	i2c_del_driver(&pca9539_driver);
+	i2c_del_driver(&pca953x_driver);
 }
-module_exit(pca9539_exit);
+module_exit(pca953x_exit);
 
 MODULE_AUTHOR("eric miao <eric.miao@marvell.com>");
-MODULE_DESCRIPTION("GPIO expander driver for PCA9539");
+MODULE_DESCRIPTION("GPIO expander driver for PCA953X");
 MODULE_LICENSE("GPL");
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index f29a502..806b86c 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -16,6 +16,10 @@
 #define ARCH_NR_GPIOS		256
 #endif
 
+#ifndef NO_GPIO
+#define NO_GPIO			((unsigned int)-1)
+#endif
+
 struct seq_file;
 
 /**
diff --git a/include/linux/i2c/pca9539.h b/include/linux/i2c/pca953x.h
similarity index 67%
rename from include/linux/i2c/pca9539.h
rename to include/linux/i2c/pca953x.h
index 611d84a..8977e57 100644
--- a/include/linux/i2c/pca9539.h
+++ b/include/linux/i2c/pca953x.h
@@ -1,9 +1,12 @@
-/* platform data for the PCA9539 16-bit I/O expander driver */
+/* platform data for the PCA953x I/O expander driver */
 
-struct pca9539_platform_data {
+struct pca953x_platform_data {
 	/* number of the first GPIO */
 	unsigned	gpio_base;
 
+	/* chip variant. Currently supported "pca9536" and "pca9539" */
+	const char	*name;
+
 	/* initial polarity inversion setting */
 	uint16_t	invert;

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[eric miao]

I like this patch, overall looks ok. See my comments below.


On Jan 31, 2008 12:48 AM, Guennadi Liakhovetski
<g.liakhovetski@pengutronix.de> wrote:
> PCA9539 and PCA9536 have identical register layout, and differ only in the
> number of GPIOs and, respectively, register size. Extend the pca9539
> driver to also support PCA9536. The driver can further be extended in the
> future to also support PCA953[4578] chips.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>
>
> ---
>
> On Sun, 27 Jan 2008, David Brownell wrote:
>
> > On Sunday 27 January 2008, Jean Delvare wrote:
> > >
> > > There is a new GPIO subsystem (under driver/gpio), that's where you
> > > should add this driver.
> >
> > That should merge soonish into kernel GIT -- Linus is merging up a
> > storm before LCA starts, but Andrew's not sent it yet -- but it's
> > easy to grab from current MM.
>
> Hope it does, as now my video patches depend on those.
>
> > There's a pca9539 driver there, and ISTR the 9536 is very similar
> > except it has no IRQ support and only has 8-bit registers.  You
> > should be able to reuse its platform_data structure.  It has a
> > registration callback that should let you establish the linkage to
> > the camera(s) you want to control.
>
> I didn't need anything like that. I just used the platform knowledge of
> the GPIO number. This way the camera driver just obtains the GPIO,
> requests and uses it.
>
> > I suspect you'll come across one other issue... one potential fix
> > being to add a mechanism matching a NOTE in the gpiolib sources.
>
> You mean gpio numbers? See above - platform code knows, that it's GPIO 1
> on the extender, that the cameras need.
>
> > I think you should consider making one driver that can handle three
> > chips: the 4-bit pca9536 (8 pins) and pca9537 (10 pins -- add irq and
> > reset), plus their 8-bit pca9538 sibling (16 pins -- four more GPIOs,
> > and two address bits).  Their register model is *identical* ... all
> > that would mean is understanding that for some chips the high nibble
> > is actually valid.  (And eventually, maybe, that some chips support
> > input-changed IRQs that can help avoid synchronous register reads.
> > If the i2c_client doesn't list an IRQ that's of course not an issue.)
>
> I think, there are more than 3. But I'd prefer staying with the two at the
> moment - others can be added later. And I haven't found in the driver
> support for IRQ and reset pins, so, no differentiation is made for those
> either.
>
> Note, the patch below uses git-specific "renamed from/to" tags, so, it
> won't work with GNU patch. But it is easier to review it this way.
>
> Thanks
> Guennadi
>
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 74fac0f..713c0fe 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -27,15 +27,15 @@ config DEBUG_GPIO
>
>  comment "I2C GPIO expanders:"
>
> -config GPIO_PCA9539
> -       tristate "PCA9539 16-bit I/O port"
> +config GPIO_PCA953X
> +       tristate "PCA953X I/O port"
>         depends on I2C
>         help
> -         Say yes here to support the PCA9539 16-bit I/O port. These
> -         parts are made by NXP and TI.
> +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.
> +         These parts are made by NXP and TI.
>
>           This driver can also be built as a module.  If so, the module
> -         will be called pca9539.
> +         will be called pca953x.
>
>  config GPIO_PCF857X
>         tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 470ecd6..fdde992 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -5,5 +5,5 @@ ccflags-$(CONFIG_DEBUG_GPIO)    += -DDEBUG
>  obj-$(CONFIG_HAVE_GPIO_LIB)    += gpiolib.o
>
>  obj-$(CONFIG_GPIO_MCP23S08)    += mcp23s08.o
> -obj-$(CONFIG_GPIO_PCA9539)     += pca9539.o
> +obj-$(CONFIG_GPIO_PCA953X)     += pca953x.o
>  obj-$(CONFIG_GPIO_PCF857X)     += pcf857x.o
> diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca953x.c
> similarity index 67%
> rename from drivers/gpio/pca9539.c
> rename to drivers/gpio/pca953x.c
> index 3e85c92..3e9d650 100644
> @@ -1,5 +1,5 @@
>  /*
> - *  pca9539.c - 16-bit I/O port with interrupt and reset
> + *  pca953x.c - I/O port with interrupt and reset
>   *
>   *  Copyright (C) 2005 Ben Gardner <bgardner@wabtec.com>
>   *  Copyright (C) 2007 Marvell International Ltd.
> @@ -14,19 +14,42 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/i2c.h>
> -#include <linux/i2c/pca9539.h>
> +#include <linux/i2c/pca953x.h>
>
>  #include <asm/gpio.h>
>
> +enum {
> +       PCA9536,
> +       PCA9539,
> +};
>
> -#define NR_PCA9539_GPIOS       16
> +struct pca953x_desc {
> +       unsigned int gpios;
> +       unsigned int input;
> +       unsigned int output;
> +       unsigned int invert;
> +       unsigned int direction;
> +};

I guess the register address can be inferred either from the number
of GPIOs or a single member field of "shift" may be sufficient.

>
> -#define PCA9539_INPUT          0
> -#define PCA9539_OUTPUT         2
> -#define PCA9539_INVERT         4
> -#define PCA9539_DIRECTION      6
> +static const struct pca953x_desc pca953x_descs[] = {
> +       [PCA9536] = {
> +               .gpios          = 4,
> +               .input          = 0,
> +               .output         = 1,
> +               .invert         = 2,
> +               .direction      = 3,
> +       },
> +       [PCA9539] = {
> +               .gpios          = 16,
> +               .input          = 0,
> +               .output         = 2,
> +               .invert         = 4,
> +               .direction      = 6,
> +       },
> +};
>
> -struct pca9539_chip {
> +struct pca953x_chip {
> +       struct pca953x_desc const *desc;
>         unsigned gpio_start;
>         uint16_t reg_output;
>         uint16_t reg_direction;
> @@ -38,19 +61,32 @@ struct pca9539_chip {
>  /* NOTE:  we can't currently rely on fault codes to come from SMBus
>   * calls, so we map all errors to EIO here and return zero otherwise.
>   */
> -static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
> +static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint16_t val)
>  {
> -       if (i2c_smbus_write_word_data(chip->client, reg, val) < 0)
> -               return -EIO;
> +       int ret;
> +
> +       if (chip->desc->gpios <= 8)
> +               ret = i2c_smbus_write_byte_data(chip->client, reg, val);
>         else
> -               return 0;
> +               ret = i2c_smbus_write_word_data(chip->client, reg, val);
> +
> +       if (ret < 0) {
> +               dev_err(&chip->client->dev, "failed writing register\n");
> +               return -EIO;
> +       }
> +
> +       return 0;
>  }
>
> -static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
> +static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint16_t *val)
>  {
>         int ret;
>
> -       ret = i2c_smbus_read_word_data(chip->client, reg);
> +       if (chip->desc->gpios <= 8)
> +               ret = i2c_smbus_read_byte_data(chip->client, reg);
> +       else
> +               ret = i2c_smbus_read_word_data(chip->client, reg);
> +
>         if (ret < 0) {
>                 dev_err(&chip->client->dev, "failed reading register\n");
>                 return -EIO;
> @@ -60,16 +96,16 @@ static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val)
>         return 0;
>  }
>
> -static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
> +static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
>  {
> -       struct pca9539_chip *chip;
> +       struct pca953x_chip *chip;
>         uint16_t reg_val;
>         int ret;
>
> -       chip = container_of(gc, struct pca9539_chip, gpio_chip);
> +       chip = container_of(gc, struct pca953x_chip, gpio_chip);
>
>         reg_val = chip->reg_direction | (1u << off);
> -       ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
> +       ret = pca953x_write_reg(chip, chip->desc->direction, reg_val);
>         if (ret)
>                 return ret;
>
> @@ -77,14 +113,14 @@ static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off)
>         return 0;
>  }
>
> -static int pca9539_gpio_direction_output(struct gpio_chip *gc,
> +static int pca953x_gpio_direction_output(struct gpio_chip *gc,
>                 unsigned off, int val)
>  {
> -       struct pca9539_chip *chip;
> +       struct pca953x_chip *chip;
>         uint16_t reg_val;
>         int ret;
>
> -       chip = container_of(gc, struct pca9539_chip, gpio_chip);
> +       chip = container_of(gc, struct pca953x_chip, gpio_chip);
>
>         /* set output level */
>         if (val)
> @@ -92,7 +128,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
>         else
>                 reg_val = chip->reg_output & ~(1u << off);
>
> -       ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val);
> +       ret = pca953x_write_reg(chip, chip->desc->output, reg_val);
>         if (ret)
>                 return ret;
>
> @@ -100,7 +136,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
>
>         /* then direction */
>         reg_val = chip->reg_direction & ~(1u << off);
> -       ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val);
> +       ret = pca953x_write_reg(chip, chip->desc->direction, reg_val);
>         if (ret)
>                 return ret;
>
> @@ -108,15 +144,15 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc,
>         return 0;
>  }
>
> -static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
> +static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
>  {
> -       struct pca9539_chip *chip;
> +       struct pca953x_chip *chip;
>         uint16_t reg_val;
>         int ret;
>
> -       chip = container_of(gc, struct pca9539_chip, gpio_chip);
> +       chip = container_of(gc, struct pca953x_chip, gpio_chip);
>
> -       ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
> +       ret = pca953x_read_reg(chip, chip->desc->input, &reg_val);
>         if (ret < 0) {
>                 /* NOTE:  diagnostic already emitted; that's all we should
>                  * do unless gpio_*_value_cansleep() calls become different
> @@ -128,58 +164,67 @@ static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
>         return (reg_val & (1u << off)) ? 1 : 0;
>  }
>
> -static void pca9539_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> +static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
>  {
> -       struct pca9539_chip *chip;
> +       struct pca953x_chip *chip;
>         uint16_t reg_val;
>         int ret;
>
> -       chip = container_of(gc, struct pca9539_chip, gpio_chip);
> +       chip = container_of(gc, struct pca953x_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);
> +       ret = pca953x_write_reg(chip, chip->desc->output, reg_val);
>         if (ret)
>                 return;
>
>         chip->reg_output = reg_val;
>  }
>
> -static int pca9539_init_gpio(struct pca9539_chip *chip)
> +static int pca953x_init_gpio(struct pca953x_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->direction_input  = pca953x_gpio_direction_input;
> +       gc->direction_output = pca953x_gpio_direction_output;
> +       gc->get = pca953x_gpio_get_value;
> +       gc->set = pca953x_gpio_set_value;
>
>         gc->base = chip->gpio_start;
> -       gc->ngpio = NR_PCA9539_GPIOS;
> -       gc->label = "pca9539";
> +       gc->ngpio = chip->desc->gpios;
> +       gc->label = "pca953x";
>
>         return gpiochip_add(gc);
>  }
>
> -static int __devinit pca9539_probe(struct i2c_client *client)
> +static int __devinit pca953x_probe(struct i2c_client *client)
>  {
> -       struct pca9539_platform_data *pdata;
> -       struct pca9539_chip *chip;
> +       struct pca953x_platform_data *pdata;
> +       struct pca953x_chip *chip;
>         int ret;
> +       int variant;
>
>         pdata = client->dev.platform_data;
>         if (pdata == NULL)
>                 return -ENODEV;
>
> -       chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
> +       if (!strcmp("pca9539", pdata->name))
> +               variant = PCA9539;
> +       else if (!strcmp("pca9536", pdata->name))
> +               variant = PCA9536;
> +       else
> +               return -ENODEV;
> +

I prefer integer here more than string, for simplicity and efficiency.
What about:

switch (pdata->model) {
case 9536:
    chip->num_gpio = 8;
case 9539:
    chip->num_gpio = 16;
default:
    warning - not supported.
}

And maybe the chip->num_gpio can serve as the "shift" to calculate
the register offset, thus avoiding the introducing of "pca953x_desc"
structure at all.

> +       chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
>         if (chip == NULL)
>                 return -ENOMEM;
>
> +       chip->desc = &pca953x_descs[variant];
>         chip->client = client;
>
>         chip->gpio_start = pdata->gpio_base;
> @@ -187,20 +232,20 @@ static int __devinit pca9539_probe(struct i2c_client *client)
>         /* initialize cached registers from their original values.
>          * we can't share this chip with another i2c master.
>          */
> -       ret = pca9539_read_reg(chip, PCA9539_OUTPUT, &chip->reg_output);
> +       ret = pca953x_read_reg(chip, chip->desc->output, &chip->reg_output);
>         if (ret)
>                 goto out_failed;
>
> -       ret = pca9539_read_reg(chip, PCA9539_DIRECTION, &chip->reg_direction);
> +       ret = pca953x_read_reg(chip, chip->desc->direction, &chip->reg_direction);
>         if (ret)
>                 goto out_failed;
>
>         /* set platform specific polarity inversion */
> -       ret = pca9539_write_reg(chip, PCA9539_INVERT, pdata->invert);
> +       ret = pca953x_write_reg(chip, chip->desc->invert, pdata->invert);
>         if (ret)
>                 goto out_failed;
>
> -       ret = pca9539_init_gpio(chip);
> +       ret = pca953x_init_gpio(chip);
>         if (ret)
>                 goto out_failed;
>
> @@ -219,10 +264,10 @@ out_failed:
>         return ret;
>  }
>
> -static int pca9539_remove(struct i2c_client *client)
> +static int pca953x_remove(struct i2c_client *client)
>  {
> -       struct pca9539_platform_data *pdata = client->dev.platform_data;
> -       struct pca9539_chip *chip = i2c_get_clientdata(client);
> +       struct pca953x_platform_data *pdata = client->dev.platform_data;
> +       struct pca953x_chip *chip = i2c_get_clientdata(client);
>         int ret = 0;
>
>         if (pdata->teardown) {
> @@ -246,26 +291,26 @@ static int pca9539_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -static struct i2c_driver pca9539_driver = {
> +static struct i2c_driver pca953x_driver = {
>         .driver = {
> -               .name   = "pca9539",
> +               .name   = "pca953x",
>         },
> -       .probe          = pca9539_probe,
> -       .remove         = pca9539_remove,
> +       .probe          = pca953x_probe,
> +       .remove         = pca953x_remove,
>  };
>
> -static int __init pca9539_init(void)
> +static int __init pca953x_init(void)
>  {
> -       return i2c_add_driver(&pca9539_driver);
> +       return i2c_add_driver(&pca953x_driver);
>  }
> -module_init(pca9539_init);
> +module_init(pca953x_init);
>
> -static void __exit pca9539_exit(void)
> +static void __exit pca953x_exit(void)
>  {
> -       i2c_del_driver(&pca9539_driver);
> +       i2c_del_driver(&pca953x_driver);
>  }
> -module_exit(pca9539_exit);
> +module_exit(pca953x_exit);
>
>  MODULE_AUTHOR("eric miao <eric.miao@marvell.com>");
> -MODULE_DESCRIPTION("GPIO expander driver for PCA9539");
> +MODULE_DESCRIPTION("GPIO expander driver for PCA953X");
>  MODULE_LICENSE("GPL");
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index f29a502..806b86c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -16,6 +16,10 @@
>  #define ARCH_NR_GPIOS          256
>  #endif
>
> +#ifndef NO_GPIO
> +#define NO_GPIO                        ((unsigned int)-1)
> +#endif
> +

I don't understand this.

>  struct seq_file;
>
>  /**
> diff --git a/include/linux/i2c/pca9539.h b/include/linux/i2c/pca953x.h
> similarity index 67%
> rename from include/linux/i2c/pca9539.h
> rename to include/linux/i2c/pca953x.h
> index 611d84a..8977e57 100644
> --- a/include/linux/i2c/pca9539.h
> +++ b/include/linux/i2c/pca953x.h
> @@ -1,9 +1,12 @@
> -/* platform data for the PCA9539 16-bit I/O expander driver */
> +/* platform data for the PCA953x I/O expander driver */
>
> -struct pca9539_platform_data {
> +struct pca953x_platform_data {
>         /* number of the first GPIO */
>         unsigned        gpio_base;
>
> +       /* chip variant. Currently supported "pca9536" and "pca9539" */
> +       const char      *name;
> +
>         /* initial polarity inversion setting */
>         uint16_t        invert;
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>



-- 
Cheers
- eric

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell]

On Wednesday 30 January 2008, eric miao wrote:
> I like this patch, overall looks ok. See my comments below.

I basically like it, but I'd still like to see some changes.  :)

I'd split this patch into two parts (feature addition, and renaming),
and use simpler chip type handling.  Re that latter, in 2.6.26 there
will be an i2c_device_id struct:

  struct i2c_device_id {
        char name[I2C_NAME_SIZE];
        kernel_ulong_t driver_data;     /* Data private to the driver */
  };

A little planning done now will make it easy to use that scheme.
First, don't add a type field to the platform_data; that's what
the i2c_client.name is for.  Then just map those strings to the
number of GPIOs (eventually just use driver_data), and everything
else follows from there.


> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -27,15 +27,15 @@ config DEBUG_GPIO
> >
> >  comment "I2C GPIO expanders:"
> >
> > -config GPIO_PCA9539
> > -       tristate "PCA9539 16-bit I/O port"
> > +config GPIO_PCA953X
> > +       tristate "PCA953X I/O port"

"Ports"

> >         depends on I2C
> >         help
> > -         Say yes here to support the PCA9539 16-bit I/O port. These
> > -         parts are made by NXP and TI.
> > +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.

This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
Same register layout as the '36 (4-bit).  Otherwise the "x" seems
misleading...


> > diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca953x.c
> > similarity index 67%
> > rename from drivers/gpio/pca9539.c
> > rename to drivers/gpio/pca953x.c

I'd prefer to see the new feature and the renaming be separate patches.


> > -#define NR_PCA9539_GPIOS       16
> > +struct pca953x_desc {
> > +       unsigned int gpios;
> > +       unsigned int input;
> > +       unsigned int output;
> > +       unsigned int invert;
> > +       unsigned int direction;
> > +};
> 
> I guess the register address can be inferred either from the number
> of GPIOs or a single member field of "shift" may be sufficient.

Yes, inferring that would be simpler.  The number of GPIOs is a
function of the particular chip, and register offsets are 0/1/2/3
except for the 16-GPIO part where they're twice that.

What I'd do is save those four offsets directly in pca953x_chip,
initialized near when gpio_chip.ngpio is set up and with no need
for a separate "desc" type or table.  Eventually there'd be:

	struct i2c_device_id chips [] = {
		{ "pca9536", 4, },
		{ "pca9537", 4, },
		{ "pca9538", 8, },
		{ "pca9539", 16, },
	};
	MODULE_DEVICE_TABLE(i2c, chips);

Meanwhile, the equivalent of that table can come from a few strcmp()
tests in the probe() logic -- like what you already have, except
not using a "desc" type.


> > -static int pca9539_init_gpio(struct pca9539_chip *chip)
> > +static int pca953x_init_gpio(struct pca953x_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->direction_input  = pca953x_gpio_direction_input;
> > +       gc->direction_output = pca953x_gpio_direction_output;
> > +       gc->get = pca953x_gpio_get_value;
> > +       gc->set = pca953x_gpio_set_value;
> >
> >         gc->base = chip->gpio_start;
> > -       gc->ngpio = NR_PCA9539_GPIOS;
> > -       gc->label = "pca9539";
> > +       gc->ngpio = chip->desc->gpios;

I'd expect "desc" to vanish, and ngpio to be an input to this function
(see below).  Then ngpio could be used to choose the values to stuff
into chip->{input,output,invert,direction} register offsets.


> > +       gc->label = "pca953x";

Why not just set gc->label to be chip->client->name?

Might as well have /sys/kernel/debug/gpio output be
that little bit more useful.  And save the space that
would otherwise be wasted by that string.  :)


> >
> >         return gpiochip_add(gc);
> >  }
> >
> > -static int __devinit pca9539_probe(struct i2c_client *client)
> > +static int __devinit pca953x_probe(struct i2c_client *client)
> >  {
> > -       struct pca9539_platform_data *pdata;
> > -       struct pca9539_chip *chip;
> > +       struct pca953x_platform_data *pdata;
> > +       struct pca953x_chip *chip;
> >         int ret;
> > +       int variant;
> >
> >         pdata = client->dev.platform_data;
> >         if (pdata == NULL)
> >                 return -ENODEV;
> >
> > -       chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL);
> > +       if (!strcmp("pca9539", pdata->name))

Use client->name instead; there's no need to change pdata.


> > +               variant = PCA9539;
> > +       else if (!strcmp("pca9536", pdata->name))
> > +               variant = PCA9536;
> > +       else
> > +               return -ENODEV;
> > +
> 
> I prefer integer here more than string, for simplicity and efficiency.

Actually I'd use strings right now, and not change pdata at all.  But
compare client->name, using that to figure out how many GPIOs.

When, later, an i2c_device_id is passed in, the driver can get the
number of GPIOs handed to it directly ... and won't need to worry
about that ENODEV case.



> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -16,6 +16,10 @@
> >  #define ARCH_NR_GPIOS          256
> >  #endif
> >
> > +#ifndef NO_GPIO
> > +#define NO_GPIO                        ((unsigned int)-1)
> > +#endif
> > +
> 
> I don't understand this.

Me either; *ANY* negative number is invalid as a GPIO number,
not just "-1"...


> > --- a/include/linux/i2c/pca9539.h
> > +++ b/include/linux/i2c/pca953x.h
> > @@ -1,9 +1,12 @@
> > -/* platform data for the PCA9539 16-bit I/O expander driver */
> > +/* platform data for the PCA953x I/O expander driver */
> >
> > -struct pca9539_platform_data {
> > +struct pca953x_platform_data {
> >         /* number of the first GPIO */
> >         unsigned        gpio_base;
> >
> > +       /* chip variant. Currently supported "pca9536" and "pca9539" */
> > +       const char      *name;
> > +

That's not needed; i2c_client.name should identify the chip.

> >         /* initial polarity inversion setting */
> >         uint16_t        invert;
> >
> > 

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[Guennadi Liakhovetski]

Thanks for all the comments, I'm going to address them in the next version 
of the patch. A couple of small clarifications first:

On Wed, 30 Jan 2008, David Brownell wrote:

> I'd split this patch into two parts (feature addition, and renaming),

Would you prefer

patch-1: git-mv && sed -e "s/pca9539/pca953x/"
patch-2: add pca953[678]

or

patch-1: git-mv
patch-2: sed -e "s/pca9539/pca953x/" && pca953[678]

or even

patch-1: git-mv
patch-2: sed -e "s/pca9539/pca953x/"
patch-3: pca953[678]

?

> > >         depends on I2C
> > >         help
> > > -         Say yes here to support the PCA9539 16-bit I/O port. These
> > > -         parts are made by NXP and TI.
> > > +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.
> 
> This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
> Same register layout as the '36 (4-bit).  Otherwise the "x" seems
> misleading...

Ok, I don't want to look through all datasheets ATM, so, I'll just trust 
you, ok?

> > > -#define NR_PCA9539_GPIOS       16
> > > +struct pca953x_desc {
> > > +       unsigned int gpios;
> > > +       unsigned int input;
> > > +       unsigned int output;
> > > +       unsigned int invert;
> > > +       unsigned int direction;
> > > +};
> > 
> > I guess the register address can be inferred either from the number
> > of GPIOs or a single member field of "shift" may be sufficient.
> 
> Yes, inferring that would be simpler.  The number of GPIOs is a
> function of the particular chip, and register offsets are 0/1/2/3
> except for the 16-GPIO part where they're twice that.
> 
> What I'd do is save those four offsets directly in pca953x_chip,
> initialized near when gpio_chip.ngpio is set up and with no need
> for a separate "desc" type or table.  Eventually there'd be:
> 
> 	struct i2c_device_id chips [] = {
> 		{ "pca9536", 4, },
> 		{ "pca9537", 4, },
> 		{ "pca9538", 8, },
> 		{ "pca9539", 16, },
> 	};
> 	MODULE_DEVICE_TABLE(i2c, chips);
> 
> Meanwhile, the equivalent of that table can come from a few strcmp()
> tests in the probe() logic -- like what you already have, except
> not using a "desc" type.

I introduced the descriptor array, to contain _constant_ chip 
descriptions, much like your "struct i2c_device_id chips []" array above. 
So, actually, using my descriptor array is nearer to what it eventually 
should become, I think. Under 2.6.26 you'd also, probably, just have a 
"struct i2c_device_id *" member in your pca953x_chip. I can change the 
descriptor table to look exactly like the i2c_device_id, and then in probe 
just walk it in a loop, comparing the name? Or would you be getting a 
pointer to "struct i2c_device_id" in the probe()?

> > > -static int pca9539_init_gpio(struct pca9539_chip *chip)
> > > +static int pca953x_init_gpio(struct pca953x_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->direction_input  = pca953x_gpio_direction_input;
> > > +       gc->direction_output = pca953x_gpio_direction_output;
> > > +       gc->get = pca953x_gpio_get_value;
> > > +       gc->set = pca953x_gpio_set_value;
> > >
> > >         gc->base = chip->gpio_start;
> > > -       gc->ngpio = NR_PCA9539_GPIOS;
> > > -       gc->label = "pca9539";
> > > +       gc->ngpio = chip->desc->gpios;
> 
> I'd expect "desc" to vanish, and ngpio to be an input to this function
> (see below).  Then ngpio could be used to choose the values to stuff
> into chip->{input,output,invert,direction} register offsets.
> 
> 
> > > +       gc->label = "pca953x";
> 
> Why not just set gc->label to be chip->client->name?

See above. I didn't invent it - just a result of "sed". But I can change 
it I guess.

> > > --- a/include/asm-generic/gpio.h
> > > +++ b/include/asm-generic/gpio.h
> > > @@ -16,6 +16,10 @@
> > >  #define ARCH_NR_GPIOS          256
> > >  #endif
> > >
> > > +#ifndef NO_GPIO
> > > +#define NO_GPIO                        ((unsigned int)-1)
> > > +#endif
> > > +
> > 
> > I don't understand this.
> 
> Me either; *ANY* negative number is invalid as a GPIO number,
> not just "-1"...

Ok, this one should rather go into a separate patch. I'd like to have such 
a macro to check whether the platform is using a GPIO with this specific 
camera or not. Similar to NO_IRQ.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell]

On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> Thanks for all the comments, I'm going to address them in the next version 
> of the patch. A couple of small clarifications first:
> 
> On Wed, 30 Jan 2008, David Brownell wrote:
> 
> > I'd split this patch into two parts (feature addition, and renaming),
> 
> Would you prefer
> 
> 	...
> 	
> patch-1: git-mv
> patch-2: sed -e "s/pca9539/pca953x/"
> patch-3: pca953[678]
> 
> ?

I think that last would be clearest in terms of GIT history
and patch reviewability... that's why you listed it, right?  :)
So that one, given my druthers.


> 
> > > >         depends on I2C
> > > >         help
> > > > -         Say yes here to support the PCA9539 16-bit I/O port. These
> > > > -         parts are made by NXP and TI.
> > > > +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.
> > 
> > This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
> > Same register layout as the '36 (4-bit).  Otherwise the "x" seems
> > misleading...
> 
> Ok, I don't want to look through all datasheets ATM, so, I'll just trust 
> you, ok?

OK by me.  The NXP website makes them easy enough to find, but
it's another one of those sites that tries to be too clever by
overusing JavaScript.


> > What I'd do is save those four offsets directly in pca953x_chip,
> > initialized near when gpio_chip.ngpio is set up and with no need
> > for a separate "desc" type or table.  Eventually there'd be:
> > 
> > 	struct i2c_device_id chips [] = {
> > 		{ "pca9536", 4, },
> > 		{ "pca9537", 4, },
> > 		{ "pca9538", 8, },
> > 		{ "pca9539", 16, },
> > 	};
> > 	MODULE_DEVICE_TABLE(i2c, chips);
> > 
> > Meanwhile, the equivalent of that table can come from a few strcmp()
> > tests in the probe() logic -- like what you already have, except
> > not using a "desc" type.
> 
> I introduced the descriptor array, to contain _constant_ chip 
> descriptions, much like your "struct i2c_device_id chips []" array above. 
> So, actually, using my descriptor array is nearer to what it eventually 
> should become, I think. Under 2.6.26 you'd also, probably, just have a 
> "struct i2c_device_id *" member in your pca953x_chip.

Well, I was also pointing out that all you need is the number of GPIOs;
no need to save any ID struct at all.  These chips are VERY similar.


> I can change the  
> descriptor table to look exactly like the i2c_device_id, and then in probe 
> just walk it in a loop, comparing the name? Or would you be getting a 
> pointer to "struct i2c_device_id" in the probe()?

The probe() would get handed the ID, not unlike how USB or PCI work:

 http://marc.info/?l=i2c&m=120091221712826&w=2


> > > > +#ifndef NO_GPIO
> > > > +#define NO_GPIO                        ((unsigned int)-1)
> > > > +#endif
> > > > +
> > > 
> > > I don't understand this.
> > 
> > Me either; *ANY* negative number is invalid as a GPIO number,
> > not just "-1"...
> 
> Ok, this one should rather go into a separate patch. I'd like to have such 
> a macro to check whether the platform is using a GPIO with this specific 
> camera or not. Similar to NO_IRQ.

Then maybe there should be an is_valid_gpio() predicate.
Anything not between 0..MAX_INT would fail.  And there
should be a Documentation/gpio.txt update to match.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[Guennadi Liakhovetski]

On Thu, 31 Jan 2008, David Brownell wrote:

> On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> > patch-1: git-mv
> > patch-2: sed -e "s/pca9539/pca953x/"
> > patch-3: pca953[678]
> > 
> > ?
> 
> I think that last would be clearest in terms of GIT history
> and patch reviewability... that's why you listed it, right?  :)
> So that one, given my druthers.

Was my preference too.

> > > > >         depends on I2C
> > > > >         help
> > > > > -         Say yes here to support the PCA9539 16-bit I/O port. These
> > > > > -         parts are made by NXP and TI.
> > > > > +         Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports.
> > > 
> > > This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
> > > Same register layout as the '36 (4-bit).  Otherwise the "x" seems
> > > misleading...
> > 
> > Ok, I don't want to look through all datasheets ATM, so, I'll just trust 
> > you, ok?
> 
> OK by me.  The NXP website makes them easy enough to find, but
> it's another one of those sites that tries to be too clever by
> overusing JavaScript.

I'm actually adding 9534 (8-bit) and 9535 (16-bit) too atm, if you don't 
mind:-)

> > > What I'd do is save those four offsets directly in pca953x_chip,
> > > initialized near when gpio_chip.ngpio is set up and with no need
> > > for a separate "desc" type or table.  Eventually there'd be:
> > > 
> > > 	struct i2c_device_id chips [] = {
> > > 		{ "pca9536", 4, },
> > > 		{ "pca9537", 4, },
> > > 		{ "pca9538", 8, },
> > > 		{ "pca9539", 16, },
> > > 	};
> > > 	MODULE_DEVICE_TABLE(i2c, chips);
> > > 
> > > Meanwhile, the equivalent of that table can come from a few strcmp()
> > > tests in the probe() logic -- like what you already have, except
> > > not using a "desc" type.
> > 
> > I introduced the descriptor array, to contain _constant_ chip 
> > descriptions, much like your "struct i2c_device_id chips []" array above. 
> > So, actually, using my descriptor array is nearer to what it eventually 
> > should become, I think. Under 2.6.26 you'd also, probably, just have a 
> > "struct i2c_device_id *" member in your pca953x_chip.
> 
> Well, I was also pointing out that all you need is the number of GPIOs;
> no need to save any ID struct at all.  These chips are VERY similar.

Ok, agreed, let's remove it then. For now I just use an array of structs 
similar to i2c_device_id only to avoid a long sequence of strcmp calls in 
probe() and compare the name in a loop. Is this alright?

> > > > > +#ifndef NO_GPIO
> > > > > +#define NO_GPIO                        ((unsigned int)-1)
> > > > > +#endif
> > > > > +
> > > > 
> > > > I don't understand this.
> > > 
> > > Me either; *ANY* negative number is invalid as a GPIO number,
> > > not just "-1"...
> > 
> > Ok, this one should rather go into a separate patch. I'd like to have such 
> > a macro to check whether the platform is using a GPIO with this specific 
> > camera or not. Similar to NO_IRQ.
> 
> Then maybe there should be an is_valid_gpio() predicate.
> Anything not between 0..MAX_INT would fail.  And there
> should be a Documentation/gpio.txt update to match.

Good, will do that.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell]

On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> 
> > > > This should IMO list the '38 (8-bit) and '37 (4-bit) parts too.
> > > > Same register layout as the '36 (4-bit).  Otherwise the "x" seems
> > > > misleading...
> > > 
> > > Ok, I don't want to look through all datasheets ATM, so, I'll just trust 
> > > you, ok?
> > 
> > OK by me.  The NXP website makes them easy enough to find, but
> > it's another one of those sites that tries to be too clever by
> > overusing JavaScript.
> 
> I'm actually adding 9534 (8-bit) and 9535 (16-bit) too atm, if you don't 
> mind:-)

Do they have the same register interfaces?  I looked at them a long
time ago, and don't recall.  If they have the same interface, then
more power to you!



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell]

On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> 
> > > So, actually, using my descriptor array is nearer to what it eventually 
> > > should become, I think. Under 2.6.26 you'd also, probably, just have a 
> > > "struct i2c_device_id *" member in your pca953x_chip.
> > 
> > Well, I was also pointing out that all you need is the number of GPIOs;
> > no need to save any ID struct at all.  These chips are VERY similar.
> 
> Ok, agreed, let's remove it then. For now I just use an array of structs 
> similar to i2c_device_id only to avoid a long sequence of strcmp calls in 
> probe() and compare the name in a loop. Is this alright?

Sure; it's your call on most such details, I just wanted clean patches.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[Guennadi Liakhovetski]

On Thu, 31 Jan 2008, David Brownell wrote:

> > > > > +#ifndef NO_GPIO
> > > > > +#define NO_GPIO                        ((unsigned int)-1)
> > > > > +#endif
> > > > > +
> > > > 
> > > > I don't understand this.
> > > 
> > > Me either; *ANY* negative number is invalid as a GPIO number,
> > > not just "-1"...
> > 
> > Ok, this one should rather go into a separate patch. I'd like to have such 
> > a macro to check whether the platform is using a GPIO with this specific 
> > camera or not. Similar to NO_IRQ.
> 
> Then maybe there should be an is_valid_gpio() predicate.
> Anything not between 0..MAX_INT would fail.  And there
> should be a Documentation/gpio.txt update to match.

hm, but if you deliberately want an invalid gpio number?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver

[David Brownell]

On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> On Thu, 31 Jan 2008, David Brownell wrote:
> 
> > > > > > +#ifndef NO_GPIO
> > > > > > +#define NO_GPIO                        ((unsigned int)-1)
> > > > > > +#endif
> > > > > > +
> > > > > 
> > > > > I don't understand this.
> > > > 
> > > > Me either; *ANY* negative number is invalid as a GPIO number,
> > > > not just "-1"...
> > > 
> > > Ok, this one should rather go into a separate patch. I'd like to have such 
> > > a macro to check whether the platform is using a GPIO with this specific 
> > > camera or not. Similar to NO_IRQ.
> > 
> > Then maybe there should be an is_valid_gpio() predicate.
> > Anything not between 0..MAX_INT would fail.  And there
> > should be a Documentation/gpio.txt update to match.
> 
> hm, but if you deliberately want an invalid gpio number?

Any negative number suffices -- like "-EINVAL".  But that's a
separate problem from the one you said you wanted to solve...

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [RFC PATCH 0/8] Patch series to add soc_camera virtual v4l2 and its users

[Mauro Carvalho Chehab]

On Sun, 27 Jan 2008 20:12:25 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@pengutronix.de> wrote:

> On Wed, 23 Jan 2008, Guennadi Liakhovetski wrote:
> 
> > Hi all
> > 
> > Following the first preliminary version I posted on 11.01 to this list, 
> > following this message I am posting the soc_camera driver, an SoC part for 
> > the PXA27x CPU, two camera drivers, a platform-specific part, that's not 
> > yet in the mainline, so, so far this is only an example, and an auxiliary 
> > i2c driver for a GPIO extender. Also two fourcc codes and a new control 
> > are added to the V4L2 API. A lot of changes since the previous version, 
> > including all new names. This patch series is against 2.6.23. There seem 
> > to have been a few significant changes around v4l2 since then, so, forward 
> > porting it to 2.6.24 will need some work too. But the good news is, that I 
> > do not expect to significantly change this driver any more. So, please 
> > review, comment, and test if you can.
> 
> Mauro, you probably saw this my patch series and the following discussion. 
> A question to you - I have fixed all issues pointed out by Trent Piepho 
> and Sakari Ailus in my local tree, now I have to forward port the patches 
> to
> 
>     ssh://master.kernel.org/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git master
> 
> After this is done, would you be prepared to accept them or do you see any 
> principle problems with those patches?

On a quick look, they seem ok. Yet, I would prefer if you later move the
sensors interface to use v4l2-int-device. This will allow future reusage of
those drivers with other cameras.

I should be analyzing your series today and commit if all are ok.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list