Re: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Phil Edworthy <phil.edworthy@renesas.com>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
Date: Sat, 11 Jun 2022 15:22:12 +0200	[thread overview]
Message-ID: <94f03fed-73f5-ce1e-7bbd-2f53f461816d@linaro.org> (raw)
In-Reply-To: <TYYPR01MB7086EFE64F1DF8C6141E8719F5A69@TYYPR01MB7086.jpnprd01.prod.outlook.com>

On 10/06/2022 16:38, Phil Edworthy wrote:
> Hi Krzysztof,
> 
> Thanks for your review.
> 
> On 08 June 2022 11:52 Krzysztof Kozlowski wrote:
>> On 07/06/2022 15:56, Phil Edworthy wrote:
>>> Add the documentation for the r9a09g011 SoC, but in doing so also
>>> reorganise the doc to make it easier to read.
>>> Additionally, make the binding require an interrupt to be specified.
>>> Whilst the driver does not need an interrupt, all of the SoCs that use
>>> this binding actually provide one.
>>>
>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>>  .../bindings/watchdog/renesas,wdt.yaml        | 63 ++++++++++++-------
>>>  1 file changed, 42 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>> b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> index a8d7dde5271b..6473734921e3 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> @@ -31,6 +31,11 @@ properties:
>>>                - renesas,r9a07g054-wdt    # RZ/V2L
>>>            - const: renesas,rzg2l-wdt
>>>
>>> +      - items:
>>> +          - enum:
>>> +              - renesas,r9a09g011-wdt    # RZ/V2M
>>> +          - const: renesas,rzv2m-wdt     # RZ/V2M
>>> +
>>>        - items:
>>>            - enum:
>>>                - renesas,r8a7742-wdt      # RZ/G1H
>>> @@ -70,13 +75,27 @@ properties:
>>>    reg:
>>>      maxItems: 1
>>>
>>> -  interrupts: true
>>> +  interrupts:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: Timeout
>>> +      - description: Parity error
>>>
>>> -  interrupt-names: true
>>> +  interrupt-names:
>>
>> This also needs minItems
> I left minItems off for interrupt-names and clock-names on the basis that
> they are only needed if you have more than one interrupt or clock.

True, but now you disallow them for one clock/interrupt cases in other
variants. Although after looking at existing bindings - it's even
messier there. For certain variants it is just ":true" which is not correct.

In general, the properties in "properties:" section should have
constraints - the most wide. These are narrowed for specific variants or
even disallowed for some. Old bindings allowed anything for some
variants, like 20 interrupt names so clearly wrong.

> 
> After adding the lines you suggested (minItems: 1), I find that
> 'make dtbs_check' passes even if there are no interrupt-names or
> clock-names specified. Is this expected?

These are not required, aren't they? If they are not required, they can
be missing...

> 
> minItems: 0 makes more sense to me, but it is required to be greater than
> or equal 1
> 
> Thanks
> Phil

Best regards,
Krzysztof