[PATCH 0/2] Add PWM fan support to Rock 5B board

[PATCH 0/2] Add PWM fan support to Rock 5B board

[Cristian Ciocaltea]

This patch series enables support for the PWM controlled heat sink fan 
on the Rock 5B SBC and, additionally, converts the hwmon PWM fan bindings 
to DT schema format.

Cristian Ciocaltea (2):
  dt-bindings: hwmon: pwm-fan: Convert to DT schema
  arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

 .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
 .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |  12 ++
 3 files changed, 122 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml

-- 
2.40.0

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

[Cristian Ciocaltea]

Convert the PWM fan bindings to DT schema format.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
 .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
 2 files changed, 110 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..448b48ec5d73
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
@@ -0,0 +1,109 @@
+# 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@suse.com>
+  - Guenter Roeck <linux@roeck-us.net>
+
+properties:
+  compatible:
+    const: pwm-fan
+
+  cooling-levels:
+    description:
+      PWM duty cycle values in a range from 0 to 255 which correspond to
+      thermal cooling states.
+    $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.
+
+  pulses-per-revolution:
+    description:
+      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.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  pwms:
+    description: The PWM that is used to control the fan.
+    maxItems: 1
+
+  pwm-names: true
+
+  "#cooling-cells":
+    description: The PWM fan can be referenced as a cooling-device.
+    const: 2
+
+required:
+  - compatible
+  - pwms
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm-fan {
+      compatible = "pwm-fan";
+      cooling-levels = <0 102 170 230>;
+      pwms = <&pwm 0 10000 0>;
+      #cooling-cells = <2>;
+    };
+
+    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>;
+          };
+        };
+      };
+    };
+
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    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>;
+    };
+
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    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>;
+    };
-- 
2.40.0

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

[Rob Herring]

On Mon, 03 Apr 2023 13:50:51 +0300, Cristian Ciocaltea wrote:
> Convert the PWM fan bindings to DT schema format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>  .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>  2 files changed, 110 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230403105052.426135-2-cristian.ciocaltea@collabora.com


pwm-fan: 'cooling-max-state', 'cooling-min-state' do not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dtb
	arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dtb

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

[Cristian Ciocaltea]

On 4/3/23 16:10, Rob Herring wrote:
> 
> On Mon, 03 Apr 2023 13:50:51 +0300, Cristian Ciocaltea wrote:
>> Convert the PWM fan bindings to DT schema format.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>>   .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>>   2 files changed, 110 insertions(+), 67 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230403105052.426135-2-cristian.ciocaltea@collabora.com
> 
> 
> pwm-fan: 'cooling-max-state', 'cooling-min-state' do not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dtb
> 	arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dtb
> 

The only references to the offending cooling-{min|max}-state are located 
in a few DTS files. Assuming they are obsolete, may I simply drop them?

$ git grep "cooling-.*-state"

arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi: cooling-min-state = <0>;
arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi: cooling-max-state = <3>;
arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts: cooling-min-state = <0>;
arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts: cooling-max-state = <3>;
arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-min-state = <0>;
arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-max-state = <3>;
arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:        cooling-min-state = <0>;
arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:        cooling-max-state = <9>;

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

[AngeloGioacchino Del Regno]

Il 03/04/23 16:32, Cristian Ciocaltea ha scritto:
> On 4/3/23 16:10, Rob Herring wrote:
>>
>> On Mon, 03 Apr 2023 13:50:51 +0300, Cristian Ciocaltea wrote:
>>> Convert the PWM fan bindings to DT schema format.
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>    .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>>>    .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>>>    2 files changed, 110 insertions(+), 67 deletions(-)
>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>
>>
>> Running 'make dtbs_check' with the schema in this patch gives the
>> following warnings. Consider if they are expected or the schema is
>> incorrect. These may not be new warnings.
>>
>> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
>> This will change in the future.
>>
>> Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230403105052.426135-2-cristian.ciocaltea@collabora.com
>>
>>
>> pwm-fan: 'cooling-max-state', 'cooling-min-state' do not match any of the regexes: 'pinctrl-[0-9]+'
>> 	arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dtb
>> 	arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dtb
>>
> 
> The only references to the offending cooling-{min|max}-state are located
> in a few DTS files. Assuming they are obsolete, may I simply drop them?
> 

If they're obsolete, you can mark them as `deprecated: true` in the binding, but
dropping them entirely would be an ABI breakage, so no, you can't.

