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

[Jean Delvare]

Hi David,

On Sat, 3 Mar 2007 12:41:00 -0800, David Brownell wrote:
> On Saturday 03 March 2007 3:04 am, Jean Delvare wrote:
> > 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... :)

Yes, it's indeed a "type" and you're right that it is not necessarily
related 1-to-1 to the top marking of the physical chip.

We call the same thing "prefix" in libsensors and in hardware
monitoring drivers documentation (because historically this is the
first part of a hardware monitoring device identifier in the
libsensors/sensors.conf syntax.) And we call it "kind" in the probing
functions in the drivers. And the sysfs file is "name". It's kinda hard
not to get confused... :(

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

Where sysfs isn't available, I'm fairly certain people won't need to
declare i2c devices at runtime.

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

Yeah. I hate them and I'm pretty certain you hate them too (or laugh at
them, at your option). They were probably a nice trick back in 1999
when we supported 5 different hardware monitoring chips or so, but it
just doesn't scale. I wanted to kill them some times ago already but
Greg pointed out that it would go away by itself once we moved to the
device driver model. Which, as you know, didn't quite happen (yet.)

The problem with there parameter arrays is not just that they increase
the driver size dramatically, they are also very fragile. More on that
in the other thread.

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

Yes, I am well aware of that. But instantiating is something that can
only be done in the kernel, so if we probe in user-space we'll need a
user/kernel interface for the instantiation part. This could be the
sysfs interface described below. I don't have that many other ideas, to
say the truth.

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

Yes, I _do_ realize that what we need is something functionally
different from what PCI's new_id does. But I believe the interface
could be somewhat similar.

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

You're right, I somewhat misunderstood your original proposal sorry.

The above looks mostly OK then, with just the exception of the
platform_data / device type part. By definition the format of the
platform_data structure depends on the driver, while I believe we need
an interface which is driver independent. Or would it be acceptable to
wrap all the platform data inside a string, and it would be up to each
driver to convert it to the relevant platform_data structure? I don't
much like the idea that each driver would have to provide its own
conversion function.

OTOH the PCI new_id syntax has room for a driver data integer those
meaning is left to the driver, so maybe we can use a similar idea.

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

Yes, this reinforces my feeling that the device type should be a "core"
attribute of the i2c device definition rather than an optional member
of the platform_data structure. More on that in the other thread where
we started discussing it.

Thanks,
-- 
Jean Delvare