Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220914151145.73253-1-brgl@bgdev.pl> <YyH/hJMyPZSqyvQj@sol>
 <CAMRc=McERgSkmpWv4k1eB1mtRU=jGhWiXYMdq72h9H9SYuF6Ng@mail.gmail.com> <YyIEbW2gx/FX7ele@sol>
In-Reply-To: <YyIEbW2gx/FX7ele@sol>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Thu, 15 Sep 2022 09:44:46 +0200
Message-ID: <CAMRc=MeyPsCzY-mySx7YizpaJ5Dtq4tnjs5ccpT_tm4ckWkuSw@mail.gmail.com>
Subject: Re: [PATCH] gpiolib: TODO: add an item about GPIO safe-state
To:     Kent Gibson <warthog618@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Drew Fustini <dfustini@baylibre.com>,
        Marek Vasut <marek.vasut@gmail.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> You call the driver to set the value after calling the free()?
>On Wed, Sep 14, 2022 at 6:42 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Sep 14, 2022 at 06:25:21PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Sep 14, 2022 at 6:21 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Wed, Sep 14, 2022 at 05:11:45PM +0200, Bartosz Golaszewski wrote:
> > > > This adds a new TODO item for gpiolib and can also be used to start
> > > > a discussion about the need for it and implementation details.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > > ---
> > > >  drivers/gpio/TODO | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > >
> > > > diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
> > > > index f87ff3fa8a53..6ab39c5cec9d 100644
> > > > --- a/drivers/gpio/TODO
> > > > +++ b/drivers/gpio/TODO
> > > > @@ -197,3 +197,25 @@ A small number of drivers have been converted (pl061, tegra186, msm,
> > > >  amd, apple), and can be used as examples of how to proceed with this
> > > >  conversion. Note that drivers using the generic irqchip framework
> > > >  cannot be converted yet, but watch this space!
> > > > +
> > > > +Safe-state of GPIOs
> > > > +
> > > > +During 2022 Linux Plumbers Conference's GPIO & pinctrl BOF it's been discussed
> > > > +that we don't have any middle ground between hogging GPIO lines and letting the
> > > > +user (either in-kernel or user-space) control them. Either the lines are forever
> > > > +reserved as hogs or their state is undefined unless requested.
> > > > +
> > > > +Currently the behavior of GPIOs that were not requested or were released is
> > > > +largely driver dependent (the provider driver decides whether the line's state
> > > > +is reverted to some predefined value or left as-is). This can be problematic
> > > > +as the output state of a line can damage physical hardware.
> > > > +
> > > > +This item is about proposing a solution, most likely in the form of a new device
> > > > +property called "safe-state" that would define the safe states of specific lines
> > > > +(e.g. output-high) but not block the line from being requested by users who
> > > > +could then modify that default state. Once released the GPIO core would then
> > > > +put the line back into the "safe-state".
> > > > +
> > >
> > > Geert suggests idle-state, rather than safe-state, but you call it
> > > the "default state" here as well - pick one.
> > >
> >
> > idle-state it is then.
> >
> > > So this idle-state would be another attribute on a line that the user
> > > could configure via the GPIO uAPI, and so replicate the "set and forget"
> > > sysfs behavior that we are currently missing, and which seems to be the
> > > biggest sticking point for a transition away from sysfs?
> > >
> >
> > No, this should only be defined on the device tree or in ACPI. As the
> > HW policy of a device. I don't think we should allow user-space to
> > override this behavior.
> >
>
> Oh, ok - from the item I got the impression you did want to be able to
> control it from user-space.
>

Oh god no, then it would be sysfs all over again.

> > > For backward compatibility the default idle-state, i.e. the value the
> > > idle-state would take if not explicitly set, would map to existing
> > > behaviour, so let the driver decide?
> > >
> > > What happens when gpiolib frees the line?  Isn't the driver still able
> > > to do what it likes to the line at that point, no matter what GPIO core
> > > has set it to previously? e.g. gpio_sim_free() restores the line to its
> > > own internal pull value.
> > >
> >
> > This "idle-state" property wouldn't be mandatory and normally would
> > only be defined for a limited set of lines. I'd say we just override
> > whatever the driver does in free() (most drivers don't implement it
> > BTW) and do what the property says we should.
> >
>
> Not sure what "override" involves.
> You call the driver to set the value after calling the free()?
>

Yes. We call the driver's free() callback and it does something in it
(potentially set some predefined state). We see that this line has the
idle-state property defined so we call into the driver and set the
value as defined by idle-state.

Bartosz