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

From: Cristian Ciocaltea
Date: Thu Apr 06 2023 - 12:55:43 EST

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@xxxxxxxxxxxxx>
>> ---
>> .../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@xxxxxxxx>
>> + - Guenter Roeck <linux@xxxxxxxxxxxx>
>> +
>> +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