Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Mon, 15 Jun 2020 at 13:49, Mark Brown wrote: > > On Mon, Jun 15, 2020 at 12:27:23PM +0200, Ulf Hansson wrote: > > > eMMC is not only about voltage levels, but also about enable/disable > > of the regulator(s). > > > More precisely, one needs to follow the steps specified in the eMMC > > spec, when disabling the regulator(s). > > > In other words, the mmc host driver needs to be probed (consumer of > > the regulator), before the regulator(s) can be disabled. > > Right, though you can generally get away with it if you completely cut > the power. Well, there are two regulators. One is the VCC (vmmc) the other is VCCQ (vqmmc). If you can cut both, yes, then you are right. However, quite often VCCQ (the rail for the I/O voltage) is an always-on regulator. Moreover, even if you can cut both regulators, I wouldn't do that, unless really necessary. It's always better with a graceful power-off sequence of a storage device, as otherwise flash corruption and other bad things can more easily happen. > The big thing is that as part of this we need to actually do > the things at the time the driver asks for them to be done rather than > some other time. Right. Kind regards Uffe
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Mon, Jun 15, 2020 at 12:27:23PM +0200, Ulf Hansson wrote: > eMMC is not only about voltage levels, but also about enable/disable > of the regulator(s). > More precisely, one needs to follow the steps specified in the eMMC > spec, when disabling the regulator(s). > In other words, the mmc host driver needs to be probed (consumer of > the regulator), before the regulator(s) can be disabled. Right, though you can generally get away with it if you completely cut the power. The big thing is that as part of this we need to actually do the things at the time the driver asks for them to be done rather than some other time. signature.asc Description: PGP signature
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Tue, 9 Jun 2020 at 12:51, Mark Brown wrote: > > On Mon, Jun 08, 2020 at 08:16:44PM -0700, Saravana Kannan wrote: > > On Mon, Jun 1, 2020 at 10:23 AM Mark Brown wrote: > > > > This is I think the first time anyone has suggested that this is likely > > > to be an issue - the previous concerns have all been about screens > > > glitching. > > > Looks like we got at least one concrete example now in [1]. > > That's the Exynos VDD_INT/CPU issue. I'm not clear that this is > entirely covered TBH, AIUI it needs a coupler all the time so it's not > just a case of waiting for the consumers. > > Please include human readable descriptions of things like commits and > issues being discussed in e-mail in your mails, this makes them much > easier for humans to read especially when they have no internet access. > I do frequently catch up on my mail on flights or while otherwise > travelling so this is even more pressing for me than just being about > making things a bit easier to read. > > > But it doesn't really ignore consumer requests though. In response to > > all your comments above, I haven't done a good job of explaining the > > issues and the solution. I'll try to redo that part again when I send > > v3 and hopefully I can do better. > > Yes, I think a lot of this is about clarity of explanation. > > > > My concern is that introducing extra delays is likely to make things > > > more fragile and complex. As far as I can see this is just making > > > things even worse by adding spurious dependencies and delaying things > > > further. > > > I wouldn't call it delaying any requests. This is just an additional > > constraint like any other consumer. This definitely makes things more > > stable in cases where all the devices probe and even in cases where > > some of those devices might never probe (example I gave in [2]). > > Since your current implementation restricts things until essentially the > entire system is running this is going to affect consumers that don't > share their regulator. Part of the reason I am so against that idea is > that when it is very important that a driver be able to change the > voltage and have that actually take effect usually the hardware will be > built such that that regulator isn't shared so shareability issues don't > apply, we have regulator_get_exclusive() for such situations though > it's wound up not as widely used as it could be for a bunch of reasons. > Things like MMC where we have to conform to a hardware spec that > includes lowering as well as raising voltages will have issues with > this. > eMMC is not only about voltage levels, but also about enable/disable of the regulator(s). More precisely, one needs to follow the steps specified in the eMMC spec, when disabling the regulator(s). In other words, the mmc host driver needs to be probed (consumer of the regulator), before the regulator(s) can be disabled. Kind regards Uffe
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Mon, Jun 08, 2020 at 08:16:44PM -0700, Saravana Kannan wrote: > On Mon, Jun 1, 2020 at 10:23 AM Mark Brown wrote: > > This is I think the first time anyone has suggested that this is likely > > to be an issue - the previous concerns have all been about screens > > glitching. > Looks like we got at least one concrete example now in [1]. That's the Exynos VDD_INT/CPU issue. I'm not clear that this is entirely covered TBH, AIUI it needs a coupler all the time so it's not just a case of waiting for the consumers. Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read. > But it doesn't really ignore consumer requests though. In response to > all your comments above, I haven't done a good job of explaining the > issues and the solution. I'll try to redo that part again when I send > v3 and hopefully I can do better. Yes, I think a lot of this is about clarity of explanation. > > My concern is that introducing extra delays is likely to make things > > more fragile and complex. As far as I can see this is just making > > things even worse by adding spurious dependencies and delaying things > > further. > I wouldn't call it delaying any requests. This is just an additional > constraint like any other consumer. This definitely makes things more > stable in cases where all the devices probe and even in cases where > some of those devices might never probe (example I gave in [2]). Since your current implementation restricts things until essentially the entire system is running this is going to affect consumers that don't share their regulator. Part of the reason I am so against that idea is that when it is very important that a driver be able to change the voltage and have that actually take effect usually the hardware will be built such that that regulator isn't shared so shareability issues don't apply, we have regulator_get_exclusive() for such situations though it's wound up not as widely used as it could be for a bunch of reasons. Things like MMC where we have to conform to a hardware spec that includes lowering as well as raising voltages will have issues with this. > > At best this is working around a problem that has been > > introduced by the decision to do everything at far too broad a level, > > it feels like it's defeating the point of trying to track dependencies > > at all. The whole performance seems completely redundant if we're > > trying to aggregate things so aggressively, we end up needing pretty > > much every device in the system to come up before we can do anything. > This is definitely not true on real Android phones. Keep in mind all > of this only applies to resources actually left on by the bootloader. > So, that drastically cuts down what all devices are affected by this. > So, if a PMIC has 5 regulators and only 1 is left on by the > bootloader, then this patch series would be a NOP for the 4 > regulators. This is your systems - it is not that unusual for systems to come up with most if not all of the regulators up in order to keep things simpler, it's generally easier during bringup and people don't always see any particular reason to modify the bootloader to change things (and may not be easily able to customize what the PMIC does itself even if they wanted to do things at that level). Such systems will be very much impacted by this change. signature.asc Description: PGP signature
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Mon, Jun 1, 2020 at 10:23 AM Mark Brown wrote:
>
> On Fri, May 29, 2020 at 07:39:33PM -0700, Saravana Kannan wrote:
> > On Fri, May 29, 2020 at 6:00 AM Mark Brown wrote:
> > > On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:
>
> > > > especially important for regulators that might be powering more than one
> > > > consumer. Otherwise, when the first consumer probes, enables and then
> > > > disables the "boot on" regulator, it'd turn off the power to rest of the
> > > > consumers of the "boot on" regulator.
>
> > > Which is a problem because...?
>
> > Those consumers that haven't probed can be powered on and active and
> > can crash the system if the power is pulled under their feet.
>
> This is I think the first time anyone has suggested that this is likely
> to be an issue - the previous concerns have all been about screens
> glitching.
Looks like we got at least one concrete example now in [1].
>
> > > > The sync_state() callback that's been added to drivers is meant for
> > > > situations like this. It gets called when all the consumers of a device
> > > > have probed successfully. To ease the transition to sync_state()
> > > > callbacks, it is never called before late_initcall_sync().
>
> > > This is not terribly useful for regulators where adding any of these
> > > delays is going to create surprises. Frankly I can't really see why
> > > deferring things until late_initcall() would help anything.
>
> > Is this different from what's done already?
>
> At the minute we implement any requested changes immediately.
>
> > Not having this delay is
> > easy -- but will have to be at a global level across
> > devices/resources.
>
> That seems fine given that there appears to be no reason to introduce a
> delay.
>
> > > > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > > > +{
>
> > > I find this name very confusing. If anything it's doing the opposite of
> > > setting a minimum state, it's trying to prevent that happening.
>
> > I agree. I renamed it to try and make it better, but I can see how
> > it's more confusing once you called it out. Suggestions?
>
> > regulator_set_boot_limits? regulator_set_boot_constraints?
>
> ignore_consumer_requests()?
But it doesn't really ignore consumer requests though. In response to
all your comments above, I haven't done a good job of explaining the
issues and the solution. I'll try to redo that part again when I send
v3 and hopefully I can do better.
> > > > + /*
> > > > + * Wait for parent supply to be ready before trying to keep this
> > > > + * regulator on.
> > > > + */
> > > > + if (rdev->supply_name && !rdev->supply)
> > > > + return;
>
> > > I can't make sense of this. This stuff only limits disabling, not
> > > enabling, regulators, we're keeping things on here anyway and why would
> > > we care about the supply for disabling?
>
> > We want to wait till supply is set up before we try to put the
> > "enable" vote on a regulator. Otherwise, it won't be propagated
> > properly? I do know that regulator_resolve_supply() takes care of
> > propagating the use count, but we could delete that code if we just
> > till the supply is resolved before "enabling" the regulator. The usual
> > clients can't "get" the regulator anyway without the supply being
> > resolved. Long story short, doing it this way can allow us to delete
> > some functionally duplicative code.
>
> So this is to support some future change that you either haven't
> written or haven't sent out yet. Please don't do stuff like this, it
> makes everything more confusing. Send out isolated, coherent changes
> that do a single thing per patch, if there's things you're thinking of
> for future work then save them for when you actually do that future
> work. That makes everything clearer and easier to follow.
Ok. Thinking more about it, when I try to add voltage handling, I'll
need to call regulator_get_voltage_rdev(). So I might still need to
wait till supply is set up. However, if I don't really need it, I'll
make sure to drop any "future improvements" related changes.
> > > I've just realised that this is even more restrictive than the
> > > descriptions have suggested - it's not just preventing any changes until
> > > all potential consumers of a given regulator have instantiated, it's
> > > preventing changes until all potential consumers of all resources
> > > provided by a given device have instantiated. Given that many systems
> > > have a single PMIC which may also be providing other things like GPIOs
> > > that would mean that any consumer that doesn't instantiate would prevent
> > > any device getting turned off which seems even more concerning.
>
> > Your understanding is correct. I'll try to clarify the commit text as
> > best as I can. Your concerns are why this is not the default behavior.
>
> My concern is that introducing extra delays is likely to make things
>
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Fri, May 29, 2020 at 07:39:33PM -0700, Saravana Kannan wrote:
> On Fri, May 29, 2020 at 6:00 AM Mark Brown wrote:
> > On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:
> > > especially important for regulators that might be powering more than one
> > > consumer. Otherwise, when the first consumer probes, enables and then
> > > disables the "boot on" regulator, it'd turn off the power to rest of the
> > > consumers of the "boot on" regulator.
> > Which is a problem because...?
> Those consumers that haven't probed can be powered on and active and
> can crash the system if the power is pulled under their feet.
This is I think the first time anyone has suggested that this is likely
to be an issue - the previous concerns have all been about screens
glitching.
> > > The sync_state() callback that's been added to drivers is meant for
> > > situations like this. It gets called when all the consumers of a device
> > > have probed successfully. To ease the transition to sync_state()
> > > callbacks, it is never called before late_initcall_sync().
> > This is not terribly useful for regulators where adding any of these
> > delays is going to create surprises. Frankly I can't really see why
> > deferring things until late_initcall() would help anything.
> Is this different from what's done already?
At the minute we implement any requested changes immediately.
> Not having this delay is
> easy -- but will have to be at a global level across
> devices/resources.
That seems fine given that there appears to be no reason to introduce a
delay.
> > > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > > +{
> > I find this name very confusing. If anything it's doing the opposite of
> > setting a minimum state, it's trying to prevent that happening.
> I agree. I renamed it to try and make it better, but I can see how
> it's more confusing once you called it out. Suggestions?
> regulator_set_boot_limits? regulator_set_boot_constraints?
ignore_consumer_requests()?
> > > + /*
> > > + * Wait for parent supply to be ready before trying to keep this
> > > + * regulator on.
> > > + */
> > > + if (rdev->supply_name && !rdev->supply)
> > > + return;
> > I can't make sense of this. This stuff only limits disabling, not
> > enabling, regulators, we're keeping things on here anyway and why would
> > we care about the supply for disabling?
> We want to wait till supply is set up before we try to put the
> "enable" vote on a regulator. Otherwise, it won't be propagated
> properly? I do know that regulator_resolve_supply() takes care of
> propagating the use count, but we could delete that code if we just
> till the supply is resolved before "enabling" the regulator. The usual
> clients can't "get" the regulator anyway without the supply being
> resolved. Long story short, doing it this way can allow us to delete
> some functionally duplicative code.
So this is to support some future change that you either haven't
written or haven't sent out yet. Please don't do stuff like this, it
makes everything more confusing. Send out isolated, coherent changes
that do a single thing per patch, if there's things you're thinking of
for future work then save them for when you actually do that future
work. That makes everything clearer and easier to follow.
> > I've just realised that this is even more restrictive than the
> > descriptions have suggested - it's not just preventing any changes until
> > all potential consumers of a given regulator have instantiated, it's
> > preventing changes until all potential consumers of all resources
> > provided by a given device have instantiated. Given that many systems
> > have a single PMIC which may also be providing other things like GPIOs
> > that would mean that any consumer that doesn't instantiate would prevent
> > any device getting turned off which seems even more concerning.
> Your understanding is correct. I'll try to clarify the commit text as
> best as I can. Your concerns are why this is not the default behavior.
My concern is that introducing extra delays is likely to make things
more fragile and complex. As far as I can see this is just making
things even worse by adding spurious dependencies and delaying things
further.
> For Android phones that are actually shipped, ensuring every enabled
> consumer actually probes is trivial and desired.
Other users exist and need to be considered. People need to be able to
run upstream kernels, they need to be able to run distros and other
systems that aren't vertically integrated like yours where people aren't
developing the DT and kernel together.
> > > + if (IS_ERR_OR_NULL(rdev->sync_supply))
> > > + return 0;
> > > +
> > > + regulator_disable(rdev->sync_supply);
> > I think sync_supply needs a better name, it's a consumer rather than a
> > supply any sync doesn't really fit with what
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Fri, May 29, 2020 at 6:00 AM Mark Brown wrote:
>
> On Thu, May 28, 2020 at 12:06:10PM -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
>
> We we don't in general have a requirement that any of the consumers of a
> regulator will ever probe. As I thought I'd already made clear that
> case really needs to be handled.
Sorry, in the eagerness to address all your concerns, I had forgotten
to update the commit text in v2. I'll reword it to clarify this'll
happen only when there's no timeout set.
> > especially important for regulators that might be powering more than one
> > consumer. Otherwise, when the first consumer probes, enables and then
> > disables the "boot on" regulator, it'd turn off the power to rest of the
> > consumers of the "boot on" regulator.
>
> Which is a problem because...?
Those consumers that haven't probed can be powered on and active and
can crash the system if the power is pulled under their feet.
> > The sync_state() callback that's been added to drivers is meant for
> > situations like this. It gets called when all the consumers of a device
> > have probed successfully. To ease the transition to sync_state()
> > callbacks, it is never called before late_initcall_sync().
>
> This is not terribly useful for regulators where adding any of these
> delays is going to create surprises. Frankly I can't really see why
> deferring things until late_initcall() would help anything.
Is this different from what's done already? Not having this delay is
easy -- but will have to be at a global level across
devices/resources.
> > sync_state() callbacks become even more useful when combined with
> > fw_devlink. If fw_devlink is off, sync_state() is called at
> > late_initcall_sync() or the regulator device probing successfully --
> > whichever is later. This is because, with fw_devlink off, there would be
> > no consumers to the regulator device when it probes.
>
> This breaks the case where no driver ever instantiates for a device.
>
> Oh, actually now I get to the very end of the patch I see there is
> actually a timeout in here which wasn't mentioned in the changelog at
> all. I very nearly didn't read the actual code as according to the
> changelog this issue hadn't been addressed.
Again, sorry I didn't update the commit text. I'll skip replying to
rest of your comments on the commit text, because all of them are due
to stale commit text.
> > 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.
> >
> > .sync_state = regulator_sync_state,
>
> Exactly the same issues as before apply here, why are devices getting
> involved here?
>
> > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > +{
>
> I find this name very confusing. If anything it's doing the opposite of
> setting a minimum state, it's trying to prevent that happening.
I agree. I renamed it to try and make it better, but I can see how
it's more confusing once you called it out. Suggestions?
regulator_set_boot_limits? regulator_set_boot_constraints?
> > + /*
> > + * Wait for parent supply to be ready before trying to keep this
> > + * regulator on.
> > + */
> > + if (rdev->supply_name && !rdev->supply)
> > + return;
>
> I can't make sense of this. This stuff only limits disabling, not
> enabling, regulators, we're keeping things on here anyway and why would
> we care about the supply for disabling?
We want to wait till supply is set up before we try to put the
"enable" vote on a regulator. Otherwise, it won't be propagated
properly? I do know that regulator_resolve_supply() takes care of
propagating the use count, but we could delete that code if we just
till the supply is resolved before "enabling" the regulator. The usual
clients can't "get" the regulator anyway without the supply being
resolved. Long story short, doing it this way can allow us to delete
some functionally duplicative code.
> > +static int regulator_rel_minimum_state(struct device *dev, void *data)
> > +{
> > + struct regulator_dev *rdev = dev_to_rdev(dev);
> > +
> > + if (dev->parent != data)
> > + return 0;
>
> I've just realised that this is even more restrictive than the
> descriptions have suggested - it's not just preventing any changes until
> all potential consumers of a given regulator have instantiated, it's
> preventing changes until all potential consumers of all resources
> provided by a given device have instantiated. Given that many systems
> have a single PMIC which may also be providing other things like GPIOs
> that would mean that any consumer that doesn't instantiate would prevent
> any device
Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks
On Thu, May 28, 2020 at 12:06:10PM -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
We we don't in general have a requirement that any of the consumers of a
regulator will ever probe. As I thought I'd already made clear that
case really needs to be handled.
> especially important for regulators that might be powering more than one
> consumer. Otherwise, when the first consumer probes, enables and then
> disables the "boot on" regulator, it'd turn off the power to rest of the
> consumers of the "boot on" regulator.
Which is a problem because...?
> The sync_state() callback that's been added to drivers is meant for
> situations like this. It gets called when all the consumers of a device
> have probed successfully. To ease the transition to sync_state()
> callbacks, it is never called before late_initcall_sync().
This is not terribly useful for regulators where adding any of these
delays is going to create surprises. Frankly I can't really see why
deferring things until late_initcall() would help anything.
> sync_state() callbacks become even more useful when combined with
> fw_devlink. If fw_devlink is off, sync_state() is called at
> late_initcall_sync() or the regulator device probing successfully --
> whichever is later. This is because, with fw_devlink off, there would be
> no consumers to the regulator device when it probes.
This breaks the case where no driver ever instantiates for a device.
Oh, actually now I get to the very end of the patch I see there is
actually a timeout in here which wasn't mentioned in the changelog at
all. I very nearly didn't read the actual code as according to the
changelog this issue hadn't been addressed.
> 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.
>
> .sync_state = regulator_sync_state,
Exactly the same issues as before apply here, why are devices getting
involved here?
> +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> +{
I find this name very confusing. If anything it's doing the opposite of
setting a minimum state, it's trying to prevent that happening.
> + /*
> + * Wait for parent supply to be ready before trying to keep this
> + * regulator on.
> + */
> + if (rdev->supply_name && !rdev->supply)
> + return;
I can't make sense of this. This stuff only limits disabling, not
enabling, regulators, we're keeping things on here anyway and why would
we care about the supply for disabling?
> +static int regulator_rel_minimum_state(struct device *dev, void *data)
> +{
> + struct regulator_dev *rdev = dev_to_rdev(dev);
> +
> + if (dev->parent != data)
> + return 0;
I've just realised that this is even more restrictive than the
descriptions have suggested - it's not just preventing any changes until
all potential consumers of a given regulator have instantiated, it's
preventing changes until all potential consumers of all resources
provided by a given device have instantiated. Given that many systems
have a single PMIC which may also be providing other things like GPIOs
that would mean that any consumer that doesn't instantiate would prevent
any device getting turned off which seems even more concerning.
> + if (IS_ERR_OR_NULL(rdev->sync_supply))
> + return 0;
> +
> + regulator_disable(rdev->sync_supply);
I think sync_supply needs a better name, it's a consumer rather than a
supply any sync doesn't really fit with what it's doing either - getting
rid of it might be syncing but it's not syncing anything.
The other thing here is that we're doing this silently which is going to
make problems harder to debug, if something gets turned off that wasn't
supposed to be turned off and the system collapses users are going to
have fun working out what happened - this is why the current code to
tidy up idle regulators prints something before it disables things.
> @@ -5188,6 +5272,15 @@ regulator_register(const struct regulator_desc
> *regulator_desc,
> !rdev->desc->fixed_uV)
> rdev->is_switch = true;
>
> + /*
> + * I'm not too enthusiastic about this. I'd rather just go set
> + * .sync_state() in all the regulator drivers. But let's get the rest
> + * of the patch sorted out first.
> + */
> + if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
> + dev_dbg(>dev, "parent sync_state() already set\n");
> + regulator_set_minimum_state(rdev);
If you don't want this to be configurable per driver then why do you
want to force it to be configured per driver? I also think that is a
bad idea, it's just make work as far as I can
[PATCH v2 2/2] regulator: Add support for sync_state() callbacks
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
especially important for regulators that might be powering more than one
consumer. Otherwise, when the first consumer probes, enables and then
disables the "boot on" regulator, it'd turn off the power to rest of the
consumers of the "boot on" regulator.
Also, if a regulator driver is loaded as a module and the regulator
device has a few "boot on" regulators, we still want them to be turned
off after all the consumers have probed (if none of them have requested
for those regulators to stay on).
The sync_state() callback that's been added to drivers is meant for
situations like this. It gets called when all the consumers of a device
have probed successfully. To ease the transition to sync_state()
callbacks, it is never called before late_initcall_sync().
sync_state() callbacks become even more useful when combined with
fw_devlink. If fw_devlink is off, sync_state() is called at
late_initcall_sync() or the regulator device probing successfully --
whichever is later. This is because, with fw_devlink off, there would be
no consumers to the regulator device when it probes.
If fw_devlink is not off (it's permissive by default now), then
sync_state() is called when all the consumers of the regulator device
have probed or at late_initcall_sync() -- whichever is later.
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.
.sync_state = regulator_sync_state,
Once that's done, then for any device that's probed by the driver, all
the "boot on" regulators supplied by the device will be kept on till all
the consumers of the device have probed. Once the consumers have probed,
the "boot on" regulators would be allowed to turn off if they are voted
on by any of the consumers.
Signed-off-by: Saravana Kannan
---
drivers/regulator/core.c | 112 +++
include/linux/regulator/driver.h | 2 +
2 files changed, 114 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 941783a14b45..913d387541d6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1768,6 +1768,88 @@ static struct regulator_dev *regulator_dev_lookup(struct
device *dev,
return ERR_PTR(-ENODEV);
}
+static void regulator_set_minimum_state(struct regulator_dev *rdev)
+{
+ struct device *dev = rdev->dev.parent;
+ struct regulation_constraints *c = rdev->constraints;
+
+ /*
+* The device doesn't have sync_state() support, or we already took
+* care of it.
+*/
+ if (!dev_has_sync_state(dev) || rdev->sync_supply)
+ return;
+
+ /*
+* Wait for parent supply to be ready before trying to keep this
+* regulator on.
+*/
+ if (rdev->supply_name && !rdev->supply)
+ return;
+
+ if (c && c->always_on)
+ return;
+
+ if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
+ return;
+
+ /*
+* If the regulator is disabled or has an error getting the status,
+* assume it's not on. We won't explicitly turn it off now, that can
+* happen later.
+*/
+ if (_regulator_is_enabled(rdev) <= 0)
+ return;
+
+ /*
+* Get a regulator client handle to make requests on the regulator in
+* way that'll prevent it from going below the state in which the boot
+* loader left it in.
+*/
+ rdev->sync_supply = create_regulator(rdev, NULL, "SYNC-SUPPLY");
+
+ /*
+* At this point, we shouldn't have trouble getting a regulator. If we
+* do, give up.
+*/
+ if (rdev->sync_supply == NULL) {
+ dev_err(>dev, "Unable to make votes on the regulator\n");
+ rdev->sync_supply = ERR_PTR(-EINVAL);
+ return;
+ }
+ rdev->open_count++;
+
+ /* If we can't put a vote to keep the regulator enabled, we give up. */
+ if (regulator_enable(rdev->sync_supply)) {
+ dev_err(>dev, "Unable to keep the regulator on\n");
+ regulator_put(rdev->sync_supply);
+ rdev->sync_supply = ERR_PTR(-EINVAL);
+ }
+}
+
+static int regulator_rel_minimum_state(struct device *dev, void *data)
+{
+ struct regulator_dev *rdev = dev_to_rdev(dev);
+
+ if (dev->parent != data)
+ return 0;
+
+ if (IS_ERR_OR_NULL(rdev->sync_supply))
+ return 0;
+
+ regulator_disable(rdev->sync_supply);
+ regulator_put(rdev->sync_supply);
+ rdev->sync_supply = NULL;
+ return 0;
+}
+
+void regulator_sync_state(struct device *dev)
+{
+