Regards,
Angelo

> $ git grep "cooling-.*-state"
> 
> arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi: cooling-min-state = <0>;
> arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi: cooling-max-state = <3>;
> arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts: cooling-min-state = <0>;
> arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts: cooling-max-state = <3>;
> arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-min-state = <0>;
> arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-max-state = <3>;
> arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:        cooling-min-state = <0>;
> arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:        cooling-max-state = <9>;
> 

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

[Cristian Ciocaltea]

On 4/3/23 17:43, AngeloGioacchino Del Regno wrote:
> Il 03/04/23 16:32, Cristian Ciocaltea ha scritto:
>> On 4/3/23 16:10, Rob Herring wrote:
>>>
>>> On Mon, 03 Apr 2023 13:50:51 +0300, Cristian Ciocaltea wrote:
>>>> Convert the PWM fan bindings to DT schema format.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>    .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>>>>    .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109
>>>> ++++++++++++++++++
>>>>    2 files changed, 110 insertions(+), 67 deletions(-)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>
>>>
>>> Running 'make dtbs_check' with the schema in this patch gives the
>>> following warnings. Consider if they are expected or the schema is
>>> incorrect. These may not be new warnings.
>>>
>>> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
>>> This will change in the future.
>>>
>>> Full log is available here:
>>> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230403105052.426135-2-cristian.ciocaltea@collabora.com
>>>
>>>
>>> pwm-fan: 'cooling-max-state', 'cooling-min-state' do not match any of
>>> the regexes: 'pinctrl-[0-9]+'
>>>     arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dtb
>>>     arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dtb
>>>
>>
>> The only references to the offending cooling-{min|max}-state are located
>> in a few DTS files. Assuming they are obsolete, may I simply drop them?
>>
> 
> If they're obsolete, you can mark them as `deprecated: true` in the
> binding, but
> dropping them entirely would be an ABI breakage, so no, you can't.

From the pwm-fan driver point of view, the properties are not supported
and I couldn't find any indication that they could have been used in the
past.

Hence I'm not sure adding them to the binding is the proper way to
handle this issue.

Thanks,
Cristian

> Regards,
> Angelo
> 
>> $ git grep "cooling-.*-state"
>>
>> arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi:
>> cooling-min-state = <0>;
>> arch/arm64/boot/dts/amlogic/meson-g12b-bananapi.dtsi:
>> cooling-max-state = <3>;
>> arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts:
>> cooling-min-state = <0>;
>> arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dts:
>> cooling-max-state = <3>;
>> arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-min-state = <0>;
>> arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts:        cooling-max-state = <3>;
>> arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:       
>> cooling-min-state = <0>;
>> arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi:       
>> cooling-max-state = <9>;
>>
> 
> 
> 

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

[Rob Herring]

On Mon, Apr 03, 2023 at 07:22:45PM +0300, Cristian Ciocaltea wrote:
> On 4/3/23 17:43, AngeloGioacchino Del Regno wrote:
> > Il 03/04/23 16:32, Cristian Ciocaltea ha scritto:
> >> On 4/3/23 16:10, Rob Herring wrote:
> >>>
> >>> On Mon, 03 Apr 2023 13:50:51 +0300, Cristian Ciocaltea wrote:
> >>>> Convert the PWM fan bindings to DT schema format.
> >>>>
> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>> ---
> >>>>    .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
> >>>>    .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109
> >>>> ++++++++++++++++++
> >>>>    2 files changed, 110 insertions(+), 67 deletions(-)
> >>>>    create mode 100644
> >>>> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> >>>>
> >>>
> >>> Running 'make dtbs_check' with the schema in this patch gives the
> >>> following warnings. Consider if they are expected or the schema is
> >>> incorrect. These may not be new warnings.
> >>>
> >>> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> >>> This will change in the future.
> >>>
> >>> Full log is available here:
> >>> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230403105052.426135-2-cristian.ciocaltea@collabora.com
> >>>
> >>>
> >>> pwm-fan: 'cooling-max-state', 'cooling-min-state' do not match any of
> >>> the regexes: 'pinctrl-[0-9]+'
> >>>     arch/arm64/boot/dts/amlogic/meson-sm1-odroid-hc4.dtb
> >>>     arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dtb
> >>>
> >>
> >> The only references to the offending cooling-{min|max}-state are located
> >> in a few DTS files. Assuming they are obsolete, may I simply drop them?
> >>
> > 
> > If they're obsolete, you can mark them as `deprecated: true` in the
> > binding, but
> > dropping them entirely would be an ABI breakage, so no, you can't.
> 
> >From the pwm-fan driver point of view, the properties are not supported
> and I couldn't find any indication that they could have been used in the
> past.
> 
> Hence I'm not sure adding them to the binding is the proper way to
> handle this issue.

