[PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

The GPIOD_FLAGS_BIT_NONEXCLUSIVE flag and devm_gpiod_unhinge() helpers
were introduced as hacky workarounds for resource ownership issues in the
regulator subsystem. Unfortunately, people started using the former in
other places too and now it's in all kinds of drivers.

Let's deprecate both symbols officially, add them to the MAINTAINERS
keywords so that it pops up on our radars when used again, add a task to
track it and I plan to use the power sequencing subsystem to handle the
cases where non-exclusive access to GPIOs is required.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v2:
- reword the task description to focus more on the solution than the
  problem description
- add a task for removing devm_gpiod_unhinge() as well
- Link to v1: https://lore.kernel.org/r/20250331-gpio-todo-remove-nonexclusive-v1-0-25f72675f304@linaro.org

---
Bartosz Golaszewski (4):
      gpio: deprecate the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag
      gpio: deprecate devm_gpiod_unhinge()
      MAINTAINERS: add more keywords for the GPIO subsystem entry
      gpio: TODO: track the removal of regulator-related workarounds

 MAINTAINERS                   |  2 ++
 drivers/gpio/TODO             | 34 ++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib-devres.c |  6 +++++-
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 42 insertions(+), 1 deletion(-)
---
base-commit: 405e2241def89c88f008dcb899eb5b6d4be8b43c
change-id: 20250331-gpio-todo-remove-nonexclusive-ed875467eb56

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v2 1/4] gpio: deprecate the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The non-exclusive GPIO request flag looks like a functional feature but
is in fact a workaround for a corner-case that got out of hand. It should
be removed so deprecate it officially so that nobody uses it anymore.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/gpio/consumer.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 45b651c05b9c..8adc8e9cb4a7 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -31,6 +31,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)
+/* GPIOD_FLAGS_BIT_NONEXCLUSIVE is DEPRECATED, don't use in new code. */
 #define GPIOD_FLAGS_BIT_NONEXCLUSIVE	BIT(4)
 
 /**

-- 
2.45.2

[PATCH v2 2/4] gpio: deprecate devm_gpiod_unhinge()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This function was introduced as a workaround for an issue with resource
ownership in the regulator subsystem. Rather than passing the ownership
of a GPIO, we should make the regulator core be able to deal with
resources it didn't request. Deprecate this function so that we don't
get more users in the tree.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-devres.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 08205f355ceb..120d1ec5af3b 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -317,11 +317,15 @@ EXPORT_SYMBOL_GPL(devm_gpiod_put);
  * @dev:	GPIO consumer
  * @desc:	GPIO descriptor to remove resource management from
  *
+ * *DEPRECATED*
+ * This function should not be used. It's been provided as a workaround for
+ * resource ownership issues in the regulator framework and should be replaced
+ * with a better solution.
+ *
  * Remove resource management from a GPIO descriptor. This is needed when
  * you want to hand over lifecycle management of a descriptor to another
  * mechanism.
  */
-
 void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
 {
 	int ret;

-- 
2.45.2

Re: [PATCH v2 2/4] gpio: deprecate devm_gpiod_unhinge()

[Linus Walleij]

On Tue, Apr 1, 2025 at 2:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This function was introduced as a workaround for an issue with resource
> ownership in the regulator subsystem. Rather than passing the ownership
> of a GPIO, we should make the regulator core be able to deal with
> resources it didn't request. Deprecate this function so that we don't
> get more users in the tree.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 3/4] MAINTAINERS: add more keywords for the GPIO subsystem entry

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add GPIOD_FLAGS_BIT_NONEXCLUSIVE and devm_gpiod_unhinge as keywords to
the GPIO entry so that we get notified if anybody tries to use these
deprecated symbols. We'll drop them from here once we remove them from
the kernel.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ce2b64f4568d..0ce265332577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10151,6 +10151,8 @@ F:	include/linux/gpio.h
 F:	include/linux/gpio/
 F:	include/linux/of_gpio.h
 K:	(devm_)?gpio_(request|free|direction|get|set)
+K:	GPIOD_FLAGS_BIT_NONEXCLUSIVE
+K:	devm_gpiod_unhinge
 
 GPIO UAPI
 M:	Bartosz Golaszewski <brgl@bgdev.pl>

-- 
2.45.2

[PATCH v2 4/4] gpio: TODO: track the removal of regulator-related workarounds

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The GPIOD_FLAGS_BIT_NONEXCLUSIVE flag and devm_gpiod_unhinge() function
should be replaced with a better solution. The pwrseq subsystem is a good
candidate. GPIOs themselves should remain a unique resource. Add a task
for tracking the removal of these deprecated symbols.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/TODO | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpio/TODO b/drivers/gpio/TODO
index b5f0a7a2e1bf..4b70cbaa1caa 100644
--- a/drivers/gpio/TODO
+++ b/drivers/gpio/TODO
@@ -186,3 +186,37 @@ their hardware offsets within the chip.
 
 Encourage users to switch to using them and eventually remove the existing
 global export/unexport attribues.
+
+-------------------------------------------------------------------------------
+
+Remove GPIOD_FLAGS_BIT_NONEXCLUSIVE
+
+GPIOs in the linux kernel are meant to be an exclusive resource. This means
+that the GPIO descriptors (the software representation of the hardware concept)
+are not reference counted and - in general - only one user at a time can
+request a GPIO line and control its settings. The consumer API is designed
+around full control of the line's state as evidenced by the fact that, for
+instance, gpiod_set_value() does indeed drive the line as requested, instead
+of bumping an enable counter of some sort.
+
+A problematic use-case for GPIOs is when two consumers want to use the same
+descriptor independently. An example of such a user is the regulator subsystem
+which may instantiate several struct regulator_dev instances containing
+a struct device but using the same enable GPIO line.
+
+A workaround was introduced in the form of the GPIOD_FLAGS_BIT_NONEXCLUSIVE
+flag but its implementation is problematic: it does not provide any
+synchronization of usage nor did it introduce any enable count meaning the
+non-exclusive users of the same descriptor will in fact "fight" for the
+control over it. This flag should be removed and replaced with a better
+solution, possibly based on the new power sequencing subsystem.
+
+-------------------------------------------------------------------------------
+
+Remove devm_gpiod_unhinge()
+
+devm_gpiod_unhinge() is provided as a way to transfer the ownership of managed
+enable GPIOs to the regulator core. Rather than doing that however, we should
+make it possible for the regulator subsystem to deal with GPIO resources the
+lifetime of which it doesn't control as logically, a GPIO obtained by a caller
+should also be freed by it.

-- 
2.45.2

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote:

> Let's deprecate both symbols officially, add them to the MAINTAINERS
> keywords so that it pops up on our radars when used again, add a task to
> track it and I plan to use the power sequencing subsystem to handle the
> cases where non-exclusive access to GPIOs is required.

What exactly is the plan here?  The regulator (and reset) usage seems
like a reasonable one TBH - the real problem is having an API from the
GPIO subsystem to discover sharing, at the minute you can't resolve a
binding enough to find out if there's sharing without actually
requesting the GPIO.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Tue, Apr 1, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote:
>
> > Let's deprecate both symbols officially, add them to the MAINTAINERS
> > keywords so that it pops up on our radars when used again, add a task to
> > track it and I plan to use the power sequencing subsystem to handle the
> > cases where non-exclusive access to GPIOs is required.
>
> What exactly is the plan here?  The regulator (and reset) usage seems
> like a reasonable one TBH - the real problem is having an API from the
> GPIO subsystem to discover sharing, at the minute you can't resolve a
> binding enough to find out if there's sharing without actually
> requesting the GPIO.

Hard disagree on the reasonable usage. Let's consider the following:

You have two users and one goes gpiod_set_value(desc, 0), the other:
gpiod_set_value(desc, 1). Who is right? Depending on the timing the
resulting value may be either.

For it to make sense, you'd have to add new interfaces:
gpiod_enable(desc) and gpiod_disable(), that would keep track of the
enable count. However you can't remove the hundreds of existing users
of gpiod_set_value() so the problem doesn't go away. But even if you
did introduce these new routines, what about
gpiod_direction_input/output()? My point is: the GPIO consumer API is
designed with exclusive usage in mind and it makes no sense to try to
ram shared GPIOs into the GPIO core.

Also: there are no non-exclusive GPIO usars under drivers/reset/.

What I want to propose is the following:

1. Audit all users of GPIOD_FLAGS_BIT_NONEXCLUSIVE

Outside of drivers/regulator/ it seems that there are several users
who don't really needs this (especially under sound/) and where using
this flag is just a result of a copy-paste.

2. Fix the actual problem with buggy DSDT

Some GPIOs are requested by GPIOLIB ACPI code even though platform
drivers also want them. Those drivers just used the non-exclusive flag
with gpiod_get() but we should instead fix it with an ACPI request
quirk in GPIO core.

3. Use pwrseq where drivers really need non-exclusive GPIOs.

The power sequencing subsystem seems like a good candidate to fix the
issue. I imagine a faux_bus pwrseq driver that would plug into the
right places and provide pwrseq handles which the affected drivers
could either call directly via the pwrseq_get(), pwrseq_power_on/off()
interfaces, or we could have this pwrseq provider register as a GPIO
chip through which the gpiod_ calls from these consumers would go and
the sharing mediated by pwrseq.

Bartosz

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 1, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote:

> > > Let's deprecate both symbols officially, add them to the MAINTAINERS
> > > keywords so that it pops up on our radars when used again, add a task to
> > > track it and I plan to use the power sequencing subsystem to handle the
> > > cases where non-exclusive access to GPIOs is required.

> > What exactly is the plan here?  The regulator (and reset) usage seems
> > like a reasonable one TBH - the real problem is having an API from the
> > GPIO subsystem to discover sharing, at the minute you can't resolve a
> > binding enough to find out if there's sharing without actually
> > requesting the GPIO.

> Hard disagree on the reasonable usage. Let's consider the following:

> You have two users and one goes gpiod_set_value(desc, 0), the other:
> gpiod_set_value(desc, 1). Who is right? Depending on the timing the
> resulting value may be either.

That's why we need to figure out if there's sharing - the usage here is
that we have some number of regulators all of which share the same GPIO
and we want to know that this is the case, provide our own reference
counting and use that to decide when to both update the GPIO and do the
additional stuff like delays that are required.  When the API was number
based we could look up the GPIO numbers for each regulator, compare them
with other GPIOs we've already got to identify sharing and then request
only once.

> For it to make sense, you'd have to add new interfaces:
> gpiod_enable(desc) and gpiod_disable(), that would keep track of the
> enable count. However you can't remove the hundreds of existing users
> of gpiod_set_value() so the problem doesn't go away. But even if you
> did introduce these new routines, what about
> gpiod_direction_input/output()? My point is: the GPIO consumer API is
> designed with exclusive usage in mind and it makes no sense to try to
> ram shared GPIOs into the GPIO core.

That's exactly what the regulator code was doing, as far as the GPIO API
saw there was only ever one user at once.  Since we can't look up
numbers any more what we now do is use non-exclusive requests and check
to see if we already have the GPIO descriptor, if we do then we group
together like we were doing based on the GPIO numbers.  The multiple
gets are just there because that's how the gpiod API tells us if we've
got two references to the same underlying GPIO, only one thing ever
actually configures the GPIO.

> 1. Audit all users of GPIOD_FLAGS_BIT_NONEXCLUSIVE

> Outside of drivers/regulator/ it seems that there are several users
> who don't really needs this (especially under sound/) and where using
> this flag is just a result of a copy-paste.

The sound use cases are roughly the same one - there's a bunch of audio
designs where we've got shared reset lines, they're not actually doing
the reference counting since the use cases mean that practically
speaking all the users will make the same change at the same time (or at
least never have actively conflicting needs) so practically it all ends
up working fine.  IIRC the long term plan was to move over to the reset
API to clean this up rather than redoing the reference counting, if
we're doing this properly we do want to get the thing the regulator API
has where we know and can control when an actual transition happens.

> 3. Use pwrseq where drivers really need non-exclusive GPIOs.

