Skip to contentSkip to conversation

use pinctrl-single in arch mmp

PATCH Superseded Haojian Zhuang 4 participants 33 messages Nov 7, 2012 – Nov 15, 2012

Subsystems (8)
Device tree bindings Arm (32-bit) Arm sub-architectures TTY layer and serial drivers +4 more subsystems

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Haojian Zhuang

author
#0 lore raw

v4 cover use pinctrl-single in arch mmp

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.

author
#1 lore raw

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

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

Signed-off-by Haojian Zhuang <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)
arch/arm/mach-mmp/Kconfig +4 -0
index 178d4da..ebdda83 100644
@@ -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.
Signature · 1 line
1.7.10.4
maintainer
#2 lore raw

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

Quoted 22 lines · Haojian Zhuang

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

Signed-off-by Haojian Zhuang <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)
arch/arm/mach-mmp/Kconfig +2 -0
index 178d4da..ebdda83 100644
@@ -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

arch/arm/mach-mmp/Kconfig config MACH_MMP2_DT · line 108

Quoted 8 lines
arch/arm/mach-mmp/Kconfig +2 -0
@@ -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

author
#3 lore raw

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

Quoted 26 lines · Tony Lindgren
Quoted 25 lines · Haojian Zhuang

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

Signed-off-by Haojian Zhuang <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)
arch/arm/mach-mmp/Kconfig +2 -0
index 178d4da..ebdda83 100644
@@ -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
Quoted 9 lines

arch/arm/mach-mmp/Kconfig config MACH_MMP2_DT · line 108

Quoted 5 lines
arch/arm/mach-mmp/Kconfig +2 -0
+     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.

#4 lore raw

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

Quoted 36 lines · Haojian Zhuang
Quoted 26 lines · Tony Lindgren
Quoted 25 lines · Haojian Zhuang

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

Signed-off-by Haojian Zhuang <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 arch/arm/mach-mmp/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)
arch/arm/mach-mmp/Kconfig +2 -0
index 178d4da..ebdda83 100644
@@ -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
Quoted 9 lines

arch/arm/mach-mmp/Kconfig config MACH_MMP2_DT · line 108

Quoted 5 lines
arch/arm/mach-mmp/Kconfig +2 -0
+     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.

Quoted 1 line

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

author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/pinctrl/pinctrl-single.c |  100 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-single.c +98 -2
index 726a729..a7c5fdd 100644
@@ -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);
Signature · 1 line
1.7.10.4
maintainer
#6 lore raw

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

Quoted 8 lines · 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.

Quoted 5 lines

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 <[email protected]>
Quoted 173 lines
Signed-off-by Haojian Zhuang <[email protected]>
---
 drivers/pinctrl/pinctrl-single.c |  100 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-single.c +98 -2
index 726a729..a7c5fdd 100644
@@ -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);
Signature · 1 line
1.7.10.4
#7 lore raw

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

Quoted 11 lines · Tony Lindgren
Quoted 10 lines · 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>

Quoted 8 lines
Quoted 3 lines

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

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

Yours,
Linus Walleij

maintainer
#8 lore raw

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

Quoted 18 lines · Linus Walleij
Quoted 17 lines · Tony Lindgren
Quoted 8 lines · 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.

Quoted 5 lines

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:

Quoted 8 lines
Quoted 2 lines
Acked-by Tony Lindgren <[email protected]>

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

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

author
#9 lore raw

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

Quoted 32 lines · Tony Lindgren
Quoted 18 lines · Linus Walleij
Quoted 17 lines · Tony Lindgren
Quoted 8 lines · 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.

Quoted 5 lines

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:

Quoted 8 lines
Quoted 2 lines
Acked-by Tony Lindgren <[email protected]>

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

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.

Quoted 6 lines

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.

author
#10 lore raw

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

In reply to Haojian Zhuang

Add pinconf generic support with POWER SOURCE, BIAS PULL.

Signed-off-by Haojian Zhuang <[email protected]>
---
 drivers/pinctrl/Kconfig          |    1 +
 drivers/pinctrl/pinctrl-single.c |  354 +++++++++++++++++++++++++++++++-------
 2 files changed, 293 insertions(+), 62 deletions(-)
drivers/pinctrl/Kconfig +1 -0
index 7bf914d..e9f2d2d 100644
@@ -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.
drivers/pinctrl/pinctrl-single.c +292 -62

546 lines not on this page · raw message

Signature · 1 line
1.7.10.4
maintainer
#11 lore raw

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

Hi,

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 486

Quoted 11 lines · Haojian Zhuang
drivers/pinctrl/pinctrl-single.c +4 -0
index a7c5fdd..77aec05 100644
 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;

...

Quoted 33 lines
drivers/pinctrl/pinctrl-single.c +10 -10
@@ -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;

drivers/pinctrl/pinctrl-single.c static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1024

Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +0 -7
 
 	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

maintainer
#12 lore raw

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

In reply to Haojian Zhuang

Hi,

Noticed few more things while playing with this.

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 486

Quoted 9 lines · Haojian Zhuang
drivers/pinctrl/pinctrl-single.c +4 -0
 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;
