Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information in missing drivers

[Javier Martinez Canillas <javier@osg.samsung.com>]

Hello Jean,

On 08/03/2015 06:50 PM, Jean Delvare wrote:
> Le Monday 03 August 2015 à 16:07 +0200, Javier Martinez Canillas a
> écrit :
>> Hello Jean,
>>
>> On 08/03/2015 01:05 PM, Jean Delvare wrote:
>>> Hi Javier,
>>>
>>> On Thu, 30 Jul 2015 18:18:31 +0200, Javier Martinez Canillas wrote:
>>>> The I2C core always reports the MODALIAS uevent as "i2c:<client name"
>>>> regardless if the driver was matched using the I2C id_table or the
>>>> of_match_table. So the driver needs to export the I2C table and this
>>>> be built into the module or udev won't have the necessary information
>>>> to auto load the correct module when the device is added.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>
>>>> ---
>>>>
>>>>  drivers/misc/eeprom/eeprom.c  | 1 +
>>>>  drivers/misc/eeprom/max6875.c | 1 +
>>>>  2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
>>>> index b432873def96..4bb54e1c40a7 100644
>>>> --- a/drivers/misc/eeprom/eeprom.c
>>>> +++ b/drivers/misc/eeprom/eeprom.c
>>>> @@ -203,6 +203,7 @@ static const struct i2c_device_id eeprom_id[] = {
>>>>  	{ "eeprom", 0 },
>>>>  	{ }
>>>>  };
>>>> +MODULE_DEVICE_TABLE(i2c, eeprom_id);
>>>>  
>>>>  static struct i2c_driver eeprom_driver = {
>>>>  	.driver = {
>>>
>>> I seem to recall this one is missing on purpose. The legacy eeprom
>>> driver is deprecated in favor of the at24 driver, so no one should
>>> declare "eeprom" i2c devices and thus the module alias is useless. So I
>>> would leave the legacy eeprom driver alone.
>>>
>>> The only feature the at24 driver is missing is device auto-detection as
>>> far as I know. Maybe it should be added to ease the transition. Or
>>> maybe not, I admit I'm not sure.
>>>
>>
>> It's up to you but since the driver is still in mainline and it has an
>> i2c_device_id table, an "eeprom" I2C device can be registered and matched
>> by the I2C core but if built as a module, it won't be autoloaded.
> 
> The eeprom driver instantiates its own devices, so auto-loading is not
> needed. "eeprom" devices should not be found in device trees, that would
> be a bug. Adding an alias is an invitation to introduce such bugs thus
> my request to not add such an alias.
>

OK.
 
>> I'm not familiar with the at24 platform so feel free to discard the patch
>> if you think that it is not needed and no one is really using this driver
>> (although in that case I think we the driver should just be removed).
> 
> I'm really talking about the at24 eeprom driver
> (drivers/misc/eeprom/at24.c) which has nothing to to with the at91
> platform, if that's what you are confusing with.
>

err, I didn't mention at91 but at24 and that was only because you mentioned
it before :-)

> Yes, the long-term plan is to get rid of the legacy eeprom driver. But
> we need a transition path for users. Either the at24 driver should be
> able to instantiate SPD and EDID devices as the eeprom driver does, or
> we need a user-space helper to do that kind of detection, so that
> consumer scripts such as decode-dimms keep working. The former is a
> smaller change, I just hope it won't have any drawback.
>

I'm in fact not familiar with any Atmel SoC. I just noticed that the I2C
core always repots MODALIAS uevents as i2c:<client->name> issue and so
wrote a script to find all the drivers that could be affected by this.
But I'll just blacklist drivers/misc/eeprom/eeprom.c as a false positive.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America