[PATCH v4 0/9] use pinctrl-single in arch mmp

[PATCH v4 0/9] use pinctrl-single in arch mmp

[Haojian Zhuang]

Changelog:
v4:
1. Define gpio range as sub-node, not label. And remove
pinctrl-single,gpio-ranges property.
2. Use new two properties in sub-node, reg &
pinctrl-single,gpio. GPIO number & GPIO function are listed in
the pinctrl-single,gpio property.
3. Reference the names like pinctrl-single,bias.
4. Add compatible name "pinconf-single". If the compatible name is
"pinctrl-single", there's no pinconf. If the compatible name is
"pinconf-single", there's the generic pinconf in pinctrl-single.
5. Update documents.

v3:
1. Add more comments in document.
2. Replace pcs_readl() & pcs_writel() by pcs->read() & pcs->write().
3. Clean code.

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 v4 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>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)

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

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

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> 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>
> ---
>  arch/arm/mach-mmp/Kconfig |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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.

I don't think you want to select PINCTRL_SINGLE here, you may want
to have it built as a module too in some cases.

Regards,

Tony

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

[Haojian Zhuang]

On Thu, Nov 8, 2012 at 9:38 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
>> 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>
>> ---
>>  arch/arm/mach-mmp/Kconfig |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> 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.
>
> I don't think you want to select PINCTRL_SINGLE here, you may want
> to have it built as a module too in some cases.
>
For the multi-platform image? OK, I can remove PINCTRL_SINGLE at here.

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

[Linus Walleij]

On Sat, Nov 10, 2012 at 3:53 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Thu, Nov 8, 2012 at 9:38 AM, Tony Lindgren <tony@atomide.com> wrote:
>> * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
>>> 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>
>>> ---
>>>  arch/arm/mach-mmp/Kconfig |    4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> 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.
>>
>> I don't think you want to select PINCTRL_SINGLE here, you may want
>> to have it built as a module too in some cases.
>>
> For the multi-platform image? OK, I can remove PINCTRL_SINGLE at here.

Let the MMP maintainer decide :-)

But Tony showed how much fun he could have when
testing out the single driver as module.

I imagine you could actually alter the DT in memory
then rmmod/insmod the module and hey, test an
entirely new mapping table anew.

But maybe that's a bit science fiction still...

Yours,
Linus Walleij

[PATCH v4 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 these properties in below.
<gpio range phandle>:
	include "pinctrl-single,gpio" & "pinctrl,gpio-func" properties.

	pinctrl-single,gpio: <gpio base, npins in range, register offset>

	pinctrl-single,gpio-func: <gpio function value in mux>

pinctrl-single,gpio-ranges: phandle list of gpio range array

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

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 726a729..a7c5fdd 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -30,6 +30,7 @@
 #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #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 +78,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 +136,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 +163,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 +420,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 -ENOTSUPP;
+	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->read(pcs->base + pin * mux_bytes) & ~pcs->fmask;
+	data |= gpio->gpio_func;
+	pcs->write(data, pcs->base + pin * mux_bytes);
+	return 0;
 }
 
 static struct pinmux_ops pcs_pinmux_ops = {
@@ -879,6 +914,62 @@ 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 0;
+	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 = devm_kzalloc(pcs->dev, sizeof(name),
+						GFP_KERNEL);
+		if (!gpio->range.name) {
+			dev_err(pcs->dev, "failed to allocate range name\n");
+			return -ENOMEM;
+		}
+		memcpy(&gpio->range.name, name, sizeof(name));
+		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, "pinctrl-single,gpio-func",
+					   &gpio->gpio_func);
+		if (ret < 0)
+			return -ENOENT;
+		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 +991,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 +1067,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pcs_add_gpio_range(np, pcs);
+	if (ret < 0)
+		goto free;
+
 	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
 		 pcs->desc.npins, pcs->base, pcs->size);
 
-- 
1.7.10.4

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

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> 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 these properties in below.
> <gpio range phandle>:
> 	include "pinctrl-single,gpio" & "pinctrl,gpio-func" properties.
> 
> 	pinctrl-single,gpio: <gpio base, npins in range, register offset>
> 
> 	pinctrl-single,gpio-func: <gpio function value in mux>
> 
> pinctrl-single,gpio-ranges: phandle list of gpio range array

This one looks OK to me now:

Acked-by: Tony Lindgren <tony@atomide.com>
 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  drivers/pinctrl/pinctrl-single.c |  100 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 726a729..a7c5fdd 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -30,6 +30,7 @@
