On Thu, Nov 27, 2014 at 11:18 AM, Sascha Hauer [off-list ref] wrote:

On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:

quoted

On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
[off-list ref] wrote:

quoted

quoted

+- mediatek,pins: 2 integers array, represents gpio pinmux number and config
+  setting. The format as following
+
+    node {
+     mediatek,pins = <PIN_NUMBER_PINMUX>;
+                     GENERIC_PINCONFIG;
+    };

As suggested by Sacha, use just "pins" and define the binding as a patch
to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
that is generic for multiplexing, so we get some order here.

I want you however to put pin multiplexing and pin configuration into
different nodes if possible. I don't like combines muxing and config
nodes. If necessary tag the node with something.

Why? I think the properties can live happily together, even when the
parsing functions go to the pinctrl core.

I'm worried about the semantic ambiguity between the pins = <...>;
property on different pin controllers, whether they are based on
function+group or per-pin. It's not even up to me to decide,
this is for the DT binding people.

In this case:

pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
          <MT8135_PIN_101_SCL0__FUNC_SCL0>;

Each element is a 32bit unsigned where the lower and higher
16 bits have different meanings.

In some other pin controller (using generic bindings and
already merged! arch/arm/boot/dts/ste-href-ab8500.dtsi):

gpio39 {
      gpio39_default_mode: gpio39_default {
             default_mux {
                      function = "gpio";
                      groups = "gpio39_a_1";
             };
             default_cfg {
                      pins = "GPIO39_E16";
                      input-enable;
                      bias-pull-down;
             };
     };
};

Can we get away with using the same core parser with this
as well, here the nodes are split and using strings to identify
pins, not 32bit numbers.

I am worried about semantic coexistance...

quoted

quoted

+               i2c0_pins_a: i2c0 at 0 {
+                       pins1 {
+                               mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
+                                               <MT8135_PIN_101_SCL0__FUNC_SCL0>;
+                               bias-disable;
+                       };
+               };

I would split it up.

i2c0_pins_a: i2c0 at 0 {
        pins1 {
                pins = <MT8135_PIN_100_SDA0>;
                function = <MT8135_PIN_100_FUNC_SDA0>;
        };

The reason to put this in a single define was to make writing the device
trees less error prone. When you have two arrays you must correlate the
entries.

I see the upside. I'm just worried about ambiguity when comparing
different device trees to each other, because they should be about
readability and understanding, not confusing...

quoted

One node for the multiplexing, one node for the config. This is the
pattern used by most drivers, so I want to have this structure.

It is also easy to tell one node from the other: if it contains "function"
we know it's a multiplexing node, if it doesn't, it's a config node.

So when merging these together a node is always multiplexing and
configuration. But do we really win anything if both are separated? When
both are separated you still need an array of pins in the nodes to
tell which pins this node is for. If this array also contains the
mux information then this won't hurt. You can still ignore it.

Well we definately have to support the case with split config and
muxing nodes at least. I am very worried that we would get into
ambguities where that is not possible.

Yours,
Linus Walleij