[PATCH v2 0/9] support pinctrl single in arch pxa/mmp

[PATCH v2 0/9] support pinctrl single in arch pxa/mmp

[Haojian Zhuang]

Changelog:

v2:
1. Remove "pinctrl-single,gpio-mask". Since GPIO function is one of the
mux function in the pinmux register of both OMAP and PXA/MMP silicons.
Use "pinctrl-single,function-mask" instead.
2. Remove "pinctrl-single,gpio-enable" & "pinctrl-single,gpio-disable".
Use "pinctrl-single,gpio-func" instead. Because GPIO mode is only one
of the mux functions in the pinmux register. Defining "gpio-enable" &
"gpio-disable" are redundant.
3. Define register with __iomem, not u32 type.
4. Remove "pinctrl-single,input-schmit-shift",
"pinctrl-single,power-source-shift", "pinctrl-single,bias-shift". All
these properties could be calculated by mask fields.
5. Return -EPROBE_DEFER if pinmux could be got in device driver. And
the device driver would be probed again deferred.

[PATCH v2 1/9] ARM: mmp: select pinctrl driver

[Haojian Zhuang]

Pinctrl driver is necessary for MMP DT & MMP2 DT platforms.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
index 178d4da..ebdda83 100644
--- a/arch/arm/mach-mmp/Kconfig
+++ b/arch/arm/mach-mmp/Kconfig
@@ -89,6 +89,8 @@ config MACH_MMP_DT
 	select CPU_PXA168
 	select CPU_PXA910
 	select USE_OF
+	select PINCTRL
+	select PINCTRL_SINGLE
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree. Needn't select any other machine while
@@ -99,6 +101,8 @@ config MACH_MMP2_DT
 	depends on !CPU_MOHAWK
 	select CPU_MMP2
 	select USE_OF
+	select PINCTRL
+	select PINCTRL_SINGLE
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree.
-- 
1.7.0.4

[PATCH v2 1/9] ARM: mmp: select pinctrl driver

[Linus Walleij]

On Mon, Oct 22, 2012 at 6:08 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Pinctrl driver is necessary for MMP DT & MMP2 DT platforms.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/9] pinctrl: single: support gpio request and free

[Haojian Zhuang]

Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
Each pin binds to one register. A lot of pins could be configured
as gpio.

Now add three properties in below.
pinctrl-single,gpio-ranges: gpio range array
pinctrl-single,gpio: <gpio base, npins in range, pin base in range>
pinctrl-single,gpio-func: <gpio function value in mux>

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/pinctrl-single.c |   94 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 726a729..6a0b24b 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -28,8 +28,10 @@
 #define DRIVER_NAME			"pinctrl-single"
 #define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
 #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
