Re: [PATCH v1] regulator: Add support for sync_state() callbacks

[Saravana Kannan]

On Wed, May 27, 2020 at 4:17 AM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Wed, May 27, 2020 at 12:40:56AM -0700, Saravana Kannan wrote:
>
> > When a regulator is left on by the bootloader or anything else before
> > the kernel starts (let's call this a "boot on" regulator), we need to
> > keep it on till all the consumers of the regulator have probed. This is
>
> No, we don't.  As ever we have no idea if there ever will be consumers -
> we don't know what drivers the system is going to load, we don't know
> what the intentions of the OS and system integration are and we have
> zero idea why the system is in the state it's in.

Ok, so not knowing if there'll ever be consumers, drivers, etc is the
issue. But that doesn't mean we don't have to keep the regulators on
till all the consumers in the system that need it have probed. So,
let's solve that problem instead? Because the hardware clearly fails
to boot properly without keeping the regulators on.

Let me try to walk through your concerns and some potential solutions.
If fw_devlink is off or not supported by the firmware (Eg: ACPI), the
behavior doesn't change. We act as if there are no consumers and turn
stuff off 30s after late_initcall_sync(). If fw_devlink is on, then it
makes sure all the consumers (in DT) are linked to the regulator
device as soon as it is added. So that solves your "we don't know if
there'll ever be consumers".

The next concern is, "will the drivers ever be loaded for these
consumers". To handle these cases, I can update the "30s timeout code"
to just release all the "hold state". And I can make the time out a
kernel command line param that if set to 0, then it will actually wait
for all the consumers.

Does that seem like something that'd work for you? That way, if no one
sets this command line param, it won't affect them. And if they set
it, then things work as intended when the system is configured so that
everything does eventually come up.

> > This commit adds a regulator_sync_state() helper function that takes
> > care of all the "boot on" regulator clean up for any regulator driver.
> > All one needs to do is add the following line to the driver struct.
>
> None of the issues around this have *anything* to do with individual
> drivers,

Fair enough. I was just trying to give a way for systems that don't
have the "consumers might never come up" issue (Eg: Android phones) a
way to start using this while we try to figure out what to do for the
systems where it might be a problem.

> all this is doing is forcing us to go through and add this to
> every single driver which doesn't accomplish anything.

I don't see what's wrong with that. The kernel has made plenty of
changes where all the drivers using an API had to be updated in one
shot. This patch doesn't even require a one shot change. Anyway, if
you want to "blanket" do this for every regulator driver, then we can
just set the device's driver.sync_state() when it registers a
regulator and if it isn't already set.

> Regulator
> drivers have no role in this, they don't set policy, so there is no
> reason why they should be aware of any of this.

Agree.

> Please go and look at the previous discussions of this topic, this needs
> to work for other users as well.

I'd be happy to, if you can point me to some of them. Sorry, I didn't
know what to even search for to get a meaningful list of search
results.

Thanks,
Saravana