On Wed, Jun 12, 2013 at 6:44 PM, Christian Ruppert
[off-list ref] wrote:

This patch allows the definition of GPIO ranges based on pin groups in
addition to the traditional linear pin ranges. GPIO ranges based on pin
groups have the following advantages over traditional pin ranges:
. Previously, pins associated to a given functionality were defined
  inside the pin controller (e.g. a pin controller can define a group
  spi0_pins defining which pins are used by SPI port 0). This mechanism
  did not apply to GPIO controllers, however, which had to define GPIO
  ranges based on pin numbers otherwise confined to the pin controller.
  With the possibility to use pin groups for pin ranges, the pins
  associated to any functionality, including GPIO, can now be entirely
  defined inside the pin controller. Everything that needs to be known
  to the outside world is the name of the pin group.
. Non-consecutive pin ranges and arbitrary pin ordering is now possible
  in GPIO ranges.
. Linux internal pin numbers now no longer leak out of the kernel, e.g.
  to device tree. If the pinctrl driver author chooses to, GPIO range
  management can now entirely be based on symbolic names of pin groups.

Signed-off-by: Christian Ruppert <redacted>

Overall this approach looks nice.

There are details that need fixing though:

(...)

Do not reference Linux particulars in bindings. The idea is to
reuse these bindings with other operating systems as well.

+
+       gpio_pio_g: gpio-controller@1480 {
+               #gpio-cells = <2>;
+               compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
+               reg = <0x1480 0x18>;
+               gpio-controller;
+               gpio-pingrps = <&pinctrl1 0>, <&pinctrl2 3>;
+               gpio-pingrp-names = "pctl1_gpio_g", "pctl2_gpio_g";
+       }

End with semicolon?

+where,
+   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.

There is something weird with spacing above.

+   Next values specify the base GPIO offset of the pin range with respect to
+   the GPIO controller's base. The number of pins in the range is the number
+   of pins in the pin group.
+
+   gpio-pingrp-names defines the name of each pingroup of the respective pin
+   controller.

That seems like a good idea.

+The pinctrl node msut have a "#gpio-pingrp-cells" property set to one to

must

+define the number of arguments to pass with the phandle.
+
+Both methods can be combined in the same GPIO controller, e.g.
+
+       gpio_pio_i: gpio-controller@14B0 {
+               #gpio-cells = <2>;
+               compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
+               reg = <0x1480 0x18>;
+               gpio-controller;
+               gpio-ranges = <&pinctrl1 0 20 10>;
+               gpio-pingrps = <&pinctrl2 10>;
+               gpio-pingrp-names = "gpio_g_pins";
+       }

Semicolon after that closing brace.

(...)

+       index = 0;
+       of_property_for_each_string(np, "gpio-pingrp-names", prop, name) {
+               ret = of_parse_phandle_with_args(np, "gpio-pingrps",
+                                                       "#gpio-pingrp-cells",
+                                                       index, &pinspec);
+               if (ret < 0)
+                       break;
+
+               index ++;
+
+               pctldev = of_pinctrl_get(pinspec.np);
+
+               gpiochip_add_pingroup_range(chip, pctldev,
+                                       pinspec.args[0], name);
+       }
 }

This part looks fine.

(...)

This thing has *waaaay* to much pinctrl stuff in it. You need to find
a better way to partition this between gpiolib and drivers/pinctrl/core.c.
For example: get rid of the function pinctrl_get_group_pins(),
why should GPIOlib know about that stuff?

Pass the groupname to the pinctrl core and let it handle this
business.

+/**
  * gpiochip_add_pin_range() - add a range for GPIO <-> pin mapping
  * @chip: the gpiochip to add the range for
  * @pinctrl_name: the dev_name() of the pin controller to map to

That seems like an unrelated fix I'd like a separate patch for.

(...)

Clever function. Document it! :-)

This seems like it could be a separate refactoring patch, i.e. a
patch refactoring the ranges to be an array of disparate pins
instead of a linear, consecutive range.

 /**
+ * pinctrl_get_group_pins() - returns a pin group
+ * @pctldev: the pin controller handling the group
+ * @pin_group: the pin group to look up
+ * @pins: returns a pointer to an array of pin numbers in the group
+ * @npins: returns the number of pins in the group
+ */
+int pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+                       const char *pin_group,
+                       unsigned const **pins, unsigned * const npins)
+{
+       const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
+       unsigned group_selector;
+
+       group_selector = pinctrl_get_group_selector(pctldev, pin_group);
+       if (group_selector < 0)
+               return group_selector;
+
+       return pctlops->get_group_pins(pctldev, group_selector, pins, npins);
+}
+EXPORT_SYMBOL_GPL(pinctrl_get_group_pins);

As mentioned I don't like these pinctrl internals to be exposed
to the whole wide world. You need to find another partitioning.

        /* Convert to the pin controllers number space */
-       pin = gpio - range->base + range->pin_base;
+       pin = gpio2pin(range, gpio);

Nice, can I have a patch adding this gpio2pin (hm maybe
you can rename that gpio_to_pin() I don't mind...)
and refactoring the code? It's a nice refactoring in its own
right.

Yours,
Linus Walleij