>  #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
>  #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 +78,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 +136,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 +163,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 +420,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 -ENOTSUPP;
> +	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->read(pcs->base + pin * mux_bytes) & ~pcs->fmask;
> +	data |= gpio->gpio_func;
> +	pcs->write(data, pcs->base + pin * mux_bytes);
> +	return 0;
>  }
>  
>  static struct pinmux_ops pcs_pinmux_ops = {
> @@ -879,6 +914,62 @@ 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 0;
> +	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 = devm_kzalloc(pcs->dev, sizeof(name),
> +						GFP_KERNEL);
> +		if (!gpio->range.name) {
> +			dev_err(pcs->dev, "failed to allocate range name\n");
> +			return -ENOMEM;
> +		}
> +		memcpy(&gpio->range.name, name, sizeof(name));
> +		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, "pinctrl-single,gpio-func",
> +					   &gpio->gpio_func);
> +		if (ret < 0)
> +			return -ENOENT;
> +		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 +991,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 +1067,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
>  		goto free;
>  	}
>  
> +	ret = pcs_add_gpio_range(np, pcs);
> +	if (ret < 0)
> +		goto free;
> +
>  	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
>  		 pcs->desc.npins, pcs->base, pcs->size);
>  
> -- 
> 1.7.10.4
> 

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

[Linus Walleij]

On Wed, Nov 7, 2012 at 11:27 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
>> 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 these properties in below.
>> <gpio range phandle>:
>>       include "pinctrl-single,gpio" & "pinctrl,gpio-func" properties.
>>
>>       pinctrl-single,gpio: <gpio base, npins in range, register offset>
>>
>>       pinctrl-single,gpio-func: <gpio function value in mux>
>>
>> pinctrl-single,gpio-ranges: phandle list of gpio range array
>
> This one looks OK to me now:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>

So:
- Will this patch in isolation apply to my pinctrl tree?
- In that case, do you want me to apply it?

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

Yours,
Linus Walleij

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

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [121113 05:09]:
> On Wed, Nov 7, 2012 at 11:27 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> >> 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 these properties in below.
> >> <gpio range phandle>:
> >>       include "pinctrl-single,gpio" & "pinctrl,gpio-func" properties.
> >>
> >>       pinctrl-single,gpio: <gpio base, npins in range, register offset>
> >>
> >>       pinctrl-single,gpio-func: <gpio function value in mux>
> >>
> >> pinctrl-single,gpio-ranges: phandle list of gpio range array
> >
> > This one looks OK to me now:
> >
> > Acked-by: Tony Lindgren <tony@atomide.com>
> >
> 
> So:
> - Will this patch in isolation apply to my pinctrl tree?
> - In that case, do you want me to apply it?
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess the best way to go is if Linus applies this and
the debugfs fix one into some immutable pinctrl branch that
various SoC branches can pull in as needed.

The generic pinconf patch still needs few updates.

Regards,

Tony

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

[Haojian Zhuang]

On Wed, Nov 14, 2012 at 1:34 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [121113 05:09]:
>> On Wed, Nov 7, 2012 at 11:27 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
>> >> 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 these properties in below.
>> >> <gpio range phandle>:
>> >>       include "pinctrl-single,gpio" & "pinctrl,gpio-func" properties.
>> >>
>> >>       pinctrl-single,gpio: <gpio base, npins in range, register offset>
>> >>
>> >>       pinctrl-single,gpio-func: <gpio function value in mux>
>> >>
>> >> pinctrl-single,gpio-ranges: phandle list of gpio range array
>> >
>> > This one looks OK to me now:
>> >
>> > Acked-by: Tony Lindgren <tony@atomide.com>
>> >
>>
>> So:
>> - Will this patch in isolation apply to my pinctrl tree?
>> - In that case, do you want me to apply it?
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> I guess the best way to go is if Linus applies this and
> the debugfs fix one into some immutable pinctrl branch that
> various SoC branches can pull in as needed.
>
> The generic pinconf patch still needs few updates.
>
> Regards,
>
> Tony
>
I prefer to hold on this patch since I found some gpio range code are
also contained in the
pinconf patch. I'll merge them together in the next round.

