Re: [PATCH] pinctrl: add params in disable_setting to differ difference usage

From: FanWu
Date: Tue May 13 2014 - 22:12:29 EST

On 05/14/2014 09:54 AM, fwu@xxxxxxxxxxx wrote:
From: Fan Wu <fwu@xxxxxxxxxxx>

The patch added params in disable_setting to differ the two possible usage,
1.Only want to disable the pin setting in SW aspect, param can be set to "0"
2.Want to disable the pin setting in both HW and SW aspect, param can be set to "1";

The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling operation which will
let Pin's desc->mux_usecount keep being added.

In the following case, the issue can be reproduced:
1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
"default" state
2)The Pin setting configuration in the two state is same, like the following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <&a_grp_setting &c_grp_setting>;
pinctrl-1 = <&b_grp_setting &c_grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following one:
c_grp_setting: c_grp_setting {
pinctrl-single,pins = <GPIO48 AF6>;
MFP_DEFAULT;
}
3)When switch the Pin state in the following official Pinctrl sequence:
pin = pinctrl_get();
state = pinctrl_lookup_state(wanted_state);
pinctrl_select_state(state);
pinctrl_put();

Test Result:
1)The switch change is completed as expectation, that is: component's
Pins configuration are changed according to the description in the
"wanted_state" group setting
2)The "desc->mux_usecount" of corresponding Pins in "c_group" is added without being
decreased, due to the original reason mentioned in this patch.

According to the comments in the original code, only the setting, in old state
but not in new state, will be "disable"(calling pinmux_disable_setting), which
is correct logic but not intact. We still need consider case that the setting
is in both old state and new state.
We can do this in the following two ways:
1) Avoid "enable"(calling pinmux_enable_setting) the Same Pins setting repeatedly.
2) "Disable"(calling pinmux_disable_setting) the Same Pins setting ahead of enable them.

Analysis:
1.The solution 2# is better because it can avoid too much iteration.
2.If we disable all of the setting in the old state and one/ones of the setting(s) is/are
existed in the new state, the Pin's mux function change may happen when
some SoC vendors defined the "pinctrl-single,function-off" in their DTS file.
old_setting=>disabled_setting=>new_setting.
3.In the pinmux framework, when Pin state is switched, the setting in the old state should be
marked as "disabled" in my understanding.

Conclusion:
Thus, the patch handle the issue mentioned above by disabling the c_grp_setting in SW asepct without
touch the HW corresponding register to avoid unnecessary Pin's mux
function change.

Change-Id: Ib3f7e7b6d4b401796733f5fbf52da73973f2efff
Signed-off-by: Fan Wu <fwu@xxxxxxxxxxx>
---
drivers/pinctrl/core.c | 17 +++--------------
drivers/pinctrl/pinmux.c | 4 ++--
drivers/pinctrl/pinmux.h | 2 +-
3 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..42877c3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -891,7 +891,7 @@ static void pinctrl_free_setting(bool disable_setting,
switch (setting->type) {
case PIN_MAP_TYPE_MUX_GROUP:
if (disable_setting)
- pinmux_disable_setting(setting);
+ pinmux_disable_setting(setting, 1);
pinmux_free_setting(setting);
break;
case PIN_MAP_TYPE_CONFIGS_PIN:
@@ -998,20 +998,9 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
* safe/disabled state.
*/
list_for_each_entry(setting, &p->state->settings, node) {
- bool found = false;
if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
continue;
- list_for_each_entry(setting2, &state->settings, node) {
- if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
- continue;
- if (setting2->data.mux.group ==
- setting->data.mux.group) {
- found = true;
- break;
- }
- }
- if (!found)
- pinmux_disable_setting(setting);
+ pinmux_disable_setting(setting, 0);
}
}

@@ -1055,7 +1044,7 @@ unapply_new_state:
* are free to be muxed by another apply_setting.
*/
if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
- pinmux_disable_setting(setting2);
+ pinmux_disable_setting(setting2, 1);
}

/* There's no infinite recursive loop here because p->state is NULL */
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 9248ce4..88544d4 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -465,7 +465,7 @@ err_pin_request:
return ret;
}

-void pinmux_disable_setting(struct pinctrl_setting const *setting)
+void pinmux_disable_setting(struct pinctrl_setting const *setting, int hwops)
{
struct pinctrl_dev *pctldev = setting->pctldev;
const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
@@ -516,7 +516,7 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
}
}

- if (ops->disable)
+ if (ops->disable && hwops)
ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group);
}

diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index d1a98b1..cd3a4af 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -29,7 +29,7 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
struct pinctrl_setting *setting);
void pinmux_free_setting(struct pinctrl_setting const *setting);
int pinmux_enable_setting(struct pinctrl_setting const *setting);
-void pinmux_disable_setting(struct pinctrl_setting const *setting);
+void pinmux_disable_setting(struct pinctrl_setting const *setting, int hwops);

#else



Dear Stephen,

I think we reach agreement on this topic that it is possible that a Pin's mux function may be changed if we disable the old setting. We can ignore the understanding about the "glitch" because the glitch I mentioned just means that the unexpected Pin's mux function change.

Just as the above patch expressed, I think we need to do SW pinmux setting disabling for all of the setting in old state ahead enabling setting in new state in pinctrl_select_state function.
Only when the user really wants to disable Pin's setting, like the disabling operation in "pinctrl_free_setting" function, we can disable the pin's mux setting in HW aspect.

Do you think this is a acceptable solution for the topic we discussed ?

Great thanks for this !

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/