[GIT PULL] SPEAr pinctrl updates for v-3.5

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Viresh Kumar]

Hi Arnd,Olof,

This have few more dependencies:
- ARM: 7376/1: clkdev: Implement managed clk_get()
- pinctrl branch from Linus

The following changes since commit 880702fb50ec63a832aaeafcb251295f3c6ce722:

  SPEAr: Update defconfigs (2012-05-09 14:53:53 +0530)

are available in the git repository at:

  git://git.stlinux.com/spear/linux-2.6.git for-v3.5-spear-pinctrl-update

for you to fetch changes up to a8f048037fb467b39ac1aeb15361a2c8366d8a2d:

  SPEAr: Add plgpio node in device tree dtsi files (2012-05-09 14:55:55 +0530)

----------------------------------------------------------------
Viresh Kumar (6):
      gpiolib: Add of_get_gpio_chip_by_phandle() helper
      pinctrl: SPEAr: Don't set all non muxreg bits on pinctrl_disable
      pinctrl: SPEAr1310: Fix pin numbers for clcd_high_res
      pinctrl: SPEAr: Add plgpio driver
      pinctrl: SPEAr: Add gpio ranges support
      SPEAr: Add plgpio node in device tree dtsi files

 arch/arm/boot/dts/spear1310-evb.dts       |    4 +
 arch/arm/boot/dts/spear1310.dtsi          |   22 +
 arch/arm/boot/dts/spear1340-evb.dts       |    4 +
 arch/arm/boot/dts/spear1340.dtsi          |   21 +
 arch/arm/boot/dts/spear310-evb.dts        |    4 +
 arch/arm/boot/dts/spear310.dtsi           |   15 +
 arch/arm/boot/dts/spear320-evb.dts        |    4 +
 arch/arm/boot/dts/spear320.dtsi           |   16 +
 drivers/of/gpio.c                         |   32 ++
 drivers/pinctrl/spear/Kconfig             |   11 +
 drivers/pinctrl/spear/Makefile            |    1 +
 drivers/pinctrl/spear/pinctrl-plgpio.c    |  738 +++++++++++++++++++++++++++++
 drivers/pinctrl/spear/pinctrl-spear.c     |  110 ++++-
 drivers/pinctrl/spear/pinctrl-spear.h     |   26 +
 drivers/pinctrl/spear/pinctrl-spear1310.c |  182 +++++++-
 drivers/pinctrl/spear/pinctrl-spear1340.c |    8 +
 drivers/pinctrl/spear/pinctrl-spear300.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear310.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear320.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear3xx.c  |   29 ++
 include/linux/of_gpio.h                   |    9 +-
 21 files changed, 1228 insertions(+), 14 deletions(-)
 create mode 100644 drivers/pinctrl/spear/pinctrl-plgpio.c

[GIT PULL] SPEAr pinctrl updates for v-3.5

[viresh kumar]

On Wed, May 9, 2012 at 4:45 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> Hi Arnd,Olof,
>
> This have few more dependencies:
> - ARM: 7376/1: clkdev: Implement managed clk_get()
> - pinctrl branch from Linus

Please pull this instead:


The following changes since commit 9f05a80564c35c397c5ce0f0b1106a420c3f5bdc:

  SPEAr: Update defconfigs (2012-05-10 16:15:08 +0530)

are available in the git repository at:

  git://git.stlinux.com/spear/linux-2.6.git for-v3.5-spear-pinctrl-update

for you to fetch changes up to 560a7694d63e2461912d3b99987a3c89ea4b56e2:

  SPEAr: Add plgpio node in device tree dtsi files (2012-05-10 16:15:09 +0530)

----------------------------------------------------------------
Viresh Kumar (6):
      gpiolib: Add of_get_gpio_chip_by_phandle() helper
      pinctrl: SPEAr: Don't set all non muxreg bits on pinctrl_disable
      pinctrl: SPEAr1310: Fix pin numbers for clcd_high_res
      pinctrl: SPEAr: Add plgpio driver
      pinctrl: SPEAr: Add gpio ranges support
      SPEAr: Add plgpio node in device tree dtsi files

 arch/arm/boot/dts/spear1310-evb.dts       |    4 +
 arch/arm/boot/dts/spear1310.dtsi          |   22 +
 arch/arm/boot/dts/spear1340-evb.dts       |    4 +
 arch/arm/boot/dts/spear1340.dtsi          |   21 +
 arch/arm/boot/dts/spear310-evb.dts        |    4 +
 arch/arm/boot/dts/spear310.dtsi           |   15 +
 arch/arm/boot/dts/spear320-evb.dts        |    4 +
 arch/arm/boot/dts/spear320.dtsi           |   16 +
 drivers/of/gpio.c                         |   32 ++
 drivers/pinctrl/spear/Kconfig             |   11 +
 drivers/pinctrl/spear/Makefile            |    1 +
 drivers/pinctrl/spear/pinctrl-plgpio.c    |  738 +++++++++++++++++++++++++++++
 drivers/pinctrl/spear/pinctrl-spear.c     |  110 ++++-
 drivers/pinctrl/spear/pinctrl-spear.h     |   26 +
 drivers/pinctrl/spear/pinctrl-spear1310.c |  182 +++++++-
 drivers/pinctrl/spear/pinctrl-spear1340.c |    8 +
 drivers/pinctrl/spear/pinctrl-spear300.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear310.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear320.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear3xx.c  |   29 ++
 include/linux/of_gpio.h                   |    9 +-
 21 files changed, 1228 insertions(+), 14 deletions(-)
 create mode 100644 drivers/pinctrl/spear/pinctrl-plgpio.c

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Arnd Bergmann]

