Messages in this thread | | | Date | Wed, 9 Jul 2014 17:21:37 -0700 | Subject | Re: [PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin | From | Bjorn Andersson <> |
| |
On Sun, Jun 8, 2014 at 6:37 PM, <fwu@marvell.com> wrote: > From: Fan Wu <fwu@marvell.com> [...] > 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
| |