| Message ID | 20240527022036.31985-1-user@blabla (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | pinctrl: ralink: mt76x8: fix pinmux function | expand |
Hi, On Mon, May 27, 2024 at 4:21 AM liweihao <cn.liweihao@gmail.com> wrote: > > 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[] = { > 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 <sergio.paracuellos@gmail.com> 于2024年5月28日周二 15:10写道： > > Hi, > > On Mon, May 27, 2024 at 4:21 AM liweihao <cn.liweihao@gmail.com> wrote: > > > > 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]. > 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@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[] = { > > 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 <Markus.Elfring@web.de> 于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 <cn.liweihao@gmail.com> wrote: > From: Weihao Li <cn.liweihao@gmail.com> > > 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 <cn.liweihao@gmail.com> 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@linaro.org> wrote: > > On Mon, May 27, 2024 at 4:21 AM liweihao <cn.liweihao@gmail.com> wrote: > > > From: Weihao Li <cn.liweihao@gmail.com> > > > > 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 <cn.liweihao@gmail.com> > > 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 <sergio.paracuellos@gmail.com> 于2024年5月28日周二 22:20写道： > > On Tue, May 28, 2024 at 3:54 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Mon, May 27, 2024 at 4:21 AM liweihao <cn.liweihao@gmail.com> wrote: > > > > > From: Weihao Li <cn.liweihao@gmail.com> > > > > > > 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 <cn.liweihao@gmail.com> > > > > 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
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), };