Message-ID: <20210109025527.GA2918377@x1>
Date:   Fri, 8 Jan 2021 18:55:27 -0800
From:   Drew Fustini <drew@...gleboard.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Pantelis Antoniou <pantelis.antoniou@...aro.org>,
        Pantelis Antoniou <pantelis.antoniou@...il.com>,
        Jason Kridner <jkridner@...gleboard.org>,
        Robert Nelson <robertcnelson@...gleboard.org>,
        Tony Lindgren <tony@...mide.com>
Subject: Re: [RFC PATCH v2] pinctrl: add helper to expose pinctrl state in
 debugfs

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@...gleboard.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

Powered by blists - more mailing lists