diff mbox

[v8,01/12] gpio: add gpio offset in gpio range cells property

Message ID 1359825953-15663-2-git-send-email-haojian.zhuang@linaro.org
State Superseded
Headers show

Commit Message

Haojian Zhuang Feb. 2, 2013, 5:25 p.m. UTC
Add gpio offset into "gpio-range-cells" property. It's used to support
sparse pinctrl range in gpio chip.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 Documentation/devicetree/bindings/gpio/gpio.txt |    6 +++---
 arch/arm/boot/dts/spear1310.dtsi                |    4 ++--
 arch/arm/boot/dts/spear1340.dtsi                |    4 ++--
 arch/arm/boot/dts/spear310.dtsi                 |    4 ++--
 arch/arm/boot/dts/spear320.dtsi                 |    4 ++--
 drivers/gpio/gpiolib-of.c                       |   15 ++-------------
 6 files changed, 13 insertions(+), 24 deletions(-)

Comments

Tony Lindgren Feb. 5, 2013, 12:23 a.m. UTC | #1
* Haojian Zhuang <haojian.zhuang@linaro.org> [130202 09:29]:
> Add gpio offset into "gpio-range-cells" property. It's used to support
> sparse pinctrl range in gpio chip.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt |    6 +++---
>  arch/arm/boot/dts/spear1310.dtsi                |    4 ++--
>  arch/arm/boot/dts/spear1340.dtsi                |    4 ++--
>  arch/arm/boot/dts/spear310.dtsi                 |    4 ++--
>  arch/arm/boot/dts/spear320.dtsi                 |    4 ++--
>  drivers/gpio/gpiolib-of.c                       |   15 ++-------------
>  6 files changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index a336287..d933af3 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -98,7 +98,7 @@ announce the pinrange to the pin ctrl subsystem. For example,
>  		compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
>  		reg = <0x1460 0x18>;
>  		gpio-controller;
> -		gpio-ranges = <&pinctrl1 20 10>, <&pinctrl2 50 20>;
> +		gpio-ranges = <&pinctrl1 0 20 10>, <&pinctrl2 10 50 20>;
>  
>      }
>  
> @@ -107,8 +107,8 @@ where,
>  
>     Next values specify the base pin and number of pins for the range
>     handled by 'qe_pio_e' gpio. In the given example from base pin 20 to
> -   pin 29 under pinctrl1 and pin 50 to pin 69 under pinctrl2 is handled
> -   by this gpio controller.
> +   pin 29 under pinctrl1 with gpio offset 0 and pin 50 to pin 69 under
> +   pinctrl2 with gpio offset 10 is handled by this gpio controller.
>  
>  The pinctrl node must have "#gpio-range-cells" property to show number of
>  arguments to pass with phandle from gpio controllers node.
> diff --git a/arch/arm/boot/dts/spear1310.dtsi b/arch/arm/boot/dts/spear1310.dtsi
> index 1513c19..122ae94 100644
> --- a/arch/arm/boot/dts/spear1310.dtsi
> +++ b/arch/arm/boot/dts/spear1310.dtsi
> @@ -89,7 +89,7 @@
>  		pinmux: pinmux@e0700000 {
>  			compatible = "st,spear1310-pinmux";
>  			reg = <0xe0700000 0x1000>;
> -			#gpio-range-cells = <2>;
> +			#gpio-range-cells = <3>;
>  		};
>  
>  		apb {

Hmm is this safe to do if there are bootloaders using the old binding?
Maybe we should support both #gpio-range-cells = <2> and <3> bindings
instead?

Regards,

Tony
Haojian Zhuang Feb. 5, 2013, 1:06 a.m. UTC | #2
On 5 February 2013 08:23, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@linaro.org> [130202 09:29]:
>> Add gpio offset into "gpio-range-cells" property. It's used to support
>> sparse pinctrl range in gpio chip.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/gpio/gpio.txt |    6 +++---
>>  arch/arm/boot/dts/spear1310.dtsi                |    4 ++--
>>  arch/arm/boot/dts/spear1340.dtsi                |    4 ++--
>>  arch/arm/boot/dts/spear310.dtsi                 |    4 ++--
>>  arch/arm/boot/dts/spear320.dtsi                 |    4 ++--
>>  drivers/gpio/gpiolib-of.c                       |   15 ++-------------
>>  6 files changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
>> index a336287..d933af3 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
>> @@ -98,7 +98,7 @@ announce the pinrange to the pin ctrl subsystem. For example,
>>               compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
>>               reg = <0x1460 0x18>;
>>               gpio-controller;
>> -             gpio-ranges = <&pinctrl1 20 10>, <&pinctrl2 50 20>;
>> +             gpio-ranges = <&pinctrl1 0 20 10>, <&pinctrl2 10 50 20>;
>>
>>      }
>>
>> @@ -107,8 +107,8 @@ where,
>>
>>     Next values specify the base pin and number of pins for the range
>>     handled by 'qe_pio_e' gpio. In the given example from base pin 20 to
>> -   pin 29 under pinctrl1 and pin 50 to pin 69 under pinctrl2 is handled
>> -   by this gpio controller.
>> +   pin 29 under pinctrl1 with gpio offset 0 and pin 50 to pin 69 under
>> +   pinctrl2 with gpio offset 10 is handled by this gpio controller.
>>
>>  The pinctrl node must have "#gpio-range-cells" property to show number of
>>  arguments to pass with phandle from gpio controllers node.
>> diff --git a/arch/arm/boot/dts/spear1310.dtsi b/arch/arm/boot/dts/spear1310.dtsi
>> index 1513c19..122ae94 100644
>> --- a/arch/arm/boot/dts/spear1310.dtsi
>> +++ b/arch/arm/boot/dts/spear1310.dtsi
>> @@ -89,7 +89,7 @@
>>               pinmux: pinmux@e0700000 {
>>                       compatible = "st,spear1310-pinmux";
>>                       reg = <0xe0700000 0x1000>;
>> -                     #gpio-range-cells = <2>;
>> +                     #gpio-range-cells = <3>;
>>               };
>>
>>               apb {
>
> Hmm is this safe to do if there are bootloaders using the old binding?
> Maybe we should support both #gpio-range-cells = <2> and <3> bindings
> instead?
>
> Regards,
>
> Tony

I think that bootloader aren't using the old binding. I always Cc Shiraz.
If it can't work, he would speak loudly.

And I don't want to add complexity in this function. So I force the number of
parameters is three.

Regards
Haojian
Linus Walleij Feb. 10, 2013, 7:03 p.m. UTC | #3
On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> Add gpio offset into "gpio-range-cells" property. It's used to support
> sparse pinctrl range in gpio chip.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Can't decide on this thing, no reply from Shiraz.

Viresh, what is your opinion?

Yours,
Linus Walleij
Viresh Kumar Feb. 11, 2013, 4:25 a.m. UTC | #4
On Mon, Feb 11, 2013 at 12:33 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Sat, Feb 2, 2013 at 6:25 PM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>
>> Add gpio offset into "gpio-range-cells" property. It's used to support
>> sparse pinctrl range in gpio chip.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>
> Can't decide on this thing, no reply from Shiraz.
>
> Viresh, what is your opinion?

Sorry for missing this thread earlier. Looks fine to me:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
James Hogan April 29, 2013, 4 p.m. UTC | #5
On 02/02/13 17:25, Haojian Zhuang wrote:
> Add gpio offset into "gpio-range-cells" property. It's used to support
> sparse pinctrl range in gpio chip.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

This is an ABI breakage. I've been using the #gpio-range-cells = <2>
since around October. Please can we try and maintain backward
compatibility in future, even if it's only temporary.

Thanks
James
Haojian Zhuang April 29, 2013, 4:49 p.m. UTC | #6
On 30 April 2013 00:00, James Hogan <james.hogan@imgtec.com> wrote:
> On 02/02/13 17:25, Haojian Zhuang wrote:
>> Add gpio offset into "gpio-range-cells" property. It's used to support
>> sparse pinctrl range in gpio chip.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> This is an ABI breakage. I've been using the #gpio-range-cells = <2>
> since around October. Please can we try and maintain backward
> compatibility in future, even if it's only temporary.
>
> Thanks
> James
>

I've updated all code with #gpio-range-cells = <3> in kernel. If you change this
back to <2>, you'll break current pinctrl-single driver. I appended this because
there may be not 1-to-1 mapping between gpio pins & pinmux pins in some
SoC. So the new parameter is used to specify the gpio pin offset.

So it's not temporary.

Regards
Haojian
James Hogan April 29, 2013, 8:16 p.m. UTC | #7
On 29/04/13 17:49, Haojian Zhuang wrote:
> On 30 April 2013 00:00, James Hogan <james.hogan@imgtec.com> wrote:
>> On 02/02/13 17:25, Haojian Zhuang wrote:
>>> Add gpio offset into "gpio-range-cells" property. It's used to support
>>> sparse pinctrl range in gpio chip.
>>>
>>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> This is an ABI breakage. I've been using the #gpio-range-cells = <2>
>> since around October. Please can we try and maintain backward
>> compatibility in future, even if it's only temporary.
>>
>> Thanks
>> James
>>
> 
> I've updated all code with #gpio-range-cells = <3> in kernel. If you change this
> back to <2>, you'll break current pinctrl-single driver. I appended this because
> there may be not 1-to-1 mapping between gpio pins & pinmux pins in some
> SoC. So the new parameter is used to specify the gpio pin offset.

Yes, I agree having 3 cells is useful, and I wasn't suggesting reverting
your patch, but it's an ABI now so the change should really be done in a
backwards compatible way so that device tree files that still have
#gpio-range-cells = <2> continue to work.

At the moment in -next, if you use an old device tree then the gpio
ranges are muddled up, with npins set apparently to a random (in my case
very large) number, I'm guessing this is just uninitialised data at the
end of the array. Even if you didn't have to maintain backwards
compatibility, it should at least check the number of cells matches what
it expects before reading all 3 entries.

> 
> So it's not temporary.

Sorry, what I meant was "even if maintaining the backwards compatibility
(supporting both <2> and <3>) is only temporary".

Cheers
James
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index a336287..d933af3 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -98,7 +98,7 @@  announce the pinrange to the pin ctrl subsystem. For example,
 		compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
 		reg = <0x1460 0x18>;
 		gpio-controller;
-		gpio-ranges = <&pinctrl1 20 10>, <&pinctrl2 50 20>;
+		gpio-ranges = <&pinctrl1 0 20 10>, <&pinctrl2 10 50 20>;
 
     }
 
@@ -107,8 +107,8 @@  where,
 
    Next values specify the base pin and number of pins for the range
    handled by 'qe_pio_e' gpio. In the given example from base pin 20 to
-   pin 29 under pinctrl1 and pin 50 to pin 69 under pinctrl2 is handled
-   by this gpio controller.
+   pin 29 under pinctrl1 with gpio offset 0 and pin 50 to pin 69 under
+   pinctrl2 with gpio offset 10 is handled by this gpio controller.
 
 The pinctrl node must have "#gpio-range-cells" property to show number of
 arguments to pass with phandle from gpio controllers node.
diff --git a/arch/arm/boot/dts/spear1310.dtsi b/arch/arm/boot/dts/spear1310.dtsi
index 1513c19..122ae94 100644
--- a/arch/arm/boot/dts/spear1310.dtsi
+++ b/arch/arm/boot/dts/spear1310.dtsi
@@ -89,7 +89,7 @@ 
 		pinmux: pinmux@e0700000 {
 			compatible = "st,spear1310-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <2>;
+			#gpio-range-cells = <3>;
 		};
 
 		apb {
@@ -212,7 +212,7 @@ 
 				interrupt-controller;
 				gpio-controller;
 				#gpio-cells = <2>;
-				gpio-ranges = <&pinmux 0 246>;
+				gpio-ranges = <&pinmux 0 0 246>;
 				status = "disabled";
 
 				st-plgpio,ngpio = <246>;
diff --git a/arch/arm/boot/dts/spear1340.dtsi b/arch/arm/boot/dts/spear1340.dtsi
index b2d41b7..7ec1eb8 100644
--- a/arch/arm/boot/dts/spear1340.dtsi
+++ b/arch/arm/boot/dts/spear1340.dtsi
@@ -63,7 +63,7 @@ 
 		pinmux: pinmux@e0700000 {
 			compatible = "st,spear1340-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <2>;
+			#gpio-range-cells = <3>;
 		};
 
 		pwm: pwm@e0180000 {
@@ -146,7 +146,7 @@ 
 				interrupt-controller;
 				gpio-controller;
 				#gpio-cells = <2>;
-				gpio-ranges = <&pinmux 0 252>;
+				gpio-ranges = <&pinmux 0 0 252>;
 				status = "disabled";
 
 				st-plgpio,ngpio = <250>;
diff --git a/arch/arm/boot/dts/spear310.dtsi b/arch/arm/boot/dts/spear310.dtsi
index ab45b8c..9537208 100644
--- a/arch/arm/boot/dts/spear310.dtsi
+++ b/arch/arm/boot/dts/spear310.dtsi
@@ -25,7 +25,7 @@ 
 		pinmux: pinmux@b4000000 {
 			compatible = "st,spear310-pinmux";
 			reg = <0xb4000000 0x1000>;
-			#gpio-range-cells = <2>;
+			#gpio-range-cells = <3>;
 		};
 
 		fsmc: flash@44000000 {
@@ -102,7 +102,7 @@ 
 				interrupt-controller;
 				gpio-controller;
 				#gpio-cells = <2>;
-				gpio-ranges = <&pinmux 0 102>;
+				gpio-ranges = <&pinmux 0 0 102>;
 				status = "disabled";
 
 				st-plgpio,ngpio = <102>;
diff --git a/arch/arm/boot/dts/spear320.dtsi b/arch/arm/boot/dts/spear320.dtsi
index caa5520..ffea342 100644
--- a/arch/arm/boot/dts/spear320.dtsi
+++ b/arch/arm/boot/dts/spear320.dtsi
@@ -24,7 +24,7 @@ 
 		pinmux: pinmux@b3000000 {
 			compatible = "st,spear320-pinmux";
 			reg = <0xb3000000 0x1000>;
-			#gpio-range-cells = <2>;
+			#gpio-range-cells = <3>;
 		};
 
 		clcd@90000000 {
@@ -130,7 +130,7 @@ 
 				interrupt-controller;
 				gpio-controller;
 				#gpio-cells = <2>;
-				gpio-ranges = <&pinmux 0 102>;
+				gpio-ranges = <&pinmux 0 0 102>;
 				status = "disabled";
 
 				st-plgpio,ngpio = <102>;
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 25b1dbe..380f84e 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -238,22 +238,11 @@  static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 		if (!pctldev)
 			break;
 
-		/*
-		 * This assumes that the n GPIO pins are consecutive in the
-		 * GPIO number space, and that the pins are also consecutive
-		 * in their local number space. Currently it is not possible
-		 * to add different ranges for one and the same GPIO chip,
-		 * as the code assumes that we have one consecutive range
-		 * on both, mapping 1-to-1.
-		 *
-		 * TODO: make the OF bindings handle multiple sparse ranges
-		 * on the same GPIO chip.
-		 */
 		ret = gpiochip_add_pin_range(chip,
 					     pinctrl_dev_get_devname(pctldev),
-					     0, /* offset in gpiochip */
 					     pinspec.args[0],
-					     pinspec.args[1]);
+					     pinspec.args[1],
+					     pinspec.args[2]);
 
 		if (ret)
 			break;