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
