[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Viresh Kumar]

There is a requirement in pinctrl subsystem, to bind pinctrl driver with
gpio_chip. The way i could think of to do this is:
- pass gpio-handle = <&gpiopinctrl>; via pinctrl node
- call of_get_gpio_chip_by_phandle() with of_node of pinctrl
- Use gpio_chip returned to get basic details of chip like its base, which is
  decided at runtime.

So implemented this helper here.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
Linus/Grant,

Once you give your comments on this patch, i would implement this solution for
SPEAr pinctrl drivers.

 drivers/gpio/gpiolib-of.c |   32 ++++++++++++++++++++++++++++++++
 include/linux/of_gpio.h   |    9 ++++++++-
 2 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bf984b6..fd97d51 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -78,6 +78,38 @@ err0:
 EXPORT_SYMBOL(of_get_named_gpio_flags);
 
 /**
+ * of_get_gpio_chip_by_phandle() - Get a GPIO Chip using "gpio-phandle"
+ * @np:		device node to get GPIO chip from
+ *
+ * Returns GPIO chip pointer or NULL on error.
+ */
+struct gpio_chip *of_get_gpio_chip_by_phandle(struct device_node *np)
+{
+	struct gpio_chip *gc;
+	struct device_node *gpio_np;
+
+	gpio_np = of_parse_phandle(np, "gpio-phandle", 0);
+	if (!gpio_np) {
+		pr_debug("%s: Looking up gpio-phandle property failed",
+				np->full_name);
+		return NULL;
+	}
+
+	gc = of_node_to_gpiochip(gpio_np);
+	if (!gc) {
+		pr_debug("%s: gpio controller %s isn't registered\n",
+				np->full_name, gpio_np->full_name);
+		goto err;
+	}
+
+err:
+	of_node_put(gpio_np);
+
+	return gc;
+}
+EXPORT_SYMBOL(of_get_gpio_chip_by_phandle);
+
+/**
  * of_gpio_named_count - Count GPIOs for a device
  * @np:		device node to count GPIOs for
  * @propname:	property name containing gpio specifier(s)
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 81733d1..764c807 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -50,6 +50,8 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
 extern int of_get_named_gpio_flags(struct device_node *np,
 		const char *list_name, int index, enum of_gpio_flags *flags);
 
+extern struct gpio_chip *of_get_gpio_chip_by_phandle(struct device_node *np);
+
 extern unsigned int of_gpio_named_count(struct device_node *np,
 					const char* propname);
 
@@ -72,6 +74,12 @@ static inline int of_get_named_gpio_flags(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline struct gpio_chip *
+of_get_gpio_chip_by_phandle(struct device_node *np)
+{
+	return NULL;
+}
+
 static inline unsigned int of_gpio_named_count(struct device_node *np,
 					const char* propname)
 {
@@ -154,5 +162,4 @@ static inline int of_get_gpio(struct device_node *np, int index)
 {
 	return of_get_gpio_flags(np, index, NULL);
 }
-
 #endif /* __LINUX_OF_GPIO_H */
-- 
1.7.9

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Stephen Warren]

On 05/02/2012 03:04 AM, Viresh Kumar wrote:
> There is a requirement in pinctrl subsystem, to bind pinctrl driver with
> gpio_chip.

Could you please explain more? I don't believe that's true.

It's certainly possible that the same driver can be both a pinctrl
driver and a GPIO driver if this is how the HW works. However, in this
case, the device tree would contain a single node to represent that HW
module, and the driver for that node would then register itself as both
a GPIO chip and a pinctrl driver. In other words, device tree content
shouldn't be influenced by this (except of course that the single node
would contain both GPIO provider and pinctrl provider properties)

When GPIO and pinmux HW are separate but interact, the GPIO/pinctrl
driver interaction is for the GPIO driver to call into pinctrl if
required. However, this is through the generic functions such as
pinctrl_request_gpio() which take global GPIO numbers, and hence have no
need for a specific GPIO driver handle.

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Stephen Warren]

