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

[Chanho Park <chanho61.park@samsung.com>]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Stephen Warren
> Sent: Saturday, October 29, 2011 1:13 AM
> To: Chanho Park; linus.walleij@linaro.org; Barry.Song@csr.com;
> Rongjun.Ying@csr.com
> Cc: linux-kernel@vger.kernel.org; Kyungmin Park
> Subject: RE: [PATCHv2] drivers: pinctrl: add a pin_base for sparse gpio-
> ranges
> 
> 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.

I'll update it. Thx.

> 
> 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);

A ops->request function requires "pin" number instead "offset".
I saw the pinmux core driver uses "pin" number all over the place
except the gpio_request_enable.
I think we use "pin" number instead "offset" for consistency.

> 
> ...
> > 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.

Oh, I was wrong. I'll remove and assign it explicitly.
Thank you for your review :)

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