linux-kernel - Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <153020606866.143105.1849265284764230975@swboyd.mtv.corp.google.com>
Date:   Thu, 28 Jun 2018 10:14:28 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org,
        Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

Quoting Linus Walleij (2018-06-28 07:25:46)
> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> <bjorn.andersson@...aro.org> wrote:
> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > >
> > > > We rely on devices to use pinmuxing configurations in DT to select the
> > > > GPIO function (function 0) if they're going to use the gpio in GPIO
> > > > mode. Let's simplify things for driver authors by implementing
> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > > > function when the gpio is use from gpiolib.
> > > >
> > > > Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> > >
> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> (...)
> > While both patch 2 and 3 are convenient ways to get around the annoyance
> > of having to specify a pinmux state both patches then ends up relying on
> > some default pinconf state; which I think is bad.

What default state are we relying on? The reset state of the pins? I'm
very confused by this statement. These last two patches are making sure
that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
mode. If this code is not in place, then we'll allow drivers to request
a GPIO but pinmuxing won't be called to actually mux that pin to GPIO
mode unless they have a DT conf for function = "gpio". That seems
entirely unexpected and easy to mess up.

> 
> Nothing stops you from setting up a default conf in
> this callback though.
> 
> But admittedly this call was added for simpler hardware.
> 
> > Further more in situations like i2c-qup (downstream), where the pins are
> > requested as gpios in order to "bitbang" a reset this would mean that
> > the driver has to counter the convenience; by either switching in the
> > default pinmux at the end of probe or postponing the gpio_request() to
> > the invocation of reset and then, after issuing the gpio_release,
> > switching in the default pinmux explicitly again.
> 
> That's a bigger problem. If the system is using device and GPIO
> mode orthogonally, it'd be good to leave like this.
> 

Doesn't that driver need to explicitly mux GPIO mode vs. device mode
right now? So having gpio_request() do the muxing to GPIO mode and then
explicit muxing of the pin to device mode would be what we have after
this patch, while before this patch we would have mux to GPIO, request
GPIO (nop), mux to device. We saved an explicit pinmux call?