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 --- drivers/pinctrl/pinctrl-st.c | 6 ------ 1 file changed, 6 deletions(-) (limited to 'drivers/pinctrl/pinctrl-st.c') diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c index 1bd6363bc95e..e1919cd43117 100644 --- a/drivers/pinctrl/pinctrl-st.c +++ b/drivers/pinctrl/pinctrl-st.c @@ -930,11 +930,6 @@ static int st_pmx_enable(struct pinctrl_dev *pctldev, unsigned fselector, return 0; } -static void st_pmx_disable(struct pinctrl_dev *pctldev, unsigned selector, - unsigned group) -{ -} - static int st_pmx_set_gpio_direction(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned gpio, bool input) @@ -957,7 +952,6 @@ static struct pinmux_ops st_pmxops = { .get_function_name = st_pmx_get_fname, .get_function_groups = st_pmx_get_groups, .enable = st_pmx_enable, - .disable = st_pmx_disable, .gpio_set_direction = st_pmx_set_gpio_direction, }; -- cgit v1.2.3 From 8708ebca7464af80accd52c6e1a1cf882593a4ab Mon Sep 17 00:00:00 2001 From: David PARIS Date: Wed, 25 Jun 2014 17:49:04 +0200 Subject: pinctrl: st: add IRQCHIP_SKIP_SET_WAKE flag no .irq_set_wake API is available for pinctrl-st driver. Add the IRQCHIP_SKIP_SET_WAKE flag to inform irq handler not to call this API. Signed-off-by: David Paris Signed-off-by: Linus Walleij --- drivers/pinctrl/pinctrl-st.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/pinctrl/pinctrl-st.c') diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c index e1919cd43117..506561131d7f 100644 --- a/drivers/pinctrl/pinctrl-st.c +++ b/drivers/pinctrl/pinctrl-st.c @@ -1448,6 +1448,7 @@ static struct irq_chip st_gpio_irqchip = { .irq_mask = st_gpio_irq_mask, .irq_unmask = st_gpio_irq_unmask, .irq_set_type = st_gpio_irq_set_type, + .flags = IRQCHIP_SKIP_SET_WAKE, }; static int st_gpiolib_register_bank(struct st_pinctrl *info, -- cgit v1.2.3 From 8b0c107ce049ddaf0ca9d8cd4faac1cf5c1e9f20 Mon Sep 17 00:00:00 2001 From: Rickard Strandqvist Date: Thu, 26 Jun 2014 13:32:49 +0200 Subject: pinctrl: pinctrl-st.c: Cleaning up if unsigned is less than zero Remove checking if a unsigned is less than zero This was found using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist Acked-by: Srinivas Kandagatla Acked-by: Maxime Coquelin Signed-off-by: Linus Walleij --- drivers/pinctrl/pinctrl-st.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/pinctrl/pinctrl-st.c') diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c index 506561131d7f..1845870413ea 100644 --- a/drivers/pinctrl/pinctrl-st.c +++ b/drivers/pinctrl/pinctrl-st.c @@ -1250,7 +1250,7 @@ static int st_pctl_parse_functions(struct device_node *np, func = &info->functions[index]; func->name = np->name; func->ngroups = of_get_child_count(np); - if (func->ngroups <= 0) { + if (func->ngroups == 0) { dev_err(info->dev, "No groups defined\n"); return -EINVAL; } -- cgit v1.2.3 From 1f978217a0c687a4cfed6ad698a3173826be4c3f Mon Sep 17 00:00:00 2001 From: Rickard Strandqvist Date: Thu, 26 Jun 2014 15:44:32 +0200 Subject: pinctrl: pinctrl-st.c: Cleaning up values that are never used Remove variable that are never used This was found using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist Acked-by: Patrice Chotard Signed-off-by: Linus Walleij --- drivers/pinctrl/pinctrl-st.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'drivers/pinctrl/pinctrl-st.c') diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c index 1845870413ea..d0fb44a095e9 100644 --- a/drivers/pinctrl/pinctrl-st.c +++ b/drivers/pinctrl/pinctrl-st.c @@ -1172,9 +1172,7 @@ static int st_pctl_dt_parse_groups(struct device_node *np, const __be32 *list; struct property *pp; struct st_pinconf *conf; - phandle phandle; struct device_node *pins; - u32 pin; int i = 0, npins = 0, nr_props; pins = of_get_child_by_name(np, "st,pins"); @@ -1212,8 +1210,8 @@ static int st_pctl_dt_parse_groups(struct device_node *np, conf = &grp->pin_conf[i]; /* bank & offset */ - phandle = be32_to_cpup(list++); - pin = be32_to_cpup(list++); + be32_to_cpup(list++); + be32_to_cpup(list++); conf->pin = of_get_named_gpio(pins, pp->name, 0); conf->name = pp->name; grp->pins[i] = conf->pin; -- cgit v1.2.3