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

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

[Chanho Park]

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.

For example, let's assume below two gpio ranges in the system.

static struct pinctrl_gpio_range gpio_range_a = {
    .name = "chip a",
    .id = 0,
    .base = 32,
    .npins = 16,
    .gc = &chip_a;
};

static struct pinctrl_gpio_range gpio_range_b = {
    .name = "chip b",
    .id = 0,
    .base = 48,
    .pin_base = 64,
    .npins = 8,
    .gc = &chip_b;
};

{
    struct pinctrl_dev *pctl;
    ...
    pinctrl_add_gpio_range(pctl, &gpio_range_a);
    pinctrl_add_gpio_range(pctl, &gpio_range_b);
}

We can calucalate a exact pin ranges even if doesn't matched with gpio ranges.

chip a:
    gpio-range : [32 .. 47]
    pin-range  : [32 .. 47]
chip b:
    gpio-range : [48 .. 55]
    pin-range  : [64 .. 71]

Changelog v1->v2:
 - Update documentation about gpio parts.
 - A pin_base makes to equal with gpio's base if not specified explicitly.
 - Modify a offset calculation of sirf's driver.
 - Modify a parameter name of gpio_request_enable.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/pinctrl.txt       |   41 ++++++++++++++++----------------------
 drivers/pinctrl/core.c          |    4 +++
 drivers/pinctrl/pinmux-sirf.c   |    4 +-
 drivers/pinctrl/pinmux.c        |    4 +-
 include/linux/pinctrl/pinctrl.h |    3 ++
 include/linux/pinctrl/pinmux.h  |    2 +-
 6 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 328adb7..5733a6c 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -273,6 +273,7 @@ static struct pinctrl_gpio_range gpio_range_a = {
 	.name = "chip b",
 	.id = 0,
 	.base = 48,
+	.pin_base = 64,
 	.npins = 8,
 	.gc = &chip_b;
 };
@@ -286,11 +287,19 @@ static struct pinctrl_gpio_range gpio_range_a = {
 }
 
 So this complex system has one pin controller handling two different
-GPIO chips. Chip a has 16 pins and chip b has 8 pins. They are mapped in
-the global GPIO pin space at:
-
-chip a: [32 .. 47]
-chip b: [48 .. 55]
+GPIO chips. Chip a has 16 pins and chip b has 8 pins. The chip b has
+pin_base(which means a start pin number of the gpio_range), the pin range
+of the chip b starts from 64. We can convert a gpio number to actual pin
+number using this "pin_base". If "pin_base" is not specified explicitly like
+chip a, the pin_base of the chip is equal to a gpio base.
+They are mapped in the global GPIO pin space at:
+
+chip a:
+ - gpio range : [32 .. 47]
+ - pin range  : [32 .. 47]
+chip b:
+ - gpio range : [48 .. 55]
+ - pin range  : [64 .. 71]
 
 When GPIO-specific functions in the pin control subsystem are called, these
 ranges will be used to look up the apropriate pin controller by inspecting
@@ -300,28 +309,12 @@ will be called on that specific pin controller.
 
 For all functionalities dealing with pin biasing, pin muxing etc, the pin
 controller subsystem will subtract the range's .base offset from the passed
-in gpio pin number, and pass that on to the pin control driver, so the driver
-will get an offset into its handled number range. Further it is also passed
+in gpio number, and add the ranges's .pin_base offset to retrive a pin number.
+After that, the subsystem passes it on to the pin control driver, so the driver
+will get an pin number into its handled number range. Further it is also passed
 the range ID value, so that the pin controller knows which range it should
 deal with.
 
-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.)
-
-
 PINMUX interfaces
 =================
 
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;
 	list_add(&range->node, &pctldev->gpio_ranges);
 	mutex_unlock(&pctldev->gpio_ranges_lock);
 }
diff --git a/drivers/pinctrl/pinmux-sirf.c b/drivers/pinctrl/pinmux-sirf.c
index ba73523..3dade7a 100644
--- a/drivers/pinctrl/pinmux-sirf.c
+++ b/drivers/pinctrl/pinmux-sirf.c
@@ -1056,7 +1056,7 @@ static int sirfsoc_pinmux_get_groups(struct pinctrl_dev *pctldev, unsigned selec
 }
 
 static int sirfsoc_pinmux_request_gpio(struct pinctrl_dev *pmxdev,
-	struct pinctrl_gpio_range *range, unsigned offset)
+	struct pinctrl_gpio_range *range, unsigned pin)
 {
 	struct sirfsoc_pmx *spmx;
 
@@ -1067,7 +1067,7 @@ static int sirfsoc_pinmux_request_gpio(struct pinctrl_dev *pmxdev,
 	spmx = pinctrl_dev_get_drvdata(pmxdev);
 
 	muxval = readl(spmx->gpio_virtbase + SIRFSOC_GPIO_PAD_EN(group));
-	muxval = muxval | (1 << offset);
+	muxval = muxval | (1 << (pin - range->pin_base));
 	writel(muxval, spmx->gpio_virtbase + SIRFSOC_GPIO_PAD_EN(group));
 
 	return 0;
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 5c4b782..d1384da 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -225,7 +225,7 @@ int pinmux_request_gpio(unsigned gpio)
 		return -EINVAL;
 
 	/* Convert to the pin controllers number space */
-	pin = gpio - range->base;
+	pin = gpio - range->base + range->pin_base;
 
 	/* Conjure some name stating what chip and pin this is taken by */
 	snprintf(gpiostr, 15, "%s:%d", range->name, gpio);
@@ -258,7 +258,7 @@ void pinmux_free_gpio(unsigned gpio)
 		return;
 
 	/* Convert to the pin controllers number space */
-	pin = gpio - range->base;
+	pin = gpio - range->base + range->pin_base;
 
 	pin_free(pctldev, pin, true);
 }
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 90dd28b..7afd099 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -127,6 +127,8 @@ struct pinctrl_pin_desc {
  * @name: a name for the chip in this range
  * @id: an ID number for the chip in this range
  * @base: base offset of the GPIO range
+ * @pin_base: base pin number of the GPIO range
+ *            (If not specified, it's equal to @base)
  * @npins: number of pins in the GPIO range, including the base number
  * @gc: an optional pointer to a gpio_chip
  */
@@ -135,6 +137,7 @@ struct pinctrl_gpio_range {
 	const char *name;
 	unsigned int id;
 	unsigned int base;
+	unsigned int pin_base;
 	unsigned int npins;
 	struct gpio_chip *gc;
 };
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index 3c430e7..9dd5c90 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -72,7 +72,7 @@ struct pinmux_ops {
 			 unsigned group_selector);
 	int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
 				    struct pinctrl_gpio_range *range,
-				    unsigned offset);
+				    unsigned pin);
 };
 
 /* External interface to pinmux */
-- 
1.7.5.4

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

[Stephen Warren]

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

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

[Chanho Park]

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

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

[Thomas Abraham]

Hi Chanho Park,

On 28 October 2011 01:41, Chanho Park <chanho61.park@samsung.com> wrote:
> 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.

Just wanted to check with you if you have started the implementation
of pinctrl driver for Samsung platforms or do plan to start on it?

Thanks,
Thomas.

[...]

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

[Chanho Park]

> Just wanted to check with you if you have started the implementation
> of pinctrl driver for Samsung platforms or do plan to start on it?
> 
> Thanks,
> Thomas.
> 
> [...]

Hi, Thomas
I have no plan to implement it for Samsung platforms(s3c/s5p/exynos).
Thanks.

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

[Thomas Abraham]

Hi Chanho Park,

On 7 November 2011 09:34, Chanho Park <chanho61.park@samsung.com> wrote:
>> Just wanted to check with you if you have started the implementation
>> of pinctrl driver for Samsung platforms or do plan to start on it?
>>
>> Thanks,
>> Thomas.
>>
>> [...]
>
> Hi, Thomas
> I have no plan to implement it for Samsung platforms(s3c/s5p/exynos).
> Thanks.

Thanks for the confirmation.

Regards,
Thomas.

>
>

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

[Linus Walleij]

On Wed, Nov 2, 2011 at 9:56 AM, Chanho Park <chanho61.park@samsung.com> wrote:

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

Chanho, do you have a v3 ready? I'd really like to apply this patch...

Yours,
Linus Walleij

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

[Chanho Park]

Hi, Linus.
Our smtp server blocked my email. :)
I'll submit v3 patch soon. (Maybe tomorrow)
Thank you.

Best Regards,
Chanho Park

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Linus Walleij
> Sent: Thursday, November 10, 2011 5:08 PM
> To: Chanho Park
> Cc: Stephen Warren; Barry.Song@csr.com; Rongjun.Ying@csr.com; linux-
> kernel@vger.kernel.org; Kyungmin Park
> Subject: Re: [PATCHv2] drivers: pinctrl: add a pin_base for sparse
> gpio-ranges
> 
> On Wed, Nov 2, 2011 at 9:56 AM, Chanho Park <chanho61.park@samsung.com>
> wrote:
> 
> >> 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.
> 
> Chanho, do you have a v3 ready? I'd really like to apply this patch...
> 
> Yours,
> Linus Walleij
> --
> 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/