> The power sequencing subsystem seems like a good candidate to fix the
> issue. I imagine a faux_bus pwrseq driver that would plug into the
> right places and provide pwrseq handles which the affected drivers
> could either call directly via the pwrseq_get(), pwrseq_power_on/off()
> interfaces, or we could have this pwrseq provider register as a GPIO
> chip through which the gpiod_ calls from these consumers would go and
> the sharing mediated by pwrseq.

This seems complicated, and I'm not sure that obscuring the concrete
thing we're dealing with isn't going to store up surprises for
ourselves.

It's also not clear to me that pwrseq doesn't just have the same problem
with trying to figure out if two GPIO properties are actually pointing
to the same underlying GPIO that everything else does?  It seems like
the base issue you've got here is that we can't figure out if we've got
two things referencing the same GPIO without fully requesting it.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 1, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote:
>
> > > > Let's deprecate both symbols officially, add them to the MAINTAINERS
> > > > keywords so that it pops up on our radars when used again, add a task to
> > > > track it and I plan to use the power sequencing subsystem to handle the
> > > > cases where non-exclusive access to GPIOs is required.
>
> > > What exactly is the plan here?  The regulator (and reset) usage seems
> > > like a reasonable one TBH - the real problem is having an API from the
> > > GPIO subsystem to discover sharing, at the minute you can't resolve a
> > > binding enough to find out if there's sharing without actually
> > > requesting the GPIO.
>
> > Hard disagree on the reasonable usage. Let's consider the following:
>
> > You have two users and one goes gpiod_set_value(desc, 0), the other:
> > gpiod_set_value(desc, 1). Who is right? Depending on the timing the
> > resulting value may be either.
>
> That's why we need to figure out if there's sharing - the usage here is
> that we have some number of regulators all of which share the same GPIO
> and we want to know that this is the case, provide our own reference
> counting and use that to decide when to both update the GPIO and do the
> additional stuff like delays that are required.  When the API was number
> based we could look up the GPIO numbers for each regulator, compare them
> with other GPIOs we've already got to identify sharing and then request
> only once.
>

That's not a good design though either, is it? For one: it relies on
an implementation detail for which there's no API contract, namely the
idea that the address of the struct gpiod_descr handed out by the call
to gpiod_get() is the same for the same hardware offset on the same
chip. It does work like that at the moment but it's a fragile
assumption. The way pwrseq is implemented for instance, the
"descriptor" obtained from the call to pwrseq_get() is instantiated
per-user, meaning that each user of the same sequence has their own,
unique descriptor. I don't see why such an approach could not be used
in GPIOLIB one day. IOW: nobody ever said that there's a single struct
gpiod_desc per GPIO line.

> > For it to make sense, you'd have to add new interfaces:
> > gpiod_enable(desc) and gpiod_disable(), that would keep track of the
> > enable count. However you can't remove the hundreds of existing users
> > of gpiod_set_value() so the problem doesn't go away. But even if you
> > did introduce these new routines, what about
> > gpiod_direction_input/output()? My point is: the GPIO consumer API is
> > designed with exclusive usage in mind and it makes no sense to try to
> > ram shared GPIOs into the GPIO core.
>
> That's exactly what the regulator code was doing, as far as the GPIO API
> saw there was only ever one user at once.  Since we can't look up
> numbers any more what we now do is use non-exclusive requests and check
> to see if we already have the GPIO descriptor, if we do then we group
> together like we were doing based on the GPIO numbers.  The multiple
> gets are just there because that's how the gpiod API tells us if we've
> got two references to the same underlying GPIO, only one thing ever
> actually configures the GPIO.
>

That's not an unusual situation. For reset-gpios we now have the
implicit wrapper in the form of the reset-gpio.c driver. Unfortunately
we cannot just make it the fallback for all kinds of shared GPIOs so I
suggested a bit more generalized approach with pwrseq. In any case:
having this logic in the regulator core is not only wonky but also
makes it impossible to unduplicate similar use-cases in audio and
networking where shared GPIOs have nothing to do with regulators.

> > 1. Audit all users of GPIOD_FLAGS_BIT_NONEXCLUSIVE
>
> > Outside of drivers/regulator/ it seems that there are several users
> > who don't really needs this (especially under sound/) and where using
> > this flag is just a result of a copy-paste.
>
> The sound use cases are roughly the same one - there's a bunch of audio
> designs where we've got shared reset lines, they're not actually doing
> the reference counting since the use cases mean that practically
> speaking all the users will make the same change at the same time (or at
> least never have actively conflicting needs) so practically it all ends
> up working fine.  IIRC the long term plan was to move over to the reset
> API to clean this up rather than redoing the reference counting, if
> we're doing this properly we do want to get the thing the regulator API
> has where we know and can control when an actual transition happens.
>

If they actually exist as "reset-gpios" in DT then they could probably
use the reset-gpio.c driver. I will take a look.

> > 3. Use pwrseq where drivers really need non-exclusive GPIOs.
>
> > The power sequencing subsystem seems like a good candidate to fix the
> > issue. I imagine a faux_bus pwrseq driver that would plug into the
> > right places and provide pwrseq handles which the affected drivers
> > could either call directly via the pwrseq_get(), pwrseq_power_on/off()
> > interfaces, or we could have this pwrseq provider register as a GPIO
> > chip through which the gpiod_ calls from these consumers would go and
> > the sharing mediated by pwrseq.
>
> This seems complicated, and I'm not sure that obscuring the concrete
> thing we're dealing with isn't going to store up surprises for
> ourselves.

IMO It would be equally as obscured if you used a shared GPIO wrapped
in a reset driver.

>
> It's also not clear to me that pwrseq doesn't just have the same problem
> with trying to figure out if two GPIO properties are actually pointing
> to the same underlying GPIO that everything else does?  It seems like
> the base issue you've got here is that we can't figure out if we've got
> two things referencing the same GPIO without fully requesting it.

Whether that's feasible (I think it is but I'll have a definite answer
once I spend more time on this) is one question. Another is: do you
have anything against removing this flag given it's replaced with a
better solution? If not, then I'd still like to apply this series and
we can discuss the actual solution once I send out the code. I hope
this will at least start happening this release cycle.

Bart

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Tue, Apr 01, 2025 at 08:57:56PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote:

> > > You have two users and one goes gpiod_set_value(desc, 0), the other:
> > > gpiod_set_value(desc, 1). Who is right? Depending on the timing the
> > > resulting value may be either.

> > That's why we need to figure out if there's sharing - the usage here is
> > that we have some number of regulators all of which share the same GPIO
> > and we want to know that this is the case, provide our own reference
> > counting and use that to decide when to both update the GPIO and do the
> > additional stuff like delays that are required.  When the API was number
> > based we could look up the GPIO numbers for each regulator, compare them
> > with other GPIOs we've already got to identify sharing and then request
> > only once.

> That's not a good design though either, is it? For one: it relies on
> an implementation detail for which there's no API contract, namely the

There is an API contract as far as I'm concerned, this was discussed
when Russell was converting things over to use descriptors since we need
something to maintain functionality.  I agree that this is an interface
that is more convenient than elegant but it's what was on offer, I think
the enthusiasm for converting to gpiod was such people were OK with it
since it does actually do the right thing.

> idea that the address of the struct gpiod_descr handed out by the call
> to gpiod_get() is the same for the same hardware offset on the same
> chip. It does work like that at the moment but it's a fragile
> assumption. The way pwrseq is implemented for instance, the
> "descriptor" obtained from the call to pwrseq_get() is instantiated
> per-user, meaning that each user of the same sequence has their own,
> unique descriptor. I don't see why such an approach could not be used
> in GPIOLIB one day. IOW: nobody ever said that there's a single struct
> gpiod_desc per GPIO line.

If gpiolib were to change this API we'd need some other way of getting
the same functionality, I'd be totally fine with that happening.  For
regulators we don't really want the pwrseq behaviour, we want to know
that there's a single underlying GPIO that we're updating.

> > That's exactly what the regulator code was doing, as far as the GPIO API
> > saw there was only ever one user at once.  Since we can't look up
> > numbers any more what we now do is use non-exclusive requests and check
> > to see if we already have the GPIO descriptor, if we do then we group
> > together like we were doing based on the GPIO numbers.  The multiple
> > gets are just there because that's how the gpiod API tells us if we've
> > got two references to the same underlying GPIO, only one thing ever
> > actually configures the GPIO.

> That's not an unusual situation. For reset-gpios we now have the
> implicit wrapper in the form of the reset-gpio.c driver. Unfortunately
> we cannot just make it the fallback for all kinds of shared GPIOs so I
> suggested a bit more generalized approach with pwrseq. In any case:
> having this logic in the regulator core is not only wonky but also
> makes it impossible to unduplicate similar use-cases in audio and
> networking where shared GPIOs have nothing to do with regulators.

Impossible seems pretty strong here?  Part of the thing here is that the
higher level users want to understand that there is GPIO sharing going
on and do something about it, the working out that the thing is shared
isn't really the interesting bit it's rather the part where we do
something about that.  It's not that you can't share some code but it
feels more like a library than an opaque abstraction.

> > The sound use cases are roughly the same one - there's a bunch of audio
> > designs where we've got shared reset lines, they're not actually doing
> > the reference counting since the use cases mean that practically
> > speaking all the users will make the same change at the same time (or at
> > least never have actively conflicting needs) so practically it all ends
> > up working fine.  IIRC the long term plan was to move over to the reset
> > API to clean this up rather than redoing the reference counting, if
> > we're doing this properly we do want to get the thing the regulator API
> > has where we know and can control when an actual transition happens.

> If they actually exist as "reset-gpios" in DT then they could probably
> use the reset-gpio.c driver. I will take a look.

Yes, that was the idea - there was some issue I can't remember that
meant it needed a bit of work on the reset API the last time someone
looked at it.  The properties might have different names reflecting the
pins or something but that seems like a readily solvable problem.

Though now I think again some of them might be closer to the regulator
enables rather than resets so those ones would not fit there and would
more want to librify what regulator is doing...  Something like that
would definitely not feel right being described as a power sequence.

> > > 3. Use pwrseq where drivers really need non-exclusive GPIOs.

> > > The power sequencing subsystem seems like a good candidate to fix the
> > > issue. I imagine a faux_bus pwrseq driver that would plug into the
> > > right places and provide pwrseq handles which the affected drivers
> > > could either call directly via the pwrseq_get(), pwrseq_power_on/off()
> > > interfaces, or we could have this pwrseq provider register as a GPIO
> > > chip through which the gpiod_ calls from these consumers would go and
> > > the sharing mediated by pwrseq.

> > This seems complicated, and I'm not sure that obscuring the concrete
> > thing we're dealing with isn't going to store up surprises for
> > ourselves.

> IMO It would be equally as obscured if you used a shared GPIO wrapped
> in a reset driver.

Yeah, it's a bit indirected but it's at least clear that it's just the
reset and not also any other aspect of the power management so you don't
have to worry about timing requirements around enabling supplies or
whatever.

> > It's also not clear to me that pwrseq doesn't just have the same problem
> > with trying to figure out if two GPIO properties are actually pointing
> > to the same underlying GPIO that everything else does?  It seems like
> > the base issue you've got here is that we can't figure out if we've got
> > two things referencing the same GPIO without fully requesting it.

> Whether that's feasible (I think it is but I'll have a definite answer
> once I spend more time on this) is one question. Another is: do you
> have anything against removing this flag given it's replaced with a
> better solution? If not, then I'd still like to apply this series and
> we can discuss the actual solution once I send out the code. I hope
> this will at least start happening this release cycle.

I'm in no way attached to this specific solution, from my point of view
the important thing is that given two devices using GPIOs we have some
reasonably convenient way of telling if they're using the same underlying 
GPIO and can coordinate between the devices appropriately.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Tue, Apr 1, 2025 at 11:55 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 01, 2025 at 08:57:56PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote:
>
> > > > You have two users and one goes gpiod_set_value(desc, 0), the other:
> > > > gpiod_set_value(desc, 1). Who is right? Depending on the timing the
> > > > resulting value may be either.
>
> > > That's why we need to figure out if there's sharing - the usage here is
> > > that we have some number of regulators all of which share the same GPIO
> > > and we want to know that this is the case, provide our own reference
> > > counting and use that to decide when to both update the GPIO and do the
> > > additional stuff like delays that are required.  When the API was number
> > > based we could look up the GPIO numbers for each regulator, compare them
> > > with other GPIOs we've already got to identify sharing and then request
> > > only once.
>
> > That's not a good design though either, is it? For one: it relies on
> > an implementation detail for which there's no API contract, namely the
>
> There is an API contract as far as I'm concerned, this was discussed
> when Russell was converting things over to use descriptors since we need
> something to maintain functionality.  I agree that this is an interface
> that is more convenient than elegant but it's what was on offer, I think
> the enthusiasm for converting to gpiod was such people were OK with it
> since it does actually do the right thing.
>

Something having been discussed years ago in times of a different
maintainer does not really constitute an API contract :). I understand
the reasoning at the time but I really dislike comparing raw pointers
in particular and this whole abstraction reversal in general. I'd like
to at least have something like gpiod_cmp() or gpiod_is_equivalent()
that would hide the actual logic. Even if, for now, it just remains a
simple pointer comparison.

> > idea that the address of the struct gpiod_descr handed out by the call
> > to gpiod_get() is the same for the same hardware offset on the same
> > chip. It does work like that at the moment but it's a fragile
> > assumption. The way pwrseq is implemented for instance, the
> > "descriptor" obtained from the call to pwrseq_get() is instantiated
> > per-user, meaning that each user of the same sequence has their own,
> > unique descriptor. I don't see why such an approach could not be used
> > in GPIOLIB one day. IOW: nobody ever said that there's a single struct
> > gpiod_desc per GPIO line.
>
> If gpiolib were to change this API we'd need some other way of getting
> the same functionality, I'd be totally fine with that happening.  For
> regulators we don't really want the pwrseq behaviour, we want to know
> that there's a single underlying GPIO that we're updating.
>

This is bothering me. This is the abstraction reversal I'm talking
about. Should the regulator drivers even be concerned about whether
they share resources or not? It's not like consumers of regulators are
concerned about sharing them with other devices. I'm not saying GPIOs
should become reference counted - as I said in a previous email, I
don't believe this makes sense - but GPIOLIB is a lower-level
abstraction to regulators thus the latter shouldn't really reach into
the GPIO core and inspect its structures in order to figure out
whether the lines are shared. This is where an intermediate
abstraction layer may be helpful. The regulator drivers then just go:

handle = pwrseq_get(dev, "enable-gpios");
pwrseq_power_on(handle);

Even if we do it in multiple places, as long as the enable count is
balanced, we're good. The consumers are not concerned by what's
happening behind the scenes just as if it was a reset handle.

> > > That's exactly what the regulator code was doing, as far as the GPIO API
> > > saw there was only ever one user at once.  Since we can't look up
> > > numbers any more what we now do is use non-exclusive requests and check
> > > to see if we already have the GPIO descriptor, if we do then we group
> > > together like we were doing based on the GPIO numbers.  The multiple
> > > gets are just there because that's how the gpiod API tells us if we've
> > > got two references to the same underlying GPIO, only one thing ever
> > > actually configures the GPIO.
>
> > That's not an unusual situation. For reset-gpios we now have the
> > implicit wrapper in the form of the reset-gpio.c driver. Unfortunately
> > we cannot just make it the fallback for all kinds of shared GPIOs so I
> > suggested a bit more generalized approach with pwrseq. In any case:
> > having this logic in the regulator core is not only wonky but also
> > makes it impossible to unduplicate similar use-cases in audio and
> > networking where shared GPIOs have nothing to do with regulators.
>
> Impossible seems pretty strong here?  Part of the thing here is that the
> higher level users want to understand that there is GPIO sharing going
> on and do something about it, the working out that the thing is shared
> isn't really the interesting bit it's rather the part where we do
> something about that.  It's not that you can't share some code but it
> feels more like a library than an opaque abstraction.
>

The part where "the higher level users want to understand that there
is GPIO sharing going on" does not sound correct. Let's take the
example of drivers/net/phy/mscc/mscc_ptp.c which uses the
non-exclusive flag for gpiod_get() because on one of the MIPS
platforms, there are four instances of this PHY that share a GPIO. IMO
it's a hack, this driver shouldn't care about it. It reverses the idea
of the DT providing hardware information to drivers and instead the
driver knows that the DT may describe a shared GPIO.

> > > The sound use cases are roughly the same one - there's a bunch of audio
> > > designs where we've got shared reset lines, they're not actually doing
> > > the reference counting since the use cases mean that practically
> > > speaking all the users will make the same change at the same time (or at
> > > least never have actively conflicting needs) so practically it all ends
> > > up working fine.  IIRC the long term plan was to move over to the reset
> > > API to clean this up rather than redoing the reference counting, if
> > > we're doing this properly we do want to get the thing the regulator API
> > > has where we know and can control when an actual transition happens.
>
> > If they actually exist as "reset-gpios" in DT then they could probably
> > use the reset-gpio.c driver. I will take a look.
>
> Yes, that was the idea - there was some issue I can't remember that
> meant it needed a bit of work on the reset API the last time someone
> looked at it.  The properties might have different names reflecting the
> pins or something but that seems like a readily solvable problem.
>

As long as the hardware description says it's a reset GPIO, we're
fine. It's when people try to use the reset framework for something
that's not a reset when DT maintainers complain.

> Though now I think again some of them might be closer to the regulator
> enables rather than resets so those ones would not fit there and would
> more want to librify what regulator is doing...  Something like that
> would definitely not feel right being described as a power sequence.
>

Well, pwrseq is just a naming convention for want of a better one. It
really is just a subsystem that mediates usage of shared resources and
doesn't bind to any specific kernel subsystem.

[snip]

>
> > > It's also not clear to me that pwrseq doesn't just have the same problem
> > > with trying to figure out if two GPIO properties are actually pointing
> > > to the same underlying GPIO that everything else does?  It seems like
> > > the base issue you've got here is that we can't figure out if we've got
> > > two things referencing the same GPIO without fully requesting it.
>
> > Whether that's feasible (I think it is but I'll have a definite answer
> > once I spend more time on this) is one question. Another is: do you
> > have anything against removing this flag given it's replaced with a
> > better solution? If not, then I'd still like to apply this series and
> > we can discuss the actual solution once I send out the code. I hope
> > this will at least start happening this release cycle.
>
> I'm in no way attached to this specific solution, from my point of view
> the important thing is that given two devices using GPIOs we have some
> reasonably convenient way of telling if they're using the same underlying
> GPIO and can coordinate between the devices appropriately.

I think that logically, consumers of GPIOs shouldn't care about
whether they're shared. IOW: the non-exclusive flag passed to
gpiod_get() should go. If the opposition to using pwrseq here is
strong, then I'd suggest at least moving the handling of the
non-exclusive flag into gpiolib quirks (in gpiolib-of.c or elsewhere)
and not exposing it to consumers who have no business knowing it.

I believe pwrseq could actually be used to hide the enable counting
for GPIOs behind a faux GPIO chip and the consumer would never see a
pwrseq handle - they would instead use GPIO consumer interfaces and
we'd have to agree on what logic would we put behind gpiod_set_value()
(should it effectively work as gpiod_enable() meaning: value is 1 as
long as at least one user sets it to 1?) and
gpiod_direction_input()/output() (same thing: highest priority is
gpiod_direction_output(HIGH) and as long as at least one user sets it
as such, we keep it).

Bartosz

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Wed, Apr 02, 2025 at 12:05:24PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 1, 2025 at 11:55 PM Mark Brown <broonie@kernel.org> wrote:

> > If gpiolib were to change this API we'd need some other way of getting
> > the same functionality, I'd be totally fine with that happening.  For
> > regulators we don't really want the pwrseq behaviour, we want to know
> > that there's a single underlying GPIO that we're updating.

> This is bothering me. This is the abstraction reversal I'm talking
> about. Should the regulator drivers even be concerned about whether
> they share resources or not? It's not like consumers of regulators are

For regulators there's similar issues with needing to know when things
happen (eg, to know if the device actually got reset and needs to be
reintialised) but it's much more likely that we'll both be sharing and
not actually have any control at all even for unshared regulators so we
provide notifiers for this case instead.

> concerned about sharing them with other devices. I'm not saying GPIOs
> should become reference counted - as I said in a previous email, I
> don't believe this makes sense - but GPIOLIB is a lower-level
> abstraction to regulators thus the latter shouldn't really reach into
> the GPIO core and inspect its structures in order to figure out
> whether the lines are shared. This is where an intermediate
> abstraction layer may be helpful. The regulator drivers then just go:

> handle = pwrseq_get(dev, "enable-gpios");
> pwrseq_power_on(handle);

> Even if we do it in multiple places, as long as the enable count is
> balanced, we're good. The consumers are not concerned by what's
> happening behind the scenes just as if it was a reset handle.

No, it's important when (or if) the enable was physically released so we
need to know when the actual hardware operation happened - there's some
delay before the hardware has finished implementing the enable.

> > Impossible seems pretty strong here?  Part of the thing here is that the
> > higher level users want to understand that there is GPIO sharing going
> > on and do something about it, the working out that the thing is shared
> > isn't really the interesting bit it's rather the part where we do
> > something about that.  It's not that you can't share some code but it
> > feels more like a library than an opaque abstraction.

> The part where "the higher level users want to understand that there
> is GPIO sharing going on" does not sound correct. Let's take the
> example of drivers/net/phy/mscc/mscc_ptp.c which uses the
> non-exclusive flag for gpiod_get() because on one of the MIPS
> platforms, there are four instances of this PHY that share a GPIO. IMO
> it's a hack, this driver shouldn't care about it. It reverses the idea
> of the DT providing hardware information to drivers and instead the
> driver knows that the DT may describe a shared GPIO.

Knowing if and when a reset line is actually asserted seems like an
important an useful thing for a driver to know, for example if a chip is
actually reset then we may need to do expensive reinitialisation which
could be skipped if that didn't happen.  

> > Though now I think again some of them might be closer to the regulator
> > enables rather than resets so those ones would not fit there and would
> > more want to librify what regulator is doing...  Something like that
> > would definitely not feel right being described as a power sequence.

> Well, pwrseq is just a naming convention for want of a better one. It
> really is just a subsystem that mediates usage of shared resources and
> doesn't bind to any specific kernel subsystem.

So what's the sequencing bit about then?  Having something that just
shares resources might be useful here, but the sequencing bit seems like
it's asking for landmines.

> > I'm in no way attached to this specific solution, from my point of view
> > the important thing is that given two devices using GPIOs we have some
> > reasonably convenient way of telling if they're using the same underlying
> > GPIO and can coordinate between the devices appropriately.

> I think that logically, consumers of GPIOs shouldn't care about
> whether they're shared. IOW: the non-exclusive flag passed to
> gpiod_get() should go. If the opposition to using pwrseq here is
> strong, then I'd suggest at least moving the handling of the
> non-exclusive flag into gpiolib quirks (in gpiolib-of.c or elsewhere)
> and not exposing it to consumers who have no business knowing it.

I don't think the nonexclusive flag is essential so long as we can
provide some way for users to discover what's going on and coordinate
with each other.  I do think it's important for users to know about at
least the impacts of sharing, and I suspect that for GPIOs usability
means knowing about the sharing.

> I believe pwrseq could actually be used to hide the enable counting
> for GPIOs behind a faux GPIO chip and the consumer would never see a
> pwrseq handle - they would instead use GPIO consumer interfaces and
> we'd have to agree on what logic would we put behind gpiod_set_value()
> (should it effectively work as gpiod_enable() meaning: value is 1 as
> long as at least one user sets it to 1?) and
> gpiod_direction_input()/output() (same thing: highest priority is
> gpiod_direction_output(HIGH) and as long as at least one user sets it
> as such, we keep it).

Like I say that doesn't do the right thing since other users need to be
able to see when something changes on the GPIO.  If that just happens on
normal gpiolib then that complicates usage for the default case since
they now have to worry about things not actually happening when
requested which doesn't seem ideal.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Wed, Apr 2, 2025 at 4:08 PM Mark Brown <broonie@kernel.org> wrote:
>
> > I believe pwrseq could actually be used to hide the enable counting
> > for GPIOs behind a faux GPIO chip and the consumer would never see a
> > pwrseq handle - they would instead use GPIO consumer interfaces and
> > we'd have to agree on what logic would we put behind gpiod_set_value()
> > (should it effectively work as gpiod_enable() meaning: value is 1 as
> > long as at least one user sets it to 1?) and
> > gpiod_direction_input()/output() (same thing: highest priority is
> > gpiod_direction_output(HIGH) and as long as at least one user sets it
> > as such, we keep it).
>
> Like I say that doesn't do the right thing since other users need to be
> able to see when something changes on the GPIO.  If that just happens on
> normal gpiolib then that complicates usage for the default case since
> they now have to worry about things not actually happening when
> requested which doesn't seem ideal.

If I understand correctly, regulator_ena_gpio_ctrl()[1] changes the
GPIO value (even if it's shared) and then notifies regulator consumers
about a DISABLE event. Now if we'd be implicitly sharing the GPIO with
an enable-counter, we could possibly emit an event that's a
false-positive?

In any case: I'll allow myself to apply this series, as the
description does not define a specific approach to take and instead
just allows us to track the rework.

Bartosz

[1] https://elixir.bootlin.com/linux/v6.13/source/drivers/regulator/core.c#L2607

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Mon, Apr 07, 2025 at 03:15:13PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 2, 2025 at 4:08 PM Mark Brown <broonie@kernel.org> wrote:

> > > I believe pwrseq could actually be used to hide the enable counting
> > > for GPIOs behind a faux GPIO chip and the consumer would never see a
> > > pwrseq handle - they would instead use GPIO consumer interfaces and
> > > we'd have to agree on what logic would we put behind gpiod_set_value()
> > > (should it effectively work as gpiod_enable() meaning: value is 1 as
> > > long as at least one user sets it to 1?) and
> > > gpiod_direction_input()/output() (same thing: highest priority is
> > > gpiod_direction_output(HIGH) and as long as at least one user sets it
> > > as such, we keep it).

> > Like I say that doesn't do the right thing since other users need to be
> > able to see when something changes on the GPIO.  If that just happens on
> > normal gpiolib then that complicates usage for the default case since
> > they now have to worry about things not actually happening when
> > requested which doesn't seem ideal.

> If I understand correctly, regulator_ena_gpio_ctrl()[1] changes the
> GPIO value (even if it's shared) and then notifies regulator consumers
> about a DISABLE event. Now if we'd be implicitly sharing the GPIO with
> an enable-counter, we could possibly emit an event that's a
> false-positive?

Yes (or vice versa for enable).  If the device thinks power got pulled
when it didn't it might get confused about what the hardware is doing.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Mon, Apr 7, 2025 at 3:24 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Apr 07, 2025 at 03:15:13PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Apr 2, 2025 at 4:08 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > I believe pwrseq could actually be used to hide the enable counting
> > > > for GPIOs behind a faux GPIO chip and the consumer would never see a
> > > > pwrseq handle - they would instead use GPIO consumer interfaces and
> > > > we'd have to agree on what logic would we put behind gpiod_set_value()
> > > > (should it effectively work as gpiod_enable() meaning: value is 1 as
> > > > long as at least one user sets it to 1?) and
> > > > gpiod_direction_input()/output() (same thing: highest priority is
> > > > gpiod_direction_output(HIGH) and as long as at least one user sets it
> > > > as such, we keep it).
>
> > > Like I say that doesn't do the right thing since other users need to be
> > > able to see when something changes on the GPIO.  If that just happens on
> > > normal gpiolib then that complicates usage for the default case since
> > > they now have to worry about things not actually happening when
> > > requested which doesn't seem ideal.
>
> > If I understand correctly, regulator_ena_gpio_ctrl()[1] changes the
> > GPIO value (even if it's shared) and then notifies regulator consumers
> > about a DISABLE event. Now if we'd be implicitly sharing the GPIO with
> > an enable-counter, we could possibly emit an event that's a
> > false-positive?
>
> Yes (or vice versa for enable).  If the device thinks power got pulled
> when it didn't it might get confused about what the hardware is doing.

Sure, that's the example I was going for, maybe my wording isn't clear.

Basically: we may set the GPIO to 1 but it was already enabled and we
tell consumers the regulator was just enabled when it wasn't OR we set
the GPIO to 0 and tell consumers the regulator was disabled when there
are still users of this GPIO so it's not true either.

AFAICT, it's used in a few places to put the regulator consumer in
reset if the power was *actually* disabled.

I'll think about how that could be improved.

Bartosz

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Mon, Apr 07, 2025 at 03:29:09PM +0200, Bartosz Golaszewski wrote:

> Basically: we may set the GPIO to 1 but it was already enabled and we
> tell consumers the regulator was just enabled when it wasn't OR we set
> the GPIO to 0 and tell consumers the regulator was disabled when there
> are still users of this GPIO so it's not true either.

> AFAICT, it's used in a few places to put the regulator consumer in
> reset if the power was *actually* disabled.

Yes, and also do things like not bother to reinitialise the hardware if
we kept the power on.

> I'll think about how that could be improved.

Thanks.

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Mark Brown]

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

On Mon, Apr 07, 2025 at 02:33:53PM +0100, Mark Brown wrote:
> On Mon, Apr 07, 2025 at 03:29:09PM +0200, Bartosz Golaszewski wrote:
> 
> > Basically: we may set the GPIO to 1 but it was already enabled and we
> > tell consumers the regulator was just enabled when it wasn't OR we set
> > the GPIO to 0 and tell consumers the regulator was disabled when there
> > are still users of this GPIO so it's not true either.
> 
> > AFAICT, it's used in a few places to put the regulator consumer in
> > reset if the power was *actually* disabled.
> 
> Yes, and also do things like not bother to reinitialise the hardware if
> we kept the power on.

BTW, as an example of something that might go wrong if we didn't get
reset when we expected to be one common thing the would be for the
driver to reset it's register cache back to the hardware defaults but if
the power wasn't really lost that won't reflect the actual hardware
state.
back to the hardware defaults 

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

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Linus Walleij]

Hi Bartosz,

this caused me to think about a thing:

On Wed, Apr 2, 2025 at 12:05 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> This is bothering me. This is the abstraction reversal I'm talking
> about. Should the regulator drivers even be concerned about whether
> they share resources or not?
(...)
> The part where "the higher level users want to understand that there
> is GPIO sharing going on" does not sound correct.

There are precedents for this type of semantic IRQF_SHARED
is used whenever two devices share the same IRQ line,
and that is something the drivers have to specify, i.e. the
driver has to be aware that it may be sharing the IRQ
with other devices, and whenever it gets an IRQ it has
to check "was it for me?" and in case it was, return
IRQ_HANDLED else IRQ_NONE.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Tue, Apr 15, 2025 at 11:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Bartosz,
>
> this caused me to think about a thing:
>
> On Wed, Apr 2, 2025 at 12:05 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > This is bothering me. This is the abstraction reversal I'm talking
> > about. Should the regulator drivers even be concerned about whether
> > they share resources or not?
> (...)
> > The part where "the higher level users want to understand that there
> > is GPIO sharing going on" does not sound correct.
>
> There are precedents for this type of semantic IRQF_SHARED
> is used whenever two devices share the same IRQ line,
> and that is something the drivers have to specify, i.e. the
> driver has to be aware that it may be sharing the IRQ
> with other devices, and whenever it gets an IRQ it has
> to check "was it for me?" and in case it was, return
> IRQ_HANDLED else IRQ_NONE.
>

First: this flag has existed (as SA_SHIRQ) since before git days and
could be considered legacy. But also: it's a bit of a different story
as sometimes you get an interrupt and need to read a specific register
to check from the status bits whether it concerns you. This never
happens with a GPIO so I don't think it's a good argument for this
specific case.

Bartosz

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Linus Walleij]

