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

[Drew Fustini]

On Mon, Feb 15, 2021 at 09:04:20PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 13, 2021 at 12:30 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> >
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when "<function-name group-name>" are written to the file. The write
> 
> The non-standard way of showing parameters, I would write that as
>  "<function-name> <group-name>".

Sorry for your comments, but I don't understand what you mean by this
one.  I think we wrote ""<function-name> <group-name>" the same way, no?

> 
> > operation pinmux_select() handles this by checking that the names map to
> > 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 ]
> 
> Format this...
> 
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> >
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> 
> ...and this with two leading spaces (for example) to make sure that
> people will understand that these lines are part of the example.

Ok, thanks.

> 
> ...
> 
> >  drivers/pinctrl/pinmux.c | 99 ++++++++++++++++++++++++++++++++++++++++
> 
> Still needs a documentation update.

There is no documentation for any of the existing pinctrl debugfs files.
I was planning to do this as part of a seperate patch, but I can make it
part of this series instead.

> 
> ...
> 
> > +       const char *usage =
> > +               "usage: echo '<function-name> <group-name>' > pinmux-select";
> 
> This is quite unusual to have in the kernel. Just return an error
> code, everything else should be simply documented.
> 
> ...
> 
> > +       if (len > PINMUX_SELECT_MAX) {
> 
> > +               dev_err(pctldev->dev, "write too big for buffer");
> 
> Noisy, the user will get an error code and interpret it properly.
> So, please drop them all. Otherwise it would be quite easy to exhaust
> kernel buffer with this noise and lost the important messages.
> 
> > +               return -EINVAL;
> 
> To achieve the above, this rather should be -ENOMEM.
> 
> > +       }

Thanks, I will remove the usage message and change the return value.

> 
> ...
> 
> > +       gname = strchr(fname, ' ');
> > +       if (!gname) {
> > +               dev_err(pctldev->dev, usage);
> > +               ret = -EINVAL;
> > +               goto free_buf;
> > +       }
> > +       *gname++ = '\0';
> 
> I was thinking about this again and I guess we may allow any amount of
> spaces in between and any kind of  (like newline or TAB).
> So, taking above into consideration the code may look like this:
> 
> /* Take the input and remove leading and trailing spaces of entire buffer */
> fname = strstrip(buf);
> /* Find a separator, i.e. a space character */
> for (gname = fname; !isspace(gname); gname++)
>   if (*gname == '\0')
>     return -EINVAL;
> /* Replace separator with %NUL to terminate first word */
> *gname = '\0';
> /* Drop space characters between first and second words */
> gname = skip_spaces(gname + 1);
> if (*gname == '\0')
>   return -EINVAL;
> 
> But please double check the logic.
> 
> ...


Thanks for the example code.  I'll test it out.


> 
> > +free_buf:
> 
> exit_free_buf:
> 

Ok, thanks.

> > +       kfree(buf);
> > +
> > +       return ret;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko