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

[David Brownell]

On Saturday 03 March 2007 3:04 am, Jean Delvare wrote:
> 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".

Not the same value for all devices managed by that driver.


> libsensors and other tools pretty much rely on the fact that the chips
> expose their name

I suspect you mean what I'd call "type".  It probably starts out as
one vendor's chip name, but then as second sources and various kinds
of updates kick in, the "name" starts mattering a lot less... :)


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

Exactly what I was pointing out we could do.


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

I'll update the probe() patch to have a "modalias" field like that,
including the value shown in sysfs, and the i2c_board_info patch to
initialize that.  Also the docs.

As a device field, it won't much help sorting out drivers.  :)

 
> > > 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 was just jerking your chain a little bit.  I'm all in favor of such
planning, where we know what needs to happen.  And hwmon is one of the
better defined families of I2C applications, so I expect more scenarios
to come from there.  The "Embedded I2C" family is all over the map, but
I think I've taken a good whack at addressing its issues already (with
the i2c_board_info stuff).


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

Right.  But I think "don't rush things" applies here too.  For
now I anticipate a few waves of system and driver conversions first,
where the new model seems to address clear deficiencies in the way
things currently work.  The OpenFirmware conversion, support for
various embedded platforms, and RTCs ... that comes quickly to mind.

The picture will become more clear when that dust settles.  The
question you asked is one of several that I expect will need to be
answered.  The answers ought to work together...


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

I didn't say that.  Setting a string value would be easy, even there;
that's not "complex".

And sysfs isn't a good answer to all problems; it might not even be
available ... it's just a quick and obvious tool to look at.


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

An issue to be addressed, sure.  How is it addressed right now?
Module parameters using all those arrays, yes?


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

Note that probing and instantiation are two separate issues.  If some
userspace tool does the probing, then a separate mechanism can be used
to instantiate the devices.

 
> * 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.

That's essentially what I described.  However the PCI (and USB) "new_id"
thing doesn't work that way ... it adds product IDs to *driver* so that
it will match() hardware that its MODULE_DEVICE_TABLE() doesn't list.

What's different about a per-adapter file would be that it's actually
instantiating new devices.


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

You misread my comments.  To get a bit more detailed, I originally
said "write i2c_board_info into a sysfs file", meaning all strings
much like you said.  Assume that file is called "new_chip", and then
matching those board_info fields with your example:

	driver		- "lm90"
	bus_num		- from the adapter
	dev_addr	- 0x4c
	platform_data	- "adm1032" (not a complex datum!!)
	irq		- (unspecified)

I don't see any significant differences between what I sketched and
what you sketched.  Specifying an IRQ would be simple, though currently
I don't think hwmon uses them.

It might be good to defend against such a userspace mechanism storing
bogus crap in platform_data; maybe that type info should be stored in
i2c_client.name instead.  Storing type information in platform_data is
clearly sufficient from the driver perspecive, but it might be easier
overall to add a type string to the board_info.  What do you think?

- Dave



- Dave