Message-ID: <c3bb82f9-5a2f-4a14-9726-f3e10bf5d427@sirena.org.uk>
Date: Tue, 1 Apr 2025 22:55:55 +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 08:57:56PM +0200, Bartosz Golaszewski wrote:
> 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:

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

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

Powered by blists - more mailing lists