On Thursday 10 May 2012, viresh kumar wrote:
> The following changes since commit 9f05a80564c35c397c5ce0f0b1106a420c3f5bdc:
> 
>   SPEAr: Update defconfigs (2012-05-10 16:15:08 +0530)
> 
> are available in the git repository at:
> 
>   git://git.stlinux.com/spear/linux-2.6.git for-v3.5-spear-pinctrl-update
> 
> for you to fetch changes up to 560a7694d63e2461912d3b99987a3c89ea4b56e2:
> 
>   SPEAr: Add plgpio node in device tree dtsi files (2012-05-10 16:15:09 +0530)
> 
> ----------------------------------------------------------------
> Viresh Kumar (6):
>       gpiolib: Add of_get_gpio_chip_by_phandle() helper
>       pinctrl: SPEAr: Don't set all non muxreg bits on pinctrl_disable
>       pinctrl: SPEAr1310: Fix pin numbers for clcd_high_res
>       pinctrl: SPEAr: Add plgpio driver
>       pinctrl: SPEAr: Add gpio ranges support
>       SPEAr: Add plgpio node in device tree dtsi files

Hi Viresh,

I thought the first patch "gpiolib: Add of_get_gpio_chip_by_phandle() helper"
was not actually accepted. What was the outcome of that discussion?

	Arnd

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Linus Walleij]

On Sat, May 12, 2012 at 11:42 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 10 May 2012, viresh kumar wrote:

>> Viresh Kumar (6):
>> ? ? ? gpiolib: Add of_get_gpio_chip_by_phandle() helper
>
> I thought the first patch "gpiolib: Add of_get_gpio_chip_by_phandle() helper"
> was not actually accepted. What was the outcome of that discussion?

None. I am ambivalent and a bit lost so I prefer if Grant advice on this one...

Yours,
Linus Walleij

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Viresh Kumar]

On 5/13/2012 5:24 AM, Linus Walleij wrote:
>>> >> Viresh Kumar (6):
>>> >>       gpiolib: Add of_get_gpio_chip_by_phandle() helper
>> >
>> > I thought the first patch "gpiolib: Add of_get_gpio_chip_by_phandle() helper"
>> > was not actually accepted. What was the outcome of that discussion?
> None. I am ambivalent and a bit lost so I prefer if Grant advice on this one...

Grant,

Can we have your view on this patch please? We need to get this patchset in v3.5

-- 
viresh

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Viresh Kumar]

On 5/13/2012 5:24 AM, Linus Walleij wrote:
>>> >> Viresh Kumar (6):
>>> >>       gpiolib: Add of_get_gpio_chip_by_phandle() helper
>> >
>> > I thought the first patch "gpiolib: Add of_get_gpio_chip_by_phandle() helper"
>> > was not actually accepted. What was the outcome of that discussion?
> None. I am ambivalent and a bit lost so I prefer if Grant advice on this one...

Hi Linus/Arnd,

Tomorrow is my last day in ST, so just trying to get this fixed before that. :)
As Grant hasn't replied till now, what about updating this routine to 

of_get_gpio_chip_base_by_phandle()

that will return base of gpio chip instead of its pointer. This would solve the
concerns shared by Linus earlier.

Will this change be good enough to get this patchset pushed?

-- 
viresh

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Linus Walleij]

On Thu, May 17, 2012 at 6:26 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> Tomorrow is my last day in ST, so just trying to get this fixed before that. :)
> As Grant hasn't replied till now, what about updating this routine to
>
> of_get_gpio_chip_base_by_phandle()
>
> that will return base of gpio chip instead of its pointer. This would solve the
> concerns shared by Linus earlier.
>
> Will this change be good enough to get this patchset pushed?

I don't know my way around devicetree well enough to answer that...
anything prefixed of_* is better decided by Grant. I suspect he's
thinking about it.

