| 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 |
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) > { >
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.
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.
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.
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.
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.
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 --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) {
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(+)