diff mbox series

ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2

Message ID 20200914104352.2165818-1-drew@beagleboard.org
State New
Headers show
Series ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2 | expand

Commit Message

Drew Fustini Sept. 14, 2020, 10:43 a.m. UTC
Document the values in pinctrl-single,pins when #pinctrl-cells = <2>

Fixes: 27c90e5e48d0 ("ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2")
Reported-by: Trent Piepho <tpiepho@gmail.com>
Link: https://lore.kernel.org/linux-omap/3139716.CMS8C0sQ7x@zen.local/
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
 .../bindings/pinctrl/pinctrl-single.txt       | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Trent Piepho Sept. 17, 2020, 9:03 a.m. UTC | #1
On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote:
>
> +
> +When #pinctrl-cells = 2, then setting a pin for a device could be done with:
> +
> +       pinctrl-single,pins = <0xdc 0x30 0x07>;
> +
> +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value.
> +See the device example and static board pins example below for more information.

Pin configuration and mux mode don't mean anything in pinctrl-single.
On another machine, mux mode might not be programmed this way or even
exist.  Or the location of bits would probably be different, and this
would seem to imply the 0x07 would get shifted to the correct location
for where the pin mux setting was on that machine's pinctrl registers.

It seems like it would be better to explain the values are ORed together.

What is the purpose of this change anyway?  It seems like in the end
it just does what it did before.  The data is now split into two cells
in the device tree, but why?
Drew Fustini Sept. 17, 2020, 9:20 a.m. UTC | #2
On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote:
> On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > +
> > +When #pinctrl-cells = 2, then setting a pin for a device could be done with:
> > +
> > +       pinctrl-single,pins = <0xdc 0x30 0x07>;
> > +
> > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value.
> > +See the device example and static board pins example below for more information.
> 
> Pin configuration and mux mode don't mean anything in pinctrl-single.
> On another machine, mux mode might not be programmed this way or even
> exist.  Or the location of bits would probably be different, and this
> would seem to imply the 0x07 would get shifted to the correct location
> for where the pin mux setting was on that machine's pinctrl registers.
> 
> It seems like it would be better to explain the values are ORed together.

I descirbed it as seoerate values as I did not want to prescribe what
the pcs driver would do with those values.  But, yes, it is a just an OR
operation, so I could change the language to reflect tat.

> What is the purpose of this change anyway?  It seems like in the end
> it just does what it did before.  The data is now split into two cells
> in the device tree, but why?

These changes were a result of desire to seperate pinconf and pinmux.
Tony raised the idea in a thread at the end of May [1].

Tony wrote:
> Only slightly related, but we should really eventually move omaps to use
> #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately
> from the mux mode. We already treat them separately with the new
> AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to
> use updated #pinctrl-cells. But I think pinctrl-single might need some
> changes before we can do that.


thanks,
drew

[1] https://lore.kernel.org/linux-omap/20200527165122.GL37466@atomide.com/
Trent Piepho Sept. 17, 2020, 10 a.m. UTC | #3
On Thu, Sep 17, 2020 at 2:20 AM Drew Fustini <drew@beagleboard.org> wrote:
>
> On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote:
> > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote:
> > >
> > > +
> > > +When #pinctrl-cells = 2, then setting a pin for a device could be done with:
> > > +
> > > +       pinctrl-single,pins = <0xdc 0x30 0x07>;
> > > +
> > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value.
> > > +See the device example and static board pins example below for more information.
> >
> > Pin configuration and mux mode don't mean anything in pinctrl-single.
> > On another machine, mux mode might not be programmed this way or even
> > exist.  Or the location of bits would probably be different, and this
> > would seem to imply the 0x07 would get shifted to the correct location
> > for where the pin mux setting was on that machine's pinctrl registers.
> >
> > It seems like it would be better to explain the values are ORed together.
>
> I descirbed it as seoerate values as I did not want to prescribe what
> the pcs driver would do with those values.  But, yes, it is a just an OR
> operation, so I could change the language to reflect tat.

If you don't say what the pinctrl-single driver does with the values,
how would anyone know how to use it?

> > What is the purpose of this change anyway?  It seems like in the end
> > it just does what it did before.  The data is now split into two cells
> > in the device tree, but why?
>
> These changes were a result of desire to seperate pinconf and pinmux.
> Tony raised the idea in a thread at the end of May [1].
>
> Tony wrote:
> > Only slightly related, but we should really eventually move omaps to use
> > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately
> > from the mux mode. We already treat them separately with the new
> > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to
> > use updated #pinctrl-cells. But I think pinctrl-single might need some
> > changes before we can do that.

I still don't see what the goal is here.  Support generic pinconf?

