Message-ID: <CAMRc=Mc_nXwvj_9w6w8cB3K58AVLHBLCV+MOO1z_6y+uuT86Og@mail.gmail.com>
Date: Wed, 2 Apr 2025 12:05:24 +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 11:55 PM Mark Brown <broonie@...nel.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@...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.
>

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

Powered by blists - more mailing lists