Re: [PATCH] dt-bindings: pwm: tegra: Convert to json-schema

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 03/11/2022 08:01, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Convert the Tegra PWFM bindings from the free-form text format to
> json-schema.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../bindings/pwm/nvidia,tegra20-pwm.txt       |  77 ----------
>  .../bindings/pwm/nvidia,tegra20-pwm.yaml      | 144 ++++++++++++++++++
>  2 files changed, 144 insertions(+), 77 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
>  create mode 100644 Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> deleted file mode 100644
> index 74c41e34c3b6..000000000000
> --- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> +++ /dev/null
> @@ -1,77 +0,0 @@
> -Tegra SoC PWFM controller
> -
> -Required properties:
> -- compatible: Must be:
> -  - "nvidia,tegra20-pwm": for Tegra20
> -  - "nvidia,tegra30-pwm", "nvidia,tegra20-pwm": for Tegra30
> -  - "nvidia,tegra114-pwm", "nvidia,tegra20-pwm": for Tegra114
> -  - "nvidia,tegra124-pwm", "nvidia,tegra20-pwm": for Tegra124
> -  - "nvidia,tegra132-pwm", "nvidia,tegra20-pwm": for Tegra132
> -  - "nvidia,tegra210-pwm", "nvidia,tegra20-pwm": for Tegra210
> -  - "nvidia,tegra186-pwm": for Tegra186
> -  - "nvidia,tegra194-pwm": for Tegra194
> -- reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.yaml in this directory for a description of
> -  the cells format.
> -- clocks: Must contain one entry, for the module clock.
> -  See ../clocks/clock-bindings.txt for details.
> -- resets: Must contain an entry for each entry in reset-names.
> -  See ../reset/reset.txt for details.
> -- reset-names: Must include the following entries:
> -  - pwm
> -
> -Optional properties:
> -============================
> -In some of the interface like PWM based regulator device, it is required
> -to configure the pins differently in different states, especially in suspend
> -state of the system. The configuration of pin is provided via the pinctrl
> -DT node as detailed in the pinctrl DT binding document
> -	Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> -
> -The PWM node will have following optional properties.
> -pinctrl-names:	Pin state names. Must be "default" and "sleep".
> -pinctrl-0:	phandle for the default/active state of pin configurations.
> -pinctrl-1:	phandle for the sleep state of pin configurations.
> -
> -Example:
> -
> -	pwm: pwm@7000a000 {
> -		compatible = "nvidia,tegra20-pwm";
> -		reg = <0x7000a000 0x100>;
> -		#pwm-cells = <2>;
> -		clocks = <&tegra_car 17>;
> -		resets = <&tegra_car 17>;
> -		reset-names = "pwm";
> -	};
> -
> -
> -Example with the pin configuration for suspend and resume:
> -=========================================================
> -Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
> -it requires PWM output to be tristated when system enters suspend.
> -Following will be DT binding to achieve this:
> -
> -#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> -
> -	pinmux@700008d4 {
> -		pwm_active_state: pwm_active_state {
> -                        pe7 {
> -                                nvidia,pins = "pe7";
> -                                nvidia,tristate = <TEGRA_PIN_DISABLE>;
> -			};
> -		};
> -
> -		pwm_sleep_state: pwm_sleep_state {
> -                        pe7 {
> -                                nvidia,pins = "pe7";
> -                                nvidia,tristate = <TEGRA_PIN_ENABLE>;
> -			};
> -		};
> -	};
> -
> -	pwm@7000a000 {
> -		/* Mandatory PWM properties */
> -		pinctrl-names = "default", "sleep";
> -		pinctrl-0 = <&pwm_active_state>;
> -		pinctrl-1 = <&pwm_sleep_state>;
> -	};
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.yaml b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.yaml
> new file mode 100644
> index 000000000000..9c73e78ff434
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/nvidia,tegra20-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra PWFM controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Jon Hunter <jonathanh@nvidia.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: nvidia,tegra20-pwm
> +
> +      - items:
> +          - enum:
> +              - nvidia,tegra30-pwm
> +              - nvidia,tegra114-pwm
> +              - nvidia,tegra124-pwm
> +              - nvidia,tegra132-pwm
> +              - nvidia,tegra210-pwm
> +          - enum:
> +              - nvidia,tegra20-pwm
> +
> +      - items:
> +          - const: nvidia,tegra186-pwm

I guess you wanted to keep some order between nvidia,tegra20-pwm and
nvidia,tegra186-pwm, but this creates impression you will have here more
items, which of course cannot happen. So either keep this one with
tegra20 as one enum or drop "items".

> +
> +      - items:
> +          - const: nvidia,tegra194-pwm
> +          - const: nvidia,tegra186-pwm
> +
> +      - items:
> +          - const: nvidia,tegra234-pwm
> +          - const: nvidia,tegra194-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: module clock

Just maxItems: 1, because description is not really helping.

> +
> +  clock-names:
> +    items:
> +      - const: pwm

This wasn't in original binding and does not look needed. Mention
changes from pure conversion.

> +
> +  resets:
> +    items:
> +      - description: module reset
> +
> +  reset-names:
> +    items:
> +      - const: pwm
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  pinctrl-names:
> +    items:
> +      - const: default
> +      - const: sleep
> +
> +  pinctrl-0:
> +    description: configuration for the default/active state
> +
> +  pinctrl-1:
> +    description: configuration for the sleep state
> +
> +  operating-points-v2:
> +    $ref: "/schemas/types.yaml#/definitions/phandle"

Drop quotes. We should actually define it in some common schema.

> +
> +  power-domains:
> +    items:
> +      - description: phandle to the core power domain
> +
> +allOf:
> +  - $ref: pwm.yaml
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/tegra20-car.h>
> +
> +    pwm: pwm@7000a000 {
> +        compatible = "nvidia,tegra20-pwm";
> +        reg = <0x7000a000 0x100>;
> +        #pwm-cells = <2>;
> +        clocks = <&tegra_car TEGRA20_CLK_PWM>;
> +        resets = <&tegra_car 17>;
> +        reset-names = "pwm";
> +    };
> +
> +  # Example with the pin configuration for suspend and resume:
> +  # ==========================================================
> +  # Suppose pin PE7 (On Tegra210) interfaced with the regulator device and it requires PWM output
> +  # to be tristated when system enters suspend.
> +  - |
> +    #include <dt-bindings/clock/tegra210-car.h>
> +    #include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> +    pinmux@700008d4 {
> +        compatible = "nvidia,tegra210-pinmux";
> +        reg = <0x700008d4 0x29c>, /* Pad control registers */
> +              <0x70003000 0x294>; /* Mux registers */
> +
> +        pwm_active_state: pwm_active_state {

No underscores in node names.


Best regards,
Krzysztof