Received-SPF: pass (google.com: domain of linux-kernel+bounces-191839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240527022036.31985-1-user@blabla>
In-Reply-To: <20240527022036.31985-1-user@blabla>
From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Date: Tue, 28 May 2024 09:10:18 +0200
Message-ID: <CAMhs-H9CEvN8QXcQgin6NRZYzYFn5T82tLsu4rBbSMMBy=Xoiw@mail.gmail.com>
Subject: Re: [PATCH] pinctrl: ralink: mt76x8: fix pinmux function
To: liweihao <cn.liweihao@gmail.com>
Cc: arinc.unal@arinc9.com, sean.wang@kernel.org, linus.walleij@linaro.org, 
	matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com, 
	linux-mips@vger.kernel.org, linux-gpio@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Shiji Yang <yangshiji66@outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, May 27, 2024 at 4:21=E2=80=AFAM liweihao <cn.liweihao@gmail.com> wr=
ote:
>
> From: Weihao Li <cn.liweihao@gmail.com>
>
> 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 <cn.liweihao@gmail.com>
> ---
>  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[] =3D {
>         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[] =3D {
>         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[] =3D {
>         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[] =3D {
>         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[] =3D {
> -       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[] =3D { FUNC(=
"wdt", 0, 38, 1) };
>  static struct mtmips_pmx_func spi_grp[] =3D { FUNC("spi", 0, 7, 4) };
>
>  static struct mtmips_pmx_func sd_mode_grp[] =3D {
> -       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[] =3D {
> -       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[] =3D {
>         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[] =3D {
> -       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[] =3D {
>         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[] =3D {
>         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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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[] =3D {
> -       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/rali=
nk/mt7628a.dtsi#L45
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediate=
k/pinctrl-mtmips.c#L297