[i2c] [patch 2.6.23.1] add support for the PCF8575 chip

Jean Delvare khali at linux-fr.org
Tue Oct 23 20:07:43 CEST 2007


Hi Bart,

On Tue, 23 Oct 2007 13:02:53 +0200, bart.vanassche at gmail.com wrote:
> I2C: add support for the PCF8575 chip.
> 
> Signed-off-by: Bart Van Assche (bart.vanassche at gmail.com)

Please replace parentheses with <>.

Thanks for the updated patch. Here's my review:

> 
> diff -uprN -X orig/linux-2.6.23.1/Documentation/dontdiff -x linux-2.6.23.1/arch/x86_64/vdso/vdso.lds orig/linux-2.6.23.1/drivers/i2c/chips/Kconfig linux-2.6.23.1/drivers/i2c/chips/Kconfig
> --- orig/linux-2.6.23.1/drivers/i2c/chips/Kconfig	2007-10-12 18:43:44.000000000 +0200
> +++ linux-2.6.23.1/drivers/i2c/chips/Kconfig	2007-10-23 10:12:39.000000000 +0200
> @@ -57,13 +57,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

I fail to see the point of this change. A detection is, by definition,
automatic, otherwise it's no longer a detection.

> +	  on mainstream hardware.  If unsure, say N.
> +
> +config SENSORS_PCF8575

Please don't include "SENSORS" in the config option name: the PCF8575
is not a sensor chip. I know that other entries are improperly named
but for new entries let's not perpetuate the mistake.

> +	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.23.1/Documentation/dontdiff -x linux-2.6.23.1/arch/x86_64/vdso/vdso.lds orig/linux-2.6.23.1/drivers/i2c/chips/Makefile linux-2.6.23.1/drivers/i2c/chips/Makefile
> --- orig/linux-2.6.23.1/drivers/i2c/chips/Makefile	2007-10-12 18:43:44.000000000 +0200
> +++ linux-2.6.23.1/drivers/i2c/chips/Makefile	2007-10-23 10:10:31.000000000 +0200
> @@ -10,6 +10,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.23.1/Documentation/dontdiff -x linux-2.6.23.1/arch/x86_64/vdso/vdso.lds orig/linux-2.6.23.1/drivers/i2c/chips/pcf8575.c linux-2.6.23.1/drivers/i2c/chips/pcf8575.c
> --- orig/linux-2.6.23.1/drivers/i2c/chips/pcf8575.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.23.1/drivers/i2c/chips/pcf8575.c	2007-10-23 12:46:13.000000000 +0200
> @@ -0,0 +1,274 @@
> +/*
> +  pcf8575.c - Part of lm_sensors, Linux kernel modules for hardware
> +  monitoring

No, this driver is not part of lm_sensors at all.

> +  Copyright (c) 2000  Frodo Looijaard <frodol at dds.nl>,
> +  Philip Edelbrock <phil at netroedge.com>,
> +  Dan Eaton <dan.eaton at rocketlogix.com>
> +  Ported to Linux 2.6 by Aurelien Jarno <aurel32 at debian.org> with
> +  the help of Jean Delvare <khali at linux-fr.org>

This is confusing. As I understand it, these are the copyrights of the
pcf8574 driver, NOT those of the new pcf8575 driver. Please reorder the
statements to make it clearer:

Copyright 2006 Michael Hennerich blah

Based on pcf8574.c by Aurelien Jarno (the other persons listed really
don't own much of the current code.)

> +
> +  Copyright (C) 2006 Michael Hennerich, Analog Devices Inc.
> +  <hennerich at blackfin.uclinux.org>
> +  based on the pcf8574.c

Either "on pcf8574.c" or "on the pcf8574 driver".

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

"a 16-bit"

The common usage is to use uppercase letters when speaking about the
chip (PCF8575) and lowercase letters for the driver (pcf8575).

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

Just one space before "=".

> +
> +/* 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];

What is "buf" for? You seem to only use 2 bytes, not 3, and it's always
for local use, so I don't see why you need to store the values in this
structure. Just use a local buffer?

> +};
> +
> +static int pcf8575_attach_adapter(struct i2c_adapter *adapter);
> +static int pcf8575_detect(struct i2c_adapter *adapter, int address, int kind);
> +static int pcf8575_detach_client(struct i2c_client *client);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver pcf8575_driver = {
> +	.driver = {
> +	       .owner = THIS_MODULE,
> +	       .name = "pcf8575",

Please use tabs for indentation.

> +	},
> +	.attach_adapter  = pcf8575_attach_adapter,
> +	.detach_client   = pcf8575_detach_client,

Please use tabs for "=" alignment.

> +};
> +
> +/* following are the sysfs callback functions */
> +static ssize_t show_read(struct device *dev,
> +		       struct device_attribute *attr,
> +		       char *buf)

