Date:   Tue, 15 Dec 2020 14:37:47 -0800
From:   Drew Fustini <drew@...gleboard.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Tony Lindgren <tony@...mide.com>
Subject: Re: [RFC PATCH] pinctrl: add helper to expose pinctrl state in
 debugfs

On Tue, Dec 15, 2020 at 09:36:33PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <drew@...gleboard.org> 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@...gleboard.org> 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@...gleboard.org> 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

Powered by blists - more mailing lists