They can be omitted.

Rob

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

[Rob Herring]

On Mon, Apr 03, 2023 at 01:50:51PM +0300, Cristian Ciocaltea wrote:
> Convert the PWM fan bindings to DT schema format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>  .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>  2 files changed, 110 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..448b48ec5d73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> @@ -0,0 +1,109 @@
> +# 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@suse.com>
> +  - Guenter Roeck <linux@roeck-us.net>
> +
> +properties:
> +  compatible:
> +    const: pwm-fan
> +
> +  cooling-levels:
> +    description:
> +      PWM duty cycle values in a range from 0 to 255 which correspond to

Don't put constraints in plain text:

items:
  maximum: 255

> +      thermal cooling states.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

Unfortunately, looks like we've wound up with same property with 2 
differing types. A problem for another day...

> +
> +  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.

How many entries? I'm not sure how more than 1 makes sense.

> +
> +  pulses-per-revolution:
> +    description:
> +      Define the number of pulses per fan revolution for each tachometer
> +      input as an integer (default is 2 interrupts per revolution).

default: 2

> +      The value must be greater than zero.

minimum: 1

maximum: ??? I assume there's some practical limit here much less than 
2^32.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array

Isn't this a scalar? 
> +
> +  pwms:
> +    description: The PWM that is used to control the fan.
> +    maxItems: 1
> +
> +  pwm-names: true
> +
> +  "#cooling-cells":
> +    description: The PWM fan can be referenced as a cooling-device.

Not that useful. What would be is what's in the 2 cells.

> +    const: 2
> +
> +required:
> +  - compatible
> +  - pwms
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm-fan {
> +      compatible = "pwm-fan";
> +      cooling-levels = <0 102 170 230>;
> +      pwms = <&pwm 0 10000 0>;
> +      #cooling-cells = <2>;
> +    };
> +
> +    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>;
> +          };
> +        };
> +      };
> +    };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    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>;
> +    };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    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>;
> +    };
> -- 
> 2.40.0
> 

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

[Guenter Roeck]

On 4/4/23 07:17, Rob Herring wrote:
> On Mon, Apr 03, 2023 at 01:50:51PM +0300, Cristian Ciocaltea wrote:
>> Convert the PWM fan bindings to DT schema format.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>>   .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>>   2 files changed, 110 insertions(+), 67 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
...
>> +
>> +  pulses-per-revolution:
>> +    description:
>> +      Define the number of pulses per fan revolution for each tachometer
>> +      input as an integer (default is 2 interrupts per revolution).
> 
> default: 2
> 
>> +      The value must be greater than zero.
> 
> minimum: 1
> 
> maximum: ??? I assume there's some practical limit here much less than
> 2^32.
> 

Should be 1 to 4.

Guenter

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

[Cristian Ciocaltea]

On 4/4/23 17:17, Rob Herring wrote:
> On Mon, Apr 03, 2023 at 01:50:51PM +0300, Cristian Ciocaltea wrote:
>> Convert the PWM fan bindings to DT schema format.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../devicetree/bindings/hwmon/pwm-fan.txt     |  68 +----------
>>  .../devicetree/bindings/hwmon/pwm-fan.yaml    | 109 ++++++++++++++++++
>>  2 files changed, 110 insertions(+), 67 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/hwmon/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..448b48ec5d73
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> @@ -0,0 +1,109 @@
>> +# 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@suse.com>
>> +  - Guenter Roeck <linux@roeck-us.net>
>> +
>> +properties:
>> +  compatible:
>> +    const: pwm-fan
>> +
>> +  cooling-levels:
>> +    description:
>> +      PWM duty cycle values in a range from 0 to 255 which correspond to
> 
> Don't put constraints in plain text:
> 
> items:
>   maximum: 255
> 
>> +      thermal cooling states.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> Unfortunately, looks like we've wound up with same property with 2 
> differing types. A problem for another day...
> 
>> +
>> +  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.
> 
> How many entries? I'm not sure how more than 1 makes sense.

The 3rd example taken from the original binding uses 2 entries. So far
it seems there are no use cases requiring more than one, so I'm going to
limit this to 5 (the driver doesn't enforce a limit).

>> +
>> +  pulses-per-revolution:
>> +    description:
>> +      Define the number of pulses per fan revolution for each tachometer
>> +      input as an integer (default is 2 interrupts per revolution).
> 
> default: 2
> 
>> +      The value must be greater than zero.
> 
> minimum: 1
> 
> maximum: ??? I assume there's some practical limit here much less than 
> 2^32.

Will set it to 4, as suggested by Guenter.

>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> Isn't this a scalar? 

As in the case of interrupts, the 3rd example uses 2 entries. Will set
the same limit (5).

>> +
>> +  pwms:
>> +    description: The PWM that is used to control the fan.
>> +    maxItems: 1
>> +
>> +  pwm-names: true
>> +
>> +  "#cooling-cells":
>> +    description: The PWM fan can be referenced as a cooling-device.
> 
> Not that useful. What would be is what's in the 2 cells.

Will describe its usage according to thermal-cooling-devices binding.

Thanks for reviewing,
Cristian

[PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Cristian Ciocaltea]

Add the necessary DT changes for the Rock 5B board to enable support for
the PWM controlled heat sink fan.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index 95805cb0adfa..bd74d9da2c17 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -17,6 +17,14 @@ chosen {
 		stdout-path = "serial2:1500000n8";
 	};
 
+	fan: pwm-fan {
+		compatible = "pwm-fan";
+		cooling-levels = <0 95 145 195 255>;
+		fan-supply = <&vcc5v0_sys>;
+		pwms = <&pwm1 0 50000 0>;
+		#cooling-cells = <2>;
+	};
+
 	vcc5v0_sys: vcc5v0-sys-regulator {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc5v0_sys";
@@ -27,6 +35,10 @@ vcc5v0_sys: vcc5v0-sys-regulator {
 	};
 };
 
+&pwm1 {
+	status = "okay";
+};
+
 &sdhci {
 	bus-width = <8>;
 	no-sdio;
-- 
2.40.0

Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Chris Morgan]

On Mon, Apr 03, 2023 at 01:50:52PM +0300, Cristian Ciocaltea wrote:
> Add the necessary DT changes for the Rock 5B board to enable support for
> the PWM controlled heat sink fan.

Honest question, but should we be adding this to the board file if not
every device has a PWM fan (they all have the socket for the fan, but
not the fan)? For example I have a passively cooled case that doesn't
include a fan.

Thank you,
Chris Morgan.

> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index 95805cb0adfa..bd74d9da2c17 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -17,6 +17,14 @@ chosen {
>  		stdout-path = "serial2:1500000n8";
>  	};
>  
> +	fan: pwm-fan {
> +		compatible = "pwm-fan";
> +		cooling-levels = <0 95 145 195 255>;
> +		fan-supply = <&vcc5v0_sys>;
> +		pwms = <&pwm1 0 50000 0>;
> +		#cooling-cells = <2>;
> +	};
> +
>  	vcc5v0_sys: vcc5v0-sys-regulator {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc5v0_sys";
> @@ -27,6 +35,10 @@ vcc5v0_sys: vcc5v0-sys-regulator {
>  	};
>  };
>  
> +&pwm1 {
> +	status = "okay";
> +};
> +
>  &sdhci {
>  	bus-width = <8>;
>  	no-sdio;
> -- 
> 2.40.0
> 

Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Peter Geis]

On Mon, Apr 3, 2023 at 1:56 PM Chris Morgan <macroalpha82@gmail.com> wrote:
>
> On Mon, Apr 03, 2023 at 01:50:52PM +0300, Cristian Ciocaltea wrote:
> > Add the necessary DT changes for the Rock 5B board to enable support for
> > the PWM controlled heat sink fan.
>
> Honest question, but should we be adding this to the board file if not
> every device has a PWM fan (they all have the socket for the fan, but
> not the fan)? For example I have a passively cooled case that doesn't
> include a fan.

Active cooling should be set up to take effect before throttling. If
it is there great, (especially if it has feedback), if it doesn't
exist it really doesn't affect anything because the throttling will
kick in as necessary.

>
> Thank you,
> Chris Morgan.
>
> >
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index 95805cb0adfa..bd74d9da2c17 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -17,6 +17,14 @@ chosen {
> >               stdout-path = "serial2:1500000n8";
> >       };
> >
> > +     fan: pwm-fan {
> > +             compatible = "pwm-fan";
> > +             cooling-levels = <0 95 145 195 255>;
> > +             fan-supply = <&vcc5v0_sys>;
> > +             pwms = <&pwm1 0 50000 0>;
> > +             #cooling-cells = <2>;
> > +     };
> > +
> >       vcc5v0_sys: vcc5v0-sys-regulator {
> >               compatible = "regulator-fixed";
> >               regulator-name = "vcc5v0_sys";
> > @@ -27,6 +35,10 @@ vcc5v0_sys: vcc5v0-sys-regulator {
> >       };
> >  };
> >
> > +&pwm1 {
> > +     status = "okay";
> > +};
> > +
> >  &sdhci {
> >       bus-width = <8>;
> >       no-sdio;
> > --
> > 2.40.0
> >
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Heiko Stübner]

Am Montag, 3. April 2023, 19:56:26 CEST schrieb Chris Morgan:
> On Mon, Apr 03, 2023 at 01:50:52PM +0300, Cristian Ciocaltea wrote:
> > Add the necessary DT changes for the Rock 5B board to enable support for
> > the PWM controlled heat sink fan.
> 
> Honest question, but should we be adding this to the board file if not
> every device has a PWM fan (they all have the socket for the fan, but
> not the fan)? For example I have a passively cooled case that doesn't
> include a fan.

We also set up the sdmmc host without knowing if the user will plug in
an sd-card ;-) .

