Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of

[Krzysztof Kozlowski]

On 15/03/2023 13:16, phinex wrote:
> Support thermal zone so that we can just rely on dts to describe a
> thermal zone and do the cooling operations.
> 
> You can define a comptible string "drivetemp,hdd-sensors" to enable

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed also DT list, which means this won't be tested by tools.

> this, such as
> 
> 	sata_port0: sata-port@0 {
> 		compatible = "drivetemp,hdd-sensors";
> 		#thermal-sensor-cells = <0>;
> 	}
> 
> Then define a thermal with this sensor to get it work.
> 
>                hdd_thermal: hdd-thermal {
>                        thermal-sensors = <&sata_port0>;
> 		}
> 
> In most of the SoC systems, using dts to handle cooling is common.
> This can eliminate the usage of user space application to check
> the value exported in hwmon and then through sysfs to cooling.
> 
> Signed-off-by: phinex <phinex@xxxxxxxxxxx>

Is phinex your full name or email alias?

> 
> ---
>  .../bindings/hwmon/drivetemp,hdd-sensors.yaml |  35 ++++++

Bindings are always separate.

>  drivers/hwmon/drivetemp.c                     | 102 +++++++++++++++++-
>  2 files changed, 133 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> new file mode 100644
> index 000000000000..939d7a923e94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/drivetemp,hdd-sensors.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Drivetemp Temperature Monitor
> +
> +maintainers:
> +  - Phinex <phinex@xxxxxxxxxxx>
> +
> +description: |
> +  Drivetemp Temperature Monitor that support a single thermal zone
> +  This single thermal zone can support multiple hard drives,
> +  it uses maximal temperature of these hard drivers as its temp value.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - drivetemp,hdd-sensors

No such vendor prefix. Are you sure you are describing real hardware?

Also device specific part looks too generic. What device is it exactly?


Best regards,
Krzysztof