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