[PATCH] pinctrl: pin configuration states

[PATCH] pinctrl: pin configuration states

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This introduce a pin configuration state structure and activation
functions similar to the pinmux map. It basically names a few
states and define the custom configuration values to be applied to
groups and pins alike when switching to a certain state.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Maybe this is closer to what people want for configuring the pins
in their platforms? I am not fully satisfied with this solution
because it basically maintains status quo of the big static tables
found in many ARM SoCs, but it does instill a common scheme for
doing it.

Also there is no way for a pinmux and its associated device to
switch states in this solution. However one does not exclude the
other, we might want per-device associated pinmux and group
states *also*.

Comments welcome.
---
 Documentation/pinctrl.txt       |   63 +++++++++++
 drivers/pinctrl/core.c          |    3 +
 drivers/pinctrl/pinconf.c       |  231 +++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinconf.h       |   11 ++
 include/linux/pinctrl/machine.h |   79 +++++++++++++
 5 files changed, 380 insertions(+), 7 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 1851f1d..29845c4 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -277,6 +277,69 @@ case each individual pin will be treated by separate pin_config_set() calls as
 well.
 
 
+Pin configuration states
+========================
+
+To help platforms to set up initial pin configuration and transit the entire
+set of pins on a pin controller between different states, pin controller
+states are supported by creating named states per controller. A board/machine
+can define a pin configuration like this:
+
+static const struct pin_config foo_pin_config_active[] = {
+	PIN_CONFIG("GPIO0", PLATFORM_X_PULL_UP),
+	PIN_CONFIG("GPIO1", PLATFORM_X_PULL_DOWN),
+};
+
+static const struct pin_config foo_pin_config_idle[] = {
+	PIN_CONFIG("GPIO0", PLATFORM_X_GROUND),
+	PIN_CONFIG("GPIO1", PLATFORM_X_GROUND),
+};
+
+static const struct pin_config foo_pin_config_idle[] = {
+	PIN_CONFIG("GPIO0", PLATFORM_X_OFF),
+	PIN_CONFIG("GPIO1", PLATFORM_X_OFF),
+};
+
+static struct pin_config_state __initdata foo_pinconf_states[] = {
+	{
+		.name = "active",
+		.ctrl_dev_name = "foo",
+		.pin_configs = foo_pin_config_active,
+		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_active),
+		.apply_on_init = true,
+	},
+	{
+		.name = "idle",
+		.ctrl_dev_name = "foo",
+		.pin_configs = foo_pin_config_idle,
+		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
+	},
+	{
+		.name = "off",
+		.ctrl_dev_name = "foo",
+		.pin_configs = foo_pin_config_idle,
+		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
+		.apply_on_exit = true,
+	},
+};
+
+The first three arrays of configurations are simply the names of some pins
+and the custom configuration that is associated with each.
+
+So there are three states for the pin controller, and the "active" state will
+pull two GPIO pins, whereas "idle" will ground them and "off" will deactivate
+them completely. In this case "active" will be auto-applied when your pin
+controller registers, and "off" will be auto-applied when the controller is
+removed. At runtime, the system may want to ground both pins by simply calling:
+
+ret = pinconf_set_state("foo", "idle");
+
+Which will apply the "idle" configuration.
+
+For simple systems that just configure the pins on boot and then forget about
+them, the first configuration table may be sufficient.
+
+
 Generic pin configuration
 =========================
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 569bdb3..15d5adf 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -541,6 +541,7 @@ static void pinctrl_init_debugfs(void)
 	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
 			    debugfs_root, NULL, &pinctrl_devices_ops);
 	pinmux_init_debugfs(debugfs_root);
+	pinconf_init_debugfs(debugfs_root);
 }
 
 #else /* CONFIG_DEBUG_FS */
@@ -622,6 +623,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	list_add(&pctldev->node, &pinctrldev_list);
 	mutex_unlock(&pinctrldev_list_mutex);
 	pinmux_hog_maps(pctldev);
+	pinconf_apply_initexit(pctldev, true);
 	return pctldev;
 
 out_err:
@@ -642,6 +644,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 		return;
 
 	pinmux_unhog_maps(pctldev);
+	pinconf_apply_initexit(pctldev, false);
 	/* TODO: check that no pinmuxes are still active? */
 	mutex_lock(&pinctrldev_list_mutex);
 	list_del(&pctldev->node);
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 819fcd9..5830786 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -23,6 +23,10 @@
 #include "core.h"
 #include "pinconf.h"
 
+/* Global pin configuration states */
+static struct pin_config_state *pinconf_states;
+static unsigned pinconf_states_num;
+
 int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 			   unsigned long *config)
 {
@@ -138,11 +142,10 @@ int pin_config_group_get(const char *dev_name, const char *pin_group,
 }
 EXPORT_SYMBOL(pin_config_group_get);
 
-
-int pin_config_group_set(const char *dev_name, const char *pin_group,
-			 unsigned long config)
+static int pin_config_group_set_for_group(struct pinctrl_dev *pctldev,
+					  const char *pin_group,
+					  unsigned long config)
 {
-	struct pinctrl_dev *pctldev;
 	const struct pinconf_ops *ops;
 	const struct pinctrl_ops *pctlops;
 	int selector;
@@ -151,9 +154,6 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 	int ret;
 	int i;
 
-	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
-	if (!pctldev)
-		return -EINVAL;
 	ops = pctldev->desc->confops;
 	pctlops = pctldev->desc->pctlops;
 
@@ -203,6 +203,20 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 
 	return 0;
 }
+
+int pin_config_group_set(const char *dev_name, const char *pin_group,
+			 unsigned long config)
+{
+	struct pinctrl_dev *pctldev;
+
+	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
+	if (!pctldev)
+		return -EINVAL;
+
+	return pin_config_group_set_for_group(pctldev,
+					      pin_group,
+					      config);
+}
 EXPORT_SYMBOL(pin_config_group_set);
 
 int pinconf_check_ops(const struct pinconf_ops *ops)
@@ -216,6 +230,167 @@ int pinconf_check_ops(const struct pinconf_ops *ops)
 	return 0;
 }
 
