From: Dong Aisheng <[email protected]>
For dt, the gpio base may be dynamically allocated, thus the original
implementation of gpio support based on static gpio number and pin id
map may not be easy to use for dt.
One solution is a) use alias id for gpio node and refer to it in gpio
range, then we can get the fixed the gpio devices for this range and
b) get gpio chip from node which is specified in gpio range structure,
then get the dynamically allocated gpio chip base and c) using the chip
gpio base and offset to map to a physical pin id for dt.
To implement that, we need add two members in pinctrl_gpio_range structure,
'np' the gpio node for this range and 'off' the offset in this gpio range
of the gpio chip.
For devicetree, user can use this two member instead of range->base
to create the map, the core will dynamically set the correct range->base
based on the gpio base plus gpio offset.
For example:
static struct pinctrl_gpio_range imx28_gpio_ranges[] = {
{ .name = "gpio", .id = 0, .pin_base = 0, .npins = 8, .np = np_gpio0 .off = 0,},
{ .name = "gpio", .id = 0, .pin_base = 16, .npins = 13, .np = np_gpio0 .off = 16,},
{ .name = "gpio", .id = 1, .pin_base = 32, .npins = 32, .np = np_gpio1 .off = 16},
...
Signed-off-by: Dong Aisheng <[email protected]>
---
This is a proposal to start the discussion on whether we can find some common way
to ease adding pinctrl gpio support for dt.
Comments are wellcome.
---
drivers/pinctrl/core.c | 11 ++++++++-
drivers/pinctrl/devicetree.c | 47 +++++++++++++++++++++++++++++++++++++++
drivers/pinctrl/devicetree.h | 7 +++++
include/linux/pinctrl/pinctrl.h | 7 +++++-
4 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c3b331b..8b882ad 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -323,12 +323,21 @@ static int pinctrl_get_device_gpio_range(unsigned gpio,
* This adds a range of GPIOs to be handled by a certain pin controller. Call
* this to register handled ranges after registering your pin controller.
*/
-void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
+int pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
struct pinctrl_gpio_range *range)
{
+ int ret;
+
+ /* first recalculate the gpio base for dt if need */
+ ret = pinctrl_recalculate_gpio_base(pctldev, range);
+ if (ret)
+ return ret;
+
mutex_lock(&pinctrl_mutex);
list_add_tail(&range->node, &pctldev->gpio_ranges);
mutex_unlock(&pinctrl_mutex);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(pinctrl_add_gpio_range);
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..00015c7 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -18,6 +18,7 @@
#include <linux/device.h>
#include <linux/of.h>
+#include <linux/of_gpio.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/slab.h>
@@ -247,3 +248,49 @@ err:
pinctrl_dt_free_maps(p);
return ret;
}
+
+int pinctrl_recalculate_gpio_base(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range)
+{
+ struct device_node *np,
+ int ret = 0;
+
+ if (!range->np || range->off < 0) {
+ dev_err(&pdev->dev, "no gpio node or invalid offset");
+ return -EINVAL;
+ }
+
+ np = range->np;
+ of_node_get(np);
+
+ id = of_alias_get_id(np, "gpio");
+ if (id < 0) {
+ dev_err(&pdev->dev,
+ "can not find alias id for node %s\n", np->name);
+ ret = id;
+ goto out;
+ }
+
+ if (range->id != id) {
+ dev_err(&pdev->dev,
+ "the range id(%d) does not match gpio(%s) alias id(%d)\n",
+ range->id, np->name, id);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ range->gc = of_node_to_gpiochip(np);
+ if (!range->gc) {
+ dev_err(pctldev->dev,
+ "can not find gpio chip of node(%s)\n",
+ np->name);
+ ret = -ENODEV;
+ goto out;
+ }
+ range->base = range->gc->base + range->off;
+
+out:
+ of_node_put(np);
+
+ return ret;
+}
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
index 760bc49..d97ad43 100644
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -21,6 +21,8 @@
void pinctrl_dt_free_maps(struct pinctrl *p);
int pinctrl_dt_to_map(struct pinctrl *p);
+int pinctrl_recalculate_gpio_base(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range);
#else
static inline int pinctrl_dt_to_map(struct pinctrl *p)
@@ -32,4 +34,9 @@ static inline void pinctrl_dt_free_maps(struct pinctrl *p)
{
}
+static inline int pinctrl_recalculate_gpio_base(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range)
+{
+ return 0;
+}
#endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..29b01b5 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -52,6 +52,9 @@ struct pinctrl_pin_desc {
* @pin_base: base pin number of the GPIO range
* @npins: number of pins in the GPIO range, including the base number
* @gc: an optional pointer to a gpio_chip
+ * @np: the gpio device node for this range, used to find the gpio chip for dt.
+ * @off: offset in the gpio range of the gpio_chip. Used for dynamically binding
+ * the gpio base from devicetree. Then @base is set as gc->base + off.
*/
struct pinctrl_gpio_range {
struct list_head node;
@@ -61,6 +64,8 @@ struct pinctrl_gpio_range {
unsigned int pin_base;
unsigned int npins;
struct gpio_chip *gc;
+ struct device_node *np;
+ unsigned int off;
};
/**
@@ -129,7 +134,7 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
struct device *dev, void *driver_data);
extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
-extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
+extern int 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);
--
1.7.0.4
On Tue, May 15, 2012 at 10:07:19PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <[email protected]>
>
> For dt, the gpio base may be dynamically allocated, thus the original
> implementation of gpio support based on static gpio number and pin id
> map may not be easy to use for dt.
>
> One solution is a) use alias id for gpio node and refer to it in gpio
> range, then we can get the fixed the gpio devices for this range and
> b) get gpio chip from node which is specified in gpio range structure,
> then get the dynamically allocated gpio chip base and c) using the chip
> gpio base and offset to map to a physical pin id for dt.
>
> To implement that, we need add two members in pinctrl_gpio_range structure,
> 'np' the gpio node for this range and 'off' the offset in this gpio range
> of the gpio chip.
>
> For devicetree, user can use this two member instead of range->base
> to create the map, the core will dynamically set the correct range->base
> based on the gpio base plus gpio offset.
>
> For example:
> static struct pinctrl_gpio_range imx28_gpio_ranges[] = {
> { .name = "gpio", .id = 0, .pin_base = 0, .npins = 8, .np = np_gpio0 .off = 0,},
> { .name = "gpio", .id = 0, .pin_base = 16, .npins = 13, .np = np_gpio0 .off = 16,},
> { .name = "gpio", .id = 1, .pin_base = 32, .npins = 32, .np = np_gpio1 .off = 16},
How do you have .np statically assigned?
--
Regards,
Shawn
On Thu, May 17, 2012 at 10:16:15AM +0800, Guo Shawn-R65073 wrote:
> On Tue, May 15, 2012 at 10:07:19PM +0800, Dong Aisheng wrote:
> > From: Dong Aisheng <[email protected]>
> >
> > For dt, the gpio base may be dynamically allocated, thus the original
> > implementation of gpio support based on static gpio number and pin id
> > map may not be easy to use for dt.
> >
> > One solution is a) use alias id for gpio node and refer to it in gpio
> > range, then we can get the fixed the gpio devices for this range and
> > b) get gpio chip from node which is specified in gpio range structure,
> > then get the dynamically allocated gpio chip base and c) using the chip
> > gpio base and offset to map to a physical pin id for dt.
> >
> > To implement that, we need add two members in pinctrl_gpio_range structure,
> > 'np' the gpio node for this range and 'off' the offset in this gpio range
> > of the gpio chip.
> >
> > For devicetree, user can use this two member instead of range->base
> > to create the map, the core will dynamically set the correct range->base
> > based on the gpio base plus gpio offset.
> >
> > For example:
> > static struct pinctrl_gpio_range imx28_gpio_ranges[] = {
> > { .name = "gpio", .id = 0, .pin_base = 0, .npins = 8, .np = np_gpio0 .off = 0,},
> > { .name = "gpio", .id = 0, .pin_base = 16, .npins = 13, .np = np_gpio0 .off = 16,},
> > { .name = "gpio", .id = 1, .pin_base = 32, .npins = 32, .np = np_gpio1 .off = 16},
>
> How do you have .np statically assigned?
>
The drivers are responsible for set .np, usually dynamically get it from whether
the driver wants.
Regards
Dong Aisheng
On 05/15/2012 08:07 AM, Dong Aisheng wrote:
> From: Dong Aisheng <[email protected]>
>
> For dt, the gpio base may be dynamically allocated, thus the original
> implementation of gpio support based on static gpio number and pin id
> map may not be easy to use for dt.
>
> One solution is a) use alias id for gpio node and refer to it in gpio
> range, then we can get the fixed the gpio devices for this range and
> b) get gpio chip from node which is specified in gpio range structure,
> then get the dynamically allocated gpio chip base and c) using the chip
> gpio base and offset to map to a physical pin id for dt.
As I just mentioned in response to Shawn's driver, I don't think that
using the aliases node is the way to go here.
Instead, what about the following:
/*
* This function parses property @propname in DT node @np. This property
* is a list of phandles, with optional @cellskip cells between them.
* For each entry in ranges, the phandle at index
* range[i].id * (1 + @cellskip) is written into range[i].np
*/
int pinctrl_dt_gpio_ranges_add_np(struct pinctrl_gpio_range *ranges,
int nranges,
struct device_node *np,
const char *propname,
int cellskip);
Note: cellskip is usually 0. However, to allow the same code to be used
for pinctrl-simple/pinctrl-generic's GPIO mapping table, we allow
additional cells between the phandles.
For example, Tegra might have:
gpio-controllers = <&tegra_gpio>; // there's just 1
i.MX23 might have:
gpio-controllers = <&gpio0 &gpio1 &gpio2 &gpio3>; // it has 4 banks
whereas pinctrl-simple/pinctrl-generic might want to put the entire
range table in this property, so might do something like:
gpio-ranges = <&gpio0 $gpio_offset $pin_offset $count>
<&gpio1 $gpio_offset $pin_offset $count> ...;
and hence set cellskip to 3. the pinctrl-simple/pinctrl-generic code
would need to parse the other 3 cells itself.
The algorithm is roughly:
prop = get_property(propname)
for range in ranges:
if not range.np:
phandle = get_phandle_by_index(prop, range.id);
Have a second function that converts the np pointer to gpio chips. This
could be used if the np field was obtained somewhere other than by
calling pinctrl_dt_add_np_to_ranges():
int pinctrl_dt_gpio_ranges_np_to_gc(struct pinctrl_gpio_range *ranges,
int nranges);
Note: For any np where of_node_to_gpiochip() fails,
pinctrl_dt_gpio_ranges_np_to_gc() should return -EPROBE_DEFER so that
the pinctrl driver can wait for the GPIO driver to probe before continuing.
The algorithm is roughly:
for range in ranges:
if range.gc:
continue
if not range.np:
continue
range.gc = of_node_to_gpiochip(range.np)
if not range.gc:
return -EPROBE_DEFER
Have a third function which converts gpio chip plus offset into the
range's real base. This would be useful even when not using DT:
int pinctrl_gpio_ranges_recalc_bases(struct pinctrl_gpio_range *ranges,
int nranges);
The algorithm is roughly:
for range in ranges:
// only convert things not already set
if range.base:
continue
if not range.gc:
continue
range.base = base(range.gc) + range.offset
One could imagine helper functions that wrapped all those 3 functions
into 1 call for drivers to use.
Does that sound like a reasonable idea?
On Fri, May 18, 2012 at 04:06:13AM +0800, Stephen Warren wrote:
> On 05/15/2012 08:07 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <[email protected]>
> >
> > For dt, the gpio base may be dynamically allocated, thus the original
> > implementation of gpio support based on static gpio number and pin id
> > map may not be easy to use for dt.
> >
> > One solution is a) use alias id for gpio node and refer to it in gpio
> > range, then we can get the fixed the gpio devices for this range and
> > b) get gpio chip from node which is specified in gpio range structure,
> > then get the dynamically allocated gpio chip base and c) using the chip
> > gpio base and offset to map to a physical pin id for dt.
>
> As I just mentioned in response to Shawn's driver, I don't think that
> using the aliases node is the way to go here.
>
Correct, i originally thought we need alias id to fix the gpio chip we're using,
Now i realized that a gpio np already fix the gpio chip which this range
represents. So i think we should remove alias id.
> Instead, what about the following:
>
> /*
> * This function parses property @propname in DT node @np. This property
> * is a list of phandles, with optional @cellskip cells between them.
> * For each entry in ranges, the phandle at index
> * range[i].id * (1 + @cellskip) is written into range[i].np
> */
> int pinctrl_dt_gpio_ranges_add_np(struct pinctrl_gpio_range *ranges,
> int nranges,
> struct device_node *np,
> const char *propname,
> int cellskip);
>
> Note: cellskip is usually 0. However, to allow the same code to be used
> for pinctrl-simple/pinctrl-generic's GPIO mapping table, we allow
> additional cells between the phandles.
>
This is a good idea to find the gpio nodes for the ranges.
I once had a similar idea but felt it's not suitable for one range
taking a list of all gpio nodes as parameter in my original implementation.
But it's very easy fixed by your API, take a list of ranges. :-)
> For example, Tegra might have:
>
> gpio-controllers = <&tegra_gpio>; // there's just 1
>
> i.MX23 might have:
>
> gpio-controllers = <&gpio0 &gpio1 &gpio2 &gpio3>; // it has 4 banks
>
> whereas pinctrl-simple/pinctrl-generic might want to put the entire
> range table in this property, so might do something like:
>
> gpio-ranges = <&gpio0 $gpio_offset $pin_offset $count>
> <&gpio1 $gpio_offset $pin_offset $count> ...;
>
> and hence set cellskip to 3. the pinctrl-simple/pinctrl-generic code
> would need to parse the other 3 cells itself.
>
I like this way. :-)
You made me come up a few more thoughts on it.
Why shouldn't we use this format as a standard binding way for gpio ranges
in pinctrl core and let core handle it totally for creating the range table?
This format has enough information for core to do that and it is fully comply
with the non-dt implementation way and should work for dt too.
We already parse pinctrl mapping in a standard way in core, so why not gpio
ranges mapping?
For example, i.MX28 might have:
gpio-ranges = <&gpio0 0 0 8>
<&gpio0 16 16 13>
<&gpio1 0 32 32>
.........
Tegra might have:
gpio-ranges = <&tegra_gpio 0 0 TEGRA_GPIO_NUM>;
IMX, similar to pinctrl-generic might have:
gpio-ranges = <&gpio0 0 IMX_PIN_X 1>
<&gpio0 1 IMX_PIN_X 1>
<&gpio0 2 IMX_PIN_X 1>
..........
The only concern is i'm not sure if any other SoC needs more cells for
this format since we want this to be a standard bind format.
But for now i did not see any need, people may raise it if any.
> The algorithm is roughly:
>
> prop = get_property(propname)
> for range in ranges:
> if not range.np:
> phandle = get_phandle_by_index(prop, range.id);
>
If doing in the core, user do not need to specify the range.id any more.
Instead, the range.id will be set to -1 by default since the id of gpios
for dt is -1.
> Have a second function that converts the np pointer to gpio chips. This
> could be used if the np field was obtained somewhere other than by
> calling pinctrl_dt_add_np_to_ranges():
>
> int pinctrl_dt_gpio_ranges_np_to_gc(struct pinctrl_gpio_range *ranges,
> int nranges);
>
> Note: For any np where of_node_to_gpiochip() fails,
> pinctrl_dt_gpio_ranges_np_to_gc() should return -EPROBE_DEFER so that
> the pinctrl driver can wait for the GPIO driver to probe before continuing.
Correct. We should do like that.
>
> The algorithm is roughly:
>
> for range in ranges:
> if range.gc:
> continue
> if not range.np:
> continue
> range.gc = of_node_to_gpiochip(range.np)
> if not range.gc:
> return -EPROBE_DEFER
>
> Have a third function which converts gpio chip plus offset into the
> range's real base. This would be useful even when not using DT:
>
> int pinctrl_gpio_ranges_recalc_bases(struct pinctrl_gpio_range *ranges,
> int nranges);
>
As above, if we do it in core, then user do not need to make the decision
to call which of these three functions.
The core will handle it transparently to drivers.
> The algorithm is roughly:
>
> for range in ranges:
> // only convert things not already set
> if range.base:
> continue
> if not range.gc:
> continue
> range.base = base(range.gc) + range.offset
>
> One could imagine helper functions that wrapped all those 3 functions
> into 1 call for drivers to use.
>
> Does that sound like a reasonable idea?
>
The idea is really good.
Thanks a lot Stephen.
I just put some more thoughts based on it.
Do you think that is reasonable?
Regards
Dong Aisheng
On 05/18/2012 01:26 AM, Dong Aisheng wrote:
> On Fri, May 18, 2012 at 04:06:13AM +0800, Stephen Warren wrote:
>> On 05/15/2012 08:07 AM, Dong Aisheng wrote:
>>> For dt, the gpio base may be dynamically allocated, thus the original
>>> implementation of gpio support based on static gpio number and pin id
...
>> For example, Tegra might have:
>>
>> gpio-controllers = <&tegra_gpio>; // there's just 1
>>
>> i.MX23 might have:
>>
>> gpio-controllers = <&gpio0 &gpio1 &gpio2 &gpio3>; // it has 4 banks
>>
>> whereas pinctrl-simple/pinctrl-generic might want to put the entire
>> range table in this property, so might do something like:
>>
>> gpio-ranges = <&gpio0 $gpio_offset $pin_offset $count>
>> <&gpio1 $gpio_offset $pin_offset $count> ...;
>>
>> and hence set cellskip to 3. the pinctrl-simple/pinctrl-generic code
>> would need to parse the other 3 cells itself.
>
> I like this way. :-)
> You made me come up a few more thoughts on it.
> Why shouldn't we use this format as a standard binding way for gpio ranges
> in pinctrl core and let core handle it totally for creating the range table?
Yes, that's probably reasonable.
Typically I'd argue that since the pinctrl driver probably knows this
information, and it's static, it should be in the driver not DT.
However, since the data is very small, and half of it needs to be in DT
anyway to provide the ability to look up the gpio_chip, it seems
reasonable to just put the whole thing in DT..
> The only concern is i'm not sure if any other SoC needs more cells for
> this format since we want this to be a standard bind format.
> But for now i did not see any need, people may raise it if any.
There's still the open question re: how to look up the gpio_chip from
the DT node, since there may be multiple gpio_chips for a single DT node
(banked controllers). Still, I hope that's just an implementation detail
rather than anything that conceptually breaks this idea.