[PATCH v4 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 |  354 +++++++++++++++++++++++++++++++-------
 2 files changed, 293 insertions(+), 62 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 a7c5fdd..77aec05 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>
 
@@ -30,7 +31,7 @@
 #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
-#define PCS_MAX_GPIO_VALUES		3
+#define PCS_MAX_GPIO_VALUES		2
 
 /**
  * struct pcs_pingroup - pingroups for a function
@@ -81,12 +82,20 @@ 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_conf - configuration of pinctrl device
+ * @nconf:	number of configuration that is defined for pinconf
+ * @is_generic:	for pin controller that want to use the generic interface
+ */
+struct pcs_conf {
+	unsigned nconfs;
+	bool is_generic;
 };
 
 /**
@@ -130,16 +139,24 @@ 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
+ * @conf:	value of pinconf
  * @pgtree:	pingroup index radix tree
  * @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
@@ -156,17 +173,25 @@ 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;
+	struct pcs_conf conf;
 	struct radix_tree_root pgtree;
 	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);
@@ -428,8 +453,6 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
 	unsigned data;
 
 	gpio = container_of(range, struct pcs_gpio_range, range);
-	if (!gpio->func_en)
-		return -ENOTSUPP;
 	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 "
@@ -452,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->read(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->read(pcs->base + offset) & ~mask;
+	pcs->write(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->read(pcs->base + offset) & ~mask;
+		pcs->write(ret | data, pcs->base + offset);
+	}
+	return 0;
 }
 
 static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
@@ -688,6 +846,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.
@@ -707,9 +866,12 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 						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, nconfs;
 
 	if (pcs->bits_per_mux) {
 		params = 3;
@@ -763,6 +925,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 		pins[found++] = pin;
 	}
 
+	nconfs = pcs->conf.nconfs;
 	pgnames[0] = np->name;
 	function = pcs_add_function(pcs, np, np->name, vals, found, pgnames, 1);
 	if (!function)
@@ -772,12 +935,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 (!nconfs)
+		return 0;
+
+	config = devm_kzalloc(pcs->dev, sizeof(*config) * nconfs, GFP_KERNEL);
+	if (!config) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	index = 0;
+	if (!of_property_read_u32(np, "pinctrl-single,input-schmitt", &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, "pinctrl-single,bias", &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, "pinctrl-single,power-source", &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 = nconfs;
 
 	return 0;
 
+free_pingroup:
+	pcs_free_pingroups(pcs);
+
 free_function:
 	pcs_remove_function(pcs, function);
 
@@ -789,6 +982,7 @@ free_vals:
 
 	return res;
 }
+
 /**
  * pcs_dt_node_to_map() - allocates and parses pinctrl maps
  * @pctldev: pinctrl instance
@@ -806,34 +1000,27 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	*map = devm_kzalloc(pcs->dev, sizeof(**map), GFP_KERNEL);
-	if (!map)
-		return -ENOMEM;
+	if (!pcs->conf.nconfs)
+		*num_maps = 1;
+	else
+		*num_maps = 2;
 
-	*num_maps = 0;
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * (*num_maps), GFP_KERNEL);
+	if (!*map)
+		return -ENOMEM;
 
 	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames), GFP_KERNEL);
-	if (!pgnames) {
-		ret = -ENOMEM;
-		goto free_map;
-	}
+	if (!pgnames)
+		return -ENOMEM;
 
 	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map, pgnames);
 	if (ret < 0) {
 		dev_err(pcs->dev, "no pins entries for %s\n",
 			np_config->name);
-		goto free_pgnames;
+		return ret;
 	}
-	*num_maps = 1;
 
 	return 0;
-
-free_pgnames:
-	devm_kfree(pcs->dev, pgnames);
-free_map:
-	devm_kfree(pcs->dev, *map);
-
-	return ret;
 }
 
 /**
@@ -918,52 +1105,40 @@ 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";
+	struct device_node *child;
+	struct resource r;
 	const char name[] = "pinctrl-single";
 	u32 gpiores[PCS_MAX_GPIO_VALUES];
-	int ret, size, i, mux_bytes = 0;
+	int ret, i = 0, mux_bytes = 0;
 
-	list = of_get_property(node, list_name, &size);
-	if (!list)
-		return 0;
-	size = size / sizeof(*list);
-	for (i = 0; i < size; i++) {
-		np = of_parse_phandle(node, list_name, i);
+	for_each_child_of_node(node, child) {
+		ret = of_address_to_resource(child, 0, &r);
+		if (ret < 0)
+			continue;
 		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
-		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
+		ret = of_property_read_u32_array(child, "pinctrl-single,gpio",
 						 gpiores, PCS_MAX_GPIO_VALUES);
 		if (ret < 0)
-			return -ENOENT;
+			continue;
 		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 = devm_kzalloc(pcs->dev, sizeof(name),
 						GFP_KERNEL);
 		if (!gpio->range.name) {
 			dev_err(pcs->dev, "failed to allocate range name\n");
 			return -ENOMEM;
 		}
-		memcpy(&gpio->range.name, name, sizeof(name));
-		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, "pinctrl-single,gpio-func",
-					   &gpio->gpio_func);
-		if (ret < 0)
-			return -ENOENT;
-		gpio->func_en = 1;
+		memcpy((char *)gpio->range.name, name, sizeof(name));
 
-		mutex_lock(&pcs->mutex);
-		list_add_tail(&gpio->range.node, &pcs->ranges);
-		pcs->nranges++;
-		mutex_unlock(&pcs->mutex);
+		gpio->range.id = i++;
+		gpio->range.base = gpiores[0];
+		gpio->gpio_func = gpiores[1];
+		mux_bytes = pcs->width / BITS_PER_BYTE;
+		gpio->range.pin_base = (r.start - pcs->res->start) / mux_bytes;
+		gpio->range.npins = (r.end - r.start) / mux_bytes + 1;
 
 		pinctrl_add_gpio_range(pcs->pctl, &gpio->range);
 	}
@@ -976,11 +1151,13 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	struct resource *res;
 	struct pcs_device *pcs;
+	const struct pcs_conf *conf;
 	int ret;
 
 	match = of_match_device(pcs_of_match, &pdev->dev);
 	if (!match)
 		return -EINVAL;
+	conf = match->data;
 
 	pcs = devm_kzalloc(&pdev->dev, sizeof(*pcs), GFP_KERNEL);
 	if (!pcs) {
@@ -988,10 +1165,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 	pcs->dev = &pdev->dev;
+	memcpy(&pcs->conf, conf, sizeof(*conf));
 	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");
@@ -1009,6 +1186,46 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	pcs->bits_per_mux = of_property_read_bool(np,
 						  "pinctrl-single,bit-per-mux");
 
+	if (conf->nconfs) {
+		pcs_pinconf_ops.is_generic = true;
+		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");
@@ -1094,8 +1311,21 @@ static int __devexit pcs_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/* PINCONF isn't supported */
+static struct pcs_conf pinctrl_conf __devinitdata = {
+	.nconfs = 0,
+	.is_generic = false,
+};
+
+/* Generic PINCONF is supported. */
+static struct pcs_conf pinconf_conf __devinitdata = {
+	.nconfs = 7,
+	.is_generic = true,
+};
+
 static struct of_device_id pcs_of_match[] __devinitdata = {
-	{ .compatible = DRIVER_NAME, },
+	{ .compatible = "pinctrl-single", .data = &pinctrl_conf },
+	{ .compatible = "pinconf-single", .data = &pinconf_conf },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, pcs_of_match);
-- 
1.7.10.4

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

[Tony Lindgren]

Hi,

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index a7c5fdd..77aec05 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
>  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;

Do you need a check here (and also in other pinconf related functions)
for driver instances that don't have pinconf enabled?

Something like:

	if (!pcs->pinconf)
		return -ENOTSUPP;
...


> @@ -806,34 +1000,27 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
>  
>  	pcs = pinctrl_dev_get_drvdata(pctldev);
>  
> -	*map = devm_kzalloc(pcs->dev, sizeof(**map), GFP_KERNEL);
> -	if (!map)
> -		return -ENOMEM;
> +	if (!pcs->conf.nconfs)
> +		*num_maps = 1;
> +	else
> +		*num_maps = 2;
>  
> -	*num_maps = 0;
> +	*map = devm_kzalloc(pcs->dev, sizeof(**map) * (*num_maps), GFP_KERNEL);
> +	if (!*map)
> +		return -ENOMEM;
>  
>  	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames), GFP_KERNEL);
> -	if (!pgnames) {
> -		ret = -ENOMEM;
> -		goto free_map;
> -	}
> +	if (!pgnames)
> +		return -ENOMEM;
>  
>  	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map, pgnames);
>  	if (ret < 0) {
>  		dev_err(pcs->dev, "no pins entries for %s\n",
>  			np_config->name);
> -		goto free_pgnames;
> +		return ret;
>  	}
> -	*num_maps = 1;
>  
>  	return 0;
> -
> -free_pgnames:
> -	devm_kfree(pcs->dev, pgnames);
> -free_map:
> -	devm_kfree(pcs->dev, *map);
> -
> -	return ret;
>  }

