[PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Stephen Warren]

This provides a single centralized name for the default state.

Update PIN_MAP_* macros to use this state name, instead of requiring the
user to pass a state name in.

Update documentation and mapping tables to use this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
These 3 patches are small cleanup/fixes triggered by review comments from
the 20-long series I posted a few days ago.

These patches are based on the 4-long series that I posted yesterday.

 Documentation/pinctrl.txt       |    8 ++++----
 arch/arm/mach-u300/core.c       |    6 +++---
 include/linux/pinctrl/machine.h |   13 ++++++++-----
 include/linux/pinctrl/pinctrl.h |    2 ++
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index fa9163a..8bf46bc 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -814,7 +814,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
 0 for mapping, for example:
 
 static struct pinctrl_map __initdata mapping[] = {
-       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
+	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
 };
 
 
@@ -930,7 +930,7 @@ foo_probe()
 	/* Allocate a state holder named "state" etc */
 	struct pinctrl p;
 
-	p = pinctrl_get(&device, NULL);
+	p = pinctrl_get(&device, PINCTRL_STATE_DEFAULT);
 	if IS_ERR(p)
 		return PTR_ERR(p);
 	pinctrl_enable(p);
@@ -988,7 +988,7 @@ This is enabled by simply setting the .dev_name field in the map to the name
 of the pin controller itself, like this:
 
 {
-	.name = "POWERMAP"
+	.name = PINCTRL_STATE_DEFAULT,
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "power_func",
 	.dev_name = "pinctrl-foo",
@@ -998,7 +998,7 @@ Since it may be common to request the core to hog a few always-applicable
 mux settings on the primary pin controller, there is a convenience macro for
 this:
 
-PIN_MAP_PRIMARY_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
+PIN_MAP_SYS_HOG("pinctrl-foo", "power_func")
 
 This gives the exact same result as the above construction.
 
diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
index d66bc97..2388dc2 100644
--- a/arch/arm/mach-u300/core.c
+++ b/arch/arm/mach-u300/core.c
@@ -1554,9 +1554,9 @@ static struct platform_device pinmux_device = {
 /* Pinmux settings */
 static struct pinctrl_map __initdata u300_pinmux_map[] = {
 	/* anonymous maps for chip power and EMIFs */
-	PIN_MAP_SYS_HOG("POWER", "pinmux-u300", "power"),
-	PIN_MAP_SYS_HOG("EMIF0", "pinmux-u300", "emif0"),
-	PIN_MAP_SYS_HOG("EMIF1", "pinmux-u300", "emif1"),
+	PIN_MAP_SYS_HOG("pinmux-u300", "power"),
+	PIN_MAP_SYS_HOG("pinmux-u300", "emif0"),
+	PIN_MAP_SYS_HOG("pinmux-u300", "emif1"),
 	/* per-device maps for MMC/SD, SPI and UART */
 	PIN_MAP("MMCSD", "pinmux-u300", "mmc0", "mmci"),
 	PIN_MAP("SPI", "pinmux-u300", "spi0", "pl022"),
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index 400f192..4743f84 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -12,6 +12,8 @@
 #ifndef __LINUX_PINCTRL_MACHINE_H
 #define __LINUX_PINCTRL_MACHINE_H
 
+#include "pinctrl.h"
+
 /**
  * struct pinctrl_map - boards/machines shall provide this map for devices
  * @name: the name of this specific map entry for the particular machine.
@@ -49,17 +51,18 @@ struct pinctrl_map {
  * Convenience macro to map a system function onto a certain pinctrl device,
  * to be hogged by the pin control core until the system shuts down.
  */
-#define PIN_MAP_SYS_HOG(a, b, c) \
-	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
+#define PIN_MAP_SYS_HOG(a, b) \
+	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
+	  .function = b, }
 
 /*
  * Convenience macro to map a system function onto a certain pinctrl device
  * using a specified group, to be hogged by the pin control core until the
  * system shuts down.
  */
-#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d)		\
-	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
-	  .group = d, }
+#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
+	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
+	  .function = b, .group = c, }
 
 #ifdef CONFIG_PINMUX
 
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 8bd22ee..411fe23 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -19,6 +19,8 @@
 #include <linux/list.h>
 #include <linux/seq_file.h>
 
+#define PINCTRL_STATE_DEFAULT "default"
+
 struct pinctrl_dev;
 struct pinmux_ops;
 struct pinconf_ops;
-- 
1.7.0.4

[PATCH 2/3] pinctrl: Re-order struct pinctrl_map

[Stephen Warren]

The lookup key in struct pinctrl_map is (.dev_name, .name). Re-order the
struct definition to put the lookup key fields first, and the result
values afterwards. To me at least, this slightly better reflects the
lookup process.

Update the documentation in a similar fashion.

Note: PIN_MAP*() macros aren't updated; I plan to update this once later
when enhancing the mapping table format to support pin config to reduce
churn.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 Documentation/pinctrl.txt       |   24 ++++++++++++------------
 include/linux/pinctrl/machine.h |   10 +++++-----
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 8bf46bc..6fe3232 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -781,19 +781,19 @@ spi on the second function mapping:
 
 static const struct pinctrl_map __initdata mapping[] = {
 	{
+		.dev_name = "foo-spi.0",
 		.ctrl_dev_name = "pinctrl-foo",
 		.function = "spi0",
-		.dev_name = "foo-spi.0",
 	},
 	{
+		.dev_name = "foo-i2c.0",
 		.ctrl_dev_name = "pinctrl-foo",
 		.function = "i2c0",
-		.dev_name = "foo-i2c.0",
 	},
 	{
+		.dev_name = "foo-mmc.0",
 		.ctrl_dev_name = "pinctrl-foo",
 		.function = "mmc0",
-		.dev_name = "foo-mmc.0",
 	},
 };
 
@@ -826,18 +826,18 @@ As it is possible to map a function to different groups of pins an optional
 
 ...
 {
+	.dev_name = "foo-spi.0",
 	.name = "spi0-pos-A",
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "spi0",
 	.group = "spi0_0_grp",
-	.dev_name = "foo-spi.0",
 },
 {
+	.dev_name = "foo-spi.0",
 	.name = "spi0-pos-B",
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "spi0",
 	.group = "spi0_1_grp",
-	.dev_name = "foo-spi.0",
 },
 ...
 
