linux-kernel - Re: [PATCH 5/7] dt-bindings: pinctrl: add compatible for Allwinner A523/T527

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGb2v663LeDtk1ZGOAztQ+Sx5OtbSx+xkLgypZYRjXkAr6_GRA@mail.gmail.com>
Date: Tue, 14 Jan 2025 15:01:31 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Andre Przywara <andre.przywara@....com>
Cc: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] dt-bindings: pinctrl: add compatible for Allwinner A523/T527

On Wed, Nov 20, 2024 at 6:12 PM Andre Przywara <andre.przywara@....com> wrote:
>
> On Wed, 13 Nov 2024 16:50:19 +0800
> Chen-Yu Tsai <wens@...e.org> wrote:
>
> Hi Chen-Yu,
>
> sorry for the late reply, I was away for a week.
>
> > On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@....com> wrote:
> > >
> > > The A523 contains a pin controller similar to previous SoCs, although
> > > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable.
> > > This introduces a new style of binding, where the pinmux values for each
> > > pin group is stored in the new "allwinner,pinmux" property in the DT
> > > node, instead of requiring every driver to store a mapping between the
> > > function names and the required pinmux.
> > >
> > > Add the new name to the list of compatible strings, and required it to
> > > have 10 interrupts described. Also add the new pinmux property.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@....com>
> > > ---
> > >  .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  | 23 +++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > index 4502405703145..6fc18e92e1e94 100644
> > > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > @@ -56,6 +56,8 @@ properties:
> > >        - allwinner,sun50i-h6-r-pinctrl
> > >        - allwinner,sun50i-h616-pinctrl
> > >        - allwinner,sun50i-h616-r-pinctrl
> > > +      - allwinner,sun55i-a523-pinctrl
> > > +      - allwinner,sun55i-a523-r-pinctrl
> > >        - allwinner,suniv-f1c100s-pinctrl
> > >        - nextthing,gr8-pinctrl
> > >
> > > @@ -64,7 +66,7 @@ properties:
> > >
> > >    interrupts:
> > >      minItems: 1
> > > -    maxItems: 8
> > > +    maxItems: 10
> > >      description:
> > >        One interrupt per external interrupt bank supported on the
> > >        controller, sorted by bank number ascending order.
> > > @@ -119,13 +121,17 @@ patternProperties:
> > >          $ref: /schemas/types.yaml#/definitions/uint32
> > >          enum: [10, 20, 30, 40]
> > >
> > > +      allwinner,pinmux:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +        description: pinmux selector for each pin
> > > +
> >
> > Why not just the standard "pinmux" property, as given in
> > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
>
> I had it like this in my last post two years ago, but learned from
> LinusW [1] that the generic pinmux property has a slightly different
> meaning, and abusing it for just the pinmux index values would not match
> the generic definition.
> We *could* use the generic definition, but then this would include what's
> in the "pins" property, like I sketched out in the cover letter, as an
> alternative to this approach:
>         pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>;
> Where the SUNXI_PIN macro would combine the pin number and the pinmux into
> one 32-bit cell. See the Apple GPIO DT nodes for an example.
> This looks indeed nicer, but requires quite some rewrite of the existing
> pinctrl driver, AFAICS.

Sorry for taking so long to get back to this.

Could we maybe add a generic replacement of the existing "function"
property, which takes a string? Like "function-id" or "function-selector"
that takes u32 (or u8). Then it could be one or the other. Not sure
if the binding maintainers would accept this or not.

I understand that we probably don't want the mux value combined with
the pin number.

ChenYu

> [1] Previous reply from LinusW:
> https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/
>
> >
> > >      required:
> > >        - pins
> > >        - function
> >
> > This section should be made to apply only to the existing
> > compatibles? Maybe we could just split the files and have
> > a clean slate for sun55i?
>
> Yeah, I couldn't find a good example how to make it *required* for one
> compatible and *not allowed* for all the others. But creating a whole new
> file is actually a good idea, as this also avoids adding another case to
> the already quite indented if-else cascade.
>
> Cheers,
> Andre
>
> > ChenYu
> >
> > >      additionalProperties: false
> > >
> > > -  "^vcc-p[a-ilm]-supply$":
> > > +  "^vcc-p[a-klm]-supply$":
> > >      description:
> > >        Power supplies for pin banks.
> > >
> > > @@ -156,6 +162,17 @@ allOf:
> > >          - interrupts
> > >          - interrupt-controller
> > >
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          enum:
> > > +            - allwinner,sun55i-a523-pinctrl
> > > +
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          minItems: 10
> > > +
> > >    - if:
> > >        properties:
> > >          compatible:
> > > @@ -166,6 +183,7 @@ allOf:
> > >        properties:
> > >          interrupts:
> > >            minItems: 8
> > > +          maxItems: 8
> > >
> > >    - if:
> > >        properties:
> > > @@ -244,6 +262,7 @@ allOf:
> > >              - allwinner,sun8i-v3s-pinctrl
> > >              - allwinner,sun9i-a80-r-pinctrl
> > >              - allwinner,sun50i-h6-r-pinctrl
> > > +            - allwinner,sun55i-a523-r-pinctrl
> > >
> > >      then:
> > >        properties:
> > > --
> > > 2.46.2
> > >
>