Re: [PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

[Guenter Roeck]

On 2/2/21 11:31 AM, Matwey V. Kornilov wrote:
> 
> 
> вт, 2 февр. 2021 г. в 22:29, Guenter Roeck <linux@xxxxxxxxxxxx <mailto:linux@xxxxxxxxxxxx>>:
>>
>> On 2/2/21 10:37 AM, Matwey V. Kornilov wrote:
>> > There is a logical flaw in lm75_probe() function introduced in
>> >
>> >     commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> >
>> > Note, that of_device_get_match_data() returns NULL when no match
>> > is found. This is the case when OF node exists but has unknown
>> > compatible line, while the module is still loaded via i2c
>> > detection.
>> >
>> > arch/powerpc/boot/dts/fsl/p2041rdb.dts:
>> >
>> >     lm75b@48 {
>> >       compatible = "nxp,lm75a";
>> >       reg = <0x48>;
>> >     };
>> >
>> > In this case, the sensor is mistakenly considered as ADT75 variant.
>> >
>> > Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> > Signed-off-by: Matwey V. Kornilov <matwey@xxxxxxxxxx <mailto:matwey@xxxxxxxxxx>>
>>
>> Looks good, but we'll also need a solution for the existing devicetree
>> nodes since they are being used. The best solution would probably
>> be to document and add "nxp,lm75a" to the list of devicetree nodes.
>> The same applies to other used but not documented compatible strings
>> (I think you sent a list earlier if I recall correctly).
>>
>> Sorry that I didn't bring this up earlier, but I am concerned that
>> if we don't do this, this patch might be considered a regression.
> 
> 
> What if I just concatenate this patch with the other patch series where "nxp,lm75a" compatible string is introduced?
>  

It just has to be handled in one series, best with the compatible string(s)
introduced first and then this patch as last patch of the series.

Thanks,
Guenter

>>
>>
>> Thanks,
>> Guenter
>>
>> > ---
>> >  Changes since v2:
>> >  * fixed typo in the message
>> >  * fixed brackets
>> >
>> >  drivers/hwmon/lm75.c | 13 ++++++++++---
>> >  1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
>> > index e447febd121a..53882c334a0d 100644
>> > --- a/drivers/hwmon/lm75.c
>> > +++ b/drivers/hwmon/lm75.c
>> > @@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
>> >       int status, err;
>> >       enum lm75_type kind;
>> >
>> > -     if (client->dev.of_node)
>> > -             kind = (enum lm75_type)of_device_get_match_data(&client->dev);
>> > -     else
>> > +     if (dev->of_node) {
>> > +             const struct of_device_id *match =
>> > +                     of_match_device(dev->driver->of_match_table, dev);
>> > +
>> > +             if (!match)
>> > +                     return -ENODEV;
>> > +
>> > +             kind = (enum lm75_type)(match->data);
>> > +     } else {
>> >               kind = i2c_match_id(lm75_ids, client)->driver_data;
>> > +     }
>> >
>> >       if (!i2c_check_functionality(client->adapter,
>> >                       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
>> >
>>
> 
> 
> --
> With best regards,
> Matwey V. Kornilov