Strange indentation. If you add spaces, this should be to align
"struct" and "char" with the first "struct". If not, then don't add
spaces.

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

Suggest parentheses around "<<".

> +
> +	return sprintf(buf, "%u\n", val);

%hu

> +}
> +
> +static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
> +
> +static ssize_t show_write(struct device *dev,
> +		       struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct pcf8575_data *data = i2c_get_clientdata(to_i2c_client(dev));

Can be written: data = dev_get_drvdata(dev);

> +	return sprintf(buf, "%u\n", data->write);

%hu

The value of data->write is uninitialized when you load the driver, you
you're returning a random value here, that's not correct. Please return
-EAGAIN in this case, as the pcf8574 driver does.

> +}
> +
> +static ssize_t set_write(struct device *dev,
> +		       struct device_attribute *attr,
> +		       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,
> +		       struct device_attribute *attr,
> +		       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;

Spaces around "<<".

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

"Show" callback should be NULL, not show_write.

> +
> +static ssize_t set_clear_bit(struct device *dev,
> +		       struct device_attribute *attr,
> +		       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;

Spaces around "<<".

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

"Show" callback should be NULL, not show_write.

These last two functions look unsafe to me. The 16-bit value you get
from i2c_master_recv() gives you the current line states. For pins
which are in input mode, this can be either 0 or 1. Then you'll write
the same value back (except for one bit), which will _change_ some of
these pins from input mode to output mode. I doubt that this side
effect is intended.

If anything, you should use the saved value data->write as the base for
the value you're going to write, so that you don't change the pin
directions. But frankly, I can't see why you need sysfs attributes for
this, as the same can be done from user-space by reading the "write"
attribute, changing one bit and writing back to the "write" attribute.

> +
> +/*
> + * 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,
> +		       int kind)
> +{
> +	struct i2c_client *new_client;

Please rename to just "client". This "new_client" name has spread from
driver to driver for years and it's about time to stop it.

> +	struct pcf8575_data *data;
> +	int err = 0;
> +	const char *client_name = "";

Useless initialization. You don't actually need a variable at all.

> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> +	       goto exit;

Not correct. You are doing "low-level" i2c transactions, so you much
check for I2C_FUNC_I2C, not I2C_FUNC_SMBUS_BYTE.

> +
> +	/* 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;

Useless initialization, kzalloc did it for you.

> +
> +	/* Now, we would do the remaining detection. But the pcf8575 is plainly
> +	  impossible to detect! Stupid chip. */
> +
> +

Doubled blank line.

> +	client_name = "pcf8575";
> +
> +	/* Fill in the remaining client fields and put into the global list */

The second part of this comment can be dropped. Not that the first part
is that useful either...

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

Doubled blank line.

> +	/* 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;

That's not correct: if an error occurs, you must remove the files which
have been created. I suggest using an attribute group (as the pcf7574
driver does), it'll make the code clearer.

> +
> +exit_free:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int pcf8575_detach_client(struct i2c_client *client)
> +{
> +	int err;
> +

You must remove the sysfs attributes here.

> +	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 at dds.nl>, "
> +	     "Philip Edelbrock <phil at netroedge.com>, "
> +	     "Dan Eaton <dan.eaton at rocketlogix.com> "
> +	     "and Aurelien Jarno <aurelien at aurel32.net>"
> +	     "Michael Hennerich <hennerich at blackfin.uclinux.org>"
> +	     "Bart Van Assche <bart.vanassche at gmail.com>");

Just Michael Hennerich and yourself, please.

> +MODULE_DESCRIPTION("pcf8575 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(pcf8575_init);
> +module_exit(pcf8575_exit);

You driver needs a module parameter to work. This should be documented.
See Documentation/i2c/chips/pcf8574, maybe you can do something similar
for your driver.

-- 
Jean Delvare



More information about the i2c mailing list