So especially in the case where there is a dedicated socket for it,
as you write, we should definitly add it for the board.

By the way, am I correct in thinking that patch 1 and 2 are actually
independent of each other? So I'd just pick patch2 for the Rockchip tree
(as we do have a text-binding) and you can handle the yaml conversion
in a followup or whatever?

Thanks
Heiko

Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Cristian Ciocaltea]

On 4/4/23 00:55, Heiko Stübner wrote:
> Am Montag, 3. April 2023, 19:56:26 CEST schrieb Chris Morgan:
>> On Mon, Apr 03, 2023 at 01:50:52PM +0300, Cristian Ciocaltea wrote:
>>> Add the necessary DT changes for the Rock 5B board to enable support for
>>> the PWM controlled heat sink fan.
>>
>> Honest question, but should we be adding this to the board file if not
>> every device has a PWM fan (they all have the socket for the fan, but
>> not the fan)? For example I have a passively cooled case that doesn't
>> include a fan.
> 
> We also set up the sdmmc host without knowing if the user will plug in
> an sd-card ;-) .
> 
> So especially in the case where there is a dedicated socket for it,
> as you write, we should definitly add it for the board.
> 
> By the way, am I correct in thinking that patch 1 and 2 are actually
> independent of each other? So I'd just pick patch2 for the Rockchip tree
> (as we do have a text-binding) and you can handle the yaml conversion
> in a followup or whatever?

Yes, the bindings conversion can be handled independently.

Thanks,
Cristian

Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-rock-5b: Add pwm-fan

[Christopher Obbard]

Hi Cristian,

On Mon, 2023-04-03 at 13:50 +0300, Cristian Ciocaltea wrote:
> Add the necessary DT changes for the Rock 5B board to enable support for
> the PWM controlled heat sink fan.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Reviewed-by: Christopher Obbard <chris.obbard@collabora.com>

> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index 95805cb0adfa..bd74d9da2c17 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -17,6 +17,14 @@ chosen {
>                 stdout-path = "serial2:1500000n8";
>         };
>  
> +       fan: pwm-fan {
> +               compatible = "pwm-fan";
> +               cooling-levels = <0 95 145 195 255>;
> +               fan-supply = <&vcc5v0_sys>;
> +               pwms = <&pwm1 0 50000 0>;
> +               #cooling-cells = <2>;
> +       };
> +
>         vcc5v0_sys: vcc5v0-sys-regulator {
>                 compatible = "regulator-fixed";
>                 regulator-name = "vcc5v0_sys";
> @@ -27,6 +35,10 @@ vcc5v0_sys: vcc5v0-sys-regulator {
>         };
>  };
>  
> +&pwm1 {
> +       status = "okay";
> +};
> +
>  &sdhci {
>         bus-width = <8>;
>         no-sdio;
> -- 
> 2.40.0
> 
>