From 2243a87d90b42eb38bc281957df3e57c712b5e56 Mon Sep 17 00:00:00 2001 From: Fan Wu Date: Mon, 9 Jun 2014 09:37:56 +0800 Subject: pinctrl: avoid duplicated calling enable_pinmux_setting for a pin What the patch does: 1. Call pinmux_disable_setting ahead of pinmux_enable_setting each time pinctrl_select_state is called 2. Remove the HW disable operation in pinmux_disable_setting function. 3. Remove the disable ops in struct pinmux_ops 4. Remove all the disable ops users in current code base. Notes: 1. Great thanks for the suggestion from Linus, Tony Lindgren and Stephen Warren and Everyone that shared comments on this patch. 2. The patch also includes comment fixes from Stephen Warren. The reason why we do this: 1. To avoid duplicated calling of the enable_setting operation without disabling operation inbetween which will let the pin descriptor desc->mux_usecount increase monotonously. 2. The HW pin disable operation is not useful for any of the existing platforms. And this can be used to avoid the HW glitch after using the item #1 modification. In the following case, the issue can be reproduced: 1. There is a driver that need to switch pin state dynamically, e.g. between "sleep" and "default" state 2. The pin setting configuration in a DTS node may be like this: 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 totally identical, maybe like following one: c_grp_setting: c_grp_setting { pinctrl-single,pins = ; } 3. When switching 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 is completed as expected, that is: the device's pin configuration is changed according to the description in the "wanted_state" group setting 2. The "desc->mux_usecount" of the corresponding pins in "c_group" is increased without being decreased, because the "desc" is for each physical pin while the setting is for each setting node in the DTS. Thus, if the "c_grp_setting" in pinctrl-0 is not disabled ahead of enabling "c_grp_setting" in pinctrl-1, the desc->mux_usecount will keep increasing without any chance to be decreased. According to the comments in the original code, only the setting, in old state but not in new state, will be "disabled" (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 to "enable"(calling pinmux_enable_setting) the "same pin setting" repeatedly 2. "Disable"(calling pinmux_disable_setting) the "same pin setting", actually two setting instances, ahead of enabling them. Analysis: 1. The solution #2 is better because it can avoid too much iteration. 2. If we disable all of the settings in the old state and one of the setting(s) exist in the new state, the pins 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 a pin state is switched, the setting in the old state should be marked as "disabled". Conclusion: 1. To Remove the HW disabling operation to above the glitch mentioned above. 2. Handle the issue mentioned above by disabling all of the settings in old state and then enable the all of the settings in new state. Signed-off-by: Fan Wu Acked-by: Stephen Warren Acked-by: Patrice Chotard Acked-by: Heiko Stuebner Acked-by: Maxime Coquelin Signed-off-by: Linus Walleij --- include/linux/pinctrl/pinmux.h | 2 -- 1 file changed, 2 deletions(-) (limited to 'include/linux/pinctrl') diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h index c15395031cb3..3097aafbeb24 100644 --- a/include/linux/pinctrl/pinmux.h +++ b/include/linux/pinctrl/pinmux.h @@ -70,8 +70,6 @@ struct pinmux_ops { unsigned * const num_groups); int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector, unsigned group_selector); - void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector, - unsigned group_selector); int (*gpio_request_enable) (struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned offset); -- cgit v1.2.3 From 03e9f0cac5da6af85758276cb4624caf5911f2b9 Mon Sep 17 00:00:00 2001 From: Linus Walleij Date: Wed, 3 Sep 2014 13:02:56 +0200 Subject: pinctrl: clean up after enable refactoring commit 2243a87d90b42eb38bc281957df3e57c712b5e56 "pinctrl: avoid duplicated calling enable_pinmux_setting for a pin" removed the .disable callback from the struct pinmux_ops, making the .enable() callback the only remaining callback. However .enable() is a bad name as it seems to imply that a muxing can also be disabled. Rename the callback to .set_mux() and also take this opportunity to clean out any remaining mentions of .disable() from the documentation. Acked-by: Stephen Warren Acked-by: Bjorn Andersson Acked-by: Fan Wu Signed-off-by: Linus Walleij --- include/linux/pinctrl/pinmux.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'include/linux/pinctrl') diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h index 3097aafbeb24..511bda9ed4bf 100644 --- a/include/linux/pinctrl/pinmux.h +++ b/include/linux/pinctrl/pinmux.h @@ -39,13 +39,12 @@ struct pinctrl_dev; * name can be used with the generic @pinctrl_ops to retrieve the * actual pins affected. The applicable groups will be returned in * @groups and the number of groups in @num_groups - * @enable: enable a certain muxing function with a certain pin group. The + * @set_mux: enable a certain muxing function with a certain pin group. The * driver does not need to figure out whether enabling this function * conflicts some other use of the pins in that group, such collisions * are handled by the pinmux subsystem. The @func_selector selects a * certain function whereas @group_selector selects a certain set of pins * to be used. On simple controllers the latter argument may be ignored - * @disable: disable a certain muxing selector with a certain pin group * @gpio_request_enable: requests and enables GPIO on a certain pin. * Implement this only if you can mux every pin individually as GPIO. The * affected GPIO range is passed along with an offset(pin number) into that @@ -68,8 +67,8 @@ struct pinmux_ops { unsigned selector, const char * const **groups, unsigned * const num_groups); - int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector, - unsigned group_selector); + int (*set_mux) (struct pinctrl_dev *pctldev, unsigned func_selector, + unsigned group_selector); int (*gpio_request_enable) (struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned offset); -- cgit v1.2.3 From 0d37899363b0e5486f8800231b7edd75e8b60942 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Wed, 3 Sep 2014 20:01:55 +0200 Subject: pinctrl: generic: Fix PIN_CONFIG_DRIVE_OPEN_SOURCE source/drain doc mismatch PIN_CONFIG_DRIVE_OPEN_SOURCE enables open source, not open drain. Signed-off-by: Geert Uytterhoeven Signed-off-by: Linus Walleij --- include/linux/pinctrl/pinconf-generic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/linux/pinctrl') diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h index a15f10727eb8..d578a60eff23 100644 --- a/include/linux/pinctrl/pinconf-generic.h +++ b/include/linux/pinctrl/pinconf-generic.h @@ -57,7 +57,7 @@ * which are then pulled up with an external resistor. Setting this * config will enable open drain mode, the argument is ignored. * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open source - * (open emitter). Setting this config will enable open drain mode, the + * (open emitter). Setting this config will enable open source mode, the * argument is ignored. * @PIN_CONFIG_DRIVE_STRENGTH: the pin will sink or source at most the current * passed as argument. The argument is in mA. -- cgit v1.2.3