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/ocp\:P9_14_pinmux/state
default
root@beaglebone:~# echo pwm > /sys/kernel/debug/ocp\:P9_14_pinmux/state
root@beaglebone:~# cat /sys/kernel/debug/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 <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Tony Lindgren <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
drivers/pinctrl/Kconfig | 10 ++
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-state-helper.c | 233 +++++++++++++++++++++++++
3 files changed, 244 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-state-helper.c
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..d11edb9ee9b4
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-state-helper.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Pinmux state helper driver
+ *
+ * Copyright (C) 2013 Pantelis Antoniou <[email protected]>
+ * Copyright (C) 2020 Drew Fustini <[email protected]>
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/interrupt.h>
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/dcache.h>
+
+#include "core.h"
+#include "devicetree.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define DRIVER_NAME "pinctrl_state_helper"
+
+struct pinctrl_state_helper_priv {
+ unsigned int offset;
+ struct device *dev;
+ struct pinctrl *pinctrl;
+ char selected_state_name[64];
+};
+
+static ssize_t pinctrl_state_read(struct file *file,
+ char __user *usr_buf,
+ size_t size, loff_t *ppos)
+{
+ int cnt;
+ char buf[64];
+ struct pinctrl_state_helper_priv *priv;
+ struct seq_file *sfile;
+ char *state_name;
+
+ sfile = file->private_data;
+ priv = sfile->private;
+ state_name = priv->selected_state_name;
+ if (state_name == NULL || strlen(state_name) == 0)
+ state_name = "none";
+
+ if (*ppos != 0)
+ return 0;
+
+ cnt = snprintf(buf, sizeof(buf), "%s\n", state_name);
+
+ return simple_read_from_buffer(usr_buf, size, ppos, buf, cnt);
+}
+
+static ssize_t pinctrl_state_write(struct file *file, const char __user *ubuf, size_t cnt, loff_t *ppos)
+{
+ int err;
+ char *s;
+ char state_name[64];
+ struct seq_file *sfile;
+ struct pinctrl *p;
+ struct pinctrl_state *state;
+ struct pinctrl_state_helper_priv *priv;
+
+ if (cnt > 63) {
+ pr_debug("%s: cnt TRUNCATED to 63", __func__);
+ cnt = 63;
+ }
+
+ if (copy_from_user(state_name, ubuf, cnt)) {
+ pr_debug("%s: return -EFAULT", __func__);
+ return -EFAULT;
+ }
+
+ state_name[cnt] = '\0';
+ s = strchr(state_name, '\n');
+ if (s != NULL)
+ *s = '\0';
+
+ sfile = file->private_data;
+ priv = sfile->private;
+ strncpy(priv->selected_state_name, state_name, 64);
+
+ p = devm_pinctrl_get(priv->dev);
+
+ state = pinctrl_lookup_state(p, state_name);
+
+ if (!IS_ERR(state)) {
+ err = pinctrl_select_state(p, state);
+ if (err != 0) {
+ pr_err("Failed to select state %s\n", state_name);
+ return -EINVAL;
+ }
+ } else {
+ err = PTR_ERR_OR_ZERO(state);
+ pr_err("Failed to find state %s err=%d\n", state_name, err);
+ return -EINVAL;
+ }
+
+ if (*ppos != 0) {
+ pr_err("%s: ppos not zero, return -EINVAL", __func__);
+ return -EINVAL;
+ }
+
+ return cnt;
+}
+
+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)
+{
+ struct pinctrl_state *state;
+ char *state_name = "default";
+ struct pinctrl_state_helper_priv *priv;
+ struct device *dev = &pdev->dev;
+ int err;
+ struct dentry *pin_dentry;
+ struct dentry *helper_dir;
+ struct dentry *parent = NULL;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (priv == NULL) {
+ dev_err(&pdev->dev, "Failed to allocate priv\n");
+ err = -ENOMEM;
+ goto err_no_mem;
+ }
+
+ state_name = devm_kzalloc(&pdev->dev, strlen(PINCTRL_STATE_DEFAULT) + 1,
+ GFP_KERNEL);
+ if (state_name == NULL) {
+ dev_err(dev, "Failed to allocate state name\n");
+ err = -ENOMEM;
+ goto err_no_state_mem;
+ }
+ strcpy(priv->selected_state_name, PINCTRL_STATE_DEFAULT);
+ platform_set_drvdata(pdev, priv);
+
+ priv->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(priv->pinctrl)) {
+ dev_err(dev, "Failed to get pinctrl\n");
+ err = PTR_ERR_OR_ZERO(priv->pinctrl);
+ goto err_no_pinctrl;
+ }
+
+ if (err != 0) {
+ state = pinctrl_lookup_state(priv->pinctrl,
+ priv->selected_state_name);
+ if (!IS_ERR(state)) {
+ err = pinctrl_select_state(priv->pinctrl, state);
+ if (err != 0) {
+ dev_err(dev, "Failed to select default state\n");
+ goto err_no_state;
+ }
+ } else {
+ *priv->selected_state_name = '\0';
+ }
+ }
+
+ helper_dir = debugfs_create_dir(dev_name(dev), parent);
+ priv->dev = dev;
+ pin_dentry = debugfs_create_file("state", 0600, helper_dir, priv, &pinctrl_state_ops);
+
+ return 0;
+
+err_no_state:
+ devm_pinctrl_put(priv->pinctrl);
+err_no_pinctrl:
+ devm_kfree(dev, priv->selected_state_name);
+err_no_state_mem:
+ devm_kfree(dev, priv);
+err_no_mem:
+ 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);
+ return 0;
+}
+
+static const struct of_device_id helper_of_match[] = {
+ {
+ .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,
+ },
+};
+
+module_platform_driver(pinctrl_state_helper_driver);
+
+MODULE_AUTHOR("Drew Fustini <[email protected]>");
+MODULE_DESCRIPTION("One-register-per-pin type device tree based pinctrl driver");
+MODULE_LICENSE("GPL v2");
--
2.25.1
On Fri, Dec 11, 2020 at 1:54 PM Drew Fustini <[email protected]> wrote:
>
> BeagleBoard.org [0] currently uses an out-of-tree driver called
> bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013.
And it looks like it's still using APIs from 2013.
Needs quite a clean up.
> 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.
Yeah, for debugfs we don't require too much and esp. there is no
requirement to keep backward compatibility thru interface.
I.o.w. it's *not* an ABI.
...
> 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?
Since it's BB specific, it should have file name and compatible string
accordingly.
But I'm wondering, why it requires this kind of thing and can't be
simply always part of the kernel based on configuration option?
> 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/ocp\:P9_14_pinmux/state
> default
> root@beaglebone:~# echo pwm > /sys/kernel/debug/ocp\:P9_14_pinmux/state
> root@beaglebone:~# cat /sys/kernel/debug/ocp\:P9_14_pinmux/state
> pwm
Shouldn't it be rather a part of a certain pin control folder:
debug/pinctrl/.../mux/...
?
> 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.
I will give time for more discussion about concepts and so, because
code (as stated above) is quite old and requires a lot of cleaning up.
--
With Best Regards,
Andy Shevchenko
On Fri, Dec 11, 2020 at 11:15:21PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 11, 2020 at 1:54 PM Drew Fustini <[email protected]> wrote:
> >
> > BeagleBoard.org [0] currently uses an out-of-tree driver called
> > bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013.
>
> And it looks like it's still using APIs from 2013.
> Needs quite a clean up.
Thanks for taking a look at my RFC and responding. It is good to know
that it is using out-dated APIs. Would you be able to elaborate?
It interacts with pinctrl core through devm_pinctrl_get(),
pinctrl_lookup_state() and pinctrl_select_state(). Is there newer way of
doing that?
> > 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.
>
> Yeah, for debugfs we don't require too much and esp. there is no
> requirement to keep backward compatibility thru interface.
> I.o.w. it's *not* an ABI.
>
> ...
>
> > 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?
>
> Since it's BB specific, it should have file name and compatible string
> accordingly.
At first, I was thinking about this as a beaglebone specific solution
and had bone in the driver name and compatible string. But then I
realized it could used in other situations where it is beneficial to
to read and select a pinctrl state through debugfs.
I'm happy to rebrand the naming as beaglebone if that would be more
acceptable.
> But I'm wondering, why it requires this kind of thing and can't be
> simply always part of the kernel based on configuration option?
Do you mean not having a new CONFIG option for this driver and just have
it be enabled by CONFIG_PINCTRL?
> > 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/ocp\:P9_14_pinmux/state
> > default
> > root@beaglebone:~# echo pwm > /sys/kernel/debug/ocp\:P9_14_pinmux/state
> > root@beaglebone:~# cat /sys/kernel/debug/ocp\:P9_14_pinmux/state
> > pwm
>
> Shouldn't it be rather a part of a certain pin control folder:
> debug/pinctrl/.../mux/...
> ?
Yes, I think that would make sense, but I was struggling to figure out
how to do that. pinctrl_init_debugfs() in pinctrl/core.c does create the
"pinctrl" directory, but I could not figure out how to use this as the
parent dir when calling debugfs_create_dir() in this driver's probe().
I thought there might be a way in debugfs API to use existing directory
path as a parent but I couldn't figure anything like that. I would
appreciate any advice.
>
> > 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.
>
> I will give time for more discussion about concepts and so, because
> code (as stated above) is quite old and requires a lot of cleaning up.
Thanks for taking the time to comment. I'll look at other drivers to see
the ways in which this drivers is out-dated.
-Drew
On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <[email protected]> wrote:
> > On Fri, Dec 11, 2020 at 11:15:21PM +0200, Andy Shevchenko wrote:
> > > On Fri, Dec 11, 2020 at 1:54 PM Drew Fustini <[email protected]> wrote:
> > > >
> > > > BeagleBoard.org [0] currently uses an out-of-tree driver called
> > > > bone-pinmux-helper [1] developed by Pantelis Antoniou [2] back in 2013.
> > >
> > > And it looks like it's still using APIs from 2013.
> > > Needs quite a clean up.
> >
> > Thanks for taking a look at my RFC and responding. It is good to know
> > that it is using out-dated APIs. Would you be able to elaborate?
> >
> > It interacts with pinctrl core through devm_pinctrl_get(),
> > pinctrl_lookup_state() and pinctrl_select_state(). Is there newer way of
> > doing that?
>
> No. I'm talking mostly about FS callbacks where some relatively old
> new APIs can be used, such as kasprintf().
Thanks for following up. I'll will take a look at that and update the code.
> > > > 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?
> > >
> > > Since it's BB specific, it should have file name and compatible string
> > > accordingly.
> >
> > At first, I was thinking about this as a beaglebone specific solution
> > and had bone in the driver name and compatible string. But then I
> > realized it could used in other situations where it is beneficial to
> > to read and select a pinctrl state through debugfs.
> >
> > I'm happy to rebrand the naming as beaglebone if that would be more
> > acceptable.
>
> See below.
>
> > > But I'm wondering, why it requires this kind of thing and can't be
> > > simply always part of the kernel based on configuration option?
> >
> > Do you mean not having a new CONFIG option for this driver and just have
> > it be enabled by CONFIG_PINCTRL?
>
> No, configuration option stays, but no compatible strings no nothing
> like that. Just probed always when loaded.
I first started down the route of implementing this inside of
pinctrl-single. I found it didn't work because devm_pinctrl_get() would
fail. I think was because it was happening too early for pinctrl to be
ready.
I do think it seems awkward to have to add this to dts and have the
driver get probed for each entry:
P1_04_pinmux {
compatible = "pinctrl,state-helper";
status = "okay";
pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pruout", "pruin";
pinctrl-0 = <&P1_04_default_pin>;
pinctrl-1 = <&P1_04_gpio_pin>;
pinctrl-2 = <&P1_04_gpio_pu_pin>;
pinctrl-3 = <&P1_04_gpio_pd_pin>;
pinctrl-4 = <&P1_04_gpio_input_pin>;
pinctrl-5 = <&P1_04_pruout_pin>;
pinctrl-6 = <&P1_04_pruin_pin>;
};
But I am having a hard time figuring out another way of doing it.
Any ideas as to what would trigger the probe() if there was not a match
on a compatible like "pinctrl,state-helper"?
> Actually not even sure we want to have it as a module.
And have just be a part of one of the existing pinctrl files like core.c?
>
> ...
>
> > > > 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/ocp\:P9_14_pinmux/state
> > > > default
> > > > root@beaglebone:~# echo pwm > /sys/kernel/debug/ocp\:P9_14_pinmux/state
> > > > root@beaglebone:~# cat /sys/kernel/debug/ocp\:P9_14_pinmux/state
> > > > pwm
> > >
> > > Shouldn't it be rather a part of a certain pin control folder:
> > > debug/pinctrl/.../mux/...
> > > ?
> >
> > Yes, I think that would make sense, but I was struggling to figure out
> > how to do that. pinctrl_init_debugfs() in pinctrl/core.c does create the
> > "pinctrl" directory, but I could not figure out how to use this as the
> > parent dir when calling debugfs_create_dir() in this driver's probe().
> >
> > I thought there might be a way in debugfs API to use existing directory
> > path as a parent but I couldn't figure anything like that. I would
> > appreciate any advice.
>
> If the option is boolean from the beginning then you just call it from
> the corresponding pin control instantiation chain.
Sorry, I am not sure I understand what you mean here. What does
"option" mean in this context? I don't think there is any value that is
boolean invovled. The pinctrl states are strings.
With regards to parent directory, I did discover there is
debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
subdirectory inside of it. This is the structure now:
/sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
/sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
/sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
/sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
etc..
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks for reviewing,
Drew
On Tue, Dec 15, 2020 at 9:36 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <[email protected]> wrote:
> > On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
...
> > With regards to parent directory, I did discover there is
> > debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
> > subdirectory inside of it. This is the structure now:
> >
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
> > etc..
Missed part to comment.
I was talking about
/sys/kernel/debug/pinctrl/<$PINCTRL>/mux/<$PIN> (maybe folder, maybe node)
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 09:36:33PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <[email protected]> wrote:
> > On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> > > On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <[email protected]> wrote:
> > > > On Fri, Dec 11, 2020 at 11:15:21PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Dec 11, 2020 at 1:54 PM Drew Fustini <[email protected]> wrote:
>
> ...
>
> > > > > But I'm wondering, why it requires this kind of thing and can't be
> > > > > simply always part of the kernel based on configuration option?
> > > >
> > > > Do you mean not having a new CONFIG option for this driver and just have
> > > > it be enabled by CONFIG_PINCTRL?
> > >
> > > No, configuration option stays, but no compatible strings no nothing
> > > like that. Just probed always when loaded.
> >
> > I first started down the route of implementing this inside of
> > pinctrl-single. I found it didn't work because devm_pinctrl_get() would
> > fail. I think was because it was happening too early for pinctrl to be
> > ready.
> >
> > I do think it seems awkward to have to add this to dts and have the
> > driver get probed for each entry:
> >
> > P1_04_pinmux {
> > compatible = "pinctrl,state-helper";
> > status = "okay";
> > pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pruout", "pruin";
> > pinctrl-0 = <&P1_04_default_pin>;
> > pinctrl-1 = <&P1_04_gpio_pin>;
> > pinctrl-2 = <&P1_04_gpio_pu_pin>;
> > pinctrl-3 = <&P1_04_gpio_pd_pin>;
> > pinctrl-4 = <&P1_04_gpio_input_pin>;
> > pinctrl-5 = <&P1_04_pruout_pin>;
> > pinctrl-6 = <&P1_04_pruin_pin>;
> > };
> >
> > But I am having a hard time figuring out another way of doing it.
>
> I'm not a DT expert and I have no clue why you need all this. To me it
> looks over engineered to engage DT for debugging things. OTOH, you may
> add a property to allow debug mux (but it prevent ACPI enabled
> platforms to utilize this).
There needs to be some mechanism through which to list the possible
valid pinctrl states for each pin on the expansion connectors (P1/P2 for
PocketBeagle and P8/P9 for BeagleBones). For these ARM boards, device
tree pinctrl bindings are the only way I can see to do this. I am not
familiar enough with ACPI to understand if this needs to be extended for
boards without device tree.
>
> ...
>
> > Any ideas as to what would trigger the probe() if there was not a match
> > on a compatible like "pinctrl,state-helper"?
> >
> > > Actually not even sure we want to have it as a module.
> >
> > And have just be a part of one of the existing pinctrl files like core.c?
>
> Separate file, but in conjunction with core.c and pinmux and so on.
>
> ...
>
> > > > > Shouldn't it be rather a part of a certain pin control folder:
> > > > > debug/pinctrl/.../mux/...
> > > > > ?
> > > >
> > > > Yes, I think that would make sense, but I was struggling to figure out
> > > > how to do that. pinctrl_init_debugfs() in pinctrl/core.c does create the
> > > > "pinctrl" directory, but I could not figure out how to use this as the
> > > > parent dir when calling debugfs_create_dir() in this driver's probe().
> > > >
> > > > I thought there might be a way in debugfs API to use existing directory
> > > > path as a parent but I couldn't figure anything like that. I would
> > > > appreciate any advice.
> > >
> > > If the option is boolean from the beginning then you just call it from
> > > the corresponding pin control instantiation chain.
> >
> > Sorry, I am not sure I understand what you mean here. What does
> > "option" mean in this context? I don't think there is any value that is
> > boolean invovled. The pinctrl states are strings.
>
> config PINMUX_DEBUG
> bool "..."
> depends on PINMUX
Okay, thanks for califying.
There is already DEBUG_PINCTRL which just adds -DDEBUG compile option.
The existing debugfs logic in pinctrl core and drivers is gated by
CONFIG_DEBUG_FS.
It seems for this new capability to expose pinctrl state in debugfs that
I should use something like:
#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_PINMUX_DEBUG)
Does that seem reasonable?
>
>
>
> >
> > With regards to parent directory, I did discover there is
> > debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
> > subdirectory inside of it. This is the structure now:
> >
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
> > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
> > etc..
>
>
> --
> With Best Regards,
> Andy Shevchenko
thanks,
drew
On Wed, Dec 16, 2020 at 12:42 AM Drew Fustini <[email protected]> wrote:
> On Tue, Dec 15, 2020 at 09:39:18PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 9:36 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <[email protected]> wrote:
> > > > On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > With regards to parent directory, I did discover there is
> > > > debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
> > > > subdirectory inside of it. This is the structure now:
> > > >
> > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
> > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
> > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
> > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
> > > > etc..
> >
> > Missed part to comment.
> >
> > I was talking about
> >
> > /sys/kernel/debug/pinctrl/<$PINCTRL>/mux/<$PIN> (maybe folder, maybe node)
>
> Thanks for the example.
>
> What would the value be "<$PINCTRL>"? The name of the driver?
The name of the device instance. This is already done in the pin control code.
> The "ocp:Px_yy_pinmux" directory name comes from dev_name(dev). Is that
> the name you were referencing in "<$PIN>"?
No, the <$PIN> is an actual pin on this controller. However, I think
we probably don't need this, just supply it as tuple of the parameters
to be set: like
echo $PIN $STATE > .../<$PINCTRL>/mux or alike.
--
With Best Regards,
Andy Shevchenko
On Fri, Dec 18, 2020 at 06:00:49PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 16, 2020 at 12:42 AM Drew Fustini <[email protected]> wrote:
> > On Tue, Dec 15, 2020 at 09:39:18PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 15, 2020 at 9:36 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <[email protected]> wrote:
> > > > > On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> > >
> > > ...
> > >
> > > > > With regards to parent directory, I did discover there is
> > > > > debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
> > > > > subdirectory inside of it. This is the structure now:
> > > > >
> > > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
> > > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
> > > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
> > > > > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
> > > > > etc..
> > >
> > > Missed part to comment.
> > >
> > > I was talking about
> > >
> > > /sys/kernel/debug/pinctrl/<$PINCTRL>/mux/<$PIN> (maybe folder, maybe node)
> >
> > Thanks for the example.
> >
> > What would the value be "<$PINCTRL>"? The name of the driver?
>
> The name of the device instance. This is already done in the pin control code.
Ah, so for the BeagleBone, that would be 44e10800.pinmux-pinctrl-single:
/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
>
> > The "ocp:Px_yy_pinmux" directory name comes from dev_name(dev). Is that
> > the name you were referencing in "<$PIN>"?
>
> No, the <$PIN> is an actual pin on this controller. However, I think
> we probably don't need this, just supply it as tuple of the parameters
> to be set: like
> echo $PIN $STATE > .../<$PINCTRL>/mux or alike.
Do you mean not having a debugfs file for each pin, but instead just using the
existing combined file?
/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-pins
There is one line in there for each pin in the pinctrl-single instance.
For example:
pin 105 (PIN105): ocp:P2_34_pinmux (GPIO UNCLAIMED) function pinmux_P2_34_default_pin group pinmux_P2_34_default_pin
So maybe one solution to changing pinctrl state could to:
echo "105 gpio_pu" > /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-pins
I'll make an attempt at implementing that.
>
> --
> With Best Regards,
> Andy Shevchenko