| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210103202319.GA973266@x1>
Date: Sun, 3 Jan 2021 12:23:19 -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>,
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>,
Linus Walleij <linus.walleij@...aro.org>,
Tony Lindgren <tony@...mide.com>
Subject: Re: [RFC PATCH v2] pinctrl: add helper to expose pinctrl state in
debugfs
On Thu, Dec 24, 2020 at 02:36:03PM -0600, Drew Fustini wrote:
> 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@...gleboard.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@...glebone:~# cat /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state
> > > default
> > > root@...glebone:~# echo pwm > /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P9_14_pinmux/state
> > > root@...glebone:~# 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.
I have tried creating a single state file:
/sys/kernel/debug/pinctrl/pinctrl_state
where one can write into it:
<device-name> <pinctrl-state-name>
such as:
ocp:P9_14_pinmux gpio
However, I can not figure out a way for this to work.
I create the pinctrl_state file in pinctrl_state_helper_init() and store
the dentry in a global variable.
pinctrl_state_helper_probe() still runs for each Px_0y_pinmux device
tree entry with compatible "pinctrl,state-helper" but there is no
per-device file created.
The problem comes in pinctrl_state_write(). I use this to extract the
device_name and state_name:
ret = sscanf(buf, "%s %s", device_name, state_name);
This does work okay but I don't know what to do with the device_name
string, such as "ocp:P9_14_pinmux". Previously, the device was saved
in the private info:
sfile = file->private_data;
priv = sfile->private;
p = devm_pinctrl_get(priv->dev); // use device_name instead?
state = pinctrl_lookup_state(p, state_name);
But I don't know how to look up a device based on its name.
Any suggestions as to how to handle that?
Thanks and happy new year!
Drew
>
> >
> >
> > > [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
Powered by blists - more mailing lists