[i2c] i2c-remove-redundant-i2c_client-list.patch
Jean Delvare
khali at linux-fr.org
Tue Jan 8 15:18:17 CET 2008
Hi David,
On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> On Sunday 06 January 2008, Jean Delvare wrote:
> > > +static struct i2c_client *verify_client(struct device *dev)
> > > +{
> > > + if (dev->bus != &i2c_bus_type)
> >
> > Is this paranoia, or can this test really succeed? I thought that all
> > children of an i2c adapter node would always be i2c clients.
>
> Let's call it well-founded paranoia. I know that when the SPI
> code got a "remove class_device" patch, I had to fix an oops
> caused by an unexpected child ... and at the top of my current
> GIT snapshot is 911833440b498e3e4fe2f12c1ae2bd44400c7004 which
> fixed similar oopsing in SCSI.
OK, fine with me.
> > > --- a/drivers/i2c/i2c-dev.c
> > > +++ b/drivers/i2c/i2c-dev.c
> > > ...
> > > - }
> > > - mutex_unlock(&adapter->clist_lock);
> > > + if (!dev->bus || strcmp("i2c", dev->bus->name) != 0)
> >
> > Please just export i2c_bus_type is you need it, or even verify_client
> > (then renamed to i2c_verify_client)? If the bus type check is really
> > needed then we'll need it for the 3 v4l drivers I mentioned earlier
> > anyway.
>
> Good point. I didn't like this part much, and that function can
> have other uses. I updated kobj_to_i2c_client() to use it too.
I wouldn't change kobj_to_i2c_client(). Drivers using it already know
that their kobj is an i2c_client, so there's no need to check for that,
and the use cases I've seen are in runtime paths that should be fast, I
don't want to slow them down without a good reason.
> > Alternatively we could write and export an i2c_for_each_client()
> > function doing all the required checks so that drivers don't have to
> > care. What do you think?
>
> Less good. We can't know in advance which things they care about.
device_for_each_child() doesn't know either, and lets the caller pass
an arbitrary pointer as a parameter to the callbask function. My idea
was to do something similar, so i2c_for_each_client() would essentially
be a wrapper for device_for_each_child().
Here's what I have:
---
drivers/i2c/i2c-core.c | 28 ++++++++++++++++++++++++++++
drivers/i2c/i2c-dev.c | 10 ++++------
drivers/media/video/dpc7146.c | 8 ++------
drivers/media/video/mxb.c | 8 ++------
drivers/media/video/tvmixer.c | 9 +++------
include/linux/i2c.h | 3 +++
6 files changed, 42 insertions(+), 24 deletions(-)
--- linux-2.6.24-rc7.orig/drivers/i2c/i2c-core.c 2008-01-08 14:19:39.000000000 +0100
+++ linux-2.6.24-rc7/drivers/i2c/i2c-core.c 2008-01-08 15:09:38.000000000 +0100
@@ -215,6 +215,34 @@ struct i2c_client *i2c_verify_client(str
}
EXPORT_SYMBOL(i2c_verify_client);
+struct i2c_for_each_client_data {
+ void *data;
+ int (*fn)(struct i2c_client *, void *);
+};
+
+static int __i2c_for_each_client(struct device *dev, void *d)
+{
+ struct i2c_for_each_client_data *_data = d;
+ struct i2c_client *client = i2c_verify_client(dev);
+
+ if (!client)
+ return 0;
+
+ return _data->fn(client, _data->data);
+}
+
+int i2c_for_each_client(struct i2c_adapter *adapter, void *data,
+ int (*fn)(struct i2c_client *, void *))
+{
+ struct i2c_for_each_client_data _data;
+
+ _data.data = data;
+ _data.fn = fn;
+
+ return device_for_each_child(&adapter->dev, &_data,
+ __i2c_for_each_client);
+}
+EXPORT_SYMBOL(i2c_for_each_client);
/**
* i2c_new_device - instantiate an i2c device for use with a new style driver
--- linux-2.6.24-rc7.orig/include/linux/i2c.h 2008-01-08 14:19:15.000000000 +0100
+++ linux-2.6.24-rc7/include/linux/i2c.h 2008-01-08 15:09:34.000000000 +0100
@@ -181,6 +181,9 @@ struct i2c_client {
extern struct i2c_client *i2c_verify_client(struct device *dev);
+extern int i2c_for_each_client(struct i2c_adapter *adapter, void *data,
+ int (*fn)(struct i2c_client *, void *));
+
static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
{
return i2c_verify_client(container_of(kobj, struct device, kobj));
--- linux-2.6.24-rc7.orig/drivers/media/video/dpc7146.c 2008-01-08 14:37:41.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/dpc7146.c 2008-01-08 14:59:28.000000000 +0100
@@ -87,13 +87,9 @@ struct dpc
int cur_input; /* current input */
};
-static int dpc_check_clients(struct device *dev, void *data)
+static int dpc_check_clients(struct i2c_client *client, void *data)
{
struct dpc* dpc = data;
- struct i2c_client *client = i2c_verify_client(dev);
-
- if( !client )
- return 0;
if( I2C_SAA7111A == client->addr )
dpc->saa7111a = client;
@@ -128,7 +124,7 @@ static int dpc_probe(struct saa7146_dev*
}
/* loop through all i2c-devices on the bus and look who is there */
- device_for_each_child(&dpc->i2c_adapter.dev, dpc, dpc_check_clients);
+ i2c_for_each_client(&dpc->i2c_adapter, dpc, dpc_check_clients);
/* check if all devices are present */
if( 0 == dpc->saa7111a ) {
--- linux-2.6.24-rc7.orig/drivers/media/video/mxb.c 2008-01-08 14:37:33.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/mxb.c 2008-01-08 15:00:12.000000000 +0100
@@ -149,13 +149,9 @@ struct mxb
static struct saa7146_extension extension;
-static int mxb_check_clients(struct device *dev, void *data)
+static int mxb_check_clients(struct i2c_client *client, void *data)
{
struct mxb* mxb = data;
- struct i2c_client *client = i2c_verify_client(dev);
-
- if( !client )
- return 0;
if( I2C_ADDR_TEA6420_1 == client->addr )
mxb->tea6420_1 = client;
@@ -218,7 +214,7 @@ static int mxb_probe(struct saa7146_dev*
}
/* loop through all i2c-devices on the bus and look who is there */
- device_for_each_child(&mxb->i2c_adapter.dev, mxb, mxb_check_clients);
+ i2c_for_each_client(&mxb->i2c_adapter, mxb, mxb_check_clients);
/* check if all devices are present */
if( 0 == mxb->tea6420_1 || 0 == mxb->tea6420_2 || 0 == mxb->tea6415c
--- linux-2.6.24-rc7.orig/drivers/media/video/tvmixer.c 2008-01-08 14:34:30.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/tvmixer.c 2008-01-08 15:01:27.000000000 +0100
@@ -235,18 +235,15 @@ static const struct file_operations tvmi
/* ----------------------------------------------------------------------- */
-static int __tvmixer_adapters(struct device *dev, void *data)
+static int __tvmixer_adapters(struct i2c_client *client, void *data)
{
- struct i2c_client *client = i2c_verify_client(dev);
-
- if (client)
- tvmixer_clients(client);
+ tvmixer_clients(client);
return 0;
}
static int tvmixer_adapters(struct i2c_adapter *adap)
{
- device_for_each_child(&adap->dev, NULL, __tvmixer_adapters);
+ i2c_for_each_client(adap, NULL, __tvmixer_adapters);
return 0;
}
--- linux-2.6.24-rc7.orig/drivers/i2c/i2c-dev.c 2008-01-08 14:19:15.000000000 +0100
+++ linux-2.6.24-rc7/drivers/i2c/i2c-dev.c 2008-01-08 15:05:25.000000000 +0100
@@ -182,14 +182,12 @@ static ssize_t i2cdev_write (struct file
return ret;
}
-static int i2cdev_check(struct device *dev, void *addrp)
+static int i2cdev_check(struct i2c_client *client, void *addrp)
{
- struct i2c_client *client = i2c_verify_client(dev);
-
- if (!client || client->addr != *(unsigned int *)addrp)
+ if (client->addr != *(unsigned int *)addrp)
return 0;
- return dev->driver ? -EBUSY : 0;
+ return client->dev.driver ? -EBUSY : 0;
}
/* This address checking function differs from the one in i2c-core
@@ -197,7 +195,7 @@ static int i2cdev_check(struct device *d
driver to it, as NOT busy. */
static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
{
- return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
+ return i2c_for_each_client(adapter, &addr, i2cdev_check);
}
static int i2cdev_ioctl(struct inode *inode, struct file *file,
Admittedly it will slow down things a bit as each iteration of the loop
will have one additional level of indirection. This makes the calling
code somewhat simpler though. Whether it is worth the additional
runtime cost, I just don't know. What do you think?
> Though I was a bit worried it might have bugs in it, since all I had
> time to do was code it, and sanity check it with a couple variants of
> one new-style config. (I no longer have those monster "build all I2C
> drivers" configs around.)
I'll give it good testing, don't worry too much about that.
--
Jean Delvare
More information about the i2c
mailing list