+#define PCS_GPIO_FUNC_NAME		"pinctrl-single,gpio-func"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
+#define PCS_MAX_GPIO_VALUES		3
 
 /**
  * struct pcs_pingroup - pingroups for a function
@@ -77,6 +79,18 @@ struct pcs_function {
 };
 
 /**
+ * struct pcs_gpio_range - pinctrl gpio range
+ * @range:	subrange of the GPIO number space
+ * @gpio_func:	gpio function value in the pinmux register
+ * @func_en:	need to handle gpio function in the pinmux register
+ */
+struct pcs_gpio_range {
+	struct pinctrl_gpio_range range;
+	int gpio_func;
+	unsigned func_en:1;
+};
+
+/**
  * struct pcs_data - wrapper for data needed by pinctrl framework
  * @pa:		pindesc array
  * @cur:	index to current element
@@ -123,8 +137,10 @@ struct pcs_name {
  * @ftree:	function index radix tree
  * @pingroups:	list of pingroups
  * @functions:	list of functions
+ * @ranges:	list of gpio ranges
  * @ngroups:	number of pingroups
  * @nfuncs:	number of functions
+ * @nranges:	number of gpio ranges
  * @desc:	pin controller descriptor
  * @read:	register read function to use
  * @write:	register write function to use
@@ -148,8 +164,10 @@ struct pcs_device {
 	struct radix_tree_root ftree;
 	struct list_head pingroups;
 	struct list_head functions;
+	struct list_head ranges;
 	unsigned ngroups;
 	unsigned nfuncs;
+	unsigned nranges;
 	struct pinctrl_desc desc;
 	unsigned (*read)(void __iomem *reg);
 	void (*write)(unsigned val, void __iomem *reg);
@@ -403,9 +421,27 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
 }
 
 static int pcs_request_gpio(struct pinctrl_dev *pctldev,
-			struct pinctrl_gpio_range *range, unsigned offset)
+			    struct pinctrl_gpio_range *range, unsigned pin)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_gpio_range *gpio = NULL;
+	int end, mux_bytes;
+	unsigned data;
+
+	gpio = container_of(range, struct pcs_gpio_range, range);
+	if (!gpio->func_en)
+		return 0;
+	end = range->pin_base + range->npins - 1;
+	if (pin < range->pin_base || pin > end) {
+		dev_err(pctldev->dev, "pin %d isn't in the range of "
+			"%d to %d\n", pin, range->pin_base, end);
+		return -EINVAL;
+	}
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	data = pcs_readl(pcs->base + pin * mux_bytes) & ~pcs->fmask;
+	data |= gpio->gpio_func;
+	pcs_writel(data, pcs->base + pin * mux_bytes);
+	return 0;
 }
 
 static struct pinmux_ops pcs_pinmux_ops = {
@@ -879,6 +915,55 @@ static void pcs_free_resources(struct pcs_device *pcs)
 
 static struct of_device_id pcs_of_match[];
 
+static int __devinit pcs_add_gpio_range(struct device_node *node,
+					struct pcs_device *pcs)
+{
+	struct pcs_gpio_range *gpio;
+	struct device_node *np;
+	const __be32 *list;
+	const char list_name[] = "pinctrl-single,gpio-ranges";
+	const char name[] = "pinctrl-single";
+	u32 gpiores[PCS_MAX_GPIO_VALUES];
+	int ret, size, i, mux_bytes = 0;
+
+	list = of_get_property(node, list_name, &size);
+	if (!list)
+		return -ENOENT;
+	size = size / sizeof(*list);
+	for (i = 0; i < size; i++) {
+		np = of_parse_phandle(node, list_name, i);
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
+						 gpiores, PCS_MAX_GPIO_VALUES);
+		if (ret < 0)
+			return -ENOENT;
+		gpio = devm_kzalloc(pcs->dev, sizeof(*gpio), GFP_KERNEL);
+		if (!gpio) {
+			dev_err(pcs->dev, "failed to allocate pcs gpio\n");
+			return -ENOMEM;
+		}
+		gpio->range.id = i;
+		gpio->range.base = gpiores[0];
+		gpio->range.npins = gpiores[1];
+		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
+		mux_bytes = pcs->width / BITS_PER_BYTE;
+		gpio->range.pin_base = gpiores[2] / mux_bytes;
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32(np, PCS_GPIO_FUNC_NAME,
+					   &gpio->gpio_func);
+		if (!ret)
+			gpio->func_en = 1;
+
+		mutex_lock(&pcs->mutex);
+		list_add_tail(&gpio->range.node, &pcs->ranges);
+		pcs->nranges++;
+		mutex_unlock(&pcs->mutex);
+
+		pinctrl_add_gpio_range(pcs->pctl, &gpio->range);
+	}
+	return 0;
+}
+
 static int __devinit pcs_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -900,6 +985,7 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	mutex_init(&pcs->mutex);
 	INIT_LIST_HEAD(&pcs->pingroups);
 	INIT_LIST_HEAD(&pcs->functions);
+	INIT_LIST_HEAD(&pcs->ranges);
 
 	PCS_GET_PROP_U32("pinctrl-single,register-width", &pcs->width,
 			 "register width not specified\n");
@@ -975,6 +1061,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pcs_add_gpio_range(np, pcs);
+	if (ret < 0)
+		return ret;
+
 	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
 		 pcs->desc.npins, pcs->base, pcs->size);
 
-- 
1.7.0.4

[PATCH v2 2/9] pinctrl: single: support gpio request and free

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121022 09:11]:
> Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
> Each pin binds to one register. A lot of pins could be configured
> as gpio.
> 
> Now add three properties in below.
> pinctrl-single,gpio-ranges: gpio range array
> pinctrl-single,gpio: <gpio base, npins in range, pin base in range>
> pinctrl-single,gpio-func: <gpio function value in mux>
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  drivers/pinctrl/pinctrl-single.c |   94 +++++++++++++++++++++++++++++++++++++-
>  1 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 726a729..6a0b24b 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -28,8 +28,10 @@
>  #define DRIVER_NAME			"pinctrl-single"
>  #define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
>  #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
> +#define PCS_GPIO_FUNC_NAME		"pinctrl-single,gpio-func"

I think we can now get rid of these defines, I initially added
them as we had a bit hard time finding a suitable name for the
driver. These are only used in one location, so let's not add
new ones here.

>  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> -			struct pinctrl_gpio_range *range, unsigned offset)
> +			    struct pinctrl_gpio_range *range, unsigned pin)
>  {
> -	return -ENOTSUPP;
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	struct pcs_gpio_range *gpio = NULL;
> +	int end, mux_bytes;
> +	unsigned data;
> +
> +	gpio = container_of(range, struct pcs_gpio_range, range);
> +	if (!gpio->func_en)
> +		return 0;
> +	end = range->pin_base + range->npins - 1;
> +	if (pin < range->pin_base || pin > end) {
> +		dev_err(pctldev->dev, "pin %d isn't in the range of "
> +			"%d to %d\n", pin, range->pin_base, end);
> +		return -EINVAL;
> +	}
> +	mux_bytes = pcs->width / BITS_PER_BYTE;
> +	data = pcs_readl(pcs->base + pin * mux_bytes) & ~pcs->fmask;
> +	data |= gpio->gpio_func;
> +	pcs_writel(data, pcs->base + pin * mux_bytes);
> +	return 0;
>  }

I think I already commented on this one.. Is this safe if you don't
have GPIOs configured? Or should you return -ENODEV in that case?

> +static int __devinit pcs_add_gpio_range(struct device_node *node,
> +					struct pcs_device *pcs)
> +{
> +	struct pcs_gpio_range *gpio;
> +	struct device_node *np;
> +	const __be32 *list;
> +	const char list_name[] = "pinctrl-single,gpio-ranges";
> +	const char name[] = "pinctrl-single";
> +	u32 gpiores[PCS_MAX_GPIO_VALUES];
> +	int ret, size, i, mux_bytes = 0;
> +
> +	list = of_get_property(node, list_name, &size);
> +	if (!list)
> +		return -ENOENT;

Here you should return 0 if not found, otherwise things go bad for
me at least as I don't have GPIOs configured. See more below.

> +		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
> +		pinctrl_add_gpio_range(pcs->pctl, &gpio->range);

Just checking.. These get released with pinctrl_unregister() when
unloading, right? And nothing else to free in pinctrl-single.c
either?

> @@ -975,6 +1061,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
>  		goto free;
>  	}
>  
> +	ret = pcs_add_gpio_range(np, pcs);
> +	if (ret < 0)
> +		return ret;
> +

Here you need to goto free on error. Maybe just fold in the
attached fix if that looks OK to you:

--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -930,7 +930,7 @@ static int __devinit pcs_add_gpio_range(struct device_node *node,
 
 	list = of_get_property(node, list_name, &size);
 	if (!list)
-		return -ENOENT;
+		return 0;
 	size = size / sizeof(*list);
 	for (i = 0; i < size; i++) {
 		np = of_parse_phandle(node, list_name, i);
@@ -1065,7 +1065,7 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 
 	ret = pcs_add_gpio_range(np, pcs);
 	if (ret < 0)
-		return ret;
+		goto free;
 
 	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
 		 pcs->desc.npins, pcs->base, pcs->size);

[PATCH v2 2/9] pinctrl: single: support gpio request and free

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121022 13:29]:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 09:11]:
> > --- a/drivers/pinctrl/pinctrl-single.c
> > +++ b/drivers/pinctrl/pinctrl-single.c
> > @@ -28,8 +28,10 @@
> >  #define DRIVER_NAME			"pinctrl-single"
> >  #define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
> >  #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
> > +#define PCS_GPIO_FUNC_NAME		"pinctrl-single,gpio-func"
> 
> I think we can now get rid of these defines, I initially added
> them as we had a bit hard time finding a suitable name for the
> driver. These are only used in one location, so let's not add
> new ones here.
> 
> >  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> > -			struct pinctrl_gpio_range *range, unsigned offset)
> > +			    struct pinctrl_gpio_range *range, unsigned pin)
> >  {
> > -	return -ENOTSUPP;
> > +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> > +	struct pcs_gpio_range *gpio = NULL;
> > +	int end, mux_bytes;
> > +	unsigned data;
> > +
> > +	gpio = container_of(range, struct pcs_gpio_range, range);
> > +	if (!gpio->func_en)
> > +		return 0;
> > +	end = range->pin_base + range->npins - 1;
> > +	if (pin < range->pin_base || pin > end) {
> > +		dev_err(pctldev->dev, "pin %d isn't in the range of "
> > +			"%d to %d\n", pin, range->pin_base, end);
> > +		return -EINVAL;
> > +	}
> > +	mux_bytes = pcs->width / BITS_PER_BYTE;
> > +	data = pcs_readl(pcs->base + pin * mux_bytes) & ~pcs->fmask;
> > +	data |= gpio->gpio_func;
> > +	pcs_writel(data, pcs->base + pin * mux_bytes);
> > +	return 0;
> >  }
> 
> I think I already commented on this one.. Is this safe if you don't
> have GPIOs configured? Or should you return -ENODEV in that case?

Oops also you should not use pcs_readl/pcs_writel in the driver
directly but use pcs_read instead as you can have register width other
than 32-bits.

Regards,

Tony

[PATCH v2 2/9] pinctrl: single: support gpio request and free

[Haojian Zhuang]

On Tue, Oct 23, 2012 at 5:37 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [121022 13:29]:
>> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 09:11]:
>> > --- a/drivers/pinctrl/pinctrl-single.c
>> > +++ b/drivers/pinctrl/pinctrl-single.c
>> > @@ -28,8 +28,10 @@
>> >  #define DRIVER_NAME                        "pinctrl-single"
>> >  #define PCS_MUX_PINS_NAME          "pinctrl-single,pins"
>> >  #define PCS_MUX_BITS_NAME          "pinctrl-single,bits"
>> > +#define PCS_GPIO_FUNC_NAME         "pinctrl-single,gpio-func"
>>
>> I think we can now get rid of these defines, I initially added
>> them as we had a bit hard time finding a suitable name for the
>> driver. These are only used in one location, so let's not add
>> new ones here.
>>
>> >  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
>> > -                   struct pinctrl_gpio_range *range, unsigned offset)
>> > +                       struct pinctrl_gpio_range *range, unsigned pin)
>> >  {
>> > -   return -ENOTSUPP;
>> > +   struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
>> > +   struct pcs_gpio_range *gpio = NULL;
>> > +   int end, mux_bytes;
>> > +   unsigned data;
>> > +
>> > +   gpio = container_of(range, struct pcs_gpio_range, range);
>> > +   if (!gpio->func_en)
>> > +           return 0;
>> > +   end = range->pin_base + range->npins - 1;
>> > +   if (pin < range->pin_base || pin > end) {
>> > +           dev_err(pctldev->dev, "pin %d isn't in the range of "
>> > +                   "%d to %d\n", pin, range->pin_base, end);
>> > +           return -EINVAL;
>> > +   }
>> > +   mux_bytes = pcs->width / BITS_PER_BYTE;
>> > +   data = pcs_readl(pcs->base + pin * mux_bytes) & ~pcs->fmask;
>> > +   data |= gpio->gpio_func;
>> > +   pcs_writel(data, pcs->base + pin * mux_bytes);
>> > +   return 0;
>> >  }
>>
>> I think I already commented on this one.. Is this safe if you don't
>> have GPIOs configured? Or should you return -ENODEV in that case?
>
> Oops also you should not use pcs_readl/pcs_writel in the driver
> directly but use pcs_read instead as you can have register width other
> than 32-bits.
>

I think you're meaning pcs->read(). I'll use this interface.

[PATCH v2 2/9] pinctrl: single: support gpio request and free

[Haojian Zhuang]

On Tue, Oct 23, 2012 at 4:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 09:11]:
>> Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
>> Each pin binds to one register. A lot of pins could be configured
>> as gpio.
>>
>> Now add three properties in below.
>> pinctrl-single,gpio-ranges: gpio range array
>> pinctrl-single,gpio: <gpio base, npins in range, pin base in range>
>> pinctrl-single,gpio-func: <gpio function value in mux>
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>> ---
>>  drivers/pinctrl/pinctrl-single.c |   94 +++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 92 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
>> index 726a729..6a0b24b 100644
>> --- a/drivers/pinctrl/pinctrl-single.c
>> +++ b/drivers/pinctrl/pinctrl-single.c
>> @@ -28,8 +28,10 @@
>>  #define DRIVER_NAME                  "pinctrl-single"
>>  #define PCS_MUX_PINS_NAME            "pinctrl-single,pins"
>>  #define PCS_MUX_BITS_NAME            "pinctrl-single,bits"
>> +#define PCS_GPIO_FUNC_NAME           "pinctrl-single,gpio-func"
>
> I think we can now get rid of these defines, I initially added
> them as we had a bit hard time finding a suitable name for the
> driver. These are only used in one location, so let's not add
> new ones here.
>
I'll fix.

>>  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
>> -                     struct pinctrl_gpio_range *range, unsigned offset)
>> +                         struct pinctrl_gpio_range *range, unsigned pin)
>>  {
>> -     return -ENOTSUPP;
>> +     struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
>> +     struct pcs_gpio_range *gpio = NULL;
>> +     int end, mux_bytes;
>> +     unsigned data;
>> +
>> +     gpio = container_of(range, struct pcs_gpio_range, range);
>> +     if (!gpio->func_en)
>> +             return 0;
>> +     end = range->pin_base + range->npins - 1;
>> +     if (pin < range->pin_base || pin > end) {
>> +             dev_err(pctldev->dev, "pin %d isn't in the range of "
>> +                     "%d to %d\n", pin, range->pin_base, end);
>> +             return -EINVAL;
>> +     }
>> +     mux_bytes = pcs->width / BITS_PER_BYTE;
>> +     data = pcs_readl(pcs->base + pin * mux_bytes) & ~pcs->fmask;
>> +     data |= gpio->gpio_func;
>> +     pcs_writel(data, pcs->base + pin * mux_bytes);
>> +     return 0;
>>  }
>
> I think I already commented on this one.. Is this safe if you don't
> have GPIOs configured? Or should you return -ENODEV in that case?
>
OK. I'll use ENOTSUPP instead.

 +     gpio = container_of(range, struct pcs_gpio_range, range);
 +     if (!gpio->func_en)
 +             return -ENOTSUPP;

>> +static int __devinit pcs_add_gpio_range(struct device_node *node,
>> +                                     struct pcs_device *pcs)
>> +{
>> +     struct pcs_gpio_range *gpio;
>> +     struct device_node *np;
>> +     const __be32 *list;
>> +     const char list_name[] = "pinctrl-single,gpio-ranges";
>> +     const char name[] = "pinctrl-single";
>> +     u32 gpiores[PCS_MAX_GPIO_VALUES];
>> +     int ret, size, i, mux_bytes = 0;
>> +
>> +     list = of_get_property(node, list_name, &size);
>> +     if (!list)
>> +             return -ENOENT;
>
> Here you should return 0 if not found, otherwise things go bad for
> me at least as I don't have GPIOs configured. See more below.
>
OK. Use return 0 instead.

>> +             gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
>> +             pinctrl_add_gpio_range(pcs->pctl, &gpio->range);
>
> Just checking.. These get released with pinctrl_unregister() when
> unloading, right? And nothing else to free in pinctrl-single.c
> either?
>
I'll fix.

>> @@ -975,6 +1061,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
>>               goto free;
>>       }
>>
>> +     ret = pcs_add_gpio_range(np, pcs);
>> +     if (ret < 0)
>> +             return ret;
>> +
>
> Here you need to goto free on error. Maybe just fold in the
> attached fix if that looks OK to you:
>
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -930,7 +930,7 @@ static int __devinit pcs_add_gpio_range(struct device_node *node,
>
>         list = of_get_property(node, list_name, &size);
>         if (!list)
> -               return -ENOENT;
> +               return 0;
>         size = size / sizeof(*list);
>         for (i = 0; i < size; i++) {
>                 np = of_parse_phandle(node, list_name, i);
> @@ -1065,7 +1065,7 @@ static int __devinit pcs_probe(struct platform_device *pdev)
>
>         ret = pcs_add_gpio_range(np, pcs);
>         if (ret < 0)
> -               return ret;
> +               goto free;
>
>         dev_info(pcs->dev, "%i pins at pa %p size %u\n",
>                  pcs->desc.npins, pcs->base, pcs->size);

I'll take your fix into this patch.

[PATCH v2 3/9] pinctrl: single: support pinconf generic

[Haojian Zhuang]

Add pinconf generic support with POWER SOURCE, BIAS PULL.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/Kconfig          |    1 +
 drivers/pinctrl/pinctrl-single.c |  276 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 266 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7bf914d..e9f2d2d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -139,6 +139,7 @@ config PINCTRL_SINGLE
 	depends on OF
 	select PINMUX
 	select PINCONF
+	select GENERIC_PINCONF
 	help
 	  This selects the device tree based generic pinctrl driver.
 
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 6a0b24b..a20da78 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
@@ -29,6 +30,9 @@
 #define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
 #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #define PCS_GPIO_FUNC_NAME		"pinctrl-single,gpio-func"
+#define PCS_BIAS_NAME			"pinctrl-single,bias"
+#define PCS_POWER_SOURCE_NAME		"pinctrl-single,power-source"
+#define PCS_SCHMITT_NAME		"pinctrl-single,input-schmitt"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
 #define PCS_MAX_GPIO_VALUES		3
@@ -131,6 +135,15 @@ struct pcs_name {
  * @fshift:	function register shift
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
+ * @bmask:	bias mask in pinconf
+ * @bshift:	bias register shift
+ * @bdis:	bias disable value in pinconf
+ * @bpullup:	bias pull up value in pinconf
+ * @bpulldown:	bias pull down value in pinconf
+ * @ismask:	input schmitt mask in pinconf
+ * @isshift:	input schmitt register shift
+ * @psmask:	power source mask in pinconf
+ * @psshift:	power source register shift
  * @names:	array of register names for pins
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
@@ -157,6 +170,15 @@ struct pcs_device {
 	unsigned fshift;
 	unsigned foff;
 	unsigned fmax;
+	unsigned bmask;
+	unsigned bshift;
+	unsigned bdis;
+	unsigned bpullup;
+	unsigned bpulldown;
+	unsigned ismask;
+	unsigned isshift;
+	unsigned psmask;
+	unsigned psshift;
 	bool bits_per_mux;
 	struct pcs_name *names;
 	struct pcs_data pins;
@@ -453,28 +475,163 @@ static struct pinmux_ops pcs_pinmux_ops = {
 	.gpio_request_enable = pcs_request_gpio,
 };
 
+static void pcs_free_pingroups(struct pcs_device *pcs);
+
 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long *config)
 {
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned data;
+	u32 offset;
+
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	data = pcs_readl(pcs->base + offset);
+
+	switch (param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->psmask;
+		data = data >> pcs->psshift;
+		*config = data;
+		return 0;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bdis == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bdis)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpullup == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpullup)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpulldown == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpulldown)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	default:
+		break;
+	}
 	return -ENOTSUPP;
 }
 
 static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	unsigned ret, mask = ~0UL;
+	u32 offset, data;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	ret = pcs_readl(pcs->base + offset) & ~mask;
+	pcs_writel(ret | data, pcs->base + offset);
+	return 0;
 }
 
 static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long *config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_pingroup *pins;
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	return pcs_pinconf_get(pctldev, pins->gpins[0], config);
 }
 
 static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	struct pcs_pingroup *pins;
+	u32 offset, data;
+	unsigned ret, mask = ~0UL;
+	int i;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	for (i = 0; i < pins->ngpins; i++) {
+		offset = pins->gpins[i] * (pcs->width / BITS_PER_BYTE);
+		ret = pcs_readl(pcs->base + offset) & ~mask;
+		pcs_writel(ret | data, pcs->base + offset);
+	}
+	return 0;
 }
 
 static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
@@ -488,6 +645,7 @@ static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static struct pinconf_ops pcs_pinconf_ops = {
+	.is_generic = true,
 	.pin_config_get = pcs_pinconf_get,
 	.pin_config_set = pcs_pinconf_set,
 	.pin_config_group_get = pcs_pinconf_group_get,
@@ -689,6 +847,7 @@ static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
  * @pcs: pinctrl driver instance
  * @np: device node of the mux entry
  * @map: map entry
+ * @num_configs: number of pin configurations
  * @pgnames: pingroup names
  *
  * Note that this binding currently supports only sets of one register + value.
@@ -705,12 +864,16 @@ static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 						struct device_node *np,
 						struct pinctrl_map **map,
+						unsigned num_configs,
 						const char **pgnames)
 {
 	struct pcs_func_vals *vals;
+	struct pinctrl_map *p = *map;
 	const __be32 *mux;
 	int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM;
 	struct pcs_function *function;
+	unsigned long *config;
+	u32 value;
 
 	if (pcs->bits_per_mux) {
 		params = 3;
@@ -773,12 +936,42 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (res < 0)
 		goto free_function;
 
-	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-	(*map)->data.mux.group = np->name;
-	(*map)->data.mux.function = np->name;
+	p->type = PIN_MAP_TYPE_MUX_GROUP;
+	p->data.mux.group = np->name;
+	p->data.mux.function = np->name;
+
+	if (!num_configs)
+		return 0;
+	config = devm_kzalloc(pcs->dev, sizeof(*config) * num_configs,
+			      GFP_KERNEL);
+	if (!config) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	index = 0;
+	if (!of_property_read_u32(np, PCS_SCHMITT_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_BIAS_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_POWER_SOURCE_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_POWER_SOURCE,
+						 value & 0xffff);
+	p++;
+	p->type = PIN_MAP_TYPE_CONFIGS_GROUP;
+	p->data.configs.group_or_pin = np->name;
+	p->data.configs.configs = config;
+	p->data.configs.num_configs = num_configs;
 
 	return 0;
 
+free_pingroup:
+	pcs_free_pingroups(pcs);
+
 free_function:
 	pcs_remove_function(pcs, function);
 
@@ -790,6 +983,30 @@ free_vals:
 
 	return res;
 }
+
+static int pcs_dt_check_maps(struct device_node *np, unsigned *num_maps,
+			     unsigned *num_configs)
+{
+	unsigned size;
+
+	*num_maps = 0;
+	*num_configs = 0;
+	if (of_get_property(np, PCS_MUX_PINS_NAME, &size)
+		|| of_get_property(np, PCS_MUX_BITS_NAME, &size))
+		(*num_maps)++;
+	if (of_get_property(np, PCS_SCHMITT_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_BIAS_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_POWER_SOURCE_NAME, &size))
+		(*num_configs)++;
+	if (*num_configs)
+		(*num_maps)++;
+	if (!(*num_maps))
+		return -EINVAL;
+	return 0;
+}
+
 /**
  * pcs_dt_node_to_map() - allocates and parses pinctrl maps
  * @pctldev: pinctrl instance
@@ -803,29 +1020,32 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 {
 	struct pcs_device *pcs;
 	const char **pgnames;
+	unsigned num_configs;
 	int ret;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	*map = devm_kzalloc(pcs->dev, sizeof(**map), GFP_KERNEL);
+	ret = pcs_dt_check_maps(np_config, num_maps, &num_configs);
+	if (ret)
+		return ret;
+
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * (*num_maps), GFP_KERNEL);
 	if (!map)
 		return -ENOMEM;
 
-	*num_maps = 0;
-
 	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames), GFP_KERNEL);
 	if (!pgnames) {
 		ret = -ENOMEM;
 		goto free_map;
 	}
 
-	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map, pgnames);
+	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map,
+					  num_configs, pgnames);
 	if (ret < 0) {
 		dev_err(pcs->dev, "no pins entries for %s\n",
 			np_config->name);
 		goto free_pgnames;
 	}
-	*num_maps = 1;
 
 	return 0;
 
@@ -1003,6 +1223,40 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	pcs->bits_per_mux = of_property_read_bool(np,
 						  "pinctrl-single,bit-per-mux");
 
+	ret = of_property_read_u32(np, "pinctrl-single,power-source-mask",
+					&pcs->psmask);
+	if (ret) {
+		pcs->psmask = PCS_OFF_DISABLED;
+		pcs->psshift = PCS_OFF_DISABLED;
+	} else
+		pcs->psshift = ffs(pcs->psmask) - 1;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-mask",
+					&pcs->bmask);
+	if (ret) {
+		pcs->bmask = PCS_OFF_DISABLED;
+		pcs->bshift = PCS_OFF_DISABLED;
+	} else
+		pcs->bshift = ffs(pcs->bmask) - 1;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-disable",
+					&pcs->bdis);
+	if (ret)
+		pcs->bdis = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-up",
+					&pcs->bpullup);
+	if (ret)
+		pcs->bpullup = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-down",
+					&pcs->bpulldown);
+	if (ret)
+		pcs->bpulldown = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,input-schmitt-mask",
+					&pcs->ismask);
+	if (ret) {
+		pcs->ismask = PCS_OFF_DISABLED;
+		pcs->isshift = PCS_OFF_DISABLED;
+	} else
+		pcs->isshift = ffs(pcs->ismask) - 1;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(pcs->dev, "could not get resource\n");
-- 
1.7.0.4

[PATCH v2 4/9] ARM: dts: support pinctrl single in pxa910

[Haojian Zhuang]

Add pinctrl-single support with device tree in pxa910 dkb platform.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 arch/arm/boot/dts/pxa910-dkb.dts |  187 +++++++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/pxa910.dtsi    |   78 ++++++++++++++++
 2 files changed, 264 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/pxa910-dkb.dts b/arch/arm/boot/dts/pxa910-dkb.dts
index 595492a..394396a 100644
--- a/arch/arm/boot/dts/pxa910-dkb.dts
+++ b/arch/arm/boot/dts/pxa910-dkb.dts
@@ -24,10 +24,195 @@
 
 	soc {
 		apb at d4000000 {
-			uart1: uart at d4017000 {
+			pmx: pinmux at d401e000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&board_pins>;
+
+				board_pins: pinmux_board_pins {
+					/* pins not owned by device driver */
+				};
+				uart1_pins: pinmux_uart1_pins {
+					pinctrl-single,pins = <
+						0x198 0x6	/* GPIO47_UART1_RXD */
+						0x19c 0x6	/* GPIO48_UART1_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0x6>;
+				};
+				uart2_pins: pinmux_uart2_pins {
+					pinctrl-single,pins = <
+						0x150 0x4	/* GPIO29_UART2_CTS */
+						0x154 0x4	/* GPIO30_UART2_RTS */
+						0x158 0x4	/* GPIO31_UART2_TXD */
+						0x15c 0x4	/* GPIO32_UART2_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				uart3_pins: pinmux_uart3_pins {
+					pinctrl-single,pins = <
+						0x188 0x7	/* GPIO43_UART3_RXD */
+						0x18c 0x7	/* GPIO44_UART3_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				twsi1_pins: pinmux_twsi1_pins {
+					pinctrl-single,pins = <
+						0x1b0 0x2	/* GPIO53_TWSI_SCL */
+						0x1b4 0x2	/* GPIO54_TWSI_SDA */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nand_pins: pinmux_nand_pins {
+					pinctrl-single,pins = <
+						0x040 0x0	/* ND_IO0 */
+						0x03c 0x0	/* ND_IO1 */
+						0x038 0x0	/* ND_IO2 */
+						0x034 0x0	/* ND_IO3 */
+						0x030 0x0	/* ND_IO4 */
+						0x02c 0x0	/* ND_IO5 */
+						0x028 0x0	/* ND_IO6 */
+						0x024 0x0	/* ND_IO7 */
+						0x020 0x0	/* ND_IO8 */
+						0x01c 0x0	/* ND_IO9 */
+						0x018 0x0	/* ND_IO10 */
+						0x014 0x0	/* ND_IO11 */
+						0x010 0x0	/* ND_IO12 */
+						0x00c 0x0	/* ND_IO13 */
+						0x008 0x0	/* ND_IO14 */
+						0x004 0x0	/* ND_IO15 */
+						0x044 0x0	/* ND_nCS0 */
+						0x060 0x1	/* ND_ALE */
+						0x05c 0x0	/* ND_CLE */
+						0x054 0x1	/* ND_nWE */
+						0x058 0x1	/* ND_nRE */
+						0x068 0x0	/* ND_RDY0 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_ldata_pins: pinmux_mmc1_ldata_pins {
+					pinctrl-single,pins = <
+						0x0a0 0x0	/* MMC1_DATA0 */
+						0x09c 0x0	/* MMC1_DATA1 */
+						0x098 0x0	/* MMC1_DATA2 */
+						0x094 0x0	/* MMC1_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_hdata_pins: pinmux_mmc1_hdata_pins {
+					pinctrl-single,pins = <
+						0x090 0x0	/* MMC1_DATA4 */
+						0x08c 0x0	/* MMC1_DATA5 */
+						0x088 0x0	/* MMC1_DATA6 */
+						0x084 0x0	/* MMC1_DATA7 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_clk_pins: pinmux_mmc1_clk_pins {
+					pinctrl-single,pins = <
+						0x0a4 0x0	/* MMC1_CMD */
+						0x0a8 0x0	/* MMC1_CLK */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_cd_pins: pinmux_mmc1_cd_pins {
+					pinctrl-single,pins = <
+						0x0ac 0x0	/* MMC1_CD */
+						0x0b0 0x0	/* MMC1_WP */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc2_pins: pinmux_mmc2_pins {
+					pinctrl-single,pins = <
+						0x180 0x1	/* MMC2_CMD */
+						0x184 0x1	/* MMC2_CLK */
+						0x17c 0x1	/* MMC2_DATA0 */
+						0x178 0x1	/* MMC2_DATA1 */
+						0x174 0x1	/* MMC2_DATA2 */
+						0x170 0x1	/* MMC2_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				w1_pins: pinmux_w1_pins {
+					pinctrl-single,pins = <
+						0x0cc 0x2	/* CLK_REQ_W1 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				ssp1_pins: pinmux_ssp1_pins {
+					pinctrl-single,pins = <
+						0x130 0x1	/* GPIO21_SSP1_SCLK */
+						0x134 0x1	/* GPIO22_SSP1_FRM */
+						0x138 0x1	/* GPIO23_SSP1_TXD */
+						0x13c 0x1	/* GPIO24_SSP1_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				keypad_pins: pinmux_keypad_pins {
+					pinctrl-single,pins = <
+						0x0dc 0x1	/* GPIO0_MKIN0 */
+						0x0e0 0x1	/* GPIO1_MKOUT0 */
+						0x0e4 0x1	/* GPIO2_MKIN1 */
+						0x0e8 0x1	/* GPIO3_MKOUT1 */
+						0x0ec 0x1	/* GPIO4_MKIN2 */
+						0x0f0 0x1	/* GPIO5_MKOUT2 */
+						0x0f4 0x1	/* GPIO6_MKIN3 */
+						0x0f8 0x1	/* GPIO7_MKOUT3 */
+						0x0fc 0x1	/* GPIO8_MKIN4 */
+						0x100 0x1	/* GPIO9_MKOUT4 */
+						0x10c 0x1	/* GPIO12_MKIN6 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nfc_pins: pinmux_nfc_pins {
+					pinctrl-single,pins = <
+						0x120 0x0	/* GPIO17 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				wlan_pins: pinmux_wlan_pins {
+					pinctrl-single,pins = <
+						0x114 0x0	/* GPIO14 */
+						0x12c 0x0	/* GPIO20 */
+						0x160 0x0	/* GPIO33 */
+						0x164 0x0	/* GPIO34 */
+						0x168 0x0	/* GPIO35 */
+						0x16c 0x0	/* GPIO36 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+			};
+			uart1: uart at d4017000 {	/* FFUART */
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart1_pins>;
+				status = "okay";
+			};
+			uart2: uart at d4018000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart2_pins>;
+				status = "okay";
+			};
+			uart3: uart at d4036000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart3_pins>;
 				status = "okay";
 			};
 			twsi1: i2c at d4011000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&twsi1_pins>;
 				status = "okay";
 
 				pmic: 88pm860x at 34 {
diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi
index 825aaca..cf807e8 100644
--- a/arch/arm/boot/dts/pxa910.dtsi
+++ b/arch/arm/boot/dts/pxa910.dtsi
@@ -54,6 +54,84 @@
 			reg = <0xd4000000 0x00200000>;
 			ranges;
 
+			pmx: pinmux at d401e000 {
+				compatible = "pinctrl-single";
+				reg = <0xd401e000 0x0330>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				pinctrl-single,register-width = <32>;
+				pinctrl-single,function-mask = <7>;
+				pinctrl-single,gpio-mask = <7>;
+				pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1
+								&gpiorange2 &gpiorange3
+								&gpiorange4 &gpiorange5
+								&gpiorange6 &gpiorange7
+								&gpiorange8 &gpiorange9
+								&gpiorange10>;
+				pinctrl-single,power-source-mask = <0x1800>;
+				pinctrl-single,bias-mask = <0xe000>;
+				pinctrl-single,bias-disable = <0>;
+				pinctrl-single,bias-pull-down = <0xa000>;
+				pinctrl-single,bias-pull-up = <0xc000>;
+				pinctrl-single,input-schmitt-mask = <0x70>;
+
+				gpiorange0: gpiorange at d401e0dc {
+					/* GPIO0 ~ GPIO54 */
+					pinctrl-single,gpio = <0 55 0x0dc>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange1: gpiorange at d401e2f0 {
+					/* GPIO55 ~ GPIO59 */
+					pinctrl-single,gpio = <55 5 0x2f0>;
+					pinctrl-single,gpio-func = <1>;
+				};
+				gpiorange2: gpiorange at d401e304 {
+					/* GPIO60 ~ GPIO66 */
+					pinctrl-single,gpio = <60 7 0x304>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange3: gpiorange at d401e1b8 {
+					/* GPIO67 ~ GPIO109 */
+					pinctrl-single,gpio = <67 43 0x1b8>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange4: gpiorange at d401e298 {
+					/* GPIO110 ~ GPIO116 */
+					pinctrl-single,gpio = <110 7 0x298>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange5: gpiorange at d401e0b4 {
+					/* GPIO117 ~ GPIO120 */
+					pinctrl-single,gpio = <117 4 0x0b4>;
+					pinctrl-single,gpio-func = <1>;
+				};
+				gpiorange6: gpiorange at d401e32c {
+					/* GPIO121 */
+					pinctrl-single,gpio = <121 1 0x32c>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange7: gpiorange at d401e0c8 {
+					/* GPIO122 ~ GPIO123 */
+					pinctrl-single,gpio = <122 2 0x0c8>;
+					pinctrl-single,gpio-func = <1>;
+				};
+				gpiorange8: gpiorange at d401e0d0 {
+					/* GPIO124 */
+					pinctrl-single,gpio = <124 1 0x0d0>;
+					pinctrl-single,gpio-func = <0>;
+				};
+				gpiorange9: gpiorange at d401e0d4 {
+					/* GPIO125 */
+					pinctrl-single,gpio = <125 1 0x0d4>;
+					pinctrl-single,gpio-func = <1>;
+				};
+				gpiorange10: gpiorange at d401e06c {
+					/* GPIO126 ~ GPIO127 */
+					pinctrl-single,gpio = <126 2 0x06c>;
+					pinctrl-single,gpio-func = <0>;
+				};
+			};
+
 			timer0: timer at d4014000 {
 				compatible = "mrvl,mmp-timer";
 				reg = <0xd4014000 0x100>;
-- 
1.7.0.4

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Haojian Zhuang]

Add comments with pinconf & gpio range in the document of
pinctrl-single.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   52 ++++++++++++++++++++
 arch/arm/boot/dts/pxa910.dtsi                      |    1 -
 2 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 2c81e45..6da2f13 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -17,6 +17,36 @@ Optional properties:
 - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
   more than one pin
 
+- pinctrl-single,gpio-ranges : gpio range list
+
+- pinctrl-single,gpio : array with gpio range start, size & register
+  offset
+
+- pinctrl-single,gpio-func : gpio function value in the pinmux register
+
+- pinctrl-single,power-source-mask : mask of setting power source in
+  the pinmux register
+
+- pinctrl-single,power-source : value of setting power source field
+  in the pinmux register
+
+- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
+  register
+
+- pinctrl-single,bias-disable : value of disabling bias in the pinmux
+  register
+
+- pinctrl-single,bias-pull-down : value of setting bias pull down in
+  the pinmux register
+
+- pinctrl-single,bias-pull-up : value of setting bias pull up in the
+  pinmux register
+
+- pinctrl-single,bias : value of setting bias in the pinmux register
+
+- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
+  in the pinmux register
+
 This driver assumes that there is only one register for each pin (unless the
 pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as
 specified in the pinctrl-bindings.txt document in this directory.
@@ -76,6 +106,28 @@ control_devconf0: pinmux at 48002274 {
 	pinctrl-single,function-mask = <0x5F>;
 };
 
+/* third controller instance for pins in gpio domain */
+pmx_gpio: pinmux at d401e000 {
+	compatible = "pinctrl-single";
+	reg = <0xd401e000 0x0330>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	pinctrl-single,register-width = <32>;
+	pinctrl-single,function-mask = <7>;
+	pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1>;
+};
+
+gpiorange0: gpiorange at d401e0dc {
+	pinctrl-single,gpio = <0 55 0x0dc>;
+	pinctrl-single,gpio-func = <0>;
+};
+
+gpiorange1: gpiorange at d401e2f0 {
+	pinctrl-single,gpio = <55 5 0x2f0>;
+	pinctrl-single,gpio-func = <1>;
+};
+
+
 /* board specific .dts file */
 
 &pmx_core {
diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi
index cf807e8..a11a582 100644
--- a/arch/arm/boot/dts/pxa910.dtsi
+++ b/arch/arm/boot/dts/pxa910.dtsi
@@ -61,7 +61,6 @@
 				#size-cells = <0>;
 				pinctrl-single,register-width = <32>;
 				pinctrl-single,function-mask = <7>;
-				pinctrl-single,gpio-mask = <7>;
 				pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1
 								&gpiorange2 &gpiorange3
 								&gpiorange4 &gpiorange5
-- 
1.7.0.4

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Stephen Warren]

On 10/22/2012 10:08 AM, Haojian Zhuang wrote:
> Add comments with pinconf & gpio range in the document of
> pinctrl-single.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-single.txt |   52 ++++++++++++++++++++
>  arch/arm/boot/dts/pxa910.dtsi                      |    1 -
>  2 files changed, 52 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> index 2c81e45..6da2f13 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> @@ -17,6 +17,36 @@ Optional properties:
>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>    more than one pin
>  
> +- pinctrl-single,gpio-ranges : gpio range list
> +
> +- pinctrl-single,gpio : array with gpio range start, size & register
> +  offset
> +
> +- pinctrl-single,gpio-func : gpio function value in the pinmux register

Some more explanation is needed here; some questions/comments:

1) Looking at the example, pinctrl-single,gpio-ranges is a property
within the main pinctrl node, whereas pinctrl-single,gpio and
pinctrl-single,gpio-func are properties within some other node. There's
no explanation of this in the binding description itself, only in the
example. Related to this, the documentation for
pinctrl-single,gpio-ranges doesn't say what it's a list of; it needs to
say that it's a list of phandles.

2) pinctrl-single,gpio is listed as optional. Presumably it's not; every
GPIO range node must have this property?

3) Why is pinctrl-single,gpio-func optional? Presumably you always need
to program the pinmux HW to select the GPIO function. Yet, the driver
code in an earlier patch seems to deliberately do nothing if this
property is missing. Shouldn't the DT parsing return an error instead?

4) I'm a little confused re: the data model. Is the idea that if
pinctrl-single,gpio-ranges is specified, then the node describes a
combined pin controller and GPIO HW? Are the pin IDs of the pin
controller expected to match the pin IDs of the GPIO HW? I'm left
wondering exactly which numbering space the values in
pinctrl-single,gpio are; do they describe the pin controller IDs that
this GPIO range describes, or do they describe the GPIO IDs that this
range describes and attempt to map them back to pin controller IDs?
Similarly, I'm not sure why there's a register offset here rather than
say a pin controller pin ID number. Shouldn't the property be a list of
<pin-controller-pin-ID GPIO-controller-GPIO-ID number-of-GPIOs>

> +- pinctrl-single,power-source-mask : mask of setting power source in
> +  the pinmux register
> +
> +- pinctrl-single,power-source : value of setting power source field
> +  in the pinmux register
> +
> +- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
> +  register
> +
> +- pinctrl-single,bias-disable : value of disabling bias in the pinmux
> +  register
> +
> +- pinctrl-single,bias-pull-down : value of setting bias pull down in
> +  the pinmux register
> +
> +- pinctrl-single,bias-pull-up : value of setting bias pull up in the
> +  pinmux register
> +
> +- pinctrl-single,bias : value of setting bias in the pinmux register
> +
> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
> +  in the pinmux register

I suppose it's OK that a generic pin controller binding would use the
generic pin configuration config options. I'm still not convinced that
the semantics of generic pin control make sense. Maybe if they're just
arbitrary names for SoC-specific things it's fine though.

Do these patches expose /all/ generic pin configuration options? It
doesn't seem worth exposing only some of them and ignoring others.

> +/* third controller instance for pins in gpio domain */
> +pmx_gpio: pinmux at d401e000 {
> +	compatible = "pinctrl-single";
> +	reg = <0xd401e000 0x0330>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;

#gpio-cells would be needed here for a GPIO controller.

> diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi

> -				pinctrl-single,gpio-mask = <7>;

I assume that's a mistake; the line shouldn't be removed in this
documentation patch?

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Haojian Zhuang]

On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/22/2012 10:08 AM, Haojian Zhuang wrote:
>> Add comments with pinconf & gpio range in the document of
>> pinctrl-single.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>> ---
>>  .../devicetree/bindings/pinctrl/pinctrl-single.txt |   52 ++++++++++++++++++++
>>  arch/arm/boot/dts/pxa910.dtsi                      |    1 -
>>  2 files changed, 52 insertions(+), 1 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
>> index 2c81e45..6da2f13 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
>> @@ -17,6 +17,36 @@ Optional properties:
>>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>>    more than one pin
>>
>> +- pinctrl-single,gpio-ranges : gpio range list
>> +
>> +- pinctrl-single,gpio : array with gpio range start, size & register
>> +  offset
>> +
>> +- pinctrl-single,gpio-func : gpio function value in the pinmux register
>
> Some more explanation is needed here; some questions/comments:
>
> 1) Looking at the example, pinctrl-single,gpio-ranges is a property
> within the main pinctrl node, whereas pinctrl-single,gpio and
> pinctrl-single,gpio-func are properties within some other node. There's
> no explanation of this in the binding description itself, only in the
> example. Related to this, the documentation for
> pinctrl-single,gpio-ranges doesn't say what it's a list of; it needs to
> say that it's a list of phandles.
>
I'll add more comments.

> 2) pinctrl-single,gpio is listed as optional. Presumably it's not; every
> GPIO range node must have this property?
>
Yes, they must be included in GPIO range node. But if GPIO feature
isn't supported in the pinctrl device, pinctrl-single,gpio is still optional.
I'll add more comments on this.

> 3) Why is pinctrl-single,gpio-func optional? Presumably you always need
> to program the pinmux HW to select the GPIO function. Yet, the driver
> code in an earlier patch seems to deliberately do nothing if this
> property is missing. Shouldn't the DT parsing return an error instead?
>
pinctrl-single,gpio-func is optional for above reason.

> 4) I'm a little confused re: the data model. Is the idea that if
> pinctrl-single,gpio-ranges is specified, then the node describes a
> combined pin controller and GPIO HW? Are the pin IDs of the pin
> controller expected to match the pin IDs of the GPIO HW?

