Message-ID: <a484f0b2-c09a-4a6a-a30e-4c8660d755a6@sirena.org.uk>
Date: Wed, 2 Apr 2025 15:08:20 +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 Wed, Apr 02, 2025 at 12:05:24PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 1, 2025 at 11:55 PM Mark Brown <broonie@...nel.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.

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

Powered by blists - more mailing lists