linux-kernel - Re: [PATCH v2 1/2] dt-bindings: hwmon: pwm-fan: Convert to DT schema

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]
Message-ID: <5dd096fc-419f-b67a-9577-7021e36da009@collabora.com>
Date:   Thu, 6 Apr 2023 19:55:34 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiko Stuebner <heiko@...ech.de>, linux-hwmon@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: pwm-fan: Convert to DT schema

On 4/6/23 19:45, Rob Herring wrote:
> On Tue, Apr 04, 2023 at 08:38:06PM +0300, Cristian Ciocaltea wrote:
>> Convert the PWM fan bindings to DT schema format.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>>  .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +---------
>>  .../devicetree/bindings/hwmon/pwm-fan.yaml    | 119 ++++++++++++++++++
>>  2 files changed, 120 insertions(+), 67 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>> index 4509e688623a..48886f0ce415 100644
>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>> @@ -1,67 +1 @@
>> -Bindings for a fan connected to the PWM lines
>> -
>> -Required properties:
>> -- compatible	: "pwm-fan"
>> -- pwms		: the PWM that is used to control the PWM fan
>> -- cooling-levels      : PWM duty cycle values in a range from 0 to 255
>> -			which correspond to thermal cooling states
>> -
>> -Optional properties:
>> -- fan-supply		: phandle to the regulator that provides power to the fan
>> -- interrupts		: This contains an interrupt specifier for each fan
>> -			  tachometer output connected to an interrupt source.
>> -			  The output signal must generate a defined number of
>> -			  interrupts per fan revolution, which require that
>> -			  it must be self resetting edge interrupts. See
>> -			  interrupt-controller/interrupts.txt for the format.
>> -- pulses-per-revolution : define the number of pulses per fan revolution for
>> -			  each tachometer input as an integer (default is 2
>> -			  interrupts per revolution). The value must be
>> -			  greater than zero.
>> -
>> -Example:
>> -	fan0: pwm-fan {
>> -		compatible = "pwm-fan";
>> -		#cooling-cells = <2>;
>> -		pwms = <&pwm 0 10000 0>;
>> -		cooling-levels = <0 102 170 230>;
>> -	};
>> -
>> -	thermal-zones {
>> -		cpu_thermal: cpu-thermal {
>> -			     thermal-sensors = <&tmu 0>;
>> -			     polling-delay-passive = <0>;
>> -			     polling-delay = <0>;
>> -			     trips {
>> -					cpu_alert1: cpu-alert1 {
>> -						    temperature = <100000>; /* millicelsius */
>> -						    hysteresis = <2000>; /* millicelsius */
>> -						    type = "passive";
>> -					};
>> -			     };
>> -			     cooling-maps {
>> -					map0 {
>> -						    trip = <&cpu_alert1>;
>> -						    cooling-device = <&fan0 0 1>;
>> -					};
>> -			     };
>> -		};
>> -
>> -Example 2:
>> -	fan0: pwm-fan {
>> -		compatible = "pwm-fan";
>> -		pwms = <&pwm 0 40000 0>;
>> -		fan-supply = <&reg_fan>;
>> -		interrupt-parent = <&gpio5>;
>> -		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>> -		pulses-per-revolution = <2>;
>> -	};
>> -
>> -Example 3:
>> -	fan0: pwm-fan {
>> -		compatible = "pwm-fan";
>> -		pwms = <&pwm1 0 25000 0>;
>> -		interrupts-extended = <&gpio1 1 IRQ_TYPE_EDGE_FALLING>,
>> -			<&gpio2 5 IRQ_TYPE_EDGE_FALLING>;
>> -		pulses-per-revolution = <2>, <1>;
>> -	};
>> +This file has moved to pwm-fan.yaml.
>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> new file mode 100644
>> index 000000000000..048b6ea794c7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> @@ -0,0 +1,119 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/pwm-fan.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Fan connected to PWM lines
>> +
>> +maintainers:
>> +  - Jean Delvare <jdelvare@...e.com>
>> +  - Guenter Roeck <linux@...ck-us.net>
>> +
>> +properties:
>> +  compatible:
>> +    const: pwm-fan
>> +
>> +  cooling-levels:
>> +    description: PWM duty cycle values corresponding to thermal cooling states.
>> +    items:
>> +      maximum: 255
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +
>> +  fan-supply:
>> +    description: Phandle to the regulator that provides power to the fan.
>> +
>> +  interrupts:
>> +    description:
>> +      This contains an interrupt specifier for each fan tachometer output
>> +      connected to an interrupt source. The output signal must generate a
>> +      defined number of interrupts per fan revolution, which require that
>> +      it must be self resetting edge interrupts.
>> +    minItems: 1
>> +    maxItems: 5
> 
> I'm not so sure I'd allow for more than 1 if the example is the only 
> case we can find. More than 1 implies you have multiple fans controlled 
> by 1 PWM. What do you do with the tach data other than display it or 
> detect a stuck fan. You can't really implement any control loop unless 
> you average the rpm's? I suppose there could be h/w, so okay.

I will drop the 3rd example and use 'maxItems: 1' here. Same for
pulses-per-revolution below.

>> +
>> +  pulses-per-revolution:
>> +    description:
>> +      Define the number of pulses per fan revolution for each tachometer
>> +      input as an integer.
>> +    items:
>> +      minimum: 1
>> +      maximum: 4
>> +      default: 2
>> +    minItems: 1
>> +    maxItems: 5
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +
>> +  pwms:
>> +    description: The PWM that is used to control the fan.
>> +    maxItems: 1
>> +
>> +  pwm-names: true
> 
> Not part of the original binding. It's not that useful in general if 
> there's only 1 entry. It's completely useless if the name is not defined 
> for the OS to use. So drop it.

Ack.

>> +
>> +  "#cooling-cells":
>> +    description:
>> +      Must be 2, in order to specify minimum and maximum cooling state used in
>> +      the cooling-maps reference. The first cell is the minimum cooling state
>> +      and the second cell is the maximum cooling state requested.
> 
> Just duplicating the same text from thermal-cooling-devices.yaml is 
> pointless.
> 
>> +    const: 2
> 
> As this is globally the only allowed value, you don't need to duplicate 
> this either.
> 
> '"#cooling-cells": true"' is sufficient.

Sure, will do.

Thanks,
Cristian