* [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file
@ 2021-01-23 6:49 Drew Fustini
2021-01-25 15:32 ` Andy Shevchenko
0 siblings, 1 reply; 3+ messages in thread
From: Drew Fustini @ 2021-01-23 6:49 UTC (permalink / raw)
To: linux-gpio
Cc: Drew Fustini, Pantelis Antoniou, Jason Kridner, Robert Nelson,
Linus Walleij, Tony Lindgren, Andy Shevchenko, Alexandre Belloni,
Geert Uytterhoeven
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()
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 */
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file
2021-01-23 6:49 [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file Drew Fustini
@ 2021-01-25 15:32 ` Andy Shevchenko
2021-01-25 21:30 ` Drew Fustini
0 siblings, 1 reply; 3+ messages in thread
From: Andy Shevchenko @ 2021-01-25 15:32 UTC (permalink / raw)
To: Drew Fustini
Cc: open list:GPIO SUBSYSTEM, Pantelis Antoniou, Jason Kridner,
Robert Nelson, Linus Walleij, Tony Lindgren, Alexandre Belloni,
Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file
2021-01-25 15:32 ` Andy Shevchenko
@ 2021-01-25 21:30 ` Drew Fustini
0 siblings, 0 replies; 3+ messages in thread
From: Drew Fustini @ 2021-01-25 21:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Pantelis Antoniou, Jason Kridner,
Robert Nelson, Linus Walleij, Tony Lindgren, Alexandre Belloni,
Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-26 20:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-23 6:49 [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file Drew Fustini
2021-01-25 15:32 ` Andy Shevchenko
2021-01-25 21:30 ` Drew Fustini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).