Messages in this thread | | | From | Saravana Kannan <> | Date | Wed, 27 May 2020 10:17:21 -0700 | Subject | Re: [PATCH v1] regulator: Add support for sync_state() callbacks |
| |
On Wed, May 27, 2020 at 4:17 AM Mark Brown <broonie@kernel.org> 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
| |