Message-ID: <20220225235203.GH10536@uf8f119305bce5e.ant.amazon.com>
Date:   Fri, 25 Feb 2022 15:52:03 -0800
From:   Eduardo Valentin <eduval@...zon.com>
To:     Alexandre Bailon <abailon@...libre.com>
CC:     <rafael@...nel.org>, <rui.zhang@...el.com>,
        <daniel.lezcano@...aro.org>, <amitk@...nel.org>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <ben.tseng@...iatek.com>, <khilman@...libre.com>,
        <mka@...omium.org>
Subject: Re: [PATCH 0/2] thermal: Add support of multiple sensors

Hello Alexandre,

On Fri, Feb 18, 2022 at 09:46:02AM +0100, Alexandre Bailon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Following this comment [1], this updates thermal_of to support multiple
> sensors.
> 
> This has some limitations:
> - A sensor must have its own termal zone, even if it is also registered
>   inside a thermal zone supporting multiple sensors.
> - Some callbacks (such as of_thermal_set_trips) have been updated to support
>   multiple sensors but I don't know if this really make sense.
> - of_thermal_get_trend have not been updated to support multiple sensors.
>   This would probably make sense to support it but I am not sure how to do it,
>   especially for the average.

Great to see this having somewhat a form now!

Overall the idea is sane and aligned to what I had in mind back during the 2019 Linux plumbers: one thermal zone should have multiple sensor inputs.
https://lpc.events/event/4/page/34-accepted-microconferences#PMSummary

In fact, that is aligned to what I originally wrote in the thermal device tree bindings:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#n79

The only major concern with your series is the usage of of-thermal to achieve the multiple sensors per thermal zone.
While that solves the problem, it has the following limitations:
(1) limited to devices described in device tree. everybody else is left out.
(2) it keeps extending the code duplication in of-thermal. 

My suggestion here is have the thermal core aware of the multiple sensors per thermal zone.

That has the advantage of:
(a) cleanup the sensor handling within of-thermal
(b) expand the multi sensor per zone to all types of thermal drivers
(c) standardize the way to handle the multi sensor.

In my original thoughts of achieving this would have include:
(i) move the sensor handling part to a specific c file within the thermal core. That would include helper functions to execute the aggregations, max, min, avg, geo avg, etc etc
(ii) a way to tell what sensors are being aggregated via sysfs, probably a simple syslink to the original device would suffice
(iii) a way to change the aggregation via sysfs. just like you proposed a way to specify the aggregation via device tree, we should have a way to specify the aggregation at runtime.
(iv) once (i)-(iii) is done, you basically cleanup of-thermal to use the new C api written, and of-thermal simply use the API created to register the sensors. I d expect that all the callbacks related sensor ops would disappear from of-thermal.

> 
> [1]: https://patchwork.kernel.org/comment/24723927/
> 
> Alexandre Bailon (2):
>   dt-bindings: thermal: Update the bindings to support multiple sensor
>   Thermal: Add support of multi sensor
> 
>  .../bindings/thermal/thermal-zones.yaml       |  20 +-
>  drivers/thermal/thermal_of.c                  | 491 +++++++++++++++---
>  2 files changed, 449 insertions(+), 62 deletions(-)
> 
> --
> 2.34.1
> 

-- 
All the best,
Eduardo Valentin

Powered by blists - more mailing lists