+int __init pinconf_register_pin_states(struct pin_config_state const *states,
+				       unsigned num_states)
+{
+	void *tmp;
+	int i;
+
+	pr_debug("add %d pin configuration states\n", num_states);
+
+	/* First sanity check the new states */
+	for (i = 0; i < num_states; i++) {
+		if (!states[i].name) {
+			pr_err("failed to register config state %d: "
+			       "no state name given\n", i);
+			return -EINVAL;
+		}
+
+		if (!states[i].ctrl_dev && !states[i].ctrl_dev_name) {
+			pr_err("failed to register config state %s (%d): "
+			       "no pin control device given\n",
+			       states[i].name, i);
+			return -EINVAL;
+		}
+
+		if (!states[i].pin_configs && !states[i].pin_group_configs) {
+			pr_err("failed to register config state %s (%d): "
+			       "no pin or group states defined in it\n",
+			       states[i].name, i);
+			return -EINVAL;
+		}
+		else
+			pr_debug("register pin config state %s\n",
+				 states[i].name);
+	}
+
+	/*
+	 * Make a copy of the config state array - string pointers will end up
+	 * in the kernel const section anyway so these do not need to be deep
+	 * copied. Note that the pointers to the config tuples are *not* being
+	 * copied as of now, these cannot be declared __init_data.
+	 */
+	if (!pinconf_states_num) {
+		/* On first call, just copy them */
+		tmp = kmemdup(states,
+			      sizeof(struct pin_config_state) * num_states,
+			      GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+	} else {
+		/* Subsequent calls, reallocate array to new size */
+		size_t oldsize = sizeof(struct pin_config_state) *
+			pinconf_states_num;
+		size_t newsize = sizeof(struct pin_config_state) * num_states;
+
+		tmp = krealloc(pinconf_states, oldsize + newsize, GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+		memcpy((tmp + oldsize), states, newsize);
+	}
+
+	pinconf_states = tmp;
+	pinconf_states_num += num_states;
+	return 0;
+}
+
+/**
+ * pinconf_apply_state() - apply a certain pin configuration state on a certain
+ * pin controller
+ * @pctldev: the pin controller to apply the state to
+ * @pstate: the configuration state to apply on the pin controller
+ */
+static int pinconf_apply_state(struct pinctrl_dev *pctldev,
+			       struct pin_config_state const *pstate)
+{
+	int ret, i;
+
+	/* Apply group configs first */
+	for (i = 0; i < pstate->nr_pin_group_configs; i++) {
+		const struct pin_group_config *groupconf =
+			&pstate->pin_group_configs[i];
+
+		ret = pin_config_group_set_for_group(pctldev,
+						     groupconf->group_name,
+						     groupconf->config);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Then apply individual pin configs */
+	for (i = 0; i < pstate->nr_pin_configs; i++) {
+		const struct pin_config *pinconf =
+			&pstate->pin_configs[i];
+		int pin;
+
+		pin = pin_get_from_name(pctldev, pinconf->pin_name);
+		if (pin < 0)
+			return pin;
+
+		ret = pin_config_set_for_pin(pctldev, pin, pinconf->config);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * pinconf_set_state() - sets the state for pins and groups on a pin controller
+ * @dev_name: the textual name for the pin controller
+ * @state: name of the state to set
+ */
+int pinconf_activate_state(const char *dev_name, const char *state)
+{
+	struct pinctrl_dev *pctldev;
+	int i;
+
+	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
+	if (!pctldev)
+		return -EINVAL;
+
+	for (i = 0; i < pinconf_states_num; i++) {
+		struct pin_config_state const *pstate = &pinconf_states[i];
+		int ret;
+
+		if (!strcmp(pstate->name, state)) {
+			ret = pinconf_apply_state(pctldev, pstate);
+			if (ret)
+				dev_err(pctldev->dev,
+					"failed to apply state %s\n",
+					pstate->name);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * pinconf_apply_initexit() - check for states to be applied on init and exit
+ * @pctldev: pin controller to be checked
+ * @init: true for checking during registration of the pin controller, false
+ *	for checking during removal of the pin controller
+ */
+void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init)
+{
+	int i;
+
+	for (i = 0; i < pinconf_states_num; i++) {
+		struct pin_config_state const *pstate = &pinconf_states[i];
+		int ret;
+
+		if ((pstate->apply_on_init && init) ||
+		    (pstate->apply_on_exit && !init)) {
+			ret = pinconf_apply_state(pctldev, pstate);
+			if (ret)
+				dev_err(pctldev->dev,
+					"failed to apply state %s on %s\n",
+					pstate->name,
+					init ? "init" : "exit");
+		}
+	}
+}
+
 #ifdef CONFIG_DEBUG_FS
 
 static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
@@ -294,6 +469,30 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 	return 0;
 }
 
+static int pinconf_states_show(struct seq_file *s, void *what)
+{
+	int i;
+
+	seq_puts(s, "Pinconfig state table:\n");
+
+	for (i = 0; i < pinconf_states_num; i++) {
+		struct pin_config_state const *pstate = &pinconf_states[i];
+
+		seq_printf(s, "%s:\n", pstate->name);
+		seq_printf(s, "  controlling device %s\n",
+			   pstate->ctrl_dev ? dev_name(pstate->ctrl_dev) :
+			   pstate->ctrl_dev_name);
+		seq_printf(s, "  %u pin configs, %u pin group configs\n",
+			   pstate->nr_pin_configs,
+			   pstate->nr_pin_group_configs);
+		seq_printf(s, "  apply on init: %s\n",
+			   pstate->apply_on_init ? "YES" : "NO");
+		seq_printf(s, "  apply on exit: %s\n",
+			   pstate->apply_on_exit ? "YES" : "NO");
+	}
+	return 0;
+}
+
 static int pinconf_pins_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, pinconf_pins_show, inode->i_private);
@@ -304,6 +503,11 @@ static int pinconf_groups_open(struct inode *inode, struct file *file)
 	return single_open(file, pinconf_groups_show, inode->i_private);
 }
 
+static int pinconf_states_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinconf_states_show, inode->i_private);
+}
+
 static const struct file_operations pinconf_pins_ops = {
 	.open		= pinconf_pins_open,
 	.read		= seq_read,
@@ -318,6 +522,13 @@ static const struct file_operations pinconf_groups_ops = {
 	.release	= single_release,
 };
 
+static const struct file_operations pinconf_states_ops = {
+	.open		= pinconf_states_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 void pinconf_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
@@ -327,4 +538,10 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
 			    devroot, pctldev, &pinconf_groups_ops);
 }
 
+void pinconf_init_debugfs(struct dentry *subsys_root)
+{
+	debugfs_create_file("pinconf-states", S_IFREG | S_IRUGO,
+			    subsys_root, NULL, &pinconf_states_ops);
+}
+
 #endif
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 81d71ae..f5872d4 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -14,8 +14,10 @@
 #ifdef CONFIG_PINCONF
 
 int pinconf_check_ops(const struct pinconf_ops *ops);
+void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 				 struct pinctrl_dev *pctldev);
+void pinconf_init_debugfs(struct dentry *subsys_root);
 int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 			   unsigned long *config);
 int pin_config_set_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
@@ -28,11 +30,20 @@ static inline int pinconf_check_ops(const struct pinconf_ops *ops)
 	return 0;
 }
 
+static inline void pinconf_apply_initexit(struct pinctrl_dev *pctldev,
+					  bool init)
+{
+}
+
 static inline void pinconf_init_device_debugfs(struct dentry *devroot,
 					       struct pinctrl_dev *pctldev)
 {
 }
 
