| Message ID | 20201218045134.4158709-1-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2] pinctrl: add helper to expose pinctrl state in debugfs | expand |
On Fri, Dec 18, 2020 at 6:52 AM Drew Fustini <drew@beagleboard.org> wrote: > > BeagleBoard.org [0] currently uses an out-of-tree driver called > bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013. > The driver assists users of our BeagleBone and PocketBeagle boards in > rapid prototyping by allowing them to change at run-time between defined > set of pinctrl states [3] for each pin on the expansion connectors [4]. > This is achieved by exposing a 'state' file in sysfs for each pin which > is used by our 'config-pin' utility [5]. > > Our goal is to eliminate all out-of-tree drivers for BeagleBoard.org > boards and thus I have been working to replace bone-pinmux-helper with a > new driver that could be acceptable upstream. My understanding is that > debugfs, unlike sysfs, could be the appropriate mechanism to expose such > functionality. No objections here. > I used the compatible string "pinctrl,state-helper" but would appreciate > advice on how to best name this. Should I create a new vendor prefix? Here is the first concern. Why does this require to be a driver with a compatible string? > The P9_14_pinmux entry would cause pinctrl-state-helper to be probed. > The driver would create the corresponding pinctrl state file in debugfs > for the pin. Here is an example of how the state can be read and > written from userspace: > > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > default > root@beaglebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > pwm > > I would very much appreciate feedback on both this general concept, and > also specific areas in which the code should be changed to be acceptable > upstream. Two more concerns: - why is it OF only? - why has it been separated from pin control per device debug folder? > [0] http://beagleboard.org/latest-images > [1] https://github.com/beagleboard/linux/blob/5.4/drivers/misc/cape/beaglebone/bone-pinmux-helper.c > [2] https://github.com/RobertCNelson/linux-dev/blob/master/patches/drivers/ti/gpio/0001-BeagleBone-pinmux-helper.patch > [3] https://github.com/beagleboard/BeagleBoard-DeviceTrees/blob/v5.4.x-ti-overlays/src/arm/am335x-bone-common-univ.dtsi#L2084 > [4] https://github.com/beagleboard/beaglebone-black/wiki/System-Reference-Manual#section-7-1 > [5] https://github.com/beagleboard/bb.org-overlays/blob/master/tools/beaglebone-universal-io/config-pin -- With Best Regards, Andy Shevchenko
On Fri, Dec 18, 2020 at 06:01:25PM +0200, Andy Shevchenko wrote: > On Fri, Dec 18, 2020 at 6:52 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > BeagleBoard.org [0] currently uses an out-of-tree driver called > > bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013. > > The driver assists users of our BeagleBone and PocketBeagle boards in > > rapid prototyping by allowing them to change at run-time between defined > > set of pinctrl states [3] for each pin on the expansion connectors [4]. > > This is achieved by exposing a 'state' file in sysfs for each pin which > > is used by our 'config-pin' utility [5]. > > > > Our goal is to eliminate all out-of-tree drivers for BeagleBoard.org > > boards and thus I have been working to replace bone-pinmux-helper with a > > new driver that could be acceptable upstream. My understanding is that > > debugfs, unlike sysfs, could be the appropriate mechanism to expose such > > functionality. > > No objections here. > > > I used the compatible string "pinctrl,state-helper" but would appreciate > > advice on how to best name this. Should I create a new vendor prefix? > > Here is the first concern. Why does this require to be a driver with a > compatible string? I have not been able to figure out how to have different active pinctrl states for each header pins (for example P2 header pin 3) unless they are represented as DT nodes with their own compatible for this helper driver such as: &ocp { P2_03_pinmux { compatible = "pinctrl,state-helper"; pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pwm"; pinctrl-0 = <&P2_03_default_pin>; pinctrl-1 = <&P2_03_gpio_pin>; pinctrl-2 = <&P2_03_gpio_pu_pin>; pinctrl-3 = <&P2_03_gpio_pd_pin>; pinctrl-4 = <&P2_03_gpio_input_pin>; pinctrl-5 = <&P2_03_pwm_pin>; }; } I can assign pinctrl states in the pin controller DT node which has compatible pinctrl-single (line 301 arch/arm/boot/dts/am33xx-l4.dtsi): &am33xx_pinmux { pinctrl-names = "default", "gpio", "pwm"; pinctrl-0 = < &P2_03_default_pin &P1_34_default_pin &P2_19_default_pin &P2_24_default_pin &P2_33_default_pin &P2_22_default_pin &P2_18_default_pin &P2_10_default_pin &P2_06_default_pin &P2_04_default_pin &P2_02_default_pin &P2_08_default_pin &P2_17_default_pin >; pinctrl-1 = < &P2_03_gpio_pin &P1_34_gpio_pin &P2_19_gpio_pin &P2_24_gpio_pin &P2_33_gpio_pin &P2_22_gpio_pin &P2_18_gpio_pin &P2_10_gpio_pin &P2_06_gpio_pin &P2_04_gpio_pin &P2_02_gpio_pin &P2_08_gpio_pin &P2_17_gpio_pin >; pinctrl-2 = < &P2_03_pwm &P1_34_pwm &P2_19_pwm &P2_24_pwm &P2_33_pwm &P2_22_pwm &P2_18_pwm &P2_10_pwm &P2_06_pwm &P2_04_pwm &P2_02_pwm &P2_08_pwm &P2_17_pwm >; } However, there is no way to later select "gpio" for P2.03 and select "pwm" for P1.34 at the same time. Thus, I can not figure out a way to select independent states per pin unless I make a node for each pin that binds to a helper driver. It feels like there may be a simpler soluation but I can't see to figure it out. Suggestions welcome! > > > The P9_14_pinmux entry would cause pinctrl-state-helper to be probed. > > The driver would create the corresponding pinctrl state file in debugfs > > for the pin. Here is an example of how the state can be read and > > written from userspace: > > > > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > default > > root@beaglebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > pwm > > > > I would very much appreciate feedback on both this general concept, and > > also specific areas in which the code should be changed to be acceptable > > upstream. > > Two more concerns: > - why is it OF only? I am open to figuring out a more general solution but I am really only familiar with Device Tree. Is there a way to represent the possible pinctrl states in ACPI? > - why has it been separated from pin control per device debug folder? From the v1 thread, I see what you mean that there could be a combined state file for each pinctrl device where one would echo '<pin> <state-name>' such as 'P2_03 pwm'. I will attempt to implement that. > > > > [0] http://beagleboard.org/latest-images > > [1] https://github.com/beagleboard/linux/blob/5.4/drivers/misc/cape/beaglebone/bone-pinmux-helper.c > > [2] https://github.com/RobertCNelson/linux-dev/blob/master/patches/drivers/ti/gpio/0001-BeagleBone-pinmux-helper.patch > > [3] https://github.com/beagleboard/BeagleBoard-DeviceTrees/blob/v5.4.x-ti-overlays/src/arm/am335x-bone-common-univ.dtsi#L2084 > > [4] https://github.com/beagleboard/beaglebone-black/wiki/System-Reference-Manual#section-7-1 > > [5] https://github.com/beagleboard/bb.org-overlays/blob/master/tools/beaglebone-universal-io/config-pin > > -- > With Best Regards, > Andy Shevchenko Thanks for reviewing, Drew
On Thu, Dec 24, 2020 at 02:36:03PM -0600, Drew Fustini wrote: > On Fri, Dec 18, 2020 at 06:01:25PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 18, 2020 at 6:52 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > > > BeagleBoard.org [0] currently uses an out-of-tree driver called > > > bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013. > > > The driver assists users of our BeagleBone and PocketBeagle boards in > > > rapid prototyping by allowing them to change at run-time between defined > > > set of pinctrl states [3] for each pin on the expansion connectors [4]. > > > This is achieved by exposing a 'state' file in sysfs for each pin which > > > is used by our 'config-pin' utility [5]. > > > > > > Our goal is to eliminate all out-of-tree drivers for BeagleBoard.org > > > boards and thus I have been working to replace bone-pinmux-helper with a > > > new driver that could be acceptable upstream. My understanding is that > > > debugfs, unlike sysfs, could be the appropriate mechanism to expose such > > > functionality. > > > > No objections here. > > > > > I used the compatible string "pinctrl,state-helper" but would appreciate > > > advice on how to best name this. Should I create a new vendor prefix? > > > > Here is the first concern. Why does this require to be a driver with a > > compatible string? > > I have not been able to figure out how to have different active pinctrl > states for each header pins (for example P2 header pin 3) unless they > are represented as DT nodes with their own compatible for this helper > driver such as: > > &ocp { > P2_03_pinmux { > compatible = "pinctrl,state-helper"; > pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pwm"; > pinctrl-0 = <&P2_03_default_pin>; > pinctrl-1 = <&P2_03_gpio_pin>; > pinctrl-2 = <&P2_03_gpio_pu_pin>; > pinctrl-3 = <&P2_03_gpio_pd_pin>; > pinctrl-4 = <&P2_03_gpio_input_pin>; > pinctrl-5 = <&P2_03_pwm_pin>; > }; > } > > I can assign pinctrl states in the pin controller DT node which has > compatible pinctrl-single (line 301 arch/arm/boot/dts/am33xx-l4.dtsi): > > &am33xx_pinmux { > > pinctrl-names = "default", "gpio", "pwm"; > pinctrl-0 = < &P2_03_default_pin &P1_34_default_pin &P2_19_default_pin &P2_24_default_pin > &P2_33_default_pin &P2_22_default_pin &P2_18_default_pin &P2_10_default_pin > &P2_06_default_pin &P2_04_default_pin &P2_02_default_pin &P2_08_default_pin > &P2_17_default_pin >; > pinctrl-1 = < &P2_03_gpio_pin &P1_34_gpio_pin &P2_19_gpio_pin &P2_24_gpio_pin > &P2_33_gpio_pin &P2_22_gpio_pin &P2_18_gpio_pin &P2_10_gpio_pin > &P2_06_gpio_pin &P2_04_gpio_pin &P2_02_gpio_pin &P2_08_gpio_pin > &P2_17_gpio_pin >; > pinctrl-2 = < &P2_03_pwm &P1_34_pwm &P2_19_pwm &P2_24_pwm > &P2_33_pwm &P2_22_pwm &P2_18_pwm &P2_10_pwm > &P2_06_pwm &P2_04_pwm &P2_02_pwm &P2_08_pwm > &P2_17_pwm >; > > } > > However, there is no way to later select "gpio" for P2.03 and select > "pwm" for P1.34 at the same time. Thus, I can not figure out a way to > select independent states per pin unless I make a node for each pin that > binds to a helper driver. > > It feels like there may be a simpler soluation but I can't see to figure > it out. Suggestions welcome! > > > > > > The P9_14_pinmux entry would cause pinctrl-state-helper to be probed. > > > The driver would create the corresponding pinctrl state file in debugfs > > > for the pin. Here is an example of how the state can be read and > > > written from userspace: > > > > > > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > > default > > > root@beaglebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state > > > pwm > > > > > > I would very much appreciate feedback on both this general concept, and > > > also specific areas in which the code should be changed to be acceptable > > > upstream. > > > > Two more concerns: > > - why is it OF only? > > I am open to figuring out a more general solution but I am really only > familiar with Device Tree. Is there a way to represent the possible > pinctrl states in ACPI? > > > - why has it been separated from pin control per device debug folder? > > >From the v1 thread, I see what you mean that there could be a combined > state file for each pinctrl device where one would echo '<pin> > <state-name>' such as 'P2_03 pwm'. I will attempt to implement that. I have tried creating a single state file: /sys/kernel/debug/pinctrl/pinctrl_state where one can write into it: <device-name> <pinctrl-state-name> such as: ocp:P9_14_pinmux gpio However, I can not figure out a way for this to work. I create the pinctrl_state file in pinctrl_state_helper_init() and store the dentry in a global variable. pinctrl_state_helper_probe() still runs for each Px_0y_pinmux device tree entry with compatible "pinctrl,state-helper" but there is no per-device file created. The problem comes in pinctrl_state_write(). I use this to extract the device_name and state_name: ret = sscanf(buf, "%s %s", device_name, state_name); This does work okay but I don't know what to do with the device_name string, such as "ocp:P9_14_pinmux". Previously, the device was saved in the private info: sfile = file->private_data; priv = sfile->private; p = devm_pinctrl_get(priv->dev); // use device_name instead? state = pinctrl_lookup_state(p, state_name); But I don't know how to look up a device based on its name. Any suggestions as to how to handle that? Thanks and happy new year! Drew > > > > > > > > [0] http://beagleboard.org/latest-images > > > [1] https://github.com/beagleboard/linux/blob/5.4/drivers/misc/cape/beaglebone/bone-pinmux-helper.c > > > [2] https://github.com/RobertCNelson/linux-dev/blob/master/patches/drivers/ti/gpio/0001-BeagleBone-pinmux-helper.patch > > > [3] https://github.com/beagleboard/BeagleBoard-DeviceTrees/blob/v5.4.x-ti-overlays/src/arm/am335x-bone-common-univ.dtsi#L2084 > > > [4] https://github.com/beagleboard/beaglebone-black/wiki/System-Reference-Manual#section-7-1 > > > [5] https://github.com/beagleboard/bb.org-overlays/blob/master/tools/beaglebone-universal-io/config-pin > > > > -- > > With Best Regards, > > Andy Shevchenko > > Thanks for reviewing, > Drew
Hi Drew, sorry for belated review. The approach is so uncommon so it had me confused. On Thu, Dec 24, 2020 at 9:36 PM Drew Fustini <drew@beagleboard.org> wrote: > > > I used the compatible string "pinctrl,state-helper" but would appreciate > > > advice on how to best name this. Should I create a new vendor prefix? > > > > Here is the first concern. Why does this require to be a driver with a > > compatible string? > > I have not been able to figure out how to have different active pinctrl > states for each header pins (for example P2 header pin 3) unless they > are represented as DT nodes with their own compatible for this helper > driver such as: > > &ocp { > P2_03_pinmux { > compatible = "pinctrl,state-helper"; > pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pwm"; > pinctrl-0 = <&P2_03_default_pin>; > pinctrl-1 = <&P2_03_gpio_pin>; > pinctrl-2 = <&P2_03_gpio_pu_pin>; > pinctrl-3 = <&P2_03_gpio_pd_pin>; > pinctrl-4 = <&P2_03_gpio_input_pin>; > pinctrl-5 = <&P2_03_pwm_pin>; > }; > } I do not think the DT people are going to appreciate this pseudo-device. Can you not just represent them as pin control hogs and have the debugfs code with the other debugfs code in drivers/pinctrl/core.c? Normal drivers cannot play around with the state assigned to a hog, but debugfs can certainly do that so go ahead and patch the core. > I can assign pinctrl states in the pin controller DT node which has > compatible pinctrl-single (line 301 arch/arm/boot/dts/am33xx-l4.dtsi): > > &am33xx_pinmux { > > pinctrl-names = "default", "gpio", "pwm"; > pinctrl-0 = < &P2_03_default_pin &P1_34_default_pin &P2_19_default_pin &P2_24_default_pin > &P2_33_default_pin &P2_22_default_pin &P2_18_default_pin &P2_10_default_pin > &P2_06_default_pin &P2_04_default_pin &P2_02_default_pin &P2_08_default_pin > &P2_17_default_pin >; > pinctrl-1 = < &P2_03_gpio_pin &P1_34_gpio_pin &P2_19_gpio_pin &P2_24_gpio_pin > &P2_33_gpio_pin &P2_22_gpio_pin &P2_18_gpio_pin &P2_10_gpio_pin > &P2_06_gpio_pin &P2_04_gpio_pin &P2_02_gpio_pin &P2_08_gpio_pin > &P2_17_gpio_pin >; > pinctrl-2 = < &P2_03_pwm &P1_34_pwm &P2_19_pwm &P2_24_pwm > &P2_33_pwm &P2_22_pwm &P2_18_pwm &P2_10_pwm > &P2_06_pwm &P2_04_pwm &P2_02_pwm &P2_08_pwm > &P2_17_pwm >; > > } > > However, there is no way to later select "gpio" for P2.03 and select > "pwm" for P1.34 at the same time. Thus, I can not figure out a way to > select independent states per pin unless I make a node for each pin that > binds to a helper driver. > > It feels like there may be a simpler soluation but I can't see to figure > it out. Suggestions welcome! I think maybe there is no solution because you are solving a problem that only pinctrl-single while trying to stay generic? The single driver is special in that it requires all states of pins to be encoded into the device tree, but for debugging that is kind of unfriendly which was mentioned in its inception. For deep debugging it is good to let the core know of all available functions and groups and single does not IIUC. Yours, Linus Walleij
On Sat, Jan 09, 2021 at 02:22:07AM +0100, Linus Walleij wrote: > Hi Drew, > > sorry for belated review. The approach is so uncommon so it had me > confused. > > On Thu, Dec 24, 2020 at 9:36 PM Drew Fustini <drew@beagleboard.org> wrote: > > > > > I used the compatible string "pinctrl,state-helper" but would appreciate > > > > advice on how to best name this. Should I create a new vendor prefix? > > > > > > Here is the first concern. Why does this require to be a driver with a > > > compatible string? > > > > I have not been able to figure out how to have different active pinctrl > > states for each header pins (for example P2 header pin 3) unless they > > are represented as DT nodes with their own compatible for this helper > > driver such as: > > > > &ocp { > > P2_03_pinmux { > > compatible = "pinctrl,state-helper"; > > pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pwm"; > > pinctrl-0 = <&P2_03_default_pin>; > > pinctrl-1 = <&P2_03_gpio_pin>; > > pinctrl-2 = <&P2_03_gpio_pu_pin>; > > pinctrl-3 = <&P2_03_gpio_pd_pin>; > > pinctrl-4 = <&P2_03_gpio_input_pin>; > > pinctrl-5 = <&P2_03_pwm_pin>; > > }; > > } > > I do not think the DT people are going to appreciate this pseudo-device. Thank you for reviewing and commenting. It is does seem like creating a platform device for each header pin and binding to this proposed helper driver is not the correct approach. > Can you not just represent them as pin control hogs and have the debugfs > code with the other debugfs code in drivers/pinctrl/core.c? I tried defining pinctrl states in the am33xx_pinmux DT node (which has compatible "pinctrl-single"). It does work to have default state defined for pins. However, I was not sure how represent having different states active for independent header pins. Instead of DT binds, maybe I need to use PIN_MAP_MUX_GROUP_HOG_DEFAULT() in pinctrl-single code? > > Normal drivers cannot play around with the state assigned to a > hog, but debugfs can certainly do that so go ahead and patch > the core. Is there an existing debugfs file that you think would be appropriate to allow the state of a hog to be changed? > > I can assign pinctrl states in the pin controller DT node which has > > compatible pinctrl-single (line 301 arch/arm/boot/dts/am33xx-l4.dtsi): > > > > &am33xx_pinmux { > > > > pinctrl-names = "default", "gpio", "pwm"; > > pinctrl-0 = < &P2_03_default_pin &P1_34_default_pin &P2_19_default_pin &P2_24_default_pin > > &P2_33_default_pin &P2_22_default_pin &P2_18_default_pin &P2_10_default_pin > > &P2_06_default_pin &P2_04_default_pin &P2_02_default_pin &P2_08_default_pin > > &P2_17_default_pin >; > > pinctrl-1 = < &P2_03_gpio_pin &P1_34_gpio_pin &P2_19_gpio_pin &P2_24_gpio_pin > > &P2_33_gpio_pin &P2_22_gpio_pin &P2_18_gpio_pin &P2_10_gpio_pin > > &P2_06_gpio_pin &P2_04_gpio_pin &P2_02_gpio_pin &P2_08_gpio_pin > > &P2_17_gpio_pin >; > > pinctrl-2 = < &P2_03_pwm &P1_34_pwm &P2_19_pwm &P2_24_pwm > > &P2_33_pwm &P2_22_pwm &P2_18_pwm &P2_10_pwm > > &P2_06_pwm &P2_04_pwm &P2_02_pwm &P2_08_pwm > > &P2_17_pwm >; > > > > } > > > > However, there is no way to later select "gpio" for P2.03 and select > > "pwm" for P1.34 at the same time. Thus, I can not figure out a way to > > select independent states per pin unless I make a node for each pin that > > binds to a helper driver. > > > > It feels like there may be a simpler soluation but I can't see to figure > > it out. Suggestions welcome! > > I think maybe there is no solution because you are solving a problem > that only pinctrl-single while trying to stay generic? The single > driver is special in that it requires all states of pins to be encoded > into the device tree, but for debugging that is kind of unfriendly > which was mentioned in its inception. For deep debugging it is good > to let the core know of all available functions and groups and > single does not IIUC. > > Yours, > Linus Walleij I discussed my use case and this patch on #armlinux earlier this week and Alexandre Belloni suggested looking at the pinmux-pins debugfs file. This made me think that a possible solution could be to define a store function for pinmux-pins to handle something like "<pin#> <function#>". I believe the ability to activate a pin function (or pin group) from userspace would satisfy our beagleboard.org use-case. Does that seem like a reasonable approach? Thank you! Drew
On Sat, Jan 9, 2021 at 3:55 AM Drew Fustini <drew@beagleboard.org> wrote: > I discussed my use case and this patch on #armlinux earlier this week > and Alexandre Belloni suggested looking at the pinmux-pins debugfs file. This sounds reasonable. > This made me think that a possible solution could be to define a store > function for pinmux-pins to handle something like "<pin#> <function#>". > I believe the ability to activate a pin function (or pin group) from > userspace would satisfy our beagleboard.org use-case. > > Does that seem like a reasonable approach? This sounds like a good approach. Yours, Linus Walleij
Hi, * Linus Walleij <linus.walleij@linaro.org> [210109 21:14]: > On Sat, Jan 9, 2021 at 3:55 AM Drew Fustini <drew@beagleboard.org> wrote: > > > I discussed my use case and this patch on #armlinux earlier this week > > and Alexandre Belloni suggested looking at the pinmux-pins debugfs file. > > This sounds reasonable. > > > This made me think that a possible solution could be to define a store > > function for pinmux-pins to handle something like "<pin#> <function#>". > > I believe the ability to activate a pin function (or pin group) from > > userspace would satisfy our beagleboard.org use-case. > > > > Does that seem like a reasonable approach? > > This sounds like a good approach. Makes sense to me too. We may want to make it into a proper sysfs interface eventually to not require debugfs be enabled in .config. But that's another set of patches, certainly makes sense to first enable it for debugfs. Regards, Tony
On Mon, Jan 11, 2021 at 12:03:18PM +0200, Tony Lindgren wrote: > Hi, > > * Linus Walleij <linus.walleij@linaro.org> [210109 21:14]: > > On Sat, Jan 9, 2021 at 3:55 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > > I discussed my use case and this patch on #armlinux earlier this week > > > and Alexandre Belloni suggested looking at the pinmux-pins debugfs file. > > > > This sounds reasonable. > > > > > This made me think that a possible solution could be to define a store > > > function for pinmux-pins to handle something like "<pin#> <function#>". > > > I believe the ability to activate a pin function (or pin group) from > > > userspace would satisfy our beagleboard.org use-case. > > > > > > Does that seem like a reasonable approach? > > > > This sounds like a good approach. > > Makes sense to me too. > > We may want to make it into a proper sysfs interface eventually to not > require debugfs be enabled in .config. But that's another set of patches, > certainly makes sense to first enable it for debugfs. > > Regards, > > Tony I have added a debugfs file "pinmux-set" to pinmux.c. This allows "<function-number> <group-number>" to be written into that file. The function pinmux_set_write() calls ops->set_mux() with fsel and gsel. I'll post an RFC with the code, but I am wondering if it would better to take the function as a name and then lookup the function number (fsel)? thanks, drew
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 8828613c4e0e..4faed5c8c83b 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -255,6 +255,16 @@ config PINCTRL_SINGLE help This selects the device tree based generic pinctrl driver. +config PINCTRL_STATE_HELPER + tristate "Helper to expose pinctrl state to debugfs" + depends on OF + depends on HAS_IOMEM + select GENERIC_PINCTRL_GROUPS + select GENERIC_PINMUX_FUNCTIONS + select GENERIC_PINCONF + help + This selects the device tree based generic pinctrl driver. + config PINCTRL_SIRF bool "CSR SiRFprimaII pin controller driver" depends on ARCH_SIRF diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index 1731b2154df9..156c356dbd3f 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o +obj-$(CONFIG_PINCTRL_STATE_HELPER) += pinctrl-state-helper.o obj-$(CONFIG_PINCTRL_SIRF) += sirf/ obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o obj-$(CONFIG_ARCH_TEGRA) += tegra/ diff --git a/drivers/pinctrl/pinctrl-state-helper.c b/drivers/pinctrl/pinctrl-state-helper.c new file mode 100644 index 000000000000..81f9932d6ec5 --- /dev/null +++ b/drivers/pinctrl/pinctrl-state-helper.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Pinmux state helper driver + * + * Copyright (C) 2013 Pantelis Antoniou <panto@antoniou-consulting.com> + * Copyright (C) 2020 Drew Fustini <drew@beagleboard.org> + */ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/fs.h> +#include <linux/path.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> +#include <linux/dcache.h> +#include "core.h" + +#define DRIVER_NAME "pinctrl_state_helper" +/* RFC: is there a better directory name? */ +#define DEBUGFS_DIR "pinctrl_state" +#define STATE_NAME_MAX 64 + +static struct dentry *debugfs_root; + +struct pinctrl_state_helper_priv { + struct device *dev; + struct pinctrl *pinctrl; + struct dentry *pin_dentry; + /* RFC: should this be dynamically allocated? */ + char selected_state_name[STATE_NAME_MAX]; +}; + +static ssize_t pinctrl_state_read(struct file *file, char __user *user_buf, + size_t cnt, loff_t *ppos) +{ + struct pinctrl_state_helper_priv *priv; + struct seq_file *sfile; + char *state_name; + ssize_t len; + char *buf; + + if (*ppos != 0) + return 0; + + sfile = file->private_data; + if (!sfile) + return -ENODEV; + + priv = sfile->private; + if (!priv) + return -ENODEV; + + state_name = priv->selected_state_name; + if (state_name == NULL || strlen(state_name) == 0) + state_name = "none"; + + buf = kasprintf(GFP_KERNEL, "%s\n", state_name); + if (!buf) + return -ENOMEM; + + if (cnt < strlen(buf)) { + kfree(buf); + return -ENOSPC; + } + + len = simple_read_from_buffer(user_buf, cnt, ppos, buf, strlen(buf)); + kfree(buf); + return len; +} + +static ssize_t pinctrl_state_write(struct file *file, const char __user *user_buf, + size_t cnt, loff_t *ppos) +{ + int err; + char *state_name; + struct seq_file *sfile; + struct pinctrl *p; + struct pinctrl_state *state; + struct pinctrl_state_helper_priv *priv; + + if (*ppos != 0) + return -EINVAL; + + if (cnt == 0) + return 0; + + state_name = memdup_user_nul(user_buf, cnt); + if (IS_ERR(state_name)) + return PTR_ERR(state_name); + + if (state_name[cnt - 1] == '\n') + state_name[cnt - 1] = '\0'; + + sfile = file->private_data; + priv = sfile->private; + p = devm_pinctrl_get(priv->dev); + state = pinctrl_lookup_state(p, state_name); + if (IS_ERR(state)) { + err = PTR_ERR_OR_ZERO(state); + goto err_no_state; + } + + err = pinctrl_select_state(p, state); + if (err != 0) { + err = -EINVAL; + goto err_no_state; + } + strncpy(priv->selected_state_name, state_name, STATE_NAME_MAX); + + kfree(state_name); + return cnt; + +err_no_state: + kfree(state_name); + return err; +} + +static int pinctrl_state_open(struct inode *inode, struct file *file) +{ + return single_open(file, NULL, inode->i_private); +} + +static const struct file_operations pinctrl_state_ops = { + .owner = THIS_MODULE, + .open = pinctrl_state_open, + .read = pinctrl_state_read, + .write = pinctrl_state_write, + .llseek = no_llseek, + .release = single_release, +}; + +static int pinctrl_state_helper_probe(struct platform_device *pdev) +{ + int err; + struct pinctrl_state *state; + struct pinctrl_state_helper_priv *priv; + struct dentry *pin_dir; + struct device *dev = &pdev->dev; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (priv == NULL) { + dev_err(dev, "Failed to allocate priv"); + return -ENOMEM; + } + + platform_set_drvdata(pdev, priv); + + priv->pinctrl = devm_pinctrl_get(dev); + if (IS_ERR(priv->pinctrl)) { + dev_err(dev, "Failed to get pinctrl"); + err = PTR_ERR_OR_ZERO(priv->pinctrl); + goto err_no_pinctrl; + } + + strcpy(priv->selected_state_name, PINCTRL_STATE_DEFAULT); + state = pinctrl_lookup_state(priv->pinctrl, priv->selected_state_name); + if (IS_ERR(state)) { + dev_err(dev, "Failed to lookup default state"); + err = PTR_ERR_OR_ZERO(state); + goto err_no_state; + } + + err = pinctrl_select_state(priv->pinctrl, state); + if (err != 0) { + dev_err(dev, "Failed to select default state"); + goto err_no_state; + } + + pin_dir = debugfs_create_dir(dev_name(dev), debugfs_root); + priv->dev = dev; + priv->pin_dentry = debugfs_create_file("state", 0600, pin_dir, priv, &pinctrl_state_ops); + + return 0; + +err_no_state: + devm_pinctrl_put(priv->pinctrl); +err_no_pinctrl: + devm_kfree(dev, priv); + return err; +} + +static int pinctrl_state_helper_remove(struct platform_device *pdev) +{ + struct pinctrl_state_helper_priv *priv; + + priv = platform_get_drvdata(pdev); + devm_pinctrl_put(priv->pinctrl); + debugfs_remove(priv->pin_dentry); + + return 0; +} + +static const struct of_device_id helper_of_match[] = { + { + /* RFC: keep generic or make begalebone specific? */ + .compatible = "pinctrl,state-helper", + }, + { }, +}; +MODULE_DEVICE_TABLE(of, helper_of_match); + +static struct platform_driver pinctrl_state_helper_driver = { + .probe = pinctrl_state_helper_probe, + .remove = pinctrl_state_helper_remove, + .driver = { + .name = "pinctrl_state_helper", + .owner = THIS_MODULE, + .of_match_table = helper_of_match, + }, +}; + +static int pinctrl_state_helper_init(void) +{ + int err; + struct dentry *pinctrl_dir; + + pinctrl_dir = debugfs_lookup("pinctrl", NULL); + debugfs_root = debugfs_create_dir(DEBUGFS_DIR, pinctrl_dir); + if (IS_ERR(debugfs_root) || !debugfs_root) { + pr_err("Failed to create debugfs directory %s", DEBUGFS_DIR); + return -EINVAL; + } + + err = platform_driver_register(&pinctrl_state_helper_driver); + return err; +} +module_init(pinctrl_state_helper_init); + +static void pinctrl_state_helper_exit(void) +{ + debugfs_remove_recursive(debugfs_root); + platform_driver_unregister(&pinctrl_state_helper_driver); +} +module_exit(pinctrl_state_helper_exit); + +MODULE_AUTHOR("Drew Fustini <drew@beagleboard.org>"); +MODULE_DESCRIPTION("Helper driver exposes pinctrl state to debugfs"); +MODULE_LICENSE("GPL v2");
BeagleBoard.org [0] currently uses an out-of-tree driver called bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013. The driver assists users of our BeagleBone and PocketBeagle boards in rapid prototyping by allowing them to change at run-time between defined set of pinctrl states [3] for each pin on the expansion connectors [4]. This is achieved by exposing a 'state' file in sysfs for each pin which is used by our 'config-pin' utility [5]. Our goal is to eliminate all out-of-tree drivers for BeagleBoard.org boards and thus I have been working to replace bone-pinmux-helper with a new driver that could be acceptable upstream. My understanding is that debugfs, unlike sysfs, could be the appropriate mechanism to expose such functionality. Here is an example of how pin P9.14 on the BeagleBone Black expansion connector [6] would be represented in device tree: &am33xx_pinmux { /* P9_14 (ZCZ ball U14) gpio1_18 */ P9_14_default_pin: pinmux_P9_14_default_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE7) >; }; P9_14_gpio_pin: pinmux_P9_14_gpio_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT | INPUT_EN | MUX_MODE7) >; }; P9_14_gpio_pu_pin: pinmux_P9_14_gpio_pu_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLUP | INPUT_EN | MUX_MODE7) >; }; P9_14_gpio_pd_pin: pinmux_P9_14_gpio_pd_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE7) >; }; P9_14_gpio_input_pin: pinmux_P9_14_gpio_input_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_INPUT | MUX_MODE7) >; }; P9_14_pwm_pin: pinmux_P9_14_pwm_pin { pinctrl-single,pins = < AM33XX_IOPAD(0x0848, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE6) >; }; }; &ocp { /* P9_14 (ZCZ ball U14) */ P9_14_pinmux { compatible = "pinctrl,state-helper"; status = "okay"; pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pwm"; pinctrl-0 = <&P9_14_default_pin>; pinctrl-1 = <&P9_14_gpio_pin>; pinctrl-2 = <&P9_14_gpio_pu_pin>; pinctrl-3 = <&P9_14_gpio_pd_pin>; pinctrl-4 = <&P9_14_gpio_input_pin>; pinctrl-5 = <&P9_14_pwm_pin>; }; }; I used the compatible string "pinctrl,state-helper" but would appreciate advice on how to best name this. Should I create a new vendor prefix? The P9_14_pinmux entry would cause pinctrl-state-helper to be probed. The driver would create the corresponding pinctrl state file in debugfs for the pin. Here is an example of how the state can be read and written from userspace: root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state default root@beaglebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state pwm I would very much appreciate feedback on both this general concept, and also specific areas in which the code should be changed to be acceptable upstream. Thank you! Drew [0] http://beagleboard.org/latest-images [1] https://github.com/beagleboard/linux/blob/5.4/drivers/misc/cape/beaglebone/bone-pinmux-helper.c [2] https://github.com/RobertCNelson/linux-dev/blob/master/patches/drivers/ti/gpio/0001-BeagleBone-pinmux-helper.patch [3] https://github.com/beagleboard/BeagleBoard-DeviceTrees/blob/v5.4.x-ti-overlays/src/arm/am335x-bone-common-univ.dtsi#L2084 [4] https://github.com/beagleboard/beaglebone-black/wiki/System-Reference-Manual#section-7-1 [5] https://github.com/beagleboard/bb.org-overlays/blob/master/tools/beaglebone-universal-io/config-pin [6] https://beagleboard.org/Support/bone101/#headers Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Cc: Pantelis Antoniou <pantelis.antoniou@linaro.org> Cc: Pantelis Antoniou <pantelis.antoniou@gmail.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> Signed-off-by: Drew Fustini <drew@beagleboard.org> --- v2 changes: - add RFC: comments in places where I would appreciate feedback - remove panto@antoniou-consulting.com as it is bouncing - add alternative email addresses for Pantelis - update pinctrl_state_read() to use kasprintf() based on Andy's advice - update pinctrl_state_write() to use memdup_user_nul() - call debugfs_lookup() to use existing "pinctrl" dir as parent - create subdir "pinctrl_state" to be parent of per-pin directories - add pinctrl_state_helper_init() and pinctrl_state_helper_exit() to handle creating and cleaning up debugfs_root ("pinctrl_state") drivers/pinctrl/Kconfig | 10 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-state-helper.c | 241 +++++++++++++++++++++++++ 3 files changed, 252 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-state-helper.c