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: <1d4573fc-407a-13c2-b049-e7a060d7929b@sifive.com>
Date:   Wed, 27 Jul 2022 11:32:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [[PATCH v2] 1/9] dt-bindings: pwm: Document Synopsys DesignWare
 snps,pwm
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>
References: <20220725212140.741644-1-ben.dooks@sifive.com>
 <922628f6-cbb1-b563-6464-e57959bafbcd@linaro.org>
 <8bb5103d-803e-90d2-fd93-132bb2aac2d6@sifive.com>
 <6317212b-1fca-65b4-9bce-0b9f7408fdae@linaro.org>
From:   Ben Dooks <ben.dooks@sifive.com>
In-Reply-To: <6317212b-1fca-65b4-9bce-0b9f7408fdae@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26/07/2022 12:05, Krzysztof Kozlowski wrote:
> On 26/07/2022 12:12, Ben Dooks wrote:
>> On 26/07/2022 11:05, Krzysztof Kozlowski wrote:
>>> On 25/07/2022 23:21, 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>
>>>> --
>>>
>>> This is not proper delimiter and causes the changelog to end up in commit.
>>>
>>> Correct also wrong formatting of subject PATCH.
>>
>> I realised that once sent and forgot the cover letter.
>> Maybe I'll try some more post covid recovery.
>>
>>>> v2:
>>>> - fix #pwm-cells to be 3
>>>> - fix indentation and ordering issues
>>>> ---
>>>>    .../devicetree/bindings/pwm/snps,pwm.yaml     | 40 +++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/snps,pwm.yaml b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> new file mode 100644
>>>> index 000000000000..594085e5e26f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/snps,pwm.yaml
>>>> @@ -0,0 +1,40 @@
>>>> +# 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,pwm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Synopsys PWM controller
>>>> +
>>>> +maintainers:
>>>> +  - Ben Dooks <ben.dooks@sifive.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: pwm.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: snps,pwm
>>>
>>> This is very generic compatible. I doubt that you cover here all
>>> Synopsys PWM designs, past and future. You need a specific compatible.
>>
>>   From what I can get from the documentation (2.13a) there hasn't been
>> a huge external interface change and what has been added is all part
>> of synthesis time options.
> 
> But you have some specific version, right? Usually these blocks are
> versioned, so you must include it. I would even argue that such generic
> compatible should not be used as fallback at all, because it is simply
> to generic (PWM is not some model name but common acronym),

I suppose dw-apb-timers is the actual document name, but that's already
been used for the timer mode in a number of SoCs so probably isn't going
to be useful. dw-apb-timers-pwm might be a better prefix if snps,pwm is
not going to be acceptable. (Yes, the block can be built as either a
PWM or a generic interrupt generating timer at IP generation time)

As for the version numbers, we could have the -v.vv suffix for these
blocks, but the v2.xx log has 22 entries already and only one feature
for programming (which is also a configurable one so can't be just
enabled by default - it's the 0/100 mode flag in the control registers).

I'm not sure what the v1.xx timers had, but I don't have access to this
information and we're getting these documents as second-generation so I
am not sure if we can get a v1.xx at-all (I suspect this is also going
to have a number of revisions and about 1 useful register api change
which would be the "new mode" double counter method which we currently
rely on having being implicitly enabled by the IP builder (again this
feature is still something that can be configured on IP genaration))

Given the configurability of the core, the version numbers might be
usable at some point, but it does seem to be a lot of churn for what
currently can be described by one boolean for the 0/100 feature that
might-be available. Is there a way of saying the compatible string
can be dw-apb-timers-pwm-2.[0-9][0-9][a-z] ?