On 05/02/2012 10:51 AM, viresh kumar wrote:
> 
> On May 2, 2012 8:57 PM, "Stephen Warren" <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
>>
>> On 05/02/2012 03:04 AM, Viresh Kumar wrote:
>> > There is a requirement in pinctrl subsystem, to bind pinctrl driver with
>> > gpio_chip.
>>
>> Could you please explain more? I don't believe that's true.
>>
>> It's certainly possible that the same driver can be both a pinctrl
>> driver and a GPIO driver if this is how the HW works. However, in this
>> case, the device tree would contain a single node to represent that HW
>> module, and the driver for that node would then register itself as both
>> a GPIO chip and a pinctrl driver. In other words, device tree content
>> shouldn't be influenced by this (except of course that the single node
>> would contain both GPIO provider and pinctrl provider properties)
>>
>> When GPIO and pinmux HW are separate but interact, the GPIO/pinctrl
>> driver interaction is for the GPIO driver to call into pinctrl if
>> required. However, this is through the generic functions such as
>> pinctrl_request_gpio() which take global GPIO numbers, and hence have no
>> need for a specific GPIO driver handle.
> 
> Whatever you explained is correct, but the
> pinctrl driver is supposed to add gpio ranges for
> which it would need gpio base number. My gpio driver
> allocates this number dynamically.
> 
> So i need someway of accessing gpio chip
> from pinctrl driver. How should i do it?

Ah yes, that's a good point. In Tegra, the GPIO and pinctrl driver
currently hard-code base==0, which probably isn't good.

So yes, this mechanism is needed after all. Sorry for the confusion.

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Linus Walleij]

On Wed, May 2, 2012 at 6:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:

> So i need someway of accessing gpio chip
> from pinctrl driver. How should i do it?

I once proposed a patch like this to gpiolib and Grant NACK:ed it.
It was one of the reasons I created pinctrl to begin with...

Yes, I know it is sometimes a trouble to get the struct gpio_chip *
pointer out, especially if the drivers for pinctrl and GPIO is in
different files with their own say platform_device probe() function
or so.

The U300 solution isn't super-clean, and I have another
solution for Nomadik which is cleaner, but there GPIO and
pinctrl is in the same file. Expect to mail that out tomorrow.

Yours,
Linus Walleij

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Linus Walleij]

On Wed, May 2, 2012 at 7:12 PM, viresh kumar <viresh.linux@gmail.com> wrote:

> There is one more way of doing this
> which is implemented for u300.
> There device node of gpio is added from pinctrl driver.
>
> I didn't liked that. Sorry Linus :(

Heh, I don't think it looks good either, but it works.

The most brutal way is to merge both driver into one file and only
spawn one device for them, with several memory resources associated.

Linus Walleij

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[viresh kumar]

On Mon, May 7, 2012 at 6:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 2, 2012 at 6:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:
>
>> So i need someway of accessing gpio chip
>> from pinctrl driver. How should i do it?
>
> I once proposed a patch like this to gpiolib and Grant NACK:ed it.
> It was one of the reasons I created pinctrl to begin with...

@Grant/Linus: Any specific reason, why it got rejected?

I don't feel there is anything wrong in this approach and is a very
clean solution to
bind two drivers.

> The U300 solution isn't super-clean, and I have another
> solution for Nomadik which is cleaner, but there GPIO and
> pinctrl is in the same file. Expect to mail that out tomorrow.

Even that is not so clean. You are creating another mess in order to fix one.
Two drivers implementing separate functionality must be kept separate.

--
viresh

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Linus Walleij]

On Mon, May 7, 2012 at 5:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:
> On Mon, May 7, 2012 at 6:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, May 2, 2012 at 6:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:
>>
>>> So i need someway of accessing gpio chip
>>> from pinctrl driver. How should i do it?
>>
>> I once proposed a patch like this to gpiolib and Grant NACK:ed it.
>> It was one of the reasons I created pinctrl to begin with...

Actually I was exposing gpio_to_chip() but the reason for
rejection is the same.

> @Grant/Linus: Any specific reason, why it got rejected?

Quoting:
http://marc.info/?l=linux-kernel&m=130281083417581&w=2

"Similar to interrupt handling, it probably isn't a good idea to expose
the gpio_chip directly."

So in the struct irqchip we found that the ability for drivers to access
and play around with the intrinsics of that struct made it hard to
maintain and subject to much suspicious code.

> I don't feel there is anything wrong in this approach and is a very
> clean solution to bind two drivers.

Yeah :-/

The same argument could be made to expose gpio_to_chip()
for any driver not using device tree.

>> The U300 solution isn't super-clean, and I have another
>> solution for Nomadik which is cleaner, but there GPIO and
>> pinctrl is in the same file. Expect to mail that out tomorrow.
>
> Even that is not so clean. You are creating another mess in order to fix one.
> Two drivers implementing separate functionality must be kept separate.

In the Nomadik case there is not so much mess I think, and I have
thought about maybe just more or less concatenating the two
separate U300 driver files (pinctrl-u300.c and pinctrl-coh901.c)
into one, with a single probe call and a single set of resources
(e.g. two base addresses and so on) provided to it. That would
be quite clean.

Yours,
Linus Walleij

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Grant Likely]

