Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b3605922-a0ef-7d44-7e83-3146b6c17068@sifive.com>
Date:   Thu, 18 Aug 2022 14:43:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.1
Subject: Re: [RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare
 snps,pwm-dw-apb-timers-pwm2
Content-Language: en-GB
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        linux-pwm@vger.kernel.org
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lee Jones <lee.jones@linaro.org>,
        u.kleine-koenig@pengutronix.de,
        Thierry Reding <thierry.reding@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Greentime Hu <greentime.hu@sifive.com>,
        jarkko.nikula@linux.intel.com,
        William Salmon <william.salmon@sifive.com>,
        Jude Onyenegecha <jude.onyenegecha@sifive.com>
References: <20220816211454.237751-1-ben.dooks@sifive.com>
 <20220816211454.237751-2-ben.dooks@sifive.com>
 <f07d2072-4056-bf2e-82d5-271d8b8792e6@linaro.org>
From:   Ben Dooks <ben.dooks@sifive.com>
In-Reply-To: <f07d2072-4056-bf2e-82d5-271d8b8792e6@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 ?