lkml.org 
[lkml]   [2020]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH] pinctrl: add helper to expose pinctrl state in debugfs
    On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
    > On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <drew@beagleboard.org> 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@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?
    >
    > No. I'm talking mostly about FS callbacks where some relatively old
    > new APIs can be used, such as kasprintf().

    Thanks for following up. I'll will take a look at that and update the code.

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

    I first started down the route of implementing this inside of
    pinctrl-single. I found it didn't work because devm_pinctrl_get() would
    fail. I think was because it was happening too early for pinctrl to be
    ready.

    I do think it seems awkward to have to add this to dts and have the
    driver get probed for each entry:

    P1_04_pinmux {
    compatible = "pinctrl,state-helper";
    status = "okay";
    pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pruout", "pruin";
    pinctrl-0 = <&P1_04_default_pin>;
    pinctrl-1 = <&P1_04_gpio_pin>;
    pinctrl-2 = <&P1_04_gpio_pu_pin>;
    pinctrl-3 = <&P1_04_gpio_pd_pin>;
    pinctrl-4 = <&P1_04_gpio_input_pin>;
    pinctrl-5 = <&P1_04_pruout_pin>;
    pinctrl-6 = <&P1_04_pruin_pin>;
    };

    But I am having a hard time figuring out another way of doing it.

    Any ideas as to what would trigger the probe() if there was not a match
    on a compatible like "pinctrl,state-helper"?

    > Actually not even sure we want to have it as a module.

    And have just be a part of one of the existing pinctrl files like core.c?

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


    Sorry, I am not sure I understand what you mean here. What does
    "option" mean in this context? I don't think there is any value that is
    boolean invovled. The pinctrl states are strings.

    With regards to parent directory, I did discover there is
    debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
    subdirectory inside of it. This is the structure now:

    /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
    /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
    /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
    /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
    etc..

    >
    > --
    > With Best Regards,
    > Andy Shevchenko

    Thanks for reviewing,
    Drew

    \
     
     \ /
      Last update: 2020-12-14 22:54    [W:2.400 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site