Also note that while AM33XX_PADCONF() is changed, there is an in tree
board that doesn't use it, so it's broken now.  I found this change
when it broke my out of tree board, due to the dtsi change not being
reflected in my board's pinctrl values.
Drew Fustini Sept. 17, 2020, 10:39 a.m. UTC | #4
On Thu, Sep 17, 2020 at 03:00:36AM -0700, Trent Piepho wrote:
> On Thu, Sep 17, 2020 at 2:20 AM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > On Thu, Sep 17, 2020 at 02:03:46AM -0700, Trent Piepho wrote:
> > > On Mon, Sep 14, 2020 at 3:44 AM Drew Fustini <drew@beagleboard.org> wrote:
> > > >
> > > > +
> > > > +When #pinctrl-cells = 2, then setting a pin for a device could be done with:
> > > > +
> > > > +       pinctrl-single,pins = <0xdc 0x30 0x07>;
> > > > +
> > > > +Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value.
> > > > +See the device example and static board pins example below for more information.
> > >
> > > Pin configuration and mux mode don't mean anything in pinctrl-single.
> > > On another machine, mux mode might not be programmed this way or even
> > > exist.  Or the location of bits would probably be different, and this
> > > would seem to imply the 0x07 would get shifted to the correct location
> > > for where the pin mux setting was on that machine's pinctrl registers.
> > >
> > > It seems like it would be better to explain the values are ORed together.
> >
> > I descirbed it as seoerate values as I did not want to prescribe what
> > the pcs driver would do with those values.  But, yes, it is a just an OR
> > operation, so I could change the language to reflect tat.
> 
> If you don't say what the pinctrl-single driver does with the values,
> how would anyone know how to use it?
> 
> > > What is the purpose of this change anyway?  It seems like in the end
> > > it just does what it did before.  The data is now split into two cells
> > > in the device tree, but why?
> >
> > These changes were a result of desire to seperate pinconf and pinmux.
> > Tony raised the idea in a thread at the end of May [1].
> >
> > Tony wrote:
> > > Only slightly related, but we should really eventually move omaps to use
> > > #pinctrl-cells = <2> (or 3) instead of 1, and pass the pinconf seprately
> > > from the mux mode. We already treat them separately with the new
> > > AM33XX_PADCONF macro, so we'd only have to change one SoC at a time to
> > > use updated #pinctrl-cells. But I think pinctrl-single might need some
> > > changes before we can do that.
> 
> I still don't see what the goal is here.  Support generic pinconf?

My interest is came out of my desire to turn on generic pinconf for AM3358
and I had to fix a bug that was breaking compatible "pinconf,single":
f46fe79ff1b6 ("pinctrl-single: fix pcs_parse_pinconf() return value")

> Also note that while AM33XX_PADCONF() is changed, there is an in tree
> board that doesn't use it, so it's broken now.  I found this change
> when it broke my out of tree board, due to the dtsi change not being
> reflected in my board's pinctrl values.

Thanks, that is a good point that arch/arm/boot/dts/am335x-guardian.dts
needs to be converted from AM33XX_IOPAD to AM33XX_PADCONF.  I'll submit
a patch for that.

Regarding AM33XX_PADCONF() restructuring, the change to have seperate
arguments for direction and mux in AM33XX_PADCONF() predates my
invovlement, so I've CC'd Christina Quast.

    commit f1ff9be7652b716c7eea67c9ca795027d911f148
    Author: Christina Quast <cquast@hanoverdisplays.com>
    Date:   Mon Apr 8 10:01:51 2019 -0700

    ARM: dts: am33xx: Added AM33XX_PADCONF macro
    
    AM33XX_PADCONF takes three instead of two parameters, to make
    future changes to #pinctrl-cells easier.
    
    For old boards which are not mainlined, we left the AM33XX_IOPAD
    macro.
    
    Signed-off-by: Christina Quast <cquast@hanoverdisplays.com>
    Reviewed-by: Rob Herring <robh@kernel.org>
    Signed-off-by: Tony Lindgren <tony@atomide.com>

Hopefully, Tony can also chime in.

-Drew
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index ba428d345a56..ef560afdd52e 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -96,16 +96,22 @@  pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as
 specified in the pinctrl-bindings.txt document in this directory.
 
 The pin configuration nodes for pinctrl-single are specified as pinctrl
-register offset and value pairs using pinctrl-single,pins. Only the bits
-specified in pinctrl-single,function-mask are updated. For example, setting
-a pin for a device could be done with:
+register offset and values using pinctrl-single,pins. Only the bits specified
+in pinctrl-single,function-mask are updated.
+
+When #pinctrl-cells = 1, then setting a pin for a device could be done with:
 
 	pinctrl-single,pins = <0xdc 0x118>;
 
-Where 0xdc is the offset from the pinctrl register base address for the
-device pinctrl register, and 0x118 contains the desired value of the
-pinctrl register. See the device example and static board pins example
-below for more information.
+Where 0xdc is the offset from the pinctrl register base address for the device
+pinctrl register, and 0x118 contains the desired value of the pinctrl register.
+
+When #pinctrl-cells = 2, then setting a pin for a device could be done with:
+
+	pinctrl-single,pins = <0xdc 0x30 0x07>;
+
+Where 0x30 is the pin configuration value and 0x07 is the pin mux mode value.
+See the device example and static board pins example below for more information.
 
 In case when one register changes more than one pin's mux the
 pinctrl-single,bits need to be used which takes three parameters: