Message-ID: <CAMRc=Mff0TkeiHbM3TAJLJ2HYU_nnPFUpUjbWsdCnW6O4E=+gQ@mail.gmail.com>
Date: Tue, 1 Apr 2025 20:57:56 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Mark Brown <broonie@...nel.org>
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 1, 2025 at 6:00 PM Mark Brown <broonie@...nel.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@...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.
>

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

Powered by blists - more mailing lists