Re: [PATCH] pinctrl: pinmux: Add pinmux-select debugfs file

Re: [PATCH] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Fri, Jan 29, 2021 at 12:15:34PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 29, 2021 at 10:49 AM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when 2 integers "<function-selector> <group-selector>" are written to
> > the file. The write operation pinmux_select() handles this by checking
> > if fsel and gsel are valid selectors and then calling ops->set_mux().
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> >
> > To activate function pinmux-i2c1 (fsel 4) and group pinmux-i2c1-pins
> > (gsel 4):
> >
> > echo '4 4' > pinmux-select
> 
> Hmm... Where is the documentation? Where is the changelog? What is the
> version of this patch? What happened to my suggestion to have another
> (preparatory) patch to move to octal permissions? I'm completely
> confused.
> 
> Please, resend according to the rules.

Sorry for causing the confusion. I dropped the change long when moving
from RFC v2 to this PATCH.  But I can see how it is better to keep so
that people just looking at this for the first time have better context
for what happened in the RFC discussion.

I did post a patch this past Monday so switch all the debugfs files in
pinctrl subsystem over to octal permission [1]. But now I think that
this should have been series with this patch. Linus has not applied it
yet so I'll make a series when I send v2 of this patch.

Regarding the documentation, there is no existing documentation for any
of the debugfs enteries for pinctrl, so it seemed to have a bigger scope
than just this patch. I also noticed that rst documentation is
confusingly named "pinctl" (no 'r') and started a thread about that [2].
Linus suggested chaning that to 'pin-control'. Thus I am thinking about
a documentation patch series where the file is renamed, references
changed and a section on the debugfs files is added. Do that sound like
a reasonable approach?

Thank you,
Drew

[1] https://lore.kernel.org/linux-gpio/20210126044742.87602-1-drew@beagleboard.org/
[2] https://lore.kernel.org/linux-gpio/20210126050817.GA187797@x1/

