[PATCH 0/5] improve i2c probing

[PATCH 0/5] improve i2c probing

[David Brownell]

Hmm, some of this resembles my prototype from last month:

   http://lists.lm-sensors.org/pipermail/lm-sensors/2005-July/013012.html

Both ended up with new driver probe() methods attaching to *devices* not
to busses, and used the probe signature the i2c core already handles.
That helps eliminate one of the surprises hitting anyone starting to use
the I2C driver stack.  But not the more fundamental one...

What would you think about actually making I2C probing work more like
standard driver model probing, instead?  That is, have the probe method
signature look like this:

    int probe(struct i2c_client *dev, void *driver_data)

In normal driver model usage, infrastructure creates the devices, and the
device drivers just bind to them.  But I2C doesn't support that even for
the submodel where it's very appropriate:  devices that have been probed,
or which (as with platform_bus) were explicitly declared to exist.

I2C drivers would either continue the old school way ... or could start
acting more like they do in other mainstream Linux driver frameworks.

- Dave

Re: [PATCH 0/5] improve i2c probing

[Mark Underwood]

Interestingly (we for me at least ;-) I have been
working on an SPI subsystem that was/is a copy of the
I2C subsystem with changes as SPI doesn’t have a
protocol like I2C. I am at the stage of tidying up the
structures in the head files and looking at where they
are used in the core layer.
To me, what I have, like I2C, doesn’t tie in very well
with the new driver model (although I’m not overly
familiar with it, I think I finally understand
platform devices :-). The current I2C core layer seems
to be only registering a bus type and devices so you
can see them in sysfs as none of the functions seem to
do anything.
I wonder how much work the new kernel subsystems can
do for us to cut down the size of i2c-core (and thus
also spi-core).
I guess there is no escaping the fact that I’m going
to gave to do some more homework and study the code.
Any thoughts or insights would be very welcome.

Mark

--- David Brownell <david-b@pacbell.net> wrote:

> Hmm, some of this resembles my prototype from last
> month:
> 
>   
>
http://lists.lm-sensors.org/pipermail/lm-sensors/2005-July/013012.html
> 
> Both ended up with new driver probe() methods
> attaching to *devices* not
> to busses, and used the probe signature the i2c core
> already handles.
> That helps eliminate one of the surprises hitting
> anyone starting to use
> the I2C driver stack.  But not the more fundamental
> one...
> 
> What would you think about actually making I2C
> probing work more like
> standard driver model probing, instead?  That is,
> have the probe method
> signature look like this:
> 
>     int probe(struct i2c_client *dev, void
> *driver_data)
> 
> In normal driver model usage, infrastructure creates
> the devices, and the
> device drivers just bind to them.  But I2C doesn't
> support that even for
> the submodel where it's very appropriate:  devices
> that have been probed,
> or which (as with platform_bus) were explicitly
> declared to exist.
> 
> I2C drivers would either continue the old school way
> ... or could start
> acting more like they do in other mainstream Linux
> driver frameworks.
> 
> - Dave
> 
> 
> -
> To unsubscribe from this list: send the line
> "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at 
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



		
___________________________________________________________ 
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com

Re: [PATCH 0/5] improve i2c probing

[David Brownell]

> Date: Sun, 21 Aug 2005 00:33:48 +0100 (BST)
> From: Mark Underwood <basicmark@yahoo.com>
>
> ...
> Interestingly (we for me at least ;-) I have been
> working on an SPI subsystem that was/is a copy of the
> I2C subsystem with changes as SPI doesn't have a
> protocol like I2C. ...
>
> To me, what I have, like I2C, doesn't tie in very well
> with the new driver model (although I'm not overly
> familiar with it, I think I finally understand
> platform devices :-).

Yes, it takes maybe a little while to sort out what the
driver model does for you, especially if you're coming
from whatever strange dimension the I2C model did.  :)