Quoted 4 lines
drivers/pinctrl/pinctrl-single.c +4 -0
+
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	data = pcs->read(pcs->base + offset);
+

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 500

Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +10 -0
+	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.
Quoted 30 lines
drivers/pinctrl/pinctrl-single.c +30 -0
+	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;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 540

Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +9 -0
+		*config = 0;
+		if (data == pcs->bpulldown)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	default:
+		break;
+	}
 	return -ENOTSUPP;

	return res;
Quoted 1 line

}

Did you forget to add the input schmitt handling to pcs_pinconf_get() above?

Quoted 18 lines

static int pcs_pinconf_set(struct pinctrl_dev *pctldev,

drivers/pinctrl/pinctrl-single.c +14 -1
 				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;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 569

Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +10 -0
+	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?

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 575

Quoted 6 lines
drivers/pinctrl/pinctrl-single.c +5 -0
+	}
+	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

maintainer
#13 lore raw

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

Quoted 3 lines · Tony Lindgren

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

maintainer
#14 lore raw

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

In reply to Haojian Zhuang

Hi Haojian,

One more comment on this one..

Quoted 39 lines · Haojian Zhuang
drivers/pinctrl/pinctrl-single.c +33 -0
@@ -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);

drivers/pinctrl/pinctrl-single.c static int __devinit pcs_probe(struct platform_device *pdev) · line 1231

Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +7 -0
+		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

author
#15 lore raw

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

In reply to Haojian Zhuang

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

Signed-off-by Haojian Zhuang <[email protected]>
---
 arch/arm/boot/dts/pxa910-dkb.dts |  186 +++++++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/pxa910.dtsi    |   74 +++++++++++++++
 2 files changed, 259 insertions(+), 1 deletion(-)
arch/arm/boot/dts/pxa910-dkb.dts +185 -1
index 595492a..5a8b533 100644
@@ -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 {
arch/arm/boot/dts/pxa910.dtsi +74 -0

80 lines not on this page · raw message

Signature · 1 line
1.7.10.4
maintainer
#16 lore raw

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

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:

arch/arm/boot/dts/pxa910-dkb.dts line 46

Quoted 7 lines · Haojian Zhuang
arch/arm/boot/dts/pxa910-dkb.dts +7 -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>;

					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>;
Quoted 9 lines
(quoted diff) +1 -0
+				};
arch/arm/boot/dts/pxa910.dtsi +0 -0
@@ -54,6 +54,80 @@
 			reg = <0xd4000000 0x00200000>;
 			ranges;

arch/arm/boot/dts/pxa910.dtsi line 65

Quoted 10 lines
arch/arm/boot/dts/pxa910.dtsi +9 -0
 
+			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..

arch/arm/boot/dts/pxa910.dtsi line 71

Quoted 6 lines
arch/arm/boot/dts/pxa910.dtsi +6 -0
+				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

author
#17 lore raw

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

Quoted 16 lines · 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:

arch/arm/boot/dts/pxa910-dkb.dts line 46

Quoted 7 lines · Haojian Zhuang
arch/arm/boot/dts/pxa910-dkb.dts +7 -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>;
Quoted 5 lines
                                        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.

Quoted 29 lines
Quoted 9 lines
(quoted diff) +1 -0
+                             };
arch/arm/boot/dts/pxa910.dtsi +0 -0
@@ -54,6 +54,80 @@
                      reg = <0xd4000000 0x00200000>;
                      ranges;

arch/arm/boot/dts/pxa910.dtsi line 65

Quoted 10 lines
arch/arm/boot/dts/pxa910.dtsi +9 -0
 
+                     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..

arch/arm/boot/dts/pxa910.dtsi line 71

Quoted 6 lines
arch/arm/boot/dts/pxa910.dtsi +6 -0
+                             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>;
Quoted 4 lines

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

maintainer
#18 lore raw

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

Quoted 53 lines · Haojian Zhuang
Quoted 16 lines · 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:

arch/arm/boot/dts/pxa910-dkb.dts line 46

Quoted 7 lines · Haojian Zhuang
arch/arm/boot/dts/pxa910-dkb.dts +7 -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>;
Quoted 5 lines
                                        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.

Quoted 29 lines
Quoted 9 lines
(quoted diff) +1 -0
+                             };
arch/arm/boot/dts/pxa910.dtsi +0 -0
@@ -54,6 +54,80 @@
                      reg = <0xd4000000 0x00200000>;
                      ranges;

arch/arm/boot/dts/pxa910.dtsi line 65

Quoted 10 lines
arch/arm/boot/dts/pxa910.dtsi +9 -0
 
+                     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..

Quoted 6 lines
arch/arm/boot/dts/pxa910.dtsi +6 -0
+                             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>;
Quoted 5 lines
Quoted 4 lines

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

author
#19 lore raw

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

In reply to Haojian Zhuang

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