GPIO function is enabled by setting proper field in mux field of pinmux
register.

> I'm left wondering exactly which numbering space the values in
> pinctrl-single,gpio are; do they describe the pin controller IDs that
> this GPIO range describes, or do they describe the GPIO IDs that this
> range describes and attempt to map them back to pin controller IDs?
> Similarly, I'm not sure why there's a register offset here rather than
> say a pin controller pin ID number. Shouldn't the property be a list of
> <pin-controller-pin-ID GPIO-controller-GPIO-ID number-of-GPIOs>
>
Whatever it's pin ID or offset of pinmux register, they're nearly same.
We can calculate offset from pin ID, vice visa. If pin ID is more acceptable
by you and Tony, I can change it later.

>> +- pinctrl-single,power-source-mask : mask of setting power source in
>> +  the pinmux register
>> +
>> +- pinctrl-single,power-source : value of setting power source field
>> +  in the pinmux register
>> +
>> +- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
>> +  register
>> +
>> +- pinctrl-single,bias-disable : value of disabling bias in the pinmux
>> +  register
>> +
>> +- pinctrl-single,bias-pull-down : value of setting bias pull down in
>> +  the pinmux register
>> +
>> +- pinctrl-single,bias-pull-up : value of setting bias pull up in the
>> +  pinmux register
>> +
>> +- pinctrl-single,bias : value of setting bias in the pinmux register
>> +
>> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
>> +  in the pinmux register
>
> I suppose it's OK that a generic pin controller binding would use the
> generic pin configuration config options. I'm still not convinced that
> the semantics of generic pin control make sense. Maybe if they're just
> arbitrary names for SoC-specific things it's fine though.
>
> Do these patches expose /all/ generic pin configuration options? It
> doesn't seem worth exposing only some of them and ignoring others.
>
I believe general pinconf can't support all cases in different silicons.
And we still have some common features that could be covered in general
pinconf. So we need a structure to support both pinconf & specific pinconf.