On Thu, Apr 17, 2025 at 8:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Tue, Apr 15, 2025 at 11:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > There are precedents for this type of semantic IRQF_SHARED
> > is used whenever two devices share the same IRQ line,
> > and that is something the drivers have to specify, i.e. the
> > driver has to be aware that it may be sharing the IRQ
> > with other devices, and whenever it gets an IRQ it has
> > to check "was it for me?" and in case it was, return
> > IRQ_HANDLED else IRQ_NONE.
> >
>
> First: this flag has existed (as SA_SHIRQ) since before git days and
> could be considered legacy. But also: it's a bit of a different story
> as sometimes you get an interrupt and need to read a specific register
> to check from the status bits whether it concerns you. This never
> happens with a GPIO so I don't think it's a good argument for this
> specific case.

But at the same time Mark describes that drivers using a shared
GPIO cannot really be opaque as to the status changes on the
GPIO line, as it may or may not need to update register contents
depending on whether the line has actually been low or not while
being disabled. Maybe this can be fixed by a per-consumer status
flag in the descriptor such as bool was_toggled;

Yours,
Linus Walleij

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

On Tue, Apr 22, 2025 at 2:05 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 17, 2025 at 8:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Apr 15, 2025 at 11:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > > There are precedents for this type of semantic IRQF_SHARED
> > > is used whenever two devices share the same IRQ line,
> > > and that is something the drivers have to specify, i.e. the
> > > driver has to be aware that it may be sharing the IRQ
> > > with other devices, and whenever it gets an IRQ it has
> > > to check "was it for me?" and in case it was, return
> > > IRQ_HANDLED else IRQ_NONE.
> > >
> >
> > First: this flag has existed (as SA_SHIRQ) since before git days and
> > could be considered legacy. But also: it's a bit of a different story
> > as sometimes you get an interrupt and need to read a specific register
> > to check from the status bits whether it concerns you. This never
> > happens with a GPIO so I don't think it's a good argument for this
> > specific case.
>
> But at the same time Mark describes that drivers using a shared
> GPIO cannot really be opaque as to the status changes on the
> GPIO line, as it may or may not need to update register contents
> depending on whether the line has actually been low or not while
> being disabled. Maybe this can be fixed by a per-consumer status
> flag in the descriptor such as bool was_toggled;
>

The few use-cases we have for that use regulator notifiers at the
moment and I think async is the way to go here.

Bart

Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO workarounds for regulators

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 01 Apr 2025 14:46:41 +0200, Bartosz Golaszewski wrote:
> The GPIOD_FLAGS_BIT_NONEXCLUSIVE flag and devm_gpiod_unhinge() helpers
> were introduced as hacky workarounds for resource ownership issues in the
> regulator subsystem. Unfortunately, people started using the former in
> other places too and now it's in all kinds of drivers.
> 
> Let's deprecate both symbols officially, add them to the MAINTAINERS
> keywords so that it pops up on our radars when used again, add a task to
> track it and I plan to use the power sequencing subsystem to handle the
> cases where non-exclusive access to GPIOs is required.
> 
> [...]

Applied, thanks!

[1/4] gpio: deprecate the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag
      https://git.kernel.org/brgl/linux/c/6deb8435f6bfcc9b6c7efe3b8a941ae2fb731495
[2/4] gpio: deprecate devm_gpiod_unhinge()
      https://git.kernel.org/brgl/linux/c/686e54ea31f3b7d9d20ac1fa2d0295d649c41f56
[3/4] MAINTAINERS: add more keywords for the GPIO subsystem entry
      https://git.kernel.org/brgl/linux/c/3af64f175b2405270bd0926153d9856a49b58352
[4/4] gpio: TODO: track the removal of regulator-related workarounds
      https://git.kernel.org/brgl/linux/c/2de1cf175c00927c286f8bd72e18602fe072af95

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>