In the IIO subsystem, we noticed a pattern in many drivers where we need
to get, enable and get the voltage of a supply that provides a reference
voltage. In these cases, we only need the voltage and not a handle to
the regulator. Another common pattern is for chips to have an internal
reference voltage that is used when an external reference is not
available. There are also a few drivers outside of IIO that do the same.
So we would like to propose a new regulator consumer API to handle these
specific cases to avoid repeating the same boilerplate code in multiple
drivers.
As an example of how these functions are used, I have included a few
patches to consumer drivers. But to avoid a giant patch bomb, I have
omitted the iio/adc and iio/dac patches I have prepared from this
series. I will send those separately but these will add 36 more users
of devm_regulator_get_enable_read_voltage() in addition to the 6 here.
In total, this will eliminate nearly 1000 lines of similar code and will
simplify writing and reviewing new drivers in the future.
---
Changes in v2:
- Reworked regulator patch to remove dev_err_probe() and properly unwind
- Combined two proposed functions into a single function
- Renamed function to devm_regulator_get_enable_read_voltage() everywhere
- Fixed other feedback on consumer patches (see individual patches)
- Picked up Jonathan's Reviewed-bys (hopefully changes were trivial enough)
- Link to v1: https://lore.kernel.org/r/20240327-regulator-get-enable-get-votlage-v1-0-5f4517faa059@baylibre.com
---
David Lechner (7):
regulator: devres: add API for reference voltage supplies
hwmon: (adc128d818) Use devm_regulator_get_enable_read_voltage()
hwmon: (da9052) Use devm_regulator_get_enable_read_voltage()
iio: addac: ad74115: Use devm_regulator_get_enable_read_voltage()
iio: frequency: admv1013: Use devm_regulator_get_enable_read_voltage()
staging: iio: impedance-analyzer: ad5933: Use devm_regulator_get_enable_read_voltage()
Input: mpr121: Use devm_regulator_get_enable_read_voltage()
Documentation/driver-api/driver-model/devres.rst | 1 +
drivers/hwmon/adc128d818.c | 57 +++++++----------------
drivers/hwmon/da9052-hwmon.c | 38 ++++-----------
drivers/iio/addac/ad74115.c | 40 ++++++----------
drivers/iio/frequency/admv1013.c | 40 ++++------------
drivers/input/keyboard/mpr121_touchkey.c | 45 ++----------------
drivers/regulator/devres.c | 59 ++++++++++++++++++++++++
drivers/staging/iio/impedance-analyzer/ad5933.c | 26 +----------
include/linux/regulator/consumer.h | 7 +++
9 files changed, 124 insertions(+), 189 deletions(-)
---
base-commit: 84c1815e46bdcb8bb0259db3d6cdd934cb70a0e9
change-id: 20240326-regulator-get-enable-get-votlage-5dedf40ff338
We can reduce boilerplate code and eliminate the driver remove()
function by using devm_regulator_get_enable_read_voltage().
A new external_vref flag is added since we no longer have the handle
to the regulator to check if it is present.
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* rename to devm_regulator_get_enable_read_voltage()
* use vref instead of err for return value
* simplify last error check to return PTR_ERR directly
---
drivers/hwmon/adc128d818.c | 57 ++++++++++++++--------------------------------
1 file changed, 17 insertions(+), 40 deletions(-)
diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 46e3c8c50765..2a35acb011eb 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -58,7 +58,6 @@ static const u8 num_inputs[] = { 7, 8, 4, 6 };
struct adc128_data {
struct i2c_client *client;
- struct regulator *regulator;
int vref; /* Reference voltage in mV */
struct mutex update_lock;
u8 mode; /* Operation mode */
@@ -389,7 +388,7 @@ static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
return 0;
}
-static int adc128_init_client(struct adc128_data *data)
+static int adc128_init_client(struct adc128_data *data, bool external_vref)
{
struct i2c_client *client = data->client;
int err;
@@ -408,7 +407,7 @@ static int adc128_init_client(struct adc128_data *data)
regval |= data->mode << 1;
/* If external vref is selected, configure the chip to use it */
- if (data->regulator)
+ if (external_vref)
regval |= 0x01;
/* Write advanced configuration register */
@@ -430,9 +429,9 @@ static int adc128_init_client(struct adc128_data *data)
static int adc128_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct regulator *regulator;
struct device *hwmon_dev;
struct adc128_data *data;
+ bool external_vref;
int err, vref;
data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
@@ -440,20 +439,15 @@ static int adc128_probe(struct i2c_client *client)
return -ENOMEM;
/* vref is optional. If specified, is used as chip reference voltage */
- regulator = devm_regulator_get_optional(dev, "vref");
- if (!IS_ERR(regulator)) {
- data->regulator = regulator;
- err = regulator_enable(regulator);
- if (err < 0)
- return err;
- vref = regulator_get_voltage(regulator);
- if (vref < 0) {
- err = vref;
- goto error;
- }
- data->vref = DIV_ROUND_CLOSEST(vref, 1000);
- } else {
+ vref = devm_regulator_get_enable_read_voltage(dev, "vref");
+ if (vref == -ENODEV) {
+ external_vref = false;
data->vref = 2560; /* 2.56V, in mV */
+ } else if (vref < 0) {
+ return vref;
+ } else {
+ external_vref = true;
+ data->vref = DIV_ROUND_CLOSEST(vref, 1000);
}
/* Operation mode is optional. If unspecified, keep current mode */
@@ -461,13 +455,12 @@ static int adc128_probe(struct i2c_client *client)
if (data->mode > 3) {
dev_err(dev, "invalid operation mode %d\n",
data->mode);
- err = -EINVAL;
- goto error;
+ return -EINVAL;
}
} else {
err = i2c_smbus_read_byte_data(client, ADC128_REG_CONFIG_ADV);
if (err < 0)
- goto error;
+ return err;
data->mode = (err >> 1) & ADC128_REG_MASK;
}
@@ -476,31 +469,16 @@ static int adc128_probe(struct i2c_client *client)
mutex_init(&data->update_lock);
/* Initialize the chip */
- err = adc128_init_client(data);
+ err = adc128_init_client(data, external_vref);
if (err < 0)
- goto error;
+ return err;
hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
data, adc128_groups);
- if (IS_ERR(hwmon_dev)) {
- err = PTR_ERR(hwmon_dev);
- goto error;
- }
+ if (IS_ERR(hwmon_dev))
+ return PTR_ERR(hwmon_dev);
return 0;
-
-error:
- if (data->regulator)
- regulator_disable(data->regulator);
- return err;
-}
-
-static void adc128_remove(struct i2c_client *client)
-{
- struct adc128_data *data = i2c_get_clientdata(client);
-
- if (data->regulator)
- regulator_disable(data->regulator);
}
static const struct i2c_device_id adc128_id[] = {
@@ -522,7 +500,6 @@ static struct i2c_driver adc128_driver = {
.of_match_table = of_match_ptr(adc128_of_match),
},
.probe = adc128_probe,
- .remove = adc128_remove,
.id_table = adc128_id,
.detect = adc128_detect,
.address_list = normal_i2c,
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* rename to devm_regulator_get_enable_read_voltage()
* add local variable tsiref_uv instead of using err
* restored error message via dev_err_probe()
* shortened pdev->dev to dev in lines we are touching anyway
---
drivers/hwmon/da9052-hwmon.c | 38 ++++++++++----------------------------
1 file changed, 10 insertions(+), 28 deletions(-)
diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
index 2bd7ae8100d7..7fb0c57dfef5 100644
--- a/drivers/hwmon/da9052-hwmon.c
+++ b/drivers/hwmon/da9052-hwmon.c
@@ -26,7 +26,6 @@ struct da9052_hwmon {
struct mutex hwmon_lock;
bool tsi_as_adc;
int tsiref_mv;
- struct regulator *tsiref;
struct completion tsidone;
};
@@ -397,7 +396,7 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct da9052_hwmon *hwmon;
struct device *hwmon_dev;
- int err;
+ int err, tsiref_uv;
hwmon = devm_kzalloc(dev, sizeof(struct da9052_hwmon), GFP_KERNEL);
if (!hwmon)
@@ -414,32 +413,20 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
device_property_read_bool(pdev->dev.parent, "dlg,tsi-as-adc");
if (hwmon->tsi_as_adc) {
- hwmon->tsiref = devm_regulator_get(pdev->dev.parent, "tsiref");
- if (IS_ERR(hwmon->tsiref)) {
- err = PTR_ERR(hwmon->tsiref);
- dev_err(&pdev->dev, "failed to get tsiref: %d", err);
- return err;
- }
-
- err = regulator_enable(hwmon->tsiref);
- if (err)
- return err;
-
- hwmon->tsiref_mv = regulator_get_voltage(hwmon->tsiref);
- if (hwmon->tsiref_mv < 0) {
- err = hwmon->tsiref_mv;
- goto exit_regulator;
- }
+ tsiref_uv = devm_regulator_get_enable_read_voltage(dev->parent,
+ "tsiref");
+ if (tsiref_uv < 0)
+ return dev_err_probe(dev, tsiref_uv,
+ "failed to get tsiref voltage\n");
/* convert from microvolt (DT) to millivolt (hwmon) */
- hwmon->tsiref_mv /= 1000;
+ hwmon->tsiref_mv = tsiref_uv / 1000;
/* TSIREF limits from datasheet */
if (hwmon->tsiref_mv < 1800 || hwmon->tsiref_mv > 2600) {
dev_err(hwmon->da9052->dev, "invalid TSIREF voltage: %d",
hwmon->tsiref_mv);
- err = -ENXIO;
- goto exit_regulator;
+ return -ENXIO;
}
/* disable touchscreen features */
@@ -456,7 +443,7 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
if (err) {
dev_err(&pdev->dev, "Failed to register TSIRDY IRQ: %d",
err);
- goto exit_regulator;
+ return err;
}
}
@@ -472,9 +459,6 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
exit_irq:
if (hwmon->tsi_as_adc)
da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
-exit_regulator:
- if (hwmon->tsiref)
- regulator_disable(hwmon->tsiref);
return err;
}
@@ -483,10 +467,8 @@ static void da9052_hwmon_remove(struct platform_device *pdev)
{
struct da9052_hwmon *hwmon = platform_get_drvdata(pdev);
- if (hwmon->tsi_as_adc) {
+ if (hwmon->tsi_as_adc)
da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
- regulator_disable(hwmon->tsiref);
- }
}
static struct platform_driver da9052_hwmon_driver = {
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
The common mode voltage is now passed as a parameter in the init
functions so we can avoid adding a state member that is only used
during init.
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* renamed to devm_regulator_get_enable_read_voltage
* restored error message
---
drivers/iio/frequency/admv1013.c | 40 ++++++++++------------------------------
1 file changed, 10 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
index 92923074f930..c0cd5d9844fe 100644
--- a/drivers/iio/frequency/admv1013.c
+++ b/drivers/iio/frequency/admv1013.c
@@ -95,7 +95,6 @@ struct admv1013_state {
struct clk *clkin;
/* Protect against concurrent accesses to the device and to data */
struct mutex lock;
- struct regulator *reg;
struct notifier_block nb;
unsigned int input_mode;
unsigned int quad_se_mode;
@@ -342,14 +341,9 @@ static int admv1013_update_quad_filters(struct admv1013_state *st)
FIELD_PREP(ADMV1013_QUAD_FILTERS_MSK, filt_raw));
}
-static int admv1013_update_mixer_vgate(struct admv1013_state *st)
+static int admv1013_update_mixer_vgate(struct admv1013_state *st, int vcm)
{
unsigned int mixer_vgate;
- int vcm;
-
- vcm = regulator_get_voltage(st->reg);
- if (vcm < 0)
- return vcm;
if (vcm <= 1800000)
mixer_vgate = (2389 * vcm / 1000000 + 8100) / 100;
@@ -443,7 +437,7 @@ static const struct iio_chan_spec admv1013_channels[] = {
ADMV1013_CHAN_CALIB(1, Q),
};
-static int admv1013_init(struct admv1013_state *st)
+static int admv1013_init(struct admv1013_state *st, int vcm_uv)
{
int ret;
unsigned int data;
@@ -483,7 +477,7 @@ static int admv1013_init(struct admv1013_state *st)
if (ret)
return ret;
- ret = admv1013_update_mixer_vgate(st);
+ ret = admv1013_update_mixer_vgate(st, vcm_uv);
if (ret)
return ret;
@@ -498,11 +492,6 @@ static int admv1013_init(struct admv1013_state *st)
st->input_mode);
}
-static void admv1013_reg_disable(void *data)
-{
- regulator_disable(data);
-}
-
static void admv1013_powerdown(void *data)
{
unsigned int enable_reg, enable_reg_msk;
@@ -557,11 +546,6 @@ static int admv1013_properties_parse(struct admv1013_state *st)
else
return -EINVAL;
- st->reg = devm_regulator_get(&spi->dev, "vcm");
- if (IS_ERR(st->reg))
- return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
- "failed to get the common-mode voltage\n");
-
ret = devm_regulator_bulk_get_enable(&st->spi->dev,
ARRAY_SIZE(admv1013_vcc_regs),
admv1013_vcc_regs);
@@ -578,7 +562,7 @@ static int admv1013_probe(struct spi_device *spi)
{
struct iio_dev *indio_dev;
struct admv1013_state *st;
- int ret;
+ int ret, vcm_uv;
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
@@ -597,16 +581,12 @@ static int admv1013_probe(struct spi_device *spi)
if (ret)
return ret;
- ret = regulator_enable(st->reg);
- if (ret) {
- dev_err(&spi->dev, "Failed to enable specified Common-Mode Voltage!\n");
- return ret;
- }
+ ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcm");
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "failed to get the common-mode voltage\n");
- ret = devm_add_action_or_reset(&spi->dev, admv1013_reg_disable,
- st->reg);
- if (ret)
- return ret;
+ vcm_uv = ret;
st->clkin = devm_clk_get_enabled(&spi->dev, "lo_in");
if (IS_ERR(st->clkin))
@@ -620,7 +600,7 @@ static int admv1013_probe(struct spi_device *spi)
mutex_init(&st->lock);
- ret = admv1013_init(st);
+ ret = admv1013_init(st, vcm_uv);
if (ret) {
dev_err(&spi->dev, "admv1013 init failed\n");
return ret;
--
2.43.2
A common use case for regulators is to supply a reference voltage to an
analog input or output device. This adds a new devres API 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.
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* removed dev_err_probe() on error return
* dropped optional version of the function since there is no more
difference after dev_err_probe() is removed
* renamed function to devm_regulator_get_enable_read_voltage()
* added unwinding on error paths
---
Documentation/driver-api/driver-model/devres.rst | 1 +
drivers/regulator/devres.c | 59 ++++++++++++++++++++++++
include/linux/regulator/consumer.h | 7 +++
3 files changed, 67 insertions(+)
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 7be8b8dd5f00..18caebad7376 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -433,6 +433,7 @@ REGULATOR
devm_regulator_bulk_put()
devm_regulator_get()
devm_regulator_get_enable()
+ devm_regulator_get_enable_read_voltage()
devm_regulator_get_enable_optional()
devm_regulator_get_exclusive()
devm_regulator_get_optional()
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 90bb0d178885..4f290b9b559b 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -145,6 +145,65 @@ struct regulator *devm_regulator_get_optional(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
+/**
+ * devm_regulator_get_enable_read_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.
+ *
+ * In cases where the supply is not strictly required, callers can check for
+ * -ENODEV error and handle it accordingly.
+ *
+ * Returns: voltage in microvolts on success, or an error code on failure.
+ */
+int devm_regulator_get_enable_read_voltage(struct device *dev, const char *id)
+{
+ struct regulator *r;
+ int ret;
+
+ /*
+ * Since we need a real voltage, we use devm_regulator_get_optional()
+ * rather than getting a dummy regulator with devm_regulator_get() and
+ * then letting regulator_get_voltage() fail with -EINVAL. This way, the
+ * caller can handle the -ENODEV error code if needed instead of the
+ * ambiguous -EINVAL.
+ */
+ r = devm_regulator_get_optional(dev, id);
+ if (IS_ERR(r))
+ return PTR_ERR(r);
+
+ ret = regulator_enable(r);
+ if (ret)
+ goto err_regulator_put;
+
+ ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
+ if (ret)
+ goto err_regulator_put;
+
+ ret = regulator_get_voltage(r);
+ if (ret < 0)
+ goto err_release_action;
+
+ return 0;
+
+err_release_action:
+ devm_release_action(dev, regulator_action_disable, r);
+err_regulator_put:
+ devm_regulator_put(r);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable_read_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 ed180ca419da..59d0b9a79e6e 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -164,6 +164,7 @@ 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_read_voltage(struct device *dev, const char *id);
void regulator_put(struct regulator *regulator);
void devm_regulator_put(struct regulator *regulator);
@@ -329,6 +330,12 @@ static inline int devm_regulator_get_enable_optional(struct device *dev,
return 0;
}
+static inline int devm_regulator_get_enable_read_voltage(struct device *dev,
+ const char *id)
+{
+ return -ENODEV;
+}
+
static inline struct regulator *__must_check
regulator_get_optional(struct device *dev, const char *id)
{
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
To maintain backwards compatibility in the case a DT does not provide
an avdd-supply, we fall back to calling devm_regulator_get_enable()
so that there is no change in user-facing behavior (e.g. dummy regulator
will still be in sysfs).
Also add an informative error message when we failed to get the voltage
and knowing the voltage is required while we are touching this.
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* renamed to devm_regulator_get_enable_read_voltage()
* restored error message on failure
* restored validation check in ad74115_setup() and added error message
* added fallback call to devm_regulator_get_enable() for compatibility
---
drivers/iio/addac/ad74115.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c
index e6bc5eb3788d..12dc43d487b4 100644
--- a/drivers/iio/addac/ad74115.c
+++ b/drivers/iio/addac/ad74115.c
@@ -199,7 +199,6 @@ struct ad74115_state {
struct spi_device *spi;
struct regmap *regmap;
struct iio_trigger *trig;
- struct regulator *avdd;
/*
* Synchronize consecutive operations when doing a one-shot
@@ -1672,13 +1671,9 @@ static int ad74115_setup(struct iio_dev *indio_dev)
if (ret)
return ret;
- if (val == AD74115_DIN_THRESHOLD_MODE_AVDD) {
- ret = regulator_get_voltage(st->avdd);
- if (ret < 0)
- return ret;
-
- st->avdd_mv = ret / 1000;
- }
+ if (val == AD74115_DIN_THRESHOLD_MODE_AVDD && !st->avdd_mv)
+ return dev_err_probe(dev, -EINVAL,
+ "AVDD voltage is required for digital input threshold mode AVDD\n");
st->din_threshold_mode = val;
@@ -1788,11 +1783,6 @@ static int ad74115_reset(struct ad74115_state *st)
return 0;
}
-static void ad74115_regulator_disable(void *data)
-{
- regulator_disable(data);
-}
-
static int ad74115_setup_trigger(struct iio_dev *indio_dev)
{
struct ad74115_state *st = iio_priv(indio_dev);
@@ -1855,20 +1845,20 @@ static int ad74115_probe(struct spi_device *spi)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &ad74115_info;
- st->avdd = devm_regulator_get(dev, "avdd");
- if (IS_ERR(st->avdd))
- return PTR_ERR(st->avdd);
-
- ret = regulator_enable(st->avdd);
- if (ret) {
- dev_err(dev, "Failed to enable avdd regulator\n");
- return ret;
+ ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
+ if (ret < 0) {
+ /*
+ * Since this is both a power supply and only optionally a
+ * reference voltage, make sure to enable it even when the
+ * voltage is not available.
+ */
+ ret = devm_regulator_get_enable(dev, "avdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable avdd\n");
+ } else {
+ st->avdd_mv = ret / 1000;
}
- ret = devm_add_action_or_reset(dev, ad74115_regulator_disable, st->avdd);
- if (ret)
- return ret;
-
ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
regulator_names);
if (ret)
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* fixed wrong driver name in patch subject
* renamed to devm_regulator_get_enable_read_voltage()
* restored error message
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 26 ++-----------------------
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 9149d41fe65b..b7af5fe63e09 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -84,7 +84,6 @@
struct ad5933_state {
struct i2c_client *client;
- struct regulator *reg;
struct clk *mclk;
struct delayed_work work;
struct mutex lock; /* Protect sensor state */
@@ -660,13 +659,6 @@ static void ad5933_work(struct work_struct *work)
}
}
-static void ad5933_reg_disable(void *data)
-{
- struct ad5933_state *st = data;
-
- regulator_disable(st->reg);
-}
-
static int ad5933_probe(struct i2c_client *client)
{
const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -685,23 +677,9 @@ static int ad5933_probe(struct i2c_client *client)
mutex_init(&st->lock);
- st->reg = devm_regulator_get(&client->dev, "vdd");
- if (IS_ERR(st->reg))
- return PTR_ERR(st->reg);
-
- ret = regulator_enable(st->reg);
- if (ret) {
- dev_err(&client->dev, "Failed to enable specified VDD supply\n");
- return ret;
- }
-
- ret = devm_add_action_or_reset(&client->dev, ad5933_reg_disable, st);
- if (ret)
- return ret;
-
- ret = regulator_get_voltage(st->reg);
+ ret = devm_regulator_get_enable_read_voltage(&client->dev, "vdd");
if (ret < 0)
- return ret;
+ return dev_err_probe(&client->dev, ret, "failed to get vdd voltage\n");
st->vref_mv = ret / 1000;
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v2 changes:
* renamed to devm_regulator_get_enable_read_voltage()
* restored error message
---
drivers/input/keyboard/mpr121_touchkey.c | 45 +++-----------------------------
1 file changed, 3 insertions(+), 42 deletions(-)
diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index d434753afab1..0ea3ab9b8bbb 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -82,42 +82,6 @@ static const struct mpr121_init_register init_reg_table[] = {
{ AUTO_CONFIG_CTRL_ADDR, 0x0b },
};
-static void mpr121_vdd_supply_disable(void *data)
-{
- struct regulator *vdd_supply = data;
-
- regulator_disable(vdd_supply);
-}
-
-static struct regulator *mpr121_vdd_supply_init(struct device *dev)
-{
- struct regulator *vdd_supply;
- int err;
-
- vdd_supply = devm_regulator_get(dev, "vdd");
- if (IS_ERR(vdd_supply)) {
- dev_err(dev, "failed to get vdd regulator: %ld\n",
- PTR_ERR(vdd_supply));
- return vdd_supply;
- }
-
- err = regulator_enable(vdd_supply);
- if (err) {
- dev_err(dev, "failed to enable vdd regulator: %d\n", err);
- return ERR_PTR(err);
- }
-
- err = devm_add_action_or_reset(dev, mpr121_vdd_supply_disable,
- vdd_supply);
- if (err) {
- dev_err(dev, "failed to add disable regulator action: %d\n",
- err);
- return ERR_PTR(err);
- }
-
- return vdd_supply;
-}
-
static void mpr_touchkey_report(struct input_dev *dev)
{
struct mpr121_touchkey *mpr121 = input_get_drvdata(dev);
@@ -233,7 +197,6 @@ static int mpr121_phys_init(struct mpr121_touchkey *mpr121,
static int mpr_touchkey_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct regulator *vdd_supply;
int vdd_uv;
struct mpr121_touchkey *mpr121;
struct input_dev *input_dev;
@@ -241,11 +204,9 @@ static int mpr_touchkey_probe(struct i2c_client *client)
int error;
int i;
- vdd_supply = mpr121_vdd_supply_init(dev);
- if (IS_ERR(vdd_supply))
- return PTR_ERR(vdd_supply);
-
- vdd_uv = regulator_get_voltage(vdd_supply);
+ vdd_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+ if (vdd_uv < 0)
+ return dev_err_probe(dev, vdd_uv, "failed to get vdd voltage\n");
mpr121 = devm_kzalloc(dev, sizeof(*mpr121), GFP_KERNEL);
if (!mpr121)
--
2.43.2
On Mon, Apr 29, 2024 at 6:40 PM David Lechner <[email protected]> wrote:
>
> A common use case for regulators is to supply a reference voltage to an
> analog input or output device. This adds a new devres API 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.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes:
> * removed dev_err_probe() on error return
> * dropped optional version of the function since there is no more
> difference after dev_err_probe() is removed
> * renamed function to devm_regulator_get_enable_read_voltage()
> * added unwinding on error paths
> ---
> Documentation/driver-api/driver-model/devres.rst | 1 +
> drivers/regulator/devres.c | 59 ++++++++++++++++++++++++
> include/linux/regulator/consumer.h | 7 +++
> 3 files changed, 67 insertions(+)
>
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 7be8b8dd5f00..18caebad7376 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -433,6 +433,7 @@ REGULATOR
> devm_regulator_bulk_put()
> devm_regulator_get()
> devm_regulator_get_enable()
> + devm_regulator_get_enable_read_voltage()
> devm_regulator_get_enable_optional()
> devm_regulator_get_exclusive()
> devm_regulator_get_optional()
> diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
> index 90bb0d178885..4f290b9b559b 100644
> --- a/drivers/regulator/devres.c
> +++ b/drivers/regulator/devres.c
> @@ -145,6 +145,65 @@ struct regulator *devm_regulator_get_optional(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
>
> +/**
> + * devm_regulator_get_enable_read_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.
> + *
> + * In cases where the supply is not strictly required, callers can check for
> + * -ENODEV error and handle it accordingly.
> + *
> + * Returns: voltage in microvolts on success, or an error code on failure.
> + */
> +int devm_regulator_get_enable_read_voltage(struct device *dev, const char *id)
> +{
> + struct regulator *r;
> + int ret;
> +
> + /*
> + * Since we need a real voltage, we use devm_regulator_get_optional()
> + * rather than getting a dummy regulator with devm_regulator_get() and
> + * then letting regulator_get_voltage() fail with -EINVAL. This way, the
> + * caller can handle the -ENODEV error code if needed instead of the
> + * ambiguous -EINVAL.
> + */
> + r = devm_regulator_get_optional(dev, id);
> + if (IS_ERR(r))
> + return PTR_ERR(r);
> +
> + ret = regulator_enable(r);
> + if (ret)
> + goto err_regulator_put;
> +
> + ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
> + if (ret)
> + goto err_regulator_put;
> +
> + ret = regulator_get_voltage(r);
> + if (ret < 0)
> + goto err_release_action;
> +
> + return 0;
Oof. Apparently I didn't test this as well as I should have after
adding the unwinding. This obviously should return ret, not 0.
I did more extensive testing today, including faking error returns to
test unwinding to make sure I didn't miss anything else.
> +
> +err_release_action:
> + devm_release_action(dev, regulator_action_disable, r);
> +err_regulator_put:
> + devm_regulator_put(r);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_get_enable_read_voltage);
> +
On 4/29/24 16:40, David Lechner wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_read_voltage().
>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
On 4/29/24 16:40, David Lechner wrote:
> We can reduce boilerplate code and eliminate the driver remove()
> function by using devm_regulator_get_enable_read_voltage().
>
> A new external_vref flag is added since we no longer have the handle
> to the regulator to check if it is present.
>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
Nit below in case you resend, otherwise
Acked-by: Guenter Roeck <[email protected]>
> ---
>
> v2 changes:
> * rename to devm_regulator_get_enable_read_voltage()
> * use vref instead of err for return value
> * simplify last error check to return PTR_ERR directly
> ---
> drivers/hwmon/adc128d818.c | 57 ++++++++++++++--------------------------------
> 1 file changed, 17 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
> index 46e3c8c50765..2a35acb011eb 100644
> --- a/drivers/hwmon/adc128d818.c
> +++ b/drivers/hwmon/adc128d818.c
> @@ -58,7 +58,6 @@ static const u8 num_inputs[] = { 7, 8, 4, 6 };
>
> struct adc128_data {
> struct i2c_client *client;
> - struct regulator *regulator;
> int vref; /* Reference voltage in mV */
> struct mutex update_lock;
> u8 mode; /* Operation mode */
> @@ -389,7 +388,7 @@ static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
> return 0;
> }
>
> -static int adc128_init_client(struct adc128_data *data)
> +static int adc128_init_client(struct adc128_data *data, bool external_vref)
> {
> struct i2c_client *client = data->client;
> int err;
> @@ -408,7 +407,7 @@ static int adc128_init_client(struct adc128_data *data)
> regval |= data->mode << 1;
>
> /* If external vref is selected, configure the chip to use it */
> - if (data->regulator)
> + if (external_vref)
> regval |= 0x01;
>
> /* Write advanced configuration register */
> @@ -430,9 +429,9 @@ static int adc128_init_client(struct adc128_data *data)
> static int adc128_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> - struct regulator *regulator;
> struct device *hwmon_dev;
> struct adc128_data *data;
> + bool external_vref;
> int err, vref;
>
> data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
> @@ -440,20 +439,15 @@ static int adc128_probe(struct i2c_client *client)
> return -ENOMEM;
>
> /* vref is optional. If specified, is used as chip reference voltage */
> - regulator = devm_regulator_get_optional(dev, "vref");
> - if (!IS_ERR(regulator)) {
> - data->regulator = regulator;
> - err = regulator_enable(regulator);
> - if (err < 0)
> - return err;
> - vref = regulator_get_voltage(regulator);
> - if (vref < 0) {
> - err = vref;
> - goto error;
> - }
> - data->vref = DIV_ROUND_CLOSEST(vref, 1000);
> - } else {
> + vref = devm_regulator_get_enable_read_voltage(dev, "vref");
> + if (vref == -ENODEV) {
> + external_vref = false;
> data->vref = 2560; /* 2.56V, in mV */
> + } else if (vref < 0) {
> + return vref;
> + } else {
> + external_vref = true;
> + data->vref = DIV_ROUND_CLOSEST(vref, 1000);
> }
>
> /* Operation mode is optional. If unspecified, keep current mode */
> @@ -461,13 +455,12 @@ static int adc128_probe(struct i2c_client *client)
> if (data->mode > 3) {
> dev_err(dev, "invalid operation mode %d\n",
> data->mode);
> - err = -EINVAL;
> - goto error;
> + return -EINVAL;
> }
> } else {
> err = i2c_smbus_read_byte_data(client, ADC128_REG_CONFIG_ADV);
> if (err < 0)
> - goto error;
> + return err;
> data->mode = (err >> 1) & ADC128_REG_MASK;
> }
>
> @@ -476,31 +469,16 @@ static int adc128_probe(struct i2c_client *client)
> mutex_init(&data->update_lock);
>
> /* Initialize the chip */
> - err = adc128_init_client(data);
> + err = adc128_init_client(data, external_vref);
> if (err < 0)
> - goto error;
> + return err;
>
> hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> data, adc128_groups);
> - if (IS_ERR(hwmon_dev)) {
> - err = PTR_ERR(hwmon_dev);
> - goto error;
> - }
> + if (IS_ERR(hwmon_dev))
> + return PTR_ERR(hwmon_dev);
>
> return 0;
return PTR_ERR_OR_ZERO(hwmon_dev);
> -
> -error:
> - if (data->regulator)
> - regulator_disable(data->regulator);
> - return err;
> -}
> -
> -static void adc128_remove(struct i2c_client *client)
> -{
> - struct adc128_data *data = i2c_get_clientdata(client);
> -
> - if (data->regulator)
> - regulator_disable(data->regulator);
> }
>
> static const struct i2c_device_id adc128_id[] = {
> @@ -522,7 +500,6 @@ static struct i2c_driver adc128_driver = {
> .of_match_table = of_match_ptr(adc128_of_match),
> },
> .probe = adc128_probe,
> - .remove = adc128_remove,
> .id_table = adc128_id,
> .detect = adc128_detect,
> .address_list = normal_i2c,
>
On Mon, 29 Apr 2024 18:40:12 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_read_voltage().
>
> To maintain backwards compatibility in the case a DT does not provide
> an avdd-supply, we fall back to calling devm_regulator_get_enable()
> so that there is no change in user-facing behavior (e.g. dummy regulator
> will still be in sysfs).
>
> Also add an informative error message when we failed to get the voltage
> and knowing the voltage is required while we are touching this.
>
> Signed-off-by: David Lechner <[email protected]>
A somewhat fiddly case. I think you've done it the best way possible though.
Acked-by: Jonathan Cameron <[email protected]>
On Mon, 29 Apr 2024 18:40:08 -0500, David Lechner wrote:
> In the IIO subsystem, we noticed a pattern in many drivers where we need
> to get, enable and get the voltage of a supply that provides a reference
> voltage. In these cases, we only need the voltage and not a handle to
> the regulator. Another common pattern is for chips to have an internal
> reference voltage that is used when an external reference is not
> available. There are also a few drivers outside of IIO that do the same.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/7] regulator: devres: add API for reference voltage supplies
commit: b250c20b64290808aa4b5cc6d68819a7ee28237f
[2/7] hwmon: (adc128d818) Use devm_regulator_get_enable_read_voltage()
commit: cffb8d74bd4e9dd0653c7093c4a5164a72c52b1f
[3/7] hwmon: (da9052) Use devm_regulator_get_enable_read_voltage()
commit: d72fd5228c9f2136a3143daf5c7822140211883a
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Mon, May 6, 2024 at 9:59 AM Mark Brown <[email protected]> wrote:
>
> On Mon, 29 Apr 2024 18:40:08 -0500, David Lechner wrote:
> > In the IIO subsystem, we noticed a pattern in many drivers where we need
> > to get, enable and get the voltage of a supply that provides a reference
> > voltage. In these cases, we only need the voltage and not a handle to
> > the regulator. Another common pattern is for chips to have an internal
> > reference voltage that is used when an external reference is not
> > available. There are also a few drivers outside of IIO that do the same.
> >
> > [...]
>
> Applied to
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
>
> Thanks!
>
> [1/7] regulator: devres: add API for reference voltage supplies
> commit: b250c20b64290808aa4b5cc6d68819a7ee28237f
> [2/7] hwmon: (adc128d818) Use devm_regulator_get_enable_read_voltage()
> commit: cffb8d74bd4e9dd0653c7093c4a5164a72c52b1f
> [3/7] hwmon: (da9052) Use devm_regulator_get_enable_read_voltage()
> commit: d72fd5228c9f2136a3143daf5c7822140211883a
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
Hi Jonathan, if Mark doesn't pick up the iio patches here, I'll resend
them after the next kernel release.
On Mon, 29 Apr 2024 18:40:08 -0500, David Lechner wrote:
> In the IIO subsystem, we noticed a pattern in many drivers where we need
> to get, enable and get the voltage of a supply that provides a reference
> voltage. In these cases, we only need the voltage and not a handle to
> the regulator. Another common pattern is for chips to have an internal
> reference voltage that is used when an external reference is not
> available. There are also a few drivers outside of IIO that do the same.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[4/7] iio: addac: ad74115: Use devm_regulator_get_enable_read_voltage()
commit: 41b94bc6d96b9b046ef08114f057dcc6c52e28b6
[5/7] iio: frequency: admv1013: Use devm_regulator_get_enable_read_voltage()
commit: 2f4bb1fa758abf4f5ee5a70ea7c2b1b8c8f7625d
[6/7] staging: iio: impedance-analyzer: ad5933: Use devm_regulator_get_enable_read_voltage()
commit: 9fcf6ef3e10b9fc605d84802058c0f30517bbaa7
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Mon, Apr 29, 2024 at 06:40:15PM -0500, David Lechner wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_read_voltage().
>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
Acked-by: Dmitry Torokhov <[email protected]>
Mark, maybe you will pick up this one as well?
Thanks.
--
Dmitry