| Message ID | 20210121051806.623743-1-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] pinctrl: pinmux: Add pinmux-set debugfs file | expand |
On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@beagleboard.org> wrote: > > This RFC is a change in approach from my previous RFC patch [1]. It adds > "pinnux-set" to debugfs. A 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_set_write() > handles this by calling ops->set_mux() with fsel and gsel. s/ops// > RFC question: should pinmux-set take function name and group name > instead of the selector numbers? I would prefer names and integers (but from user p.o.v. names are easier to understand, while numbers are good for scripting). The following is better to include in documentation and remove from the commit message. > The following is an example on the PocketBeagle [2] which has the 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 now be set back to gpio mode through pinmux-set. ... > The following shows the pin functions registered for the pin controller: > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pinmux-functions Shorter is better, what about simply # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions ? Btw in reST format you may create a nice citation of this. And yes, this should also go to the documentation. > 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): > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '2 2' > pinmux-set > > Extra debug output that I added shows that pinctrl-single's set_mux() > has set the register correctly for gpio mode: > > pinmux core: DEBUG pinmux_set_write(): returned 0 > pinmux core: DEBUG pinmux_set_write(): buf=[2 2] > pinmux core: DEBUG pinmux_set_write(): sscanf(2,2) > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=2, gsel=2) > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=2 > pinctrl-single 44e10800.pinmux: enabling (null) function2 > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x21 val=0x2f > > Activate the pinmux_P1_36_pwm_pin function (fsel 6): > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '6 6' > pinmux-set > > pinctrl-single set_mux() is able to set register correctly for pwm mode: > > pinmux core: DEBUG pinmux_set_write(): returned 0 > pinmux core: DEBUG pinmux_set_write(): buf=[6 6] > pinmux core: DEBUG pinmux_set_write(): sscanf(6,6) > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=6, gsel=6) > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=6 > pinctrl-single 44e10800.pinmux: enabling (null) function6 > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x2f val=0x21 This and above is still part of documentation, and not a commit message thingy. ... > +static ssize_t pinmux_set_write(struct file *file, const char __user *user_buf, > + size_t cnt, loff_t *ppos) > +{ > + int err; > + int fsel; > + int gsel; > + int ret; > + char *buf; > + struct seq_file *sfile; > + struct pinctrl_dev *pctldev; > + const struct pinmux_ops *ops; Reversed xmas tree order please, and you may group some of them, like int fsel, gsel; > + if (*ppos != 0) > + return -EINVAL; > + if (cnt == 0) > + return 0; Has it ever happened here? > + buf = memdup_user_nul(user_buf, cnt); > + if (IS_ERR(buf)) > + return PTR_ERR(buf); > + > + if (buf[cnt - 1] == '\n') > + buf[cnt - 1] = '\0'; Shouldn't you rather use strndup_from_user() (or how is it called?) > + ret = sscanf(buf, "%d %d", &fsel, &gsel); > + if (ret != 2) { > + pr_warn("%s: sscanf() expects '<fsel> <gsel>'", __func__); No __func__ and instead use dev_err() (it is strange you are using warn level for errors). > + err = -EINVAL; > + goto err_freebuf; > + } > + sfile = file->private_data; > + pctldev = sfile->private; These can be applied directly in the definition block above. > + ops = pctldev->desc->pmxops; > + ret = ops->set_mux(pctldev, fsel, gsel); > + if (ret != 0) { if (ret) > + pr_warn("%s(): set_mux() failed: %d", __func__, ret); As above. > + err = -EINVAL; > + goto err_freebuf; > + } > + kfree(buf); > + return cnt; > + > +err_freebuf: > + kfree(buf); > + return err; Can be simply err_freebuf: kfree(buf); return err ?: cnt; > +} > + ... > + debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR, > + devroot, pctldev, &pinmux_set_ops); I would rather call it 'pinmux-select'. Overall since it's a debugfs I do not much care about interfaces and particular implementation details, but in general looks good to me, thanks for doing this!
On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote: > On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > This RFC is a change in approach from my previous RFC patch [1]. It adds > > "pinnux-set" to debugfs. A 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_set_write() > > handles this by calling ops->set_mux() with fsel and gsel. > > s/ops// Ok, thanks. > > > RFC question: should pinmux-set take function name and group name > > instead of the selector numbers? > > I would prefer names and integers (but from user p.o.v. names are > easier to understand, while numbers are good for scripting). I don't actually see any example of looking up the function name in the existing pinctrl code. There is pin_function_tree in struct pinctrl_dev. pinmux_generic_get_function_name() does radix_tree_lookup() with the selector integer as the key, but there is no corresponding "get function selector by name" function. I think I would need to go through all the nodes in the radix tree to find the name that matches. Although, I am just learning now about the radix implementation in Linux so there might be a simpler way that I am missing. > > The following is better to include in documentation and remove from > the commit message. > > > The following is an example on the PocketBeagle [2] which has the 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 now be set back to gpio mode through pinmux-set. > > ... > > > The following shows the pin functions registered for the pin controller: > > > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pinmux-functions > > Shorter is better, what about simply > > # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions > ? > > Btw in reST format you may create a nice citation of this. And yes, > this should also go to the documentation. Good point, I'll shorten the example lines in v2. > > 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): > > > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '2 2' > pinmux-set > > > > Extra debug output that I added shows that pinctrl-single's set_mux() > > has set the register correctly for gpio mode: > > > > pinmux core: DEBUG pinmux_set_write(): returned 0 > > pinmux core: DEBUG pinmux_set_write(): buf=[2 2] > > pinmux core: DEBUG pinmux_set_write(): sscanf(2,2) > > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=2, gsel=2) > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=2 > > pinctrl-single 44e10800.pinmux: enabling (null) function2 > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x21 val=0x2f > > > > Activate the pinmux_P1_36_pwm_pin function (fsel 6): > > > > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '6 6' > pinmux-set > > > > pinctrl-single set_mux() is able to set register correctly for pwm mode: > > > > pinmux core: DEBUG pinmux_set_write(): returned 0 > > pinmux core: DEBUG pinmux_set_write(): buf=[6 6] > > pinmux core: DEBUG pinmux_set_write(): sscanf(6,6) > > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=6, gsel=6) > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=6 > > pinctrl-single 44e10800.pinmux: enabling (null) function6 > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 > > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x2f val=0x21 > > This and above is still part of documentation, and not a commit message thingy. Is something I should add to Documentation/driver-api/pinctl.rst in a seperate patch? > > ... > > > +static ssize_t pinmux_set_write(struct file *file, const char __user *user_buf, > > + size_t cnt, loff_t *ppos) > > +{ > > + int err; > > + int fsel; > > + int gsel; > > + int ret; > > + char *buf; > > + struct seq_file *sfile; > > + struct pinctrl_dev *pctldev; > > + const struct pinmux_ops *ops; > > Reversed xmas tree order please, and you may group some of them, like > > int fsel, gsel; > Ok, understood. > > + if (*ppos != 0) > > + return -EINVAL; > > > + if (cnt == 0) > > + return 0; > > Has it ever happened here? Good point, I guess there is no reason for userspace to write 0 bytes. > > + buf = memdup_user_nul(user_buf, cnt); > > + if (IS_ERR(buf)) > > + return PTR_ERR(buf); > > + > > + if (buf[cnt - 1] == '\n') > > + buf[cnt - 1] = '\0'; > > Shouldn't you rather use strndup_from_user() (or how is it called?) > > > + ret = sscanf(buf, "%d %d", &fsel, &gsel); > > + if (ret != 2) { > > + pr_warn("%s: sscanf() expects '<fsel> <gsel>'", __func__); > > No __func__ and instead use dev_err() (it is strange you are using > warn level for errors). > Ok, that makes sense. I used warn because I wasn't sure if bad format in a write to a debugfs file rises to the level of error. > > + err = -EINVAL; > > + goto err_freebuf; > > + } > > > + sfile = file->private_data; > > + pctldev = sfile->private; > > These can be applied directly in the definition block above. I'll clean that up. > > > + ops = pctldev->desc->pmxops; > > + ret = ops->set_mux(pctldev, fsel, gsel); > > > + if (ret != 0) { > > if (ret) > > > + pr_warn("%s(): set_mux() failed: %d", __func__, ret); > > As above. > > > + err = -EINVAL; > > + goto err_freebuf; > > + } > > > + kfree(buf); > > + return cnt; > > + > > +err_freebuf: > > + kfree(buf); > > + return err; > > Can be simply > > err_freebuf: > kfree(buf); > return err ?: cnt; Thanks, I didn't really like the duplication but was having trouble thinking of a cleaner way to write it. That is good to know it is ok to use the ternary operator in a return statement. > > > +} > > + > > ... > > > + debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR, > > + devroot, pctldev, &pinmux_set_ops); > > I would rather call it 'pinmux-select'. I think that makes sense, too. > > Overall since it's a debugfs I do not much care about interfaces and > particular implementation details, but in general looks good to me, > thanks for doing this! Thanks for the review. I'll get a v2 posted. -Drew
On Fri, Jan 22, 2021 at 1:26 AM Drew Fustini <drew@beagleboard.org> wrote: > On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@beagleboard.org> wrote: ... > > > RFC question: should pinmux-set take function name and group name > > > instead of the selector numbers? > > > > I would prefer names and integers (but from user p.o.v. names are > > easier to understand, while numbers are good for scripting). > > I don't actually see any example of looking up the function name in the > existing pinctrl code. There is pin_function_tree in struct pinctrl_dev. > pinmux_generic_get_function_name() does radix_tree_lookup() with the > selector integer as the key, but there is no corresponding "get function > selector by name" function. > > I think I would need to go through all the nodes in the radix tree to > find the name that matches. Although, I am just learning now about the > radix implementation in Linux so there might be a simpler way that I am > missing. I probably have to revive my work towards gluing ACPI with pin control where AFAIR I have created some kind of radix / rbtree for something (not sure it's exactly what you need here, so consider this just as a side note). ... > > The following is better to include in documentation and remove from > > the commit message. > > Shorter is better, what about simply > > > > # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions > > ? > > > > Btw in reST format you may create a nice citation of this. And yes, > > this should also go to the documentation. > > Good point, I'll shorten the example lines in v2. Even better to tell that we operate on the level of mount point of debugfs and use # cat pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions > > This and above is still part of documentation, and not a commit message thingy. > > Is something I should add to Documentation/driver-api/pinctl.rst in a > seperate patch? Not sure, I think more about as a part of this very path you change code and documentation. But usually it's a preference of the certain subsystem. ... > > > + if (cnt == 0) > > > + return 0; > > > > Has it ever happened here? > > Good point, I guess there is no reason for userspace to write 0 bytes. My point is that this check is done somewhere in the guts of kernfs. When in doubt I recommend to look around in the kernel and check most recent code with similar code pieces. ... > > > + buf = memdup_user_nul(user_buf, cnt); > > > + if (IS_ERR(buf)) > > > + return PTR_ERR(buf); > > > + > > > + if (buf[cnt - 1] == '\n') > > > + buf[cnt - 1] = '\0'; > > > > Shouldn't you rather use strndup_from_user() (or how is it called?) Any comments? ... > > Can be simply > > > > err_freebuf: > > kfree(buf); > > return err ?: cnt; > > Thanks, I didn't really like the duplication but was having trouble > thinking of a cleaner way to write it. That is good to know it is ok to > use the ternary operator in a return statement. Again, depends on certain subsystem maintainer's preferences. > > > + debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR, > > > + devroot, pctldev, &pinmux_set_ops); One more thing, as a preparatory patch please move from S_I* to plain octal numbers as it's preferable.
On Fri, Jan 22, 2021 at 11:50:52AM +0200, Andy Shevchenko wrote: > On Fri, Jan 22, 2021 at 1:26 AM Drew Fustini <drew@beagleboard.org> wrote: > > On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote: > > > On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@beagleboard.org> wrote: > > ... > > > > > RFC question: should pinmux-set take function name and group name > > > > instead of the selector numbers? > > > > > > I would prefer names and integers (but from user p.o.v. names are > > > easier to understand, while numbers are good for scripting). > > > > I don't actually see any example of looking up the function name in the > > existing pinctrl code. There is pin_function_tree in struct pinctrl_dev. > > pinmux_generic_get_function_name() does radix_tree_lookup() with the > > selector integer as the key, but there is no corresponding "get function > > selector by name" function. > > > > I think I would need to go through all the nodes in the radix tree to > > find the name that matches. Although, I am just learning now about the > > radix implementation in Linux so there might be a simpler way that I am > > missing. > > I probably have to revive my work towards gluing ACPI with pin control > where AFAIR I have created some kind of radix / rbtree for something > (not sure it's exactly what you need here, so consider this just as a > side note). > > ... > > > > The following is better to include in documentation and remove from > > > the commit message. > > > > Shorter is better, what about simply > > > > > > # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions > > > ? > > > > > > Btw in reST format you may create a nice citation of this. And yes, > > > this should also go to the documentation. > > > > Good point, I'll shorten the example lines in v2. > > Even better to tell that we operate on the level of mount point of > debugfs and use > > # cat pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions > > > > This and above is still part of documentation, and not a commit message thingy. > > > > Is something I should add to Documentation/driver-api/pinctl.rst in a > > seperate patch? > > Not sure, I think more about as a part of this very path you change > code and documentation. But usually it's a preference of the certain > subsystem. > > ... > > > > > + if (cnt == 0) > > > > + return 0; > > > > > > Has it ever happened here? > > > > Good point, I guess there is no reason for userspace to write 0 bytes. > > My point is that this check is done somewhere in the guts of kernfs. > When in doubt I recommend to look around in the kernel and check most > recent code with similar code pieces. > > ... > > > > > + buf = memdup_user_nul(user_buf, cnt); > > > > + if (IS_ERR(buf)) > > > > + return PTR_ERR(buf); > > > > + > > > > + if (buf[cnt - 1] == '\n') > > > > + buf[cnt - 1] = '\0'; > > > > > > Shouldn't you rather use strndup_from_user() (or how is it called?) > > Any comments? Sorry, I had meant to comment on that. I tried strndup_user() but had difficulty in using it as 'length > n' was always true and thus returned an error. There are not that many users of it either. I've switched to this based on how armada_debugfs_crtc_reg_write() in armada_debugfs.c handles the user writing multiple integers: char buf[16]; if (cnt > sizeof(buf) - 1) cnt = sizeof(buf) - 1; 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'; // the parse with sscanf() I choose 16 for buf as two integer numbers seperated by a space should never be more than 16. I suppose the downside is that it is allocated on the stack but it is a small buffer. I'll post v2 so it can be evaluated in the full patch context. > > ... > > > > Can be simply > > > > > > err_freebuf: > > > kfree(buf); > > > return err ?: cnt; > > > > Thanks, I didn't really like the duplication but was having trouble > > thinking of a cleaner way to write it. That is good to know it is ok to > > use the ternary operator in a return statement. > > Again, depends on certain subsystem maintainer's preferences. > > > > > > + debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR, > > > > + devroot, pctldev, &pinmux_set_ops); > > One more thing, as a preparatory patch please move from S_I* to plain > octal numbers as it's preferable. > > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index bab888fe3f8e..300e2b3d0ea8 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -673,6 +673,69 @@ void pinmux_show_setting(struct seq_file *s, DEFINE_SHOW_ATTRIBUTE(pinmux_functions); DEFINE_SHOW_ATTRIBUTE(pinmux_pins); +static ssize_t pinmux_set_write(struct file *file, const char __user *user_buf, + size_t cnt, loff_t *ppos) +{ + int err; + int fsel; + int gsel; + int ret; + char *buf; + struct seq_file *sfile; + struct pinctrl_dev *pctldev; + const struct pinmux_ops *ops; + + if (*ppos != 0) + return -EINVAL; + + if (cnt == 0) + return 0; + + buf = memdup_user_nul(user_buf, cnt); + if (IS_ERR(buf)) + return PTR_ERR(buf); + + if (buf[cnt - 1] == '\n') + buf[cnt - 1] = '\0'; + + ret = sscanf(buf, "%d %d", &fsel, &gsel); + if (ret != 2) { + pr_warn("%s: sscanf() expects '<fsel> <gsel>'", __func__); + err = -EINVAL; + goto err_freebuf; + } + + sfile = file->private_data; + pctldev = sfile->private; + ops = pctldev->desc->pmxops; + ret = ops->set_mux(pctldev, fsel, gsel); + if (ret != 0) { + pr_warn("%s(): set_mux() failed: %d", __func__, ret); + err = -EINVAL; + goto err_freebuf; + } + kfree(buf); + return cnt; + +err_freebuf: + kfree(buf); + return err; +} + +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_set_write, + .llseek = no_llseek, + .release = single_release, +}; + void pinmux_init_device_debugfs(struct dentry *devroot, struct pinctrl_dev *pctldev) { @@ -680,6 +743,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-set", S_IFREG | S_IWUSR, + 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-set" to debugfs. A 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_set_write() handles this by calling ops->set_mux() with fsel and gsel. RFC question: should pinmux-set take function name and group name instead of the selector numbers? The following is an example on the PocketBeagle [2] which has the 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 now be set back to gpio mode through pinmux-set. &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: root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pinmux-functions 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): root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '2 2' > pinmux-set Extra debug output that I added shows that pinctrl-single's set_mux() has set the register correctly for gpio mode: pinmux core: DEBUG pinmux_set_write(): returned 0 pinmux core: DEBUG pinmux_set_write(): buf=[2 2] pinmux core: DEBUG pinmux_set_write(): sscanf(2,2) pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=2, gsel=2) pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=2 pinctrl-single 44e10800.pinmux: enabling (null) function2 pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x21 val=0x2f Activate the pinmux_P1_36_pwm_pin function (fsel 6): root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '6 6' > pinmux-set pinctrl-single set_mux() is able to set register correctly for pwm mode: pinmux core: DEBUG pinmux_set_write(): returned 0 pinmux core: DEBUG pinmux_set_write(): buf=[6 6] pinmux core: DEBUG pinmux_set_write(): sscanf(6,6) pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=6, gsel=6) pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=6 pinctrl-single 44e10800.pinmux: enabling (null) function6 pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1 pinctrl-single 44e10800.pinmux: DEBUG 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 | 65 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)