[i2c] Unprobable chips and new style i2c drivers

[Jean Delvare]

Hi Jon,

On Tue, 20 Nov 2007 16:22:14 -0500, Jon Smirl wrote:
> On 11/20/07, Jean Delvare <khali at linux-fr.org> wrote:
> > On Sun, 18 Nov 2007 16:55:26 -0500, Jon Smirl wrote:
> > > What do you do for an rtc chip on the x86 platform? Nobody has built
> > > platform specific configurations for the thousands of x86 designs. The
> > > old solution to this was a parameter on the kernel boot command line.
> > > Maybe we should add an 'enable' parameter to new style drivers.
> >
> > Out of curiosity, have you ever seen a RTC chip on I2C on x86?
> 
> No, I just picked x86 since it doesn't have platform descriptions and
> loads everything via probes.

But then your point is no longer that interesting. It's complex enough
to handle cases which really happen without trying to handle cases which
never happen ;) Please come out with real world examples so that we can
have more useful discussions.

> > The matter is what was being discussed in this thread:
> > http://lists.lm-sensors.org/pipermail/i2c/2007-November/002205.html
> >
> > We could add platform-like init code for x86 that would instantiate
> > new-style I2C devices based on DMI data. I'm not sure how maintainable
> > this would be though... This would have to be limited to devices that
> > really cannot be probed, otherwise the list would grow very long very
> > quickly. The advantage would be that things just work at boot. This
> > could also be done in a kernel driver, after boot, however this will
> > break if the device is needed early in the boot process (not sure if we
> > have to care.)
> >
> > Your proposal of adding module command-line parameters to new-style
> > drivers could work as well, however this would have the same drawbacks
> > as the legacy driver code had, for example you can't pass
> > device-specific parameters such as IRQ number that way. Also, this
> > needs specific code in all i2c chip drivers, and I'd rather avoid that.
> > It's everything but elegant.
> 
> How about a very simple module parameter 'enable'. Then build each
> module with its expected irq, address range, etc. Setting enable will
> trigger a scan in this default range. If you want anything outside of
> this you need to use the platform code to setup the device. This is
> similar to the old drivers but less flexible.

"Less flexible" is a pleasant euphemism. It's not flexible at all!
There's no such thing as a "expected IRQ" or "expected address range".
The same I2C chip can be used in completely different ways on different
systems. And the same chip can be present more than once in a given
system, using different setups.

> Using the platform code for setup doesn't work for modprobe since the
> platform code only runs at boot. An 'enable' parameter fixes that.

I don't follow you here. The platform code works just fine for setup,
that's what all embedded platforms are doing. I can't find any relation
between the fact that the setup code runs at boot time and modprobe.
Please explain.

That platform code isn't easily doable on x86 because there are too
many different setups is a different issue.

> Enable just fills in the i2c_board_info structure with the default
> addresses and calls i2c_new_adapter() in the init() function of the
> module.

I guess that you mean i2c_new_device(). It takes an i2c_adapter* as its
first parameter, that's something you do not have at the new-style i2c
chip driver level. Basically you're trying to reimplement the legacy
i2c model on top of the new model. I don't understand why would want to
do that.

> > That's the reason why my current idea to instantiate new-style i2c
> > devices from user-space is a sysfs interface. It would live on i2c
> > adapters (but the code behind would be generic, in i2c-core.) Something
> > like:
> >
> > echo "lm75 0x48" > /sys/class/i2c-adapter/i2c-0/add_device
> 
> This works too, but it is highly frowned upon (GregKH!) to make sysfs
> parameters that need to be parsed. The enable parameter would live on
> each driver. I doubt that he will ever allow a two part attribute like
> this.

I'm taking the idea from the pci subsystem's "new_id" sysfs file. It
was either written by Greg himself or wholeheartedly acked by him ;)
I think that write-only sysfs files have less strict rules.

Anyway, if a sysfs file is really not acceptable for whatever reason,
I'll look into configfs, that's only an implementation detail. The idea
remains the same: let user-space declare I2C devices at the i2c_adapter
level. This complies with the device driver model (drivers do not
instantiate their own devices), contrary to the legacy i2c code and
your "enable" module parameter proposal.

> > Optional device-specific data could be passed as additional parameters.
> > If implemented properly, this should even support chip driver
> > auto-loading. Let's wait for the device naming discussions to settle
> > first, though.
> 
> How would autoloading work? Autoloading will need to check those
> 'compatible' names from the PowerPC device tree. The mapping table
> from PowerPC name to Linux module names is inside the chip module (see
> my patch).

Aliases can be generated at build time and we would feed modprobe with
them, as is done for PCI devices for example. Not everything is in
place yet, but there's no reason why it couldn't be done.

-- 
Jean Delvare