Here looks like you're changing the behaviour to not free entries in cases
where parsing with cs_parse_one_pinctrl_entry() fails. I'd prefer the freeing
it in case of parsing errors so we don't waste memory when the system is
running. Also there's a tiny chance that *num_maps will be wrong if the
second devm_kzalloc fails.. But it's more likely that we get a broken device
tree to parse and end up potentially hogging hundreds of maps.

Regards,

Tony

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

[Tony Lindgren]

Hi,

Noticed few more things while playing with this.

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
>  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;

	I suggest adding:

	int res = -ENOTSUPP;

> +
> +	offset = pin * (pcs->width / BITS_PER_BYTE);
> +	data = pcs->read(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;

	then you can do:

		res = 0;
		break;

	instead of the return 0 and 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;

	return res;

>  }


Did you forget to add the input schmitt handling to pcs_pinconf_get()
above?
  
>  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;

Here we should probably return -ENOTSUPP instead for the unhandled
ones?

> +	}
> +	offset = pin * (pcs->width / BITS_PER_BYTE);
> +	ret = pcs->read(pcs->base + offset) & ~mask;
> +	pcs->write(ret | data, pcs->base + offset);
> +	return 0;
>  }

Then looks like pcs_pinconf_set() is also missing handling for few
of the things configured in the probe?

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121108 15:00]:
> Hi,
> 
> Noticed few more things while playing with this.

Also please run checkpatch.pl --strict on these patches,
at least this one generates some warnings.

Regards,

Tony

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

[Tony Lindgren]

Hi Haojian,

