[PATCH v3 3/5] iio: amplifiers: ada4250: use devm_regulator_get_enable_read_voltage()

David Lechner posted 5 patches 4 months ago
[PATCH v3 3/5] iio: amplifiers: ada4250: use devm_regulator_get_enable_read_voltage()
Posted by David Lechner 4 months ago
Use devm_regulator_get_enable_read_voltage() to simplify the code.

Replace 1000000 with MICRO while we are touching this for better
readability.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

Note for reviewers: if you are tempted to comment on the new variable
not being grouped with offset_uv, see the next patch in this series.
---
 drivers/iio/amplifiers/ada4250.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index 1bd7c0c3c695b3872b8c389fb4ae89bf5d24f51c..c367c53a075b26327a221e0c3a9dc8e788891f32 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -14,6 +14,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 /* ADA4250 Register Map */
 #define ADA4250_REG_GAIN_MUX        0x00
@@ -55,9 +56,9 @@ enum ada4250_current_bias {
 struct ada4250_state {
 	struct spi_device	*spi;
 	struct regmap		*regmap;
-	struct regulator	*reg;
 	/* Protect against concurrent accesses to the device and data content */
 	struct mutex		lock;
+	int			avdd_uv;
 	u8			bias;
 	u8			gain;
 	int			offset_uv;
@@ -91,8 +92,7 @@ static int ada4250_set_offset_uv(struct iio_dev *indio_dev,
 	if (st->bias == 0 || st->bias == 3)
 		return -EINVAL;
 
-	voltage_v = regulator_get_voltage(st->reg);
-	voltage_v = DIV_ROUND_CLOSEST(voltage_v, 1000000);
+	voltage_v = DIV_ROUND_CLOSEST(st->avdd_uv, MICRO);
 
 	if (st->bias == ADA4250_BIAS_AVDD)
 		x[0] = voltage_v;
@@ -292,11 +292,6 @@ static const struct iio_chan_spec ada4250_channels[] = {
 	}
 };
 
-static void ada4250_reg_disable(void *data)
-{
-	regulator_disable(data);
-}
-
 static int ada4250_init(struct ada4250_state *st)
 {
 	struct device *dev = &st->spi->dev;
@@ -305,21 +300,11 @@ static int ada4250_init(struct ada4250_state *st)
 
 	st->refbuf_en = device_property_read_bool(dev, "adi,refbuf-enable");
 
-	st->reg = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(st->reg))
-		return dev_err_probe(dev, PTR_ERR(st->reg),
+	st->avdd_uv = devm_regulator_get_enable_read_voltage(dev, "avdd");
+	if (st->avdd_uv < 0)
+		return dev_err_probe(dev, st->avdd_uv,
 				     "failed to get the AVDD voltage\n");
 
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(dev, "Failed to enable specified AVDD supply\n");
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(dev, ada4250_reg_disable, st->reg);
-	if (ret)
-		return ret;
-
 	ret = regmap_write(st->regmap, ADA4250_REG_RESET,
 			   FIELD_PREP(ADA4250_RESET_MSK, 1));
 	if (ret)

-- 
2.43.0
Re: [PATCH v3 3/5] iio: amplifiers: ada4250: use devm_regulator_get_enable_read_voltage()
Posted by Andy Shevchenko 4 months ago
On Wed, Jun 11, 2025 at 04:33:03PM -0500, David Lechner wrote:
> Use devm_regulator_get_enable_read_voltage() to simplify the code.
> 
> Replace 1000000 with MICRO while we are touching this for better
> readability.

...

> -	voltage_v = DIV_ROUND_CLOSEST(voltage_v, 1000000);
> +	voltage_v = DIV_ROUND_CLOSEST(st->avdd_uv, MICRO);

Side note. I'm always worry about CLOSEST choice when it's related to voltage
or current. Imagine the table which gives you 5, 3.3, and 1.2. If it happens to
be closest to higher value, it may damage HW forever.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 3/5] iio: amplifiers: ada4250: use devm_regulator_get_enable_read_voltage()
Posted by David Lechner 4 months ago
On 6/12/25 8:03 AM, Andy Shevchenko wrote:
> On Wed, Jun 11, 2025 at 04:33:03PM -0500, David Lechner wrote:
>> Use devm_regulator_get_enable_read_voltage() to simplify the code.
>>
>> Replace 1000000 with MICRO while we are touching this for better
>> readability.
> 
> ...
> 
>> -	voltage_v = DIV_ROUND_CLOSEST(voltage_v, 1000000);
>> +	voltage_v = DIV_ROUND_CLOSEST(st->avdd_uv, MICRO);
> 
> Side note. I'm always worry about CLOSEST choice when it's related to voltage
> or current. Imagine the table which gives you 5, 3.3, and 1.2. If it happens to
> be closest to higher value, it may damage HW forever.
> 

The rounding to volts seems strange to me too, but I could not find a public
datasheet for this part, so I wasn't able to determine what the "right" thing
to do is. It sounds like this is just some sort of small gain/offset calibration,
so I don't think there is any serious problem here, like it could damage the part.
So I will have to leave it to someone with the part and the datasheet to figure
out if there is actually a problem here.