Re: [PATCH 15/27] regulator: fan53555: Export I2C module alias information

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

Hello Paul,

Thanks a lot for the feedback.

On 08/03/2015 01:43 PM, Paul Bolle wrote:
> Hi Javier,
> 
> (Mark already applied this patch. Still, I couldn't wrap my head around
> it. So maybe you'd still like to answer a question or two, basically to
> educate me.)
> 
> On do, 2015-07-30 at 18:18 +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.
> 
> It's the other way round, I think.
>
> Both MODULE_DEVICE_TABLE() macros add a set of aliases to this module.
> These aliases are used by userspace to load the fan53555 module if it
> notices a uevent that contains the proper "MODALIAS=" string. Only after
> this module is loaded by userspace will the I2C id_table and the
> of_match_table be available to match this driver to the hardware found

You are right on that but...

> in the machine and, if matching hardware is found, call
>  fan53555_regulator_probe() to get this module to actually do something.
> 

...I2C is a little special in that it uses the id_table again to match
in i2c_device_probe() and pass a i2c_device_id to the I2C driver's probe
function. That is what I meant by matching but maybe I could had been more
precise.

> That being said, before this patch the fan53555 module contained these
> aliases:
>     alias:          of:N*T*Csilergy,syr828*
>     alias:          of:N*T*Csilergy,syr827*
>     alias:          of:N*T*Cfcs,fan53555*
> 
> While this patch ad these two aliases:
>     alias:          i2c:syr82x
>     alias:          i2c:fan53555
> 
> Now I don't have an "of" or "i2c" capable machine at hand, which makes
> it a bit hard to figure out how all of this is supposed to fit together.
> But I'm guessing that parsing a device tree blob that contains strings
> like
>     compatible = "silergy,syr828"
> 
> would add strings like
>     MODALIAS=of:N[...]T[...]Csilergy,syr828
>

That would be the correct behavior and is what the RFC patch #27 does.
 
> to the related uevents. (Likewise for the two other aliases.) Doesn't
> that happen here?
>

No, that is exactly the problem. Take a look to i2c_device_uevent() [0],
it just does:

add_uevent_var(env, "MODALIAS=%s%s", I2C_MODULE_PREFIX, client->name))

So if you have a i2c_device_id table but no MODULE_DEVICE_TABLE(i2c,...),
then module autoload won't work.

>> 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.
> 
> s/the correct module/this module/, right?
>

I don't think it's a big difference in semantics but probably yes.
 
>> --- a/drivers/regulator/fan53555.c
>> +++ b/drivers/regulator/fan53555.c
> 
>> +MODULE_DEVICE_TABLE(i2c, fan53555_id);
> 
> As I said above this patch adds two aliases to the fan53555 module:
>     alias:          i2c:syr82x
>     alias:          i2c:fan53555
> 
> But neither the string "fan53555" nor the string "syr82x" generate
> interesting hits in current linux-next. Are these strings perhaps only
> used in out of tree device tree source files?
> 

There are a lot of drivers whose platform code has not been upstreamed
so I haven't checked which ones are used an which ones are not.

But if someone is using this driver and registering a I2C device either
via board code or a Device Tree, it will break as soon as someone builds
it as a module without this patch.

> Thanks,
> 
> 
> Paul Bolle
> 

[0]: http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L483

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