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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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