pinctrl: reserve pins when states are activated

Commit Message

Linus Walleij Oct. 18, 2012, 10:01 a.m. UTC

From: Linus Walleij <[e-mail]>

This switches the way that pins are reserved for multiplexing:

We used to do this when the map was parsed, at the creation of
the settings inside the pinctrl handle, in pinmux_map_to_setting().

However this does not work for us, because we want to use the
same set of pins with different devices at different times: the
current code assumes that the pin groups in a pinmux state will
only be used with one single device, albeit different groups can
be active at different times. For example if a single I2C driver
block is used to drive two different busses located on two
pin groups A and B, then the pins for all possible states of a
function are reserved when fetching the pinctrl handle: the
I2C bus can choose either set A or set B by a mux state at
runtime, but all pins in both group A and B (the superset) are
effectively reserved for that I2C function and mapped to the
device. Another device can never get in and use the pins in
group A, even if the device/function is using group B at the
moment.

Instead: let use reserve the pins when the state is activated
and drop them when the state is disabled, i.e. when we move to
another state. This way different devices/functions can use the
same pins at different times.

We know that this is an odd way of doing things, but we really
need to switch e.g. an SD-card slot to become a tracing output
sink at runtime: we plug in a special "tracing card" then mux
the pins that used to be an SD slot around to the tracing
unit and push out tracing data there instead of SD-card
traffic.

Cc: Patrice Chotard <[e-mail]>
Cc: Jean Nicolas Graux <[e-mail]>
Cc: Loic Pallardy <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
 Documentation/pinctrl.txt |  4 ++-
 drivers/pinctrl/core.c    |  2 ++
 drivers/pinctrl/core.h    |  2 ++
 drivers/pinctrl/pinmux.c  | 70 +++++++++++++++++------------------------------
 4 files changed, 32 insertions(+), 46 deletions(-)

Comments

Stephen Warren Oct. 18, 2012, 11:33 p.m. UTC | #1

On 10/18/2012 04:01 AM, Linus Walleij wrote:
> This switches the way that pins are reserved for multiplexing:

The conceptual change makes perfect sense to me.

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

>  void pinmux_free_setting(struct pinctrl_setting const *setting)
...
> +	/*
> +	 * If a setting is active, disable it so that all pins are released
> +	 */
> +	pinmux_disable_setting(setting);

"*If* a setting is active ..." implies there should be a C if statement
here to check if the setting is actually active before trying to disable it?

Linus Walleij Oct. 19, 2012, 12:51 p.m. UTC | #2

On Fri, Oct 19, 2012 at 1:33 AM, Stephen Warren <[e-mail]> wrote:
> On 10/18/2012 04:01 AM, Linus Walleij wrote:
>> This switches the way that pins are reserved for multiplexing:
>
> The conceptual change makes perfect sense to me.

Thanks...

>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>
>>  void pinmux_free_setting(struct pinctrl_setting const *setting)
> ...
>> +     /*
>> +      * If a setting is active, disable it so that all pins are released
>> +      */
>> +     pinmux_disable_setting(setting);
>
> "*If* a setting is active ..." implies there should be a C if statement
> here to check if the setting is actually active before trying to disable it?

Spot on.

Actually in core.c we're already checking this:

	case PIN_MAP_TYPE_MUX_GROUP:
		if (state == p->state)
			pinmux_disable_setting(setting);
		pinmux_free_setting(setting);
		break;

So this is just buggy driving the refcount down to negative.
(I think Jean-Nicolas noticed this phenomenon...)

I should just delete pinmux_free_setting().

Yours,
Linus Walleij

Yours,
Linus Walleij

Patch

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 3b4ee53..a1cd2f9 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1193,4 +1193,6 @@  foo_switch()
 	...
 }
 
-The above has to be done from process context.
+The above has to be done from process context. The reservation of the pins
+will be done when the state is activated, so in effect one specific pin
+can be used by different functions at different times on a running system.
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 0f1ec9e..5c6dff2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -563,6 +563,8 @@  static int add_setting(struct pinctrl *p, struct pinctrl_map const *map)
 		return -EPROBE_DEFER;
 	}
 