On Wed, 9 May 2012 14:01:15 +0200, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 7, 2012 at 5:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:
> > On Mon, May 7, 2012 at 6:38 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> On Wed, May 2, 2012 at 6:51 PM, viresh kumar <viresh.linux@gmail.com> wrote:
> >>
> >>> So i need someway of accessing gpio chip
> >>> from pinctrl driver. How should i do it?
> >>
> >> I once proposed a patch like this to gpiolib and Grant NACK:ed it.
> >> It was one of the reasons I created pinctrl to begin with...
> 
> Actually I was exposing gpio_to_chip() but the reason for
> rejection is the same.
> 
> > @Grant/Linus: Any specific reason, why it got rejected?
> 
> Quoting:
> http://marc.info/?l=linux-kernel&m=130281083417581&w=2
> 
> "Similar to interrupt handling, it probably isn't a good idea to expose
> the gpio_chip directly."
> 
> So in the struct irqchip we found that the ability for drivers to access
> and play around with the intrinsics of that struct made it hard to
> maintain and subject to much suspicious code.
> 
> > I don't feel there is anything wrong in this approach and is a very
> > clean solution to bind two drivers.
> 
> Yeah :-/
> 
> The same argument could be made to expose gpio_to_chip()
> for any driver not using device tree.

Linus, feel free to corner me at connect and we'll talk about it
again.

however, there is a bigger issue now.  I'm changing the xlate
mechanism to allow multiple gpio_chips to refer to the same device
tree node which will break the assumption that this patch uses.  The
reason for this is to make it easier to support banked gpio
controllers where a separate gpio_chip is used for each bank, but it
is still described by a single device tree node.  To properly resolve
this will require a full gpio specifier reference instead of just the
phandle.  Will that work for your use-case?

g.

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Viresh Kumar]

On 5/18/2012 5:38 AM, Grant Likely wrote:
> however, there is a bigger issue now.  I'm changing the xlate
> mechanism to allow multiple gpio_chips to refer to the same device
> tree node which will break the assumption that this patch uses.  The
> reason for this is to make it easier to support banked gpio
> controllers where a separate gpio_chip is used for each bank, but it
> is still described by a single device tree node.  To properly resolve
> this will require a full gpio specifier reference instead of just the
> phandle.  Will that work for your use-case?

What about the other routine i suggested yesterday

of_get_gpio_chip_base_by_phandle()?

This would work with your updates too and will work for me as well.

-- 
viresh

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Stephen Warren]

On 05/17/2012 06:08 PM, Grant Likely wrote:
...
> however, there is a bigger issue now.  I'm changing the xlate
> mechanism to allow multiple gpio_chips to refer to the same device
> tree node which will break the assumption that this patch uses.  The
> reason for this is to make it easier to support banked gpio
> controllers where a separate gpio_chip is used for each bank, but it
> is still described by a single device tree node.  To properly resolve
> this will require a full gpio specifier reference instead of just the
> phandle.  Will that work for your use-case?

Is a full GPIO specifier necessary? Perhaps a phandle plus bank index
would suffice?

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Grant Likely]

On Thu, May 17, 2012 at 9:43 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 5/18/2012 5:38 AM, Grant Likely wrote:
>> however, there is a bigger issue now. ?I'm changing the xlate
>> mechanism to allow multiple gpio_chips to refer to the same device
>> tree node which will break the assumption that this patch uses. ?The
>> reason for this is to make it easier to support banked gpio
>> controllers where a separate gpio_chip is used for each bank, but it
>> is still described by a single device tree node. ?To properly resolve
>> this will require a full gpio specifier reference instead of just the
>> phandle. ?Will that work for your use-case?
>
> What about the other routine i suggested yesterday
>
> of_get_gpio_chip_base_by_phandle()?
>
> This would work with your updates too and will work for me as well.

Ugh; that seems a little messy.  With dynamic allocation of gpio
numbers, there is no guarantee that all the gpio_chips will have
contiguous gpio_bases.  It would require internal knowledge of each
gpio controller's binding.  How do you intend to use that API?

g.

[PATCH] gpiolib: Add of_get_gpio_chip_by_phandle() helper

[Viresh Kumar]

On 5/18/2012 11:16 AM, Grant Likely wrote:
> Ugh; that seems a little messy.  With dynamic allocation of gpio
> numbers, there is no guarantee that all the gpio_chips will have
> contiguous gpio_bases.  It would require internal knowledge of each
> gpio controller's binding.  How do you intend to use that API?

I need gpio_chip base in my pinctrl driver to convert from gpio number
to pin number. That's it. Please suggest how should i fix this up. :)

-- 
viresh