| Message ID | 20210123064909.466225-1-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2] pinctrl: pinmux: Add pinmux-select debugfs file | expand |
On Sat, Jan 23, 2021 at 9:29 AM Drew Fustini <drew@beagleboard.org> wrote: > > This RFC is a change in approach from my previous RFC patch [1]. It adds > "pinnux-select" to debugfs. Function and group on the pin control device > will be activated when 2 integers "<function-selector> <group-selector>" > are written to the file. The debugfs write operation pinmux_select() > handles this by calling ops->set_mux() with fsel and gsel. ... > RFC notes: Please, move below to reST formatted document. ... > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf, > + size_t cnt, loff_t *ppos) > +{ > + struct seq_file *sfile = file->private_data; > + struct pinctrl_dev *pctldev = sfile->private; > + const struct pinmux_ops *ops = pctldev->desc->pmxops; > + int fsel, gsel, ret; > + // RFC note: two integers separated by a space should never exceed 16 > + char buf[16]; > + if (*ppos != 0) > + return -EINVAL; But why? Do we really care about it? Moreover, you have no_llseek() below. > + ret = strncpy_from_user(buf, user_buf, cnt); Potential buffer overflow. cnt -> sizeof(buf) > + if (ret < 0) > + return ret; > + buf[cnt] = '\0'; Not sure, shouldn't be buf[sizeof(buf) - 1] = '\0'; ? > + if (buf[cnt - 1] == '\n') > + buf[cnt - 1] = '\0'; strstrip() ? > + ret = sscanf(buf, "%d %d", &fsel, &gsel); > + if (ret != 2) { > + dev_err(pctldev->dev, "%s: sscanf() expects '<fsel> <gsel>'", __func__); __func__ is useless, please drop it. And below as well. > + return -EINVAL; > + } > + ret = ops->set_mux(pctldev, fsel, gsel); > + if (ret != 0) { I thought I gave you a comment on this... if (ret) > + dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret); > + return -EINVAL; > + } > + > + return cnt; > +} ... > debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO, > devroot, pctldev, &pinmux_pins_fops); > + debugfs_create_file("pinmux-select", 0200, > + devroot, pctldev, &pinmux_set_ops); Consider to add another (prerequisite) patch to get rid of symbolic permissions. -- With Best Regards, Andy Shevchenko
On Mon, Jan 25, 2021 at 05:32:39PM +0200, Andy Shevchenko wrote: > On Sat, Jan 23, 2021 at 9:29 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > This RFC is a change in approach from my previous RFC patch [1]. It adds > > "pinnux-select" to debugfs. Function and group on the pin control device > > will be activated when 2 integers "<function-selector> <group-selector>" > > are written to the file. The debugfs write operation pinmux_select() > > handles this by calling ops->set_mux() with fsel and gsel. > > ... > > > RFC notes: > > Please, move below to reST formatted document. Ok, I'll make it a series and include Documentation/driver-api/pinctl.rst > > ... > > > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf, > > + size_t cnt, loff_t *ppos) > > +{ > > + struct seq_file *sfile = file->private_data; > > + struct pinctrl_dev *pctldev = sfile->private; > > + const struct pinmux_ops *ops = pctldev->desc->pmxops; > > + int fsel, gsel, ret; > > + // RFC note: two integers separated by a space should never exceed 16 > > + char buf[16]; > > > + if (*ppos != 0) > > + return -EINVAL; > > But why? Do we really care about it? Moreover, you have no_llseek() below. Good point, I'll get rid of it. > > > + ret = strncpy_from_user(buf, user_buf, cnt); > > Potential buffer overflow. > > cnt -> sizeof(buf) > Thanks, that is a good point. > > + if (ret < 0) > > + return ret; > > > + buf[cnt] = '\0'; > > Not sure, shouldn't be > > buf[sizeof(buf) - 1] = '\0'; > > ? I'll take a look at it. > > > + if (buf[cnt - 1] == '\n') > > + buf[cnt - 1] = '\0'; > > strstrip() ? > Neat, I wasn't aware of that one. > > + ret = sscanf(buf, "%d %d", &fsel, &gsel); > > + if (ret != 2) { > > > + dev_err(pctldev->dev, "%s: sscanf() expects '<fsel> <gsel>'", __func__); > > __func__ is useless, please drop it. And below as well. Sorry, I should have removed that. > > > + return -EINVAL; > > + } > > > + ret = ops->set_mux(pctldev, fsel, gsel); > > + if (ret != 0) { > > I thought I gave you a comment on this... > > if (ret) Yes, sorry, I should have changed that. > > > + dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret); > > + return -EINVAL; > > + } > > + > > + return cnt; > > +} > > ... > > > debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO, > > devroot, pctldev, &pinmux_pins_fops); > > + debugfs_create_file("pinmux-select", 0200, > > + devroot, pctldev, &pinmux_set_ops); > > Consider to add another (prerequisite) patch to get rid of symbolic permissions. Ok, I'll do that. > > -- > With Best Regards, > Andy Shevchenko Thanks for the comments, Drew
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index bab888fe3f8e..326b3fc41b55 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -673,6 +673,55 @@ 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 cnt, loff_t *ppos) +{ + struct seq_file *sfile = file->private_data; + struct pinctrl_dev *pctldev = sfile->private; + const struct pinmux_ops *ops = pctldev->desc->pmxops; + int fsel, gsel, ret; + // RFC note: two integers separated by a space should never exceed 16 + char buf[16]; + + if (*ppos != 0) + return -EINVAL; + + ret = strncpy_from_user(buf, user_buf, cnt); + if (ret < 0) + return ret; + buf[cnt] = '\0'; + if (buf[cnt - 1] == '\n') + buf[cnt - 1] = '\0'; + + ret = sscanf(buf, "%d %d", &fsel, &gsel); + if (ret != 2) { + dev_err(pctldev->dev, "%s: sscanf() expects '<fsel> <gsel>'", __func__); + return -EINVAL; + } + + ret = ops->set_mux(pctldev, fsel, gsel); + if (ret != 0) { + dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret); + return -EINVAL; + } + + return cnt; +} + +static int pinmux_set_open(struct inode *inode, struct file *file) +{ + return single_open(file, NULL, inode->i_private); +} + +static const struct file_operations pinmux_set_ops = { + .owner = THIS_MODULE, + .open = pinmux_set_open, + .read = seq_read, + .write = pinmux_select, + .llseek = no_llseek, + .release = single_release, +}; + void pinmux_init_device_debugfs(struct dentry *devroot, struct pinctrl_dev *pctldev) { @@ -680,6 +729,8 @@ void pinmux_init_device_debugfs(struct dentry *devroot, devroot, pctldev, &pinmux_functions_fops); debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO, devroot, pctldev, &pinmux_pins_fops); + debugfs_create_file("pinmux-select", 0200, + devroot, pctldev, &pinmux_set_ops); } #endif /* CONFIG_DEBUG_FS */
This RFC is a change in approach from my previous RFC patch [1]. It adds "pinnux-select" to debugfs. Function and group on the pin control device will be activated when 2 integers "<function-selector> <group-selector>" are written to the file. The debugfs write operation pinmux_select() handles this by calling ops->set_mux() with fsel and gsel. RFC notes: I'm not sure if this is worth including in the commit message but the following is an example on the PocketBeagle [2] which has the TI AM3358 SoC and binds to pinctrl-single. I added this to the device tree [3] to represent two of the pins on the expansion header as an example: P1.36 and P2.01. Both of these header pins are designed to be set to PWM mode by default [4] but can be set back to gpio mode through pinmux-select. &am33xx_pinmux { /* use the pin controller itself as the owner device */ pinctrl-names = "default", "P1_36_gpio", "P1_36_gpio_pu", "P1_36_gpio_pd", "P1_36_gpio_input", "P1_36_pwm", "P2_01_gpio", "P2_01_gpio_pu", "P2_01_gpio_pd", "P2_01_gpio_input", "P2_01_pwm"; /* set hog for default mode */ pinctrl-0 = < &P1_36_default_pin &P2_01_default_pin >; /* these will create pin functions for each mode */ pinctrl-1 = <&P1_36_gpio_pin>; pinctrl-2 = <&P1_36_gpio_pu_pin>; pinctrl-3 = <&P1_36_gpio_pd_pin>; pinctrl-4 = <&P1_36_gpio_input_pin>; pinctrl-5 = <&P1_36_pwm_pin>; pinctrl-6 = <&P2_01_default_pin>; pinctrl-7 = <&P2_01_gpio_pin>; pinctrl-8 = <&P2_01_gpio_pu_pin>; pinctrl-9 = <&P2_01_gpio_pd_pin>; pinctrl-10 = <&P2_01_gpio_input_pin>; pinctrl-11 = <&P2_01_pwm_pin>; /* P1_36 (ZCZ ball A13) ehrpwm0a */ P1_36_default_pin: pinmux_P1_36_default_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE1) >; }; P1_36_gpio_pin: pinmux_P1_36_gpio_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT | INPUT_EN | MUX_MODE7) >; }; P1_36_gpio_pu_pin: pinmux_P1_36_gpio_pu_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLUP | INPUT_EN | MUX_MODE7) >; }; P1_36_gpio_pd_pin: pinmux_P1_36_gpio_pd_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE7) >; }; P1_36_gpio_input_pin: pinmux_P1_36_gpio_input_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_INPUT | MUX_MODE7) >; }; P1_36_pwm_pin: pinmux_P1_36_pwm_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE1) >; }; P1_36_spi_sclk_pin: pinmux_P1_36_spi_sclk_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLUP | INPUT_EN | MUX_MODE3) >; }; P1_36_pruout_pin: pinmux_P1_36_pruout_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE5) >; }; P1_36_pruin_pin: pinmux_P1_36_pruin_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0990, PIN_INPUT | MUX_MODE6) >; }; /* P2_01 (ZCZ ball U14) ehrpwm1a */ P2_01_default_pin: pinmux_P2_01_default_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE6) >; }; P2_01_gpio_pin: pinmux_P2_01_gpio_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT | INPUT_EN | MUX_MODE7) >; }; P2_01_gpio_pu_pin: pinmux_P2_01_gpio_pu_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLUP | INPUT_EN | MUX_MODE7) >; }; P2_01_gpio_pd_pin: pinmux_P2_01_gpio_pd_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE7) >; }; P2_01_gpio_input_pin: pinmux_P2_01_gpio_input_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_INPUT | MUX_MODE7) >; }; P2_01_pwm_pin: pinmux_P2_01_pwm_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE6) >; }; The following shows the pin functions registered for the pin controller: function: pinmux_P1_36_default_pin, groups = [ pinmux_P1_36_default_pin ] function: pinmux_P2_01_default_pin, groups = [ pinmux_P2_01_default_pin ] function: pinmux_P1_36_gpio_pin, groups = [ pinmux_P1_36_gpio_pin ] function: pinmux_P1_36_gpio_pu_pin, groups = [ pinmux_P1_36_gpio_pu_pin ] function: pinmux_P1_36_gpio_pd_pin, groups = [ pinmux_P1_36_gpio_pd_pin ] function: pinmux_P1_36_gpio_input_pin, groups = [ pinmux_P1_36_gpio_input_pin ] function: pinmux_P1_36_pwm_pin, groups = [ pinmux_P1_36_pwm_pin ] function: pinmux_P2_01_gpio_pin, groups = [ pinmux_P2_01_gpio_pin ] function: pinmux_P2_01_gpio_pu_pin, groups = [ pinmux_P2_01_gpio_pu_pin ] function: pinmux_P2_01_gpio_pd_pin, groups = [ pinmux_P2_01_gpio_pd_pin ] function: pinmux_P2_01_gpio_input_pin, groups = [ pinmux_P2_01_gpio_input_pin ] function: pinmux_P2_01_pwm_pin, groups = [ pinmux_P2_01_pwm_pin ] function: pinmux-uart0-pins, groups = [ pinmux-uart0-pins ] function: pinmux-mmc0-pins, groups = [ pinmux-mmc0-pins ] function: pinmux-i2c0-pins, groups = [ pinmux-i2c0-pins ] Activate the pinmux_P1_36_gpio_pin function (fsel 2): Extra debug output that I added shows that pinctrl-single's set_mux() has set the register correctly for gpio mode: pinmux core: pinmux_select(): returned 0 pinmux core: pinmux_select(): buf=[2 2] pinmux core: pinmux_select(): sscanf(2,2) pinmux core: pinmux_select(): call ops->set_mux(fsel=2, gsel=2) pinctrl-single 44e10800.pinmux: enabling (null) function2 pinctrl-single 44e10800.pinmux: pcs_set_mux(): offset=0x190 old_val=0x21 val=0x2f Activate the pinmux_P1_36_pwm_pin function (fsel 6): pinctrl-single set_mux() is able to set register correctly for pwm mode: pinmux core: pinmux_select(): returned 0 pinmux core: pinmux_select(): buf=[6 6] pinmux core: pinmux_select(): sscanf(6,6) pinmux core: pinmux_select(): call ops->set_mux(fsel=6, gsel=6) pinctrl-single 44e10800.pinmux: enabling (null) function6 pinctrl-single 44e10800.pinmux: pcs_set_mux(): offset=0x190 old_val=0x2f val=0x21 I would appreciate any feedback on this approach. Thank you! -Drew [1] https://lore.kernel.org/linux-gpio/20201218045134.4158709-1-drew@beagleboard.org/ [2] https://beagleboard.org/pocket [3] arch/arm/boot/dts/am335x-pocketbeagle.dts [4] https://github.com/beagleboard/pocketbeagle/wiki/System-Reference-Manual#70-connectors- Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Cc: Jason Kridner <jkridner@beagleboard.org>, Cc: Robert Nelson <robertcnelson@beagleboard.org>, Cc: Linus Walleij <linus.walleij@linaro.org>, Cc: Tony Lindgren <tony@atomide.com>, Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Drew Fustini <drew@beagleboard.org> --- drivers/pinctrl/pinmux.c | 51 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) v2 changes: - rename debugfs file "pinmux-set" to "pinmux-select" - renmae pinmux_set_write() to pinmux_select() - switch from memdup_user_nul() to strncpy_from_user() - switch from pr_warn() to dev_err()