On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <d...@beagleboard.org> wrote:
> On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> > On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <d...@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 <d...@beagleboard.org>
> > > > wrote:
...
> > > > 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.
I'm not a DT expert and I have no clue why you need all this. To me it
looks over engineered to engage DT for debugging things. OTOH, you may
add a property to allow debug mux (but it prevent ACPI enabled
platforms to utilize this).
...
> 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?
Separate file, but in conjunction with core.c and pinmux and so on.
...
> > > > 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.
config PINMUX_DEBUG
bool "..."
depends on PINMUX
>
> 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
