Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Doug Anderson (2018-06-28 11:45:30)
> Hi,
> 
> On Thu, Jun 28, 2018 at 10:14 AM, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > Quoting Linus Walleij (2018-06-28 07:25:46)
> >> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> >> <bjorn.andersson@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> > >
> >> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >> (...)
> >> > 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?
> 
> After reading these replies I'm slightly worried about some of the
> stuff Bjorn is worried about too.  In Bjorn's example I think that the
> "default" state of the pins is probably i2c-mode and that it might
> switch to GPIO mode only when it needs to do the special unwedge of
> the i2c port.
> 
> So maybe the i2c driver does this:
> 
> 1. "Default" pinmux state of i2c is i2c-mode, "unwedge" pinmux state
> is gpio-mode.
> 
> 2. In probe, request the GPIOs for use later in case we need to
> unwedge.  Don't use them right now since we don't need to unwedge.
> Assumes pinmux state stays as "default".
> 
> 3. When you decide you need to unwedge the bus, pinmux to the special
> i2c mode and drive the pins you requested in probe.  Then pinmux back.
> 
> 
> ...I think your patch will break this because when you request the
> GPIOs you'll have an implicit (and unexpected?) pinmux transition.
> 
> What do you think?
> 
> 

I could do with some more clarity from Linus in the "Drivers needing
both pin control and GPIOs" section of
Documentation/driver-api/pinctl.rst but I read that section as stating
that the GPIO driver needs to mux the pin as a GPIO by requesting the
pinctrl backend to do so, unless the hardware overrides the muxed
function selection when the GPIO is used, without involving pinctrl
software.