>> +/* third controller instance for pins in gpio domain */
>> +pmx_gpio: pinmux at d401e000 {
>> +     compatible = "pinctrl-single";
>> +     reg = <0xd401e000 0x0330>;
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>
> #gpio-cells would be needed here for a GPIO controller.
>
>> diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi
>
>> -                             pinctrl-single,gpio-mask = <7>;
>
> I assume that's a mistake; the line shouldn't be removed in this
> documentation patch?
>
Yes, this part should be included in the previous patch. I'll fix it.

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Stephen Warren]

On 10/31/2012 10:58 AM, Haojian Zhuang wrote:
> On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/22/2012 10:08 AM, Haojian Zhuang wrote:
>>> Add comments with pinconf & gpio range in the document of
>>> pinctrl-single.
...
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
...
>>> @@ -17,6 +17,36 @@ Optional properties:
>>>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>>>    more than one pin
>>>
>>> +- pinctrl-single,gpio-ranges : gpio range list
>>> +
>>> +- pinctrl-single,gpio : array with gpio range start, size & register
>>> +  offset
>>> +
>>> +- pinctrl-single,gpio-func : gpio function value in the pinmux register
...
>> 2) pinctrl-single,gpio is listed as optional. Presumably it's not; every
>> GPIO range node must have this property?
>>
> Yes, they must be included in GPIO range node. But if GPIO feature
> isn't supported in the pinctrl device, pinctrl-single,gpio is still optional.
> I'll add more comments on this.
> 
>> 3) Why is pinctrl-single,gpio-func optional? Presumably you always need
>> to program the pinmux HW to select the GPIO function. Yet, the driver
>> code in an earlier patch seems to deliberately do nothing if this
>> property is missing. Shouldn't the DT parsing return an error instead?
>>
> pinctrl-single,gpio-func is optional for above reason.