One more comment on this one..

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -1009,6 +1186,46 @@ static int __devinit pcs_probe(struct platform_device *pdev)
>  	pcs->bits_per_mux = of_property_read_bool(np,
>  						  "pinctrl-single,bit-per-mux");
>  
> +	if (conf->nconfs) {
> +		pcs_pinconf_ops.is_generic = true;
> +		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");

I tried to make pinconf work with the omap CONTROL_PBIASLITE
register, but noticed that we need to change the binding to make
pinctrl-single,bits type controllers work.

Basically we need to move the pinconf properties to be defined
under pinctrl-single,pins and pinctrl-single,bits rather than being
pinmux controller instance specific properties.

In the pinctrl-single,bits case we have multiple pinconf masks
for a single register, like in the omap CONTROL_PBIASLITE example
we discussed earlier.

Then let's just have a single pinctrl driver instance specific
property  pinctrl-single,pinconf that we can use to optimize out
the pinconf parsing for ranges that don't support pinconf.

Other than that, I think I'm finally done with my comments for
this patch, sorry it took so long and took so many emails.

Regards,

Tony

[PATCH v4 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 |  186 +++++++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/pxa910.dtsi    |   74 +++++++++++++++
 2 files changed, 259 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/pxa910-dkb.dts b/arch/arm/boot/dts/pxa910-dkb.dts
index 595492a..5a8b533 100644
--- a/arch/arm/boot/dts/pxa910-dkb.dts
+++ b/arch/arm/boot/dts/pxa910-dkb.dts
@@ -24,10 +24,194 @@
 
 	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 */
+					/* w1 */
+					pinctrl-single,pins = <
+						0x0cc 0x2	/* CLK_REQ_W1 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				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>;
+				};
+				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..aac5a2e 100644
--- a/arch/arm/boot/dts/pxa910.dtsi
+++ b/arch/arm/boot/dts/pxa910.dtsi
@@ -54,6 +54,80 @@
 			reg = <0xd4000000 0x00200000>;
 			ranges;
 
+			pmx: pinmux at d401e000 {
+				compatible = "pinconf-single";
+				reg = <0xd401e000 0x0330>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges;
+
+				pinctrl-single,register-width = <32>;
+				pinctrl-single,function-mask = <7>;
+				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>;
+
+				range0: range at d401e0dc {
+					/* GPIO0 ~ GPIO54 */
+					reg = <0xd401e0dc 0xdc>;
+					/* gpio base & gpio func */
+					pinctrl-single,gpio = <0 0>;
+				};
+				range1: range at d401e2f0 {
+					/* GPIO55 ~ GPIO59 */
+					reg = <0xd401e2f0 0x14>;
+					pinctrl-single,gpio = <55 1>;
+				};
+				range2: range at d401e304 {
+					/* GPIO60 ~ GPIO66 */
+					reg = <0xd401e304 0x1c>;
+					pinctrl-single,gpio = <60 0>;
+				};
+				range3: range at d401e1b8 {
+					/* GPIO67 ~ GPIO109 */
+					reg = <0xd401e1b8 0xac>;
+					pinctrl-single,gpio = <67 0>;
+				};
+				range4: range at d401e298 {
+					/* GPIO110 ~ GPIO116 */
+					reg = <0xd401e298 0x1c>;
+					pinctrl-single,gpio = <110 0>;
+				};
+				range5: range at d401e0b4 {
+					/* GPIO117 ~ GPIO120 */
+					reg = <0xd401e0b4 0x10>;
+					pinctrl-single,gpio = <117 1>;
+				};
+				range6: range at d401e32c {
+					/* GPIO121 */
+					reg = <0xd401e32c 0x04>;
+					pinctrl-single,gpio = <121 0>;
+				};
+				range7: range at d401e0c8 {
+					/* GPIO122 ~ GPIO123 */
+					reg = <0xd401e0c8 0x08>;
+					pinctrl-single,gpio = <122 1>;
+				};
+				range8: range at d401e0d0 {
+					/* GPIO124 */
+					reg = <0xd401e0d0 0x04>;
+					pinctrl-single,gpio = <124 0>;
+				};
+				range9: range at d401e0d4 {
+					/* GPIO125 */
+					reg = <0xd401e0d4 0x04>;
+					pinctrl-single,gpio = <125 1>;
+				};
+				range10: range at d401e06c {
+					/* GPIO126 ~ GPIO127 */
+					reg = <0xd401e06c 0x08>;
+					pinctrl-single,gpio = <126 0>;
+				};
+			};
+
 			timer0: timer at d4014000 {
 				compatible = "mrvl,mmp-timer";
 				reg = <0xd4014000 0x100>;
-- 
1.7.10.4

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

[Tony Lindgren]

Hi,

To clarify my binding change comment for the generic pinconf
support, here's an example.

We need to move pinctrl-single pinconf properties here for
each pin group:

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> +				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>;
					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>;
> +				};



