Re: [PATCH RFC 1/2] regulator: core: Disable unused regulators with unknown status

[Mark Brown]

On Wed, Oct 25, 2023 at 09:51:51PM +0200, Stephan Gerhold wrote:
> On Wed, Oct 25, 2023 at 06:49:47PM +0100, Mark Brown wrote:

> > In these cases where we simply can't read the expectation is that we'll
> > always be using the logical state - one way of thinking about it is that
> > the operation is mostly a bootstrapping helper to figure out what the
> > initial state is.  A quick survey of users suggest they'll pretty much
> > all be buggy if we start returning errors, and I frankly even if all the
> > current users were fixed I'd expect that to continue to be a common
> > error.  I suppose that the effect of ignoring the possibility of error
> > is like the current behaviour though.

> regulator_is_enabled() already returns error codes in various cases,
> e.g. regulator_is_enabled_regmap() returns the original error code from
> the regmap_read() call if that fails. So if users ignore that and
> interpret the value as logical one they either don't care (which is
> probably fine in some cases?) or already use it wrong. Or am I missing
> something?

That's broadly what I just indicated.  Expecting anybody to do anything
useful with an error report is probably optimistic, but it's probably
going to give the same behaviour as we have currently so it's probably
fine.

> > We have to do the reference count in the core anyway since it's a
> > reference count not just a simple on/off so it doesn't really cost us
> > anything to make it available to drivers.

> I assume you're referring to "use_count" as the reference counter?

Yes.

> On a closer look I think it cannot be used as-is for my purpose:

>  1. With "regulator-boot-on", set_machine_constraints() explicitly
>     enables the regulator, but doesn't increase the use_count.
>     In that case we should return true in ->is_enabled(). I'm not sure
>     how we would know, just based on use_count = 0.

OK, so use_count plus other information we also already have to hand.
Or OTOH it's not that much overhead to track the enable state explicitly
for hardware without readback as you're suggesting below if it ends up
being too much hassle.

>  2. To cleanup unused regulators that may or may not be enabled we need
>     to know if the regulator was ever explicitly enabled/disabled before.
>     It's pointless to send a disable request for a regulator that we
>     already disabled explicitly before (after a enable -> disable cycle).
>     use_count just tells us if there is currently a user, but not if
>     there was one before.

It's pointless, but equally well it's not huge overhead.

> I think I would literally need to move the existing "enabled" field from
> the RPM regulator drivers to the core and manage it similarly there
> based on ->enable() and ->disable() calls. Which would be a (slight)
> overhead for all regulators rather than being isolated for the few RPM
> regulator drivers.

These aren't the only regulators with this limitation, we've also got
similar open coding for GPIO controlled regulators like the fixed
regualtor for example.

Attachment: signature.asc
Description: PGP signature