Presumably the node that contains the pinctrl-single,gpio-func property
is optional, but once you have such a node, pinctrl-single,gpio-func is
required?

>>> +- pinctrl-single,power-source-mask : mask of setting power source in
>>> +  the pinmux register
>>> +
>>> +- pinctrl-single,power-source : value of setting power source field
>>> +  in the pinmux register
...
>> I suppose it's OK that a generic pin controller binding would use the
>> generic pin configuration config options. I'm still not convinced that
>> the semantics of generic pin control make sense. Maybe if they're just
>> arbitrary names for SoC-specific things it's fine though.
>>
>> Do these patches expose /all/ generic pin configuration options? It
>> doesn't seem worth exposing only some of them and ignoring others.
>
> I believe general pinconf can't support all cases in different silicons.

I tend to agree.

> And we still have some common features that could be covered in general
> pinconf. So we need a structure to support both pinconf & specific pinconf.

But that tends to imply that adding support for generic pinconf into the
pinctrl-simple driver isnt' actually going to be useful for anyone. If
pinctrl-single only supports some part of your HW, how can you use it?
Or, do you intend to somehow make pinctrl-single support both the common
generic pinconf stuff, and somehow be extensible to support any
SoC-specific pin config fields?

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Haojian Zhuang]

On Wed, Oct 31, 2012 at 11:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/31/2012 10:58 AM, Haojian Zhuang wrote:
>> On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> 3) Why is pinctrl-single,gpio-func optional? Presumably you always need
>>> to program the pinmux HW to select the GPIO function. Yet, the driver
>>> code in an earlier patch seems to deliberately do nothing if this
>>> property is missing. Shouldn't the DT parsing return an error instead?
>>>
>> pinctrl-single,gpio-func is optional for above reason.
>
> Presumably the node that contains the pinctrl-single,gpio-func property
> is optional, but once you have such a node, pinctrl-single,gpio-func is
> required?
>
Yes, gpio-func is must required if the phandle of gpio range is defined.
I'll add such comments in document.