@@ -852,45 +852,45 @@ case), we define a mapping like this:
 
 ...
 {
+	.dev_name = "foo-mmc.0",
 	.name = "2bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "mmc0",
 	.group = "mmc0_1_grp",
-	.dev_name = "foo-mmc.0",
 },
 {
+	.dev_name = "foo-mmc.0",
 	.name = "4bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "mmc0",
 	.group = "mmc0_1_grp",
-	.dev_name = "foo-mmc.0",
 },
 {
+	.dev_name = "foo-mmc.0",
 	.name = "4bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "mmc0",
 	.group = "mmc0_2_grp",
-	.dev_name = "foo-mmc.0",
 },
 {
+	.dev_name = "foo-mmc.0",
 	.name = "8bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.group = "mmc0_1_grp",
-	.dev_name = "foo-mmc.0",
 },
 {
+	.dev_name = "foo-mmc.0",
 	.name = "8bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "mmc0",
 	.group = "mmc0_2_grp",
-	.dev_name = "foo-mmc.0",
 },
 {
+	.dev_name = "foo-mmc.0",
 	.name = "8bit"
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "mmc0",
 	.group = "mmc0_3_grp",
-	.dev_name = "foo-mmc.0",
 },
 ...
 
@@ -988,10 +988,10 @@ This is enabled by simply setting the .dev_name field in the map to the name
 of the pin controller itself, like this:
 
 {
+	.dev_name = "pinctrl-foo",
 	.name = PINCTRL_STATE_DEFAULT,
 	.ctrl_dev_name = "pinctrl-foo",
 	.function = "power_func",
-	.dev_name = "pinctrl-foo",
 },
 
 Since it may be common to request the core to hog a few always-applicable
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index 4743f84..20e9735 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -16,6 +16,10 @@
 
 /**
  * struct pinctrl_map - boards/machines shall provide this map for devices
+ * @dev_name: the name of the device using this specific mapping, the name
+ *	must be the same as in your struct device*. If this name is set to the
+ *	same name as the pin controllers own dev_name(), the map entry will be
+ *	hogged by the driver itself upon registration
  * @name: the name of this specific map entry for the particular machine.
  *	This is the second parameter passed to pinmux_get() when you want
  *	to have several mappings to the same device
@@ -27,17 +31,13 @@
  * @group: sometimes a function can map to different pin groups, so this
  *	selects a certain specific pin group to activate for the function, if
  *	left as NULL, the first applicable group will be used
- * @dev_name: the name of the device using this specific mapping, the name
- *	must be the same as in your struct device*. If this name is set to the
- *	same name as the pin controllers own dev_name(), the map entry will be
- *	hogged by the driver itself upon registration
  */
 struct pinctrl_map {
+	const char *dev_name;
 	const char *name;
 	const char *ctrl_dev_name;
 	const char *function;
 	const char *group;
-	const char *dev_name;
 };
 
 /*
-- 
1.7.0.4

[PATCH 3/3] pinctrl: Move pinctrl-maps debugfs file to top-level

[Stephen Warren]

The debugfs file pinctrl-maps is a system-wide file, not specific to any
pin controller, so place it in the top-level directory.

Also, move the code implementing the file to keep the order of all the
functions matching the order they're created in pinctrl_init_*debugfs().
The only non-obvious change here is no private data is passed to
debugfs_create_file() or single_open().

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/pinctrl/core.c |   70 ++++++++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 633b97e..376cede 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -889,28 +889,6 @@ static int pinctrl_gpioranges_show(struct seq_file *s, void *what)
 	return 0;
 }
 
-static int pinctrl_maps_show(struct seq_file *s, void *what)
-{
-	struct pinctrl_maps *maps_node;
-	int i;
-	struct pinctrl_map const *map;
-
-	seq_puts(s, "Pinctrl maps:\n");
-
-	mutex_lock(&pinctrl_maps_mutex);
-	for_each_maps(maps_node, i, map) {
-		seq_printf(s, "%s:\n", map->name);
-		seq_printf(s, "  device: %s\n", map->dev_name);
-		seq_printf(s, "  controlling device %s\n", map->ctrl_dev_name);
-		seq_printf(s, "  function: %s\n", map->function);
-		seq_printf(s, "  group: %s\n", map->group ? map->group :
-			   "(default)");
-	}
-	mutex_unlock(&pinctrl_maps_mutex);
-
-	return 0;
-}
-
 static int pinmux_hogs_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
@@ -947,6 +925,28 @@ static int pinctrl_devices_show(struct seq_file *s, void *what)
 	return 0;
 }
 
+static int pinctrl_maps_show(struct seq_file *s, void *what)
+{
+	struct pinctrl_maps *maps_node;
+	int i;
+	struct pinctrl_map const *map;
+
+	seq_puts(s, "Pinctrl maps:\n");
+
+	mutex_lock(&pinctrl_maps_mutex);
+	for_each_maps(maps_node, i, map) {
+		seq_printf(s, "%s:\n", map->name);
+		seq_printf(s, "  device: %s\n", map->dev_name);
+		seq_printf(s, "  controlling device %s\n", map->ctrl_dev_name);
+		seq_printf(s, "  function: %s\n", map->function);
+		seq_printf(s, "  group: %s\n", map->group ? map->group :
+			   "(default)");
+	}
+	mutex_unlock(&pinctrl_maps_mutex);
+
+	return 0;
+}
+
 static int pinctrl_show(struct seq_file *s, void *what)
 {
 	struct pinctrl *p;
@@ -988,11 +988,6 @@ static int pinctrl_gpioranges_open(struct inode *inode, struct file *file)
 	return single_open(file, pinctrl_gpioranges_show, inode->i_private);
 }
 
-static int pinctrl_maps_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, pinctrl_maps_show, inode->i_private);
-}
-
 static int pinmux_hogs_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, pinmux_hogs_show, inode->i_private);
@@ -1003,6 +998,11 @@ static int pinctrl_devices_open(struct inode *inode, struct file *file)
 	return single_open(file, pinctrl_devices_show, NULL);
 }
 
+static int pinctrl_maps_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinctrl_maps_show, NULL);
+}
+
 static int pinctrl_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, pinctrl_show, NULL);
@@ -1029,22 +1029,22 @@ static const struct file_operations pinctrl_gpioranges_ops = {
 	.release	= single_release,
 };
 
-static const struct file_operations pinctrl_maps_ops = {
-	.open		= pinctrl_maps_open,
+static const struct file_operations pinmux_hogs_ops = {
+	.open		= pinmux_hogs_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= single_release,
 };
 
-static const struct file_operations pinmux_hogs_ops = {
-	.open		= pinmux_hogs_open,
+static const struct file_operations pinctrl_devices_ops = {
+	.open		= pinctrl_devices_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= single_release,
 };
 
-static const struct file_operations pinctrl_devices_ops = {
-	.open		= pinctrl_devices_open,
+static const struct file_operations pinctrl_maps_ops = {
+	.open		= pinctrl_maps_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= single_release,
@@ -1078,8 +1078,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
 			    device_root, pctldev, &pinctrl_groups_ops);
 	debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
 			    device_root, pctldev, &pinctrl_gpioranges_ops);
-	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
-			    device_root, pctldev, &pinctrl_maps_ops);
 	debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
 			    device_root, pctldev, &pinmux_hogs_ops);
 	pinmux_init_device_debugfs(device_root, pctldev);
@@ -1102,6 +1100,8 @@ static void pinctrl_init_debugfs(void)
 
 	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
 			    debugfs_root, NULL, &pinctrl_devices_ops);
+	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
+			    debugfs_root, NULL, &pinctrl_maps_ops);
 	debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
 			    debugfs_root, NULL, &pinctrl_ops);
 }
-- 
1.7.0.4

Re: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Dong Aisheng]

On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
> This provides a single centralized name for the default state.
> 
> Update PIN_MAP_* macros to use this state name, instead of requiring the
> user to pass a state name in.
> 
> Update documentation and mapping tables to use this.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> These 3 patches are small cleanup/fixes triggered by review comments from
> the 20-long series I posted a few days ago.
> 
> These patches are based on the 4-long series that I posted yesterday.
> 
>  Documentation/pinctrl.txt       |    8 ++++----
>  arch/arm/mach-u300/core.c       |    6 +++---
>  include/linux/pinctrl/machine.h |   13 ++++++++-----
>  include/linux/pinctrl/pinctrl.h |    2 ++
>  4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index fa9163a..8bf46bc 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -814,7 +814,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
>  0 for mapping, for example:
>  
>  static struct pinctrl_map __initdata mapping[] = {
> -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
To keep align with the following
PIN_MAP_SYS_HOG("pinmux-u300", "power"),
Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?

And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.

Regards
Dong Aisheng

>  };
>  
>  
> @@ -930,7 +930,7 @@ foo_probe()
>  	/* Allocate a state holder named "state" etc */
>  	struct pinctrl p;
>  
> -	p = pinctrl_get(&device, NULL);
> +	p = pinctrl_get(&device, PINCTRL_STATE_DEFAULT);
>  	if IS_ERR(p)
>  		return PTR_ERR(p);
>  	pinctrl_enable(p);
> @@ -988,7 +988,7 @@ This is enabled by simply setting the .dev_name field in the map to the name
>  of the pin controller itself, like this:
>  
>  {
> -	.name = "POWERMAP"
> +	.name = PINCTRL_STATE_DEFAULT,
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "power_func",
>  	.dev_name = "pinctrl-foo",
> @@ -998,7 +998,7 @@ Since it may be common to request the core to hog a few always-applicable
>  mux settings on the primary pin controller, there is a convenience macro for
>  this:
>  
> -PIN_MAP_PRIMARY_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
> +PIN_MAP_SYS_HOG("pinctrl-foo", "power_func")
>  
>  This gives the exact same result as the above construction.
>  
> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
> index d66bc97..2388dc2 100644
> --- a/arch/arm/mach-u300/core.c
> +++ b/arch/arm/mach-u300/core.c
> @@ -1554,9 +1554,9 @@ static struct platform_device pinmux_device = {
>  /* Pinmux settings */
>  static struct pinctrl_map __initdata u300_pinmux_map[] = {
>  	/* anonymous maps for chip power and EMIFs */
> -	PIN_MAP_SYS_HOG("POWER", "pinmux-u300", "power"),
> -	PIN_MAP_SYS_HOG("EMIF0", "pinmux-u300", "emif0"),
> -	PIN_MAP_SYS_HOG("EMIF1", "pinmux-u300", "emif1"),
> +	PIN_MAP_SYS_HOG("pinmux-u300", "power"),
> +	PIN_MAP_SYS_HOG("pinmux-u300", "emif0"),
> +	PIN_MAP_SYS_HOG("pinmux-u300", "emif1"),
>  	/* per-device maps for MMC/SD, SPI and UART */
>  	PIN_MAP("MMCSD", "pinmux-u300", "mmc0", "mmci"),
>  	PIN_MAP("SPI", "pinmux-u300", "spi0", "pl022"),
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index 400f192..4743f84 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -12,6 +12,8 @@
>  #ifndef __LINUX_PINCTRL_MACHINE_H
>  #define __LINUX_PINCTRL_MACHINE_H
>  
> +#include "pinctrl.h"
> +
>  /**
>   * struct pinctrl_map - boards/machines shall provide this map for devices
>   * @name: the name of this specific map entry for the particular machine.
> @@ -49,17 +51,18 @@ struct pinctrl_map {
>   * Convenience macro to map a system function onto a certain pinctrl device,
>   * to be hogged by the pin control core until the system shuts down.
>   */
> -#define PIN_MAP_SYS_HOG(a, b, c) \
> -	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
> +#define PIN_MAP_SYS_HOG(a, b) \
> +	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> +	  .function = b, }
>  
>  /*
>   * Convenience macro to map a system function onto a certain pinctrl device
>   * using a specified group, to be hogged by the pin control core until the
>   * system shuts down.
>   */
> -#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d)		\
> -	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
> -	  .group = d, }
> +#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
> +	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> +	  .function = b, .group = c, }
>  
>  #ifdef CONFIG_PINMUX
>  
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 8bd22ee..411fe23 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -19,6 +19,8 @@
>  #include <linux/list.h>
>  #include <linux/seq_file.h>
>  
> +#define PINCTRL_STATE_DEFAULT "default"
> +
>  struct pinctrl_dev;
>  struct pinmux_ops;
>  struct pinconf_ops;
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 2/3] pinctrl: Re-order struct pinctrl_map

