[i2c] [patch 1/5] i2c stack can probe()

[Jean Delvare]

Hi David,

On Fri, 2 Mar 2007 17:37:14 -0800, David Brownell wrote:
> On Friday 02 March 2007 12:15 pm, Jean Delvare wrote:
> > On Thu, 15 Feb 2007 00:41:37 -0800, David Brownell wrote:
> > >  static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > >  {
> > > -	return 1;
> > > +	struct i2c_client	*client = to_i2c_client(dev);
> > > +	struct i2c_driver	*driver = to_i2c_driver(drv);
> > > +
> > > +	/* legacy i2c drivers bypass driver model probing entirely;
> > > +	 * such drivers scan each i2c adapter/bus themselves.
> > > +	 */
> > > +	if (!driver->probe)
> > > +		return 0;
> > > +
> > > +	/* new style drivers use the same driver matching policy as
> > > +	 * platform devices or SPI:  compare device and driver names.
> > > +	 */
> > > +	return strcmp(client->name, drv->name) == 0;
> > >  }
> > 
> > We still need to discuss this. I think this approach is too simplistic
> > for some of our i2c drivers. Many of them support several device types, 
> > and so far we have used a different client name for each of them (in
> > most cases.)
> 
> If the issue is that client->name is non-constant, that could easily
> be addressed by switching to a new field, initialized only by new-style
> drivers.  So the device-to-driver association would be as found in
> this patch (other than using that field not client->name), but the
> driver could expose additional typing information in sysfs if they
> really wanted to do so.
> 
> In fact I was wondering if maybe a new field should be defined for
> that specific purpose.

I'm not sure I understand what you mean with "non-constant".

libsensors and other tools pretty much rely on the fact that the chips
expose their name through the "name" sysfs attribute, so indeed
changing this will break the compatibility. This isn't necessarily a
blocker though, as the compatibility will pretty much be broken by the
class_device changes anyway. But I'd like to make the migration as
smooth as possible, and it's convenient for user-space to know which
device they have exactly (both for the end users and for us as
supporters), so I see little point in changing it.

OTOH there's nothing wrong with having one string (the driver name)
presented in "modalias" and another string (the chip name) presented in
"name", is there?

So indeed I believe we want a new field (const char *) in i2c_client to
store the name of the driver they need. The name field is currently
used for something different. Given that legacy clients wouldn't have
this field set, you could even use this as a discriminant between new
style and legacy clients, this could be handy.

> > Another (possibly related) thing to consider here is that we will need
> > a way to instantiate i2c clients from user-space. The hardware
> > monitoring drivers let us do that at the moment, this is very useful
> > when detection doesn't work for any reason, and I don't want to lose
> > the functionality in the migration. How do you plan to implement that
> > in the new model (if you thought about it at all)?
> 
> I focussed on step #1 (getting standard kerne driver model support), and
> now you're asking about steps #2 and #3?  Geez.  ;)

No, I'm not asking you to do it now. Actually I might be the one who
does it, later, as hardware monitoring drivers will need it. I'm merely
asking that we think about it now, so that we don't have bad surprises
later. I would hate to have to rewrite parts of the changes you're
doing now and break compatibility again after discovering that your
model misses something which we really need and can't do without.

> I don't see all the legacy drivers converting overnight.  As with all
> such migrations, issues turn up that need to be addressed.  I won't pretend
> to own a crystal ball (that works).  In the interim, I expect all of the
> current solutions will continue to work.

Fair enough, but again I want the transition to be as smooth as
possible. I want to encourage driver authors to port their code to the
new model. Otherwise we'll need to maintain the two models pretty much
forever, and this isn't how the Linux kernel works.

> As for instantiating a new-style client from userspace, one way would be
> to write the i2c_board_info data into a per-adapter sysfs file.  That would
> not be able to handle platform_data of any complexity, or IRQs, of course.

This is exactly the point I was coming to. First you say that the
handling of different chip types belongs to platform_data, and now you
say that platform data will not be settable from user-space. This
pretty much means the impossibility to instantiate clients from
user-space in the case where the chip type cannot be probed by the
driver, while the whole point of allowing user-space to instantiate
clients is exactly to workaround detection problems. So we have a
problem here.

I have two ideas to solve it:

* In order to port the i2c-based hardware monitoring drivers to the new
model, we'll need to move the probing and client instantiation to a
separate place. If we decide in favor of a kernel module, then we can
add parameters to this kernel module to influence its probing decisions.

I'm not too sure about this one, because I'm not sure if we want to do
the probing in kernel space at all - this needs to be discussed.

* We could have a sysfs file for each i2c_adapter, somewhat similar to
the "new_id" the PCI devices have. Say we name our file "new_chip", and
people can write something like "0x4c lm90 adm1032" to it to
instantiate an i2c_client at address 0x4c, using driver lm90, with
device type adm1032.

Likewise we could have a "remove_chip" file to un-instantiate clients.

In both cases, the idea is to actually let user-space provide
platform_data-like information, contrary to what you suggested above.
Comments?

-- 
Jean Delvare