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 = <GPIO48 AF6>;
  }

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 MIPS#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 <[email protected]>
Acked-by: Stephen Warren <[email protected]>
Acked-by: Patrice Chotard <[email protected]>
Acked-by: Heiko Stuebner <[email protected]>
Acked-by: Maxime Coquelin <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>

drivers/pinctrl/core.c

@@ -992,29 +992,15 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 
 	if (p->state) {
 		/*
-		 * The set of groups with a mux configuration in the old state
-		 * may not be identical to the set of groups with a mux setting
-		 * in the new state. While this might be unusual, it's entirely
-		 * possible for the "user"-supplied mapping table to be written
-		 * that way. For each group that was configured in the old state
-		 * but not in the new state, this code puts that group into a
-		 * safe/disabled state.
+		 * For each pinmux setting in the old state, forget SW's record
+		 * of mux owner for that pingroup. Any pingroups which are
+		 * still owned by the new state will be re-acquired by the call
+		 * to pinmux_enable_setting() in the loop below.
 		 */
 		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);
 		}
 	}
 

drivers/pinctrl/pinctrl-abx500.c

@@ -737,20 +737,6 @@ static int abx500_pmx_enable(struct pinctrl_dev *pctldev, unsigned function,
 	return ret;
 }
 
-static void abx500_pmx_disable(struct pinctrl_dev *pctldev,
-			       unsigned function, unsigned group)
-{
-	struct abx500_pinctrl *pct = pinctrl_dev_get_drvdata(pctldev);
-	const struct abx500_pingroup *g;
-
-	g = &pct->soc->groups[group];
-	if (g->altsetting < 0)
-		return;
-
-	/* FIXME: poke out the mux, set the pin to some default state? */
-	dev_dbg(pct->dev, "disable group %s, %u pins\n", g->name, g->npins);
-}
-
 static int abx500_gpio_request_enable(struct pinctrl_dev *pctldev,
 			       struct pinctrl_gpio_range *range,
 			       unsigned offset)
@@ -799,7 +785,6 @@ static const struct pinmux_ops abx500_pinmux_ops = {
 	.get_function_name = abx500_pmx_get_func_name,
 	.get_function_groups = abx500_pmx_get_func_groups,
 	.enable = abx500_pmx_enable,
-	.disable = abx500_pmx_disable,
 	.gpio_request_enable = abx500_gpio_request_enable,
 	.gpio_disable_free = abx500_gpio_disable_free,
 };

drivers/pinctrl/pinctrl-adi2.c

@@ -652,35 +652,6 @@ static int adi_pinmux_enable(struct pinctrl_dev *pctldev, unsigned func_id,
 	return 0;
 }
 