> I wonder how much work the new kernel subsystems can
> do for us to cut down the size of i2c-core (and thus
> also spi-core).
> I guess there is no escaping the fact that I'm going
> to gave to do some more homework and study the code.
> Any thoughts or insights would be very welcome.

Well, I've just posted a sketch of how to use the driver
model in a more traditional way for SPI.  That same
approach could be taken with I2C if/when anyone gets
motivated to make it happen ... except that, unlike SPI,
I2C can actually use hardware probing in common usage.
(It could kick in right after the pre-declared devices
Get initialized.)

- Dave

Re: [PATCH 0/5] improve i2c probing

[Nathan Lutchansky]

On Thu, 18 Aug 2005, Greg KH wrote:
> On Mon, Aug 15, 2005 at 01:51:06PM -0400, Nathan Lutchansky wrote:
> > This patch series makes a couple of improvements to the i2c device
> > probing process.
> 
> <snip>
> 
> These all generally look quite good, thanks.  But it looks like you and
> Jean went back and forth on a few issues, care to repost an updated
> series of patches based on that exchange so I can have them get some
> testing in the -mm tree?

Yup, I'm going to break the series into two parts and post the first one 
probably tomorrow.  I haven't been in a hurry since Jean's been away this 
week.  -Nathan

Re: [PATCH 0/5] improve i2c probing

[Greg KH]

On Mon, Aug 15, 2005 at 01:51:06PM -0400, Nathan Lutchansky wrote:
> Hi everyone,
> 
> This patch series makes a couple of improvements to the i2c device
> probing process.

<snip>

These all generally look quite good, thanks.  But it looks like you and
Jean went back and forth on a few issues, care to repost an updated
series of patches based on that exchange so I can have them get some
testing in the -mm tree?

thanks,

greg k-h

Re: [PATCH 0/5] improve i2c probing

[Jean Delvare]

Hi Nathan,

> OK, so I realized a few hours ago that the i2c_probe_device and
> i2c_remove_device interface is probably the wrong way to go about
> things, and it's broken anyway because the instantiated devices don't
> survive if the client driver module is unloaded and reloaded.

There is no way a client would survive a driver module cycling. This is
the whole point of module cycling :) Clients have to be destroyed at the
time the driver module is unloaded, as the driver holds all the client
code. This will happen whatever the implementation, so I don't see what
you think is wrong here.

>                     (...)  With the i2c_probe_device function I was
> attempting to provide a means for video capture card drivers to
> directly instantiate the i2c clients it already knows exist without
> having to probe for them.

You don't need to convince me that i2c_probe_device() or something
similar is a good idea :) Media/video and RTC driver authors have been
complaining for quite some time about the lack of such a mechanism.

> (The i2c_remove_device function was only present for symmetry...)

Symmetry is not needed. If you look at the current code, you'll see that
loading a client driver and unloading it are not similar operations.

> My new (well, old) idea for explicitly instantiating i2c clients,
> instead of i2c_probe_device, is to put a new field into the
> i2c_adapter with a list of (driver id, address, kind) tuples that
> should be force-detected by the i2c core.
> 
>  1. Video capture card driver discovers new capture card
>  2. Driver creates new (unprobed) i2c adapter with this device list:
>      {
>          { I2C_DRIVERID_EEPROM, 0x50, 0 }
>      }
>  3. i2c core does no probing but force-detects an EEPROM at 0x50
>  4. Driver reads EEPROM and updates the device list:
>      {
>          { I2C_DRIVERID_EEPROM, 0x50, 0 }
>          { I2C_DRIVERID_SAA7115, 0x20, 0 }
>          { I2C_DRIVERID_TUNER, 0x61, TUNER_PHILIPS_FM1236_MK3 }
>      }
>  5. Driver somehow notifies the i2c core that the device list changed
>  6. i2c core force-detects the remaining two clients

I don't much like it. You are trying to add to i2c_adapter something
that doesn't belong to it IMHO. Why not simply pass the client force
information to the probe function explicitely, like you originally
planned? Sounds much more modular to me. I really don't get what problem
you are trying to solve with this new proposal.

