Re: [PATCH] hwmon: Handle failure to register sensor with thermal zone correctly

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

On 2/21/22 09:59, Dmitry Osipenko wrote:
> 21.02.2022 20:12, Guenter Roeck пишет:
>> On 2/21/22 08:56, Dmitry Osipenko wrote:
>>> 21.02.2022 19:44, Guenter Roeck пишет:
>>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>>> index 3501a3ead4ba..4bfe3791a5ba 100644
>>>> --- a/drivers/hwmon/hwmon.c
>>>> +++ b/drivers/hwmon/hwmon.c
>>>> @@ -214,12 +214,14 @@ static int hwmon_thermal_add_sensor(struct
>>>> device *dev, int index)
>>>>          tzd = devm_thermal_zone_of_sensor_register(dev, index, tdata,
>>>>                               &hwmon_thermal_ops);
>>>> -    /*
>>>> -     * If CONFIG_THERMAL_OF is disabled, this returns -ENODEV,
>>>> -     * so ignore that error but forward any other error.
>>>> -     */
>>>> -    if (IS_ERR(tzd) && (PTR_ERR(tzd) != -ENODEV))
>>>> -        return PTR_ERR(tzd);
>>>> +    if (IS_ERR(tzd)) {
>>>> +        if (PTR_ERR(tzd) != -ENODEV)
>>>> +            return PTR_ERR(tzd);
>>>> +        dev_warn(dev, "Failed to register temp%d_input with thermal
>>>> zone\n",
>>>> +             index + 1);
>>>
>>> Do we really need this warning? I suppose it should be okay if sensor
>>> isn't attached to any device in a device-tree and just reports
>>> temperature.
>>
>> I'd rather leave it there for the time being. It will only affect
>> devicetree
>> systems (turns out there is already a check for of_node elsewhere). Thermal
>> zone specification is not always easy and there may be a mismatch between
>> what is reported by the driver and what the user (programmer) expects to
>> see (which I think is what happens here). I don't want to silently
>> ignore such problems without any notification.
>>
>> We could make it dev_notice and/or change the message (instead of
>> "Failed to
>> ..." just say "temp%d_input not registered with thermal zone" , maybe ?).
> 
> I'd change it to:
> 
> dev_info(dev, "temp%d_input not attached to any thermal zone\n", index + 1);
> 

Makes sense. I just sent v2 with that change.

> I'd also add an info message to print out to which tzd attachment happened.

Oh, I don't know. Kernel logs are already way too noisy. Either case that would
not be a bug fix, so such a change should not be part of this patch.

Thanks,
Guenter