Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181011143531.7195-1-linus.walleij@linaro.org> <20181011144330.GF25351@sirena.org.uk>
In-Reply-To: <20181011144330.GF25351@sirena.org.uk>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Thu, 11 Oct 2018 19:01:13 +0200
Message-ID: <CACRpkdbTnBQhPQARX77KfjtfWXY+t0oH6-B=WUN4hid8eQT0DA@mail.gmail.com>
Subject: Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access
To:     Mark Brown <broonie@kernel.org>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 11, 2018 at 4:43 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, Oct 11, 2018 at 04:35:31PM +0200, Linus Walleij wrote:
>
> > +     /*
> > +      * Some fixed regulators share the enable line between two
> > +      * regulators which makes it necessary to get a handle on the
> > +      * same descriptor for two different consumers. This will get
> > +      * the GPIO descriptor, but only the first call will initialize
> > +      * it so any flags such as inversion or open drain will only
> > +      * be set up by the first caller and assumed identical on the
> > +      * next caller.
> > +      *
> > +      * FIXME: find a better way to deal with this.
> > +      */
> > +     gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
> > +
>
> It's not just fixed regulators that do this, often regulators with
> register control can do this as well.

Yeah I thought that was implicit since the comment is inside
fixed.c but I can change it, or you can edit when applying if
you like.

>  Since power up is often a
> performance critical path but regulators tend to be controlled via slow
> buses like I2C it's common to have register control combined with a GPIO
> enable line so you don't need to use the bus to do the enables.  That
> should just be a case of adding this flag to all the drivers that have
> already been converted to gpiod (including the core code that's in
> regulator_ena_gpio_request() which I thought was coping with this
> already) unless I'm missing something?

Sorry if I don't get it... we already have code in the core to
check if the same gpiod is used by two regulators.
regulator_ena_gpio_request() does this:

        if (config->ena_gpiod)
                gpiod = config->ena_gpiod;
        else
                gpiod = gpio_to_desc(config->ena_gpio);

So after the change I made to fixed.c this comes in through
config->ena_gpiod.

Later in this function:

        list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
                if (pin->gpiod == gpiod) {
                        rdev_dbg(rdev, "GPIO %d is already used\n",
                                config->ena_gpio);

So there is the reference counting code, where we identify
that the same descriptor is already in use.

The only change this patch really does is make it possible for
the same gpiod to come in twice from fixed.c (as is proper).

If it's a simple and single consumer enable/disable disable
line all is fine and it doesn't get reference counted etc, since
there is one and only one consumer for the GPIO.
gpiod_get[_optional]() is just fine in these cases.

When I straight out the FIXME:s I would add code to check
that the flags passed to gpiolib are coherent and maybe store
all users in an array instead of a simple char * inside the
gpio descriptor, so we properly handle more than one user
in gpiolib.

Possibly the reference counting should be moved
over from regulator core as well: this would apply if there
are more subsystems than regulator using the same
GPIO with multiple consumers.

But that is for the future fixups.

Yours,
Linus Walleij