+static inline void pinconf_init_debugfs(struct dentry *subsys_root)
+{
+}
+
 #endif
 
 /*
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index d0aecb7..a4f78bd 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -82,6 +82,67 @@ struct pinmux_map {
 	{ .name = a, .ctrl_dev_name = b, .function = c, .group = d, \
 	  .hog_on_boot = true }
 
+/**
+ * struct pin_config - configuration tuple for a single pin
+ * @pin_name: name of the pin affected by this configuration
+ * @config: configuration of the named pin (custom format)
+ */
+struct pin_config {
+	const char *pin_name;
+	u32 config;
+};
+
+/*
+ * Convenience macro to set up a simple config for a named pin
+ */
+#define PIN_CONFIG(a, b) \
+	{ .pin_name = a, .config = b }
+
+/**
+ * struct pin_group_config - configuration tuple for a group
+ * @group_name: name of the group affected by this configuration
+ * @config: configuration of the named group (custom format)
+ */
+struct pin_group_config {
+	const char *group_name;
+	u32 config;
+};
+
+/*
+ * Convenience macro to set up a simple config for a named pin group
+ */
+#define PIN_GROUP_CONFIG(a, b) \
+	{ .group_name = a, .config = b }
+
+/**
+ * struct pin_config_state - configuration for an array of pins
+ * @name: name of this configuration state
+ * @ctrl_dev: the pin control device to be used for this state, may be NULL
+ *	if you provide .ctrl_dev_name instead (this is more common)
+ * @ctrl_dev_name: the name of the device controlling this specific state,
+ *	the name must be the same as in your struct device*, may be NULL if
+ *	you provide .ctrl_dev instead
+ * @pin_configs: array of pin configuration tuples
+ * @nr_pin_configs: number of pin_configuration tuples in the array
+ * @pin_group_configs: array of pin group configuration tuples
+ * @nr_pin_group_configs: number of pin_group_configuration tuples in the array
+ * @apply_on_init: whether this configuration shall be auto-applied when
+ *	the pin controller is registered
+ * @apply_on_exit: whether this configuration shall be auto-applied when
+ *	the pin controller is unregistered
+ */
+struct pin_config_state {
+	const char *name;
+	struct device *ctrl_dev;
+	const char *ctrl_dev_name;
+	const struct pin_config *pin_configs;
+	unsigned nr_pin_configs;
+	const struct pin_group_config *pin_group_configs;
+	unsigned nr_pin_group_configs;
+	bool apply_on_init;
+	bool apply_on_exit;
+};
+
 #ifdef CONFIG_PINMUX
 
 extern int pinmux_register_mappings(struct pinmux_map const *map,
@@ -96,4 +157,22 @@ static inline int pinmux_register_mappings(struct pinmux_map const *map,
 }
 
 #endif /* !CONFIG_PINMUX */
+
+#ifdef CONFIG_PINCONF
+
+extern int pinconf_register_pin_states(struct pin_config_state const
+				       *states,
+				       unsigned num_states);
+
+#else
+
+static inline int pinconf_register_pin_states(struct pin_config_state const
+					*states,
+					unsigned num_states)
+{
+	return 0;
+}
+
+#endif /* !CONFIG_PINCONF */
+
 #endif
-- 
1.7.8

RE: [PATCH] pinctrl: pin configuration states

[Dong Aisheng-B29396]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Linus Walleij
> Sent: Monday, January 16, 2012 10:52 PM
> To: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: Stephen Warren; Grant Likely; Barry Song; Guo Shawn-R65073; Thomas Abraham;
> Dong Aisheng; Rajendra Nayak; Haojian Zhuang; Linus Walleij
> Subject: [PATCH] pinctrl: pin configuration states
> 
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This introduce a pin configuration state structure and activation functions
> similar to the pinmux map. It basically names a few states and define the custom
> configuration values to be applied to groups and pins alike when switching to a
> certain state.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Maybe this is closer to what people want for configuring the pins in their
> platforms? 
Yes, this patch provides a way to us to do platform specific pinconfig initialization,
And different pinctrl states also supported.

> I am not fully satisfied with this solution because it basically
> maintains status quo of the big static tables found in many ARM SoCs, but it
> does instill a common scheme for doing it.
> 
> Also there is no way for a pinmux and its associated device to switch states in
> this solution. However one does not exclude the other, we might want per-device
> associated pinmux and group states *also*.
> 
I guess this may be needed for some devices wanting to set different pin configuration
During runtime.
For example, IMX6Q usdhc needs to set different pinconfig when working on different
Clock frequency after detecting different cards.

> Comments welcome.
> ---
>  Documentation/pinctrl.txt       |   63 +++++++++++
>  drivers/pinctrl/core.c          |    3 +
>  drivers/pinctrl/pinconf.c       |  231 +++++++++++++++++++++++++++++++++++++-
>  drivers/pinctrl/pinconf.h       |   11 ++
>  include/linux/pinctrl/machine.h |   79 +++++++++++++
>  5 files changed, 380 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt index
> 1851f1d..29845c4 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -277,6 +277,69 @@ case each individual pin will be treated by separate
> pin_config_set() calls as  well.
> 
> 
> +Pin configuration states
> +========================
> +
> +To help platforms to set up initial pin configuration and transit the
> +entire set of pins on a pin controller between different states, pin
> +controller states are supported by creating named states per
> +controller. A board/machine can define a pin configuration like this:
> +
> +static const struct pin_config foo_pin_config_active[] = {
> +	PIN_CONFIG("GPIO0", PLATFORM_X_PULL_UP),
> +	PIN_CONFIG("GPIO1", PLATFORM_X_PULL_DOWN), };
> +
> +static const struct pin_config foo_pin_config_idle[] = {
> +	PIN_CONFIG("GPIO0", PLATFORM_X_GROUND),
> +	PIN_CONFIG("GPIO1", PLATFORM_X_GROUND), };
> +
> +static const struct pin_config foo_pin_config_idle[] = {
s/foo_pin_config_idle/foo_pin_config_off

> +	PIN_CONFIG("GPIO0", PLATFORM_X_OFF),
> +	PIN_CONFIG("GPIO1", PLATFORM_X_OFF),
> +};
> +
> +static struct pin_config_state __initdata foo_pinconf_states[] = {
> +	{
> +		.name = "active",
> +		.ctrl_dev_name = "foo",
> +		.pin_configs = foo_pin_config_active,
> +		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_active),
> +		.apply_on_init = true,
> +	},
> +	{
> +		.name = "idle",
> +		.ctrl_dev_name = "foo",
> +		.pin_configs = foo_pin_config_idle,
> +		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
> +	},
> +	{
> +		.name = "off",
> +		.ctrl_dev_name = "foo",
> +		.pin_configs = foo_pin_config_idle,
s/foo_pin_config_idle/foo_pin_config_off

> +		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
> +		.apply_on_exit = true,
> +	},
> +};
> +
> +The first three arrays of configurations are simply the names of some
> +pins and the custom configuration that is associated with each.
> +
> +So there are three states for the pin controller, and the "active"
> +state will pull two GPIO pins,
This sentence does not align with what you define above.

> whereas "idle" will ground them and
> +"off" will deactivate them completely. In this case "active" will be
> +auto-applied when your pin controller registers, and "off" will be
> +auto-applied when the controller is removed. At runtime, the system may want to
> ground both pins by simply calling:
> +
> +ret = pinconf_set_state("foo", "idle");
> +
Maybe:
s/"foo"/"pinctrl-foo"
Easier to understand.