We most certainly won't get much further without real patches to
illustrate the possibilities anyway.

> > One possibility would be to have an additional class of client, say
> > I2C_CLASS_MISC. This would cover all the chip drivers which do not
> > have a well-defined class, so that every client would *have to*
> > define a class (we could enforce that at core level - I think this
> > was the planned ultimate goal of .class when it was first
> > introduced.)
> 
> I'm not sure I like lumping all the "unclassified" clients together.
> The class mechanism is used to limit probing of an adapter to client
> drivers with similar functions, so putting a bunch of client drivers
> that would never be on the same bus into the same class is kind of
> silly. (You'd never expect to find a keyboard controller on the same
> bus as an IR motion sensor, for example.)

This definitely wouldn't be worse than the current state where these
drivers will probe for clients on all adapters ;)

However, you are right that I2C_CLASS_MISC is a bad idea. We can use
I2C_CLASS_ALL for the exact same purpose, as described below.

Can we establish a list of these unclassified clients, and determine
their needs? I do believe that for example RTC drivers do know exactly
where their clients are, and would be fine with not probing for them,
just like is the case of the media/video drivers. So far, we have been
discussing about adapters that didn't want to be probed, but I now tend
to think that this applies to client drivers as well. In other words, a
client driver not defining a class would mean "never probe" rather than
"probe all busses". This brings back some symmetry into the problem.

The benefit here would be that the class matching code would be reduced
to its most simple expression, with no exceptions. Adapters and client
drivers which are fine with the automatic probing mechanism would *have
to* define their classes. Adapters and client drivers which do not want
the i2c-core to handle client registrations for them simply do not
define their class, and handle everything on their own (through
attach_adapter().)

It is probably worth reminding the semantics for the i2c_adapter class.
It doesn't define the kind of devices that can be found on this bus, but
instead the kind of client devices the i2c-core is *allowed to probe
for* on this bus. I hope that we agree on this. Likewise, the i2c_driver
class is not about the kind of chip the driver is for, but about which
busses this client driver wants to attach to in a probed, automated way.

> > If I2C_CLASS_MISC doesn't please you, then we can simply keep the
> > idea that i2c_adapters that do not want to be probed do not define a
> > class.
> 
> I like this better.  :-)  This kind of implies, though, that i2c
> adapters that define *any* class would be probed by drivers that had
> no class defined.  This might be the correct way to go about it. 

On second thought, I don't think so. The fact that adding a given class
to an adapter would allow client drivers that do *not* define this class
to probe the bus is a weird side effect, and hard to justify. But
fortunately my new proposal doesn't suffer from this problem.

I propose that we go for the most simple and symmetrical model.
i2c_add_driver and i2c_add_adapter call driver->attach_adapter() if it
is defined, then call i2c_probe() if everything needed is defined and if
(and only if) driver->class & adap->class is non-zero. Adapter or client
drivers that really want to accept all clients or to connect to all
adapters, resepectively, can use I2C_CLASS_ALL, although I wouldn't
expect many to do. Note that this is only the expression of an
acceptance, a client driver with I2C_CLASS_ALL will still not connect to
an adapter with no class defined (and that's exactly what we want, isn't
it?)

I don't think we need anything more complex than that, unless someone
can point out a need that my proposal doesn't fulfill.

Of course, we will need your i2c_probe_device() function (or anything
equivalent, but it looked just fine to me) for the non-probed client
registrations.

Thanks,
-- 
Jean Delvare

Re: [PATCH 0/5] improve i2c probing

[Nathan Lutchansky]