[Dong Aisheng]

On Thu, Feb 23, 2012 at 05:04:39PM -0700, Stephen Warren wrote:
> The lookup key in struct pinctrl_map is (.dev_name, .name). Re-order the
> struct definition to put the lookup key fields first, and the result
> values afterwards. To me at least, this slightly better reflects the
> lookup process.
> 

Not only better reflects the llokup process, i think the map name meaning
is a litlle different as before since it represents states now rather than
a map name, right?
And a map entry becomes a specific device's map entry, here the device
becomes the main entity, so it's reasonable to me to put it in the first.

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

> Update the documentation in a similar fashion.
> 
> Note: PIN_MAP*() macros aren't updated; I plan to update this once later
> when enhancing the mapping table format to support pin config to reduce
> churn.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  Documentation/pinctrl.txt       |   24 ++++++++++++------------
>  include/linux/pinctrl/machine.h |   10 +++++-----
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index 8bf46bc..6fe3232 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -781,19 +781,19 @@ spi on the second function mapping:
>  
>  static const struct pinctrl_map __initdata mapping[] = {
>  	{
> +		.dev_name = "foo-spi.0",
>  		.ctrl_dev_name = "pinctrl-foo",
>  		.function = "spi0",
> -		.dev_name = "foo-spi.0",
>  	},
>  	{
> +		.dev_name = "foo-i2c.0",
>  		.ctrl_dev_name = "pinctrl-foo",
>  		.function = "i2c0",
> -		.dev_name = "foo-i2c.0",
>  	},
>  	{
> +		.dev_name = "foo-mmc.0",
>  		.ctrl_dev_name = "pinctrl-foo",
>  		.function = "mmc0",
> -		.dev_name = "foo-mmc.0",
>  	},
>  };
>  
> @@ -826,18 +826,18 @@ As it is possible to map a function to different groups of pins an optional
>  
>  ...
>  {
> +	.dev_name = "foo-spi.0",
>  	.name = "spi0-pos-A",
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "spi0",
>  	.group = "spi0_0_grp",
> -	.dev_name = "foo-spi.0",
>  },
>  {
> +	.dev_name = "foo-spi.0",
>  	.name = "spi0-pos-B",
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "spi0",
>  	.group = "spi0_1_grp",
> -	.dev_name = "foo-spi.0",
>  },
>  ...
>  
> @@ -852,45 +852,45 @@ case), we define a mapping like this:
>  
>  ...
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "2bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "mmc0",
>  	.group = "mmc0_1_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "4bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "mmc0",
>  	.group = "mmc0_1_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "4bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "mmc0",
>  	.group = "mmc0_2_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "8bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.group = "mmc0_1_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "8bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "mmc0",
>  	.group = "mmc0_2_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  {
> +	.dev_name = "foo-mmc.0",
>  	.name = "8bit"
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "mmc0",
>  	.group = "mmc0_3_grp",
> -	.dev_name = "foo-mmc.0",
>  },
>  ...
>  
> @@ -988,10 +988,10 @@ This is enabled by simply setting the .dev_name field in the map to the name
>  of the pin controller itself, like this:
>  
>  {
> +	.dev_name = "pinctrl-foo",
>  	.name = PINCTRL_STATE_DEFAULT,
>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "power_func",
> -	.dev_name = "pinctrl-foo",
>  },
>  
>  Since it may be common to request the core to hog a few always-applicable
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index 4743f84..20e9735 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -16,6 +16,10 @@
>  
>  /**
>   * struct pinctrl_map - boards/machines shall provide this map for devices
> + * @dev_name: the name of the device using this specific mapping, the name
> + *	must be the same as in your struct device*. If this name is set to the
> + *	same name as the pin controllers own dev_name(), the map entry will be
> + *	hogged by the driver itself upon registration
>   * @name: the name of this specific map entry for the particular machine.
>   *	This is the second parameter passed to pinmux_get() when you want
>   *	to have several mappings to the same device
> @@ -27,17 +31,13 @@
>   * @group: sometimes a function can map to different pin groups, so this
>   *	selects a certain specific pin group to activate for the function, if
>   *	left as NULL, the first applicable group will be used
> - * @dev_name: the name of the device using this specific mapping, the name
> - *	must be the same as in your struct device*. If this name is set to the
> - *	same name as the pin controllers own dev_name(), the map entry will be
> - *	hogged by the driver itself upon registration
>   */
>  struct pinctrl_map {
> +	const char *dev_name;
>  	const char *name;
>  	const char *ctrl_dev_name;
>  	const char *function;
>  	const char *group;
> -	const char *dev_name;
>  };
>  
>  /*
> -- 
> 1.7.0.4
> 
> 

Re: [PATCH 3/3] pinctrl: Move pinctrl-maps debugfs file to top-level

[Dong Aisheng]

On Thu, Feb 23, 2012 at 05:04:40PM -0700, Stephen Warren wrote:
> The debugfs file pinctrl-maps is a system-wide file, not specific to any
> pin controller, so place it in the top-level directory.
> 
Reasonable to me.

> Also, move the code implementing the file to keep the order of all the
> functions matching the order they're created in pinctrl_init_*debugfs().
> The only non-obvious change here is no private data is passed to
> debugfs_create_file() or single_open().
Looks correct.

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/pinctrl/core.c |   70 ++++++++++++++++++++++++------------------------
>  1 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 633b97e..376cede 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -889,28 +889,6 @@ static int pinctrl_gpioranges_show(struct seq_file *s, void *what)
>  	return 0;
>  }
>  
> -static int pinctrl_maps_show(struct seq_file *s, void *what)
> -{
> -	struct pinctrl_maps *maps_node;
> -	int i;
> -	struct pinctrl_map const *map;
> -
> -	seq_puts(s, "Pinctrl maps:\n");
> -
> -	mutex_lock(&pinctrl_maps_mutex);
> -	for_each_maps(maps_node, i, map) {
> -		seq_printf(s, "%s:\n", map->name);
> -		seq_printf(s, "  device: %s\n", map->dev_name);
> -		seq_printf(s, "  controlling device %s\n", map->ctrl_dev_name);
> -		seq_printf(s, "  function: %s\n", map->function);
> -		seq_printf(s, "  group: %s\n", map->group ? map->group :
> -			   "(default)");
> -	}
> -	mutex_unlock(&pinctrl_maps_mutex);
> -
> -	return 0;
> -}
> -
>  static int pinmux_hogs_show(struct seq_file *s, void *what)
>  {
>  	struct pinctrl_dev *pctldev = s->private;
> @@ -947,6 +925,28 @@ static int pinctrl_devices_show(struct seq_file *s, void *what)
>  	return 0;
>  }
>  
> +static int pinctrl_maps_show(struct seq_file *s, void *what)
> +{
> +	struct pinctrl_maps *maps_node;
> +	int i;
> +	struct pinctrl_map const *map;
> +
> +	seq_puts(s, "Pinctrl maps:\n");
> +
> +	mutex_lock(&pinctrl_maps_mutex);
> +	for_each_maps(maps_node, i, map) {
> +		seq_printf(s, "%s:\n", map->name);
> +		seq_printf(s, "  device: %s\n", map->dev_name);
> +		seq_printf(s, "  controlling device %s\n", map->ctrl_dev_name);
> +		seq_printf(s, "  function: %s\n", map->function);
> +		seq_printf(s, "  group: %s\n", map->group ? map->group :
> +			   "(default)");
> +	}
> +	mutex_unlock(&pinctrl_maps_mutex);
> +
> +	return 0;
> +}
> +
>  static int pinctrl_show(struct seq_file *s, void *what)
>  {
>  	struct pinctrl *p;
> @@ -988,11 +988,6 @@ static int pinctrl_gpioranges_open(struct inode *inode, struct file *file)
>  	return single_open(file, pinctrl_gpioranges_show, inode->i_private);
>  }
>  
> -static int pinctrl_maps_open(struct inode *inode, struct file *file)
> -{
> -	return single_open(file, pinctrl_maps_show, inode->i_private);
> -}
> -
>  static int pinmux_hogs_open(struct inode *inode, struct file *file)
>  {
>  	return single_open(file, pinmux_hogs_show, inode->i_private);
> @@ -1003,6 +998,11 @@ static int pinctrl_devices_open(struct inode *inode, struct file *file)
>  	return single_open(file, pinctrl_devices_show, NULL);
>  }
>  
> +static int pinctrl_maps_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, pinctrl_maps_show, NULL);
> +}
> +
>  static int pinctrl_open(struct inode *inode, struct file *file)
>  {
>  	return single_open(file, pinctrl_show, NULL);
> @@ -1029,22 +1029,22 @@ static const struct file_operations pinctrl_gpioranges_ops = {
>  	.release	= single_release,
>  };
>  
> -static const struct file_operations pinctrl_maps_ops = {
> -	.open		= pinctrl_maps_open,
> +static const struct file_operations pinmux_hogs_ops = {
> +	.open		= pinmux_hogs_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
>  	.release	= single_release,
>  };
>  
> -static const struct file_operations pinmux_hogs_ops = {
> -	.open		= pinmux_hogs_open,
> +static const struct file_operations pinctrl_devices_ops = {
> +	.open		= pinctrl_devices_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
>  	.release	= single_release,
>  };
>  
> -static const struct file_operations pinctrl_devices_ops = {
> -	.open		= pinctrl_devices_open,
> +static const struct file_operations pinctrl_maps_ops = {
> +	.open		= pinctrl_maps_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
>  	.release	= single_release,
> @@ -1078,8 +1078,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
>  			    device_root, pctldev, &pinctrl_groups_ops);
>  	debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
>  			    device_root, pctldev, &pinctrl_gpioranges_ops);
> -	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> -			    device_root, pctldev, &pinctrl_maps_ops);
>  	debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
>  			    device_root, pctldev, &pinmux_hogs_ops);
>  	pinmux_init_device_debugfs(device_root, pctldev);
> @@ -1102,6 +1100,8 @@ static void pinctrl_init_debugfs(void)
>  
>  	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
>  			    debugfs_root, NULL, &pinctrl_devices_ops);
> +	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> +			    debugfs_root, NULL, &pinctrl_maps_ops);
>  	debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
>  			    debugfs_root, NULL, &pinctrl_ops);
>  }
> -- 
> 1.7.0.4
> 
> 

RE: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Stephen Warren]

Dong Aisheng wrote at Thursday, February 23, 2012 8:25 PM:
> On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
> > This provides a single centralized name for the default state.
> >
> > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > user to pass a state name in.
...
> > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
...
> > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
>
> To keep align with the following
> PIN_MAP_SYS_HOG("pinmux-u300", "power"),
> Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?
> 
> And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.

I don't think so.

PIN_MAP_SYS_HOG hard-codes the state name inside the macro because its
sole purpose is to support drivers/pinctrl/core.c's pinctrl_get() call
for pin hogging purposes, so that state name is always "default", or
rather, PINCTRL_STATE_DEFAULT.

The same isn't true of PIN_MAP; it's a general purpose macro intended for
any device/situation/... I'd rather not keep inventing tons of macros in
consumer.h and clutter it up.

I guess if you really want, you can submit a patch to add a macro e.g.
PIN_MAP_MUX_GROUP_DEFAULT() which hard-codes the default state name if
you wish, following on from my patch to extend the mapping table to
support pin config.

BTW, did you have any further thoughts on (not) allowing NULL state names?
I'd like to repost the final version of that patch, or rework it so that
the code actually does allow NULL state names ASAP (i.e. early Friday for
me), since it blocks some of the later patches in the series. Thanks.

-- 
nvpublic

Re: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Linus Walleij]

On Fri, Feb 24, 2012 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:

> This provides a single centralized name for the default state.
>
> Update PIN_MAP_* macros to use this state name, instead of requiring the
> user to pass a state name in.
>
> Update documentation and mapping tables to use this.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

I like the direction this is taking for sure. This is however not
working because
it break U300, no hogs nor device requests work after this patch so I cannot
apply it.

I folded in this:

diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
index 57ef1e6..ac12e08 100644
--- a/arch/arm/mach-u300/core.c
+++ b/arch/arm/mach-u300/core.c
@@ -1612,9 +1612,9 @@ static struct pinctrl_map __initdata u300_pinmux_map[] = {
        PIN_MAP_SYS_HOG("pinmux-u300", "emif0"),
        PIN_MAP_SYS_HOG("pinmux-u300", "emif1"),
        /* per-device maps for MMC/SD, SPI and UART */
