From: Weihao Li <[email protected]>
The current version of the pinctrl driver has some issues:
1. Duplicated "gpio" pmx function
The common code will add a "gpio" pmx functon to every pin group, so
it's not necessary to define a separate "gpio" pmx function in pin
groups.
2. Duplicated pmx function name
There are some same function name in different pin groups, which will
cause some problems. For example, when we want to use PAD_GPIO0 as
refclk output function, the common clk framework code will search the
entire pin function lists, then return the first one matched, in this
case the matched function list only include the PAD_CO_CLKO pin group
because there are three "refclk" pin function, which is added by
refclk_grp, spi_cs1_grp and gpio_grp.
To solve this problem, a simple way is just add a pingrp refix to
function name like mt7620 pinctrl driver does.
3. Useless "-" or "rsvd" functon
It's really unnecessary to add a reserved pin mux function to the
function lists, because we never use it.
Signed-off-by: Weihao Li <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-mt76x8.c | 88 +++++++----------------
1 file changed, 27 insertions(+), 61 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
index e7d6ad2f62e4e..2bc8d4409ca27 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
@@ -37,36 +37,30 @@
static struct mtmips_pmx_func pwm1_grp[] = {
FUNC("sdxc d6", 3, 19, 1),
- FUNC("utif", 2, 19, 1),
- FUNC("gpio", 1, 19, 1),
+ FUNC("pwm1 utif", 2, 19, 1),
FUNC("pwm1", 0, 19, 1),
};
static struct mtmips_pmx_func pwm0_grp[] = {
FUNC("sdxc d7", 3, 18, 1),
- FUNC("utif", 2, 18, 1),
- FUNC("gpio", 1, 18, 1),
+ FUNC("pwm0 utif", 2, 18, 1),
FUNC("pwm0", 0, 18, 1),
};
static struct mtmips_pmx_func uart2_grp[] = {
FUNC("sdxc d5 d4", 3, 20, 2),
- FUNC("pwm", 2, 20, 2),
- FUNC("gpio", 1, 20, 2),
+ FUNC("uart2 pwm", 2, 20, 2),
FUNC("uart2", 0, 20, 2),
};
static struct mtmips_pmx_func uart1_grp[] = {
FUNC("sw_r", 3, 45, 2),
- FUNC("pwm", 2, 45, 2),
- FUNC("gpio", 1, 45, 2),
+ FUNC("uart1 pwm", 2, 45, 2),
FUNC("uart1", 0, 45, 2),
};
static struct mtmips_pmx_func i2c_grp[] = {
- FUNC("-", 3, 4, 2),
FUNC("debug", 2, 4, 2),
- FUNC("gpio", 1, 4, 2),
FUNC("i2c", 0, 4, 2),
};
@@ -76,128 +70,100 @@ static struct mtmips_pmx_func wdt_grp[] = { FUNC("wdt", 0, 38, 1) };
static struct mtmips_pmx_func spi_grp[] = { FUNC("spi", 0, 7, 4) };
static struct mtmips_pmx_func sd_mode_grp[] = {
- FUNC("jtag", 3, 22, 8),
- FUNC("utif", 2, 22, 8),
- FUNC("gpio", 1, 22, 8),
+ FUNC("sdxc jtag", 3, 22, 8),
+ FUNC("sdxc utif", 2, 22, 8),
FUNC("sdxc", 0, 22, 8),
};
static struct mtmips_pmx_func uart0_grp[] = {
- FUNC("-", 3, 12, 2),
- FUNC("-", 2, 12, 2),
- FUNC("gpio", 1, 12, 2),
FUNC("uart0", 0, 12, 2),
};
static struct mtmips_pmx_func i2s_grp[] = {
FUNC("antenna", 3, 0, 4),
FUNC("pcm", 2, 0, 4),
- FUNC("gpio", 1, 0, 4),
FUNC("i2s", 0, 0, 4),
};
static struct mtmips_pmx_func spi_cs1_grp[] = {
- FUNC("-", 3, 6, 1),
- FUNC("refclk", 2, 6, 1),
- FUNC("gpio", 1, 6, 1),
+ FUNC("spi refclk", 2, 6, 1),
FUNC("spi cs1", 0, 6, 1),
};
static struct mtmips_pmx_func spis_grp[] = {
FUNC("pwm_uart2", 3, 14, 4),
- FUNC("utif", 2, 14, 4),
- FUNC("gpio", 1, 14, 4),
+ FUNC("spis utif", 2, 14, 4),
FUNC("spis", 0, 14, 4),
};
static struct mtmips_pmx_func gpio_grp[] = {
FUNC("pcie", 3, 11, 1),
- FUNC("refclk", 2, 11, 1),
- FUNC("gpio", 1, 11, 1),
- FUNC("gpio", 0, 11, 1),
+ FUNC("gpio refclk", 2, 11, 1),
};
static struct mtmips_pmx_func p4led_kn_grp[] = {
- FUNC("jtag", 3, 30, 1),
- FUNC("utif", 2, 30, 1),
- FUNC("gpio", 1, 30, 1),
+ FUNC("p4led_kn jtag", 3, 30, 1),
+ FUNC("p4led_kn utif", 2, 30, 1),
FUNC("p4led_kn", 0, 30, 1),
};
static struct mtmips_pmx_func p3led_kn_grp[] = {
- FUNC("jtag", 3, 31, 1),
- FUNC("utif", 2, 31, 1),
- FUNC("gpio", 1, 31, 1),
+ FUNC("p3led_kn jtag", 3, 31, 1),
+ FUNC("p3led_kn utif", 2, 31, 1),
FUNC("p3led_kn", 0, 31, 1),
};
static struct mtmips_pmx_func p2led_kn_grp[] = {
- FUNC("jtag", 3, 32, 1),
- FUNC("utif", 2, 32, 1),
- FUNC("gpio", 1, 32, 1),
+ FUNC("p2led_kn jtag", 3, 32, 1),
+ FUNC("p2led_kn utif", 2, 32, 1),
FUNC("p2led_kn", 0, 32, 1),
};
static struct mtmips_pmx_func p1led_kn_grp[] = {
- FUNC("jtag", 3, 33, 1),
- FUNC("utif", 2, 33, 1),
- FUNC("gpio", 1, 33, 1),
+ FUNC("p1led_kn jtag", 3, 33, 1),
+ FUNC("p1led_kn utif", 2, 33, 1),
FUNC("p1led_kn", 0, 33, 1),
};
static struct mtmips_pmx_func p0led_kn_grp[] = {
- FUNC("jtag", 3, 34, 1),
- FUNC("rsvd", 2, 34, 1),
- FUNC("gpio", 1, 34, 1),
+ FUNC("p0led_kn jtag", 3, 34, 1),
FUNC("p0led_kn", 0, 34, 1),
};
static struct mtmips_pmx_func wled_kn_grp[] = {
- FUNC("rsvd", 3, 35, 1),
- FUNC("rsvd", 2, 35, 1),
- FUNC("gpio", 1, 35, 1),
FUNC("wled_kn", 0, 35, 1),
};
static struct mtmips_pmx_func p4led_an_grp[] = {
- FUNC("jtag", 3, 39, 1),
- FUNC("utif", 2, 39, 1),
- FUNC("gpio", 1, 39, 1),
+ FUNC("p4led_an jtag", 3, 39, 1),
+ FUNC("p4led_an utif", 2, 39, 1),
FUNC("p4led_an", 0, 39, 1),
};
static struct mtmips_pmx_func p3led_an_grp[] = {
- FUNC("jtag", 3, 40, 1),
- FUNC("utif", 2, 40, 1),
- FUNC("gpio", 1, 40, 1),
+ FUNC("p3led_an jtag", 3, 40, 1),
+ FUNC("p3led_an utif", 2, 40, 1),
FUNC("p3led_an", 0, 40, 1),
};
static struct mtmips_pmx_func p2led_an_grp[] = {
- FUNC("jtag", 3, 41, 1),
- FUNC("utif", 2, 41, 1),
- FUNC("gpio", 1, 41, 1),
+ FUNC("p2led_an jtag", 3, 41, 1),
+ FUNC("p2led_an utif", 2, 41, 1),
FUNC("p2led_an", 0, 41, 1),
};
static struct mtmips_pmx_func p1led_an_grp[] = {
- FUNC("jtag", 3, 42, 1),
- FUNC("utif", 2, 42, 1),
- FUNC("gpio", 1, 42, 1),
+ FUNC("p1led_an jtag", 3, 42, 1),
+ FUNC("p1led_an utif", 2, 42, 1),
FUNC("p1led_an", 0, 42, 1),
};
static struct mtmips_pmx_func p0led_an_grp[] = {
- FUNC("jtag", 3, 43, 1),
- FUNC("rsvd", 2, 43, 1),
- FUNC("gpio", 1, 43, 1),
+ FUNC("p0led_an jtag", 3, 43, 1),
FUNC("p0led_an", 0, 43, 1),
};
static struct mtmips_pmx_func wled_an_grp[] = {
- FUNC("rsvd", 3, 44, 1),
- FUNC("rsvd", 2, 44, 1),
- FUNC("gpio", 1, 44, 1),
FUNC("wled_an", 0, 44, 1),
};
--
2.39.2
Hi,
On Mon, May 27, 2024 at 4:21 AM liweihao <[email protected]> wrote:
>
> From: Weihao Li <[email protected]>
>
> The current version of the pinctrl driver has some issues:
Regarding the dtsi file which is in the kernel tree mt76x8 is using
'pinctrl-single' for pin muxing [0].
>
> 1. Duplicated "gpio" pmx function
>
> The common code will add a "gpio" pmx functon to every pin group, so
> it's not necessary to define a separate "gpio" pmx function in pin
> groups.
Do you mean that pin 0 always has a GPIO function? [1]
> 2. Duplicated pmx function name
>
> There are some same function name in different pin groups, which will
> cause some problems. For example, when we want to use PAD_GPIO0 as
> refclk output function, the common clk framework code will search the
> entire pin function lists, then return the first one matched, in this
> case the matched function list only include the PAD_CO_CLKO pin group
> because there are three "refclk" pin function, which is added by
> refclk_grp, spi_cs1_grp and gpio_grp.
>
> To solve this problem, a simple way is just add a pingrp refix to
> function name like mt7620 pinctrl driver does.
>
> 3. Useless "-" or "rsvd" functon
>
> It's really unnecessary to add a reserved pin mux function to the
> function lists, because we never use it.
>
> Signed-off-by: Weihao Li <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-mt76x8.c | 88 +++++++----------------
> 1 file changed, 27 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> index e7d6ad2f62e4e..2bc8d4409ca27 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> @@ -37,36 +37,30 @@
>
> static struct mtmips_pmx_func pwm1_grp[] = {
> FUNC("sdxc d6", 3, 19, 1),
> - FUNC("utif", 2, 19, 1),
> - FUNC("gpio", 1, 19, 1),
> + FUNC("pwm1 utif", 2, 19, 1),
> FUNC("pwm1", 0, 19, 1),
> };
>
> static struct mtmips_pmx_func pwm0_grp[] = {
> FUNC("sdxc d7", 3, 18, 1),
> - FUNC("utif", 2, 18, 1),
> - FUNC("gpio", 1, 18, 1),
> + FUNC("pwm0 utif", 2, 18, 1),
> FUNC("pwm0", 0, 18, 1),
> };
>
> static struct mtmips_pmx_func uart2_grp[] = {
> FUNC("sdxc d5 d4", 3, 20, 2),
> - FUNC("pwm", 2, 20, 2),
> - FUNC("gpio", 1, 20, 2),
> + FUNC("uart2 pwm", 2, 20, 2),
> FUNC("uart2", 0, 20, 2),
> };
>
> static struct mtmips_pmx_func uart1_grp[] = {
> FUNC("sw_r", 3, 45, 2),
> - FUNC("pwm", 2, 45, 2),
> - FUNC("gpio", 1, 45, 2),
> + FUNC("uart1 pwm", 2, 45, 2),
> FUNC("uart1", 0, 45, 2),
> };
>
> static struct mtmips_pmx_func i2c_grp[] = {
> - FUNC("-", 3, 4, 2),
> FUNC("debug", 2, 4, 2),
> - FUNC("gpio", 1, 4, 2),
> FUNC("i2c", 0, 4, 2),
> };
>
> @@ -76,128 +70,100 @@ static struct mtmips_pmx_func wdt_grp[] = { FUNC("wdt", 0, 38, 1) };
> static struct mtmips_pmx_func spi_grp[] = { FUNC("spi", 0, 7, 4) };
>
> static struct mtmips_pmx_func sd_mode_grp[] = {
> - FUNC("jtag", 3, 22, 8),
> - FUNC("utif", 2, 22, 8),
> - FUNC("gpio", 1, 22, 8),
> + FUNC("sdxc jtag", 3, 22, 8),
> + FUNC("sdxc utif", 2, 22, 8),
> FUNC("sdxc", 0, 22, 8),
> };
>
> static struct mtmips_pmx_func uart0_grp[] = {
> - FUNC("-", 3, 12, 2),
> - FUNC("-", 2, 12, 2),
> - FUNC("gpio", 1, 12, 2),
> FUNC("uart0", 0, 12, 2),
> };
>
> static struct mtmips_pmx_func i2s_grp[] = {
> FUNC("antenna", 3, 0, 4),
> FUNC("pcm", 2, 0, 4),
> - FUNC("gpio", 1, 0, 4),
> FUNC("i2s", 0, 0, 4),
> };
>
> static struct mtmips_pmx_func spi_cs1_grp[] = {
> - FUNC("-", 3, 6, 1),
> - FUNC("refclk", 2, 6, 1),
> - FUNC("gpio", 1, 6, 1),
> + FUNC("spi refclk", 2, 6, 1),
> FUNC("spi cs1", 0, 6, 1),
> };
>
> static struct mtmips_pmx_func spis_grp[] = {
> FUNC("pwm_uart2", 3, 14, 4),
> - FUNC("utif", 2, 14, 4),
> - FUNC("gpio", 1, 14, 4),
> + FUNC("spis utif", 2, 14, 4),
> FUNC("spis", 0, 14, 4),
> };
>
> static struct mtmips_pmx_func gpio_grp[] = {
> FUNC("pcie", 3, 11, 1),
> - FUNC("refclk", 2, 11, 1),
> - FUNC("gpio", 1, 11, 1),
> - FUNC("gpio", 0, 11, 1),
> + FUNC("gpio refclk", 2, 11, 1),
> };
>
> static struct mtmips_pmx_func p4led_kn_grp[] = {
> - FUNC("jtag", 3, 30, 1),
> - FUNC("utif", 2, 30, 1),
> - FUNC("gpio", 1, 30, 1),
> + FUNC("p4led_kn jtag", 3, 30, 1),
> + FUNC("p4led_kn utif", 2, 30, 1),
> FUNC("p4led_kn", 0, 30, 1),
> };
>
> static struct mtmips_pmx_func p3led_kn_grp[] = {
> - FUNC("jtag", 3, 31, 1),
> - FUNC("utif", 2, 31, 1),
> - FUNC("gpio", 1, 31, 1),
> + FUNC("p3led_kn jtag", 3, 31, 1),
> + FUNC("p3led_kn utif", 2, 31, 1),
> FUNC("p3led_kn", 0, 31, 1),
> };
>
> static struct mtmips_pmx_func p2led_kn_grp[] = {
> - FUNC("jtag", 3, 32, 1),
> - FUNC("utif", 2, 32, 1),
> - FUNC("gpio", 1, 32, 1),
> + FUNC("p2led_kn jtag", 3, 32, 1),
> + FUNC("p2led_kn utif", 2, 32, 1),
> FUNC("p2led_kn", 0, 32, 1),
> };
>
> static struct mtmips_pmx_func p1led_kn_grp[] = {
> - FUNC("jtag", 3, 33, 1),
> - FUNC("utif", 2, 33, 1),
> - FUNC("gpio", 1, 33, 1),
> + FUNC("p1led_kn jtag", 3, 33, 1),
> + FUNC("p1led_kn utif", 2, 33, 1),
> FUNC("p1led_kn", 0, 33, 1),
> };
>
> static struct mtmips_pmx_func p0led_kn_grp[] = {
> - FUNC("jtag", 3, 34, 1),
> - FUNC("rsvd", 2, 34, 1),
> - FUNC("gpio", 1, 34, 1),
> + FUNC("p0led_kn jtag", 3, 34, 1),
> FUNC("p0led_kn", 0, 34, 1),
> };
>
> static struct mtmips_pmx_func wled_kn_grp[] = {
> - FUNC("rsvd", 3, 35, 1),
> - FUNC("rsvd", 2, 35, 1),
> - FUNC("gpio", 1, 35, 1),
> FUNC("wled_kn", 0, 35, 1),
> };
>
> static struct mtmips_pmx_func p4led_an_grp[] = {
> - FUNC("jtag", 3, 39, 1),
> - FUNC("utif", 2, 39, 1),
> - FUNC("gpio", 1, 39, 1),
> + FUNC("p4led_an jtag", 3, 39, 1),
> + FUNC("p4led_an utif", 2, 39, 1),
> FUNC("p4led_an", 0, 39, 1),
> };
>
> static struct mtmips_pmx_func p3led_an_grp[] = {
> - FUNC("jtag", 3, 40, 1),
> - FUNC("utif", 2, 40, 1),
> - FUNC("gpio", 1, 40, 1),
> + FUNC("p3led_an jtag", 3, 40, 1),
> + FUNC("p3led_an utif", 2, 40, 1),
> FUNC("p3led_an", 0, 40, 1),
> };
>
> static struct mtmips_pmx_func p2led_an_grp[] = {
> - FUNC("jtag", 3, 41, 1),
> - FUNC("utif", 2, 41, 1),
> - FUNC("gpio", 1, 41, 1),
> + FUNC("p2led_an jtag", 3, 41, 1),
> + FUNC("p2led_an utif", 2, 41, 1),
> FUNC("p2led_an", 0, 41, 1),
> };
>
> static struct mtmips_pmx_func p1led_an_grp[] = {
> - FUNC("jtag", 3, 42, 1),
> - FUNC("utif", 2, 42, 1),
> - FUNC("gpio", 1, 42, 1),
> + FUNC("p1led_an jtag", 3, 42, 1),
> + FUNC("p1led_an utif", 2, 42, 1),
> FUNC("p1led_an", 0, 42, 1),
> };
>
> static struct mtmips_pmx_func p0led_an_grp[] = {
> - FUNC("jtag", 3, 43, 1),
> - FUNC("rsvd", 2, 43, 1),
> - FUNC("gpio", 1, 43, 1),
> + FUNC("p0led_an jtag", 3, 43, 1),
> FUNC("p0led_an", 0, 43, 1),
> };
>
> static struct mtmips_pmx_func wled_an_grp[] = {
> - FUNC("rsvd", 3, 44, 1),
> - FUNC("rsvd", 2, 44, 1),
> - FUNC("gpio", 1, 44, 1),
> FUNC("wled_an", 0, 44, 1),
> };
>
> --
> 2.39.2
>
Changes look good to me. However I cannot test them.
[+cc Shiji Yang] who probably has a related board to test.
Thanks,
Sergio Paracuellos
[0]: https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/ralink/mt7628a.dtsi#L45
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mtmips.c#L297
Hi. Thanks for your reply!
I forgot to reply all, so I resend this mail. Sorry about that orz.
Sergio Paracuellos <[email protected]> 于2024年5月28日周二 15:10写道：
>
> Hi,
>
> On Mon, May 27, 2024 at 4:21 AM liweihao <[email protected]> wrote:
> >
> > From: Weihao Li <[email protected]>
> >
> > The current version of the pinctrl driver has some issues:
>
> Regarding the dtsi file which is in the kernel tree mt76x8 is using
> 'pinctrl-single' for pin muxing [0].
>
You're right. But in my application, I need to set the PAD_GPIO0 as
refclk function, and the 'pinctrl-single' driver did not implement
this function, so I have to use 'ralink,mt76x8-pinctrl' instead of
'pinctrl-single'. And for some compatibility reasons, i prefer to use
the dedicated driver instead of 'pinctrl-single' driver. That's why I
just committed a single driver patch. the previous text 'current
version of the pinctrl driver' just means
drivers/pinctrl/mediatek/pinctrl-mt76x8.c.
> >
> > 1. Duplicated "gpio" pmx function
> >
> > The common code will add a "gpio" pmx functon to every pin group, so
> > it's not necessary to define a separate "gpio" pmx function in pin
> > groups.
>
> Do you mean that pin 0 always has a GPIO function? [1]
>
No. The mediatek pinctrl common code will add a 'gpio' function to
every pingrp [0].
> > 2. Duplicated pmx function name
> >
> > There are some same function name in different pin groups, which will
> > cause some problems. For example, when we want to use PAD_GPIO0 as
> > refclk output function, the common clk framework code will search the
> > entire pin function lists, then return the first one matched, in this
> > case the matched function list only include the PAD_CO_CLKO pin group
> > because there are three "refclk" pin function, which is added by
> > refclk_grp, spi_cs1_grp and gpio_grp.
> >
> > To solve this problem, a simple way is just add a pingrp refix to
> > function name like mt7620 pinctrl driver does.
> >
> > 3. Useless "-" or "rsvd" functon
> >
> > It's really unnecessary to add a reserved pin mux function to the
> > function lists, because we never use it.
> >
> > Signed-off-by: Weihao Li <[email protected]>
> > ---
> > drivers/pinctrl/mediatek/pinctrl-mt76x8.c | 88 +++++++----------------
> > 1 file changed, 27 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> > index e7d6ad2f62e4e..2bc8d4409ca27 100644
> > --- a/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> > +++ b/drivers/pinctrl/mediatek/pinctrl-mt76x8.c
> > @@ -37,36 +37,30 @@
> >
> > static struct mtmips_pmx_func pwm1_grp[] = {
> > FUNC("sdxc d6", 3, 19, 1),
> > - FUNC("utif", 2, 19, 1),
> > - FUNC("gpio", 1, 19, 1),
> > + FUNC("pwm1 utif", 2, 19, 1),
> > FUNC("pwm1", 0, 19, 1),
> > };
> >
> > static struct mtmips_pmx_func pwm0_grp[] = {
> > FUNC("sdxc d7", 3, 18, 1),
> > - FUNC("utif", 2, 18, 1),
> > - FUNC("gpio", 1, 18, 1),
> > + FUNC("pwm0 utif", 2, 18, 1),
> > FUNC("pwm0", 0, 18, 1),
> > };
> >
> > static struct mtmips_pmx_func uart2_grp[] = {
> > FUNC("sdxc d5 d4", 3, 20, 2),
> > - FUNC("pwm", 2, 20, 2),
> > - FUNC("gpio", 1, 20, 2),
> > + FUNC("uart2 pwm", 2, 20, 2),
> > FUNC("uart2", 0, 20, 2),
> > };
> >
> > static struct mtmips_pmx_func uart1_grp[] = {
> > FUNC("sw_r", 3, 45, 2),
> > - FUNC("pwm", 2, 45, 2),
> > - FUNC("gpio", 1, 45, 2),
> > + FUNC("uart1 pwm", 2, 45, 2),
> > FUNC("uart1", 0, 45, 2),
> > };
> >
> > static struct mtmips_pmx_func i2c_grp[] = {
> > - FUNC("-", 3, 4, 2),
> > FUNC("debug", 2, 4, 2),
> > - FUNC("gpio", 1, 4, 2),
> > FUNC("i2c", 0, 4, 2),
> > };
> >
> > @@ -76,128 +70,100 @@ static struct mtmips_pmx_func wdt_grp[] = { FUNC("wdt", 0, 38, 1) };
> > static struct mtmips_pmx_func spi_grp[] = { FUNC("spi", 0, 7, 4) };
> >
> > static struct mtmips_pmx_func sd_mode_grp[] = {
> > - FUNC("jtag", 3, 22, 8),
> > - FUNC("utif", 2, 22, 8),
> > - FUNC("gpio", 1, 22, 8),
> > + FUNC("sdxc jtag", 3, 22, 8),
> > + FUNC("sdxc utif", 2, 22, 8),
> > FUNC("sdxc", 0, 22, 8),
> > };
> >
> > static struct mtmips_pmx_func uart0_grp[] = {
> > - FUNC("-", 3, 12, 2),
> > - FUNC("-", 2, 12, 2),
> > - FUNC("gpio", 1, 12, 2),
> > FUNC("uart0", 0, 12, 2),
> > };
> >
> > static struct mtmips_pmx_func i2s_grp[] = {
> > FUNC("antenna", 3, 0, 4),
> > FUNC("pcm", 2, 0, 4),
> > - FUNC("gpio", 1, 0, 4),
> > FUNC("i2s", 0, 0, 4),
> > };
> >
> > static struct mtmips_pmx_func spi_cs1_grp[] = {
> > - FUNC("-", 3, 6, 1),
> > - FUNC("refclk", 2, 6, 1),
> > - FUNC("gpio", 1, 6, 1),
> > + FUNC("spi refclk", 2, 6, 1),
> > FUNC("spi cs1", 0, 6, 1),
> > };
> >
> > static struct mtmips_pmx_func spis_grp[] = {
> > FUNC("pwm_uart2", 3, 14, 4),
> > - FUNC("utif", 2, 14, 4),
> > - FUNC("gpio", 1, 14, 4),
> > + FUNC("spis utif", 2, 14, 4),
> > FUNC("spis", 0, 14, 4),
> > };
> >
> > static struct mtmips_pmx_func gpio_grp[] = {
> > FUNC("pcie", 3, 11, 1),
> > - FUNC("refclk", 2, 11, 1),
> > - FUNC("gpio", 1, 11, 1),
> > - FUNC("gpio", 0, 11, 1),
> > + FUNC("gpio refclk", 2, 11, 1),
> > };
> >
> > static struct mtmips_pmx_func p4led_kn_grp[] = {
> > - FUNC("jtag", 3, 30, 1),
> > - FUNC("utif", 2, 30, 1),
> > - FUNC("gpio", 1, 30, 1),
> > + FUNC("p4led_kn jtag", 3, 30, 1),
> > + FUNC("p4led_kn utif", 2, 30, 1),
> > FUNC("p4led_kn", 0, 30, 1),
> > };
> >
> > static struct mtmips_pmx_func p3led_kn_grp[] = {
> > - FUNC("jtag", 3, 31, 1),
> > - FUNC("utif", 2, 31, 1),
> > - FUNC("gpio", 1, 31, 1),
> > + FUNC("p3led_kn jtag", 3, 31, 1),
> > + FUNC("p3led_kn utif", 2, 31, 1),
> > FUNC("p3led_kn", 0, 31, 1),
> > };
> >
> > static struct mtmips_pmx_func p2led_kn_grp[] = {
> > - FUNC("jtag", 3, 32, 1),
> > - FUNC("utif", 2, 32, 1),
> > - FUNC("gpio", 1, 32, 1),
> > + FUNC("p2led_kn jtag", 3, 32, 1),
> > + FUNC("p2led_kn utif", 2, 32, 1),
> > FUNC("p2led_kn", 0, 32, 1),
> > };
> >
> > static struct mtmips_pmx_func p1led_kn_grp[] = {
> > - FUNC("jtag", 3, 33, 1),
> > - FUNC("utif", 2, 33, 1),
> > - FUNC("gpio", 1, 33, 1),
> > + FUNC("p1led_kn jtag", 3, 33, 1),
> > + FUNC("p1led_kn utif", 2, 33, 1),
> > FUNC("p1led_kn", 0, 33, 1),
> > };
> >
> > static struct mtmips_pmx_func p0led_kn_grp[] = {
> > - FUNC("jtag", 3, 34, 1),
> > - FUNC("rsvd", 2, 34, 1),
> > - FUNC("gpio", 1, 34, 1),
> > + FUNC("p0led_kn jtag", 3, 34, 1),
> > FUNC("p0led_kn", 0, 34, 1),
> > };
> >
> > static struct mtmips_pmx_func wled_kn_grp[] = {
> > - FUNC("rsvd", 3, 35, 1),
> > - FUNC("rsvd", 2, 35, 1),
> > - FUNC("gpio", 1, 35, 1),
> > FUNC("wled_kn", 0, 35, 1),
> > };
> >
> > static struct mtmips_pmx_func p4led_an_grp[] = {
> > - FUNC("jtag", 3, 39, 1),
> > - FUNC("utif", 2, 39, 1),
> > - FUNC("gpio", 1, 39, 1),
> > + FUNC("p4led_an jtag", 3, 39, 1),
> > + FUNC("p4led_an utif", 2, 39, 1),
> > FUNC("p4led_an", 0, 39, 1),
> > };
> >
> > static struct mtmips_pmx_func p3led_an_grp[] = {
> > - FUNC("jtag", 3, 40, 1),
> > - FUNC("utif", 2, 40, 1),
> > - FUNC("gpio", 1, 40, 1),
> > + FUNC("p3led_an jtag", 3, 40, 1),
> > + FUNC("p3led_an utif", 2, 40, 1),
> > FUNC("p3led_an", 0, 40, 1),
> > };
> >
> > static struct mtmips_pmx_func p2led_an_grp[] = {
> > - FUNC("jtag", 3, 41, 1),
> > - FUNC("utif", 2, 41, 1),
> > - FUNC("gpio", 1, 41, 1),
> > + FUNC("p2led_an jtag", 3, 41, 1),
> > + FUNC("p2led_an utif", 2, 41, 1),
> > FUNC("p2led_an", 0, 41, 1),
> > };
> >
> > static struct mtmips_pmx_func p1led_an_grp[] = {
> > - FUNC("jtag", 3, 42, 1),
> > - FUNC("utif", 2, 42, 1),
> > - FUNC("gpio", 1, 42, 1),
> > + FUNC("p1led_an jtag", 3, 42, 1),
> > + FUNC("p1led_an utif", 2, 42, 1),
> > FUNC("p1led_an", 0, 42, 1),
> > };
> >
> > static struct mtmips_pmx_func p0led_an_grp[] = {
> > - FUNC("jtag", 3, 43, 1),
> > - FUNC("rsvd", 2, 43, 1),
> > - FUNC("gpio", 1, 43, 1),
> > + FUNC("p0led_an jtag", 3, 43, 1),
> > FUNC("p0led_an", 0, 43, 1),
> > };
> >
> > static struct mtmips_pmx_func wled_an_grp[] = {
> > - FUNC("rsvd", 3, 44, 1),
> > - FUNC("rsvd", 2, 44, 1),
> > - FUNC("gpio", 1, 44, 1),
> > FUNC("wled_an", 0, 44, 1),
> > };
> >
> > --
> > 2.39.2
> >
>
> Changes look good to me. However I cannot test them.
>
> [+cc Shiji Yang] who probably has a related board to test.
>
> Thanks,
> Sergio Paracuellos
>
> [0]: https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/ralink/mt7628a.dtsi#L45
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mtmips.c#L297
[0]: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mtmips.c#L217
…
> There are some same function name in different pin groups, …
names?
…
> To solve this problem, a simple way is just add a pingrp refix to
pin group prefix?
Regards,
Markus
Markus Elfring <[email protected]> 于2024年5月28日周二 19:37写道：
>
> …
> > There are some same function name in different pin groups, …
>
> names?
>
refclk [0][1], pwm [2][3] and some others. For example, without this
patch, part of the pinmux-functions in the kernel debug fs looks like
this
root@mt7628:/sys/kernel/debug/pinctrl/pinctrl-ralink-pinmux# cat
pinmux-functions
function 0: gpio, groups = [ pwm1 pwm0 uart2 uart1 i2c refclk perst
wdt spi sdmode uart0 i2s spi cs1 spis gpio0 wled_an p0led_an p1led_an
p2led_an p3led_an p4led_an wled_kn p0led_kn p1led_kn p2led_kn p3led_kn
p4led_kn ]
function 1: sdxc d6, groups = [ pwm1 ]
function 2: utif, groups = [ pwm1 ]
function 3: gpio, groups = [ pwm1 ]
function 4: pwm1, groups = [ pwm1 ]
function 5: sdxc d7, groups = [ pwm0 ]
function 6: utif, groups = [ pwm0 ]
function 7: gpio, groups = [ pwm0 ]
function 8: pwm0, groups = [ pwm0 ]
function 9: sdxc d5 d4, groups = [ uart2 ]
function 10: pwm, groups = [ uart2 ]
function 11: gpio, groups = [ uart2 ]
function 12: uart2, groups = [ uart2 ]
function 13: sw_r, groups = [ uart1 ]
function 14: pwm, groups = [ uart1 ]
function 15: gpio, groups = [ uart1 ]
function 16: uart1, groups = [ uart1 ]
function 17: -, groups = [ i2c ]
function 18: debug, groups = [ i2c ]
function 19: gpio, groups = [ i2c ]
function 20: i2c, groups = [ i2c ]
function 21: refclk, groups = [ refclk ]
function 22: perst, groups = [ perst ]
function 23: wdt, groups = [ wdt ]
function 24: spi, groups = [ spi ]
function 25: jtag, groups = [ sdmode ]
function 26: utif, groups = [ sdmode ]
function 27: gpio, groups = [ sdmode ]
function 28: sdxc, groups = [ sdmode ]
function 29: -, groups = [ uart0 ]
function 30: -, groups = [ uart0 ]
function 31: gpio, groups = [ uart0 ]
function 32: uart0, groups = [ uart0 ]
function 33: antenna, groups = [ i2s ]
function 34: pcm, groups = [ i2s ]
function 35: gpio, groups = [ i2s ]
function 36: i2s, groups = [ i2s ]
function 37: -, groups = [ spi cs1 ]
function 38: refclk, groups = [ spi cs1 ]
>
> …
> > To solve this problem, a simple way is just add a pingrp refix to
>
> pin group prefix?
>
Yes. Just add a pin group prefix like pinctrl-mt7620.c does.
> Regards,
> Markus
[0] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mt76x8.c#L73
[1] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mt76x8.c#L101
[2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mt76x8.c#L54
[3] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mt76x8.c#L61
On Mon, May 27, 2024 at 4:21 AM liweihao <[email protected]> wrote:
> From: Weihao Li <[email protected]>
>
> The current version of the pinctrl driver has some issues:
>
> 1. Duplicated "gpio" pmx function
>
> The common code will add a "gpio" pmx functon to every pin group, so
> it's not necessary to define a separate "gpio" pmx function in pin
> groups.
>
> 2. Duplicated pmx function name
>
> There are some same function name in different pin groups, which will
> cause some problems. For example, when we want to use PAD_GPIO0 as
> refclk output function, the common clk framework code will search the
> entire pin function lists, then return the first one matched, in this
> case the matched function list only include the PAD_CO_CLKO pin group
> because there are three "refclk" pin function, which is added by
> refclk_grp, spi_cs1_grp and gpio_grp.
>
> To solve this problem, a simple way is just add a pingrp refix to
> function name like mt7620 pinctrl driver does.
>
> 3. Useless "-" or "rsvd" functon
>
> It's really unnecessary to add a reserved pin mux function to the
> function lists, because we never use it.
>
> Signed-off-by: Weihao Li <[email protected]>
The patch looks good to me and Sergio: patch applied so
it gets some testing in linux-next.
If Arinc has issues with it or something else occurs I can
always drop it again.
Yours,
Linus Walleij
On Tue, May 28, 2024 at 3:54 PM Linus Walleij <linus.walleij@linaroorg> wrote:
>
> On Mon, May 27, 2024 at 4:21 AM liweihao <[email protected]> wrote:
>
> > From: Weihao Li <[email protected]>
> >
> > The current version of the pinctrl driver has some issues:
> >
> > 1. Duplicated "gpio" pmx function
> >
> > The common code will add a "gpio" pmx functon to every pin group, so
> > it's not necessary to define a separate "gpio" pmx function in pin
> > groups.
> >
> > 2. Duplicated pmx function name
> >
> > There are some same function name in different pin groups, which will
> > cause some problems. For example, when we want to use PAD_GPIO0 as
> > refclk output function, the common clk framework code will search the
> > entire pin function lists, then return the first one matched, in this
> > case the matched function list only include the PAD_CO_CLKO pin group
> > because there are three "refclk" pin function, which is added by
> > refclk_grp, spi_cs1_grp and gpio_grp.
> >
> > To solve this problem, a simple way is just add a pingrp refix to
> > function name like mt7620 pinctrl driver does.
> >
> > 3. Useless "-" or "rsvd" functon
> >
> > It's really unnecessary to add a reserved pin mux function to the
> > function lists, because we never use it.
> >
> > Signed-off-by: Weihao Li <[email protected]>
>
> The patch looks good to me and Sergio: patch applied so
> it gets some testing in linux-next.
>
> If Arinc has issues with it or something else occurs I can
> always drop it again.
Thanks, Linus :)
Best regards,
Sergio Paracuellos
Sergio Paracuellos <[email protected]> 于2024年5月28日周二 22:20写道：
>
> On Tue, May 28, 2024 at 3:54 PM Linus Walleij <[email protected]> wrote:
> >
> > On Mon, May 27, 2024 at 4:21 AM liweihao <[email protected]> wrote:
> >
> > > From: Weihao Li <[email protected]>
> > >
> > > The current version of the pinctrl driver has some issues:
> > >
> > > 1. Duplicated "gpio" pmx function
> > >
> > > The common code will add a "gpio" pmx functon to every pin group, so
> > > it's not necessary to define a separate "gpio" pmx function in pin
> > > groups.
> > >
> > > 2. Duplicated pmx function name
> > >
> > > There are some same function name in different pin groups, which will
> > > cause some problems. For example, when we want to use PAD_GPIO0 as
> > > refclk output function, the common clk framework code will search the
> > > entire pin function lists, then return the first one matched, in this
> > > case the matched function list only include the PAD_CO_CLKO pin group
> > > because there are three "refclk" pin function, which is added by
> > > refclk_grp, spi_cs1_grp and gpio_grp.
> > >
> > > To solve this problem, a simple way is just add a pingrp refix to
> > > function name like mt7620 pinctrl driver does.
> > >
> > > 3. Useless "-" or "rsvd" functon
> > >
> > > It's really unnecessary to add a reserved pin mux function to the
> > > function lists, because we never use it.
> > >
> > > Signed-off-by: Weihao Li <[email protected]>
> >
> > The patch looks good to me and Sergio: patch applied so
> > it gets some testing in linux-next.
> >
> > If Arinc has issues with it or something else occurs I can
> > always drop it again.
>
> Thanks, Linus :)
>
> Best regards,
> Sergio Paracuellos
Thanks for reviewing the code.
Best regards,
Weihao Li