linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] I2C: add support for the PCF8575 chip
@ 2007-10-05  9:32 Bart Van Assche
  2007-10-05 20:11 ` Jean Delvare
  2007-10-22 21:58 ` Andrew Morton
  0 siblings, 2 replies; 7+ messages in thread
From: Bart Van Assche @ 2007-10-05  9:32 UTC (permalink / raw)
  To: khali; +Cc: linux-kernel

From: Bart Van Assche

Add support for the PCF8575 I2C chip.

Signed-off-by: Bart Van Assche (bart.vanassche@gmail.com)
---
diff -uprN -X orig/linux-2.6.22.9/Documentation/dontdiff
orig/linux-2.6.22.9/drivers/i2c/chips/Kconfig
linux-2.6.22.9/drivers/i2c/chips/Kconfig
--- orig/linux-2.6.22.9/drivers/i2c/chips/Kconfig	2007-09-26
20:03:01.000000000 +0200
+++ linux-2.6.22.9/drivers/i2c/chips/Kconfig	2007-10-05 09:27:19.000000000 +0200
@@ -41,13 +41,27 @@ config SENSORS_PCF8574
 	default n
 	help
 	  If you say yes here you get support for Philips PCF8574 and
-	  PCF8574A chips.
+	  PCF8574A chips. These chips are 8-bit I/O expanders for the I2C bus.

 	  This driver can also be built as a module.  If so, the module
 	  will be called pcf8574.

-	  These devices are hard to detect and rarely found on mainstream
-	  hardware.  If unsure, say N.
+	  These devices are hard to detect automatically and are rarely found
+	  on mainstream hardware.  If unsure, say N.
+
+config SENSORS_PCF8575
+	tristate "Philips PCF8575"
+	default n
+	help
+	  If you say yes here you get support for Philips PCF8575 chip.
+	  This chip is a 16-bit I/O expander for the I2C bus.  Several other
+	  chip manufacturers sell equivalent chips, e.g. Texas Instruments.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called pcf8575.
+
+	  This device is hard to detect automatically and is rarely found on
+	  mainstream hardware.  If unsure, say N.

 config SENSORS_PCA9539
 	tristate "Philips PCA9539 16-bit I/O port"
diff -uprN -X orig/linux-2.6.22.9/Documentation/dontdiff
orig/linux-2.6.22.9/drivers/i2c/chips/Makefile
linux-2.6.22.9/drivers/i2c/chips/Makefile
--- orig/linux-2.6.22.9/drivers/i2c/chips/Makefile	2007-09-26
20:03:01.000000000 +0200
+++ linux-2.6.22.9/drivers/i2c/chips/Makefile	2007-10-05
09:16:21.000000000 +0200
@@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_MAX6875)	+= max6875
 obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