-static void adi_pinmux_disable(struct pinctrl_dev *pctldev, unsigned func_id,
-	unsigned group_id)
-{
-	struct adi_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
-	struct gpio_port *port;
-	struct pinctrl_gpio_range *range;
-	unsigned long flags;
-	unsigned short *mux, pin;
-
-	mux = (unsigned short *)pinctrl->soc->groups[group_id].mux;
-
-	while (*mux) {
-		pin = P_IDENT(*mux);
-
-		range = pinctrl_find_gpio_range_from_pin(pctldev, pin);
-		if (range == NULL) /* should not happen */
-			return;
-
-		port = container_of(range->gc, struct gpio_port, chip);
-
-		spin_lock_irqsave(&port->lock, flags);
-
-		port_setup(port, pin_to_offset(range, pin), true);
-		mux++;
-
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
-}
-
 static int adi_pinmux_get_funcs_count(struct pinctrl_dev *pctldev)
 {
 	struct adi_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -728,7 +699,6 @@ static int adi_pinmux_request_gpio(struct pinctrl_dev *pctldev,
 
 static struct pinmux_ops adi_pinmux_ops = {
 	.enable = adi_pinmux_enable,
-	.disable = adi_pinmux_disable,
 	.get_functions_count = adi_pinmux_get_funcs_count,
 	.get_function_name = adi_pinmux_get_func_name,
 	.get_function_groups = adi_pinmux_get_groups,

drivers/pinctrl/pinctrl-at91.c

@@ -611,26 +611,6 @@ static int at91_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
-static void at91_pmx_disable(struct pinctrl_dev *pctldev, unsigned selector,
-			   unsigned group)
-{
-	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
-	const struct at91_pmx_pin *pins_conf = info->groups[group].pins_conf;
-	const struct at91_pmx_pin *pin;
-	uint32_t npins = info->groups[group].npins;
-	int i;
-	unsigned mask;
-	void __iomem *pio;
-
-	for (i = 0; i < npins; i++) {
-		pin = &pins_conf[i];
-		at91_pin_dbg(info->dev, pin);
-		pio = pin_to_controller(info, pin->bank);
-		mask = pin_to_mask(pin->pin);
-		at91_mux_gpio_enable(pio, mask, 1);
-	}
-}
-
 static int at91_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
 {
 	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
@@ -705,7 +685,6 @@ static const struct pinmux_ops at91_pmx_ops = {
 	.get_function_name	= at91_pmx_get_func_name,
 	.get_function_groups	= at91_pmx_get_groups,
 	.enable			= at91_pmx_enable,
-	.disable		= at91_pmx_disable,
 	.gpio_request_enable	= at91_gpio_request_enable,
 	.gpio_disable_free	= at91_gpio_disable_free,
 };

drivers/pinctrl/pinctrl-bcm2835.c

@@ -841,16 +841,6 @@ static int bcm2835_pmx_enable(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static void bcm2835_pmx_disable(struct pinctrl_dev *pctldev,
-		unsigned func_selector,
-		unsigned group_selector)
-{
-	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
-
-	/* disable by setting to GPIO_IN */
-	bcm2835_pinctrl_fsel_set(pc, group_selector, BCM2835_FSEL_GPIO_IN);
-}
-
 static void bcm2835_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
 		struct pinctrl_gpio_range *range,
 		unsigned offset)
@@ -880,7 +870,6 @@ static const struct pinmux_ops bcm2835_pmx_ops = {
 	.get_function_name = bcm2835_pmx_get_function_name,
 	.get_function_groups = bcm2835_pmx_get_function_groups,
 	.enable = bcm2835_pmx_enable,
-	.disable = bcm2835_pmx_disable,
 	.gpio_disable_free = bcm2835_pmx_gpio_disable_free,
 	.gpio_set_direction = bcm2835_pmx_gpio_set_direction,
 };

drivers/pinctrl/pinctrl-exynos5440.c

@@ -371,13 +371,6 @@ static int exynos5440_pinmux_enable(struct pinctrl_dev *pctldev, unsigned select
 	return 0;
 }
 
-/* disable a specified pinmux by writing to registers */
-static void exynos5440_pinmux_disable(struct pinctrl_dev *pctldev,
-					unsigned selector, unsigned group)
-{
-	exynos5440_pinmux_setup(pctldev, selector, group, false);
-}
-
 /*
  * The calls to gpio_direction_output() and gpio_direction_input()
  * leads to this function call (via the pinctrl_gpio_direction_{input|output}()
@@ -395,7 +388,6 @@ static const struct pinmux_ops exynos5440_pinmux_ops = {
 	.get_function_name	= exynos5440_pinmux_get_fname,
 	.get_function_groups	= exynos5440_pinmux_get_groups,
 	.enable			= exynos5440_pinmux_enable,
-	.disable		= exynos5440_pinmux_disable,
 	.gpio_set_direction	= exynos5440_pinmux_gpio_set_direction,
 };
 

drivers/pinctrl/pinctrl-msm.c

@@ -165,36 +165,11 @@ static int msm_pinmux_enable(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static void msm_pinmux_disable(struct pinctrl_dev *pctldev,
-			       unsigned function,
-			       unsigned group)
-{
-	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	const struct msm_pingroup *g;
-	unsigned long flags;
-	u32 val;
-
-	g = &pctrl->soc->groups[group];
-
-	if (WARN_ON(g->mux_bit < 0))
-		return;
-
-	spin_lock_irqsave(&pctrl->lock, flags);
-
-	/* Clear the mux bits to select gpio mode */
-	val = readl(pctrl->regs + g->ctl_reg);
-	val &= ~(0x7 << g->mux_bit);
-	writel(val, pctrl->regs + g->ctl_reg);
-
-	spin_unlock_irqrestore(&pctrl->lock, flags);
-}
-
 static const struct pinmux_ops msm_pinmux_ops = {
 	.get_functions_count	= msm_get_functions_count,
 	.get_function_name	= msm_get_function_name,
 	.get_function_groups	= msm_get_function_groups,
 	.enable			= msm_pinmux_enable,
-	.disable		= msm_pinmux_disable,
 };
 
 static int msm_config_reg(struct msm_pinctrl *pctrl,

drivers/pinctrl/pinctrl-nomadik.c

@@ -1765,21 +1765,6 @@ static int nmk_pmx_enable(struct pinctrl_dev *pctldev, unsigned function,
 	return ret;
 }
 
-static void nmk_pmx_disable(struct pinctrl_dev *pctldev,
-			    unsigned function, unsigned group)
-{
-	struct nmk_pinctrl *npct = pinctrl_dev_get_drvdata(pctldev);
-	const struct nmk_pingroup *g;
-
-	g = &npct->soc->groups[group];
-
-	if (g->altsetting < 0)
-		return;
-
-	/* Poke out the mux, set the pin to some default state? */
-	dev_dbg(npct->dev, "disable group %s, %u pins\n", g->name, g->npins);
-}
-
 static int nmk_gpio_request_enable(struct pinctrl_dev *pctldev,
 				   struct pinctrl_gpio_range *range,
 				   unsigned offset)
@@ -1826,7 +1811,6 @@ static const struct pinmux_ops nmk_pinmux_ops = {
 	.get_function_name = nmk_pmx_get_func_name,
 	.get_function_groups = nmk_pmx_get_func_groups,
 	.enable = nmk_pmx_enable,
-	.disable = nmk_pmx_disable,
 	.gpio_request_enable = nmk_gpio_request_enable,
 	.gpio_disable_free = nmk_gpio_disable_free,
 };

drivers/pinctrl/pinctrl-rockchip.c

@@ -657,23 +657,6 @@ static int rockchip_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
-static void rockchip_pmx_disable(struct pinctrl_dev *pctldev,
-					unsigned selector, unsigned group)
-{
-	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
-	const unsigned int *pins = info->groups[group].pins;
-	struct rockchip_pin_bank *bank;
-	int cnt;
-
-	dev_dbg(info->dev, "disable function %s group %s\n",
-		info->functions[selector].name, info->groups[group].name);
-
-	for (cnt = 0; cnt < info->groups[group].npins; cnt++) {
-		bank = pin_to_bank(info, pins[cnt]);
-		rockchip_set_mux(bank, pins[cnt] - bank->pin_base, 0);
-	}
-}
-
 /*
  * The calls to gpio_direction_output() and gpio_direction_input()
  * leads to this function call (via the pinctrl_gpio_direction_{input|output}()
@@ -716,7 +699,6 @@ static const struct pinmux_ops rockchip_pmx_ops = {
 	.get_function_name	= rockchip_pmx_get_func_name,
 	.get_function_groups	= rockchip_pmx_get_groups,
 	.enable			= rockchip_pmx_enable,
-	.disable		= rockchip_pmx_disable,
 	.gpio_set_direction	= rockchip_pmx_gpio_set_direction,
 };
 

drivers/pinctrl/pinctrl-samsung.c

@@ -333,13 +333,6 @@ static int samsung_pinmux_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
-/* disable a specified pinmux by writing to registers */
-static void samsung_pinmux_disable(struct pinctrl_dev *pctldev,
-					unsigned selector, unsigned group)
-{
-	samsung_pinmux_setup(pctldev, selector, group, false);
-}
-
 /*
  * The calls to gpio_direction_output() and gpio_direction_input()
  * leads to this function call (via the pinctrl_gpio_direction_{input|output}()
@@ -390,7 +383,6 @@ static const struct pinmux_ops samsung_pinmux_ops = {
 	.get_function_name	= samsung_pinmux_get_fname,
 	.get_function_groups	= samsung_pinmux_get_groups,
 	.enable			= samsung_pinmux_enable,
-	.disable		= samsung_pinmux_disable,
 	.gpio_set_direction	= samsung_pinmux_gpio_set_direction,
 };
 

drivers/pinctrl/pinctrl-single.c

@@ -488,61 +488,6 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
 	return 0;
 }
 
-static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
-					unsigned group)
-{
-	struct pcs_device *pcs;
-	struct pcs_function *func;
-	int i;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-	/* If function mask is null, needn't disable it. */
-	if (!pcs->fmask)
-		return;
-
-	func = radix_tree_lookup(&pcs->ftree, fselector);
-	if (!func) {
-		dev_err(pcs->dev, "%s could not find function%i\n",
-			__func__, fselector);
-		return;
-	}
-
-	/*
-	 * Ignore disable if function-off is not specified. Some hardware
-	 * does not have clearly defined disable function. For pin specific
-	 * off modes, you can use alternate named states as described in
-	 * pinctrl-bindings.txt.
-	 */
-	if (pcs->foff == PCS_OFF_DISABLED) {
-		dev_dbg(pcs->dev, "ignoring disable for %s function%i\n",
-			func->name, fselector);
-		return;
-	}
-
-	dev_dbg(pcs->dev, "disabling function%i %s\n",
-		fselector, func->name);
-
-	for (i = 0; i < func->nvals; i++) {
-		struct pcs_func_vals *vals;
-		unsigned long flags;
-		unsigned val, mask;
-
-		vals = &func->vals[i];
-		raw_spin_lock_irqsave(&pcs->lock, flags);
-		val = pcs->read(vals->reg);
-
-		if (pcs->bits_per_mux)
-			mask = vals->mask;
-		else
-			mask = pcs->fmask;
-
-		val &= ~mask;
-		val |= pcs->foff << pcs->fshift;
-		pcs->write(val, vals->reg);
-		raw_spin_unlock_irqrestore(&pcs->lock, flags);
-	}
-}
-
 static int pcs_request_gpio(struct pinctrl_dev *pctldev,
 			    struct pinctrl_gpio_range *range, unsigned pin)
 {
@@ -575,7 +520,6 @@ static const struct pinmux_ops pcs_pinmux_ops = {
 	.get_function_name = pcs_get_function_name,
 	.get_function_groups = pcs_get_function_groups,
 	.enable = pcs_enable,
-	.disable = pcs_disable,
 	.gpio_request_enable = pcs_request_gpio,
 };