[PATCH v2] regulator: devres: introduce managed enable and disable operations

[PATCH v2] regulator: devres: introduce managed enable and disable operations

[Dmitry Torokhov]

While preferred time to power up the device is when there are users of it
present (i.e. when device is "open"ed), there are times when it makes more
sense to power up the device in probe(). One such example is when device is
expected to be always used (such as a touchscreen on a mobile device, or
similar) and powering it in probe() simplifies driver code, when we need to
power up device to inspect it before completing probe, or when
re-initializing device is too slow/costly.

This patch introduces managed versions of regulator_enable() and
regulator_bulk_enable() so that such drivers do not mix managed and regular
resources, which is usually error-prone.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v2: restored lost regulator_disable() stub

Mark, note that there is also patch introducing devm_clk_prepare() and
devm_clk_prepare_enable() that Russell did not hate so I think it will
get applied eventually. I believe lack of CLK methods was cited as a
reason for not having managed enable for regulators.

 drivers/regulator/devres.c         | 138 +++++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  34 +++++++++
 2 files changed, 172 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 784e3bf32210..ada2081bc24e 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -120,6 +120,62 @@ void devm_regulator_put(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_put);
 
+static void __devm_regulator_disable(struct device *dev, void *res)
+{
+	regulator_disable(*(struct regulator **)res);
+}
+
+/**
+ * devm_regulator_enable - Resource managed regulator_enable()
+ * @regulator: regulator to enable
+ *
+ * Managed regulator_enable(). Regulators enabled by this function are
+ * automatically disabled on driver detach. See regulator_enable() for more
+ * information.
+ */
+int devm_regulator_enable(struct regulator *regulator)
+{
+	struct regulator **ptr;
+	int error;
+
+	ptr = devres_alloc(__devm_regulator_disable, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	error = regulator_enable(regulator);
+	if (error) {
+		devres_free(ptr);
+		return error;
+	}
+
+	*ptr = regulator;
+	devres_add(regulator->dev, ptr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_enable);
+
+/**
+ * devm_regulator_disable - Resource managed regulator_disable()
+ * @regulator: regulator to disable
+ *
+ * Disable a regulator enabled with devm_regulator_enable(). Normally
+ * this function will not need to be called and the resource management
+ * code will ensure that the regulator is disabled.
+ */
+int devm_regulator_disable(struct regulator *regulator)
+{
+	int error;
+
+	error = devres_destroy(regulator->dev, __devm_regulator_disable,
+			       devm_regulator_match, regulator);
+	if (WARN_ON(error))
+		return error;
+
+	return regulator_disable(regulator);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_disable);
+
 struct regulator_bulk_devres {
 	struct regulator_bulk_data *consumers;
 	int num_consumers;
@@ -171,6 +227,88 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_get);
 
+static void __devm_regulator_bulk_disable(struct device *dev, void *res)
+{
+	struct regulator_bulk_devres *devres = res;
+
+	regulator_bulk_disable(devres->num_consumers, devres->consumers);
+}
+
+static int devm_regulator_bulk_match(struct device *dev, void *res, void *data)
+{
+	struct regulator_bulk_devres *r1 = res;
+	struct regulator_bulk_devres *r2 = data;
+
+	if (WARN_ON(!r1 || !r1->consumers || !r1->num_consumers))
+		return 0;
+
+	return r1->consumers == r2->consumers &&
+	       r1->num_consumers == r2->num_consumers;
+}
+
+/**
+ * devm_regulator_bulk_enable - Resource managed regulator_bulk_enable()
+ * @dev: device owning this resource
+ * @num_consumers: number of consumers in @consumers array
+ * @consumers: consumers that need be enabled
+ *
+ * Managed regulator_bulk_enable(). Regulators enabled by this function are
+ * automatically disabled on driver detach. See regulator_bulk_enable() for
+ * more information.
+ */
+int devm_regulator_bulk_enable(struct device *dev, int num_consumers,
+			       struct regulator_bulk_data *consumers)
+{
+	struct regulator_bulk_devres *devres;
+	int error;
+
+	devres = devres_alloc(__devm_regulator_bulk_disable,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	error = regulator_bulk_enable(num_consumers, consumers);
+	if (error) {
+		devres_free(devres);
+		return error;
+	}
+
+	devres->consumers = consumers;
+	devres->num_consumers = num_consumers;
+	devres_add(dev, devres);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_enable);
+
+/**
+ * devm_regulator_bulk_disable - Resource managed regulator_bulk_disable()
+ * @dev: device owning this resource
+ * @num_consumers: number of consumers in @consumers array
+ * @consumers: consumers that were enabled with devm_regulator_bulk_enable()
+ *
+ * Disable regulators enabled with devm_regulator_bulk_enable(). Normally
+ * this function will not need to be called and the resource management
+ * code will ensure that the regulator is disabled.
+ */
+int devm_regulator_bulk_disable(struct device *dev, int num_consumers,
+				struct regulator_bulk_data *consumers)
+{
+	struct regulator_bulk_devres devres = {
+		.consumers	= consumers,
+		.num_consumers	= num_consumers,
+	};
+	int error;
+
+	error = devres_destroy(dev, __devm_regulator_disable,
+			       devm_regulator_bulk_match, &devres);
+	if (WARN_ON(error))
+		return error;
+
+	return regulator_bulk_disable(num_consumers, consumers);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_disable);
+
 static void devm_rdev_release(struct device *dev, void *res)
 {
 	regulator_unregister(*(struct regulator_dev **)res);
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index acaeeec279af..0a81c9ac7b21 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -225,7 +225,9 @@ void devm_regulator_bulk_unregister_supply_alias(struct device *dev,
 
 /* regulator output control and status */
 int __must_check regulator_enable(struct regulator *regulator);
+int __must_check devm_regulator_enable(struct regulator *regulator);
 int regulator_disable(struct regulator *regulator);
+int devm_regulator_disable(struct regulator *regulator);
 int regulator_force_disable(struct regulator *regulator);
 int regulator_is_enabled(struct regulator *regulator);
 int regulator_disable_deferred(struct regulator *regulator, int ms);
@@ -236,8 +238,13 @@ int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
 int __must_check regulator_bulk_enable(int num_consumers,
 				       struct regulator_bulk_data *consumers);
+int __must_check devm_regulator_bulk_enable(struct device *dev,
+					int num_consumers,
+					struct regulator_bulk_data *consumers);
 int regulator_bulk_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers);
+int devm_regulator_bulk_disable(struct device *dev, int num_consumers,
+				struct regulator_bulk_data *consumers);
 int regulator_bulk_force_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers);
 void regulator_bulk_free(int num_consumers,
@@ -399,11 +406,23 @@ static inline int regulator_enable(struct regulator *regulator)
 	return 0;
 }
 
+static inline int devm_regulator_enable(struct device *dev,
+					struct regulator *regulator)
+{
+	return 0;
+}
+
 static inline int regulator_disable(struct regulator *regulator)
 {
 	return 0;
 }
 
+static inline int devm_regulator_disable(struct device *dev,
+					 struct regulator *regulator)
+{
+	return 0;
+}
+
 static inline int regulator_force_disable(struct regulator *regulator)
 {
 	return 0;
@@ -439,12 +458,27 @@ static inline int regulator_bulk_enable(int num_consumers,
 	return 0;
 }
 
+static inline int devm_regulator_bulk_enable(struct device *dev,
+					int num_consumers,
+					struct regulator_bulk_data *consumers)
+{
+	return 0;
+}
+
+
 static inline int regulator_bulk_disable(int num_consumers,
 					 struct regulator_bulk_data *consumers)
 {
 	return 0;
 }
 
+static inline int devm_regulator_bulk_disable(struct device *dev,
+					int num_consumers,
+					struct regulator_bulk_data *consumers)
+{
+	return 0;
+}
+
 static inline int regulator_bulk_force_disable(int num_consumers,
 					struct regulator_bulk_data *consumers)
 {
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Sun, Feb 12, 2017 at 06:32:49PM -0800, Dmitry Torokhov wrote:

> v2: restored lost regulator_disable() stub

> Mark, note that there is also patch introducing devm_clk_prepare() and
> devm_clk_prepare_enable() that Russell did not hate so I think it will
> get applied eventually. I believe lack of CLK methods was cited as a
> reason for not having managed enable for regulators.

No, that's never been an issue.  The concern is partly that nobody
bothered writing the patch but also that it gets messy over suspend and
resume since you end up with drivers either doing explicit releases of
managed resources (which is not normally a good pattern) or mixing
managed and unmanaged access to the same resource which is also fun.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Dmitry Torokhov]

On Mon, Feb 13, 2017 at 06:01:29PM +0000, Mark Brown wrote:
> On Sun, Feb 12, 2017 at 06:32:49PM -0800, Dmitry Torokhov wrote:
> 
> > v2: restored lost regulator_disable() stub
> 
> > Mark, note that there is also patch introducing devm_clk_prepare() and
> > devm_clk_prepare_enable() that Russell did not hate so I think it will
> > get applied eventually. I believe lack of CLK methods was cited as a
> > reason for not having managed enable for regulators.
> 
> No, that's never been an issue.  The concern is partly that nobody
> bothered writing the patch but also that it gets messy over suspend and
> resume since you end up with drivers either doing explicit releases of
> managed resources (which is not normally a good pattern) or mixing
> managed and unmanaged access to the same resource which is also fun.  

I see where you are coming from, but I think that it is lesser concern
than mixing managed and unmanaged resources in probe() and remove() and
making sure that release order is right when they are mixed like that.

I think it is helps if you think about devm_regulator_enable and regular
regulator_enable as managed and unmanaged *actions*, not resources. So
managed action of enabling regulator will be undone on remove() and you
have to manually undo unmanaged regulator_disable() on resume(). It is
not worse than having unbalanced regulator_enable/disable between
probe()/suspend()/resume()/remove().

Thanks.

-- 
Dmitry

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:

> I think it is helps if you think about devm_regulator_enable and regular
> regulator_enable as managed and unmanaged *actions*, not resources. So

That's how I see them but it's still not really helping my concern, in
general if you do a thing with devm_ you don't want to also be
interacting with the same resource in the same way with a non-managed
call.

> managed action of enabling regulator will be undone on remove() and you
> have to manually undo unmanaged regulator_disable() on resume(). It is
> not worse than having unbalanced regulator_enable/disable between
> probe()/suspend()/resume()/remove().

I find it that bit harder to think about - tracking balancing of the
same thing is a lot easier than tracking balancing of two different not
quite equivalent things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Dmitry Torokhov]

On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:
> 
> > I think it is helps if you think about devm_regulator_enable and regular
> > regulator_enable as managed and unmanaged *actions*, not resources. So
> 
> That's how I see them but it's still not really helping my concern, in
> general if you do a thing with devm_ you don't want to also be
> interacting with the same resource in the same way with a non-managed
> call.

It really depends on how you structure your API. For input, for example,
I only provide devm_input_alloc_device() and I made the rest of the
functions handle both managed and unmanaged input devices and they
internally sort it all out between themselves.

But that is what I meant here about managed action. You are not
interacting with managed regulator here, you have managed enable. There
is absolutely nothing preventing you from calling
devm_regulator_enable() on a regulator that was obtained with
regulator_get() (i.e. non-managed).

> 
> > managed action of enabling regulator will be undone on remove() and you
> > have to manually undo unmanaged regulator_disable() on resume(). It is
> > not worse than having unbalanced regulator_enable/disable between
> > probe()/suspend()/resume()/remove().
> 
> I find it that bit harder to think about - tracking balancing of the
> same thing is a lot easier than tracking balancing of two different not
> quite equivalent things.

Hmm... so what do we do (because I think this devm API is quite useful
for cleaning up probe and remove in many drivers)? Do you want it to
operate on a separate counter which we can check against underflow
separately from classic regulator_enable() and regulator_disable()?
Not sure if this will buy us much though and it will make bulk code
uglier...

Thanks.

-- 
Dmitry

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Tue, Feb 21, 2017 at 12:30:03AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> > On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:

> But that is what I meant here about managed action. You are not
> interacting with managed regulator here, you have managed enable. There
> is absolutely nothing preventing you from calling
> devm_regulator_enable() on a regulator that was obtained with
> regulator_get() (i.e. non-managed).

That's not the point, the point is using both devm_regulator_enable()
and regulator_enable() and so on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Dmitry Torokhov]

On Tue, Feb 21, 2017 at 10:56:34AM -0800, Mark Brown wrote:
> On Tue, Feb 21, 2017 at 12:30:03AM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> > > On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:
>
> > But that is what I meant here about managed action. You are not
> > interacting with managed regulator here, you have managed enable. There
> > is absolutely nothing preventing you from calling
> > devm_regulator_enable() on a regulator that was obtained with
> > regulator_get() (i.e. non-managed).
>
> That's not the point, the point is using both devm_regulator_enable()
> and regulator_enable() and so on.

I understand that you have objection that devm_regulator_enable() and
regulator_enable() can be used together, I just do not see it being a
problem in practice.

I still think we need a way for the drivers to "undo" the enable
automatically. Do you have some other idea how to achieve this? Do you
maybe want regulator_put() to undo all outstanding disables for the
regulator? Then drivers would not need to care about disabling
regulators in error paths/driver teardown.

Where would you want to take the API?

Thanks.

-- 
Dmitry