Yours,
Linus Walleij

[GIT PULL] SPEAr pinctrl updates for v-3.5

[viresh kumar]

On Sun, May 20, 2012 at 9:45 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, May 17, 2012 at 6:26 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
>
>> Tomorrow is my last day in ST, so just trying to get this fixed before that. :)
>> As Grant hasn't replied till now, what about updating this routine to
>>
>> of_get_gpio_chip_base_by_phandle()
>>
>> that will return base of gpio chip instead of its pointer. This would solve the
>> concerns shared by Linus earlier.
>>
>> Will this change be good enough to get this patchset pushed?
>
> I don't know my way around devicetree well enough to answer that...
> anything prefixed of_* is better decided by Grant. I suspect he's
> thinking about it.

@Grant: Can we have your comments here please. Or any other better
solution to the problem
we are facing. I do hope, my last suggestion is not that bad.

--
viresh

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Linus Walleij]

On Tue, Jun 19, 2012 at 4:11 PM, viresh kumar <viresh.linux@gmail.com> wrote:

> @Grant: Can we have your comments here please. Or any other better
> solution to the problem
> we are facing. I do hope, my last suggestion is not that bad.

I did hunt Grant down in Hong Kong (this make me sound like some
secret agent...) and we discussed this with Dong Aisheng.

The outcome was basically this work item from Linaro:
https://blueprints.launchpad.net/linux-linaro/+spec/pinctrl-gpiorange-makeover

So the idea is that GPIO drivers should register their pin ranges
instead of the other way around (pinctrl drivers register them, as it
is done today), and that we need to move the mapping to be
GPIO-driver local, which in turn mandates stashing the
struct gpio_chip into the GPIO range mapping.

Not a simple refactoring but probably necessary (all current users need
to be switched over).

Interested? ;-)

Yours,
Linus Walleij

[GIT PULL] SPEAr pinctrl updates for v-3.5

[shiraz hashim]

Hi Linus, Grant,

On Wed, Jun 20, 2012 at 1:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jun 19, 2012 at 4:11 PM, viresh kumar <viresh.linux@gmail.com> wrote:
>
>> @Grant: Can we have your comments here please. Or any other better
>> solution to the problem
>> we are facing. I do hope, my last suggestion is not that bad.
>
> I did hunt Grant down in Hong Kong (this make me sound like some
> secret agent...) and we discussed this with Dong Aisheng.
>
> The outcome was basically this work item from Linaro:
> https://blueprints.launchpad.net/linux-linaro/+spec/pinctrl-gpiorange-makeover
>
> So the idea is that GPIO drivers should register their pin ranges
> instead of the other way around (pinctrl drivers register them, as it
> is done today), and that we need to move the mapping to be
> GPIO-driver local, which in turn mandates stashing the
> struct gpio_chip into the GPIO range mapping.
>
> Not a simple refactoring but probably necessary (all current users need
> to be switched over).
>
> Interested? ;-)

I tried to prepare a basic patch based upon this, can you please
see if the approach is fine.

8<--------------------------------------------------------------------------

    gpiolib: provide provision for gpios to register pin range

    pinctrl subsystem needs gpio chip base to prepare set of gpio pin ranges
    which a given pinctrl driver can handle. This is important to handle
    pinctrl gpio request calls in order to program a given pin properly for
    gpio operation.

    As gpio base is allocated dynamically while gpiochip registration,
    presently there exists no clean way to pass this information to the
    pinctrl subsystem.

    After few discussions from [1], it was concluded that may be gpio
    reporting the pin range it supports is a better way than pinctrl
    subsystem directly registering it.

    [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/166668

    Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
b/Documentation/devicetree/bindings/gpio/gpio.txt
index 4e16ba4..70d9e2e 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -75,4 +75,38 @@ Example of two SOC GPIO banks defined as
gpio-controller nodes:
 		gpio-controller;
 	};

+2.1) gpio-controller and pinctrl subsystem
+------------------------------------------

+gpio-controller on a SOC might be tightly coupled with the pinctrl
+subsystem, in the sense that the pins can be used by other functions
+together with optional gpio feature.
+
+While the pin allocation is totally managed by the pin ctrl subsystem,
+gpio (under gpiolib) is still maintained by gpio drivers. It may happen
+that different pin ranges in a SoC is managed by different gpio drivers.
+
+This makes it logical to let gpio drivers announce their pin ranges to
+the pin ctrl subsystem and call 'pinctrl_request_gpio' in order to
+request the corresponding pin before any gpio usage.
+
+For this, the gpio controller can use a pinctrl phandle and pins to
+announce the pinrange to the pin ctrl subsystem. For example,
+
+	qe_pio_e: gpio-controller at 1460 {
+		#gpio-cells = <2>;
+		compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
+		reg = <0x1460 0x18>;
+		gpio-controller;
+                                 pins = <&pinctrl1 20 10>,
+                                <&pinctrl2 50 20>;
+
+    }
+
+where,
+   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.
+
+   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.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d18068a..cadbbff 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_gpio.h>
+#include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>

 /* Private data structure for of_gpiochip_is_match */
@@ -213,6 +214,64 @@ err0:
 }
 EXPORT_SYMBOL(of_mm_gpiochip_add);

+#ifdef CONFIG_PINCTRL
+void of_gpiochip_add_pin_range(struct gpio_chip *chip)
+{
+	struct device_node *np = chip->of_node;
+	struct gpio_pin_range *pin_range;
+	struct of_phandle_args pinspec;
+	int index = 0;
+
+	if (!chip->of_node)
+		return;
+
+	INIT_LIST_HEAD(&chip->pin_ranges);
+
+	do {
+		int ret;
+
+		ret = of_parse_phandle_with_args(np, "gpio-ranges", "#gpio-range-cells",
+				index, &pinspec);
+
+		if (ret)
+			break;
+
+		pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
+		if (!pin_range) {
+			pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
+					chip->label);
+			break;
+		}
+
+		pin_range->range.name = chip->label;
+		pin_range->range.base = chip->base;
+		pin_range->range.pin_base = pinspec.args[0];
+		pin_range->range.npins = pinspec.args[1];
+		pin_range->pctldev = of_pinctrl_add_gpio_range(pinspec.np,
+				&pin_range->range);
+
+		list_add_tail(&pin_range->node, &chip->pin_ranges);
+
+	} while (index++);
+
+}
+
+void of_gpiochip_remove_pin_range(struct gpio_chip *chip)
+{
+	struct gpio_pin_range *pin_range, *tmp;
+
+	list_for_each_entry_safe(pin_range, tmp, &chip->pin_ranges, node) {
+		pinctrl_remove_gpio_range(pin_range->pctldev,
+				&pin_range->range);
+		list_del(&pin_range->node);
+		kfree(pin_range);
+	}
+}
+#else
+void of_gpiochip_add_pin_range(struct gpio_chip *chip) {}
+void of_gpiochip_remove_pin_range(struct gpio_chip *chip) {}
+#endif
+
 void of_gpiochip_add(struct gpio_chip *chip)
 {
 	if ((!chip->of_node) && (chip->dev))
@@ -226,11 +285,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
 		chip->of_xlate = of_gpio_simple_xlate;
 	}

+	of_gpiochip_add_pin_range(chip);
 	of_node_get(chip->of_node);
 }

 void of_gpiochip_remove(struct gpio_chip *chip)
 {
+	of_gpiochip_remove_pin_range(chip);
+
 	if (chip->of_node)
 		of_node_put(chip->of_node);
 }
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..6728ec7 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -106,6 +106,19 @@ static struct pinctrl_dev
*find_pinctrl_by_of_node(struct device_node *np)
 	return NULL;
 }

+struct pinctrl_dev *of_pinctrl_add_gpio_range(struct device_node *np,
+		struct pinctrl_gpio_range *range)
+{
+	struct pinctrl_dev *pctldev;
+
+	pctldev = find_pinctrl_by_of_node(np);
+	if (!pctldev)
+		return NULL;
+
+	pinctrl_add_gpio_range(pctldev, range);
+	return pctldev;
+}
+
 static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 				struct device_node *np_config)
 {
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 365ea09..f7e0648 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -5,6 +5,7 @@
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>

 #ifdef CONFIG_GPIOLIB

@@ -47,6 +48,21 @@ struct seq_file;
 struct module;
 struct device_node;

+#ifdef CONFIG_PINCTRL
+/**
+ * struct gpio_pin_range - pin range controlled by a gpio chip
+ * @head: list for maintaining set of pin ranges, used internally
+ * @pctldev: pinctrl device which handles corresponding pins
+ * @range: actual range of pins controlled by a gpio controller
+ */
+
+struct gpio_pin_range {
+	struct list_head node;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_gpio_range range;
+};
+#endif
+
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
@@ -132,6 +148,14 @@ struct gpio_chip {
 	int (*of_xlate)(struct gpio_chip *gc,
 		        const struct of_phandle_args *gpiospec, u32 *flags);
 #endif
+#ifdef CONFIG_PINCTRL
+	/* If CONFIG_PINCTRL is enabled, then gpio controllers can
+	 * optionally describe the actual pin range which they serve in
+	 * an SoC. This information would be used by pinctrl subsystem
+	 * to configure corresponding pins for gpio usage.
+	 */
+	struct list_head pin_ranges;
+#endif
 };

 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..4222b83 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -133,6 +133,12 @@ extern void pinctrl_add_gpio_range(struct
pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
+#ifdef CONFIG_OF
+extern struct pinctrl_dev *of_pinctrl_add_gpio_range(struct device_node *np,
+		struct pinctrl_gpio_range *range);
+
+#endif
+
 extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 #else

-------------------------------------------------------------------------->8

-- 
regards
Shiraz Hashim

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Linus Walleij]

On Sat, Sep 1, 2012 at 1:22 PM, shiraz hashim
<shiraz.linux.kernel@gmail.com> wrote:
> Hi Linus, Grant,
> On Wed, Jun 20, 2012 at 1:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Jun 19, 2012 at 4:11 PM, viresh kumar <viresh.linux@gmail.com> wrote:

>> The outcome was basically this work item from Linaro:
>> https://blueprints.launchpad.net/linux-linaro/+spec/pinctrl-gpiorange-makeover
>>
>> So the idea is that GPIO drivers should register their pin ranges
>> instead of the other way around (pinctrl drivers register them, as it
>> is done today), and that we need to move the mapping to be
>> GPIO-driver local, which in turn mandates stashing the
>> struct gpio_chip into the GPIO range mapping.
>>
>> Not a simple refactoring but probably necessary (all current users need
>> to be switched over).
>>
>> Interested? ;-)
>
> I tried to prepare a basic patch based upon this, can you please
> see if the approach is fine.

