| Message ID | 20210212223015.727608-3-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: pinmux: Add pinmux-select debugfs file | expand |
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>". > 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; > +}
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
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index c651b2db0925..c16a6167ac3a 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -673,6 +673,103 @@ void pinmux_show_setting(struct seq_file *s, DEFINE_SHOW_ATTRIBUTE(pinmux_functions); DEFINE_SHOW_ATTRIBUTE(pinmux_pins); +#define PINMUX_SELECT_MAX 128 +static ssize_t pinmux_select(struct file *file, const char __user *user_buf, + size_t len, loff_t *ppos) +{ + const char *usage = + "usage: echo '<function-name> <group-name>' > pinmux-select"; + struct seq_file *sfile = file->private_data; + struct pinctrl_dev *pctldev = sfile->private; + const struct pinmux_ops *pmxops = pctldev->desc->pmxops; + const char *const *groups; + char *buf, *fname, *gname; + unsigned int num_groups; + int fsel, gsel, ret; + + if (len > PINMUX_SELECT_MAX) { + dev_err(pctldev->dev, "write too big for buffer"); + return -EINVAL; + } + + buf = kzalloc(PINMUX_SELECT_MAX, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = strncpy_from_user(buf, user_buf, PINMUX_SELECT_MAX); + if (ret < 0) { + dev_err(pctldev->dev, "failed to copy buffer from userspace"); + goto free_buf; + } + buf[len-1] = '\0'; + + fname = strstrip(buf); + if (*fname == '\0') { + dev_err(pctldev->dev, usage); + ret = -EINVAL; + goto free_buf; + } + + gname = strchr(fname, ' '); + if (!gname) { + dev_err(pctldev->dev, usage); + ret = -EINVAL; + goto free_buf; + } + *gname++ = '\0'; + + fsel = pinmux_func_name_to_selector(pctldev, fname); + if (fsel < 0) { + dev_err(pctldev->dev, "invalid function %s in map table\n", fname); + ret = fsel; + goto free_buf; + } + + ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups); + if (ret) { + dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname); + goto free_buf; + } + + ret = match_string(groups, num_groups, gname); + if (ret < 0) { + dev_err(pctldev->dev, "invalid group %s", gname); + goto free_buf; + } + + gsel = pinctrl_get_group_selector(pctldev, gname); + if (gsel < 0) { + dev_err(pctldev->dev, "failed to get group selector for %s", gname); + ret = gsel; + goto free_buf; + } + + ret = pmxops->set_mux(pctldev, fsel, gsel); + if (ret) { + dev_err(pctldev->dev, "set_mux() failed: %d", ret); + goto free_buf; + } + ret = len; + +free_buf: + kfree(buf); + + return ret; +} + +static int pinmux_select_open(struct inode *inode, struct file *file) +{ + return single_open(file, NULL, inode->i_private); +} + +static const struct file_operations pinmux_select_ops = { + .owner = THIS_MODULE, + .open = pinmux_select_open, + .write = pinmux_select, + .llseek = no_llseek, + .release = single_release, +}; + void pinmux_init_device_debugfs(struct dentry *devroot, struct pinctrl_dev *pctldev) { @@ -680,6 +777,8 @@ void pinmux_init_device_debugfs(struct dentry *devroot, devroot, pctldev, &pinmux_functions_fops); debugfs_create_file("pinmux-pins", 0444, devroot, pctldev, &pinmux_pins_fops); + debugfs_create_file("pinmux-select", 0200, + devroot, pctldev, &pinmux_select_ops); } #endif /* CONFIG_DEBUG_FS */
Add "pinmux-select" to debugfs which will activate a function and group when "<function-name group-name>" are written to the file. The write 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 ] To activate function pinmux-i2c1 and group pinmux-i2c1-pins: echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select Signed-off-by: Drew Fustini <drew@beagleboard.org> --- drivers/pinctrl/pinmux.c | 99 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)