Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201218045134.4158709-1-drew@beagleboard.org>
In-Reply-To: <20201218045134.4158709-1-drew@beagleboard.org>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 18 Dec 2020 18:01:25 +0200
Message-ID: <CAHp75Vfwb+f3k2+mAj+jB=XsKFX-hCxx61A_PCmwz6y-YKHMcg@mail.gmail.com>
Subject: Re: [RFC PATCH v2] pinctrl: add helper to expose pinctrl state in debugfs
To:     Drew Fustini <drew@beagleboard.org>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

> 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?
 - why has it been separated from pin control per device debug folder?


> [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