linux-kernel - Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs

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] [day] [month] [year] [list]

Message-ID: <CACRpkdZn75ks4Gc7rm8jzkKM6y0JeQmUF3qmbJA+O+cEA9r--Q@mail.gmail.com>
Date: Sun, 22 Dec 2024 10:08:50 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Xianwei Zhao <xianwei.zhao@...ogic.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>, 
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>, linux-gpio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs

Hi Xianwei!

On Wed, Dec 18, 2024 at 10:37 AM Xianwei Zhao <xianwei.zhao@...ogic.com> wrote:

> [Me]
> > In any way I recommend:
> >
> > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic
> >    so we keep this sorted by actual vendor, sunxi is apparently
> >    yours (AMlogic:s) isn't it?
> >
>
> It isn't. Sunxi is Allwinner SoCs.

My apologies. I mixed it up completely. :(

What do you think of the idea of a separate drivers/pinctrl/amlogic directory
though? I think there are already quite a few amlogic SoCs that need
to be supported and more will come.

> >> +       ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs,
> >> +                                             &grp->num_configs);
> >
> > But can't you just move this code around? grp->num_configs give the
> > number of configs, so why do you have to go and look up pinmux
> > above, can't you just use grp->num_configs instead of of_pins
> > and npins above?
> >
> They are different.
> The of_pins(grp->npins) specifies the mux values for pin-mux register
> and pin index in pinctrl. It can include multiple pins in groups.
>
> The grp->configs and grp->num_configs specify the configuration
> information for all pins of this groups(such as bias-pull-up,
> drive-strength-microamp)
>
> uart-d-pins2{
>         pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>,
>                 <AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>,
>                 <AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>,
>                 <AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>;
>         bias-pull-up;
>         drive-strength-microamp = <4000>;
> };

OK I get it ... I think. It's nice that you combine muxing and pin config
into the same node like this, it's very readable.

Think about if you even want to add generic helpers for this in
the generic code.

> >> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info,
> >> +                                   struct device_node *np)
> >> +{
> >> +       struct device_node *child;
> >> +
> >> +       for_each_child_of_node(np, child) {
> >> +               if (of_property_read_bool(child, "gpio-controller")) {
> >> +                       info->nbanks++;
> >> +               } else {
> >> +                       info->nfunctions++;
> >> +                       info->ngroups += of_get_child_count(child);
> >> +               }
> >> +       }
> >> +}
> >
> > This looks like a weird dependency between gpio chips and
> > pins that I don't quite understand. Some comments and
> > references to the bindings will be needed so it is clear
> > what is going on.
> >
>
> A pinctrl device contains two types of nodes. The one named GPIO bank
> which includes "gpio-controller" property. The other one named function
> which includes one or more pin groups.
> The pin group include pinmux property(pin index in pinctrl dev,and mux
> vlaue in mux reg) and pin configuration properties.

OK I  guess the binding patch explains why you need several
separate gpio controller "bank" nodes instead of just the base
node being one for all of the pins (which is the most
common). In a way I like it because it often helps to divide
up GPIOs by bank.

Yours,
Linus Walleij