Re: [RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2

[Ben Dooks <ben.dooks@sifive.com>]

On 17/08/2022 06:58, Krzysztof Kozlowski wrote:
> On 17/08/2022 00:14, Ben Dooks wrote:
>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>> as we will be adding DT/platform support to the Linux driver soon.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>> v4:
>>   - fixed typos, added reg
>> v3:
>>   - add description and example
>>   - merge the snps,pwm-number into this patch
>>   - rename snps,pwm to snps,dw-apb-timers-pwm2
>> v2:
>>   - fix #pwm-cells to be 3
>>   - fix indentation and ordering issues
>> ---
>>   .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 69 +++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>> new file mode 100644
>> index 000000000000..e7feae6d4404
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>> @@ -0,0 +1,69 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 SiFive, Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/snps,dw-apb-timers-pwm2.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Synopsys DW-APB timers PWM controller
>> +
>> +maintainers:
>> +  - Ben Dooks <ben.dooks@sifive.com>
>> +
>> +description:
>> +  This describes the DesignWare APB timers module when used in the PWM
>> +  mode. The IP core can be generated with various options which can
>> +  control the functionality, the number of PWMs available and other
>> +  internal controls the designer requires.
>> +
>> +  The IP block has a version register so this can be used for detection
>> +  instead of having to encode the IP version number in the device tree
>> +  comaptible.
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: snps,dw-apb-timers-pwm2
>> +
>> +  "#pwm-cells":
>> +    const: 3
>> +
>> +  clocks:
>> +    items:
>> +      - description: Interface bus clock
>> +      - description: PWM reference clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: bus
>> +      - const: timer
>> +
>> +  snps,pwm-number:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: The number of PWM channels configured for this instance
>> +    enum: [1, 2, 3, 4, 5, 6, 7, 8]
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - "#pwm-cells"
>> +  - compatible
>> +  - reg
> 
> Keep the same order as list of properties.

ok, will fix.

>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +
> 
> Just one blank line.
> 
>> +examples:
>> +  - |
>> +    pwm: pwm@180000 {
>> +      #pwm-cells = <3>;
>> +      compatible = "snps,dw-apb-timers-pwm2";
>> +      reg = <0x180000 0x200>;
> 
> The convention of DTS is: compatible, then reg, then rest of properties.
> 
>> +      clocks = <&bus &timer>;
> 
> You put here one item, not two. This has to be <&bus>, <&timer>
> 
>> +      clock-names = "bus", "timer";
>> +    };

Argh, thanks, I completely missed this as our platform only has
one clock provider for this (the bus and timer clocks are the
same)


> Best regards,
> Krzysztof

Thanks for the review.

I guess this is now too late for 6.0-rc ?