Hey :-)

Thanks for working on this!!

>     gpiolib: provide provision for gpios to register pin range
>
>     pinctrl subsystem needs gpio chip base to prepare set of gpio pin ranges
>     which a given pinctrl driver can handle. This is important to handle
>     pinctrl gpio request calls in order to program a given pin properly for
>     gpio operation.
>
>     As gpio base is allocated dynamically while gpiochip registration,
>     presently there exists no clean way to pass this information to the
>     pinctrl subsystem.
>
>     After few discussions from [1], it was concluded that may be gpio
>     reporting the pin range it supports is a better way than pinctrl
>     subsystem directly registering it.
>
>     [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/166668
>
>     Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>

(Some spelling/grammar errors above but I can fix that, no big issue.)

> +2.1) gpio-controller and pinctrl subsystem
> +------------------------------------------
>
> +gpio-controller on a SOC might be tightly coupled with the pinctrl
> +subsystem, in the sense that the pins can be used by other functions
> +together with optional gpio feature.
> +
> +While the pin allocation is totally managed by the pin ctrl subsystem,
> +gpio (under gpiolib) is still maintained by gpio drivers. It may happen
> +that different pin ranges in a SoC is managed by different gpio drivers.
> +
> +This makes it logical to let gpio drivers announce their pin ranges to
> +the pin ctrl subsystem and call 'pinctrl_request_gpio' in order to
> +request the corresponding pin before any gpio usage.
> +
> +For this, the gpio controller can use a pinctrl phandle and pins to
> +announce the pinrange to the pin ctrl subsystem. For example,
> +
> +       qe_pio_e: gpio-controller at 1460 {
> +               #gpio-cells = <2>;
> +               compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> +               reg = <0x1460 0x18>;
> +               gpio-controller;
> +                                 pins = <&pinctrl1 20 10>,
> +                                <&pinctrl2 50 20>;
> +
> +    }

This needs to go into the binding document in
Documentation/devicetree/bindings/gpio/gpio.txt as well.

> +
> +where,
> +   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.
> +
> +   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.

The above implies that only devcie tree is used for this. That is not
the case: other architectures like Blackfin, MIPS and x86 shall also use
the pinctrl subsystem, and they may not have device tree. Maybe they
have ACPI, maybe BIOS or SFI (Simple Firmware Interface).

So the basic problem with the patch is that you cannot hard-code it
to use *only* device tree, it must also accept ranges registered
directly, not using of_*.

Basically this comes down to creating two functions more
to add pin ranges from GPIO chips, see below comments.

Also, you should put a big "DEPRECATED" mark on the old gpio
range concept, and refer to the new way of doing things.

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index d18068a..cadbbff 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_gpio.h>
> +#include <linux/pinctrl/pinctrl.h>

This is a good idea, patching gpiolib so everyone can use this
easily.

(...)
> +#ifdef CONFIG_PINCTRL
> +void of_gpiochip_add_pin_range(struct gpio_chip *chip)
> +{
> +       struct device_node *np = chip->of_node;
> +       struct gpio_pin_range *pin_range;
> +       struct of_phandle_args pinspec;
> +       int index = 0;
> +
> +       if (!chip->of_node)
> +               return;
> +
> +       INIT_LIST_HEAD(&chip->pin_ranges);
> +
> +       do {
> +               int ret;
> +
> +               ret = of_parse_phandle_with_args(np, "gpio-ranges", "#gpio-range-cells",
> +                               index, &pinspec);
> +
> +               if (ret)
> +                       break;
> +
> +               pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
> +               if (!pin_range) {
> +                       pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
> +                                       chip->label);
> +                       break;
> +               }
> +
> +               pin_range->range.name = chip->label;
> +               pin_range->range.base = chip->base;
> +               pin_range->range.pin_base = pinspec.args[0];
> +               pin_range->range.npins = pinspec.args[1];
> +               pin_range->pctldev = of_pinctrl_add_gpio_range(pinspec.np,
> +                               &pin_range->range);
> +
> +               list_add_tail(&pin_range->node, &chip->pin_ranges);
> +
> +       } while (index++);
> +
> +}
> +
> +void of_gpiochip_remove_pin_range(struct gpio_chip *chip)
> +{
> +       struct gpio_pin_range *pin_range, *tmp;
> +
> +       list_for_each_entry_safe(pin_range, tmp, &chip->pin_ranges, node) {
> +               pinctrl_remove_gpio_range(pin_range->pctldev,
> +                               &pin_range->range);
> +               list_del(&pin_range->node);
> +               kfree(pin_range);
> +       }
> +}
> +#else
> +void of_gpiochip_add_pin_range(struct gpio_chip *chip) {}
> +void of_gpiochip_remove_pin_range(struct gpio_chip *chip) {}
> +#endif
> +
>  void of_gpiochip_add(struct gpio_chip *chip)
>  {
>         if ((!chip->of_node) && (chip->dev))
> @@ -226,11 +285,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
>                 chip->of_xlate = of_gpio_simple_xlate;
>         }
>
> +       of_gpiochip_add_pin_range(chip);
>         of_node_get(chip->of_node);
>  }
>
>  void of_gpiochip_remove(struct gpio_chip *chip)
>  {
> +       of_gpiochip_remove_pin_range(chip);
> +
>         if (chip->of_node)
>                 of_node_put(chip->of_node);
>  }

This part looks good.

But I want two similar function named:

gpiochip_add_pin_range();
gpiochip_remove_pin_range();

that can be used for platforms that doesn't support DT.

For example I'd like to convert over some of my existing
drivers that do not have DT support to do this thing instead
of registering ranges from the pin controller...

(...)
> +++ b/drivers/pinctrl/devicetree.c
> @@ -106,6 +106,19 @@ static struct pinctrl_dev
> *find_pinctrl_by_of_node(struct device_node *np)
>         return NULL;
>  }
>
> +struct pinctrl_dev *of_pinctrl_add_gpio_range(struct device_node *np,
> +               struct pinctrl_gpio_range *range)
> +{
> +       struct pinctrl_dev *pctldev;
> +
> +       pctldev = find_pinctrl_by_of_node(np);
> +       if (!pctldev)
> +               return NULL;
> +
> +       pinctrl_add_gpio_range(pctldev, range);
> +       return pctldev;
> +}
> +
>  static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
>                                 struct device_node *np_config)
>  {
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 365ea09..f7e0648 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -5,6 +5,7 @@
>  #include <linux/types.h>
>  #include <linux/errno.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/pinctrl.h>
>
>  #ifdef CONFIG_GPIOLIB
>
> @@ -47,6 +48,21 @@ struct seq_file;
>  struct module;
>  struct device_node;
>
> +#ifdef CONFIG_PINCTRL

There's no need to #ifdef this I think.

> +/**
> + * struct gpio_pin_range - pin range controlled by a gpio chip
> + * @head: list for maintaining set of pin ranges, used internally
> + * @pctldev: pinctrl device which handles corresponding pins
> + * @range: actual range of pins controlled by a gpio controller
> + */
> +
> +struct gpio_pin_range {
> +       struct list_head node;
> +       struct pinctrl_dev *pctldev;
> +       struct pinctrl_gpio_range range;
> +};
> +#endif

Hm I cannot really decide whether thus range should be in
<linux/pinctrl/pinctrl.h> or here. Isn't it easier to just put it in
<linux/pinctrl/pinctrl.h>?

>  /**
>   * struct gpio_chip - abstract a GPIO controller
>   * @label: for diagnostics
> @@ -132,6 +148,14 @@ struct gpio_chip {
>         int (*of_xlate)(struct gpio_chip *gc,
>                         const struct of_phandle_args *gpiospec, u32 *flags);
>  #endif
> +#ifdef CONFIG_PINCTRL
> +       /* If CONFIG_PINCTRL is enabled, then gpio controllers can
> +        * optionally describe the actual pin range which they serve in
> +        * an SoC. This information would be used by pinctrl subsystem
> +        * to configure corresponding pins for gpio usage.
> +        */
> +       struct list_head pin_ranges;
> +#endif

OK...

(...)
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -133,6 +133,12 @@ extern void pinctrl_add_gpio_range(struct
> pinctrl_dev *pctldev,
>                                 struct pinctrl_gpio_range *range);
>  extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
>                                 struct pinctrl_gpio_range *range);
> +#ifdef CONFIG_OF
> +extern struct pinctrl_dev *of_pinctrl_add_gpio_range(struct device_node *np,
> +               struct pinctrl_gpio_range *range);
> +
> +#endif