>>> I suppose it's OK that a generic pin controller binding would use the
>>> generic pin configuration config options. I'm still not convinced that
>>> the semantics of generic pin control make sense. Maybe if they're just
>>> arbitrary names for SoC-specific things it's fine though.
>>>
>>> Do these patches expose /all/ generic pin configuration options? It
>>> doesn't seem worth exposing only some of them and ignoring others.
>>
>> I believe general pinconf can't support all cases in different silicons.
>
> I tend to agree.
>
>> And we still have some common features that could be covered in general
>> pinconf. So we need a structure to support both pinconf & specific pinconf.
>
> But that tends to imply that adding support for generic pinconf into the
> pinctrl-simple driver isnt' actually going to be useful for anyone. If
> pinctrl-single only supports some part of your HW, how can you use it?
> Or, do you intend to somehow make pinctrl-single support both the common
> generic pinconf stuff, and somehow be extensible to support any
> SoC-specific pin config fields?

I'm intend to make pinctrl-single to support both the common generic pinconf
stuff and any SoC-specific pinconf. I'll try to handle it. But they
won't be included
in V3.

[PATCH v2 5/9] document: devicetree: bind pinconf with pin-single

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121031 15:53]:
> On Wed, Oct 31, 2012 at 11:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >
> > But that tends to imply that adding support for generic pinconf into the
> > pinctrl-simple driver isnt' actually going to be useful for anyone. If
> > pinctrl-single only supports some part of your HW, how can you use it?
> > Or, do you intend to somehow make pinctrl-single support both the common
> > generic pinconf stuff, and somehow be extensible to support any
> > SoC-specific pin config fields?
> 
> I'm intend to make pinctrl-single to support both the common generic pinconf
> stuff and any SoC-specific pinconf. I'll try to handle it. But they
> won't be included
> in V3.

