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: <20201211042625.129255-1-drew@beagleboard.org>
In-Reply-To: <20201211042625.129255-1-drew@beagleboard.org>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 11 Dec 2020 23:15:21 +0200
Message-ID: <CAHp75VcAbdrSnb_ag9Rc0tny3Vtqjs1if+ahk7U36V2eaKMpSw@mail.gmail.com>
Subject: Re: [RFC PATCH] 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 <panto@antoniou-consulting.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Tony Lindgren <tony@atomide.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 11, 2020 at 1:54 PM 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.

And it looks like it's still using APIs from 2013.
Needs quite a clean up.

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

Yeah, for debugfs we don't require too much and esp. there is no
requirement to keep backward compatibility thru interface.
I.o.w. it's *not* an ABI.

...

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

Since it's BB specific, it should have file name and compatible string
accordingly.
But I'm wondering, why it requires this kind of thing and can't be
simply always part of the kernel based on configuration option?

> 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/ocp\:P9_14_pinmux/state
> default
> root@beaglebone:~# echo pwm > /sys/kernel/debug/ocp\:P9_14_pinmux/state
> root@beaglebone:~# cat /sys/kernel/debug/ocp\:P9_14_pinmux/state
> pwm

Shouldn't it be rather a part of a certain pin control folder:
debug/pinctrl/.../mux/...
?

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

I will give time for more discussion about concepts and so, because
code (as stated above) is quite old and requires a lot of cleaning up.

-- 
With Best Regards,
Andy Shevchenko