RE: [PATCHv2] drivers: pinctrl: add a pin_base for sparse gpio-ranges

[Stephen Warren <swarren@nvidia.com>]

Chanho Park wrote at Thursday, October 27, 2011 11:42 PM:
> This patch enables mapping a base offset of gpio ranges with
> a pin offset even if does'nt matched. A "base" of pinctrl_gpio_range
> means a start offset of gpio. However, we cannot convert gpio to pin
> number for sparse gpio ranges just only using a gpio base offset.
> We can convert a gpio to real pin number using a new pin_base
> (which means a base pin offset of requested gpio range).
> If the pin_base is not specified explicitly, the controller sybsystem
> makes to equal with gpio's base offset. Now, a pinctrl subsystem passes
> the pin number to the driver instead a offset.

...
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
...
> -For example: if a user issues pinctrl_gpio_set_foo(50), the pin control
> -subsystem will find that the second range on this pin controller matches,
> -subtract the base 48 and call the
> -pinctrl_driver_gpio_set_foo(pinctrl, range, 2) where the latter function has
> -this signature:
> -
> -int pinctrl_driver_gpio_set_foo(struct pinctrl_dev *pctldev,
> -    struct pinctrl_gpio_range *rangeid,
> -    unsigned offset);
> -
> -Now the driver knows that we want to do some GPIO-specific operation on the
> -second GPIO range handled by "chip b", at offset 2 in that specific range.
> -
> -(If the GPIO subsystem is ever refactored to use a local per-GPIO controller
> -pin space, this mapping will need to be augmented accordingly.)
> -
> -

struct pinmux_ops.gpio_request_enable's documentation states that:

 * @gpio_request_enable: requests and enables GPIO on a certain pin.
 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide

You'll need to update that documentation now that the final parameter is
a pin number, not an offset into the range.

That said, I wonder why there's a need to change this function's parameters;
you could modify pin_request() to perform the calculation, and then not
need to change any of the drivers:

-	if (gpio_range && ops->gpio_request_enable)
-		/* This requests and enables a single GPIO pin */
-		status = ops->gpio_request_enable(pctldev, gpio_range, pin);
+	if (gpio_range && ops->gpio_request_enable)
+		/* This requests and enables a single GPIO pin */
+		status = ops->gpio_request_enable(pctldev, gpio_range,
+								pin - range->pin_base);

...
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index a13354e..4d9fc44 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -261,11 +261,15 @@ int pinctrl_get_device_gpio_range(unsigned gpio,
>   *
>   * This adds a range of GPIOs to be handled by a certain pin controller. Call
>   * this to register handled ranges after registering your pin controller.
> + * If a pin_base offset is not specified explicitly,
> + * it is equal to a gpio base offset.
>   */
>  void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
>  			    struct pinctrl_gpio_range *range)
>  {
>  	mutex_lock(&pctldev->gpio_ranges_lock);
> +	if (!range->pin_base)
> +		range->pin_base = range->base;

This doesn't seem right; what if we really want a range with a pin_base
of zero, yet a non-zero "base". I think we'd be better off just requiring
all GPIO ranges to specify both values.

-- 
nvpublic