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

* [PATCH v1] regulator: Add support for sync_state() callbacks
@ 2020-05-27  7:40 Saravana Kannan
  2020-05-27 11:17 ` Mark Brown
  0 siblings, 1 reply; 7+ messages in thread
From: Saravana Kannan @ 2020-05-27  7:40 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown
  Cc: Saravana Kannan, John Stultz, Greg Kroah-Hartman, kernel-team,
	linux-kernel

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 <saravanak@google.com>
---
Mark,

This is just a first cut RFC of how I think this could be handled.  I've
implemented it based on my limited understanding of the regulator
framework. So, I wouldn't be surprised if I've missed out on some corner
(or not so corner) cases. But if you point out the issues, I'd be happy
to try and address them.

Ideally, we can just do this for all the regulators, but we can start
with an "opt-in" scheme where the older behavior is still preserved if a
driver doesn't opt in. I'd also like to remove the arbitrary time out
that we use today, but that's a super long term goal because I know you
have some use cases which might have time limits (and maybe, we'll just
end up with only those drivers opting out of this scheme and sticking
with timeouts).

Also, with what I understand of the regulator code, I think we might be
able to replace/drop the "supply enable propagation" code with a few
tweaks to my patch. But I didn't want to do them in this patch.

I've tested this patch on simple on/off regulators in real hardware with
their drivers compiled as modules and it seems to work correctly. So it
at least works on some level.

Let me know what you think.

Thanks,
Saravana

 drivers/regulator/core.c         | 64 ++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  2 +
 2 files changed, 66 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 941783a14b45..373704b9014e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1768,6 +1768,66 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+static void regulator_hold_state(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev->dev.parent;
+	struct regulation_constraints *c = rdev->constraints;
+
+	if (!dev_has_sync_state(dev) || rdev->sync_supply)
+		return;
+
+	if (rdev->supply_name && !rdev->supply)
+		return;
+
+	if (c && c->always_on)
+		return;
+
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
+		return;
+
+	if (_regulator_is_enabled(rdev) <= 0)
+		return;
+
+	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) {
+		rdev->sync_supply = ERR_PTR(-EINVAL);
+		return;
+	}
+	rdev->open_count++;
+
+	if (regulator_enable(rdev->sync_supply)) {
+		regulator_put(rdev->sync_supply);
+		rdev->sync_supply = ERR_PTR(-EINVAL);
+	}
+}
+
+static int regulator_sync_state_release(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)
+{
+	class_for_each_device(&regulator_class, NULL, dev,
+			      regulator_sync_state_release);
+}
+EXPORT_SYMBOL_GPL(regulator_sync_state);
+
 static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator_dev *r;
@@ -1826,6 +1886,8 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return ret;
 	}
 
+	regulator_hold_state(rdev);
+
 	/*
 	 * In set_machine_constraints() we may have turned this regulator on
 	 * but we couldn't propagate to the supply if it hadn't been resolved
@@ -5188,6 +5250,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	    !rdev->desc->fixed_uV)
 		rdev->is_switch = true;
 
+	regulator_hold_state(rdev);
+
 	dev_set_drvdata(&rdev->dev, rdev);
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 7eb9fea8e482..2e2208e7b771 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -451,6 +451,7 @@ struct regulator_dev {
 	struct regulator *supply;	/* for tree */
 	const char *supply_name;
 	struct regmap *regmap;
+	struct regulator *sync_supply;
 
 	struct delayed_work disable_work;
 
@@ -475,6 +476,7 @@ devm_regulator_register(struct device *dev,
 			const struct regulator_desc *regulator_desc,
 			const struct regulator_config *config);
 void regulator_unregister(struct regulator_dev *rdev);
+void regulator_sync_state(struct device *dev);
 void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev);
 
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread