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: 4074 bytes --]

On Thu, Jun 27, 2024 at 12:03:38PM +0200, AngeloGioacchino Del Regno wrote:
> 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.

It has not been in a release yet, so it's not a hard block for me,
however, I would want to see a commit message update and a Fixes: tag.
The patch should also appear in 6.10, rather than being 6.11 material.

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