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

[Linus Walleij]

Hi Xianwei!

On Wed, Dec 18, 2024 at 10:37 AM Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx> 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