linux-kernel - Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Mar 2023 14:11:41 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     phinex <phinex@...ltek.com>, jdelvare@...e.com
Cc:     linux@...ck-us.net, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for
 thermal_of

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@...ltek.com>

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@...ltek.com>
> +
> +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