Re: [PATCH] pinctrl: ralink: mt76x8: fix pinmux function

[李维豪]

Hi. Thanks for your reply!

I forgot to reply all, so I resend this mail. Sorry about that orz.

Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> 于2024年5月28日周二 15:10写道：
>
> Hi,
>
> On Mon, May 27, 2024 at 4:21 AM liweihao <cn.liweihao@xxxxxxxxx> wrote:
> >
> > From: Weihao Li <cn.liweihao@xxxxxxxxx>
> >
> > 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 <cn.liweihao@xxxxxxxxx>
> > ---
> >  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