Re: [PATCH 1/2] dt-bindings: pwm: convert atmel pwm to json-schema

[<Sergiu.Moga@microchip.com>]

On 07.03.2022 15:22, Krzysztof Kozlowski wrote:
> On 07/03/2022 14:03, Sergiu Moga wrote:
>> Convert PWM binding for Atmel/Microchip SoCs to Device Tree Schema
>> format.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>   .../devicetree/bindings/pwm/atmel-pwm.txt     | 35 -----------
>>   .../devicetree/bindings/pwm/atmel-pwm.yaml    | 62 +++++++++++++++++++
>>   2 files changed, 62 insertions(+), 35 deletions(-)
>>   delete mode 100644 Documentation/devicetree/bindings/pwm/atmel-pwm.txt
>>   create mode 100644 Documentation/devicetree/bindings/pwm/atmel-pwm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
>> deleted file mode 100644
>> index fbb5325be1f0..000000000000
>> --- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> I would prefer for new files to switch to common DT conversion:
> vendor,name.yaml
>
> You have there another PWM bindings for Atmel, so the name should not be
> just "pwm" but something more specific, maybe "at91sam-pwm"?
>
>
Understood, yes, it does make more sense like this. Thank you for the 
suggestion, I will rename the file to `atmel,at91sam-pwm.yaml`.

>> +++ /dev/null
>> @@ -1,35 +0,0 @@
>> -Atmel PWM controller
>> -
>> -Required properties:
>> -  - compatible: should be one of:
>> -    - "atmel,at91sam9rl-pwm"
>> -    - "atmel,sama5d3-pwm"
>> -    - "atmel,sama5d2-pwm"
>> -    - "microchip,sam9x60-pwm"
>> -  - reg: physical base address and length of the controller's registers
>> -  - #pwm-cells: Should be 3. See pwm.yaml in this directory for a
>> -    description of the cells format.
>> -
>> -Example:
>> -
>> -     pwm0: pwm@f8034000 {
>> -             compatible = "atmel,at91sam9rl-pwm";
>> -             reg = <0xf8034000 0x400>;
>> -             #pwm-cells = <3>;
>> -     };
>> -
>> -     pwmleds {
>> -             compatible = "pwm-leds";
>> -
>> -             d1 {
>> -                     label = "d1";
>> -                     pwms = <&pwm0 3 5000 0>
>> -                     max-brightness = <255>;
>> -             };
>> -
>> -             d2 {
>> -                     label = "d2";
>> -                     pwms = <&pwm0 1 5000 1>
>> -                     max-brightness = <255>;
>> -             };
>> -     };
>> diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.yaml b/Documentation/devicetree/bindings/pwm/atmel-pwm.yaml
>> new file mode 100644
>> index 000000000000..f5378a2226bd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.yaml
>> @@ -0,0 +1,62 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/atmel-pwm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel PWM controller
> title: Atmel/Microchip PWM controller
>
>> +
>> +maintainers:
>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>> +
> allOf including pwm.yaml
>
Noted.

>> +properties:
>> +  compatible:
>> +    enum:
>> +      - atmel,at91sam9rl-pwm
>> +      - atmel,sama5d3-pwm
>> +      - atmel,sama5d2-pwm
>> +      - microchip,sam9x60-pwm
>> +
>> +  reg:
>> +    description: |
>> +      Physical base address and length of the controller's
>> +      registers
> Skip description, it's obvious.
>
>> +    maxItems: 1
>> +
>> +  "#pwm-cells":
>> +    description: |
>> +      Should be 3. See pwm.yaml in this directory for a
>> +      description of the cells format.
> Skip description, it's obvious.
>
>> +    const: 3
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#pwm-cells"
> Skip pwm-cells, required by pwm.yaml.
>
Understood, will remove it in the next version along with the unnecessary descriptions.

>> +
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +        pwm0: pwm@f8034000 {
>> +                compatible = "atmel,at91sam9rl-pwm";
>> +                reg = <0xf8034000 0x400>;
>> +                #pwm-cells = <3>;
>> +        };
>> +
>> +        pwmleds {
> Skip this node. No actual benefit. All clients are the same.
>
Understood. I will include `pwm0` only.

>> +                compatible = "pwm-leds";
>> +
>> +                led-1 {
>> +                        label = "led-1";
>> +                        pwms = <&pwm0 3 5000 0>;
>> +                        max-brightness = <255>;
>> +                };
>> +
>> +                led-2 {
>> +                        label = "led-2";
>> +                        pwms = <&pwm0 1 5000 1>;
>> +                        max-brightness = <255>;
>> +                };
>> +        };
>
> Best regards,
> Krzysztof

Thank you for the detailed feedback.

Sergiu