[PATCH] gpio/regulator: Allow nonexclusive GPIO access

[PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Linus Walleij]

This allows nonexclusive (simultaneous) access to a single
GPIO line for the fixed regulator enable line. This happens
when several regulators use the same GPIO for enabling and
disabling a regulator, and all need a handle on their GPIO
descriptor.

This solution with a special flag is not entirely elegant
and should ideally be replaced by something more careful as
this makes it possible for several consumers to
enable/disable the same GPIO line to the left and right
without any consistency. The current use inside the regulator
core should however be fine as it takes special care to
handle this.

For the state of the GPIO backend, this is still the
lesser evil compared to going back to global GPIO
numbers.

Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Hi Marek, it would be great if you could try this on top
of linux-next and report if it solves your problem on the
Samsung Exynos.

If it does I hope it will apply cleanly on the regulator
tree.
---
 drivers/gpio/gpiolib.c        | 19 +++++++++++++++++--
 drivers/regulator/fixed.c     | 13 +++++++++++++
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7c222df8f834..f82a741ff428 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	 * the device name as label
 	 */
 	status = gpiod_request(desc, con_id ? con_id : devname);
-	if (status < 0)
-		return ERR_PTR(status);
+	if (status < 0) {
+		if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
+			/*
+			 * This happens when there are several consumers for
+			 * the same GPIO line: we just return here without
+			 * further initialization. It is a bit if a hack.
+			 * This is necessary to support fixed regulators.
+			 *
+			 * FIXME: Make this more sane and safe.
+			 */
+			dev_info(dev, "nonexclusive access to GPIO for %s\n",
+				 con_id);
+			return desc;
+		} else {
+			return ERR_PTR(status);
+		}
+	}
 
 	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
 	if (status < 0) {
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 7d639ad953b6..ccc29038f19a 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -170,6 +170,19 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 			gflags = GPIOD_OUT_LOW_OPEN_DRAIN;
 	}
 