Signed-off-by Haojian Zhuang <[email protected]>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   67 +++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +66 -1
index 2c81e45..0b8705f 100644
@@ -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 {
Signature · 1 line
1.7.10.4
maintainer
#20 lore raw

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

Hi,

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 24

Quoted 12 lines · Haojian Zhuang
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +3 -0
@@ -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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 26

Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +2 -0
+- 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

Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6 -0
+- 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
+

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 44

Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +10 -0
+- 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.

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Where 0xdc is the offset from the pinctrl register base address for the · line 75

Quoted 9 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +5 -0
@@ -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

#21 lore raw

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

In reply to Haojian Zhuang
Quoted 16 lines · Haojian Zhuang

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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +3 -1
 
 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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 27

Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6 -0
   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.

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 41

Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +13 -0
+- 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?

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 44

Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +2 -0
+- 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?

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Where 0xdc is the offset from the pinctrl register base address for the · line 75

Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +4 -0
+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?

author
#22 lore raw

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

Quoted 31 lines · Stephen Warren
Quoted 16 lines · Haojian Zhuang

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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +3 -1
 
 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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 27

Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6 -0
   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?

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 41

Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +13 -0
+- 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.

Quoted 6 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 44

Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +2 -0
+- 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?

Quoted 9 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Where 0xdc is the offset from the pinctrl register base address for the · line 75

Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +4 -0
+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.

author
#23 lore raw

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

In reply to Stephen Warren
Quoted 31 lines · Stephen Warren
Quoted 16 lines · Haojian Zhuang

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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +3 -1
 
 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

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 27

Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6 -0
   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?

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 17 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 41

Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +13 -0
+- 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.

Quoted 6 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 44

Quoted 2 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +2 -0
+- 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?

Quoted 9 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Where 0xdc is the offset from the pinctrl register base address for the · line 75

Quoted 4 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +4 -0
+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.

author
#24 lore raw

PATCH v4 6/9 tty: pxa: configure pin

In reply to Haojian Zhuang

Configure pins by pinctrl driver.

Signed-off-by Haojian Zhuang <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 drivers/tty/serial/pxa.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/tty/serial/pxa.c +7 -1
index 9033fc6..02dc771 100644
@@ -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);
Signature · 1 line
1.7.10.4
author
#25 lore raw

PATCH v4 7/9 i2c: pxa: use devm_kzalloc

In reply to Haojian Zhuang

Use devm_kzalloc & add checking in probe() function.

Signed-off-by Haojian Zhuang <[email protected]>
Cc Ben Dooks <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/i2c/busses/i2c-pxa.c +10 -16
index 1034d93..7c8b5d0 100644
@@ -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;
 }
Signature · 1 line
1.7.10.4
author
#26 lore raw

PATCH v4 8/9 i2c: pxa: configure pinmux

In reply to Haojian Zhuang

Configure pins by pinctrl driver.

Signed-off-by Haojian Zhuang <[email protected]>
Cc Ben Dooks <[email protected]>
Acked-by Linus Walleij <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    5 +++++
 1 file changed, 5 insertions(+)
drivers/i2c/busses/i2c-pxa.c +5 -0
index 7c8b5d0..11e4a30 100644
@@ -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))
Signature · 1 line
1.7.10.4
author
#27 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/pinctrl/pinctrl-single.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-single.c +4 -4
index 77aec05..243a9ca 100644
@@ -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);
 }
Signature · 1 line
1.7.10.4
maintainer
#28 lore raw

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

Quoted 5 lines · 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.

Acked-by Tony Lindgren <[email protected]>
Quoted 32 lines
Signed-off-by Haojian Zhuang <[email protected]>
---
 drivers/pinctrl/pinctrl-single.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-single.c +4 -4
index 77aec05..243a9ca 100644
@@ -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);
 }
Signature · 1 line
1.7.10.4
#29 lore raw

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

Quoted 8 lines · Tony Lindgren
Quoted 5 lines · 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.

Acked-by Tony Lindgren <[email protected]>
Acked-by Linus Walleij <[email protected]>

Shall I apply this to the pinctrl tree?

Yours,
Linus Walleij

maintainer
#30 lore raw

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

Quoted 6 lines · Linus Walleij
Quoted 4 lines · Tony Lindgren
Quoted 3 lines · Haojian Zhuang

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

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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

Acked-by Tony Lindgren <[email protected]>
Acked-by Linus Walleij <[email protected]>

Shall I apply this to the pinctrl tree?

Yes please.

Regards,

Tony

author
#31 lore raw

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

Quoted 14 lines · Tony Lindgren
Quoted 13 lines · Linus Walleij
Quoted 8 lines · Tony Lindgren
Quoted 5 lines · 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.

Acked-by Tony Lindgren <[email protected]>
Acked-by Linus Walleij <[email protected]>
Quoted 7 lines
Quoted 1 line

Shall I apply this to the pinctrl tree?

Yes please.

Regards,

Tony

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

#32 lore raw

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

Quoted 17 lines · Haojian Zhuang
Quoted 16 lines · Tony Lindgren
Quoted 14 lines · Linus Walleij
Quoted 8 lines · Tony Lindgren
Quoted 5 lines · 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.

Acked-by Tony Lindgren <[email protected]>
Acked-by Linus Walleij <[email protected]>

Shall I apply this to the pinctrl tree?

Quoted 7 lines
Quoted 5 lines

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