Re: [RFC PATCH] pinctrl: add helper to expose pinctrl state in debugfs

[Drew Fustini]

On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> 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 <drew@xxxxxxxxxxxxxxx> 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