[PATCH] pinctrl: mxs: add gpio range support

[PATCH] pinctrl: mxs: add gpio range support

[Shawn Guo]

The mxs pins are organized in banks and each bank contains 32 pins.
i.MX23 has 4 banks in total, and every pin in the first 3 banks has
gpio function available.  i.MX28 has 7 banks and the first 5 banks
are capable of gpio mode.

As the gpio base is runtime determined for each port when booting
from device tree, the .base of struct pinctrl_gpio_range can only be
initialized with a offset local to the port, and have to plus gpio base
of the port at runtime.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pinctrl/pinctrl-imx23.c |   12 ++++++++
 drivers/pinctrl/pinctrl-imx28.c |   19 +++++++++++++
 drivers/pinctrl/pinctrl-mxs.c   |   55 +++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-mxs.h   |    2 +
 4 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-imx23.c b/drivers/pinctrl/pinctrl-imx23.c
index 75d3eff..695b64b 100644
--- a/drivers/pinctrl/pinctrl-imx23.c
+++ b/drivers/pinctrl/pinctrl-imx23.c
@@ -261,10 +261,22 @@ static struct mxs_regs imx23_regs = {
 	.pull = 0x400,
 };
 
+/*
+ * The .base is initialized as the gpio offset local to the port, and will
+ * have gpio base of the port added at runtime.
+ */
+static struct pinctrl_gpio_range imx23_gpio_ranges[] = {
+	{ .name = "gpio", .id = 0, .base = 0, .pin_base = 0,  .npins = 32, },
+	{ .name = "gpio", .id = 1, .base = 0, .pin_base = 32, .npins = 31, },
+	{ .name = "gpio", .id = 2, .base = 0, .pin_base = 64, .npins = 32, },
+};
+
 static struct mxs_pinctrl_soc_data imx23_pinctrl_data = {
 	.regs = &imx23_regs,
 	.pins = imx23_pins,
 	.npins = ARRAY_SIZE(imx23_pins),
+	.gpio_ranges = imx23_gpio_ranges,
+	.gpio_num_ranges = ARRAY_SIZE(imx23_gpio_ranges),
 };
 
 static int __devinit imx23_pinctrl_probe(struct platform_device *pdev)
diff --git a/drivers/pinctrl/pinctrl-imx28.c b/drivers/pinctrl/pinctrl-imx28.c
index b973026..c9a8e67 100644
--- a/drivers/pinctrl/pinctrl-imx28.c
+++ b/drivers/pinctrl/pinctrl-imx28.c
@@ -377,10 +377,29 @@ static struct mxs_regs imx28_regs = {
 	.pull = 0x600,
 };
 
+/*
+ * The .base is initialized as the gpio offset local to the port, and will
+ * have gpio base of the port added at runtime.
+ */
+static struct pinctrl_gpio_range imx28_gpio_ranges[] = {
+	{ .name = "gpio", .id = 0, .base = 0,  .pin_base = 0,   .npins = 8, },
+	{ .name = "gpio", .id = 0, .base = 16, .pin_base = 16,  .npins = 13, },
+	{ .name = "gpio", .id = 1, .base = 0,  .pin_base = 32,  .npins = 32, },
+	{ .name = "gpio", .id = 2, .base = 0,  .pin_base = 64,  .npins = 11, },
+	{ .name = "gpio", .id = 2, .base = 12, .pin_base = 76,  .npins = 10, },
+	{ .name = "gpio", .id = 2, .base = 24, .pin_base = 88,  .npins = 4, },
+	{ .name = "gpio", .id = 3, .base = 0,  .pin_base = 96,  .npins = 19, },
+	{ .name = "gpio", .id = 3, .base = 20, .pin_base = 116, .npins = 11, },
+	{ .name = "gpio", .id = 4, .base = 0,  .pin_base = 128, .npins = 17, },
+	{ .name = "gpio", .id = 4, .base = 20, .pin_base = 148, .npins = 1, },
+};
+
 static struct mxs_pinctrl_soc_data imx28_pinctrl_data = {
 	.regs = &imx28_regs,
 	.pins = imx28_pins,
 	.npins = ARRAY_SIZE(imx28_pins),
+	.gpio_ranges = imx28_gpio_ranges,
+	.gpio_num_ranges = ARRAY_SIZE(imx28_gpio_ranges),
 };
 
 static int __devinit imx28_pinctrl_probe(struct platform_device *pdev)
