[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren [off-list ref] wrote:

On 10/22/2012 10:08 AM, Haojian Zhuang wrote:

quoted

Add comments with pinconf & gpio range in the document of
pinctrl-single.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   52 ++++++++++++++++++++
 arch/arm/boot/dts/pxa910.dtsi                      |    1 -
 2 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 2c81e45..6da2f13 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

@@ -17,6 +17,36 @@ Optional properties:
 - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
   more than one pin

+- pinctrl-single,gpio-ranges : gpio range list
+
+- pinctrl-single,gpio : array with gpio range start, size & register
+  offset
+
+- pinctrl-single,gpio-func : gpio function value in the pinmux register

Some more explanation is needed here; some questions/comments:

1) Looking at the example, pinctrl-single,gpio-ranges is a property
within the main pinctrl node, whereas pinctrl-single,gpio and
pinctrl-single,gpio-func are properties within some other node. There's
no explanation of this in the binding description itself, only in the
example. Related to this, the documentation for
pinctrl-single,gpio-ranges doesn't say what it's a list of; it needs to
say that it's a list of phandles.

I'll add more comments.

2) pinctrl-single,gpio is listed as optional. Presumably it's not; every
GPIO range node must have this property?

Yes, they must be included in GPIO range node. But if GPIO feature
isn't supported in the pinctrl device, pinctrl-single,gpio is still optional.
I'll add more comments on this.

3) Why is pinctrl-single,gpio-func optional? Presumably you always need
to program the pinmux HW to select the GPIO function. Yet, the driver
code in an earlier patch seems to deliberately do nothing if this
property is missing. Shouldn't the DT parsing return an error instead?

pinctrl-single,gpio-func is optional for above reason.

4) I'm a little confused re: the data model. Is the idea that if
pinctrl-single,gpio-ranges is specified, then the node describes a
combined pin controller and GPIO HW? Are the pin IDs of the pin
controller expected to match the pin IDs of the GPIO HW?

GPIO function is enabled by setting proper field in mux field of pinmux
register.

I'm left wondering exactly which numbering space the values in
pinctrl-single,gpio are; do they describe the pin controller IDs that
this GPIO range describes, or do they describe the GPIO IDs that this
range describes and attempt to map them back to pin controller IDs?
Similarly, I'm not sure why there's a register offset here rather than
say a pin controller pin ID number. Shouldn't the property be a list of
<pin-controller-pin-ID GPIO-controller-GPIO-ID number-of-GPIOs>

Whatever it's pin ID or offset of pinmux register, they're nearly same.
We can calculate offset from pin ID, vice visa. If pin ID is more acceptable
by you and Tony, I can change it later.

quoted

+- pinctrl-single,power-source-mask : mask of setting power source in
+  the pinmux register
+
+- pinctrl-single,power-source : value of setting power source field
+  in the pinmux register
+
+- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
+  register
+
+- pinctrl-single,bias-disable : value of disabling bias in the pinmux
+  register
+
+- pinctrl-single,bias-pull-down : value of setting bias pull down in
+  the pinmux register
+
+- pinctrl-single,bias-pull-up : value of setting bias pull up in the
+  pinmux register
+
+- pinctrl-single,bias : value of setting bias in the pinmux register
+
+- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
+  in the pinmux register

I suppose it's OK that a generic pin controller binding would use the
generic pin configuration config options. I'm still not convinced that
the semantics of generic pin control make sense. Maybe if they're just
arbitrary names for SoC-specific things it's fine though.

Do these patches expose /all/ generic pin configuration options? It
doesn't seem worth exposing only some of them and ignoring others.

I believe general pinconf can't support all cases in different silicons.
And we still have some common features that could be covered in general
pinconf. So we need a structure to support both pinconf & specific pinconf.

quoted

+/* third controller instance for pins in gpio domain */
+pmx_gpio: pinmux at d401e000 {
+     compatible = "pinctrl-single";
+     reg = <0xd401e000 0x0330>;
+     #address-cells = <1>;
+     #size-cells = <0>;

#gpio-cells would be needed here for a GPIO controller.

quoted

diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi

quoted

-                             pinctrl-single,gpio-mask = <7>;

I assume that's a mistake; the line shouldn't be removed in this
documentation patch?

Yes, this part should be included in the previous patch. I'll fix it.