-       PIN_MAP("MMCSD", "pinctrl-u300", "mmc0", "mmci"),
-       PIN_MAP("SPI", "pinctrl-u300", "spi0", "pl022"),
-       PIN_MAP("UART0", "pinctrl-u300", "uart0", "uart0"),
+       PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "mmc0", "mmci"),
+       PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "spi0", "pl022"),
+       PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "uart0", "uart0"),
 };

 struct u300_mux_hog {
@@ -1646,7 +1646,7 @@ static int __init u300_pinctrl_fetch(void)
                struct pinctrl *p;
                int ret;

-               p = pinctrl_get(u300_mux_hogs[i].dev, NULL);
+               p = pinctrl_get(u300_mux_hogs[i].dev, PINCTRL_STATE_DEFAULT);
                if (IS_ERR(p)) {
                        pr_err("u300: could not get pinmux hog %s\n",
                               u300_mux_hogs[i].name);


Still no luck :-(

What am I missing?

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: Re-order struct pinctrl_map

[Linus Walleij]

On Fri, Feb 24, 2012 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:

> The lookup key in struct pinctrl_map is (.dev_name, .name). Re-order the
> struct definition to put the lookup key fields first, and the result
> values afterwards. To me at least, this slightly better reflects the
> lookup process.
>
> Update the documentation in a similar fashion.
>
> Note: PIN_MAP*() macros aren't updated; I plan to update this once later
> when enhancing the mapping table format to support pin config to reduce
> churn.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Rebased and applied with Dong's ACK.

Thanks,
Linus Walleij

Re: [PATCH 3/3] pinctrl: Move pinctrl-maps debugfs file to top-level

[Linus Walleij]

On Fri, Feb 24, 2012 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:

> The debugfs file pinctrl-maps is a system-wide file, not specific to any
> pin controller, so place it in the top-level directory.
>
> Also, move the code implementing the file to keep the order of all the
> functions matching the order they're created in pinctrl_init_*debugfs().
> The only non-obvious change here is no private data is passed to
> debugfs_create_file() or single_open().
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Obviously correct, thanks, applied with Dong's ACK.

Yours,
Linus Walleij

Re: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Dong Aisheng]

On Thu, Feb 23, 2012 at 09:26:14PM -0800, Stephen Warren wrote:
> Dong Aisheng wrote at Thursday, February 23, 2012 8:25 PM:
> > On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
> > > This provides a single centralized name for the default state.
> > >
> > > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > > user to pass a state name in.
> ...
> > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> ...
> > > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > > +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
> >
> > To keep align with the following
> > PIN_MAP_SYS_HOG("pinmux-u300", "power"),
> > Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?
> > 
> > And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.
> 
> I don't think so.
> 
> PIN_MAP_SYS_HOG hard-codes the state name inside the macro because its
> sole purpose is to support drivers/pinctrl/core.c's pinctrl_get() call
> for pin hogging purposes, so that state name is always "default", or
> rather, PINCTRL_STATE_DEFAULT.

I'm considering that we may also need to support multi states for system hog
functions, do you agree?
If we support that, it may not mean the system function's state is always "defualt".
And then we may be better to have separate convenience macro for those people
who want to support multi states for system functions.

> 
> The same isn't true of PIN_MAP; it's a general purpose macro intended for
> any device/situation/... I'd rather not keep inventing tons of macros in
> consumer.h and clutter it up.
> 
It's just the basic one and useful.
If we do not do it, i guess people will do it in their drivers
which makes code duplication in kernel.

> I guess if you really want, you can submit a patch to add a macro e.g.
> PIN_MAP_MUX_GROUP_DEFAULT() which hard-codes the default state name if
> you wish, following on from my patch to extend the mapping table to
> support pin config.
> 
> BTW, did you have any further thoughts on (not) allowing NULL state names?
> I'd like to repost the final version of that patch, or rework it so that
> the code actually does allow NULL state names ASAP (i.e. early Friday for
> me), since it blocks some of the later patches in the series. Thanks.
> 
Personally i wish to support NULL state name.
But if it really can't, i can also accept no NULL state names.

You mentioned it may cause many problems on DT side to support NULL state.
I'm still thinking the problems we're facing for dt and if we can fix it.

Thinking the model we discussed in Linaro connect:
sdhci@c8000200 {
	pinctrl-0 = <&pmx_sdhci &pmx_sdhci_active>;
	pinctrl-1 = <&pmx_sdhci &pmx_sdhci_standby>;
	/* An optional list of state names; depends on SDHCI driver */
	pinctrl-names = "active", "suspend";
};
The orginally purpose will convert the id of pinctrl-0 (eg. it's '0' here)
to be the state name if no pincgtrl-names property defined.
I'm wondering can we not use that id, just treat this case as no state names?
If that we can allow dt to define null state names.
And it's easy for map writers to write such map.

But i do not know how much troubles it will be caused in implementation since
you're writing all these code.

What do you think of it?

Regards
Dong Aisheng

RE: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Stephen Warren]

Linus Walleij wrote at Thursday, February 23, 2012 11:09 PM:
> On Fri, Feb 24, 2012 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > This provides a single centralized name for the default state.
> >
> > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > user to pass a state name in.
> >
> > Update documentation and mapping tables to use this.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> I like the direction this is taking for sure. This is however not
> working because it break U300, no hogs nor device requests work
> after this patch so I cannot apply it.

Ah, I think I see what's happening.

Right now, pinctrl_hog_maps() iterates over every map entry, and when
it finds a hog entry, it tries to get() and enable() that one entry.
get() again iterates over the mapping table finding every entry with
the same name. This all works fine if every hog entry in the mapping
table for a given pin controller has a different name. However, this
patch changes the name field to PINCTRL_STATE_DEFAULT in all cases,
so the first get() finds 3 matching table entries and enables them all,
then the outer loop in pinctrl_hog_maps() finds the second table entry
and calls get() again which again finds all 3 entries and tries to
enable them all.

I think the fix for this is to replace the body of pinctrl_hog_maps()
with:

static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
{
        int ret;

        INIT_LIST_HEAD(&pctldev->pinctrl_hogs);

        mutex_lock(&pinctrl_maps_mutex);
        ret = pinctrl_hog_map(pctldev, PINCTRL_STATE_DEFAULT);
        mutex_unlock(&pinctrl_maps_mutex);

        return ret;
}

Does that work better? If so, I can fold it into the patch and repost,
or you can fix it up as you apply; whichever you wish.

-- 
nvpublic

RE: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Stephen Warren]

Dong Aisheng wrote at Friday, February 24, 2012 12:10 AM:
> On Thu, Feb 23, 2012 at 09:26:14PM -0800, Stephen Warren wrote:
> > Dong Aisheng wrote at Thursday, February 23, 2012 8:25 PM:
> > > On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
> > > > This provides a single centralized name for the default state.
> > > >
> > > > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > > > user to pass a state name in.
> > ...
> > > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> > ...
> > > > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > > > +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > >
> > > To keep align with the following
> > > PIN_MAP_SYS_HOG("pinmux-u300", "power"),
> > > Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?
> > >
> > > And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.
> >
> > I don't think so.
> >
> > PIN_MAP_SYS_HOG hard-codes the state name inside the macro because its
> > sole purpose is to support drivers/pinctrl/core.c's pinctrl_get() call
> > for pin hogging purposes, so that state name is always "default", or
> > rather, PINCTRL_STATE_DEFAULT.
> 
> I'm considering that we may also need to support multi states for system hog
> functions, do you agree?

Well, I don't think that'd be hogging; to my mind, hogging is specifically 
setting up PINCTRL_STATE_DEFAULT when a pin controller is registered.

But yes, I can see that the pinctrl core might want to use other state
names for a pin controller, e.g. for suspend/resume, when unregistering
the pin controller driver, etc.

That's exactly why in the mapping table rework patch I posted, I wrote
a single all-encompassing table entry macro that'd work for all these
situations, without the need to create a ton of special-case macros.

Still, I suppose now I've already enhanced that patch to support a few
special case macros, I can add yet more. But, please can I defer this
to that later patch rather than this one, since I'm already reworking
all the macros there to support the enhanced mapping table format for
pin config too?

> > BTW, did you have any further thoughts on (not) allowing NULL state names?
> > I'd like to repost the final version of that patch, or rework it so that
> > the code actually does allow NULL state names ASAP (i.e. early Friday for
> > me), since it blocks some of the later patches in the series. Thanks.
>
> Personally i wish to support NULL state name.
> But if it really can't, i can also accept no NULL state names.

OK, can I take that as an ACK for my patch that completely removes the
partial support for them ("Assume map table entries can't have a NULL
name filed")?

> You mentioned it may cause many problems on DT side to support NULL state.
> I'm still thinking the problems we're facing for dt and if we can fix it.
> 
> Thinking the model we discussed in Linaro connect:
> sdhci@c8000200 {
> 	pinctrl-0 = <&pmx_sdhci &pmx_sdhci_active>;
> 	pinctrl-1 = <&pmx_sdhci &pmx_sdhci_standby>;
> 	/* An optional list of state names; depends on SDHCI driver */
> 	pinctrl-names = "active", "suspend";
> };
> The orginally purpose will convert the id of pinctrl-0 (eg. it's '0' here)
> to be the state name if no pincgtrl-names property defined.
> I'm wondering can we not use that id, just treat this case as no state names?
> If that we can allow dt to define null state names.
> And it's easy for map writers to write such map.
> 
> But i do not know how much troubles it will be caused in implementation since
> you're writing all these code.
> 
> What do you think of it?

That sure seems like adding a special-case to the device tree parsing
code solely to provide justification for a special case in the mapping
table processing code.

My root issue here: Why do we want a NULL name field? It serves no
benefit that I can see, slightly complicates the code, obscures information
in device drivers (since they aren't requesting a particular state name)
and the pinctrl debugfs files (since we have these unnamed states, so
people need to know what that means), etc.

Given I've agreed to make macros that set .name = PINCTRL_STATE_DEFAULT
so that no mapping table author will need to do this, I don't think that
having .name=PINCTRL_STATE_DEFAULT is going to save anything either.

-- 
nvpublic

RE: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Stephen Warren]

Stephen Warren wrote at Friday, February 24, 2012 10:10 AM:
> Linus Walleij wrote at Thursday, February 23, 2012 11:09 PM:
> > On Fri, Feb 24, 2012 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:
> >
> > > This provides a single centralized name for the default state.
> > >
> > > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > > user to pass a state name in.
> > >
> > > Update documentation and mapping tables to use this.
...
> > I like the direction this is taking for sure. This is however not
> > working because it break U300, no hogs nor device requests work
> > after this patch so I cannot apply it.
...
> I think the fix for this is to replace the body of pinctrl_hog_maps()
> with:
> 
> static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
> {
>         int ret;
> 
>         INIT_LIST_HEAD(&pctldev->pinctrl_hogs);
> 
>         mutex_lock(&pinctrl_maps_mutex);
>         ret = pinctrl_hog_map(pctldev, PINCTRL_STATE_DEFAULT);
>         mutex_unlock(&pinctrl_maps_mutex);
> 
>         return ret;
> }

Oh, that won't work; the second parameter to pinctrl_hog_map() is a
map entry, not a state name.

Fixing this requires removing pctldev->pinctrl_hogs, and removing the hog
debugfs files (since that all relies on storing a list of mapping entries
that were hogged) and just doing a single pinctrl_get()/pinctrl_enable(),
and storing the one resulting struct pinctrl. I already did that as part
of my later patch for the API rework. I can extract that part into this
patch, and couple it with introducing define PINCTRL_STATE_DEFAULT, and
repost.

-- 
nvpublic

Re: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define, and use it

[Dong Aisheng]

On Sat, Feb 25, 2012 at 1:32 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Dong Aisheng wrote at Friday, February 24, 2012 12:10 AM:
>> On Thu, Feb 23, 2012 at 09:26:14PM -0800, Stephen Warren wrote:
>> > Dong Aisheng wrote at Thursday, February 23, 2012 8:25 PM:
>> > > On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
>> > > > This provides a single centralized name for the default state.
>> > > >
>> > > > Update PIN_MAP_* macros to use this state name, instead of requiring the
>> > > > user to pass a state name in.
>> > ...
>> > > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
>> > ...
>> > > > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
>> > > > +       PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
>> > >
>> > > To keep align with the following
>> > > PIN_MAP_SYS_HOG("pinmux-u300", "power"),
>> > > Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?
>> > >
>> > > And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.
>> >
>> > I don't think so.
>> >
>> > PIN_MAP_SYS_HOG hard-codes the state name inside the macro because its
>> > sole purpose is to support drivers/pinctrl/core.c's pinctrl_get() call
>> > for pin hogging purposes, so that state name is always "default", or
>> > rather, PINCTRL_STATE_DEFAULT.
>>
>> I'm considering that we may also need to support multi states for system hog
>> functions, do you agree?
>
> Well, I don't think that'd be hogging; to my mind, hogging is specifically
> setting up PINCTRL_STATE_DEFAULT when a pin controller is registered.
>
The hogging is determined by the device name not state name.
And i can see there may be need for different pin config setting for
system hog function
during different state like suspend and resume.
So the hog function may have different state like suspend and resume.

> But yes, I can see that the pinctrl core might want to use other state
> names for a pin controller, e.g. for suspend/resume, when unregistering
> the pin controller driver, etc.
>
> That's exactly why in the mapping table rework patch I posted, I wrote
> a single all-encompassing table entry macro that'd work for all these
> situations, without the need to create a ton of special-case macros.
>
Looks good, Will see it.

> Still, I suppose now I've already enhanced that patch to support a few
> special case macros, I can add yet more. But, please can I defer this
> to that later patch rather than this one, since I'm already reworking
> all the macros there to support the enhanced mapping table format for
> pin config too?
>
Of course, yes.
It's ok to me since actually it's not a big deal.

>> > BTW, did you have any further thoughts on (not) allowing NULL state names?
>> > I'd like to repost the final version of that patch, or rework it so that
>> > the code actually does allow NULL state names ASAP (i.e. early Friday for
>> > me), since it blocks some of the later patches in the series. Thanks.
>>
>> Personally i wish to support NULL state name.
>> But if it really can't, i can also accept no NULL state names.
>
> OK, can I take that as an ACK for my patch that completely removes the
> partial support for them ("Assume map table entries can't have a NULL
> name filed")?
>
Yes, please take my ACK.
I may miss something since i've not read over all your patches.
You're the main contributor of the dt binding proposal and you may
know it better
We can go for forward to see what we get if you think disallow NULL
name is better.

>> You mentioned it may cause many problems on DT side to support NULL state.
>> I'm still thinking the problems we're facing for dt and if we can fix it.
>>
>> Thinking the model we discussed in Linaro connect:
>> sdhci@c8000200 {
>>       pinctrl-0 = <&pmx_sdhci &pmx_sdhci_active>;
>>       pinctrl-1 = <&pmx_sdhci &pmx_sdhci_standby>;
>>       /* An optional list of state names; depends on SDHCI driver */
>>       pinctrl-names = "active", "suspend";
>> };
>> The orginally purpose will convert the id of pinctrl-0 (eg. it's '0' here)
>> to be the state name if no pincgtrl-names property defined.
>> I'm wondering can we not use that id, just treat this case as no state names?
>> If that we can allow dt to define null state names.
>> And it's easy for map writers to write such map.
>>
>> But i do not know how much troubles it will be caused in implementation since
>> you're writing all these code.
>>
>> What do you think of it?
>
> That sure seems like adding a special-case to the device tree parsing
> code solely to provide justification for a special case in the mapping
> table processing code.
>
> My root issue here: Why do we want a NULL name field? It serves no
The original reason to me is that 1) the mandatory state name does not
make too much sense for those devices which do not support multi
states.
2) And we do not need to change pinctrl_get(dev, NULL) to pinctrl_get(dev,
PINCTR_DEFAULT_STATE) for device drivers already in kernel if allow
NULL state name.
3) it also simplify the dt pinctrl map writing (do not need to add
"default" state name for devices not support multi states).

> benefit that I can see, slightly complicates the code, obscures information
> in device drivers (since they aren't requesting a particular state name)
This is true.

> and the pinctrl debugfs files (since we have these unnamed states, so
> people need to know what that means), etc.
Maybe define these unnamed states to a special name to tell people that means
not support multi states, right?

>
> Given I've agreed to make macros that set .name = PINCTRL_STATE_DEFAULT
> so that no mapping table author will need to do this, I don't think that
For non-dt, yes, for dt we still can not use such macros right?

> having .name=PINCTRL_STATE_DEFAULT is going to save anything either.
>

Regards
Dong Aisheng