diff --git a/drivers/pinctrl/pinctrl-mxs.c b/drivers/pinctrl/pinctrl-mxs.c
index ab63d38..ea33ebc 100644
--- a/drivers/pinctrl/pinctrl-mxs.c
+++ b/drivers/pinctrl/pinctrl-mxs.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -220,12 +221,41 @@ static void mxs_pinctrl_disable(struct pinctrl_dev *pctldev,
 	/* Nothing to do here */
 }
 
+static int mxs_gpio_request_enable(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *range,
+				   unsigned pinid)
+{
+	struct mxs_pinctrl_data *d = pinctrl_dev_get_drvdata(pctldev);
+	void __iomem *reg;
+	u8 bank, shift;
+	u16 pin;
+
+	bank = PINID_TO_BANK(pinid);
+	pin = PINID_TO_PIN(pinid);
+	reg = d->base + d->soc->regs->muxsel;
+	reg += bank * 0x20 + pin / 16 * 0x10;
+	shift = pin % 16 * 2;
+
+	writel(0x3 << shift, reg + SET);
+
+	return 0;
+}
+
+static void mxs_gpio_disable_free(struct pinctrl_dev *pctldev,
+				  struct pinctrl_gpio_range *range,
+				  unsigned pinid)
+{
+	/* Nothing to do here */
+}
+
 static struct pinmux_ops mxs_pinmux_ops = {
 	.get_functions_count = mxs_pinctrl_get_funcs_count,
 	.get_function_name = mxs_pinctrl_get_func_name,
 	.get_function_groups = mxs_pinctrl_get_func_groups,
 	.enable = mxs_pinctrl_enable,
 	.disable = mxs_pinctrl_disable,
+	.gpio_request_enable = mxs_gpio_request_enable,
+	.gpio_disable_free = mxs_gpio_disable_free,
 };
 
 static int mxs_pinconf_get(struct pinctrl_dev *pctldev,
@@ -482,7 +512,9 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
 				struct mxs_pinctrl_soc_data *soc)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct device_node *child;
 	struct mxs_pinctrl_data *d;
+	int id, i, j = 0;
 	int ret;
 
 	d = devm_kzalloc(&pdev->dev, sizeof(*d), GFP_KERNEL);
@@ -500,6 +532,26 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
 	mxs_pinctrl_desc.npins = d->soc->npins;
 	mxs_pinctrl_desc.name = dev_name(&pdev->dev);
 
+	for_each_child_of_node(np, child) {
+		if (!of_device_is_compatible(child, "fsl,mxs-gpio"))
+			continue;
+
+		id = of_alias_get_id(child, "gpio");
+		if (id < 0 || id > soc->gpio_num_ranges) {
+			dev_err(&pdev->dev, "invalid gpio id: %d\n", id);
+			return id;
+		}
+
+		for (i = j; i < soc->gpio_num_ranges; i++) {
+			struct pinctrl_gpio_range *range = &soc->gpio_ranges[i];
+			if (range->id == id) {
+				range->gc = of_node_to_gpiochip(child);
+				range->base += range->gc->base;
+				j++;
+			}
+		}
+	}
+
 	platform_set_drvdata(pdev, d);
 
 	ret = mxs_pinctrl_probe_dt(pdev, d);
@@ -515,6 +567,9 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
 		goto err;
 	}
 
+	for (i = 0; i < soc->gpio_num_ranges; i++)
+		pinctrl_add_gpio_range(d->pctl, &soc->gpio_ranges[i]);
+
 	return 0;
 
 err:
