All of lore.kernel.org
 help / color / mirror / Atom feed
From: Drew Fustini <drew@beagleboard.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
	Pantelis Antoniou <pantelis.antoniou@linaro.org>,
	Pantelis Antoniou <pantelis.antoniou@gmail.com>,
	Jason Kridner <jkridner@beagleboard.org>,
	Robert Nelson <robertcnelson@beagleboard.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Tony Lindgren <tony@atomide.com>
Subject: Re: [RFC PATCH v2] pinctrl: add helper to expose pinctrl state in debugfs
Date: Thu, 24 Dec 2020 14:36:03 -0600	[thread overview]
Message-ID: <20201224203603.GA59600@x1> (raw)
In-Reply-To: <CAHp75Vfwb+f3k2+mAj+jB=XsKFX-hCxx61A_PCmwz6y-YKHMcg@mail.gmail.com>

On Fri, Dec 18, 2020 at 06:01:25PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 18, 2020 at 6:52 AM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > 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.
> 
> No objections here.
> 
> > 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 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!

> 
> > 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/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state
> > default
> > root@beaglebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state
> > root@beaglebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/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.
> 
> Two more concerns:
>  - why is it OF only?

I am open to figuring out a more general solution but I am really only
familiar with Device Tree.  Is there a way to represent the possible
pinctrl states in ACPI?

>  - why has it been separated from pin control per device debug folder?

From the v1 thread, I see what you mean that there could be a combined
state file for each pinctrl device where one would echo '<pin>
<state-name>' such as 'P2_03 pwm'.  I will attempt to implement that.

> 
> 
> > [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
> 
> --
> With Best Regards,
> Andy Shevchenko

Thanks for reviewing,
Drew

  reply	other threads:[~2020-12-24 20:37 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-18  4:51 [RFC PATCH v2] pinctrl: add helper to expose pinctrl state in debugfs Drew Fustini
2020-12-18 16:01 ` Andy Shevchenko
2020-12-24 20:36   ` Drew Fustini [this message]
2021-01-03 20:23     ` Drew Fustini
2021-01-09  1:22     ` Linus Walleij
2021-01-09  2:55       ` Drew Fustini
2021-01-09 21:14         ` Linus Walleij
2021-01-11 10:03           ` Tony Lindgren
2021-01-21  3:19             ` Drew Fustini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201224203603.GA59600@x1 \
    --to=drew@beagleboard.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=jkridner@beagleboard.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pantelis.antoniou@gmail.com \
    --cc=pantelis.antoniou@konsulko.com \
    --cc=pantelis.antoniou@linaro.org \
    --cc=robertcnelson@beagleboard.org \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.