On Mon, Aug 15, 2005 at 11:39:58PM +0200, Jean Delvare wrote:
> > The second improvement (which is really the point of this patch set)
> > is to add the functions i2c_probe_device and i2c_remove_device for
> > directly creating and destroying i2c clients on a particular adapter:
> > 
> >     int i2c_probe_device(struct i2c_adapter *adapter, int driver_id,
> >                          int addr, int kind);
> >     int i2c_remove_device(struct i2c_adapter *adapter, int driver_id,
> >                           int addr);
> > 
> 
> I think I understand the point of i2c_probe_device(). However, it would
> help if you could additionally show how this is going to help the
> media/video drivers. Currently, all these drivers use the traditional
> probing mecanism, and have to jam "foreign" probes, right? I would hope
> that these two patches will make it possible to improve this. Can you
> provide a few  examples of use? We need to figure out how good this new
> interface/mechanism is, and this can only be done with concrete
> examples.

OK, so I realized a few hours ago that the i2c_probe_device and
i2c_remove_device interface is probably the wrong way to go about
things, and it's broken anyway because the instantiated devices don't
survive if the client driver module is unloaded and reloaded.

Here's what I'm after.  Devices like video capture cards often have
on-board i2c buses for controlling chips like the video decoder and TV
tuner.  The devices connected to these buses and their addresses can
almost always be determined by the PCI/USB ID of the card or by reading
an on-board EEPROM.  With these special-purpose i2c buses, there's
really no need to do any i2c probing, but we've always been forced to
use probing anyway because that's the only way to instantiate new i2c
clients.  With the i2c_probe_device function I was attempting to provide
a means for video capture card drivers to directly instantiate the i2c
clients it already knows exist without having to probe for them.  (The
i2c_remove_device function was only present for symmetry...)

My new (well, old) idea for explicitly instantiating i2c clients,
instead of i2c_probe_device, is to put a new field into the i2c_adapter
with a list of (driver id, address, kind) tuples that should be
force-detected by the i2c core.

 1. Video capture card driver discovers new capture card
 2. Driver creates new (unprobed) i2c adapter with this device list:
     {
         { I2C_DRIVERID_EEPROM, 0x50, 0 }
     }
 3. i2c core does no probing but force-detects an EEPROM at 0x50
 4. Driver reads EEPROM and updates the device list:
     {
         { I2C_DRIVERID_EEPROM, 0x50, 0 }
         { I2C_DRIVERID_SAA7115, 0x20, 0 }
         { I2C_DRIVERID_TUNER, 0x61, TUNER_PHILIPS_FM1236_MK3 }
     }
 5. Driver somehow notifies the i2c core that the device list changed
 6. i2c core force-detects the remaining two clients

I'd really like to model the known-device list after the PCI subsystem
and other buses that track what devices "should" be connected based on
configuration information from the bus's host adapter, but I haven't had
time to research it yet.

> I am not totally convinced by the reintroduction of the i2c_adapter
> flags. I hope we can do without it.
> 
> One possibility would be to have an additional class of client, say
> I2C_CLASS_MISC. This would cover all the chip drivers which do not have
> a well-defined class, so that every client would *have to* define a
> class (we could enforce that at core level - I think this was the
> planned ultimate goal of .class when it was first introduced.)

I'm not sure I like lumping all the "unclassified" clients together.
The class mechanism is used to limit probing of an adapter to client
drivers with similar functions, so putting a bunch of client drivers that
would never be on the same bus into the same class is kind of silly.
(You'd never expect to find a keyboard controller on the same bus
as an IR motion sensor, for example.)

> If I2C_CLASS_MISC doesn't please you, then we can simply keep the idea
> that i2c_adapters that do not want to be probed do not define a class.

I like this better.  :-)  This kind of implies, though, that i2c
adapters that define *any* class would be probed by drivers that had no
class defined.  This might be the correct way to go about it.  -Nathan

Re: [PATCH 0/5] improve i2c probing

[Jean Delvare]

Hi Nathan,

> Virtually every i2c client driver uses exactly the same code, so
> there's little point in requiring them all to implement this callback.
> The first patch in this series adds two new fields to the i2c_driver
> structure, `address_data' and `detect_client', and if they are set by
> the driver, the i2c core will automatically call i2c_probe using those
> fields as the second and third argument.