Re: [PATCH] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Fri, Jan 29, 2021 at 12:02:43PM +0100, Geert Uytterhoeven wrote:
> Hi Drew,
> 
> Thanks for your patch!
> 
> On Fri, Jan 29, 2021 at 9:49 AM Drew Fustini <drew@beagleboard.org> wrote:
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when 2 integers "<function-selector> <group-selector>" are written to
> > the file. The write operation pinmux_select() handles this by checking
> > if fsel and gsel are valid selectors and then calling ops->set_mux().
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> >
> > To activate function pinmux-i2c1 (fsel 4) and group pinmux-i2c1-pins
> > (gsel 4):
> >
> > echo '4 4' > pinmux-select
> 
> Shouldn't gsel be "0" in your example, as "pinmux-i2c1-pins" is the
> first entry in the groups array?
> 
> [ /me actually tries this]
> 
> Oh, gsel is not the index inside the groups array, but the global index?
> 
> On R-Car M2-W, which does not use pinctrl-single, I have:
> 
>     function 0: audio_clk, groups = [ audio_clk_a audio_clk_b
> audio_clk_b_b audio_clk_c audio_clkout ]
>     function 1: avb, groups = [ avb_link avb_magic avb_phy_int
> avb_mdio avb_mii avb_gmii ]
>     function 2: can0, groups = [ can0_data can0_data_b can0_data_c
> can0_data_d can0_data_e can0_data_f can_clk can_clk_b can_clk_c
> can_clk_d ]
>     function 3: can1, groups = [ can1_data can1_data_b can1_data_c
> can1_data_d can_clk can_clk_b can_clk_c can_clk_d ]
>     function 4: can_clk, groups = [ can_clk can_clk_b can_clk_c can_clk_d ]
>     function 5: du, groups = [ du_rgb666 du_rgb888 du_clk_out_0
> du_clk_out_1 du_sync du_oddf du_cde du_disp ]
>     function 6: du0, groups = [ du0_clk_in ]
>     function 7: du1, groups = [ du1_clk_in du1_clk_in_b du1_clk_in_c ]
>     function 8: eth, groups = [ eth_link eth_magic eth_mdio eth_rmii ]
>     function 9: hscif0, groups = [ hscif0_data hscif0_clk hscif0_ctrl
> hscif0_data_b hscif0_ctrl_b hscif0_data_c hscif0_clk_c ]
>     function 10: hscif1, groups = [ hscif1_data hscif1_clk hscif1_ctrl
> hscif1_data_b hscif1_data_c hscif1_clk_c hscif1_ctrl_c hscif1_data_d
> hscif1_data_e hscif1_clk_e hscif1_ctrl_e ]
>     function 11: hscif2, groups = [ hscif2_data hscif2_clk hscif2_ctrl
> hscif2_data_b hscif2_ctrl_b hscif2_data_c hscif2_clk_c hscif2_data_d ]
>     function 12: i2c0, groups = [ i2c0 i2c0_b i2c0_c ]
>     function 13: i2c1, groups = [ i2c1 i2c1_b i2c1_c i2c1_d i2c1_e ]
>     function 14: i2c2, groups = [ i2c2 i2c2_b i2c2_c i2c2_d ]
>     [...]
> 
> Took me a few tries, crashes, and debug prints, to discover that I did
> not count wrong, but that names were de-duplicated (the "can_clk*" in
> functions 2 and 3 are not accessible by gsel), and that "75" not "83" is
> the right number to configure i2c2.
> 
> But then it works (on the Koelsch board):
> 
>     # cd /sys/kernel/debug/pinctrl/e6060000.pinctrl-sh-pfc/
>     # echo 14 289 > pinmux-select   # Configure i2c2 pins for ssi2_ctrl
>     # i2cdetect -y -a 2             # Fails
>     # echo 14 75 > pinmux-select    # Restore i2c2
>     # i2cdetect -y -a 2             # Works again
> 
> Nice!
> 
> So we definitely want to use the actual names, not error-prone numbers,
> at least for pinctrl drivers that use the concept of names.
> As we do specify the names in DT, cfr.
> arch/arm/boot/dts/r8a7791-koelsch.dts:
> 
>         i2c2_pins: i2c2 {
>                 groups = "i2c2";
>                 function = "i2c2";
>         };
> 
> the pinmux core already knows how to look them up.
> See pinmux_map_to_setting().
> 
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -673,6 +673,65 @@ void pinmux_show_setting(struct seq_file *s,
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
> >
> > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> > +                                  size_t len, loff_t *ppos)
> > +{
> > +       struct seq_file *sfile = file->private_data;
> > +       struct pinctrl_dev *pctldev = sfile->private;
> > +       const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
> > +       const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> > +       int fsel, gsel, ret;
> > +       char buf[16];
> > +
> > +       if (len > sizeof(buf)) {
> > +               dev_err(pctldev->dev, "write too big for buffer");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = strncpy_from_user(buf, user_buf, sizeof(buf));
> > +       if (ret < 0)
> > +               return ret;
> > +       buf[len-1] = '\0';
> > +
> > +       ret = sscanf(buf, "%d %d", &fsel, &gsel);
> > +       if (ret != 2) {
> > +               dev_err(pctldev->dev, "expected format: <function#> <group#>");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!pmxops->get_function_name(pctldev, fsel)) {
> 
> At least for the Renesas pinctrl driver, get_function_name() happily
> iterates beyond the end of the internal array, which may crash.
> Using pinmux_func_name_to_selector() would avoid that.
> 
> > +               dev_err(pctldev->dev, "function selector %u not valid\n", fsel);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!pctlops->get_group_name(pctldev, gsel)) {
> 
> Likewise.
> 
> Use pmxops->get_function_groups() and match_string() instead?
> 
> > +               dev_err(pctldev->dev, "group selector %u not valid\n", gsel);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = pmxops->set_mux(pctldev, fsel, gsel);
> > +       if (ret) {
> > +               dev_err(pctldev->dev, "set_mux() failed: %d", ret);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return len;
> > +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Thanks very much for the comments, Geert. It is great to get feedback on
how it behaves on other pin control hardware.

I agree the selectors get confusing, and it does make more sense to use
the function and group names in 'pinmux-select'. I was initially
discouraged when I didn't see an efficient way to do a lookup in the
radix trees based on name. I'll take a look at the functions that you
mentioned as they look promising.

thanks,
drew