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

[Drew Fustini <drew@beagleboard.org>]

On Fri, Dec 11, 2020 at 11:15:21PM +0200, Andy Shevchenko wrote:
> 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.

Thanks for taking a look at my RFC and responding. It is good to know
that it is using out-dated APIs. Would you be able to elaborate?

It interacts with pinctrl core through devm_pinctrl_get(),
pinctrl_lookup_state() and pinctrl_select_state(). Is there newer way of
doing that?

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

At first, I was thinking about this as a beaglebone specific solution
and had bone in the driver name and compatible string. But then I 
realized it could used in other situations where it is beneficial to
to read and select a pinctrl state through debugfs.

I'm happy to rebrand the naming as beaglebone if that would be more
acceptable.

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

Do you mean not having a new CONFIG option for this driver and just have
it be enabled by CONFIG_PINCTRL?

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

Yes, I think that would make sense, but I was struggling to figure out
how to do that. pinctrl_init_debugfs() in pinctrl/core.c does create the
"pinctrl" directory, but I could not figure out how to use this as the
parent dir when calling debugfs_create_dir() in this driver's probe().

I thought there might be a way in debugfs API to use existing directory
path as a parent but I couldn't figure anything like that. I would
appreciate any advice.

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

Thanks for taking the time to comment. I'll look at other drivers to see
the ways in which this drivers is out-dated.

-Drew