+	setting->dev_name = map->dev_name;
+
 	switch (map->type) {
 	case PIN_MAP_TYPE_MUX_GROUP:
 		ret = pinmux_map_to_setting(map, setting);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 1f40ff6..12f5694 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -105,12 +105,14 @@  struct pinctrl_setting_configs {
  * @type: the type of setting
  * @pctldev: pin control device handling to be programmed. Not used for
  *   PIN_MAP_TYPE_DUMMY_STATE.
+ * @dev_name: the name of the device using this state
  * @data: Data specific to the setting type
  */
 struct pinctrl_setting {
 	struct list_head node;
 	enum pinctrl_map_type type;
 	struct pinctrl_dev *pctldev;
+	const char *dev_name;
 	union {
 		struct pinctrl_setting_mux mux;
 		struct pinctrl_setting_configs configs;
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 9301a7a..dadc891 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -314,14 +314,11 @@  int pinmux_map_to_setting(struct pinctrl_map const *map,
 {
 	struct pinctrl_dev *pctldev = setting->pctldev;
 	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
-	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
 	char const * const *groups;
 	unsigned num_groups;
 	int ret;
 	const char *group;
 	int i;
-	const unsigned *pins;
-	unsigned num_pins;
 
 	if (!pmxops) {
 		dev_err(pctldev->dev, "does not support mux function\n");
@@ -376,53 +373,15 @@  int pinmux_map_to_setting(struct pinctrl_map const *map,
 	}
 	setting->data.mux.group = ret;
 
-	ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, &pins,
-				      &num_pins);
-	if (ret) {
-		dev_err(pctldev->dev,
-			"could not get pins for device %s group selector %d\n",
-			pinctrl_dev_get_name(pctldev), setting->data.mux.group);
-			return -ENODEV;
-	}
-
-	/* Try to allocate all pins in this group, one by one */
-	for (i = 0; i < num_pins; i++) {
-		ret = pin_request(pctldev, pins[i], map->dev_name, NULL);
-		if (ret) {
-			dev_err(pctldev->dev,
-				"could not request pin %d on device %s\n",
-				pins[i], pinctrl_dev_get_name(pctldev));
-			/* On error release all taken pins */
-			i--; /* this pin just failed */
-			for (; i >= 0; i--)
-				pin_free(pctldev, pins[i], NULL);
-			return -ENODEV;
-		}
-	}
-
 	return 0;
 }
 
 void pinmux_free_setting(struct pinctrl_setting const *setting)
 {
-	struct pinctrl_dev *pctldev = setting->pctldev;
-	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
-	const unsigned *pins;
-	unsigned num_pins;
-	int ret;
-	int i;
-
-	ret = pctlops->get_group_pins(pctldev, setting->data.mux.group,
-				      &pins, &num_pins);
-	if (ret) {
-		dev_err(pctldev->dev,
-			"could not get pins for device %s group selector %d\n",
-			pinctrl_dev_get_name(pctldev), setting->data.mux.group);
-		return;
-	}
-
-	for (i = 0; i < num_pins; i++)
-		pin_free(pctldev, pins[i], NULL);
+	/*
+	 * If a setting is active, disable it so that all pins are released
+	 */
+	pinmux_disable_setting(setting);
 }
 
 int pinmux_enable_setting(struct pinctrl_setting const *setting)
@@ -446,6 +405,22 @@  int pinmux_enable_setting(struct pinctrl_setting const *setting)
 		num_pins = 0;
 	}
 
+	/* Try to allocate all pins in this group, one by one */
+	for (i = 0; i < num_pins; i++) {
+		ret = pin_request(pctldev, pins[i], setting->dev_name, NULL);
+		if (ret) {
+			dev_err(pctldev->dev,
+				"could not request pin %d on device %s\n",
+				pins[i], pinctrl_dev_get_name(pctldev));
+			/* On error release all taken pins */
+			i--; /* this pin just failed */
+			for (; i >= 0; i--)
+				pin_free(pctldev, pins[i], NULL);
+			return -ENODEV;
+		}
+	}
+
+	/* Now that we have acquired the pins, encode the mux setting */
 	for (i = 0; i < num_pins; i++) {
 		desc = pin_desc_get(pctldev, pins[i]);
 		if (desc == NULL) {
@@ -482,6 +457,7 @@  void pinmux_disable_setting(struct pinctrl_setting const *setting)
 		num_pins = 0;
 	}
 
+	/* Flag the descs that no setting is active */
 	for (i = 0; i < num_pins; i++) {
 		desc = pin_desc_get(pctldev, pins[i]);
 		if (desc == NULL) {
@@ -493,6 +469,10 @@  void pinmux_disable_setting(struct pinctrl_setting const *setting)
 		desc->mux_setting = NULL;
 	}
 
+	/* And release the pins */
+	for (i = 0; i < num_pins; i++)
+		pin_free(pctldev, pins[i], NULL);
+
 	if (ops->disable)
 		ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group);
 }