On Fri, 12 Sep 2014, Javier Martinez Canillas wrote:

[adding Sjoerd as cc who was the one that raised the module auto-loading issue]

Hello,

On Fri, Sep 12, 2014 at 3:46 PM, Wolfram Sang [off-list ref] wrote:

quoted

quoted

Placing this firmly back on your plate.  I truly hope we don't miss
another merge-window.  This patch-set has the support of some pretty
senior kernel maintainers, so I hope acceptance shouldn't be too
difficult.

As previously discussed I believe it should be okay for an I2C device
driver _not_ supply an I2C ID table to match to.  The I2C subsystem
should be able to match via other means, such as via OF tables.  The
blocking factor during our previous conversation was to keep
registering via sysfs up and running.  This set does that.

As mentioned in another thread, modaliases are one other possible side
effect. As Javier correctly mentions, the beaviour does not really
change with your patchset. Yet, if we remove i2c_device_id from drivers
too carelessly, they might not be bound anymore.

Right, removing the I2C ID table even from drivers that don't really
need it (e.g: IP blocks only present in DT platforms) will as you said
break module auto-loading. Probing will work since the OF table is
used to match the device in i2c_device_match() but is the I2C table
what is used to fill the valid module aliases with the current
behavior of i2c_device_uevent(), the aliases filled from the OF table
are never used.

So what I propose is to do it incrementally:

1) Merge Lee's series since that is definitely a step in the right
direction to not make an I2C table mandatory (still will be needed for
module auto loading though).

2) On a follow-up series, make sure that all I2C drivers have a proper
OF table and that are using the MODULE_DEVICE_TABLE(of,...) macro to
fill the of:<foo> module aliases. That way the modules will have the
proper aliases of the form "of:<foo>" besides the "i2c:<foo>" one.
(even when always i2c:<foo> is reported to user-space currently).

3) Apply the patch I posted [0] that changes the behavior of
i2c_device_uevent() to report the OF uevent env vars to user-space in
case of DT probing which after 2) should not regress any driver module
auto-loading since all drivers should fill the of:<foo> aliases.

This sounds resonable.

After this, DT-only drivers will only need an OF table and legacy
drivers will only need an I2C table. Drivers that support both will
still need the two tables though which is a drawback of this approach
since unnecessary duplication will exist on these drivers and can
cause issues when both tables are not in sync as we saw on the issue
reported by Sjoerd on [1].

So an alternate approach could be to do the opposite, just remove the
OF tables entirely from the I2C drivers and only use the I2C table
even for DT-only drivers. This is possible since i2c_device_match()
will succeed even without an OF table because i2c_match_id() matches
for compatible strings and what is reported as uevent is what is in
the I2C table anyways. I believe that is what Sjoerd suggested but
I'll let him comment on that in case I misunderstood.

This would be really bad.  It would go completely against what I have
working to achieve and OF conventions.

By the way, the SPI subsystem has the same behavior, I raised the issue on [2].

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/9/11/269
[1]: https://lkml.org/lkml/2014/9/9/100
[2]: https://lkml.org/lkml/2014/9/11/458

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog