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

[PATCH v2] regulator/gpio: 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.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Jon Hunter <jonathanh@nvidia.com>
Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix the print string to use ternary operator and alternative
  text.
- Collect Tested-by's from affected systems.
- Mark: I tested to apply this on the regulator tree pulled
  in my for-next branches for GPIO and pin control on top and
  it seems to work! Could you apply it?
---
 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..56178af4ecd9 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 ? con_id : devname);
+			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.2

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

[jacopo mondi]

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

Hi Linus,
   + Laurent, as he reviewed most of that driver code

Sorry, I'm going slightly OT with this, but please read below.

On Fri, Oct 12, 2018 at 02:54:12PM +0200, 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.
>

I might have a use case for shared GPIO lines used as 'simple' reset
signal for camera devices and not for regulators.

See drivers/media/i2c/ov772x.c FIXME note in power_on() function at:
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832

The reset line is in this case is passed to the driver by board file:
https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350

As you can see PTT3 is used for both sensors (I know, questionable
HW design...)

Do you think extending gpiod_lookup_flags with this newly introduced
NONEXCLUSIVE one is an acceptable solution to avoid handling this in
the sensor driver like we're doing today?

Please note this is an ancient architecture that boots from board
files, but the same might happen in modern designs with OF support. Is
there any clean way to handle shared GPIOs I might not be aware of for
those systems?

Thanks
   j


> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Jon Hunter <jonathanh@nvidia.com>
> Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix the print string to use ternary operator and alternative
>   text.
> - Collect Tested-by's from affected systems.
> - Mark: I tested to apply this on the regulator tree pulled
>   in my for-next branches for GPIO and pin control on top and
>   it seems to work! Could you apply it?
> ---
>  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..56178af4ecd9 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 ? con_id : devname);
> +			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.2
>
>

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

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

[Mark Brown]

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

On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote:

> Sorry, I'm going slightly OT with this, but please read below.

> On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote:
> > This allows nonexclusive (simultaneous) access to a single
> > GPIO line for the fixed regulator enable line. This happens

> I might have a use case for shared GPIO lines used as 'simple' reset
> signal for camera devices and not for regulators.

This recently came up in ASoC too with audio CODECs sharing reset lines,
there was a discussion started with the reset API maintainer though no
response yet.  CCing in Cheng-yi who had that problem.  Not deleting
context for that.

> See drivers/media/i2c/ov772x.c FIXME note in power_on() function at:
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832
> 
> The reset line is in this case is passed to the driver by board file:
> https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350
> 
> As you can see PTT3 is used for both sensors (I know, questionable
> HW design...)
> 
> Do you think extending gpiod_lookup_flags with this newly introduced
> NONEXCLUSIVE one is an acceptable solution to avoid handling this in
> the sensor driver like we're doing today?

One problem you've got there is that you need the two devices to know
about each other so they coordinate their use of the reset line.  This
was relatively easy in the sysetm Cheng-yi has as it's just one driver
but what if there's mutiple drivers?  That's relatively likely with
audio where you might have something like a CODEC with a separate power
amplifier.  The regulator enable stuff is handling this in the core but
it's less clear where to put that for reset lines.

> Please note this is an ancient architecture that boots from board
> files, but the same might happen in modern designs with OF support. Is
> there any clean way to handle shared GPIOs I might not be aware of for
> those systems?

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

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

[jacopo mondi]

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

Hi Mark,

On Fri, Oct 12, 2018 at 06:44:24PM +0200, Mark Brown wrote:
> On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote:
>
> > Sorry, I'm going slightly OT with this, but please read below.
>
> > On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote:
> > > This allows nonexclusive (simultaneous) access to a single
> > > GPIO line for the fixed regulator enable line. This happens
>
> > I might have a use case for shared GPIO lines used as 'simple' reset
> > signal for camera devices and not for regulators.
>
> This recently came up in ASoC too with audio CODECs sharing reset lines,
> there was a discussion started with the reset API maintainer though no
> response yet.  CCing in Cheng-yi who had that problem.  Not deleting
> context for that.
>

Thanks

> > See drivers/media/i2c/ov772x.c FIXME note in power_on() function at:
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832
> >
> > The reset line is in this case is passed to the driver by board file:
> > https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350
> >
> > As you can see PTT3 is used for both sensors (I know, questionable
> > HW design...)
> >
> > Do you think extending gpiod_lookup_flags with this newly introduced
> > NONEXCLUSIVE one is an acceptable solution to avoid handling this in
> > the sensor driver like we're doing today?
>
> One problem you've got there is that you need the two devices to know
> about each other so they coordinate their use of the reset line.  This

That's exactly why the current implementation in those drivers is not
even sub-optimal :)

> was relatively easy in the sysetm Cheng-yi has as it's just one driver
> but what if there's mutiple drivers?  That's relatively likely with
> audio where you might have something like a CODEC with a separate power
> amplifier.  The regulator enable stuff is handling this in the core but
> it's less clear where to put that for reset lines.
>

