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

[Bjorn Andersson]

On Sun, Jun 8, 2014 at 6:37 PM,  <fwu@xxxxxxxxxxx> wrote:
> From: Fan Wu <fwu@xxxxxxxxxxx>
[...]
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/