Don't #ifdef this, then all code using it has to be #ifdef:ed too.

Provide a stub function below like with all the other functions instead.

Yours,
Linus Walleij

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Shiraz Hashim]

Hi Linus,

Thanks for reviewing,

On Mon, Sep 03, 2012 at 01:48:04PM +0200, Linus Walleij wrote:
> On Sat, Sep 1, 2012 at 1:22 PM, shiraz hashim
> >     gpiolib: provide provision for gpios to register pin range
> >
> >     pinctrl subsystem needs gpio chip base to prepare set of gpio pin ranges
> >     which a given pinctrl driver can handle. This is important to handle
> >     pinctrl gpio request calls in order to program a given pin properly for
> >     gpio operation.
> >
> >     As gpio base is allocated dynamically while gpiochip registration,
> >     presently there exists no clean way to pass this information to the
> >     pinctrl subsystem.
> >
> >     After few discussions from [1], it was concluded that may be gpio
> >     reporting the pin range it supports is a better way than pinctrl
> >     subsystem directly registering it.
> >
> >     [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/166668
> >
> >     Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> 
> (Some spelling/grammar errors above but I can fix that, no big issue.)
> 

Sorry for the English.

> > +2.1) gpio-controller and pinctrl subsystem
> > +------------------------------------------
> >
> > +gpio-controller on a SOC might be tightly coupled with the pinctrl
> > +subsystem, in the sense that the pins can be used by other functions
> > +together with optional gpio feature.
> > +
> > +While the pin allocation is totally managed by the pin ctrl subsystem,
> > +gpio (under gpiolib) is still maintained by gpio drivers. It may happen
> > +that different pin ranges in a SoC is managed by different gpio drivers.
> > +
> > +This makes it logical to let gpio drivers announce their pin ranges to
> > +the pin ctrl subsystem and call 'pinctrl_request_gpio' in order to
> > +request the corresponding pin before any gpio usage.
> > +
> > +For this, the gpio controller can use a pinctrl phandle and pins to
> > +announce the pinrange to the pin ctrl subsystem. For example,
> > +
> > +       qe_pio_e: gpio-controller at 1460 {
> > +               #gpio-cells = <2>;
> > +               compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> > +               reg = <0x1460 0x18>;
> > +               gpio-controller;
> > +                                 pins = <&pinctrl1 20 10>,
> > +                                <&pinctrl2 50 20>;
> > +
> > +    }
> 
> This needs to go into the binding document in
> Documentation/devicetree/bindings/gpio/gpio.txt as well.
> 

right.

> > +
> > +where,
> > +   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.
> > +
> > +   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.
> 
> The above implies that only devcie tree is used for this. That is not
> the case: other architectures like Blackfin, MIPS and x86 shall also use
> the pinctrl subsystem, and they may not have device tree. Maybe they
> have ACPI, maybe BIOS or SFI (Simple Firmware Interface).

Okay.

> So the basic problem with the patch is that you cannot hard-code it
> to use *only* device tree, it must also accept ranges registered
> directly, not using of_*.

For describing gpio pin ranges, we need to know corresponding pinctrl
implementation handling this range, base pin and number of pins
belonging to the range. Further there can be multiple such ranges for
a gpio driver.

With devicetree it was easy to provide this information and parse it
inside gpiolib itself. Without DT how do you suggest gpio drivers must
pass range information to gpiolib ?


> Basically this comes down to creating two functions more
> to add pin ranges from GPIO chips, see below comments.
> 
> Also, you should put a big "DEPRECATED" mark on the old gpio
> range concept, and refer to the new way of doing things.

Okay.


[...]

> >  void of_gpiochip_remove(struct gpio_chip *chip)
> >  {
> > +       of_gpiochip_remove_pin_range(chip);
> > +
> >         if (chip->of_node)
> >                 of_node_put(chip->of_node);
> >  }
> 
> This part looks good.
> 
> But I want two similar function named:
> 
> gpiochip_add_pin_range();
> gpiochip_remove_pin_range();
> 
> that can be used for platforms that doesn't support DT.

How to pass the information required by these functions is something we
need to think about.


[...]

> >  #ifdef CONFIG_GPIOLIB
> >
> > @@ -47,6 +48,21 @@ struct seq_file;
> >  struct module;
> >  struct device_node;
> >
> > +#ifdef CONFIG_PINCTRL
> 
> There's no need to #ifdef this I think.
> 

