Re: [PATCH v1 1/2] hwmon: Support set_trips() of thermal device ops

[Dmitry Osipenko]

20.06.2021 22:21, Guenter Roeck пишет:
> On Sun, Jun 20, 2021 at 08:38:27PM +0300, Dmitry Osipenko wrote:
>> 20.06.2021 20:23, Guenter Roeck пишет:
>>> On Sun, Jun 20, 2021 at 07:12:22PM +0300, Dmitry Osipenko wrote:
>>>> Support set_trips() callback of thermal device ops. This allows HWMON
>>>> device to operatively notify thermal core about temperature changes, which
>>>> is very handy to have in a case where HWMON sensor is used by CPU thermal
>>>> zone that performs passive cooling and emergency shutdown on overheat.
>>>> Thermal core will be able to react faster to temperature changes.
>>>>
>>>
>>> Why would this require a driver callback, and why can it not be handled
>>> in the hwmon core alone ? The hwmon core could register a set_trip function
>>> if the chip (driver) supports setting low and high limits, and it could
>>> call the appropriate driver functions when hwmon_thermal_set_trips()
>>> is called.
>>
>> I wasn't sure about what other hwmon drivers may need and want to do for
>> programming of the trips, so decided to start with this variant. I'll
>> prepare v2 since you're suggesting that the universal callback should
>> work okay for all drivers, thanks.
> 
> It will require some checks during probe to make sure that writeable limits
> exist, but that is still better than per-driver code. If for whatever
> reason some platform expects a different set of registers (say,
> critical limits instead of warning limits to attach to trip points),
> or if some platform expects that limits are _not_ used as trip points,
> that would not be driver but platform specific. You would not be able
> to address that on driver level with a single callback either (after all,
> lm90 compatible chips support up to three sets of limits).
> That means you already made an implementation specific choice with your
> code, by selecting one of those three sets of limits to act as trip
> points, and by making trip point support mandatory for all lm90 compatible
> chips. If we need to make that configurable, we'll need a better solution
> than a single driver callback, and that solution may as well be generic
> and driver independent.

Thank you for the clarification! If device makes a special use of lm90,
then very likely that it won't attach sensor to thermal zone. At least
all devices supported by mainline kernel should be okay here.

I think other sensors should be in a similar position. If a more complex
solution will be needed, then indeed hwmon API could be improved
further. The thermal device is created only for hwmon sensors that are
attached to thermal zone in a device-tree, so the scope of potentially
affected device should be small. Seems lm90 is actually the only hwmon
sensor that is used by thermal zones today.

AFAICS, all drivers return -EOPNOTSUPP if limits can't be changed, so we
could equal this error code to success in a case of set_trips(). The
set_trips() is very optional, if driver can't set limits, then the trips
won't trigger and thermal core will continue to work like set_trips()
wasn't hooked up. I'll implement this in v2.