Re: [RFC] Allow GPIO ranges based on pinctl pin groups

[Linus Walleij]

On Wed, Jun 12, 2013 at 6:44 PM, Christian Ruppert
<christian.ruppert@xxxxxxxxxx> 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 <christian.ruppert@xxxxxxxxxx>

Overall this approach looks nice.

There are details that need fixing though:

> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
(...)
> @@ -112,3 +112,39 @@ where,
>
>  The pinctrl node must have "#gpio-range-cells" property to show number of
>  arguments to pass with phandle from gpio controllers node.
> +
> +In addition, gpio ranges can be mapped to pin groups of a given pin
> +controller (see Documentation/pinctrl.txt):

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.

> +++ b/drivers/gpio/gpiolib-of.c
(...)
> +       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.

(...)
> +++ b/drivers/gpio/gpiolib.c
> @@ -1318,6 +1318,54 @@ EXPORT_SYMBOL_GPL(gpiochip_find);
>  #ifdef CONFIG_PINCTRL
>
>  /**
> + * gpiochip_add_pingroup_range() - add a range for GPIO <-> pin mapping
> + * @chip: the gpiochip to add the range for
> + * @pinctrl: the dev_name() of the pin controller to map to
> + * @gpio_offset: the start offset in the current gpio_chip number space
> + * @pin_group: name of the pin group inside the pin controller
> + */
> +int gpiochip_add_pingroup_range(struct gpio_chip *chip,
> +                       struct pinctrl_dev *pctldev,
> +                       unsigned int gpio_offset, const char *pin_group)
> +{
> +       struct gpio_pin_range *pin_range;
> +       int ret;
> +
> +       pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
> +       if (!pin_range) {
> +               pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
> +                               chip->label);
> +               return -ENOMEM;
> +       }
> +
> +       /* Use local offset as range ID */
> +       pin_range->range.id = gpio_offset;
> +       pin_range->range.gc = chip;
> +       pin_range->range.name = chip->label;
> +       pin_range->range.base = chip->base + gpio_offset;
> +       pin_range->range.pin_base = 0;
> +       ret = pinctrl_get_group_pins(pctldev, pin_group,
> +                                       &pin_range->range.pins,
> +                                       &pin_range->range.npins);
> +       if (ret < 0) {
> +               pr_err("%s: GPIO chip: could not create pin range %s\n",
> +                      chip->label, pin_group);
> +       }
> +       pin_range->pctldev = pctldev;
> +       pinctrl_add_gpio_range(pctldev, &pin_range->range);
> +
> +       pr_debug("GPIO chip %s: created GPIO range %d->%d ==> %s PINGRP %s\n",
> +                chip->label, gpio_offset,
> +                gpio_offset + pin_range->range.npins - 1,
> +                pinctrl_dev_get_devname(pctldev), pin_group);
> +
> +       list_add_tail(&pin_range->node, &chip->pin_ranges);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);

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.

(...)
> +++ b/drivers/pinctrl/core.c
> @@ -279,6 +279,15 @@ static int pinctrl_register_pins(struct pinctrl_dev *pctldev,
>         return 0;
>  }
>
> +static inline int gpio2pin(struct pinctrl_gpio_range *range, unsigned int gpio)
> +{
> +       unsigned int offset = gpio - range->base;
> +       if (range->pins)
> +               return range->pins[offset];
> +       else
> +               return range->pin_base + offset;
> +}

Clever function. Document it! :-)

>  /**
>   * pinctrl_match_gpio_range() - check if a certain GPIO pin is in range
>   * @pctldev: pin controller device to check
> @@ -444,8 +453,14 @@ pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
>         /* Loop over the ranges */
>         list_for_each_entry(range, &pctldev->gpio_ranges, node) {
>                 /* Check if we're in the valid range */
> -               if (pin >= range->pin_base &&
> -                   pin < range->pin_base + range->npins) {
> +               if (range->pins) {
> +                       int a;
> +                       for (a = 0; a < range->npins; a++) {
> +                               if (range->pins[a] == pin)
> +                                       return range;
> +                       }
> +               } else if (pin >= range->pin_base &&
> +                          pin < range->pin_base + range->npins) {
>                         mutex_unlock(&pctldev->mutex);
>                         return range;
>                 }

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/