> +Which will apply the "idle" configuration.
> +
> +For simple systems that just configure the pins on boot and then forget
> +about them, the first configuration table may be sufficient.
> +
> +
>  Generic pin configuration
>  =========================
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index
> 569bdb3..15d5adf 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -541,6 +541,7 @@ static void pinctrl_init_debugfs(void)
>  	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
>  			    debugfs_root, NULL, &pinctrl_devices_ops);
>  	pinmux_init_debugfs(debugfs_root);
> +	pinconf_init_debugfs(debugfs_root);
>  }
> 
>  #else /* CONFIG_DEBUG_FS */
> @@ -622,6 +623,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc
> *pctldesc,
>  	list_add(&pctldev->node, &pinctrldev_list);
>  	mutex_unlock(&pinctrldev_list_mutex);
>  	pinmux_hog_maps(pctldev);
> +	pinconf_apply_initexit(pctldev, true);
>  	return pctldev;
> 
>  out_err:
> @@ -642,6 +644,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
>  		return;
> 
>  	pinmux_unhog_maps(pctldev);
> +	pinconf_apply_initexit(pctldev, false);
>  	/* TODO: check that no pinmuxes are still active? */
>  	mutex_lock(&pinctrldev_list_mutex);
>  	list_del(&pctldev->node);
> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c index
> 819fcd9..5830786 100644
> --- a/drivers/pinctrl/pinconf.c
> +++ b/drivers/pinctrl/pinconf.c
> @@ -23,6 +23,10 @@
>  #include "core.h"
>  #include "pinconf.h"
> 
> +/* Global pin configuration states */
> +static struct pin_config_state *pinconf_states; static unsigned
> +pinconf_states_num;
> +
>  int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
>  			   unsigned long *config)
>  {
> @@ -138,11 +142,10 @@ int pin_config_group_get(const char *dev_name, const char
> *pin_group,  }  EXPORT_SYMBOL(pin_config_group_get);
> 
> -
> -int pin_config_group_set(const char *dev_name, const char *pin_group,
> -			 unsigned long config)
> +static int pin_config_group_set_for_group(struct pinctrl_dev *pctldev,
> +					  const char *pin_group,
> +					  unsigned long config)
>  {
> -	struct pinctrl_dev *pctldev;
>  	const struct pinconf_ops *ops;
>  	const struct pinctrl_ops *pctlops;
>  	int selector;
> @@ -151,9 +154,6 @@ int pin_config_group_set(const char *dev_name, const char
> *pin_group,
>  	int ret;
>  	int i;
> 
> -	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> -	if (!pctldev)
> -		return -EINVAL;
>  	ops = pctldev->desc->confops;
>  	pctlops = pctldev->desc->pctlops;
> 
> @@ -203,6 +203,20 @@ int pin_config_group_set(const char *dev_name, const char
> *pin_group,
> 
>  	return 0;
>  }
> +
> +int pin_config_group_set(const char *dev_name, const char *pin_group,
> +			 unsigned long config)
> +{
> +	struct pinctrl_dev *pctldev;
> +
> +	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	return pin_config_group_set_for_group(pctldev,
> +					      pin_group,
> +					      config);
> +}
>  EXPORT_SYMBOL(pin_config_group_set);
> 
>  int pinconf_check_ops(const struct pinconf_ops *ops) @@ -216,6 +230,167 @@ int
> pinconf_check_ops(const struct pinconf_ops *ops)
>  	return 0;
>  }
> 
> +int __init pinconf_register_pin_states(struct pin_config_state const *states,
> +				       unsigned num_states)
> +{
> +	void *tmp;
> +	int i;
> +
> +	pr_debug("add %d pin configuration states\n", num_states);
> +
> +	/* First sanity check the new states */
> +	for (i = 0; i < num_states; i++) {
> +		if (!states[i].name) {
> +			pr_err("failed to register config state %d: "
> +			       "no state name given\n", i);
> +			return -EINVAL;
> +		}
> +
> +		if (!states[i].ctrl_dev && !states[i].ctrl_dev_name) {
> +			pr_err("failed to register config state %s (%d): "
> +			       "no pin control device given\n",
> +			       states[i].name, i);
> +			return -EINVAL;
> +		}
> +
> +		if (!states[i].pin_configs && !states[i].pin_group_configs) {
> +			pr_err("failed to register config state %s (%d): "
> +			       "no pin or group states defined in it\n",
> +			       states[i].name, i);
> +			return -EINVAL;
> +		}
> +		else
Do we need this 'else'?

> +			pr_debug("register pin config state %s\n",
> +				 states[i].name);
> +	}
> +
> +	/*
> +	 * Make a copy of the config state array - string pointers will end up
> +	 * in the kernel const section anyway so these do not need to be deep
> +	 * copied. Note that the pointers to the config tuples are *not* being
> +	 * copied as of now, these cannot be declared __init_data.
> +	 */
> +	if (!pinconf_states_num) {
> +		/* On first call, just copy them */
> +		tmp = kmemdup(states,
> +			      sizeof(struct pin_config_state) * num_states,
> +			      GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +	} else {
> +		/* Subsequent calls, reallocate array to new size */
> +		size_t oldsize = sizeof(struct pin_config_state) *
> +			pinconf_states_num;
> +		size_t newsize = sizeof(struct pin_config_state) * num_states;
> +
> +		tmp = krealloc(pinconf_states, oldsize + newsize, GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +		memcpy((tmp + oldsize), states, newsize);
> +	}
> +
> +	pinconf_states = tmp;
> +	pinconf_states_num += num_states;
> +	return 0;
> +}
> +
> +/**
> + * pinconf_apply_state() - apply a certain pin configuration state on a
> +certain
> + * pin controller
> + * @pctldev: the pin controller to apply the state to
> + * @pstate: the configuration state to apply on the pin controller  */
> +static int pinconf_apply_state(struct pinctrl_dev *pctldev,
> +			       struct pin_config_state const *pstate) {
> +	int ret, i;
> +
> +	/* Apply group configs first */
> +	for (i = 0; i < pstate->nr_pin_group_configs; i++) {
> +		const struct pin_group_config *groupconf =
> +			&pstate->pin_group_configs[i];
> +
> +		ret = pin_config_group_set_for_group(pctldev,
> +						     groupconf->group_name,
> +						     groupconf->config);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* Then apply individual pin configs */
> +	for (i = 0; i < pstate->nr_pin_configs; i++) {
> +		const struct pin_config *pinconf =
> +			&pstate->pin_configs[i];
> +		int pin;
> +
> +		pin = pin_get_from_name(pctldev, pinconf->pin_name);
> +		if (pin < 0)
> +			return pin;
> +
> +		ret = pin_config_set_for_pin(pctldev, pin, pinconf->config);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pinconf_set_state() - sets the state for pins and groups on a pin
> +controller
> + * @dev_name: the textual name for the pin controller
> + * @state: name of the state to set
> + */
> +int pinconf_activate_state(const char *dev_name, const char *state) {
s/ pinconf_activate_state/ pinconf_set_state ?
Also, I did not see this function in the .h file.

> +	struct pinctrl_dev *pctldev;
> +	int i;
> +
> +	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	for (i = 0; i < pinconf_states_num; i++) {
> +		struct pin_config_state const *pstate = &pinconf_states[i];
> +		int ret;
> +
> +		if (!strcmp(pstate->name, state)) {
> +			ret = pinconf_apply_state(pctldev, pstate);
> +			if (ret)
> +				dev_err(pctldev->dev,
> +					"failed to apply state %s\n",
> +					pstate->name);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pinconf_apply_initexit() - check for states to be applied on init
> +and exit
> + * @pctldev: pin controller to be checked
> + * @init: true for checking during registration of the pin controller, false
> + *	for checking during removal of the pin controller
> + */
> +void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init) {
> +	int i;
> +
> +	for (i = 0; i < pinconf_states_num; i++) {
> +		struct pin_config_state const *pstate = &pinconf_states[i];
> +		int ret;
> +
> +		if ((pstate->apply_on_init && init) ||
> +		    (pstate->apply_on_exit && !init)) {
> +			ret = pinconf_apply_state(pctldev, pstate);
> +			if (ret)
> +				dev_err(pctldev->dev,
> +					"failed to apply state %s on %s\n",
> +					pstate->name,
> +					init ? "init" : "exit");
> +		}
> +	}
> +}
> +
>  #ifdef CONFIG_DEBUG_FS
> 
>  static void pinconf_dump_pin(struct pinctrl_dev *pctldev, @@ -294,6 +469,30 @@
> static int pinconf_groups_show(struct seq_file *s, void *what)
>  	return 0;
>  }
> 
> +static int pinconf_states_show(struct seq_file *s, void *what) {
> +	int i;
> +
> +	seq_puts(s, "Pinconfig state table:\n");
> +
> +	for (i = 0; i < pinconf_states_num; i++) {
> +		struct pin_config_state const *pstate = &pinconf_states[i];
> +
> +		seq_printf(s, "%s:\n", pstate->name);
> +		seq_printf(s, "  controlling device %s\n",
> +			   pstate->ctrl_dev ? dev_name(pstate->ctrl_dev) :
> +			   pstate->ctrl_dev_name);
> +		seq_printf(s, "  %u pin configs, %u pin group configs\n",
> +			   pstate->nr_pin_configs,
> +			   pstate->nr_pin_group_configs);
> +		seq_printf(s, "  apply on init: %s\n",
> +			   pstate->apply_on_init ? "YES" : "NO");
> +		seq_printf(s, "  apply on exit: %s\n",
> +			   pstate->apply_on_exit ? "YES" : "NO");
> +	}
> +	return 0;
> +}
> +
>  static int pinconf_pins_open(struct inode *inode, struct file *file)  {
>  	return single_open(file, pinconf_pins_show, inode->i_private); @@ -304,6
> +503,11 @@ static int pinconf_groups_open(struct inode *inode, struct file *file)
>  	return single_open(file, pinconf_groups_show, inode->i_private);  }
> 
> +static int pinconf_states_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, pinconf_states_show, inode->i_private); }
> +
>  static const struct file_operations pinconf_pins_ops = {
>  	.open		= pinconf_pins_open,
>  	.read		= seq_read,
> @@ -318,6 +522,13 @@ static const struct file_operations pinconf_groups_ops = {
>  	.release	= single_release,
>  };
> 
> +static const struct file_operations pinconf_states_ops = {
> +	.open		= pinconf_states_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
>  void pinconf_init_device_debugfs(struct dentry *devroot,
>  			 struct pinctrl_dev *pctldev)
>  {
> @@ -327,4 +538,10 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
>  			    devroot, pctldev, &pinconf_groups_ops);  }
> 
> +void pinconf_init_debugfs(struct dentry *subsys_root) {
> +	debugfs_create_file("pinconf-states", S_IFREG | S_IRUGO,
> +			    subsys_root, NULL, &pinconf_states_ops); }
> +
>  #endif
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h index
> 81d71ae..f5872d4 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -14,8 +14,10 @@
>  #ifdef CONFIG_PINCONF
> 
>  int pinconf_check_ops(const struct pinconf_ops *ops);
> +void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init);
>  void pinconf_init_device_debugfs(struct dentry *devroot,
>  				 struct pinctrl_dev *pctldev);
> +void pinconf_init_debugfs(struct dentry *subsys_root);
>  int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
>  			   unsigned long *config);
>  int pin_config_set_for_pin(struct pinctrl_dev *pctldev, unsigned pin, @@ -28,11
> +30,20 @@ static inline int pinconf_check_ops(const struct pinconf_ops *ops)
>  	return 0;
>  }
> 
> +static inline void pinconf_apply_initexit(struct pinctrl_dev *pctldev,
> +					  bool init)
> +{
> +}
> +
>  static inline void pinconf_init_device_debugfs(struct dentry *devroot,
>  					       struct pinctrl_dev *pctldev)  {  }
> 
> +static inline void pinconf_init_debugfs(struct dentry *subsys_root) { }
> +
>  #endif
> 
>  /*
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index d0aecb7..a4f78bd 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -82,6 +82,67 @@ struct pinmux_map {
>  	{ .name = a, .ctrl_dev_name = b, .function = c, .group = d, \
>  	  .hog_on_boot = true }
> 
> +/**
> + * struct pin_config - configuration tuple for a single pin
> + * @pin_name: name of the pin affected by this configuration
> + * @config: configuration of the named pin (custom format)  */ struct
> +pin_config {
> +	const char *pin_name;
> +	u32 config;
> +};
> +
> +/*
> + * Convenience macro to set up a simple config for a named pin  */
> +#define PIN_CONFIG(a, b) \
> +	{ .pin_name = a, .config = b }
> +
> +/**
> + * struct pin_group_config - configuration tuple for a group
> + * @group_name: name of the group affected by this configuration
> + * @config: configuration of the named group (custom format)  */ struct
> +pin_group_config {
> +	const char *group_name;
> +	u32 config;
> +};
> +
> +/*
> + * Convenience macro to set up a simple config for a named pin group
> +*/ #define PIN_GROUP_CONFIG(a, b) \
> +	{ .group_name = a, .config = b }
> +
> +/**
> + * struct pin_config_state - configuration for an array of pins
> + * @name: name of this configuration state
> + * @ctrl_dev: the pin control device to be used for this state, may be NULL
> + *	if you provide .ctrl_dev_name instead (this is more common)
> + * @ctrl_dev_name: the name of the device controlling this specific state,
> + *	the name must be the same as in your struct device*, may be NULL if
> + *	you provide .ctrl_dev instead
> + * @pin_configs: array of pin configuration tuples
> + * @nr_pin_configs: number of pin_configuration tuples in the array
> + * @pin_group_configs: array of pin group configuration tuples
> + * @nr_pin_group_configs: number of pin_group_configuration tuples in
> +the array
> + * @apply_on_init: whether this configuration shall be auto-applied when
> + *	the pin controller is registered
> + * @apply_on_exit: whether this configuration shall be auto-applied when
> + *	the pin controller is unregistered
> + */
> +struct pin_config_state {
> +	const char *name;
> +	struct device *ctrl_dev;
> +	const char *ctrl_dev_name;
> +	const struct pin_config *pin_configs;
> +	unsigned nr_pin_configs;
> +	const struct pin_group_config *pin_group_configs;
> +	unsigned nr_pin_group_configs;
> +	bool apply_on_init;
> +	bool apply_on_exit;
> +};
> +
>  #ifdef CONFIG_PINMUX
> 
>  extern int pinmux_register_mappings(struct pinmux_map const *map, @@ -96,4
> +157,22 @@ static inline int pinmux_register_mappings(struct pinmux_map const
> *map,  }
> 
>  #endif /* !CONFIG_PINMUX */
> +
> +#ifdef CONFIG_PINCONF
> +
> +extern int pinconf_register_pin_states(struct pin_config_state const
> +				       *states,
> +				       unsigned num_states);
> +
> +#else
> +
> +static inline int pinconf_register_pin_states(struct pin_config_state const
> +					*states,
> +					unsigned num_states)
> +{
> +	return 0;
> +}
> +
> +#endif /* !CONFIG_PINCONF */
> +
>  #endif
> --
> 1.7.8
> 
> --
> 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] pinctrl: pin configuration states

[Stephen Warren]

Linus Walleij wrote at Monday, January 16, 2012 7:52 AM:
> This introduce a pin configuration state structure and activation
> functions similar to the pinmux map. It basically names a few
> states and define the custom configuration values to be applied to
> groups and pins alike when switching to a certain state.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Maybe this is closer to what people want for configuring the pins
> in their platforms? I am not fully satisfied with this solution
> because it basically maintains status quo of the big static tables
> found in many ARM SoCs, but it does instill a common scheme for
> doing it.

At a pretty high conceptual level, this looks good. I've got some specific
comments below though. Overall, moving in the right direction for me.

> Also there is no way for a pinmux and its associated device to
> switch states in this solution. However one does not exclude the
> other, we might want per-device associated pinmux and group
> states *also*.

Yes, I think per-device states are a primary use-case...

> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h

> +/**
> + * struct pin_config_state - configuration for an array of pins
> + * @name: name of this configuration state
> + * @ctrl_dev: the pin control device to be used for this state, may be NULL
> + *	if you provide .ctrl_dev_name instead (this is more common)
> + * @ctrl_dev_name: the name of the device controlling this specific state,
> + *	the name must be the same as in your struct device*, may be NULL if
> + *	you provide .ctrl_dev instead
> + * @pin_configs: array of pin configuration tuples
> + * @nr_pin_configs: number of pin_configuration tuples in the array
> + * @pin_group_configs: array of pin group configuration tuples
> + * @nr_pin_group_configs: number of pin_group_configuration tuples in the array
> + * @apply_on_init: whether this configuration shall be auto-applied when
> + *	the pin controller is registered
> + * @apply_on_exit: whether this configuration shall be auto-applied when
> + *	the pin controller is unregistered
> + */
> +struct pin_config_state {
> +	const char *name;
> +	struct device *ctrl_dev;
> +	const char *ctrl_dev_name;
> +	const struct pin_config *pin_configs;
> +	unsigned nr_pin_configs;
> +	const struct pin_group_config *pin_group_configs;
> +	unsigned nr_pin_group_configs;
> +	bool apply_on_init;
> +	bool apply_on_exit;
> +};

In the long-run for per-device states, I think you'll need the per-
device fields that the pinmux mapping table has:

        struct device *dev;
        const char *dev_name;

In that case, .apply_on_init/.apply_on_exit don't really apply that well.
I'd suggest combining those two fields into a "bool hog" field just like
the pinmux mapping table, and defining that .name name must be "init" or
"exit" when .hog==true. That seems more in line with devices using the
.name field to define which states happen when.

That then makes the lookup structure of this new pin config table the
Same as the existing lookup structure of the pinmux mapping table.

Bikeshedding a but, the names init/fini are a slightly more match pair,
but perhaps less understandable? Probe/remove match the Linux driver
model better, but wouldn't map as well to device tree which isn't supposed
to be influenced by Linux.

> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt

> +static const struct pin_config foo_pin_config_idle[] = {
...
> +static const struct pin_config foo_pin_config_idle[] = {

s/_idle/_off/

> +static struct pin_config_state __initdata foo_pinconf_states[] = {
...
> +	{
> +		.name = "idle",
> +		.ctrl_dev_name = "foo",
> +		.pin_configs = foo_pin_config_idle,
> +		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
> +	},
> +	{
> +		.name = "off",
> +		.ctrl_dev_name = "foo",
> +		.pin_configs = foo_pin_config_idle,
> +		.nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),

s/_idle/_off/ on the 2 lines above.

> +		.apply_on_exit = true,
> +	},
> +};

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

> +int __init pinconf_register_pin_states(struct pin_config_state const *states,
> +				       unsigned num_states)
> +{
> +	void *tmp;
> +	int i;
> +
> +	pr_debug("add %d pin configuration states\n", num_states);
> +
> +	/* First sanity check the new states */
> +	for (i = 0; i < num_states; i++) {
> +		if (!states[i].name) {
> +			pr_err("failed to register config state %d: "
> +			       "no state name given\n", i);
> +			return -EINVAL;
> +		}
> +
> +		if (!states[i].ctrl_dev && !states[i].ctrl_dev_name) {

This should probably check that both aren't set at the same time to:

if (!!states[i].ctrl_dev == !!states[i].ctrl_dev_name)

> +			pr_err("failed to register config state %s (%d): "
> +			       "no pin control device given\n",
> +			       states[i].name, i);
> +			return -EINVAL;
> +		}
> +
> +		if (!states[i].pin_configs && !states[i].pin_group_configs) {

Don't you want to check the length fields to: Something more like:

if (!!states[i].pin_configs != !!states[i].nr_pin_configs)
    error
if (!!states[i].pin_group_configs != !!states[i].nr_pin_group_configs)
    error
if (!states[i].pin_configs && !states[i].pin_group_configs)
    error

> +			pr_err("failed to register config state %s (%d): "
> +			       "no pin or group states defined in it\n",
> +			       states[i].name, i);
> +			return -EINVAL;
> +		}
> +		else
> +			pr_debug("register pin config state %s\n",
> +				 states[i].name);
> +	}
> +
> +	/*
> +	 * Make a copy of the config state array - string pointers will end up
...
> +	pinconf_states = tmp;
> +	pinconf_states_num += num_states;

We need to allow multiple tables to be registered, for all the same
reasons we do for the pinmux mapping table. This implementation only
keeps the most recently registered table.

> +int pinconf_activate_state(const char *dev_name, const char *state)
> +{
> +	struct pinctrl_dev *pctldev;
> +	int i;
> +
> +	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	for (i = 0; i < pinconf_states_num; i++) {
> +		struct pin_config_state const *pstate = &pinconf_states[i];
> +		int ret;

Shouldn't this check that the entry is for the pctldev, not some other
pin control device?

> +		if (!strcmp(pstate->name, state)) {
> +			ret = pinconf_apply_state(pctldev, pstate);
> +			if (ret)
> +				dev_err(pctldev->dev,
> +					"failed to apply state %s\n",
> +					pstate->name);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pinconf_apply_initexit() - check for states to be applied on init and exit
> + * @pctldev: pin controller to be checked
> + * @init: true for checking during registration of the pin controller, false
> + *	for checking during removal of the pin controller
> + */
> +void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init)
> +{
> +	int i;
> +
> +	for (i = 0; i < pinconf_states_num; i++) {
> +		struct pin_config_state const *pstate = &pinconf_states[i];
> +		int ret;

Shouldn't this check that the entry is for the pctldev, not some other
pin control device?

> +		if ((pstate->apply_on_init && init) ||
> +		    (pstate->apply_on_exit && !init)) {
> +			ret = pinconf_apply_state(pctldev, pstate);
> +			if (ret)
> +				dev_err(pctldev->dev,
> +					"failed to apply state %s on %s\n",
> +					pstate->name,
> +					init ? "init" : "exit");
> +		}
> +	}
> +}

-- 
nvpublic

Re: [PATCH] pinctrl: pin configuration states

[Thomas Abraham]

Hi Linus,

On 16 January 2012 20:21, Linus Walleij <linus.walleij@stericsson.com> wrote:
>
> From: Linus Walleij <linus.walleij@linaro.org>
>
> This introduce a pin configuration state structure and activation
> functions similar to the pinmux map. It basically names a few
> states and define the custom configuration values to be applied to
> groups and pins alike when switching to a certain state.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Maybe this is closer to what people want for configuring the pins
> in their platforms? I am not fully satisfied with this solution
> because it basically maintains status quo of the big static tables
> found in many ARM SoCs, but it does instill a common scheme for
> doing it.
>
> Also there is no way for a pinmux and its associated device to
> switch states in this solution. However one does not exclude the
> other, we might want per-device associated pinmux and group
> states *also*.
>
> Comments welcome.
> ---
>  Documentation/pinctrl.txt       |   63 +++++++++++
>  drivers/pinctrl/core.c          |    3 +
>  drivers/pinctrl/pinconf.c       |  231 +++++++++++++++++++++++++++++++++++++-
>  drivers/pinctrl/pinconf.h       |   11 ++
>  include/linux/pinctrl/machine.h |   79 +++++++++++++
>  5 files changed, 380 insertions(+), 7 deletions(-)
>

For Samsung platforms, a majority of the pinconfig is setup during
driver's probe/release/suspend/resume. This patch is helpful for any
system wide pinconfig setup from platform code but since Samsung
platforms rely pinconfig to be initiated by the driver, there still
need be a way to setup pinconfig from a driver. There are few minor
comments listed below.

>
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index 1851f1d..29845c4 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -277,6 +277,69 @@ case each individual pin will be treated by separate pin_config_set() calls as
>  well.
>
>
> +Pin configuration states
> +========================
> +
> +To help platforms to set up initial pin configuration and transit the entire
> +set of pins on a pin controller between different states, pin controller
> +states are supported by creating named states per controller. A board/machine
> +can define a pin configuration like this:
> +
> +static const struct pin_config foo_pin_config_active[] = {
> +       PIN_CONFIG("GPIO0", PLATFORM_X_PULL_UP),
> +       PIN_CONFIG("GPIO1", PLATFORM_X_PULL_DOWN),
> +};
> +
> +static const struct pin_config foo_pin_config_idle[] = {
> +       PIN_CONFIG("GPIO0", PLATFORM_X_GROUND),
> +       PIN_CONFIG("GPIO1", PLATFORM_X_GROUND),
> +};
> +
> +static const struct pin_config foo_pin_config_idle[] = {
> +       PIN_CONFIG("GPIO0", PLATFORM_X_OFF),
> +       PIN_CONFIG("GPIO1", PLATFORM_X_OFF),
> +};
> +
> +static struct pin_config_state __initdata foo_pinconf_states[] = {
> +       {
> +               .name = "active",
> +               .ctrl_dev_name = "foo",
> +               .pin_configs = foo_pin_config_active,
> +               .nr_pin_configs = ARRAY_SIZE(foo_pin_config_active),
> +               .apply_on_init = true,
> +       },
> +       {
> +               .name = "idle",
> +               .ctrl_dev_name = "foo",
> +               .pin_configs = foo_pin_config_idle,
> +               .nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
> +       },
> +       {
> +               .name = "off",
> +               .ctrl_dev_name = "foo",
> +               .pin_configs = foo_pin_config_idle,
> +               .nr_pin_configs = ARRAY_SIZE(foo_pin_config_idle),
> +               .apply_on_exit = true,
> +       },
> +};
> +
> +The first three arrays of configurations are simply the names of some pins
> +and the custom configuration that is associated with each.
> +
> +So there are three states for the pin controller, and the "active" state will
> +pull two GPIO pins, whereas "idle" will ground them and "off" will deactivate
> +them completely. In this case "active" will be auto-applied when your pin
> +controller registers, and "off" will be auto-applied when the controller is
> +removed. At runtime, the system may want to ground both pins by simply calling:
> +
> +ret = pinconf_set_state("foo", "idle");

ret = pinconf_activate_state("foo", "idle");

> +
> +Which will apply the "idle" configuration.
> +
> +For simple systems that just configure the pins on boot and then forget about
> +them, the first configuration table may be sufficient.
> +
> +
>  Generic pin configuration
>  =========================
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 569bdb3..15d5adf 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -541,6 +541,7 @@ static void pinctrl_init_debugfs(void)
>        debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
>                            debugfs_root, NULL, &pinctrl_devices_ops);
>        pinmux_init_debugfs(debugfs_root);
> +       pinconf_init_debugfs(debugfs_root);
>  }
>
>  #else /* CONFIG_DEBUG_FS */
> @@ -622,6 +623,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>        list_add(&pctldev->node, &pinctrldev_list);
>        mutex_unlock(&pinctrldev_list_mutex);
>        pinmux_hog_maps(pctldev);
> +       pinconf_apply_initexit(pctldev, true);
>        return pctldev;
>
>  out_err:
> @@ -642,6 +644,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
>                return;
>
>        pinmux_unhog_maps(pctldev);
> +       pinconf_apply_initexit(pctldev, false);
>        /* TODO: check that no pinmuxes are still active? */
>        mutex_lock(&pinctrldev_list_mutex);
>        list_del(&pctldev->node);
> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
> index 819fcd9..5830786 100644
> --- a/drivers/pinctrl/pinconf.c
> +++ b/drivers/pinctrl/pinconf.c
> @@ -23,6 +23,10 @@
>  #include "core.h"
>  #include "pinconf.h"
>
> +/* Global pin configuration states */
> +static struct pin_config_state *pinconf_states;
> +static unsigned pinconf_states_num;
> +
>  int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
>                           unsigned long *config)
>  {
> @@ -138,11 +142,10 @@ int pin_config_group_get(const char *dev_name, const char *pin_group,
>  }
>  EXPORT_SYMBOL(pin_config_group_get);
>
> -
> -int pin_config_group_set(const char *dev_name, const char *pin_group,
> -                        unsigned long config)
> +static int pin_config_group_set_for_group(struct pinctrl_dev *pctldev,
> +                                         const char *pin_group,
> +                                         unsigned long config)
>  {
> -       struct pinctrl_dev *pctldev;
>        const struct pinconf_ops *ops;
>        const struct pinctrl_ops *pctlops;
>        int selector;
> @@ -151,9 +154,6 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
>        int ret;
>        int i;
>
> -       pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> -       if (!pctldev)
> -               return -EINVAL;
>        ops = pctldev->desc->confops;
>        pctlops = pctldev->desc->pctlops;
>
> @@ -203,6 +203,20 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
>
>        return 0;
>  }
> +
> +int pin_config_group_set(const char *dev_name, const char *pin_group,
> +                        unsigned long config)
> +{
> +       struct pinctrl_dev *pctldev;
> +
> +       pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> +       if (!pctldev)
> +               return -EINVAL;
> +
> +       return pin_config_group_set_for_group(pctldev,
> +                                             pin_group,
> +                                             config);
> +}
>  EXPORT_SYMBOL(pin_config_group_set);
>
>  int pinconf_check_ops(const struct pinconf_ops *ops)
> @@ -216,6 +230,167 @@ int pinconf_check_ops(const struct pinconf_ops *ops)
>        return 0;
>  }
>
> +int __init pinconf_register_pin_states(struct pin_config_state const *states,
> +                                      unsigned num_states)
> +{
> +       void *tmp;
> +       int i;
> +
> +       pr_debug("add %d pin configuration states\n", num_states);
> +
> +       /* First sanity check the new states */
> +       for (i = 0; i < num_states; i++) {
> +               if (!states[i].name) {
> +                       pr_err("failed to register config state %d: "
> +                              "no state name given\n", i);
> +                       return -EINVAL;
> +               }
> +
> +               if (!states[i].ctrl_dev && !states[i].ctrl_dev_name) {
> +                       pr_err("failed to register config state %s (%d): "
> +                              "no pin control device given\n",
> +                              states[i].name, i);
> +                       return -EINVAL;
> +               }
> +
> +               if (!states[i].pin_configs && !states[i].pin_group_configs) {
> +                       pr_err("failed to register config state %s (%d): "
> +                              "no pin or group states defined in it\n",
> +                              states[i].name, i);
> +                       return -EINVAL;
> +               }
> +               else
> +                       pr_debug("register pin config state %s\n",
> +                                states[i].name);
> +       }
> +
> +       /*
> +        * Make a copy of the config state array - string pointers will end up
> +        * in the kernel const section anyway so these do not need to be deep
> +        * copied. Note that the pointers to the config tuples are *not* being
> +        * copied as of now, these cannot be declared __init_data.
> +        */
> +       if (!pinconf_states_num) {
> +               /* On first call, just copy them */
> +               tmp = kmemdup(states,
> +                             sizeof(struct pin_config_state) * num_states,
> +                             GFP_KERNEL);
> +               if (!tmp)
> +                       return -ENOMEM;
> +       } else {
> +               /* Subsequent calls, reallocate array to new size */
> +               size_t oldsize = sizeof(struct pin_config_state) *
> +                       pinconf_states_num;
> +               size_t newsize = sizeof(struct pin_config_state) * num_states;
> +
> +               tmp = krealloc(pinconf_states, oldsize + newsize, GFP_KERNEL);
> +               if (!tmp)
> +                       return -ENOMEM;
> +               memcpy((tmp + oldsize), states, newsize);
> +       }
> +
> +       pinconf_states = tmp;
> +       pinconf_states_num += num_states;
> +       return 0;
> +}
> +
> +/**
> + * pinconf_apply_state() - apply a certain pin configuration state on a certain
> + * pin controller
> + * @pctldev: the pin controller to apply the state to
> + * @pstate: the configuration state to apply on the pin controller
> + */
> +static int pinconf_apply_state(struct pinctrl_dev *pctldev,
> +                              struct pin_config_state const *pstate)
> +{
> +       int ret, i;
> +
> +       /* Apply group configs first */
> +       for (i = 0; i < pstate->nr_pin_group_configs; i++) {
> +               const struct pin_group_config *groupconf =
> +                       &pstate->pin_group_configs[i];
> +
> +               ret = pin_config_group_set_for_group(pctldev,
> +                                                    groupconf->group_name,
> +                                                    groupconf->config);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       /* Then apply individual pin configs */
> +       for (i = 0; i < pstate->nr_pin_configs; i++) {
> +               const struct pin_config *pinconf =
> +                       &pstate->pin_configs[i];
> +               int pin;
> +
> +               pin = pin_get_from_name(pctldev, pinconf->pin_name);
> +               if (pin < 0)
> +                       return pin;
> +
> +               ret = pin_config_set_for_pin(pctldev, pin, pinconf->config);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * pinconf_set_state() - sets the state for pins and groups on a pin controller

* pinconf_activate_state() - sets the ....

> + * @dev_name: the textual name for the pin controller
> + * @state: name of the state to set
> + */
> +int pinconf_activate_state(const char *dev_name, const char *state)
> +{
> +       struct pinctrl_dev *pctldev;
> +       int i;
> +
> +       pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
> +       if (!pctldev)
> +               return -EINVAL;
> +
> +       for (i = 0; i < pinconf_states_num; i++) {
> +               struct pin_config_state const *pstate = &pinconf_states[i];
> +               int ret;
> +
> +               if (!strcmp(pstate->name, state)) {
> +                       ret = pinconf_apply_state(pctldev, pstate);
> +                       if (ret)
> +                               dev_err(pctldev->dev,
> +                                       "failed to apply state %s\n",
> +                                       pstate->name);
> +               }
> +       }
> +
> +       return 0;
> +}

EXPORT_SYMBOL(pinconf_activate_state);

> +
> +/**
> + * pinconf_apply_initexit() - check for states to be applied on init and exit
> + * @pctldev: pin controller to be checked
> + * @init: true for checking during registration of the pin controller, false
> + *     for checking during removal of the pin controller
> + */
> +void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init)
> +{
> +       int i;
> +
> +       for (i = 0; i < pinconf_states_num; i++) {
> +               struct pin_config_state const *pstate = &pinconf_states[i];
> +               int ret;
> +
> +               if ((pstate->apply_on_init && init) ||
> +                   (pstate->apply_on_exit && !init)) {
> +                       ret = pinconf_apply_state(pctldev, pstate);
> +                       if (ret)
> +                               dev_err(pctldev->dev,
> +                                       "failed to apply state %s on %s\n",
> +                                       pstate->name,
> +                                       init ? "init" : "exit");
> +               }
> +       }
> +}
> +
>  #ifdef CONFIG_DEBUG_FS
>
>  static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
> @@ -294,6 +469,30 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
>        return 0;
>  }
>
> +static int pinconf_states_show(struct seq_file *s, void *what)
> +{
> +       int i;
> +
> +       seq_puts(s, "Pinconfig state table:\n");
> +
> +       for (i = 0; i < pinconf_states_num; i++) {
> +               struct pin_config_state const *pstate = &pinconf_states[i];
> +
> +               seq_printf(s, "%s:\n", pstate->name);
> +               seq_printf(s, "  controlling device %s\n",
> +                          pstate->ctrl_dev ? dev_name(pstate->ctrl_dev) :
> +                          pstate->ctrl_dev_name);
> +               seq_printf(s, "  %u pin configs, %u pin group configs\n",
> +                          pstate->nr_pin_configs,
> +                          pstate->nr_pin_group_configs);
> +               seq_printf(s, "  apply on init: %s\n",
> +                          pstate->apply_on_init ? "YES" : "NO");
> +               seq_printf(s, "  apply on exit: %s\n",
> +                          pstate->apply_on_exit ? "YES" : "NO");
> +       }
> +       return 0;
> +}
> +
>  static int pinconf_pins_open(struct inode *inode, struct file *file)
>  {
>        return single_open(file, pinconf_pins_show, inode->i_private);
> @@ -304,6 +503,11 @@ static int pinconf_groups_open(struct inode *inode, struct file *file)
>        return single_open(file, pinconf_groups_show, inode->i_private);
>  }
>
> +static int pinconf_states_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, pinconf_states_show, inode->i_private);
> +}
> +
>  static const struct file_operations pinconf_pins_ops = {
>        .open           = pinconf_pins_open,
>        .read           = seq_read,
> @@ -318,6 +522,13 @@ static const struct file_operations pinconf_groups_ops = {
>        .release        = single_release,
>  };
>
> +static const struct file_operations pinconf_states_ops = {
> +       .open           = pinconf_states_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};
> +
>  void pinconf_init_device_debugfs(struct dentry *devroot,
>                         struct pinctrl_dev *pctldev)
>  {
> @@ -327,4 +538,10 @@ void pinconf_init_device_debugfs(struct dentry *devroot,
>                            devroot, pctldev, &pinconf_groups_ops);
>  }
>
> +void pinconf_init_debugfs(struct dentry *subsys_root)
> +{
> +       debugfs_create_file("pinconf-states", S_IFREG | S_IRUGO,
> +                           subsys_root, NULL, &pinconf_states_ops);
> +}
> +
>  #endif
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> index 81d71ae..f5872d4 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -14,8 +14,10 @@
>  #ifdef CONFIG_PINCONF
>
>  int pinconf_check_ops(const struct pinconf_ops *ops);
> +void pinconf_apply_initexit(struct pinctrl_dev *pctldev, bool init);

int pinconf_activate_state(const char *dev_name, const char *state);

>  void pinconf_init_device_debugfs(struct dentry *devroot,
>                                 struct pinctrl_dev *pctldev);
> +void pinconf_init_debugfs(struct dentry *subsys_root);
>  int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
>                           unsigned long *config);
>  int pin_config_set_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
> @@ -28,11 +30,20 @@ static inline int pinconf_check_ops(const struct pinconf_ops *ops)
>        return 0;
>  }

#else

>
> +static inline void pinconf_apply_initexit(struct pinctrl_dev *pctldev,
> +                                         bool init)
> +{
> +}
> +
>  static inline void pinconf_init_device_debugfs(struct dentry *devroot,
>                                               struct pinctrl_dev *pctldev)
>  {
>  }
>
> +static inline void pinconf_init_debugfs(struct dentry *subsys_root)
> +{
> +}
> +
>  #endif
>
>  /*
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index d0aecb7..a4f78bd 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -82,6 +82,67 @@ struct pinmux_map {
>        { .name = a, .ctrl_dev_name = b, .function = c, .group = d, \
>          .hog_on_boot = true }
>
> +/**
> + * struct pin_config - configuration tuple for a single pin
> + * @pin_name: name of the pin affected by this configuration
> + * @config: configuration of the named pin (custom format)
> + */
> +struct pin_config {
> +       const char *pin_name;
> +       u32 config;
> +};
> +
> +/*
> + * Convenience macro to set up a simple config for a named pin
> + */
> +#define PIN_CONFIG(a, b) \
> +       { .pin_name = a, .config = b }
> +
> +/**
> + * struct pin_group_config - configuration tuple for a group
> + * @group_name: name of the group affected by this configuration
> + * @config: configuration of the named group (custom format)
> + */
> +struct pin_group_config {
> +       const char *group_name;
> +       u32 config;
> +};
> +
> +/*
> + * Convenience macro to set up a simple config for a named pin group
> + */
> +#define PIN_GROUP_CONFIG(a, b) \
> +       { .group_name = a, .config = b }
> +
> +/**
> + * struct pin_config_state - configuration for an array of pins
> + * @name: name of this configuration state
> + * @ctrl_dev: the pin control device to be used for this state, may be NULL
> + *     if you provide .ctrl_dev_name instead (this is more common)
> + * @ctrl_dev_name: the name of the device controlling this specific state,
> + *     the name must be the same as in your struct device*, may be NULL if
> + *     you provide .ctrl_dev instead
> + * @pin_configs: array of pin configuration tuples
> + * @nr_pin_configs: number of pin_configuration tuples in the array
> + * @pin_group_configs: array of pin group configuration tuples
> + * @nr_pin_group_configs: number of pin_group_configuration tuples in the array
> + * @apply_on_init: whether this configuration shall be auto-applied when
> + *     the pin controller is registered
> + * @apply_on_exit: whether this configuration shall be auto-applied when
> + *     the pin controller is unregistered
> + */
> +struct pin_config_state {
> +       const char *name;
> +       struct device *ctrl_dev;
> +       const char *ctrl_dev_name;
> +       const struct pin_config *pin_configs;
> +       unsigned nr_pin_configs;
> +       const struct pin_group_config *pin_group_configs;
> +       unsigned nr_pin_group_configs;
> +       bool apply_on_init;
> +       bool apply_on_exit;
> +};
> +
>  #ifdef CONFIG_PINMUX
>
>  extern int pinmux_register_mappings(struct pinmux_map const *map,
> @@ -96,4 +157,22 @@ static inline int pinmux_register_mappings(struct pinmux_map const *map,
>  }
>
>  #endif /* !CONFIG_PINMUX */
> +
> +#ifdef CONFIG_PINCONF
> +
> +extern int pinconf_register_pin_states(struct pin_config_state const
> +                                      *states,
> +                                      unsigned num_states);
> +
> +#else
> +
> +static inline int pinconf_register_pin_states(struct pin_config_state const
> +                                       *states,
> +                                       unsigned num_states)
> +{
> +       return 0;
> +}
> +
> +#endif /* !CONFIG_PINCONF */
> +
>  #endif
> --
> 1.7.8
>

Thanks,
Thomas.

Re: [PATCH] pinctrl: pin configuration states

[Linus Walleij]

On Thu, Jan 19, 2012 at 8:03 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Monday, January 16, 2012 7:52 AM:
>> This introduce a pin configuration state structure and activation
>> functions similar to the pinmux map. (etc)

>> Also there is no way for a pinmux and its associated device to
>> switch states in this solution. However one does not exclude the
>> other, we might want per-device associated pinmux and group
>> states *also*.
>
> Yes, I think per-device states are a primary use-case...

OK added this for v2.

> In the long-run for per-device states, I think you'll need the per-
> device fields that the pinmux mapping table has:
>
>        struct device *dev;
>        const char *dev_name;

OK fixed this.

> In that case, .apply_on_init/.apply_on_exit don't really apply that well.
> I'd suggest combining those two fields into a "bool hog" field just like
> the pinmux mapping table, and defining that .name name must be "init" or
> "exit" when .hog==true. That seems more in line with devices using the
> .name field to define which states happen when.

I don't know, it looks fragile, "hog" is not intuitive since that state is
not going to stay or be "hogged" in any way. I'm staying with the
strongly-typed bools instead of hard-coded strings FTM.

But using #defined strings has the beauty of enforcing it a bit
across platforms so I'm still considering it.

> Bikeshedding a but, the names init/fini are a slightly more match pair,
> but perhaps less understandable?

Hehe :-) a bashism?

> Probe/remove match the Linux driver
> model better, but wouldn't map as well to device tree which isn't supposed
> to be influenced by Linux.

I'm trying to keep orthogonal to the device tree here, trying to take
that into account for everything just burns me out and distorts
focus currently. We can always refactor or translate whatever the
DT discussions lead up to.

>> +     /*
>> +      * Make a copy of the config state array - string pointers will end up
> ...
>> +     pinconf_states = tmp;
>> +     pinconf_states_num += num_states;
>
> We need to allow multiple tables to be registered, for all the same
> reasons we do for the pinmux mapping table. This implementation only
> keeps the most recently registered table.

? beats me.

Please check the code for how I realloc the tmp variable (in the v2
patch set), I cannot spot the problem. It was designed to allow exactly
multiple calls to add tables piece by piece.

The rest are fixed as I addressed yours, Dongs and Thomas' comments,
or so I hope.

Thanks,
Linus Walleij

RE: [PATCH] pinctrl: pin configuration states

[Stephen Warren]

Linus Walleij wrote at Wednesday, February 01, 2012 12:29 PM:
> On Thu, Jan 19, 2012 at 8:03 PM, Stephen Warren <swarren@nvidia.com> wrote:
...
> >> +     /*
> >> +      * Make a copy of the config state array - string pointers will end up
> > ...
> >> +     pinconf_states = tmp;
> >> +     pinconf_states_num += num_states;
> >
> > We need to allow multiple tables to be registered, for all the same
> > reasons we do for the pinmux mapping table. This implementation only
> > keeps the most recently registered table.
> 
> ? beats me.
> 
> Please check the code for how I realloc the tmp variable (in the v2
> patch set), I cannot spot the problem. It was designed to allow exactly
> multiple calls to add tables piece by piece.

Sorry, I must have been asleep when I wrote that...

I was expecting the implementation to copy each table separately and
store them in a list (since this would allow easy dynamic removel of the
entries too), hence when I saw the assignment to a single global, I
assumed it was just over-writing it; I guess I didn't even look at the
realloc above or noticed that it was assigning "tmp" not just the caller-
supplied parameter:-(

-- 
nvpublic