Re: [PATCH] WIP: dt-bindings: arm: hwmon: gpio-fan: Convert txt bindings to yaml

[David Heidelberg]

On Tue, Oct 19 2021 at 16:35:17 -0500, Rob Herring <robh@xxxxxxxxxx> 
wrote:
> On Sat, Oct 09, 2021 at 12:43:09PM +0200, David Heidelberg wrote:
> >  Convert fan devices connected to GPIOs to the YAML syntax.
> >
> >  Signed-off-by: David Heidelberg <david@xxxxxxx>
> >  ---
> >   .../devicetree/bindings/hwmon/gpio-fan.txt    | 41 -----------
> >   .../devicetree/bindings/hwmon/gpio-fan.yaml   | 69 
> > +++++++++++++++++++
> >   2 files changed, 69 insertions(+), 41 deletions(-)
> >   delete mode 100644 
> > Documentation/devicetree/bindings/hwmon/gpio-fan.txt
> >   create mode 100644 
> > Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> >
> >  diff --git a/Documentation/devicetree/bindings/hwmon/gpio-fan.txt 
> > b/Documentation/devicetree/bindings/hwmon/gpio-fan.txt
> >  deleted file mode 100644
> >  index f4cfa350f6a1..000000000000
> >  --- a/Documentation/devicetree/bindings/hwmon/gpio-fan.txt
> >  +++ /dev/null
> >  @@ -1,41 +0,0 @@
> >  -Bindings for fan connected to GPIO lines
> >  -
> >  -Required properties:
> >  -- compatible : "gpio-fan"
> >  -
> >  -Optional properties:
> >  -- gpios: Specifies the pins that map to bits in the control value,
> >  -  ordered MSB-->LSB.
> >  -- gpio-fan,speed-map: A mapping of possible fan RPM speeds and the
> >  -  control value that should be set to achieve them. This array
> >  -  must have the RPM values in ascending order.
> >  -- alarm-gpios: This pin going active indicates something is wrong 
> > with
> >  -  the fan, and a udev event will be fired.
> >  -- #cooling-cells: If used as a cooling device, must be <2>
> >  -  Also see:
> >  -  
> > Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml
> >  -  min and max states are derived from the speed-map of the fan.
> >  -
> >  -Note: At least one the "gpios" or "alarm-gpios" properties must be 
> > set.
> >  -
> >  -Examples:
> >  -
> >  -	gpio_fan {
> >  -		compatible = "gpio-fan";
> >  -		gpios = <&gpio1 14 1
> >  -			 &gpio1 13 1>;
> >  -		gpio-fan,speed-map = <0    0
> >  -				      3000 1
> >  -				      6000 2>;
> >  -		alarm-gpios = <&gpio1 15 1>;
> >  -	};
> >  -	gpio_fan_cool: gpio_fan {
> >  -		compatible = "gpio-fan";
> >  -		gpios = <&gpio2 14 1
> >  -			 &gpio2 13 1>;
> >  -		gpio-fan,speed-map =	<0    0>,
> >  -					<3000 1>,
> >  -					<6000 2>;
> >  -		alarm-gpios = <&gpio2 15 1>;
> >  -		#cooling-cells = <2>; /* min followed by max */
> >  -	};
> >  diff --git a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml 
> > b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> >  new file mode 100644
> >  index 000000000000..e2db65d58a92
> >  --- /dev/null
> >  +++ b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> >  @@ -0,0 +1,69 @@
> >  +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >  +%YAML 1.2
> >  +---
> >  +$id: "http://devicetree.org/schemas/hwmon/gpio-fan.yaml#";
> >  +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> >  +
> >  +title: Bindings for fan connected to GPIO lines
> >  +
> >  +maintainers:
> >  +  - Rob Herring <robh+dt@xxxxxxxxxx>
>
> Just robh@xxxxxxxxxx
>
> >  +
> >  +properties:
> >  +  compatible:
> >  +    const: gpio-fan
> >  +
> >  +  gpios:
> >  +    description: |
> >  +      Specifies the pins that map to bits in the control value,
> >  +      ordered MSB-->LSB.
>
> minItems: 1
> maxItems: 7 ?
>
> >  +
> >  +  gpio-fan,speed-map:
> >  +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >  +    minItems: 4
> >  +    maxItems: 254
> >  +    description: |
> >  +      A mapping of possible fan RPM speeds and the
> >  +      control value that should be set to achieve them. This array
> >  +      must have the RPM values in ascending order.
>
> Really this should be a uint32-matrix with this schema:
>
> items:
>   minItems: 2
>   maxItems: 127
>   items:
>     - description: fan speed in RPMs
>     - description: control value
>
> >  +
> >  +  alarm-gpios:
> >  +    description: |
> >  +      This pin going active indicates something is wrong with
> >  +      the fan, and a udev event will be fired.
>
> maxItems: 1
>
> udev is a linuxism and shouldn't be in the binding.
>
> >  +
> >  +  '#cooling-cells':
> >  +    const: 2
> >  +
> >  +required:
> >  +  - compatible
> >  +  - gpio-fan,speed-map
> >  +
> >  +anyOf:
> >  +  - required: [gpios]
>
> How is 'gpios' not always required?
>
> >  +  - required: [alarm-gpios]
> >  +
> >  +additionalProperties: false
> >  +
> >  +examples:
> >  +  - |
> >  +    gpio_fan {
> >  +      compatible = "gpio-fan";
> >  +      gpios = <&gpio1 14 1
> >  +               &gpio1 13 1>;
> >  +      gpio-fan,speed-map = <0    0
> >  +                            3000 1
> >  +                            6000 2>;
>
> Brackets needed around each pair.

Well, that's the issue. I would love to use u32-matrix, but all the 
drivers use < x1 x2 y1 x2 ... z1 z2 > syntax and driver suggests it's 
the right solution.
        * Speed map is in the form <RPM ctrl_val RPM ctrl_val ...>

Someone had to rewrite the driver and the DTS files to fix it. We could 
mark old format as deprecated and use u32-matrix, but for now with 
current drivers it's not a solution.

What you think? Should I document it as it is (so u32-array)?

David

> >  +      alarm-gpios = <&gpio1 15 1>;
> >  +    };
> >  +  - |
> >  +    gpio_fan_cool: gpio_fan {
> >  +      compatible = "gpio-fan";
> >  +      gpios = <&gpio2 14 1
> >  +               &gpio2 13 1>;
> >  +      gpio-fan,speed-map = <0    0
> >  +                            3000 1
> >  +                            6000 2>;
> >  +      alarm-gpios = <&gpio2 15 1>;
> >  +      #cooling-cells = <2>; /* min followed by max */
> >  +    };
> >  --
> >  2.33.0