lkml.org 
[lkml]   [2021]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/2] pinctrl: pinmux: Add pinmux-select debugfs file
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@beagleboard.org> 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

\
 
 \ /
  Last update: 2021-02-15 22:13    [W:0.106 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site