diff mbox series

[RFC,1/7] regulator: devres: add APIs for reference supplies

Message ID 20240327-regulator-get-enable-get-votlage-v1-1-5f4517faa059@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series regulator: new APIs for voltage reference supplies | expand

Commit Message

David Lechner March 27, 2024, 11:18 p.m. UTC
A common use case for regulators is to supply a reference voltage to an
analog input or output device. This adds two new devres APIs to get,
enable, and get the voltage in a single call. This allows eliminating
boilerplate code in drivers that use reference supplies in this way.

devm_regulator_get_enable_get_voltage() is intended for cases where the
supply is required to provide an external reference voltage.

devm_regulator_get_optional_enable_get_voltage() is intended for cases
where the supply is optional and device typically uses an internal
reference voltage if the supply is not available.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/driver-api/driver-model/devres.rst |  2 +
 drivers/regulator/devres.c                       | 83 ++++++++++++++++++++++++
 include/linux/regulator/consumer.h               | 14 ++++
 3 files changed, 99 insertions(+)

Comments

Jonathan Cameron March 28, 2024, 1:47 p.m. UTC | #1
On Wed, 27 Mar 2024 18:18:50 -0500
David Lechner <dlechner@baylibre.com> wrote:

> A common use case for regulators is to supply a reference voltage to an
> analog input or output device. This adds two new devres APIs to get,
> enable, and get the voltage in a single call. This allows eliminating
> boilerplate code in drivers that use reference supplies in this way.
> 
> devm_regulator_get_enable_get_voltage() is intended for cases where the
Maybe avoid the double get?
devm_regulator_get_enable_read_voltage() perhaps?

> supply is required to provide an external reference voltage.
> 
> devm_regulator_get_optional_enable_get_voltage() is intended for cases
> where the supply is optional and device typically uses an internal
> reference voltage if the supply is not available.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

In general I'll find this very useful (there are 50+ incidence
of the pattern this can replace in IIO).
I would keep it more similar to other devm regulator related functions
and not do error printing internally though.

Jonathan

> ---
>  Documentation/driver-api/driver-model/devres.rst |  2 +
>  drivers/regulator/devres.c                       | 83 ++++++++++++++++++++++++
>  include/linux/regulator/consumer.h               | 14 ++++
>  3 files changed, 99 insertions(+)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 7be8b8dd5f00..fd954d09e13c 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -433,9 +433,11 @@ REGULATOR
>    devm_regulator_bulk_put()
>    devm_regulator_get()
>    devm_regulator_get_enable()
> +  devm_regulator_get_enable_get_voltage()
>    devm_regulator_get_enable_optional()
>    devm_regulator_get_exclusive()
>    devm_regulator_get_optional()
> +  devm_regulator_get_optional_enable_get_voltage()
>    devm_regulator_irq_helper()
>    devm_regulator_put()
>    devm_regulator_register()
> diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
> index 90bb0d178885..e240926defc5 100644
> --- a/drivers/regulator/devres.c
> +++ b/drivers/regulator/devres.c
> @@ -145,6 +145,89 @@ struct regulator *devm_regulator_get_optional(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
>  
> +static int _devm_regulator_get_enable_get_voltage(struct device *dev,
> +						  const char *id,
> +						  bool silent_enodev)
> +{
> +	struct regulator *r;
> +	int ret;
> +
> +	/*
> +	 * Since we need a real voltage, we use devm_regulator_get_optional()
> +	 * here to avoid getting a dummy regulator if the supply is not present.
> +	 */
> +	r = devm_regulator_get_optional(dev, id);
> +	if (silent_enodev && PTR_ERR(r) == -ENODEV)
> +		return -ENODEV;
> +	if (IS_ERR(r))
> +		return dev_err_probe(dev, PTR_ERR(r),

There isn't any obvious precedence that I can see for using dev_err_probe() in these
calls.  I'd not introduce it here.

It's probably enough to print an error message at the caller that
just says we failed to get the voltage (rather than which step failed)./

> +				     "failed to get regulator '%s'\n", id);
> +
> +	ret = regulator_enable(r);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to enable regulator '%s'\n", id);
> +
> +	ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to add disable action for regulator '%s'\n",
> +				     id);
> +
> +	ret = regulator_get_voltage(r);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get voltage for regulator '%s'\n",
> +				     id);
> +
> +	return ret;
> +}
> +
> +/**
> + * devm_regulator_get_enable_get_voltage - Resource managed regulator get and
> + *                                         enable that returns the voltage
> + * @dev: device to supply
> + * @id:  supply name or regulator ID.
> + *
> + * Get and enable regulator for duration of the device life-time.
> + * regulator_disable() and regulator_put() are automatically called on driver
> + * detach. See regulator_get_optional(), regulator_enable(), and
> + * regulator_get_voltage() for more information.
> + *
> + * This is a convenience function for supplies that provide a reference voltage
> + * where the consumer driver just needs to know the voltage and keep the
> + * regulator enabled. Also, as a convenience, this prints error messages on
> + * failure.
> + *
> + * Returns: voltage in microvolts on success, or an error code on failure.
> + */
> +int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id)
> +{
> +	return _devm_regulator_get_enable_get_voltage(dev, id, false);
> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_get_enable_get_voltage);
> +
> +/**
> + * devm_regulator_get_optional_enable_get_voltage - Resource managed regulator
> + *                                                  get and enable that returns
> + *                                                  the voltage
> + * @dev: device to supply
> + * @id:  supply name or regulator ID.
> + *
> + * This function is identical to devm_regulator_get_enable_get_voltage() except
> + * that it does not print an error message in the case of -ENODEV. Callers are
> + * expected to handle -ENODEV as a special case instead of passing it on as an
> + * error.
> + *
> + * Returns: voltage in microvolts on success, or an error code on failure.
> + */
> +int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
> +						   const char *id)
> +{
> +	return _devm_regulator_get_enable_get_voltage(dev, id, true);
> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_get_optional_enable_get_voltage);
> +
>  static int devm_regulator_match(struct device *dev, void *res, void *data)
>  {
>  	struct regulator **r = res;
> diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
> index 4660582a3302..35596db521a0 100644
> --- a/include/linux/regulator/consumer.h
> +++ b/include/linux/regulator/consumer.h
> @@ -164,6 +164,8 @@ struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
>  							   const char *id);
>  int devm_regulator_get_enable(struct device *dev, const char *id);
>  int devm_regulator_get_enable_optional(struct device *dev, const char *id);
> +int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id);
> +int devm_regulator_get_optional_enable_get_voltage(struct device *dev, const char *id);
>  void regulator_put(struct regulator *regulator);
>  void devm_regulator_put(struct regulator *regulator);
>  
> @@ -329,6 +331,18 @@ static inline int devm_regulator_get_enable_optional(struct device *dev,
>  	return -ENODEV;
>  }
>  
> +static inline int devm_regulator_get_enable_get_voltage(struct device *dev,
> +							const char *id)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
> +								 const char *id)
> +{
> +	return -ENODEV;
> +}
> +
>  static inline struct regulator *__must_check
>  regulator_get_optional(struct device *dev, const char *id)
>  {
>
David Lechner March 28, 2024, 3:54 p.m. UTC | #2
On Thu, Mar 28, 2024 at 8:48 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 27 Mar 2024 18:18:50 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > A common use case for regulators is to supply a reference voltage to an
> > analog input or output device. This adds two new devres APIs to get,
> > enable, and get the voltage in a single call. This allows eliminating
> > boilerplate code in drivers that use reference supplies in this way.
> >
> > devm_regulator_get_enable_get_voltage() is intended for cases where the
> Maybe avoid the double get?
> devm_regulator_get_enable_read_voltage() perhaps?

ok with me

>
> > supply is required to provide an external reference voltage.
> >
> > devm_regulator_get_optional_enable_get_voltage() is intended for cases
> > where the supply is optional and device typically uses an internal
> > reference voltage if the supply is not available.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> In general I'll find this very useful (there are 50+ incidence

I didn't find quite that many. Still close to 40 though.

> of the pattern this can replace in IIO).
> I would keep it more similar to other devm regulator related functions
> and not do error printing internally though.

I did this because it seems like we could be losing potentially losing
useful information when something goes wrong making it harder to
troubleshoot which function actually failed. But looking into it more,
the regulator functions already print errors for many of the error
paths, so printing more errors does seem a bit redundant. So I will
remove the dev_error_probe() in v2.
Dmitry Torokhov March 28, 2024, 6:03 p.m. UTC | #3
On Wed, Mar 27, 2024 at 06:18:50PM -0500, David Lechner wrote:
> A common use case for regulators is to supply a reference voltage to an
> analog input or output device. This adds two new devres APIs to get,
> enable, and get the voltage in a single call. This allows eliminating
> boilerplate code in drivers that use reference supplies in this way.
> 
> devm_regulator_get_enable_get_voltage() is intended for cases where the
> supply is required to provide an external reference voltage.
> 
> devm_regulator_get_optional_enable_get_voltage() is intended for cases
> where the supply is optional and device typically uses an internal
> reference voltage if the supply is not available.

So because we decided that we could not have devm_regulator_enable()
because of (IMO) contrived example of someone totally mixing up the devm
and non-devm APIs we now have to make more and more devm- variants
simply because we do not have access to the regulator structure with
devm_regulator_get_enable() and so all normal APIs are not available.

This is quite bad honestly. Mark, could we please reverse this
shortsighted decision and have normal devm_regulator_enable() operating
on a regulator?

Thanks.
Mark Brown March 28, 2024, 6:18 p.m. UTC | #4
On Thu, Mar 28, 2024 at 11:03:23AM -0700, Dmitry Torokhov wrote:

> So because we decided that we could not have devm_regulator_enable()
> because of (IMO) contrived example of someone totally mixing up the devm
> and non-devm APIs we now have to make more and more devm- variants
> simply because we do not have access to the regulator structure with
> devm_regulator_get_enable() and so all normal APIs are not available.

I don't follow what you're saying here?  What normal APIs are not
available?  AFAICT this has nothing to do with a devm enable, it's a
combined operation which reports the voltage for the regulator if one is
available which would still be being added even if it used a devm
enable.

> This is quite bad honestly. Mark, could we please reverse this
> shortsighted decision and have normal devm_regulator_enable() operating
> on a regulator?

Nothing has changed here.
Dmitry Torokhov March 28, 2024, 8:17 p.m. UTC | #5
On Thu, Mar 28, 2024 at 06:18:32PM +0000, Mark Brown wrote:
> On Thu, Mar 28, 2024 at 11:03:23AM -0700, Dmitry Torokhov wrote:
> 
> > So because we decided that we could not have devm_regulator_enable()
> > because of (IMO) contrived example of someone totally mixing up the devm
> > and non-devm APIs we now have to make more and more devm- variants
> > simply because we do not have access to the regulator structure with
> > devm_regulator_get_enable() and so all normal APIs are not available.
> 
> I don't follow what you're saying here?  What normal APIs are not
> available?  AFAICT this has nothing to do with a devm enable, it's a
> combined operation which reports the voltage for the regulator if one is
> available which would still be being added even if it used a devm
> enable.

You can not do devm_regulator_get_enable() and then call
regulator_get_voltage(), you need a new combined API.

> 
> > This is quite bad honestly. Mark, could we please reverse this
> > shortsighted decision and have normal devm_regulator_enable() operating
> > on a regulator?
> 
> Nothing has changed here.

Yes, unfortunately. We could have:

	reg = devm_regulator_get(...);
	...
	err = devm_regulator_enable(dev, reg);
	...
	err = regulator_get_voltage(reg);

and not multiply APIs, but we do not have devm_regulator_enable().

Thanks.
Mark Brown March 28, 2024, 8:25 p.m. UTC | #6
On Thu, Mar 28, 2024 at 01:17:52PM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 28, 2024 at 06:18:32PM +0000, Mark Brown wrote:

> > I don't follow what you're saying here?  What normal APIs are not
> > available?  AFAICT this has nothing to do with a devm enable, it's a
> > combined operation which reports the voltage for the regulator if one is
> > available which would still be being added even if it used a devm
> > enable.

> You can not do devm_regulator_get_enable() and then call
> regulator_get_voltage(), you need a new combined API.

I think the theory here is that there are so many instances of this
reference voltage pattern that it's useful to have a helper for that
reason alone.
Jonathan Cameron March 30, 2024, 4:02 p.m. UTC | #7
On Thu, 28 Mar 2024 20:25:31 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Mar 28, 2024 at 01:17:52PM -0700, Dmitry Torokhov wrote:
> > On Thu, Mar 28, 2024 at 06:18:32PM +0000, Mark Brown wrote:  
> 
> > > I don't follow what you're saying here?  What normal APIs are not
> > > available?  AFAICT this has nothing to do with a devm enable, it's a
> > > combined operation which reports the voltage for the regulator if one is
> > > available which would still be being added even if it used a devm
> > > enable.  
> 
> > You can not do devm_regulator_get_enable() and then call
> > regulator_get_voltage(), you need a new combined API.  
> 
> I think the theory here is that there are so many instances of this
> reference voltage pattern that it's useful to have a helper for that
> reason alone.

Exactly that - this is just adding a convenience function to
remove boilerplate.  -20ish lines of cut and paste code per
driver. 

Jonathan
diff mbox series

Patch

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 7be8b8dd5f00..fd954d09e13c 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -433,9 +433,11 @@  REGULATOR
   devm_regulator_bulk_put()
   devm_regulator_get()
   devm_regulator_get_enable()
+  devm_regulator_get_enable_get_voltage()
   devm_regulator_get_enable_optional()
   devm_regulator_get_exclusive()
   devm_regulator_get_optional()
+  devm_regulator_get_optional_enable_get_voltage()
   devm_regulator_irq_helper()
   devm_regulator_put()
   devm_regulator_register()
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 90bb0d178885..e240926defc5 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -145,6 +145,89 @@  struct regulator *devm_regulator_get_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
 
+static int _devm_regulator_get_enable_get_voltage(struct device *dev,
+						  const char *id,
+						  bool silent_enodev)
+{
+	struct regulator *r;
+	int ret;
+
+	/*
+	 * Since we need a real voltage, we use devm_regulator_get_optional()
+	 * here to avoid getting a dummy regulator if the supply is not present.
+	 */
+	r = devm_regulator_get_optional(dev, id);
+	if (silent_enodev && PTR_ERR(r) == -ENODEV)
+		return -ENODEV;
+	if (IS_ERR(r))
+		return dev_err_probe(dev, PTR_ERR(r),
+				     "failed to get regulator '%s'\n", id);
+
+	ret = regulator_enable(r);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to enable regulator '%s'\n", id);
+
+	ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to add disable action for regulator '%s'\n",
+				     id);
+
+	ret = regulator_get_voltage(r);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "failed to get voltage for regulator '%s'\n",
+				     id);
+
+	return ret;
+}
+
+/**
+ * devm_regulator_get_enable_get_voltage - Resource managed regulator get and
+ *                                         enable that returns the voltage
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get_optional(), regulator_enable(), and
+ * regulator_get_voltage() for more information.
+ *
+ * This is a convenience function for supplies that provide a reference voltage
+ * where the consumer driver just needs to know the voltage and keep the
+ * regulator enabled. Also, as a convenience, this prints error messages on
+ * failure.
+ *
+ * Returns: voltage in microvolts on success, or an error code on failure.
+ */
+int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id)
+{
+	return _devm_regulator_get_enable_get_voltage(dev, id, false);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable_get_voltage);
+
+/**
+ * devm_regulator_get_optional_enable_get_voltage - Resource managed regulator
+ *                                                  get and enable that returns
+ *                                                  the voltage
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * This function is identical to devm_regulator_get_enable_get_voltage() except
+ * that it does not print an error message in the case of -ENODEV. Callers are
+ * expected to handle -ENODEV as a special case instead of passing it on as an
+ * error.
+ *
+ * Returns: voltage in microvolts on success, or an error code on failure.
+ */
+int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
+						   const char *id)
+{
+	return _devm_regulator_get_enable_get_voltage(dev, id, true);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_optional_enable_get_voltage);
+
 static int devm_regulator_match(struct device *dev, void *res, void *data)
 {
 	struct regulator **r = res;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 4660582a3302..35596db521a0 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -164,6 +164,8 @@  struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
 							   const char *id);
 int devm_regulator_get_enable(struct device *dev, const char *id);
 int devm_regulator_get_enable_optional(struct device *dev, const char *id);
+int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id);
+int devm_regulator_get_optional_enable_get_voltage(struct device *dev, const char *id);
 void regulator_put(struct regulator *regulator);
 void devm_regulator_put(struct regulator *regulator);
 
@@ -329,6 +331,18 @@  static inline int devm_regulator_get_enable_optional(struct device *dev,
 	return -ENODEV;
 }
 
+static inline int devm_regulator_get_enable_get_voltage(struct device *dev,
+							const char *id)
+{
+	return -ENODEV;
+}
+
+static inline int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
+								 const char *id)
+{
+	return -ENODEV;
+}
+
 static inline struct regulator *__must_check
 regulator_get_optional(struct device *dev, const char *id)
 {