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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]