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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Linus,
    Thanks for your reply.

On 2024/12/22 17:08, Linus Walleij wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Xianwei!
> 
> On Wed, Dec 18, 2024 at 10:37 AM Xianwei Zhao <xianwei.zhao@amlogic.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.
> 

 From the existing specifications of several subsequent chips, the 
support for the new chip does not require additional files, and there 
may be a little difference for special bank in the future chip, which 
can be solved by private architecture.

>>>> +       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.
> 

I will try to add API for pinmux property to pinconf-generic.c.

>>>> +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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic