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 couple of new regulator consumer APIs 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 12 more users
of devm_regulator_get_enable_get_voltage() and 24 more users of
devm_regulator_get_optional_enable_get_voltage(). In total, this will
eliminate nearly 1000 lines of similar code.
---
David Lechner (7):
regulator: devres: add APIs for reference supplies
hwmon: (adc128d818) Use devm_regulator_get_optional_enable_get_voltage()
hwmon: (da9052) Use devm_regulator_get_enable_get_voltage()
iio: addac: ad74115: Use devm_regulator_get_enable_get_voltage()
iio: frequency: admv1013: Use devm_regulator_get_enable_get_voltage()
staging: iio: impedance-analyzer: admv1013: Use devm_regulator_get_enable_get_voltage()
Input: mpr121: Use devm_regulator_get_enable_get_voltage()
Documentation/driver-api/driver-model/devres.rst | 2 +
drivers/hwmon/adc128d818.c | 55 +++++-----------
drivers/hwmon/da9052-hwmon.c | 33 ++--------
drivers/iio/addac/ad74115.c | 28 +-------
drivers/iio/frequency/admv1013.c | 37 +++--------
drivers/input/keyboard/mpr121_touchkey.c | 45 +------------
drivers/regulator/devres.c | 83 ++++++++++++++++++++++++
drivers/staging/iio/impedance-analyzer/ad5933.c | 24 +------
include/linux/regulator/consumer.h | 14 ++++
9 files changed, 138 insertions(+), 183 deletions(-)
---
base-commit: c5b2db5859957150ac6ed305ab41a4a92ca40cfb
change-id: 20240326-regulator-get-enable-get-votlage-5dedf40ff338
We can reduce boilerplate code by using
devm_regulator_get_enable_get_voltage().
Signed-off-by: David Lechner <[email protected]>
---
drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
index 2bd7ae8100d7..70e7bc72e980 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;
};
@@ -414,32 +413,19 @@ 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);
+ err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
+ "tsiref");
+ if (err < 0)
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;
- }
/* convert from microvolt (DT) to millivolt (hwmon) */
- hwmon->tsiref_mv /= 1000;
+ hwmon->tsiref_mv = err / 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 +442,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 +458,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 +466,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
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 <[email protected]>
---
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),
+ "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)
{
--
2.43.2
We can reduce boilerplate code and eliminate the driver remove()
function by using devm_regulator_get_optional_enable_get_voltage().
A new external_vref flag is added since we no longer have the handle
to the regulator to check if it is present.
Signed-off-by: David Lechner <[email protected]>
---
drivers/hwmon/adc128d818.c | 55 ++++++++++++++--------------------------------
1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 46e3c8c50765..119e2841720f 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,30 +429,25 @@ 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;
- int err, vref;
+ bool external_vref;
+ int err;
data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
if (!data)
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 {
+ err = devm_regulator_get_optional_enable_get_voltage(dev, "vref");
+ if (err == -ENODEV) {
+ external_vref = false;
data->vref = 2560; /* 2.56V, in mV */
+ } else if (err < 0) {
+ return err;
+ } else {
+ external_vref = true;
+ data->vref = DIV_ROUND_CLOSEST(err, 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,18 @@ 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;
+ return err;
}
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 +502,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_get_voltage().
Signed-off-by: David Lechner <[email protected]>
---
drivers/iio/addac/ad74115.c | 28 +++-------------------------
1 file changed, 3 insertions(+), 25 deletions(-)
diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c
index e6bc5eb3788d..01073d7de6aa 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,14 +1671,6 @@ 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;
- }
-
st->din_threshold_mode = val;
ret = ad74115_apply_fw_prop(st, &ad74115_dac_bipolar_fw_prop, &val);
@@ -1788,11 +1779,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,19 +1841,11 @@ 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");
+ ret = devm_regulator_get_enable_get_voltage(dev, "avdd");
+ if (ret < 0)
return ret;
- }
- ret = devm_add_action_or_reset(dev, ad74115_regulator_disable, st->avdd);
- if (ret)
- return ret;
+ st->avdd_mv = ret / 1000;
ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
regulator_names);
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_get_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.
Signed-off-by: David Lechner <[email protected]>
---
drivers/iio/frequency/admv1013.c | 37 ++++++++-----------------------------
1 file changed, 8 insertions(+), 29 deletions(-)
diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
index 92923074f930..b0aa3cc27ea9 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,11 @@ 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");
+ ret = devm_regulator_get_enable_get_voltage(&spi->dev, "vcm");
+ if (ret < 0)
return ret;
- }
- 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 +599,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
We can reduce boilerplate code by using
devm_regulator_get_enable_get_voltage().
Signed-off-by: David Lechner <[email protected]>
---
drivers/staging/iio/impedance-analyzer/ad5933.c | 24 +-----------------------
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 9149d41fe65b..e4942833b793 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,21 +677,7 @@ 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_get_voltage(&client->dev, "vdd");
if (ret < 0)
return ret;
--
2.43.2
We can reduce boilerplate code by using
devm_regulator_get_enable_get_voltage().
Signed-off-by: David Lechner <[email protected]>
---
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..c59e7451f3cd 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_get_voltage(dev, "vdd");
+ if (vdd_uv < 0)
+ return vdd_uv;
mpr121 = devm_kzalloc(dev, sizeof(*mpr121), GFP_KERNEL);
if (!mpr121)
--
2.43.2
On Wed, 27 Mar 2024 18:18:50 -0500
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 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 <[email protected]>
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 Wed, 27 Mar 2024 18:18:55 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
Wrong device in patch title. I thought it odd we had a driver for same part
number in staging and out of it!.
Otherwise LGTM
Not sure how we'll merge this set assuming everyone agrees it's a good idea.
Immutable branch probably. On off chance we don't do that, with the patch title
fixed.
Reviewed-by: Jonathan Cameron <[email protected]>
Jonathan
>
> Signed-off-by: David Lechner <[email protected]>
> ---
> drivers/staging/iio/impedance-analyzer/ad5933.c | 24 +-----------------------
> 1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 9149d41fe65b..e4942833b793 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,21 +677,7 @@ 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_get_voltage(&client->dev, "vdd");
> if (ret < 0)
> return ret;
>
>
On Wed, 27 Mar 2024 18:18:54 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_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.
>
> Signed-off-by: David Lechner <[email protected]>
LGTM - on basis this might not go through my tree
Reviewed-by: Jonathan Cameron <[email protected]>
On Wed, 27 Mar 2024 18:18:53 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
There is a change in behaviour in this one. I'd like some
explanation in the patch title for why it is always safe to get
the voltage of avdd_mv when previously it wasn't.
There seems to me to be a corner case where a DTS is not providing
the entry because it's always powered on so we get a stub regulator
but that doesn't matter because we aren't in DIN_THRESHOLD_MOD_AVDD.
After this change that dts is broken as now we read the voltage
whatever.
You could use the optional form and then fail the probe if
in a mode where the value will be used?
>
> Signed-off-by: David Lechner <[email protected]>
> ---
> drivers/iio/addac/ad74115.c | 28 +++-------------------------
> 1 file changed, 3 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c
> index e6bc5eb3788d..01073d7de6aa 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,14 +1671,6 @@ 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;
> - }
> -
> st->din_threshold_mode = val;
>
> ret = ad74115_apply_fw_prop(st, &ad74115_dac_bipolar_fw_prop, &val);
> @@ -1788,11 +1779,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,19 +1841,11 @@ 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");
> + ret = devm_regulator_get_enable_get_voltage(dev, "avdd");
> + if (ret < 0)
> return ret;
> - }
>
> - ret = devm_add_action_or_reset(dev, ad74115_regulator_disable, st->avdd);
> - if (ret)
> - return ret;
> + st->avdd_mv = ret / 1000;
>
> ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> regulator_names);
>
On Wed, 27 Mar 2024 18:18:51 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code and eliminate the driver remove()
> function by using devm_regulator_get_optional_enable_get_voltage().
>
> A new external_vref flag is added since we no longer have the handle
> to the regulator to check if it is present.
>
> Signed-off-by: David Lechner <[email protected]>
One trivial thing.
With that tidied up...
Reviewed-by: Jonathan Cameron <[email protected]>
> 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;
> + return err;
return PTR_ERR()
> }
>
> return 0;
> -
> -error:
> - if (data->regulator)
> - regulator_disable(data->regulator);
> - return err;
> -}
On Wed, 27 Mar 2024 18:18:52 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
>
> Signed-off-by: David Lechner <[email protected]>
A few comments inline, but nothing substantial.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
> 1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> index 2bd7ae8100d7..70e7bc72e980 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;
> };
>
> @@ -414,32 +413,19 @@ 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);
> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
> + "tsiref");
> + if (err < 0)
> 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;
> - }
>
> /* convert from microvolt (DT) to millivolt (hwmon) */
> - hwmon->tsiref_mv /= 1000;
> + hwmon->tsiref_mv = err / 1000;
>
Using a variable called err for a good value is a bit ugly but fair enough if that
is precedence in this driver.
> }
> @@ -483,10 +466,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);
Superficially looks like devm_da9052_request_irq could be added that
uses devm_request_threaded_irq() to allow dropping this remaining handling.
Thanks,
Jonathan
> - regulator_disable(hwmon->tsiref);
> - }
> }
>
> static struct platform_driver da9052_hwmon_driver = {
>
On Wed, 27 Mar 2024 18:18:56 -0500
David Lechner <[email protected]> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
>
> Signed-off-by: David Lechner <[email protected]>
LGTM though you may want to bring an error message back if you drop the
prints from the regulator functions.
Reviewed-by: Jonathan Cameron <[email protected]>
On 3/28/24 07:20, Jonathan Cameron wrote:
> On Wed, 27 Mar 2024 18:18:52 -0500
> David Lechner <[email protected]> wrote:
>
>> We can reduce boilerplate code by using
>> devm_regulator_get_enable_get_voltage().
>>
>> Signed-off-by: David Lechner <[email protected]>
>
> A few comments inline, but nothing substantial.
>
> Reviewed-by: Jonathan Cameron <[email protected]>
>> ---
>> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
>> 1 file changed, 7 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
>> index 2bd7ae8100d7..70e7bc72e980 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;
>> };
>>
>> @@ -414,32 +413,19 @@ 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);
>> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
>> + "tsiref");
>> + if (err < 0)
>> 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;
>> - }
>>
>> /* convert from microvolt (DT) to millivolt (hwmon) */
>> - hwmon->tsiref_mv /= 1000;
>> + hwmon->tsiref_mv = err / 1000;
>>
>
> Using a variable called err for a good value is a bit ugly but fair enough if that
> is precedence in this driver.
>
It isn't. The existing code assigns the return value from regulator_get_voltage()
to hwmon->tsiref_mv and then evaluates it. I would not oppose introducing a variable
such as tsiref_uv, but not the misuse of 'err'. I am not going to accept the code
as suggested. It is bad style, and it would invite others to use it as precedent
when trying to introduce similar code.
>> }
>> @@ -483,10 +466,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);
> Superficially looks like devm_da9052_request_irq could be added that
> uses devm_request_threaded_irq() to allow dropping this remaining handling.
>
That should be a separate series of patches. A local solution might be
to use devm_add_action_or_reset(), but that should also be a separate patch.
Thanks,
Guenter
On Thu, Mar 28, 2024 at 8:48 AM Jonathan Cameron <[email protected]> wrote:
>
> On Wed, 27 Mar 2024 18:18:50 -0500
> 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 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 <[email protected]>
>
> 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 Thu, 28 Mar 2024 08:20:00 -0700
Guenter Roeck <[email protected]> wrote:
> On 3/28/24 07:20, Jonathan Cameron wrote:
> > On Wed, 27 Mar 2024 18:18:52 -0500
> > David Lechner <[email protected]> wrote:
> >
> >> We can reduce boilerplate code by using
> >> devm_regulator_get_enable_get_voltage().
> >>
> >> Signed-off-by: David Lechner <[email protected]>
> >
> > A few comments inline, but nothing substantial.
> >
> > Reviewed-by: Jonathan Cameron <[email protected]>
> >> ---
> >> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
> >> 1 file changed, 7 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> >> index 2bd7ae8100d7..70e7bc72e980 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;
> >> };
> >>
> >> @@ -414,32 +413,19 @@ 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);
> >> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
> >> + "tsiref");
> >> + if (err < 0)
> >> 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;
> >> - }
> >>
> >> /* convert from microvolt (DT) to millivolt (hwmon) */
> >> - hwmon->tsiref_mv /= 1000;
> >> + hwmon->tsiref_mv = err / 1000;
> >>
> >
> > Using a variable called err for a good value is a bit ugly but fair enough if that
> > is precedence in this driver.
> >
>
> It isn't. The existing code assigns the return value from regulator_get_voltage()
> to hwmon->tsiref_mv and then evaluates it. I would not oppose introducing a variable
> such as tsiref_uv, but not the misuse of 'err'. I am not going to accept the code
> as suggested. It is bad style, and it would invite others to use it as precedent
> when trying to introduce similar code.
I was too lazy to look and see if there were existing cases :) Local variable indeed
the right way to go.
>
> >> }
> >> @@ -483,10 +466,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);
> > Superficially looks like devm_da9052_request_irq could be added that
> > uses devm_request_threaded_irq() to allow dropping this remaining handling.
> >
>
> That should be a separate series of patches. A local solution might be
> to use devm_add_action_or_reset(), but that should also be a separate patch.
Agreed. Just a passing comment whilst the code was in front of me.
Jonathan
>
> Thanks,
> Guenter
>
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.
--
Dmitry
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.
--
Dmitry
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 <[email protected]> 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