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

[Drew Fustini <drew@xxxxxxxxxxxxxxx>]

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

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