[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