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

[Andy Shevchenko]

On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <drew@xxxxxxxxxxxxxxx> 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 <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:

...

> > > > 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).

...

> 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



>
> 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