Re: [PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin

2014-07-09 Thread Bjorn Andersson 
On Sun, Jun 8, 2014 at 6:37 PM,   wrote:
> From: Fan Wu 
[...]
> diff --git a/drivers/pinctrl/pinctrl-msm.c b/drivers/pinctrl/pinctrl-msm.c
> index df6dda4c..bdfaba4 100644
> --- a/drivers/pinctrl/pinctrl-msm.c
> +++ b/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,
>  };
>

Sorry for being so late to the game. Totally missed when this until Linus today
told me that 'disable' is gone.

Given the following dt snippet:

active: active {
foo {
pins = "pin1";
function = "function";
};
};

sleep: sleep {
foo {
pins = "pin1";
};
};

Calling:
pinctrl_select_state(&active);
pinctrl_select_state(&sleep);

If I understand the code correctly after your change there will be no pinmux
operations performed during this transition. As you can see from the comment
above the Qualcomm driver relies on this fact to disable muxing; i.e. select
gpio function.

So most likely I have misunderstood what the disable function was supposed to
do, and we would need to add an explicit "gpio" function to all pingroups as
well as explicitly specifying that for every gpio-configured pin in the device
tree.

I.e. the lack of the string 'function = "gpio"' in below example makes the
state incomplete:

foo {
bar {
pins = "pin1";
bias-pull-up;
};
};

Is this assumption correct?

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

[PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin

2014-06-08 Thread fwu 
From: Fan Wu 

What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of
  calling pinctrl_select_state
2.Remove the HW disable operation in 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 to do this is that:
1.To avoid duplicated calling enable_setting operation without disabling
  operation which will let Pin's desc->mux_usecount keep being added.
2.The HW pin disable operation is not useful for most of the vendors' platform.
  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 need to switch Pin state dynamicly, E.g. b/t "sleep" and
  "default" state
2)The Pin setting configuration in DTS node may be 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 = ;
  }
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 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, 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 be kept added
  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 "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", actually
  two setting instance, 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 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".

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 
---
 drivers/pinctrl/core.c|   24 +++---
 drivers/pinctrl/pinctrl-abx500.c  |   15 -
 drivers/pinctrl/pinctrl-adi2.c|   30 -
 drivers/pinctrl/pinctrl-at91.c|   21 
 drivers/pinctrl/pinctrl-bcm2835.c |   11 ---
 drivers/pinctrl/pinctrl-exynos5440.c  |8 -
 drivers/pinctrl/pinctrl-msm.c |   25 --
 drivers/pinctrl/pinctrl-nomadik.c |   16 -
 drivers/pinctrl/pinctrl-rockchip.c|   18 --
 drivers/pinctrl/pinctrl-samsung.c |8 -
 drivers/pinctrl/pinctrl-single.c  |   56 ---
 drivers/pinctrl/pinctrl-st.c  |6 
 drivers/pinctrl/pinctrl-tb10x.c   |   17 --
 drivers/pinctrl/pinctrl-tegra.c   |   13 
 drivers/pinctrl/pinctrl-tz1090-pdc.c  |   28 
 drivers/pinctrl/pinctrl-tz1090.c  |   58 -
 drivers/pinctrl/pinctrl-u300.c|   14 
 drivers/pinctrl/pinmux.c  |4 ---
 drivers/pinctrl/sh-pfc/pinctrl.c  |   22 -
 drivers/pinctrl/sirf/pinctrl-sirf.c   |   10 --
 drivers/pinctrl/spear/pinctrl-spear.c |7 
 drivers/pinctrl/vt8500/pinctrl-wmt.c  |   12 ---
 include/linux/pinctrl/pinmux.h|2 --
 23 files changed, 5 insertions(+), 420 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/cor