+obj-$(CONFIG_SENSORS_PCF8575)	+= pcf8575.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
diff -uprN -X orig/linux-2.6.22.9/Documentation/dontdiff
orig/linux-2.6.22.9/drivers/i2c/chips/pcf8575.c
linux-2.6.22.9/drivers/i2c/chips/pcf8575.c
--- orig/linux-2.6.22.9/drivers/i2c/chips/pcf8575.c	2007-10-05
09:44:02.000000000 +0200
+++ linux-2.6.22.9/drivers/i2c/chips/pcf8575.c	2007-10-05
11:10:46.000000000 +0200
@@ -0,0 +1,305 @@
+/*
+  pcf8575.c - Part of lm_sensors, Linux kernel modules for hardware
+  monitoring
+  Copyright (c) 2000  Frodo Looijaard <frodol@dds.nl>,
+  Philip Edelbrock <phil@netroedge.com>,
+  Dan Eaton <dan.eaton@rocketlogix.com>
+  Ported to Linux 2.6 by Aurelien Jarno <aurel32@debian.org> with
+  the help of Jean Delvare <khali@linux-fr.org>
+
+  Copyright (C) 2006 Michael Hennerich, Analog Devices Inc.
+  <hennerich@blackfin.uclinux.org>
+  based on the pcf8574.c
+
+  Ported from ucLinux to mainstream Linux kernel by Bart Van Assche.
+
+  About the pcf8575: the pcf8575 is an 16-bit I/O expander for the I2C bus
+  produced by a.o. Philips Semiconductors.
+
+  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; either version 2 of the License, or
+  (at your option) any later version.
+
+  This program is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU General Public License for more details.
+
+  You should have received a copy of the GNU General Public License
+  along with this program; if not, write to the Free Software
+  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/i2c.h>
+
+
+/* Addresses to scan */
+static unsigned short normal_i2c[]       = { I2C_CLIENT_END };
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 20)
+static unsigned short normal_i2c_range[] = { I2C_CLIENT_END };
+#endif
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD;
+
+
+/* Each client has this additional data */
+struct pcf8575_data {
+	struct i2c_client client;
+
+	u16 write;  /* Remember last written value */
+	u8  buf[3];
+};
+
+static int pcf8575_attach_adapter(struct i2c_adapter *adapter);
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
+static int pcf8575_detect(struct i2c_adapter *adapter, int address, int kind);
+#else
+static int pcf8575_detect(struct i2c_adapter *adapter,
+			int addr, unsigned short flags, int kind);
+#endif
+static int pcf8575_detach_client(struct i2c_client *client);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver pcf8575_driver = {
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "pcf8575",
+	},
+#elif LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
+	.owner           = THIS_MODULE,
+	.name            = "pcf8575",
+	.flags           = I2C_DF_NOTIFY,
+#else
+#error
+#endif
+	.attach_adapter  = pcf8575_attach_adapter,
+	.detach_client   = pcf8575_detach_client,
+};
+
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
+/* following are the sysfs callback functions */
+static ssize_t show_read(struct device *dev,
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+			struct device_attribute *attr,
+#endif
+			char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pcf8575_data *data = i2c_get_clientdata(client);
+	unsigned short val;
+
+	i2c_master_recv(client, data->buf, 2);
+
+	val = data->buf[0];
+	val |= data->buf[1]<<8;
+
+	return sprintf(buf, "%u\n", val);
+}
+
+static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
+
+static ssize_t show_write(struct device *dev,
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+			struct device_attribute *attr,
+#endif
+			char *buf)
+{
+	struct pcf8575_data *data = i2c_get_clientdata(to_i2c_client(dev));
+	return sprintf(buf, "%u\n", data->write);
+}
+
+static ssize_t set_write(struct device *dev,
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+			struct device_attribute *attr,
+#endif
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pcf8575_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+
+	if (val > 0xffff)
+		return -EINVAL;
+
+	data->write = val;
+
+	data->buf[0] = val & 0xFF;
+	data->buf[1] = val >> 8;
+
+	i2c_master_send(client, data->buf, 2);
+
+	return count;
+}
+
+static DEVICE_ATTR(write, S_IWUSR | S_IRUGO, show_write, set_write);
+
+static ssize_t set_set_bit(struct device *dev,
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+			struct device_attribute *attr,
+#endif
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pcf8575_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	unsigned short dummy;
+	if (val > 15)
+		return -EINVAL;
+
+	i2c_master_recv(client, data->buf, 2);
+
+	dummy = data->buf[0];
+	dummy |= data->buf[1]<<8;
+
+	dummy |= 1 << val;
+
+	data->buf[0] = dummy & 0xFF;
+	data->buf[1] = dummy >> 8;
+
+	i2c_master_send(client, data->buf, 2);
+
+	return count;
+}
+
+static DEVICE_ATTR(set_bit, S_IWUSR, show_write, set_set_bit);
+
+static ssize_t set_clear_bit(struct device *dev,
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
+			struct device_attribute *attr,
+#endif
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pcf8575_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	unsigned short dummy;
+	if (val > 15)
+		return -EINVAL;
+
+	i2c_master_recv(client, data->buf, 2);
+
+	dummy = data->buf[0];
+	dummy |= data->buf[1]<<8;
+
+	dummy &= ~(1 << val);
+
+	data->buf[0] = dummy & 0xFF;
+	data->buf[1] = dummy >> 8;
+
+	i2c_master_send(client, data->buf, 2);
+
+	return count;
+}
+
+static DEVICE_ATTR(clear_bit, S_IWUSR, show_write, set_clear_bit);
+#endif
+
+/*
+ * Real code
+ */
+
+static int pcf8575_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_probe(adapter, &addr_data, pcf8575_detect);
+}
+
+/* This function is called by i2c_probe */
+static int pcf8575_detect(struct i2c_adapter *adapter,
+			int address,
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 0)
+			unsigned short flags,
+#endif
+			int kind)
+{
+	struct i2c_client *new_client;
+	struct pcf8575_data *data;
+	int err = 0;
+	const char *client_name = "";
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+		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. */
+	data = kzalloc(sizeof(struct pcf8575_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	new_client = &data->client;
+	i2c_set_clientdata(new_client,  data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &pcf8575_driver;
+	new_client->flags = 0;
+
+	/* Now, we would do the remaining detection. But the pcf8575 is plainly
+	   impossible to detect! Stupid chip. */
+
+
+	client_name = "pcf8575";
+
+	/* Fill in the remaining client fields and put into the global list */
+	strlcpy(new_client->name, client_name, I2C_NAME_SIZE);
+
+	/* Tell the I2C layer a new client has arrived */
+	err = i2c_attach_client(new_client);
+	if (err)
+		goto exit_free;
+
+
+	/* Register sysfs hooks */
+	if (device_create_file(&new_client->dev, &dev_attr_read) >= 0
+	    && device_create_file(&new_client->dev, &dev_attr_write) >= 0
+	    && device_create_file(&new_client->dev, &dev_attr_set_bit) >= 0
+	    && device_create_file(&new_client->dev, &dev_attr_clear_bit) >= 0) {
+		return 0;
+	}
+
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int pcf8575_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	err = i2c_detach_client(client);
+	if (err)
+		return err;
+
+	kfree(i2c_get_clientdata(client));
+	return 0;
+}
+
+static int __init pcf8575_init(void)
+{
+	return i2c_add_driver(&pcf8575_driver);
+}
+
+static void __exit pcf8575_exit(void)
+{
+	i2c_del_driver(&pcf8575_driver);
+}
+
+
+MODULE_AUTHOR("Frodo Looijaard <frodol@dds.nl>, "
+	      "Philip Edelbrock <phil@netroedge.com>, "
+	      "Dan Eaton <dan.eaton@rocketlogix.com> "
+	      "and Aurelien Jarno <aurelien@aurel32.net>"
+	      "Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("pcf8575 driver");
+MODULE_LICENSE("GPL");
+
+module_init(pcf8575_init);
+module_exit(pcf8575_exit);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] I2C: add support for the PCF8575 chip
  2007-10-05  9:32 [PATCH] I2C: add support for the PCF8575 chip Bart Van Assche
@ 2007-10-05 20:11 ` Jean Delvare
  2007-10-22 21:58 ` Andrew Morton
  1 sibling, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2007-10-05 20:11 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: linux-kernel

Hi Bart,

On Fri, 5 Oct 2007 11:32:35 +0200, Bart Van Assche wrote:
> From: Bart Van Assche
> 
> Add support for the PCF8575 I2C chip.

Please read MAINTAINERS and send your patch to the right list.

Thanks,
-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] I2C: add support for the PCF8575 chip
  2007-10-05  9:32 [PATCH] I2C: add support for the PCF8575 chip Bart Van Assche
  2007-10-05 20:11 ` Jean Delvare
@ 2007-10-22 21:58 ` Andrew Morton
  2007-10-22 22:32   ` [i2c] " Trent Piepho
  2007-10-23 11:50   ` Jean Delvare
  1 sibling, 2 replies; 7+ messages in thread
From: Andrew Morton @ 2007-10-22 21:58 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: khali, linux-kernel, i2c

On Fri, 5 Oct 2007 11:32:35 +0200
"Bart Van Assche" <bart.vanassche@gmail.com> wrote:

> From: Bart Van Assche
> 
> Add support for the PCF8575 I2C chip.
> 

I'll comment on this 17-day-old patch.

Jean, this illustrates why explicitly steering people *away* from lkml or
from any other mailing list is a poor idea.  If Bart has posted an updated
version to the i2c list then I end up reviewing an outdated patch, and
probably duplicating other people's comments.

googling for 'PCF8575 Assche' indicates that he has not sent an updated
patch.  Perhaps he was discouraged by your quite unconstructive response.

> linux-2.6.22.9/drivers/i2c/chips/pcf8575.c
> --- orig/linux-2.6.22.9/drivers/i2c/chips/pcf8575.c	2007-10-05
> 09:44:02.000000000 +0200
> +++ linux-2.6.22.9/drivers/i2c/chips/pcf8575.c	2007-10-05
> 11:10:46.000000000 +0200

Your email client is wordwrapping patches.  Fortunately only the patch
headers were wrapped so it still applies OK, but some reconfiguration is
neded for next time, please.

> @@ -0,0 +1,305 @@
> +/*
> +  pcf8575.c - Part of lm_sensors, Linux kernel modules for hardware
> +  monitoring
> +  Copyright (c) 2000  Frodo Looijaard <frodol@dds.nl>,
> +  Philip Edelbrock <phil@netroedge.com>,
> +  Dan Eaton <dan.eaton@rocketlogix.com>
> +  Ported to Linux 2.6 by Aurelien Jarno <aurel32@debian.org> with
> +  the help of Jean Delvare <khali@linux-fr.org>
> +
> +  Copyright (C) 2006 Michael Hennerich, Analog Devices Inc.
> +  <hennerich@blackfin.uclinux.org>
> +  based on the pcf8574.c
> +
> +  Ported from ucLinux to mainstream Linux kernel by Bart Van Assche.
> +
> +  About the pcf8575: the pcf8575 is an 16-bit I/O expander for the I2C bus
> +  produced by a.o. Philips Semiconductors.
> +
> +  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; either version 2 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program; if not, write to the Free Software
> +  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/version.h>
> +#include <linux/i2c.h>
> +
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[]       = { I2C_CLIENT_END };
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 20)
> +static unsigned short normal_i2c_range[] = { I2C_CLIENT_END };
> +#endif

Please remove all the LINUX_VERSION_CODE tests and target only the current
mainline tree.

> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD;
> +
> +
> +/* Each client has this additional data */
> +struct pcf8575_data {
> +	struct i2c_client client;
> +
> +	u16 write;  /* Remember last written value */
> +	u8  buf[3];
> +};
> +
> +static int pcf8575_attach_adapter(struct i2c_adapter *adapter);
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
> +static int pcf8575_detect(struct i2c_adapter *adapter, int address, int kind);
> +#else
> +static int pcf8575_detect(struct i2c_adapter *adapter,
> +			int addr, unsigned short flags, int kind);
> +#endif
> +static int pcf8575_detach_client(struct i2c_client *client);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver pcf8575_driver = {
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "pcf8575",
> +	},
> +#elif LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
> +	.owner           = THIS_MODULE,
> +	.name            = "pcf8575",
> +	.flags           = I2C_DF_NOTIFY,
> +#else
> +#error
> +#endif
> +	.attach_adapter  = pcf8575_attach_adapter,
> +	.detach_client   = pcf8575_detach_client,
> +};
> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
> +/* following are the sysfs callback functions */
> +static ssize_t show_read(struct device *dev,
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +			struct device_attribute *attr,
> +#endif
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pcf8575_data *data = i2c_get_clientdata(client);
> +	unsigned short val;
> +
> +	i2c_master_recv(client, data->buf, 2);
> +
> +	val = data->buf[0];
> +	val |= data->buf[1]<<8;
> +
> +	return sprintf(buf, "%u\n", val);
> +}
> +
> +static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
> +
> +static ssize_t show_write(struct device *dev,
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +			struct device_attribute *attr,
> +#endif

yeah, I agree with me ;)  It really messes the code up.

> +			char *buf)
> +{
> +	struct pcf8575_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +	return sprintf(buf, "%u\n", data->write);
> +}
> +
> +static ssize_t set_write(struct device *dev,
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +			struct device_attribute *attr,
> +#endif
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pcf8575_data *data = i2c_get_clientdata(client);
> +	unsigned long val = simple_strtoul(buf, NULL, 10);
> +
> +	if (val > 0xffff)
> +		return -EINVAL;
> +
> +	data->write = val;
> +
> +	data->buf[0] = val & 0xFF;
> +	data->buf[1] = val >> 8;
> +
> +	i2c_master_send(client, data->buf, 2);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(write, S_IWUSR | S_IRUGO, show_write, set_write);
> +
> +static ssize_t set_set_bit(struct device *dev,
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +			struct device_attribute *attr,
> +#endif
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pcf8575_data *data = i2c_get_clientdata(client);
> +	unsigned long val = simple_strtoul(buf, NULL, 10);
> +	unsigned short dummy;
> +	if (val > 15)
> +		return -EINVAL;
> +
> +	i2c_master_recv(client, data->buf, 2);
> +
> +	dummy = data->buf[0];
> +	dummy |= data->buf[1]<<8;
> +
> +	dummy |= 1 << val;
> +
> +	data->buf[0] = dummy & 0xFF;
> +	data->buf[1] = dummy >> 8;
> +
> +	i2c_master_send(client, data->buf, 2);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(set_bit, S_IWUSR, show_write, set_set_bit);
> +
> +static ssize_t set_clear_bit(struct device *dev,
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
> +			struct device_attribute *attr,
> +#endif
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pcf8575_data *data = i2c_get_clientdata(client);
> +	unsigned long val = simple_strtoul(buf, NULL, 10);
> +	unsigned short dummy;
> +	if (val > 15)
> +		return -EINVAL;
> +
> +	i2c_master_recv(client, data->buf, 2);
> +
> +	dummy = data->buf[0];
> +	dummy |= data->buf[1]<<8;
> +
> +	dummy &= ~(1 << val);
> +
> +	data->buf[0] = dummy & 0xFF;
> +	data->buf[1] = dummy >> 8;
> +
> +	i2c_master_send(client, data->buf, 2);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(clear_bit, S_IWUSR, show_write, set_clear_bit);
> +#endif

ick, they're nested two-deep!

> +/*
> + * Real code
> + */
> +
> +static int pcf8575_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	return i2c_probe(adapter, &addr_data, pcf8575_detect);
> +}
> +
> +/* This function is called by i2c_probe */
> +static int pcf8575_detect(struct i2c_adapter *adapter,
> +			int address,
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 0)
> +			unsigned short flags,
> +#endif
> +			int kind)
> +{
> +	struct i2c_client *new_client;
> +	struct pcf8575_data *data;
> +	int err = 0;
> +	const char *client_name = "";
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> +		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. */
> +	data = kzalloc(sizeof(struct pcf8575_data), GFP_KERNEL);
> +	if (!data) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	new_client = &data->client;
> +	i2c_set_clientdata(new_client,  data);
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &pcf8575_driver;
> +	new_client->flags = 0;
> +
> +	/* Now, we would do the remaining detection. But the pcf8575 is plainly
> +	   impossible to detect! Stupid chip. */
> +
> +
> +	client_name = "pcf8575";
> +
> +	/* Fill in the remaining client fields and put into the global list */
> +	strlcpy(new_client->name, client_name, I2C_NAME_SIZE);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	err = i2c_attach_client(new_client);
> +	if (err)
> +		goto exit_free;
> +
> +
> +	/* Register sysfs hooks */
> +	if (device_create_file(&new_client->dev, &dev_attr_read) >= 0
> +	    && device_create_file(&new_client->dev, &dev_attr_write) >= 0
> +	    && device_create_file(&new_client->dev, &dev_attr_set_bit) >= 0
> +	    && device_create_file(&new_client->dev, &dev_attr_clear_bit) >= 0) {
> +		return 0;
> +	}
> +
> +exit_free:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int pcf8575_detach_client(struct i2c_client *client)
> +{
> +	int err;
> +
> +	err = i2c_detach_client(client);
> +	if (err)
> +		return err;
> +
> +	kfree(i2c_get_clientdata(client));
> +	return 0;
> +}
> +
> +static int __init pcf8575_init(void)
> +{
> +	return i2c_add_driver(&pcf8575_driver);
> +}
> +
> +static void __exit pcf8575_exit(void)
> +{
> +	i2c_del_driver(&pcf8575_driver);
> +}
> +
> +
> +MODULE_AUTHOR("Frodo Looijaard <frodol@dds.nl>, "
> +	      "Philip Edelbrock <phil@netroedge.com>, "
> +	      "Dan Eaton <dan.eaton@rocketlogix.com> "
> +	      "and Aurelien Jarno <aurelien@aurel32.net>"
> +	      "Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("pcf8575 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(pcf8575_init);
> +module_exit(pcf8575_exit);

Apart from that it looks OK to me.  I'll add the patch as-is to the -mm
tree so that it does not get lost.  Thanks.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [i2c] [PATCH] I2C: add support for the PCF8575 chip
  2007-10-22 21:58 ` Andrew Morton
@ 2007-10-22 22:32   ` Trent Piepho
  2007-10-23 11:50   ` Jean Delvare
  1 sibling, 0 replies; 7+ messages in thread
From: Trent Piepho @ 2007-10-22 22:32 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Bart Van Assche, linux-kernel, i2c

On Mon, 22 Oct 2007, Andrew Morton wrote:
> On Fri, 5 Oct 2007 11:32:35 +0200
> "Bart Van Assche" <bart.vanassche@gmail.com> wrote:
> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 20)
> > +static unsigned short normal_i2c_range[] = { I2C_CLIENT_END };
> > +#endif
>
> Please remove all the LINUX_VERSION_CODE tests and target only the current
> mainline tree.

If you want to have a module that can be compiled out of tree and support
multiple kernel versions, there is a script used in the v4l-dvb out of tree
build system that can make it easier.

http://linuxtv.org/hg/v4l-dvb/raw-file/4ae65d0844cf/v4l/scripts/gentree.pl

This script will process #if/#ifdef/#elif/etc.  directives and remove them
from the source.  It is smart enough to understand LINUX_KERNEL_VERSION
tests, && ||, constants, complex expression, etc.  It also knows when the
result of an expression isn't known and will leave it in the code in that
case.  We use it to strip the compat code from the source when it's sent to
the kernel.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] I2C: add support for the PCF8575 chip
  2007-10-22 21:58 ` Andrew Morton
  2007-10-22 22:32   ` [i2c] " Trent Piepho
@ 2007-10-23 11:50   ` Jean Delvare
  2007-10-23 16:18     ` Andrew Morton
  1 sibling, 1 reply; 7+ messages in thread
From: Jean Delvare @ 2007-10-23 11:50 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Bart Van Assche, linux-kernel, i2c

Hi Andrew,

On Mon, 22 Oct 2007 14:58:14 -0700, Andrew Morton wrote:
> On Fri, 5 Oct 2007 11:32:35 +0200
> "Bart Van Assche" <bart.vanassche@gmail.com> wrote:
> 
> > From: Bart Van Assche
> > 
> > Add support for the PCF8575 I2C chip.
> 
> I'll comment on this 17-day-old patch.
> 
> Jean, this illustrates why explicitly steering people *away* from lkml or
> from any other mailing list is a poor idea.  If Bart has posted an updated
> version to the i2c list then I end up reviewing an outdated patch, and
> probably duplicating other people's comments.

You're unfair. I can use the same argument the other way around: if
Bart did not post to the wrong list in the first place, then there
would have been no risk for you to miss any update.

I stand on my initial affirmation that sending all patches, bug reports
etc. to LKML when more specialized lists exist, is a bad idea. Maybe it
makes you happy because you want to know everything that's going on in
every area on the kernel, and are lucky enough to be able to actually
do that and survive. But for others, it's essentially wasting their
time (not to mention bandwidth and disk space.)

If you are so interested about i2c patches, then I'd suggest that you
simply subscribe to the i2c list.

> googling for 'PCF8575 Assche' indicates that he has not sent an updated
> patch.  Perhaps he was discouraged by your quite unconstructive response.

Actually Bart resent his patch to the i2c list 3 days later, twice. But
it was probably the exact same patch (it didn't mention any changes at
least.) There's no reason why Bart would have sent an updated patch as
he did not receive feedback at this point. I fail to see how my
response would have had any influence in that respect.

Anyway, thanks for the review. I didn't have much time left for reviews
these last few weeks.

Still... I am worried that you, Andrew Morton, co-top-maintainer of the
Linux 2.6 kernel, one of most brilliant kernel developers we have,
waste your time doing the initial review of a random i2c patch that
about anyone remotely involved in kernel development would have been
able to review. There's something wrong here.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] I2C: add support for the PCF8575 chip
  2007-10-23 11:50   ` Jean Delvare
@ 2007-10-23 16:18     ` Andrew Morton
  2007-10-24  8:34       ` Jean Delvare
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2007-10-23 16:18 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Bart Van Assche, linux-kernel, i2c

On Tue, 23 Oct 2007 13:50:30 +0200 Jean Delvare <khali@linux-fr.org> wrote:

> Hi Andrew,
> 
> On Mon, 22 Oct 2007 14:58:14 -0700, Andrew Morton wrote:
> > On Fri, 5 Oct 2007 11:32:35 +0200
> > "Bart Van Assche" <bart.vanassche@gmail.com> wrote:
> > 
> > > From: Bart Van Assche
> > > 
> > > Add support for the PCF8575 I2C chip.
> > 
> > I'll comment on this 17-day-old patch.
> > 
> > Jean, this illustrates why explicitly steering people *away* from lkml or
> > from any other mailing list is a poor idea.  If Bart has posted an updated
> > version to the i2c list then I end up reviewing an outdated patch, and
> > probably duplicating other people's comments.
> 
> You're unfair. I can use the same argument the other way around: if
> Bart did not post to the wrong list in the first place, then there
> would have been no risk for you to miss any update.

Sure.  But what I was referring to was the recommendation that submitters
explicitly _remove_ lkml from the cc.

> I stand on my initial affirmation that sending all patches, bug reports
> etc. to LKML when more specialized lists exist, is a bad idea. Maybe it
> makes you happy because you want to know everything that's going on in
> every area on the kernel, and are lucky enough to be able to actually
> do that and survive. But for others, it's essentially wasting their
> time (not to mention bandwidth and disk space.)
> 
> If you are so interested about i2c patches, then I'd suggest that you
> simply subscribe to the i2c list.

I am subscribed to a large number of lists, but I don't read them.  I keep
them around so I can go find and reply to the original email thread when a
fishy patch turns up in the tree.

But I'm not just talking about me.  Consider the example of a random lkml
lurker who has a PCF8575 and who can end up using an old version of the
code.

> > googling for 'PCF8575 Assche' indicates that he has not sent an updated
> > patch.  Perhaps he was discouraged by your quite unconstructive response.
> 
> Actually Bart resent his patch to the i2c list 3 days later, twice. But
> it was probably the exact same patch (it didn't mention any changes at
> least.) There's no reason why Bart would have sent an updated patch as
> he did not receive feedback at this point. I fail to see how my
> response would have had any influence in that respect.
> 
> Anyway, thanks for the review. I didn't have much time left for reviews
> these last few weeks.
> 
> Still... I am worried that you, Andrew Morton, co-top-maintainer of the
> Linux 2.6 kernel, one of most brilliant kernel developers we have,
> waste your time doing the initial review of a random i2c patch that
> about anyone remotely involved in kernel development would have been
> able to review. There's something wrong here.

It goes like this:

- patch floats past, I save it

- a week or three later I check to see if it is still unmerged

- if so, go look at the lkml thread

- if nothing much has happened then I'll assume that it got lost and will
  pick it up so that it gets consideration

If the discussion got steered to a different list and lkml got removed from
that discussion then I end up wasting everyone's time.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] I2C: add support for the PCF8575 chip
  2007-10-23 16:18     ` Andrew Morton
@ 2007-10-24  8:34       ` Jean Delvare
  0 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2007-10-24  8:34 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Bart Van Assche, linux-kernel, i2c

Hi Andrew,

On Tue, 23 Oct 2007 09:18:45 -0700, Andrew Morton wrote:
> On Tue, 23 Oct 2007 13:50:30 +0200 Jean Delvare <khali@linux-fr.org> wrote:
> > You're unfair. I can use the same argument the other way around: if
> > Bart did not post to the wrong list in the first place, then there
> > would have been no risk for you to miss any update.
> 
> Sure.  But what I was referring to was the recommendation that submitters
> explicitly _remove_ lkml from the cc.

Again you are unfair. I never suggested that people _remove_ LKML from
the Cc. I asked that they do not include it to start with, which is
different.

That being said, chances are higher that a list gets dropped in the
course of the discussions if many lists were included in the first
place. Which is just one more reason to not flood too many lists in the
first place.

> (...)
> But I'm not just talking about me.  Consider the example of a random lkml
> lurker who has a PCF8575 and who can end up using an old version of the
> code.

Anyone picking a random patch on any mailing list instead of waiting
for it to go upstream or at least in some developer tree, should be
aware that he/she is running experimental code, and should search for
possible updates. This includes google, marc.info and getting in touch
with the author of the patch if the patch is getting old. If nothing
else, letting the author know that you tested his/her patch is the bare
minimum you can do as a tester if you care about the patch going
upstream.

This really doesn't have anything to do with what list(s) the patch has
been sent to in the first place.

> > Still... I am worried that you, Andrew Morton, co-top-maintainer of the
> > Linux 2.6 kernel, one of most brilliant kernel developers we have,
> > waste your time doing the initial review of a random i2c patch that
> > about anyone remotely involved in kernel development would have been
> > able to review. There's something wrong here.
> 
> It goes like this:
> 
> - patch floats past, I save it
> 
> - a week or three later I check to see if it is still unmerged
> 
> - if so, go look at the lkml thread
> 
> - if nothing much has happened then I'll assume that it got lost and will
>   pick it up so that it gets consideration
> 
> If the discussion got steered to a different list and lkml got removed from
> that discussion then I end up wasting everyone's time.

That's unfortunate, but I'd guess this happens all the time and this is
inherently bound to the way you work. Picking random patches after 3
weeks and assuming that nothing happened to them just because LKML says
so, is quite a risky bet, methinks.

My opinion on the matter is that it's up to submitters to make sure
that non-bugfix patches they send aren't lost, not maintainers. If
someone sends a patch adding support for a new device, and nobody
replies, I say it's up to the sender to resend the patch, or even
better to actively look for someone to review the patch, instead of
passively waiting for something to happen. The kernel is growing fast,
and the only way for us maintainers to scale is to teach contributors
how to help us keep up with the workload.

Bugfix patches are of course different, it's up to the maintainers to
review them quickly and make sure they aren't lost, no question about
that.

You have a different opinion, you insist on picking _all_ patches and
taking care of everything. This is your own right, you are free to work
the way you want and I respect that. But please don't expect me to
change the way _I_ want to work to accommodate the way _you_ want work.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-10-24  8:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-05  9:32 [PATCH] I2C: add support for the PCF8575 chip Bart Van Assche
2007-10-05 20:11 ` Jean Delvare
2007-10-22 21:58 ` Andrew Morton
2007-10-22 22:32   ` [i2c] " Trent Piepho
2007-10-23 11:50   ` Jean Delvare
2007-10-23 16:18     ` Andrew Morton
2007-10-24  8:34       ` Jean Delvare

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).