> --- a/arch/arm/boot/dts/pxa910.dtsi
> +++ b/arch/arm/boot/dts/pxa910.dtsi
> @@ -54,6 +54,80 @@
>  			reg = <0xd4000000 0x00200000>;
>  			ranges;
>  
> +			pmx: pinmux at d401e000 {
> +				compatible = "pinconf-single";
> +				reg = <0xd401e000 0x0330>;
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges;
> +
> +				pinctrl-single,register-width = <32>;
> +				pinctrl-single,function-mask = <7>;

And then..

> +				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>;

..remove these from here. Otherwise pinctrl-single,bits type controllers
won't be able to use the pinconf functions as we can have multiple pins
supported in a single register.

Regards,

Tony

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

[Haojian Zhuang]

On Sat, Nov 10, 2012 at 6:48 AM, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> To clarify my binding change comment for the generic pinconf
> support, here's an example.
>
> We need to move pinctrl-single pinconf properties here for
> each pin group:
>
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
>> +                             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>;
>                                         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>;
It's OK.

>> +                             };
>
>
>
>> --- a/arch/arm/boot/dts/pxa910.dtsi
>> +++ b/arch/arm/boot/dts/pxa910.dtsi
>> @@ -54,6 +54,80 @@
>>                       reg = <0xd4000000 0x00200000>;
>>                       ranges;
>>
>> +                     pmx: pinmux at d401e000 {
>> +                             compatible = "pinconf-single";
>> +                             reg = <0xd401e000 0x0330>;
>> +                             #address-cells = <1>;
>> +                             #size-cells = <1>;
>> +                             ranges;
>> +
>> +                             pinctrl-single,register-width = <32>;
>> +                             pinctrl-single,function-mask = <7>;
>
> And then..
>
>> +                             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>;
>
> ..remove these from here. Otherwise pinctrl-single,bits type controllers
> won't be able to use the pinconf functions as we can have multiple pins
> supported in a single register.
>
Will do

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

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121109 21:38]:
> On Sat, Nov 10, 2012 at 6:48 AM, Tony Lindgren <tony@atomide.com> wrote:
> > Hi,
> >
> > To clarify my binding change comment for the generic pinconf
> > support, here's an example.
> >
> > We need to move pinctrl-single pinconf properties here for
> > each pin group:
> >
> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:21]:
> >> +                             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>;
> >                                         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>;
> It's OK.
> 
> >> +                             };
> >
> >
> >
> >> --- a/arch/arm/boot/dts/pxa910.dtsi
> >> +++ b/arch/arm/boot/dts/pxa910.dtsi
> >> @@ -54,6 +54,80 @@
> >>                       reg = <0xd4000000 0x00200000>;
> >>                       ranges;
> >>
> >> +                     pmx: pinmux at d401e000 {
> >> +                             compatible = "pinconf-single";
> >> +                             reg = <0xd401e000 0x0330>;
> >> +                             #address-cells = <1>;
> >> +                             #size-cells = <1>;
> >> +                             ranges;
> >> +
> >> +                             pinctrl-single,register-width = <32>;
> >> +                             pinctrl-single,function-mask = <7>;
> >
> > And then..
> >
> >> +                             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>;
> >
> > ..remove these from here. Otherwise pinctrl-single,bits type controllers
> > won't be able to use the pinconf functions as we can have multiple pins
> > supported in a single register.
> >
> Will do

OK thanks!

Tony

[PATCH v4 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 |   67 +++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 2c81e45..0b8705f 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -1,7 +1,9 @@
 One-register-per-pin type device tree based pinctrl driver
 
 Required properties:
-- compatible : "pinctrl-single"
+- compatible : "pinctrl-single" or "pinconf-single".
+  "pinctrl-single" means that pinconf isn't supported.
+  "pinconf-single" means that generic pinconf is supported.
 
 - reg : offset and length of the register set for the mux registers
 
@@ -14,9 +16,33 @@ Optional properties:
 - pinctrl-single,function-off : function off mode for disabled state if
   available and same for all registers; if not specified, disabling of
   pin functions is ignored
+
 - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
   more than one pin
 
+- 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.
@@ -42,6 +68,25 @@ Where 0xdc is the offset from the pinctrl register base address for the
 device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
 be used when applying this change to the register.
 
+
+Optional sub-node: In case some pins could be configured as GPIO in the pinmux
+register. If both GPIO nubmer and pin base of those pins are in ascending order,
+those pins could be defined as a GPIO range. The sub-node should be defined in
+.dtsi files of those silicons.
+
+Required properties in sub-node:
+- reg : offset and length of the GPIO range sub-node.
+
+- pinctrl-single,gpio : array of GPIO base number in the range and the GPIO
+  function in the pinmux register.
+
+	range0: {
+		/* GPIO0 ~ GPIO54 */
+		reg = <0xd401e0dc 55>;
+		pinctrl-single,gpio = <0 0>;
+	};
+
+
 Example:
 
 /* SoC common file */
@@ -76,6 +121,26 @@ 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 = <1>;
+	ranges;
+
+	pinctrl-single,register-width = <32>;
+	pinctrl-single,function-mask = <7>;
+
+	range0: range at d401e0dc {
+		/* GPIO0 ~ GPIO54 */
+		reg = <0xd401e0dc 0xdc>;
+		/* gpio base & gpio func */
+		pinctrl-single,gpio = <0 0>;
+	};     
+};
+
+
 /* board specific .dts file */
 
 &pmx_core {
-- 
1.7.10.4

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

[Tony Lindgren]

Hi,

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> @@ -14,9 +16,33 @@ Optional properties:
>  - pinctrl-single,function-off : function off mode for disabled state if
>    available and same for all registers; if not specified, disabling of
>    pin functions is ignored
> +
>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>    more than one pin
>  
> +- pinctrl-single,power-source-mask : mask of setting power source in
> +  the pinmux register

My non-native english suggests:

- pinctrl-single,power-source-mask : mask for setting the power source in
  the pinmux register

> +- pinctrl-single,power-source : value of setting power source field
> +  in the pinmux register

- pinctrl-single,power-source : value for setting the 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

And the same for the rest.

> @@ -42,6 +68,25 @@ Where 0xdc is the offset from the pinctrl register base address for the
>  device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
>  be used when applying this change to the register.
>  
> +
> +Optional sub-node: In case some pins could be configured as GPIO in the pinmux
> +register. If both GPIO nubmer and pin base of those pins are in ascending order,
> +those pins could be defined as a GPIO range. The sub-node should be defined in
> +.dtsi files of those silicons.

I suggest you update the above to say:

Optional sub-node: In case some pins can be configured as GPIO in the pinmux
register. If both the GPIO number and pin base of those pins are in ascending
order, these pins can be defined as a GPIO range.

Then maybe clarify the sub-node part a bit, or just leave it out? Actually the
"ascending order" part is a bit unclear to me too..

Regards,

Tony

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

[Stephen Warren]

On 11/07/2012 08:19 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

>  Required properties:
> -- compatible : "pinctrl-single"
> +- compatible : "pinctrl-single" or "pinconf-single".
> +  "pinctrl-single" means that pinconf isn't supported.
> +  "pinconf-single" means that generic pinconf is supported.
>  
>  - reg : offset and length of the register set for the mux registers
>  
> @@ -14,9 +16,33 @@ Optional properties:
>  - pinctrl-single,function-off : function off mode for disabled state if
>    available and same for all registers; if not specified, disabling of
>    pin functions is ignored
> +
>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>    more than one pin
>  
> +- 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

Isn't power-source an enumeration? As such, shouldn't the pinctrl state
definition supply the value, rather than the pin controller definition?

The above two properties imply to me that you're saying "these bits
(power-source-mask) in any pin register must be set to this one value
(power-source)". However, I think you want to say "these bits
(power-source-mask) are used to configure the power source", and then
allow the state nodes (what's reference from pinctrl client devices'
pinctrl-n properties) to define the value.

Perhaps that's your intent already, but if so, the properties for the
two different nodes should be documented separately, not all interleaved
in a single list, without any indication of which node they get put into.

> +- 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

If there are bias-disable, bias-pull-down, and bias-pull-up properties,
what is the plain "bias" property for?

> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
> +  in the pinmux register

And if the "value" properties go into the pinctrl state nodes, why isn't
there a "value" property for schmitt?

> +Optional sub-node: In case some pins could be configured as GPIO in the pinmux
> +register. If both GPIO nubmer and pin base of those pins are in ascending order,
> +those pins could be defined as a GPIO range. The sub-node should be defined in
> +.dtsi files of those silicons.

Pinctrl state definitions are also nodes directly inside the pin
controller node. How does the pin controller driver know whether a child
node is a state definition, or a GPIO range? Is the node required to
have some specific name?

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

[Haojian Zhuang]

On Tue, Nov 13, 2012 at 4:37 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/07/2012 08:19 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
>
>>  Required properties:
>> -- compatible : "pinctrl-single"
>> +- compatible : "pinctrl-single" or "pinconf-single".
>> +  "pinctrl-single" means that pinconf isn't supported.
>> +  "pinconf-single" means that generic pinconf is supported.
>>
>>  - reg : offset and length of the register set for the mux registers
>>
>> @@ -14,9 +16,33 @@ Optional properties:
>>  - pinctrl-single,function-off : function off mode for disabled state if
>>    available and same for all registers; if not specified, disabling of
>>    pin functions is ignored
>> +
>>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>>    more than one pin
>>
>> +- 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
>
> Isn't power-source an enumeration? As such, shouldn't the pinctrl state
> definition supply the value, rather than the pin controller definition?
>
> The above two properties imply to me that you're saying "these bits
> (power-source-mask) in any pin register must be set to this one value
> (power-source)". However, I think you want to say "these bits
> (power-source-mask) are used to configure the power source", and then
> allow the state nodes (what's reference from pinctrl client devices'
> pinctrl-n properties) to define the value.

Got it.

>
> Perhaps that's your intent already, but if so, the properties for the
> two different nodes should be documented separately, not all interleaved
> in a single list, without any indication of which node they get put into.
>
>> +- 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
>
> If there are bias-disable, bias-pull-down, and bias-pull-up properties,
> what is the plain "bias" property for?
>
Sure. I'll use input bias to explain it.

>> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
>> +  in the pinmux register
>
> And if the "value" properties go into the pinctrl state nodes, why isn't
> there a "value" property for schmitt?
>
>> +Optional sub-node: In case some pins could be configured as GPIO in the pinmux
>> +register. If both GPIO nubmer and pin base of those pins are in ascending order,
>> +those pins could be defined as a GPIO range. The sub-node should be defined in
>> +.dtsi files of those silicons.
>
> Pinctrl state definitions are also nodes directly inside the pin
> controller node. How does the pin controller driver know whether a child
> node is a state definition, or a GPIO range? Is the node required to
> have some specific name?

I think that "pinctrl-single,gpio" is enough. If the child node
doesn't contain the
gpio property, it won't be parsed as gpio range sub-node.

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

[Haojian Zhuang]

On Tue, Nov 13, 2012 at 4:37 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/07/2012 08:19 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
>
>>  Required properties:
>> -- compatible : "pinctrl-single"
>> +- compatible : "pinctrl-single" or "pinconf-single".
>> +  "pinctrl-single" means that pinconf isn't supported.
>> +  "pinconf-single" means that generic pinconf is supported.
>>
>>  - reg : offset and length of the register set for the mux registers
>>
>> @@ -14,9 +16,33 @@ Optional properties:
>>  - pinctrl-single,function-off : function off mode for disabled state if
>>    available and same for all registers; if not specified, disabling of
>>    pin functions is ignored
>> +
>>  - pinctrl-single,bit-per-mux : boolean to indicate that one register controls
>>    more than one pin
>>
>> +- 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
>
> Isn't power-source an enumeration? As such, shouldn't the pinctrl state
> definition supply the value, rather than the pin controller definition?
>
> The above two properties imply to me that you're saying "these bits
> (power-source-mask) in any pin register must be set to this one value
> (power-source)". However, I think you want to say "these bits
> (power-source-mask) are used to configure the power source", and then
> allow the state nodes (what's reference from pinctrl client devices'
> pinctrl-n properties) to define the value.

Got it.

>
> Perhaps that's your intent already, but if so, the properties for the
> two different nodes should be documented separately, not all interleaved
> in a single list, without any indication of which node they get put into.
>
>> +- 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
>
> If there are bias-disable, bias-pull-down, and bias-pull-up properties,
> what is the plain "bias" property for?
>
Sure. I'll use input bias to explain it.

>> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
>> +  in the pinmux register
>
> And if the "value" properties go into the pinctrl state nodes, why isn't
> there a "value" property for schmitt?
>
>> +Optional sub-node: In case some pins could be configured as GPIO in the pinmux
>> +register. If both GPIO nubmer and pin base of those pins are in ascending order,
>> +those pins could be defined as a GPIO range. The sub-node should be defined in
>> +.dtsi files of those silicons.
>
> Pinctrl state definitions are also nodes directly inside the pin
> controller node. How does the pin controller driver know whether a child
> node is a state definition, or a GPIO range? Is the node required to
> have some specific name?

I think that "pinctrl-single,gpio" is enough. If the child node
doesn't contain the
gpio property, it won't be parsed as gpio range sub-node.

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

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/pxa.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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

[PATCH v4 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>
Cc: Ben Dooks <ben-linux@fluff.org>
---
 drivers/i2c/busses/i2c-pxa.c |   26 ++++++++++----------------
 1 file 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.10.4

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

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c |    5 +++++
 1 file changed, 5 insertions(+)

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

[PATCH v4 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 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 77aec05..243a9ca 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -286,15 +286,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->read(pcs->base + pin * mux_bytes);
 
 	seq_printf(s, "%08x %s " , val, DRIVER_NAME);
 }
-- 
1.7.10.4

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

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
> 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.

Acked-by: Tony Lindgren <tony@atomide.com>
 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  drivers/pinctrl/pinctrl-single.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 77aec05..243a9ca 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -286,15 +286,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->read(pcs->base + pin * mux_bytes);
>  
>  	seq_printf(s, "%08x %s " , val, DRIVER_NAME);
>  }
> -- 
> 1.7.10.4
> 

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

[Linus Walleij]

On Thu, Nov 8, 2012 at 2:25 AM, Tony Lindgren <tony@atomide.com> wrote:

> * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
>> 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.
>
> Acked-by: Tony Lindgren <tony@atomide.com>

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

Shall I apply this to the pinctrl tree?

Yours,
Linus Walleij

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

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [121113 05:10]:
> On Thu, Nov 8, 2012 at 2:25 AM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
> >> 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.
> >
> > Acked-by: Tony Lindgren <tony@atomide.com>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Shall I apply this to the pinctrl tree?

Yes please.

Regards,

Tony

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

[Haojian Zhuang]

On Wed, Nov 14, 2012 at 1:34 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [121113 05:10]:
>> On Thu, Nov 8, 2012 at 2:25 AM, Tony Lindgren <tony@atomide.com> wrote:
>>
>> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
>> >> 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.
>> >
>> > Acked-by: Tony Lindgren <tony@atomide.com>
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Shall I apply this to the pinctrl tree?
>
> Yes please.
>
> Regards,
>
> Tony

Yes, this patch doesn't impact others. Please help to merge.

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

[Linus Walleij]

On Wed, Nov 14, 2012 at 2:45 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Wed, Nov 14, 2012 at 1:34 AM, Tony Lindgren <tony@atomide.com> wrote:
>> * Linus Walleij <linus.walleij@linaro.org> [121113 05:10]:
>>> On Thu, Nov 8, 2012 at 2:25 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>
>>> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121107 07:22]:
>>> >> 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.
>>> >
>>> > Acked-by: Tony Lindgren <tony@atomide.com>
>>>
>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Shall I apply this to the pinctrl tree?
>>
>> Yes please.
>>
>> Regards,
>>
>> Tony
>
> Yes, this patch doesn't impact others. Please help to merge.

OK patch applied to the pinctrl tree.

Yours,
Linus Walleij