The easiest way to support generic pinconf in pinctrl-single.c
is to set up a separate driver instance for the register ranges
that need it. Otherwise things can get messy.

Regards,

Tony

[PATCH v2 6/9] tty: pxa: configure pin

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/tty/serial/pxa.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 9033fc6..02dc771 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -37,6 +37,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -809,6 +810,7 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 			       struct uart_pxa_port *sport)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	int ret;
 
 	if (!np)
@@ -819,6 +821,10 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
 		return ret;
 	}
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		return -EPROBE_DEFER;
+
 	sport->port.line = ret;
 	return 0;
 }
@@ -857,7 +863,7 @@ static int serial_pxa_probe(struct platform_device *dev)
 	ret = serial_pxa_probe_dt(dev, sport);
 	if (ret > 0)
 		sport->port.line = dev->id;
-	else if (ret < 0)
+	if (ret < 0)
 		goto err_clk;
 	snprintf(sport->name, PXA_NAME_LEN - 1, "UART%d", sport->port.line + 1);
 
-- 
1.7.0.4

[PATCH v2 6/9] tty: pxa: configure pin

[Linus Walleij]

On Mon, Oct 22, 2012 at 6:08 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

So as I think the distributed approach to this is OK:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 7/9] i2c: pxa: use devm_kzalloc