Isn't DT the natural place where to define a reset line (or any kind of
shared GPIO line) as shared? And for non-OF archs, the board file?

Maybe I should start by adding the NONEXCLUSIVE flags to the ones accepted
by gpio lookup tables and see how it looks?

Thanks
   j


> > Please note this is an ancient architecture that boots from board
> > files, but the same might happen in modern designs with OF support. Is
> > there any clean way to handle shared GPIOs I might not be aware of for
> > those systems?



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

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

[Linus Walleij]

On Fri, Oct 12, 2018 at 6:44 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote:

> > Do you think extending gpiod_lookup_flags with this newly introduced
> > NONEXCLUSIVE one is an acceptable solution to avoid handling this in
> > the sensor driver like we're doing today?
>
> One problem you've got there is that you need the two devices to know
> about each other so they coordinate their use of the reset line.  This
> was relatively easy in the sysetm Cheng-yi has as it's just one driver
> but what if there's mutiple drivers?  That's relatively likely with
> audio where you might have something like a CODEC with a separate power
> amplifier.  The regulator enable stuff is handling this in the core but
> it's less clear where to put that for reset lines.

Yes spot on.

What we need to do for that to work is to move the reference
counting for shared lines over to gpiolib as well, else every subsystem
that needs to share a GPIO line essentially has to reimplement it.

If the consumers are in different subsystems they would even
have to share a reference count and this would lead to a big mess.

So I was thinking to pull that over to gpiolib for the next kernel :)

Hopefully I could do that without breaking anything, but I don't
have a good track record on that so I think the fine people who saw this
breakage will have to help me out.

Yours,
Linus Walleij

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

[Laurent Pinchart]

Hello,

On Friday, 12 October 2018 19:44:24 EEST Mark Brown wrote:
> On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote:
> > Sorry, I'm going slightly OT with this, but please read below.
> > 
> > On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote:
> > > This allows nonexclusive (simultaneous) access to a single
> > > GPIO line for the fixed regulator enable line. This happens
> > 
> > I might have a use case for shared GPIO lines used as 'simple' reset
> > signal for camera devices and not for regulators.
> 
> This recently came up in ASoC too with audio CODECs sharing reset lines,
> there was a discussion started with the reset API maintainer though no
> response yet.  CCing in Cheng-yi who had that problem.  Not deleting
> context for that.
> 
> > See drivers/media/i2c/ov772x.c FIXME note in power_on() function at:
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#
> > L832
> > 
> > The reset line is in this case is passed to the driver by board file:
> > https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/s
> > etup.c#L350
> > 
> > As you can see PTT3 is used for both sensors (I know, questionable
> > HW design...)
> > 
> > Do you think extending gpiod_lookup_flags with this newly introduced
> > NONEXCLUSIVE one is an acceptable solution to avoid handling this in
> > the sensor driver like we're doing today?
> 
> One problem you've got there is that you need the two devices to know
> about each other so they coordinate their use of the reset line.  This
> was relatively easy in the sysetm Cheng-yi has as it's just one driver
> but what if there's mutiple drivers?  That's relatively likely with
> audio where you might have something like a CODEC with a separate power
> amplifier.  The regulator enable stuff is handling this in the core but
> it's less clear where to put that for reset lines.

I've seen other boards where two components sharing a reset signal have an 
active low reset for one, and an active high reset for the other one. Only one 
of the two can be out of reset at a time. That's probably considered as 
"clever" by the hardware engineers, but is awful to support for us.

The core issue in my opinion is that we need code to handle this, and since 
the removal of board files there is no place anymore for such code. Board 
drivers exist in drivers/staging/board/, but that's hardly a solution moving 
forward (the TODO file explicitly states that removal of that code is the end 
goal).

> > Please note this is an ancient architecture that boots from board
> > files, but the same might happen in modern designs with OF support. Is
> > there any clean way to handle shared GPIOs I might not be aware of for
> > those systems?

-- 
Regards,

Laurent Pinchart

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

[Linus Walleij]

On Tue, Oct 16, 2018 at 1:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I've seen other boards where two components sharing a reset signal have an
> active low reset for one, and an active high reset for the other one. Only one
> of the two can be out of reset at a time. That's probably considered as
> "clever" by the hardware engineers, but is awful to support for us.

Haha what a feat. If/when we run into that we simply invent a new
flag like GPIOD_ACTIVE_AMBIGUOUS.

> The core issue in my opinion is that we need code to handle this, and since
> the removal of board files there is no place anymore for such code. Board
> drivers exist in drivers/staging/board/, but that's hardly a solution moving
> forward (the TODO file explicitly states that removal of that code is the end
> goal).

Yeah I kind of already concluded that I need to pull the multiple user
reference counting out of the regulator core and take it over to the
GPIO subsystem so I'm going to attempt that for the next kernel
cycle. It's a neat feature anyways.

Yours,
Linus Walleij