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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> 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@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.
> >
> > 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?

No. I'm talking mostly about FS callbacks where some relatively old
new APIs can be used, such as kasprintf().

...

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

See below.

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

No, configuration option stays, but no compatible strings no nothing
like that. Just probed always when loaded.
Actually not even sure we want to have it as a module.

...

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

If the option is boolean from the beginning then you just call it from
the corresponding pin control instantiation chain.

-- 
With Best Regards,
Andy Shevchenko