Re: [PATCH v4 RESEND 1/2] dt-bindings: Add DT bindings for the DT-based virtual sensor driver

[Daniel Lezcano]

On 11/01/2022 11:33, Alexandre Bailon wrote:
> This adds the DT bindings for the DT-based virtual sensor driver.
> This driver provides a way, using DT, to aggregate the temperature
> of multiple thermal zones and get some useful data from it.
>
> Signed-off-by: Alexandre Bailon <abailon@xxxxxxxxxxxx>
> ---

[ ... ]

> +description: |
> +  The virtual thermal sensor device provides a way to aggregate the temperature
> +  from multiple thermal zones. Basically, this could be used to get the
> +  maximum, minimum or average temperature.
> +
> +allOf:
> +  - $ref: thermal-sensor.yaml#
> +
> +properties:
> +  compatible:
> +    const: virtual,thermal-sensor
> +
> +  aggregation-function:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Used to select the operations to perform on the sensors to get the virtual
> +      sensor temperature.
> +    enum:
> +      - VIRTUAL_THERMAL_SENSOR_MIN_VAL
> +      - VIRTUAL_THERMAL_SENSOR_MAX_VAL
> +      - VIRTUAL_THERMAL_SENSOR_AVG_VAL
> +
> +  thermal-sensors:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description:
> +      The names of the thermal zone to aggregate.

IMO this is not correct. We are dealing with virtual sensor in order to 
aggregate a group of sensors in a thermal zone. But actually this 
description aggregates the thermal zones.

I recalled a detail about the binding from the old 'txt' format from [1].

"- thermal-sensors: A list of thermal sensor phandles and sensor specifier

   Type: list of phandles + sensor specifier used while monitoring the 
thermal zone."

This information was lost in the conversion to yaml with commit 
1202a442a31fd

In addition, the "coefficients" property tells the 'thermal-sensors' 
property can have multiple sensors.

So actually, the bindings should be fixed as part of the information was 
lost during of the txt -> yaml conversion.

And the multiple sensors per thermal zone could be implemented as that 
was something planned since a while.

I realize we initially asked to create a virtual sensor but that is not 
the most adequate ATM. Doing the multiple sensors support makes more 
sense to me, especially it does not require any extra binding except an 
attribute tell what kind of temperature aggregation we do. Defaulting to 
max should suffice when no aggregation method is set.

  -- Daniel

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/thermal/thermal.txt?h=v4.5#n147



--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog