| Message ID | 1359825953-15663-2-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
* 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
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
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
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>
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
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
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 --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;
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(-)