diff --git a/drivers/pinctrl/pinctrl-mxs.h b/drivers/pinctrl/pinctrl-mxs.h
index fdd88d0b..7feef15 100644
--- a/drivers/pinctrl/pinctrl-mxs.h
+++ b/drivers/pinctrl/pinctrl-mxs.h
@@ -82,6 +82,8 @@ struct mxs_pinctrl_soc_data {
 	unsigned nfunctions;
 	struct mxs_group *groups;
 	unsigned ngroups;
+	struct pinctrl_gpio_range *gpio_ranges;
+	unsigned gpio_num_ranges;
 };
 
 int mxs_pinctrl_probe(struct platform_device *pdev,
-- 
1.7.4.1

[PATCH] pinctrl: mxs: add gpio range support

[Dong Aisheng]

On Tue, May 15, 2012 at 01:56:14PM +0800, Shawn Guo wrote:
> The mxs pins are organized in banks and each bank contains 32 pins.
> i.MX23 has 4 banks in total, and every pin in the first 3 banks has
> gpio function available.  i.MX28 has 7 banks and the first 5 banks
> are capable of gpio mode.
> 
> As the gpio base is runtime determined for each port when booting
> from device tree, the .base of struct pinctrl_gpio_range can only be
> initialized with a offset local to the port, and have to plus gpio base
> of the port at runtime.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/pinctrl/pinctrl-imx23.c |   12 ++++++++
>  drivers/pinctrl/pinctrl-imx28.c |   19 +++++++++++++
>  drivers/pinctrl/pinctrl-mxs.c   |   55 +++++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-mxs.h   |    2 +
>  4 files changed, 88 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-imx23.c b/drivers/pinctrl/pinctrl-imx23.c
> index 75d3eff..695b64b 100644
> --- a/drivers/pinctrl/pinctrl-imx23.c
> +++ b/drivers/pinctrl/pinctrl-imx23.c
> @@ -261,10 +261,22 @@ static struct mxs_regs imx23_regs = {
>  	.pull = 0x400,
>  };
>  
> +/*
> + * The .base is initialized as the gpio offset local to the port, and will
> + * have gpio base of the port added at runtime.
> + */
> +static struct pinctrl_gpio_range imx23_gpio_ranges[] = {
> +	{ .name = "gpio", .id = 0, .base = 0, .pin_base = 0,  .npins = 32, },
> +	{ .name = "gpio", .id = 1, .base = 0, .pin_base = 32, .npins = 31, },
> +	{ .name = "gpio", .id = 2, .base = 0, .pin_base = 64, .npins = 32, },
> +};
> +
>  static struct mxs_pinctrl_soc_data imx23_pinctrl_data = {
>  	.regs = &imx23_regs,
>  	.pins = imx23_pins,
>  	.npins = ARRAY_SIZE(imx23_pins),
> +	.gpio_ranges = imx23_gpio_ranges,
> +	.gpio_num_ranges = ARRAY_SIZE(imx23_gpio_ranges),
>  };
>  
>  static int __devinit imx23_pinctrl_probe(struct platform_device *pdev)
> diff --git a/drivers/pinctrl/pinctrl-imx28.c b/drivers/pinctrl/pinctrl-imx28.c
> index b973026..c9a8e67 100644
> --- a/drivers/pinctrl/pinctrl-imx28.c
> +++ b/drivers/pinctrl/pinctrl-imx28.c
> @@ -377,10 +377,29 @@ static struct mxs_regs imx28_regs = {
>  	.pull = 0x600,
>  };
>  
> +/*
> + * The .base is initialized as the gpio offset local to the port, and will
> + * have gpio base of the port added at runtime.
> + */
A good idea.
I was also trying to do this for pinctrl gpio dt improvement but i planned to
do such things in core since i thought other SoCs may face the same issue.
Maybe i should send out a RFC patch for open discussion.

> +static struct pinctrl_gpio_range imx28_gpio_ranges[] = {
> +	{ .name = "gpio", .id = 0, .base = 0,  .pin_base = 0,   .npins = 8, },
> +	{ .name = "gpio", .id = 0, .base = 16, .pin_base = 16,  .npins = 13, },
> +	{ .name = "gpio", .id = 1, .base = 0,  .pin_base = 32,  .npins = 32, },
> +	{ .name = "gpio", .id = 2, .base = 0,  .pin_base = 64,  .npins = 11, },
> +	{ .name = "gpio", .id = 2, .base = 12, .pin_base = 76,  .npins = 10, },
> +	{ .name = "gpio", .id = 2, .base = 24, .pin_base = 88,  .npins = 4, },
> +	{ .name = "gpio", .id = 3, .base = 0,  .pin_base = 96,  .npins = 19, },
> +	{ .name = "gpio", .id = 3, .base = 20, .pin_base = 116, .npins = 11, },
> +	{ .name = "gpio", .id = 4, .base = 0,  .pin_base = 128, .npins = 17, },
> +	{ .name = "gpio", .id = 4, .base = 20, .pin_base = 148, .npins = 1, },
> +};
> +
>  static struct mxs_pinctrl_soc_data imx28_pinctrl_data = {
>  	.regs = &imx28_regs,
>  	.pins = imx28_pins,
>  	.npins = ARRAY_SIZE(imx28_pins),
> +	.gpio_ranges = imx28_gpio_ranges,
> +	.gpio_num_ranges = ARRAY_SIZE(imx28_gpio_ranges),
>  };
>  
>  static int __devinit imx28_pinctrl_probe(struct platform_device *pdev)
> diff --git a/drivers/pinctrl/pinctrl-mxs.c b/drivers/pinctrl/pinctrl-mxs.c
> index ab63d38..ea33ebc 100644
> --- a/drivers/pinctrl/pinctrl-mxs.c
> +++ b/drivers/pinctrl/pinctrl-mxs.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/pinctrl/machine.h>
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/pinctrl/pinctrl.h>
> @@ -220,12 +221,41 @@ static void mxs_pinctrl_disable(struct pinctrl_dev *pctldev,
>  	/* Nothing to do here */
>  }
>  
> +static int mxs_gpio_request_enable(struct pinctrl_dev *pctldev,
> +				   struct pinctrl_gpio_range *range,
> +				   unsigned pinid)
> +{
> +	struct mxs_pinctrl_data *d = pinctrl_dev_get_drvdata(pctldev);
> +	void __iomem *reg;
> +	u8 bank, shift;
> +	u16 pin;
> +
> +	bank = PINID_TO_BANK(pinid);
> +	pin = PINID_TO_PIN(pinid);
> +	reg = d->base + d->soc->regs->muxsel;
> +	reg += bank * 0x20 + pin / 16 * 0x10;
> +	shift = pin % 16 * 2;
> +
> +	writel(0x3 << shift, reg + SET);
> +
> +	return 0;
> +}
> +
> +static void mxs_gpio_disable_free(struct pinctrl_dev *pctldev,
> +				  struct pinctrl_gpio_range *range,
> +				  unsigned pinid)
> +{
> +	/* Nothing to do here */
> +}
It seems .gpio_disable_free can be optional, so you can remove this
empty function.

> +
>  static struct pinmux_ops mxs_pinmux_ops = {
>  	.get_functions_count = mxs_pinctrl_get_funcs_count,
>  	.get_function_name = mxs_pinctrl_get_func_name,
>  	.get_function_groups = mxs_pinctrl_get_func_groups,
>  	.enable = mxs_pinctrl_enable,
>  	.disable = mxs_pinctrl_disable,
> +	.gpio_request_enable = mxs_gpio_request_enable,
> +	.gpio_disable_free = mxs_gpio_disable_free,
>  };
>  
>  static int mxs_pinconf_get(struct pinctrl_dev *pctldev,
> @@ -482,7 +512,9 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
>  				struct mxs_pinctrl_soc_data *soc)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *child;
>  	struct mxs_pinctrl_data *d;
> +	int id, i, j = 0;
>  	int ret;
>  
>  	d = devm_kzalloc(&pdev->dev, sizeof(*d), GFP_KERNEL);
> @@ -500,6 +532,26 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
>  	mxs_pinctrl_desc.npins = d->soc->npins;
>  	mxs_pinctrl_desc.name = dev_name(&pdev->dev);
>  
> +	for_each_child_of_node(np, child) {
> +		if (!of_device_is_compatible(child, "fsl,mxs-gpio"))
> +			continue;
I planned to use a phandle list to point to gpio nodes rather than forcing
put gpio nodes under pinctrl node.
I will send out the common patch for discussion.

> +
> +		id = of_alias_get_id(child, "gpio");
> +		if (id < 0 || id > soc->gpio_num_ranges) {
> +			dev_err(&pdev->dev, "invalid gpio id: %d\n", id);
> +			return id;
> +		}
> +
> +		for (i = j; i < soc->gpio_num_ranges; i++) {
I'm wondering this may fail if the gpio nodes are not sequentially listed
in dts file.

> +			struct pinctrl_gpio_range *range = &soc->gpio_ranges[i];
> +			if (range->id == id) {
> +				range->gc = of_node_to_gpiochip(child);
shouldn't check return?

> +				range->base += range->gc->base;
> +				j++;
> +			}
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, d);
>  
>  	ret = mxs_pinctrl_probe_dt(pdev, d);
> @@ -515,6 +567,9 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
>  		goto err;
>  	}
>  
> +	for (i = 0; i < soc->gpio_num_ranges; i++)
> +		pinctrl_add_gpio_range(d->pctl, &soc->gpio_ranges[i]);
> +
>  	return 0;
>  
>  err:
> diff --git a/drivers/pinctrl/pinctrl-mxs.h b/drivers/pinctrl/pinctrl-mxs.h
> index fdd88d0b..7feef15 100644
> --- a/drivers/pinctrl/pinctrl-mxs.h
> +++ b/drivers/pinctrl/pinctrl-mxs.h
> @@ -82,6 +82,8 @@ struct mxs_pinctrl_soc_data {
>  	unsigned nfunctions;
>  	struct mxs_group *groups;
>  	unsigned ngroups;
> +	struct pinctrl_gpio_range *gpio_ranges;
> +	unsigned gpio_num_ranges;
>  };
>  
>  int mxs_pinctrl_probe(struct platform_device *pdev,
> -- 
> 1.7.4.1
> 

Regards
Dong Aisheng

[PATCH] pinctrl: mxs: add gpio range support

[Shawn Guo]

On Tue, May 15, 2012 at 07:51:14PM +0800, Dong Aisheng wrote:
> > +/*
> > + * The .base is initialized as the gpio offset local to the port, and will
> > + * have gpio base of the port added at runtime.
> > + */
> A good idea.
> I was also trying to do this for pinctrl gpio dt improvement but i planned to
> do such things in core since i thought other SoCs may face the same issue.
> Maybe i should send out a RFC patch for open discussion.
> 
Yes, I just had a look on your patch.  It will be good if we can really
implement this at core level. 

> > +static void mxs_gpio_disable_free(struct pinctrl_dev *pctldev,
> > +				  struct pinctrl_gpio_range *range,
> > +				  unsigned pinid)
> > +{
> > +	/* Nothing to do here */
> > +}
> It seems .gpio_disable_free can be optional, so you can remove this
> empty function.
> 
Ok.

> > +
> >  static struct pinmux_ops mxs_pinmux_ops = {
> >  	.get_functions_count = mxs_pinctrl_get_funcs_count,
> >  	.get_function_name = mxs_pinctrl_get_func_name,
> >  	.get_function_groups = mxs_pinctrl_get_func_groups,
> >  	.enable = mxs_pinctrl_enable,
> >  	.disable = mxs_pinctrl_disable,
> > +	.gpio_request_enable = mxs_gpio_request_enable,
> > +	.gpio_disable_free = mxs_gpio_disable_free,
> >  };
> >  
> >  static int mxs_pinconf_get(struct pinctrl_dev *pctldev,
> > @@ -482,7 +512,9 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
> >  				struct mxs_pinctrl_soc_data *soc)
> >  {
> >  	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *child;
> >  	struct mxs_pinctrl_data *d;
> > +	int id, i, j = 0;
> >  	int ret;
> >  
> >  	d = devm_kzalloc(&pdev->dev, sizeof(*d), GFP_KERNEL);
> > @@ -500,6 +532,26 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
> >  	mxs_pinctrl_desc.npins = d->soc->npins;
> >  	mxs_pinctrl_desc.name = dev_name(&pdev->dev);
> >  
> > +	for_each_child_of_node(np, child) {
> > +		if (!of_device_is_compatible(child, "fsl,mxs-gpio"))
> > +			continue;
> I planned to use a phandle list to point to gpio nodes rather than forcing
> put gpio nodes under pinctrl node.

On mxs, the gpio nodes are naturally under pinctrl node, because gpio
controller is part of pin controller and share the same IO space range
with pin controller.

For the phandle approach, there is a patch from Viresh Kumar "gpiolib:
Add of_get_gpio_chip_by_phandle() helper" are currently flowing around
on the list.  But it hasn't been accepted.  The whole pull-request of
"SPEAr pinctrl updates for v-3.5" is being held on this.

http://thread.gmane.org/gmane.linux.ports.arm.kernel/166668/focus=167395

> I will send out the common patch for discussion.
> 
> > +
> > +		id = of_alias_get_id(child, "gpio");
> > +		if (id < 0 || id > soc->gpio_num_ranges) {
> > +			dev_err(&pdev->dev, "invalid gpio id: %d\n", id);
> > +			return id;
> > +		}
> > +
> > +		for (i = j; i < soc->gpio_num_ranges; i++) {
> I'm wondering this may fail if the gpio nodes are not sequentially listed
> in dts file.
> 
This is a mxs specific implementation, and we do have gpio nodes
sequentially listed under pinctrl node.

> > +			struct pinctrl_gpio_range *range = &soc->gpio_ranges[i];
> > +			if (range->id == id) {
> > +				range->gc = of_node_to_gpiochip(child);
> shouldn't check return?
> 
Yes.  Will do.

-- 
Regards,
Shawn

[PATCH] pinctrl: mxs: add gpio range support

[Dong Aisheng]

On Thu, May 17, 2012 at 10:47:15AM +0800, Guo Shawn-R65073 wrote:
...
> > > +	for_each_child_of_node(np, child) {
> > > +		if (!of_device_is_compatible(child, "fsl,mxs-gpio"))
> > > +			continue;
> > I planned to use a phandle list to point to gpio nodes rather than forcing
> > put gpio nodes under pinctrl node.
> 
> On mxs, the gpio nodes are naturally under pinctrl node, because gpio
> controller is part of pin controller and share the same IO space range
> with pin controller.
> 
Well, for that point, it seems reasonable to me.

> For the phandle approach, there is a patch from Viresh Kumar "gpiolib:
> Add of_get_gpio_chip_by_phandle() helper" are currently flowing around
> on the list.  But it hasn't been accepted.  The whole pull-request of
> "SPEAr pinctrl updates for v-3.5" is being held on this.
> 
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/166668/focus=167395
> 
Thanks for the info.

> > I will send out the common patch for discussion.
> > 
> > > +
> > > +		id = of_alias_get_id(child, "gpio");
> > > +		if (id < 0 || id > soc->gpio_num_ranges) {
> > > +			dev_err(&pdev->dev, "invalid gpio id: %d\n", id);
> > > +			return id;
> > > +		}
> > > +
> > > +		for (i = j; i < soc->gpio_num_ranges; i++) {
> > I'm wondering this may fail if the gpio nodes are not sequentially listed
> > in dts file.
> > 
> This is a mxs specific implementation, and we do have gpio nodes
> sequentially listed under pinctrl node.
> 
I'm wondering if dt forces user to define the same device sequentially.
But if not, the driver may be better not assume how the device node are
organized in dts, right?
If we really want do like this, do we need to add some comments in binding doc?

Regards
Dong Aisheng

[PATCH] pinctrl: mxs: add gpio range support

[Shawn Guo]

On Thu, May 17, 2012 at 10:50:26AM +0800, Dong Aisheng wrote:
> I'm wondering if dt forces user to define the same device sequentially.
> But if not, the driver may be better not assume how the device node are
> organized in dts, right?
> If we really want do like this, do we need to add some comments in binding doc?
> 
Yeah, will add a note in binding doc, though the example demonstrates
that already.

-- 
Regards,
Shawn

[PATCH] pinctrl: mxs: add gpio range support

[Stephen Warren]

On 05/14/2012 11:56 PM, Shawn Guo wrote:
> The mxs pins are organized in banks and each bank contains 32 pins.
> i.MX23 has 4 banks in total, and every pin in the first 3 banks has
> gpio function available.  i.MX28 has 7 banks and the first 5 banks
> are capable of gpio mode.
> 
> As the gpio base is runtime determined for each port when booting
> from device tree, the .base of struct pinctrl_gpio_range can only be
> initialized with a offset local to the port, and have to plus gpio base
> of the port at runtime.

> diff --git a/drivers/pinctrl/pinctrl-imx23.c b/drivers/pinctrl/pinctrl-imx23.c

> +static struct pinctrl_gpio_range imx23_gpio_ranges[] = {
> +	{ .name = "gpio", .id = 0, .base = 0, .pin_base = 0,  .npins = 32, },
> +	{ .name = "gpio", .id = 1, .base = 0, .pin_base = 32, .npins = 31, },

Is that last number meant to be 31 or 32?

> +	{ .name = "gpio", .id = 2, .base = 0, .pin_base = 64, .npins = 32, },

> diff --git a/drivers/pinctrl/pinctrl-mxs.c b/drivers/pinctrl/pinctrl-mxs.c

> @@ -500,6 +532,26 @@ int __devinit mxs_pinctrl_probe(struct platform_device *pdev,
>  	mxs_pinctrl_desc.npins = d->soc->npins;
>  	mxs_pinctrl_desc.name = dev_name(&pdev->dev);
>  
> +	for_each_child_of_node(np, child) {
> +		if (!of_device_is_compatible(child, "fsl,mxs-gpio"))
> +			continue;
> +
> +		id = of_alias_get_id(child, "gpio");

Hmmm. I'm not sure if using /aliases is the correct approach here.
Rather, shouldn't the pin controller node have an explicit list of GPIO
phandles in the order they're needed.

In other words, rather than:

	aliases {
		gpio0 = "/some/path/gpio at 0";
		gpio1 = "/some/path/gpio at 1";
		...
	};

Instead have the following property in the pin controller's own node:

gpio-controllers = <&imx_gpio0 &imx_gpio1 ...>;

That seems a little more direct/explicit; relying on /aliases seems
slightly fragile - what if someone wants to change those aliases and
doesn't realize the implications?