Message-ID: <846010c0-7dc1-421c-8136-9ae2894c9acd@sirena.org.uk>
Date: Tue, 1 Apr 2025 17:00:23 +0100
From: Mark Brown <broonie@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 0/4] gpio: deprecate and track the removal of GPIO
 workarounds for regulators

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists