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

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 3/15/23 05: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
> 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>
>
> ---
>   .../bindings/hwmon/drivetemp,hdd-sensors.yaml |  35 ++++++
>   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
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sata_port0: sata-port@0 {
> +        ompatible = "drivetemp,hdd-sensors";
> +        #thermal-sensor-cells = <0>;
> +    };
> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> index 1eb37106a220..9a60315d732c 100644
> --- a/drivers/hwmon/drivetemp.c
> +++ b/drivers/hwmon/drivetemp.c
> @@ -107,6 +107,14 @@
>   #include <scsi/scsi_device.h>
>   #include <scsi/scsi_driver.h>
>   #include <scsi/scsi_proto.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +#include <linux/libata.h>
> +
> +/*A single thermal_zone for all HDD sensors */
> +static struct thermal_zone_device *tz;


This is conceptually wrong. It returns the maximum temperature from all drives,
not the temperature from a single drive.

This is not much different from collecting all temperatures from all sensors
in the system and declaring the maximum of those as single thermal zone.

If anything, each drive would have to reflect a thermal zone. The big question
is how to determine the associated devicetree property.

Also, essentially your patch claims that arch/arm/boot/dts/kirkwood-nsa310s.dts
doesn't work and no one ever noticed. I would like to see that confirmed.

Thanks,
Guenter