Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1198745 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information  in missing drivers Jean Delvare <jdelvare@suse.de> - 2015-08-03 13:10 +0200
    Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information  in missing drivers Javier Martinez Canillas <javier@osg.samsung.com> - 2015-08-03 16:10 +0200
      Re: [PATCH 06/27] misc: eeprom: Export I2C module alias  information in missing drivers Jean Delvare <jdelvare@suse.de> - 2015-08-03 20:00 +0200
        Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information  in missing drivers Javier Martinez Canillas <javier@osg.samsung.com> - 2015-08-03 21:20 +0200
          Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information  in missing drivers Jean Delvare <jdelvare@suse.de> - 2015-08-04 09:20 +0200
            Re: [PATCH 06/27] misc: eeprom: Export I2C module alias information  in missing drivers Javier Martinez Canillas <javier@osg.samsung.com> - 2015-08-04 09:30 +0200

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

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.

> diff --git a/drivers/misc/eeprom/max6875.c b/drivers/misc/eeprom/max6875.c
> index 580ff9df5529..c74920cc3d18 100644
> --- a/drivers/misc/eeprom/max6875.c
> +++ b/drivers/misc/eeprom/max6875.c
> @@ -197,6 +197,7 @@ static const struct i2c_device_id max6875_id[] = {
>  	{ "max6875", 0 },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(i2c, max6875_id);
>  
>  static struct i2c_driver max6875_driver = {
>  	.driver = {

That one is needed, I agree.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1198876

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.

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

>> diff --git a/drivers/misc/eeprom/max6875.c b/drivers/misc/eeprom/max6875.c
>> index 580ff9df5529..c74920cc3d18 100644
>> --- a/drivers/misc/eeprom/max6875.c
>> +++ b/drivers/misc/eeprom/max6875.c
>> @@ -197,6 +197,7 @@ static const struct i2c_device_id max6875_id[] = {
>>  	{ "max6875", 0 },
>>  	{ }
>>  };
>> +MODULE_DEVICE_TABLE(i2c, max6875_id);
>>  
>>  static struct i2c_driver max6875_driver = {
>>  	.driver = {
> 
> That one is needed, I agree.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.

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

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.

-- 
Jean Delvare
SUSE L3 Support

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199207

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199465

On Mon, 3 Aug 2015 21:12:39 +0200, Javier Martinez Canillas wrote:
> On 08/03/2015 06:50 PM, Jean Delvare wrote:
> > 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.

Again, AT24* are EEPROMs, not SoCs.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199477

Hello Jean,

On 08/04/2015 09:19 AM, Jean Delvare wrote:
> On Mon, 3 Aug 2015 21:12:39 +0200, Javier Martinez Canillas wrote:
>> On 08/03/2015 06:50 PM, Jean Delvare wrote:
>>> 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.
> 
> Again, AT24* are EEPROMs, not SoCs.
>

Oh, sorry I misunderstood. Thanks a lot for the clarification.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web