This sounds very good to me. I have been wanting to do that some times
ago, but for some reason didn't. Most probably I was waiting to be done
with the various other i2c-core cleanups that were needed before (and
this just happened.)

> If the `class' field of the i2c_driver structure is set, it will be
> compared with the adapter class first.

I'm not sure I agree with the "if" clause, more about that below.

> Patches 2 and 3 add these fields to the i2c_driver initializer in the
> i2c hwmon and misc i2c chip drivers and remove the corresponding
> attach_adapter callbacks.

Looks good to me too.

> The second improvement (which is really the point of this patch set)
> is to add the functions i2c_probe_device and i2c_remove_device for
> directly creating and destroying i2c clients on a particular adapter:
> 
>     int i2c_probe_device(struct i2c_adapter *adapter, int driver_id,
>                          int addr, int kind);
>     int i2c_remove_device(struct i2c_adapter *adapter, int driver_id,
>                           int addr);
> 
> These functions make the i2c subsystem usable for special-purpose i2c
> buses where probing isn't possible, either because probing is known to
> be dangerous for devices that are present on the bus, or because the
> i2c adapter lacks quick writes and/or error reporting.
> 
> The final patch adds a new i2c adapter flag to indicate that the
> adapter should never be probed.

I think I understand the point of i2c_probe_device(). However, it would
help if you could additionally show how this is going to help the
media/video drivers. Currently, all these drivers use the traditional
probing mecanism, and have to jam "foreign" probes, right? I would hope
that these two patches will make it possible to improve this. Can you
provide a few  examples of use? We need to figure out how good this new
interface/mechanism is, and this can only be done with concrete
examples.

I do not understand the point of i2c_remove_device() though. You
shouldn't have to search for the clients you have been registering
explicitely before. Instead, you should keep a reference to them (which
suggests that maybe i2c_remove_device should return an i2c_client*
instead of an int.) With that reference, you should be able to
unregister the client directly, which is much more efficient that
walking all the list of clients for that adapter.

I am although a bit surprised that you are passing the adapter as an
i2c_adapter* and the driver as an id. Why not passing the driver as a
i2c_driver*? Or both the adapter and the driver as ids? This is a truly
naïve question, I just don't know. This is where concrete examples
really help.

I am not totally convinced by the reintroduction of the i2c_adapter
flags. I hope we can do without it.

One possibility would be to have an additional class of client, say
I2C_CLASS_MISC. This would cover all the chip drivers which do not have
a well-defined class, so that every client would *have to* define a
class (we could enforce that at core level - I think this was the
planned ultimate goal of .class when it was first introduced.) Adapters
that do not want to be probed would then simply *not* define a class, so
the class test would always fail for them, and no client could attach
through the "automatic" probe mechanism.

If I2C_CLASS_MISC doesn't please you, then we can simply keep the idea
that i2c_adapters that do not want to be probed do not define a class.
This alone could be a condition to skip any probing to that adapters, we
just have to make an explicit test on i2c_adapter.id not being 0 before
going on with generic probing.

Another possibility, in case you really don't like neither the first one
nor its variant, would be to keep two separate lists of i2c adapters,
one with those which want probing, and one with those which don't. As it
seems that these are two very different types of adapters, on which we
will use completely different probing functions, this seems to make
sense, and should speed up the processing on both sides. I admit I don't
know if doing so would have implications at the i2c-core level. Maybe
Greg can comment on this idea?

I have additional comments on the code itself, which I will post as
replies to each patch.

Thanks,
-- 
Jean Delvare

[PATCH 0/5] improve i2c probing

[Nathan Lutchansky]

Hi everyone,

This patch series makes a couple of improvements to the i2c device
probing process.

Currently, when a new i2c bus needs to be probed, the i2c subsystem
calls the attach_adapter callback for each loaded i2c client driver,
which must call the i2c_probe function with a list of addresses to be
probed and another callback for reporting detected devices:

    static int foo_attach_adapter(struct i2c_adapter *adapter)
    {
            if (!(adapter->class & I2C_CLASS_HWMON))
                    return 0;
            return i2c_probe(adapter, &addr_data, foo_detect);
    }

Virtually every i2c client driver uses exactly the same code, so there's
little point in requiring them all to implement this callback.  The
first patch in this series adds two new fields to the i2c_driver
structure, `address_data' and `detect_client', and if they are set by
the driver, the i2c core will automatically call i2c_probe using those
fields as the second and third argument.  If the `class' field of the
i2c_driver structure is set, it will be compared with the adapter class
first.

Patches 2 and 3 add these fields to the i2c_driver initializer in the
i2c hwmon and misc i2c chip drivers and remove the corresponding
attach_adapter callbacks.

The second improvement (which is really the point of this patch set) is
to add the functions i2c_probe_device and i2c_remove_device for directly
creating and destroying i2c clients on a particular adapter:

    int i2c_probe_device(struct i2c_adapter *adapter, int driver_id,
                         int addr, int kind);
    int i2c_remove_device(struct i2c_adapter *adapter, int driver_id,
                          int addr);

These functions make the i2c subsystem usable for special-purpose i2c
buses where probing isn't possible, either because probing is known to
be dangerous for devices that are present on the bus, or because the i2c
adapter lacks quick writes and/or error reporting.

The final patch adds a new i2c adapter flag to indicate that the adapter
should never be probed.

This patch set applies cleanly to the end of Greg KH's i2c patch queue,
as of 12-Aug-2005.  -Nathan

 Documentation/i2c/writing-clients |   58 ++++++++++++++---------
 drivers/hwmon/adm1021.c           |   12 +---
 drivers/hwmon/adm1025.c           |   12 +---
 drivers/hwmon/adm1026.c           |   13 +----
 drivers/hwmon/adm1031.c           |   13 +----
 drivers/hwmon/adm9240.c           |   12 +---
 drivers/hwmon/asb100.c            |   17 +-----
 drivers/hwmon/atxp1.c             |    9 ---
 drivers/hwmon/ds1621.c            |   10 ----
 drivers/hwmon/fscher.c            |   12 +---
 drivers/hwmon/fscpos.c            |   12 +---
 drivers/hwmon/gl518sm.c           |   12 +---
 drivers/hwmon/gl520sm.c           |   12 +---
 drivers/hwmon/it87.c              |   17 +-----
 drivers/hwmon/lm63.c              |   12 +---
 drivers/hwmon/lm75.c              |   13 +----
 drivers/hwmon/lm77.c              |   13 +----
 drivers/hwmon/lm78.c              |   17 +-----
 drivers/hwmon/lm80.c              |   12 +---
 drivers/hwmon/lm83.c              |   12 +---
 drivers/hwmon/lm85.c              |   12 +---
 drivers/hwmon/lm87.c              |   12 +---
 drivers/hwmon/lm90.c              |   12 +---
 drivers/hwmon/lm92.c              |   11 +---
 drivers/hwmon/max1619.c           |   12 +---
 drivers/hwmon/w83781d.c           |   17 +-----
 drivers/hwmon/w83792d.c           |   18 +------
 drivers/hwmon/w83l785ts.c         |   12 +---
 drivers/i2c/chips/ds1337.c        |    9 ---
 drivers/i2c/chips/ds1374.c        |    8 ---
 drivers/i2c/chips/eeprom.c        |   10 ----
 drivers/i2c/chips/m41t00.c        |    9 ---
 drivers/i2c/chips/max6875.c       |   10 ----
 drivers/i2c/chips/pca9539.c       |   10 ----
 drivers/i2c/chips/pcf8574.c       |   10 ----
 drivers/i2c/chips/pcf8591.c       |   10 ----
 drivers/i2c/chips/rtc8564.c       |    8 ---
 drivers/i2c/i2c-core.c            |   95 ++++++++++++++++++++++++++++++++++++--
 include/linux/i2c.h               |   21 ++++++++
 39 files changed, 246 insertions(+), 360 deletions(-)