Re: [PATCH v3 1/4] dt-bindings: clock: airoha: Add reset support to EN7581 clock binding

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 27/06/24 11:57, Conor Dooley ha scritto:
> On Thu, Jun 27, 2024 at 11:53:00AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 27/06/24 11:47, Conor Dooley ha scritto:
>>> On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote:
>>>> Il 13/06/24 14:47, Lorenzo Bianconi ha scritto:
>>>>> Introduce reset capability to EN7581 device-tree clock binding
>>>>> documentation.
>>>>>
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>>>> ---
>>>>>     .../bindings/clock/airoha,en7523-scu.yaml     | 25 ++++++-
>>>>>     .../dt-bindings/reset/airoha,en7581-reset.h   | 66 +++++++++++++++++++
>>>>>     2 files changed, 90 insertions(+), 1 deletion(-)
>>>>>     create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
>>>>> index 3f4266637733..84353fd09428 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
>>>>> @@ -35,7 +35,7 @@ properties:
>>>>>       reg:
>>>>>         minItems: 2
>>>>> -    maxItems: 3
>>>>> +    maxItems: 4
>>>>>       "#clock-cells":
>>>>>         description:
>>>>> @@ -43,6 +43,10 @@ properties:
>>>>>           clocks.
>>>>>         const: 1
>>>>> +  '#reset-cells':
>>>>> +    description: ID of the controller reset line
>>>>> +    const: 1
>>>>> +
>>>>>     required:
>>>>>       - compatible
>>>>>       - reg
>>>>> @@ -60,6 +64,8 @@ allOf:
>>>>>                 - description: scu base address
>>>>>                 - description: misc scu base address
>>>>> +        '#reset-cells': false
>>>>> +
>>>>>       - if:
>>>>>           properties:
>>>>>             compatible:
>>>>> @@ -70,6 +76,7 @@ allOf:
>>>>>               items:
>>>>>                 - description: scu base address
>>>>>                 - description: misc scu base address
>>>>> +            - description: reset base address
>>>>
>>>> Are you sure that the indentation is correct? :-)
>>>>
>>>> After fixing the indentation,
>>>>
>>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>
>>>>>                 - description: pb scu base address
>>>
>>> The indentation actually looks okay when I apply this locally, but how is
>>> it backwards compatible to add this register in the middle of the list??
>>
>> It's not, and this is actually something done on purpose - there is no DT using
>> this binding yet (here, nor uboot), and Lorenzo acknowledged the mistake before
>> it was too late...
>>
>> At least this time, it wasn't a misattention :-P
> 
> The rationale for this being okay really should be in the commit
> message...
> 
>> Btw, as far as I know, the reset base address is in between misc scu and pb scu,
>> that's why it was put there in the middle.
> 
> ...but I don't really see why this needs to be done incompatibly at all
> in the first place. Just put it at the end of the list, there's no rule
> that the order of reg properties needs to match the MMIO addresses.
> 

It's just a perfection thing... but whatever, if that's unacceptable for you,
putting it at the end of the list isn't a huge deal anyway.

Your call - it's okay for me either way.

Cheers,
Angelo