Yes, you are right.

> > +/**
> > + * struct gpio_pin_range - pin range controlled by a gpio chip
> > + * @head: list for maintaining set of pin ranges, used internally
> > + * @pctldev: pinctrl device which handles corresponding pins
> > + * @range: actual range of pins controlled by a gpio controller
> > + */
> > +
> > +struct gpio_pin_range {
> > +       struct list_head node;
> > +       struct pinctrl_dev *pctldev;
> > +       struct pinctrl_gpio_range range;
> > +};
> > +#endif
> 
> Hm I cannot really decide whether thus range should be in
> <linux/pinctrl/pinctrl.h> or here. Isn't it easier to just put it in
> <linux/pinctrl/pinctrl.h>?
> 

pinctrl describes gpio range through 'pinctrl_gpio_range' while
'gpio_pin_range' is defined here to maintain gpio exported pin ranges.

Since pinctrl is only bothered about pinctrl_gpio_range and not
gpio_pin_range, I think we can keep it here.

[...]

> > +++ b/include/linux/pinctrl/pinctrl.h
> > @@ -133,6 +133,12 @@ extern void pinctrl_add_gpio_range(struct
> > pinctrl_dev *pctldev,
> >                                 struct pinctrl_gpio_range *range);
> >  extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
> >                                 struct pinctrl_gpio_range *range);
> > +#ifdef CONFIG_OF
> > +extern struct pinctrl_dev *of_pinctrl_add_gpio_range(struct device_node *np,
> > +               struct pinctrl_gpio_range *range);
> > +
> > +#endif
> 
> Don't #ifdef this, then all code using it has to be #ifdef:ed too.
> 
> Provide a stub function below like with all the other functions instead.

Sure.

--
regards
Shiraz

[GIT PULL] SPEAr pinctrl updates for v-3.5

[Linus Walleij]

On Tue, Sep 4, 2012 at 6:19 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> [Me]
>> So the basic problem with the patch is that you cannot hard-code it
>> to use *only* device tree, it must also accept ranges registered
>> directly, not using of_*.
>
> For describing gpio pin ranges, we need to know corresponding pinctrl
> implementation handling this range, base pin and number of pins
> belonging to the range. Further there can be multiple such ranges for
> a gpio driver.
>
> With devicetree it was easy to provide this information and parse it
> inside gpiolib itself. Without DT how do you suggest gpio drivers must
> pass range information to gpiolib ?

That's the real trick right. You need a key to search pinctrldev_list
in pinctrl/core.c, so use the name of the pinctrl device and
you get a function like this:

void gpiochip_add_pin_range(char *name, struct gpio_chip *chip);

Loop over pinctrldev_list until dev_name(pctldev->dev) == name
then add the range.

The remove function can look like this however:

void gpiochip_remove_pin_range(struct gpio_chip *chip);

In this case you just look for all pin controllers with the matching
gpiochip pointer.

Yours,
Linus Walleij

[GIT PULL] SPEAr pinctrl updates for v-3.5

[viresh kumar]

Hi Linus,

Sorry for replying on very old thread. Actually i wanted to give
answer to some of
your review comments you already gave.

On Mon, Sep 3, 2012 at 5:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Sep 1, 2012 at 1:22 PM, shiraz hashim
> <shiraz.linux.kernel@gmail.com> wrote:

>> +2.1) gpio-controller and pinctrl subsystem
>> +------------------------------------------
>>

<...>

>> +   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.
>> +
>> +   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.

> Also, you should put a big "DEPRECATED" mark on the old gpio
> range concept, and refer to the new way of doing things.

I have added following on reply to this request:

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index a1cd2f9..da40efb 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -364,6 +364,9 @@ 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.

+Calling pinctrl_add_gpio_range from pinctrl driver is DEPRECATED. Please see
+section 2.1 of Documentation/devicetree/bindings/gpio/gpio.txt on how to bind
+pinctrl and gpio drivers.


>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h

>> +#ifdef CONFIG_PINCTRL
>
> There's no need to #ifdef this I think.

Actually there is, as definition of struct pinctrl_gpio_range is enclosed
in this config option.

>> +/**
>> + * struct gpio_pin_range - pin range controlled by a gpio chip
>> + * @head: list for maintaining set of pin ranges, used internally
>> + * @pctldev: pinctrl device which handles corresponding pins
>> + * @range: actual range of pins controlled by a gpio controller
>> + */
>> +
>> +struct gpio_pin_range {
>> +       struct list_head node;
>> +       struct pinctrl_dev *pctldev;
>> +       struct pinctrl_gpio_range range;
>> +};
>> +#endif

--
viresh