[Haojian Zhuang]

Use devm_kzalloc & add checking in probe() function.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/i2c/busses/i2c-pxa.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..7c8b5d0 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1078,6 +1078,8 @@ static int i2c_pxa_probe_pdata(struct platform_device *pdev,
 	struct i2c_pxa_platform_data *plat = pdev->dev.platform_data;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 
+	if (!id)
+		return -EINVAL;
 	*i2c_types = id->driver_data;
 	if (plat) {
 		i2c->use_pio = plat->use_pio;
@@ -1094,29 +1096,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
-	}
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
 
 	ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
+		return ret;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
-		ret = -ENODEV;
-		goto eclk;
-	}
+	if (res == NULL || irq < 0)
+		return -ENODEV;
 
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
-		ret = -ENOMEM;
-		goto eclk;
-	}
+	if (!request_mem_region(res->start, resource_size(res), res->name))
+		return -ENOMEM;
 
 	i2c->adap.owner   = THIS_MODULE;
 	i2c->adap.retries = 5;
@@ -1209,8 +1205,6 @@ ereqirq:
 eremap:
 	clk_put(i2c->clk);
 eclk:
-	kfree(i2c);
-emalloc:
 	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
-- 
1.7.0.4

[PATCH v2 8/9] i2c: pxa: configure pinmux

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/i2c/busses/i2c-pxa.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 7c8b5d0..11e4a30 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -32,6 +32,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_i2c.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -1051,6 +1052,7 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 			    enum pxa_i2c_types *i2c_types)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	const struct of_device_id *of_id =
 			of_match_device(i2c_pxa_dt_ids, &pdev->dev);
 	int ret;
@@ -1063,6 +1065,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 		return ret;
 	}
 	pdev->id = ret;
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		return -EPROBE_DEFER;
 	if (of_get_property(np, "mrvl,i2c-polling", NULL))
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
-- 
1.7.0.4

[PATCH v2 8/9] i2c: pxa: configure pinmux

[Linus Walleij]

On Mon, Oct 22, 2012 at 6:08 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

So as I think the distributed approach to this is OK:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 9/9] pinctrl: single: dump pinmux register value

[Haojian Zhuang]

Dump pinmux register value, not only function part in the pinmux
register.

Also fix the issue on caluclating pin offset. The last parameter
should be pin number, not register offset.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/pinctrl-single.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index a20da78..6ba2a5d 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -284,15 +284,15 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
 
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
-					unsigned offset)
+					unsigned pin)
 {
 	struct pcs_device *pcs;
-	unsigned val;
+	unsigned val, mux_bytes;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	val = pcs->read(pcs->base + offset);
-	val &= pcs->fmask;
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	val = pcs_readl(pcs->base + pin * mux_bytes);
 
 	seq_printf(s, "%08x %s " , val, DRIVER_NAME);
 }
-- 
1.7.0.4

[PATCH v2 9/9] pinctrl: single: dump pinmux register value

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121022 09:13]:
> Dump pinmux register value, not only function part in the pinmux
> register.

This makes sense, but should be done using pcs_read, not pcs_readl,
see below.

> Also fix the issue on caluclating pin offset. The last parameter
> should be pin number, not register offset.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  drivers/pinctrl/pinctrl-single.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index a20da78..6ba2a5d 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -284,15 +284,15 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
>  
>  static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
>  					struct seq_file *s,
> -					unsigned offset)
> +					unsigned pin)
>  {
>  	struct pcs_device *pcs;
> -	unsigned val;
> +	unsigned val, mux_bytes;
>  
>  	pcs = pinctrl_dev_get_drvdata(pctldev);
>  
> -	val = pcs->read(pcs->base + offset);
> -	val &= pcs->fmask;
> +	mux_bytes = pcs->width / BITS_PER_BYTE;
> +	val = pcs_readl(pcs->base + pin * mux_bytes);
>  
>  	seq_printf(s, "%08x %s " , val, DRIVER_NAME);
>  }

You should not use pcs_readl here directly as the register
width can vary. Can you please update the patch for that and
make it independent from the rest of the series? That way we
can merge the fix for v3.7-rc series.

Regards,

Tony