linux-kernel - Re: [PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJAp7OjhjkRZvAb8DvURCasQr_=yCvW7npWnNyOzHZH5ndKdrg@mail.gmail.com>
Date:	Wed, 9 Jul 2014 17:21:37 -0700
From:	Bjorn Andersson <bjorn@...o.se>
To:	fwu@...vell.com
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Tony Lindgren <tony@...mide.com>, plagnioj@...osoft.com,
	Kukjin Kim <kgene.kim@...sung.com>, heiko@...ech.de,
	Tomasz Figa <t.figa@...sung.com>, thomas.abraham@...aro.org,
	srinivas.kandagatla@...il.com, maxime.coquelin@...com,
	patrice.chotard@...com, thierry.reding@...il.com,
	baohua@...nel.org, viresh.linux@...il.com,
	Matt Porter <matt.porter@...aro.org>, syin@...adcom.com,
	jg1.han@...sung.com, Rongjun.Ying@....com, Rong.Wang@....com,
	linux@...sktech.co.nz, yongjun_wei@...ndmicro.com.cn,
	laurent.navet@...il.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stephen Warren <swarren@...dia.com>, cxie4@...vell.com,
	ylmao@...vell.com, njiang1@...vell.com, tianxf@...vell.com,
	fswu@...vell.com
Subject: Re: [PATCH v6] pinctrl: to avoid duplicated calling
 enable_pinmux_setting for a pin

On Sun, Jun 8, 2014 at 6:37 PM,  <fwu@...vell.com> wrote:
> From: Fan Wu <fwu@...vell.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/