+	/*
+	 * 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;
+
 	cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, gflags);
 	if (IS_ERR(cfg.ena_gpiod))
 		return PTR_ERR(cfg.ena_gpiod);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 0f350616d372..f2f887795d43 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -39,6 +39,7 @@ struct gpio_descs {
 #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
 #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
 #define GPIOD_FLAGS_BIT_OPEN_DRAIN	BIT(3)
+#define GPIOD_FLAGS_BIT_NONEXCLUSIVE	BIT(4)
 
 /**
  * Optional flags that can be passed to one of gpiod_* to configure direction
-- 
2.17.1

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

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.  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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Linus Walleij]

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

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Thu, Oct 11, 2018 at 07:01:13PM +0200, Linus Walleij wrote:
> 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:

> > 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.

Other regulators that have GPIO control for their enables do their own
requests (as does the core) so don't they all need to set the flag
GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work
in systems where the regulators after the first that request the GPIO
are fixed regulators?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Linus Walleij]

On Thu, Oct 11, 2018 at 7:54 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, Oct 11, 2018 at 07:01:13PM +0200, Linus Walleij wrote:
> > 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:
>
> > > 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.
>
> Other regulators that have GPIO control for their enables do their own
> requests (as does the core) so don't they all need to set the flag
> GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work
> in systems where the regulators after the first that request the GPIO
> are fixed regulators?

Yeah ... I guess I assumed that the same enable line is not used
for a fixed regulator and then some other regulator (which is not
fixed).

If this is common practice we need to account for it. Obviously
I have no idea what is common practice :/

gpio-regulator.c will work fine since it passes in a global
number (still) so it will just grab the desc out of the GPIO
with the legacy function and go on. (I need to keep it in mind
when refactoring that one later though!)

Do we know some way to identify the affected systems or
should we just tag on GPIOD_FLAGS_BIT_NONEXCLUSIVE
on all the stuff we converted to descs to play it safe?
(I can cook a patch.)

Yours,
Linus Walleij

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Thu, Oct 11, 2018 at 08:41:16PM +0200, Linus Walleij wrote:
> On Thu, Oct 11, 2018 at 7:54 PM Mark Brown <broonie@kernel.org> wrote:

> > Other regulators that have GPIO control for their enables do their own
> > requests (as does the core) so don't they all need to set the flag
> > GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work
> > in systems where the regulators after the first that request the GPIO
> > are fixed regulators?

> Yeah ... I guess I assumed that the same enable line is not used
> for a fixed regulator and then some other regulator (which is not
> fixed).

> If this is common practice we need to account for it. Obviously
> I have no idea what is common practice :/

> Do we know some way to identify the affected systems or
> should we just tag on GPIOD_FLAGS_BIT_NONEXCLUSIVE
> on all the stuff we converted to descs to play it safe?
> (I can cook a patch.)

I think we need to set the flag everywhere, previously we'd essentially
been doing it by asking the GPIO framework if we resolved the same
regulator as one we already had.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Marek Szyprowski]

Hi Linus,

On 2018-10-11 16:35, Linus Walleij wrote:
> This allows nonexclusive (simultaneous) access to a single
> GPIO line for the fixed regulator enable line. This happens
> when several regulators use the same GPIO for enabling and
> disabling a regulator, and all need a handle on their GPIO
> descriptor.
>
> This solution with a special flag is not entirely elegant
> and should ideally be replaced by something more careful as
> this makes it possible for several consumers to
> enable/disable the same GPIO line to the left and right
> without any consistency. The current use inside the regulator
> core should however be fine as it takes special care to
> handle this.
>
> For the state of the GPIO backend, this is still the
> lesser evil compared to going back to global GPIO
> numbers.
>
> Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

It looks to be fixing the Sii9234 probe issue, but I didn't do any
further tests. I must go out the office and I will be back on Monday.
I think You can add my:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> Hi Marek, it would be great if you could try this on top
> of linux-next and report if it solves your problem on the
> Samsung Exynos.
>
> If it does I hope it will apply cleanly on the regulator
> tree.
> ---
>  drivers/gpio/gpiolib.c        | 19 +++++++++++++++++--
>  drivers/regulator/fixed.c     | 13 +++++++++++++
>  include/linux/gpio/consumer.h |  1 +
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7c222df8f834..f82a741ff428 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>  	 * the device name as label
>  	 */
>  	status = gpiod_request(desc, con_id ? con_id : devname);
> -	if (status < 0)
> -		return ERR_PTR(status);
> +	if (status < 0) {
> +		if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
> +			/*
> +			 * This happens when there are several consumers for
> +			 * the same GPIO line: we just return here without
> +			 * further initialization. It is a bit if a hack.
> +			 * This is necessary to support fixed regulators.
> +			 *
> +			 * FIXME: Make this more sane and safe.
> +			 */
> +			dev_info(dev, "nonexclusive access to GPIO for %s\n",
> +				 con_id);
> +			return desc;
> +		} else {
> +			return ERR_PTR(status);
> +		}
> +	}
>  
>  	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
>  	if (status < 0) {
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 7d639ad953b6..ccc29038f19a 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -170,6 +170,19 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
>  			gflags = GPIOD_OUT_LOW_OPEN_DRAIN;
>  	}
>  
> +	/*
> +	 * 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;
> +
>  	cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, gflags);
>  	if (IS_ERR(cfg.ena_gpiod))
>  		return PTR_ERR(cfg.ena_gpiod);
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 0f350616d372..f2f887795d43 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -39,6 +39,7 @@ struct gpio_descs {
>  #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
>  #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
>  #define GPIOD_FLAGS_BIT_OPEN_DRAIN	BIT(3)
> +#define GPIOD_FLAGS_BIT_NONEXCLUSIVE	BIT(4)
>  
>  /**
>   * Optional flags that can be passed to one of gpiod_* to configure direction

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access

[Jon Hunter]

Hi Linus,

On 11/10/18 15:35, Linus Walleij wrote:
> This allows nonexclusive (simultaneous) access to a single
> GPIO line for the fixed regulator enable line. This happens
> when several regulators use the same GPIO for enabling and
> disabling a regulator, and all need a handle on their GPIO
> descriptor.
> 
> This solution with a special flag is not entirely elegant
> and should ideally be replaced by something more careful as
> this makes it possible for several consumers to
> enable/disable the same GPIO line to the left and right
> without any consistency. The current use inside the regulator
> core should however be fine as it takes special care to
> handle this.
> 
> For the state of the GPIO backend, this is still the
> lesser evil compared to going back to global GPIO
> numbers.
> 
> Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

This also fixes a regression I was seeing on Tegra124 Jetson TK1. So ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

> ---
> Hi Marek, it would be great if you could try this on top
> of linux-next and report if it solves your problem on the
> Samsung Exynos.
> 
> If it does I hope it will apply cleanly on the regulator
> tree.
> ---
>  drivers/gpio/gpiolib.c        | 19 +++++++++++++++++--
>  drivers/regulator/fixed.c     | 13 +++++++++++++
>  include/linux/gpio/consumer.h |  1 +
>  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7c222df8f834..f82a741ff428 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>  	 * the device name as label
>  	 */
>  	status = gpiod_request(desc, con_id ? con_id : devname);
> -	if (status < 0)
> -		return ERR_PTR(status);
> +	if (status < 0) {
> +		if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
> +			/*
> +			 * This happens when there are several consumers for
> +			 * the same GPIO line: we just return here without
> +			 * further initialization. It is a bit if a hack.
> +			 * This is necessary to support fixed regulators.
> +			 *
> +			 * FIXME: Make this more sane and safe.
> +			 */
> +			dev_info(dev, "nonexclusive access to GPIO for %s\n",
> +				 con_id);

Nit-pick, for me the 'con_id' is NULL so should we use the same ternary
operator here as above to print devname instead in this case?

Cheers!
Jon

-- 
nvpublic