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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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>".

> 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.

...

>  drivers/pinctrl/pinmux.c | 99 ++++++++++++++++++++++++++++++++++++++++

Still needs a documentation update.

...

> +       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.

> +       }

...

> +       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.

...

> +free_buf:

exit_free_buf:

> +       kfree(buf);
> +
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko