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