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:

quoted

On Fri, Dec 18, 2020 at 6:52 AM Drew Fustini [off-list ref] wrote:

quoted

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.

quoted

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!

quoted

quoted

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?

quoted

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

quoted

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

quoted

quoted

[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