gpiolib: Add of_get_gpio_chip_by_phandle() helper

PATCH New Viresh Kumar 5 participants 12 messages May 2, 2012 – May 18, 2012 2 messages not in the archive

Subsystems (2)

GPIO Device tree

lore

Share & tools

lore

Viresh Kumar

author

2012-05-02 09:04 UTC #0 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

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 <[email protected]>

---
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(-)

drivers/gpio/gpiolib-of.c +32 -0

index bf984b6..fd97d51 100644
@@ -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)

include/linux/of_gpio.h +8 -1

index 81733d1..764c807 100644
@@ -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 */

Signature · 1 line

1.7.9

Stephen Warren

2012-05-02 15:27 UTC #1 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 2 lines · Viresh Kumar

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.

A message that is not in the archive (sent off-list or not yet received): CAOh2x=nrNDrtNjOroWOm6HkYkc3f2qfaWrtN97d1M3rWv1qmsQ@mail.gmail.com

Stephen Warren

2012-05-02 17:04 UTC #3 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 24 lines · viresh kumar

Quoted 15 lines · On May 2, 2012 8:57 PM, "Stephen Warren" <[email protected]…
Quoted 2 lines · Viresh Kumar
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)
Quoted 5 lines
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.

Quoted 7 lines

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.

A message that is not in the archive (sent off-list or not yet received): CAOh2x=kvdB3VxGCToBBUkXAkTUh8bmpgu_rbkhxY4qLt3Lcp2w@mail.gmail.com

Linus Walleij

2012-05-07 13:09 UTC #5 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 5 lines · viresh kumar

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

2012-05-07 13:08 UTC #6 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

In reply to a message not in the archive

Quoted 2 lines · viresh kumar

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

viresh kumar

2012-05-07 15:51 UTC #7 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 7 lines · Linus Walleij

Quoted 2 lines · viresh kumar
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.

Quoted 3 lines

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

Linus Walleij

2012-05-09 12:01 UTC #8 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 8 lines · viresh kumar

Quoted 7 lines · Linus Walleij
Quoted 2 lines · viresh kumar
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.

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines
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

Grant Likely

2012-05-18 00:08 UTC #9 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 25 lines · Linus Walleij

Quoted 8 lines · viresh kumar
Quoted 7 lines · Linus Walleij
Quoted 2 lines · viresh kumar
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.
Quoted 1 line
@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.

Quoted 7 lines

Quoted 2 lines
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?

Viresh Kumar

author

2012-05-18 03:43 UTC #10 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 8 lines · Grant Likely

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.

Signature · 1 line

viresh

Grant Likely

2012-05-18 05:46 UTC #11 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 10 lines · Viresh Kumar

Quoted 8 lines · Grant Likely
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?

Quoted 5 lines

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?

Viresh Kumar

author

2012-05-18 06:43 UTC #12 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

Quoted 4 lines · Grant Likely

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

Signature · 1 line

viresh

Stephen Warren

2012-05-18 04:35 UTC #13 lore raw

PATCH 1/1 gpiolib: Add of_get_gpio_chip_by_phandle() helper

In reply to Grant Likely

On 05/17/2012 06:08 PM, Grant Likely wrote:
...

Quoted 8 lines

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?