Re: [PATCH] hwmon: lm75: Use zero lm75_type for lm75

[Guenter Roeck <linux@roeck-us.net>]

On 1/30/21 7:43 AM, Matwey V. Kornilov wrote:
> 
> 
> сб, 30 янв. 2021 г. в 18:31, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>>:
>>
>> On 1/30/21 2:10 AM, Matwey V. Kornilov wrote:
>> > There is a logical flaw in lm75_probe() function introduced in
>> >
>> >     e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> >
>> > Note, that of_device_get_match_data() returns NULL when no match
>> > 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.
>> > The simplest way to handle this issue is to make the LM75 code
>> > zero.
>> >
>>
>> This doesn't really solve the problem since it would match _all_
>> non-existing entries with lm75 (instead of whatever is intended).
> 
> Just exactly how it happened before e97a45f1b460
> 
>> That doesn't matter for lm75a, but it would matter if someone
>> would enter, say, "bla,adt75".
>>
>> On a side note, "nxp,lm75a" (nor "nxp,lm75", for that matter) is not a
>> documented compatible string for this driver. If anything, we would
>> need a means to explicitly reject such undefined compatible strings.
>> One option might be to define the first entry in enum lm75_type
>> explicitly as invalid, check for it and reject it if returned.
> 
> It is fine for me. I am afraid that this will break some dts files in the tree.
> The following compatible strings missed in the driver are currently in use:
> 
> ti,lm75
> nxp,lm75
> nxp,lm75a
> 
> I suppose these boards currently rely on the i2c detection path.
> 

Correct. But relying on a bug doesn't improve the situation.
The above compatible strings need to be documented (and properly
implemented), or removed. And we really need a better means to handle
NULL returns from of_device_get_match_data(). Maybe we should use
of_match_device() instead and check for a NULL return.

Guenter