[PATCH v3 0/5] iio: amplifiers: ada4250: various cleanups

[PATCH v3 0/5] iio: amplifiers: ada4250: various cleanups

[David Lechner]

While investigating some potential bugs, we noticed quite a few
opportunities for small improvements in the ada4250 driver.

---
Changes in v3:
- Split into multiple patches.
- Added even more cleanups.
- Link to v2: https://lore.kernel.org/r/20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-v2-1-1bf9b033aaf5@baylibre.com

Changes in v2:
- Totally new patch.
- Link to v1: https://lore.kernel.org/r/20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-v1-1-7e7bd6dad423@baylibre.com

---
David Lechner (5):
      iio: amplifiers: ada4250: used dev local variable
      iio: amplifiers: ada4250: don't fail on bad chip ID
      iio: amplifiers: ada4250: use devm_regulator_get_enable_read_voltage()
      iio: amplifiers: ada4250: move offset_uv in struct
      iio: amplifiers: ada4250: use dev_err_probe()

 drivers/iio/amplifiers/ada4250.c | 45 ++++++++++++----------------------------
 1 file changed, 13 insertions(+), 32 deletions(-)
---
base-commit: 4c6073fec2fee4827fa0dd8a4ab4e6f7bbc05ee6
change-id: 20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-93ebb1344295

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v3 1/5] iio: amplifiers: ada4250: used dev local variable

[David Lechner]

Replace local spi variable with dev in ada4250_init() since spi is not
used directly.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/ada4250.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index f81438460aa51ce30f8f605c60ee5be5c8c251d3..397c1e1545cfccad9b0ff58b133796d415130064 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -299,24 +299,24 @@ static void ada4250_reg_disable(void *data)
 
 static int ada4250_init(struct ada4250_state *st)
 {
+	struct device *dev = &st->spi->dev;
 	int ret;
 	u16 chip_id;
-	struct spi_device *spi = st->spi;
 
-	st->refbuf_en = device_property_read_bool(&spi->dev, "adi,refbuf-enable");
+	st->refbuf_en = device_property_read_bool(dev, "adi,refbuf-enable");
 
-	st->reg = devm_regulator_get(&spi->dev, "avdd");
+	st->reg = devm_regulator_get(dev, "avdd");
 	if (IS_ERR(st->reg))
-		return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
+		return dev_err_probe(dev, PTR_ERR(st->reg),
 				     "failed to get the AVDD voltage\n");
 
 	ret = regulator_enable(st->reg);
 	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVDD supply\n");
+		dev_err(dev, "Failed to enable specified AVDD supply\n");
 		return ret;
 	}
 
-	ret = devm_add_action_or_reset(&spi->dev, ada4250_reg_disable, st->reg);
+	ret = devm_add_action_or_reset(dev, ada4250_reg_disable, st->reg);
 	if (ret)
 		return ret;
 
@@ -333,7 +333,7 @@ static int ada4250_init(struct ada4250_state *st)
 	chip_id = le16_to_cpu(st->reg_val_16);
 
 	if (chip_id != ADA4250_CHIP_ID) {
-		dev_err(&spi->dev, "Invalid chip ID.\n");
+		dev_err(dev, "Invalid chip ID.\n");
 		return -EINVAL;
 	}
 

-- 
2.43.0

[PATCH v3 2/5] iio: amplifiers: ada4250: don't fail on bad chip ID

[David Lechner]

Only print an information message instead of error message and failing
to probe the device if the chip ID is not recognized. Experience shows
that this can be fragile and some devices may not return the expected
chip ID even though the driver is still able to work with them.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Closes: https://lore.kernel.org/linux-iio/20250421122409.79f5580c@jic23-huawei/
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/ada4250.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index 397c1e1545cfccad9b0ff58b133796d415130064..1bd7c0c3c695b3872b8c389fb4ae89bf5d24f51c 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -332,10 +332,8 @@ static int ada4250_init(struct ada4250_state *st)
 
 	chip_id = le16_to_cpu(st->reg_val_16);
 
-	if (chip_id != ADA4250_CHIP_ID) {
-		dev_err(dev, "Invalid chip ID.\n");
-		return -EINVAL;
-	}
+	if (chip_id != ADA4250_CHIP_ID)
+		dev_info(dev, "Invalid chip ID: 0x%02X.\n", chip_id);
 
 	return regmap_write(st->regmap, ADA4250_REG_REFBUF_EN,
 			    FIELD_PREP(ADA4250_REFBUF_MSK, st->refbuf_en));

-- 
2.43.0

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

[David Lechner]

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()

[Andy Shevchenko]

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()

[David Lechner]

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.

[PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[David Lechner]

Move offset_uv in struct ada4250_state. This keeps things logically
grouped and reduces holes in the struct.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/ada4250.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index c367c53a075b26327a221e0c3a9dc8e788891f32..d20ca410c506226fce7f172632d46b2ebb140a12 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -59,9 +59,9 @@ struct ada4250_state {
 	/* Protect against concurrent accesses to the device and data content */
 	struct mutex		lock;
 	int			avdd_uv;
+	int			offset_uv;
 	u8			bias;
 	u8			gain;
-	int			offset_uv;
 	bool			refbuf_en;
 	__le16			reg_val_16 __aligned(IIO_DMA_MINALIGN);
 };

-- 
2.43.0

Re: [PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:33:04PM -0500, David Lechner wrote:
> Move offset_uv in struct ada4250_state. This keeps things logically
> grouped and reduces holes in the struct.

Can the (smallest part of) the diff for `pahole` runs be added here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[David Lechner]

On 6/12/25 8:04 AM, Andy Shevchenko wrote:
> On Wed, Jun 11, 2025 at 04:33:04PM -0500, David Lechner wrote:
>> Move offset_uv in struct ada4250_state. This keeps things logically
>> grouped and reduces holes in the struct.
> 
> Can the (smallest part of) the diff for `pahole` runs be added here?
> 

Well, I didn't use pahole. I could just tell by looking at it. There
was int followed by two u8 followed by int, so we know there was a 2
byte hole before the last int.

Re: [PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[Andy Shevchenko]

On Thu, Jun 12, 2025 at 6:31 PM David Lechner <dlechner@baylibre.com> wrote:
> On 6/12/25 8:04 AM, Andy Shevchenko wrote:
> > On Wed, Jun 11, 2025 at 04:33:04PM -0500, David Lechner wrote:
> >> Move offset_uv in struct ada4250_state. This keeps things logically
> >> grouped and reduces holes in the struct.
> >
> > Can the (smallest part of) the diff for `pahole` runs be added here?
>
> Well, I didn't use pahole. I could just tell by looking at it. There
> was int followed by two u8 followed by int, so we know there was a 2
> byte hole before the last int.

Yes, but since we have a tool for such cases, why not use it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[David Lechner]

On 6/12/25 1:34 PM, Andy Shevchenko wrote:
> On Thu, Jun 12, 2025 at 6:31 PM David Lechner <dlechner@baylibre.com> wrote:
>> On 6/12/25 8:04 AM, Andy Shevchenko wrote:
>>> On Wed, Jun 11, 2025 at 04:33:04PM -0500, David Lechner wrote:
>>>> Move offset_uv in struct ada4250_state. This keeps things logically
>>>> grouped and reduces holes in the struct.
>>>
>>> Can the (smallest part of) the diff for `pahole` runs be added here?
>>
>> Well, I didn't use pahole. I could just tell by looking at it. There
>> was int followed by two u8 followed by int, so we know there was a 2
>> byte hole before the last int.
> 
> Yes, but since we have a tool for such cases, why not use it?
> 

In cases like this, I don't think the we are getting much value
from it compared to the amount of time it would take me to do it.

Re: [PATCH v3 4/5] iio: amplifiers: ada4250: move offset_uv in struct

[Andy Shevchenko]

On Thu, Jun 12, 2025 at 01:45:16PM -0500, David Lechner wrote:
> On 6/12/25 1:34 PM, Andy Shevchenko wrote:
> > On Thu, Jun 12, 2025 at 6:31 PM David Lechner <dlechner@baylibre.com> wrote:
> >> On 6/12/25 8:04 AM, Andy Shevchenko wrote:
> >>> On Wed, Jun 11, 2025 at 04:33:04PM -0500, David Lechner wrote:
> >>>> Move offset_uv in struct ada4250_state. This keeps things logically
> >>>> grouped and reduces holes in the struct.
> >>>
> >>> Can the (smallest part of) the diff for `pahole` runs be added here?
> >>
> >> Well, I didn't use pahole. I could just tell by looking at it. There
> >> was int followed by two u8 followed by int, so we know there was a 2
> >> byte hole before the last int.
> > 
> > Yes, but since we have a tool for such cases, why not use it?
> 
> In cases like this, I don't think the we are getting much value
> from it compared to the amount of time it would take me to do it.

It helps reviewers to see the difference without checking themselves.
Also, it's not a waste of 15 minutes (that's my experience with pahole
when I started it first time), it's an investment to the future similar
changes by you or others. Believe me, as a reviewer you will find this tool
very helpful.

So, please try it once and then you will know how to do that again much
quicker.

-- 
With Best Regards,
Andy Shevchenko

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

[David Lechner]

Use dev_err_probe() when returning an error in the probe function.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/amplifiers/ada4250.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index d20ca410c506226fce7f172632d46b2ebb140a12..40f396ea906950ab79bf72cdb162794e95f76094 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -351,10 +351,8 @@ static int ada4250_probe(struct spi_device *spi)
 	mutex_init(&st->lock);
 
 	ret = ada4250_init(st);
-	if (ret) {
-		dev_err(&spi->dev, "ADA4250 init failed\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "ADA4250 init failed\n");
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }

-- 
2.43.0

Re: [PATCH v3 5/5] iio: amplifiers: ada4250: use dev_err_probe()

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:33:05PM -0500, David Lechner wrote:
> Use dev_err_probe() when returning an error in the probe function.

...

>  	mutex_init(&st->lock);

Side note. Switch to devm?

...

>  	ret = ada4250_init(st);

Is this used only in ->probe() stage? If so, please also move to
dev_err_probe() there.

> -	if (ret) {
> -		dev_err(&spi->dev, "ADA4250 init failed\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "ADA4250 init failed\n");
>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/5] iio: amplifiers: ada4250: use dev_err_probe()

[David Lechner]

On 6/12/25 8:05 AM, Andy Shevchenko wrote:
> On Wed, Jun 11, 2025 at 04:33:05PM -0500, David Lechner wrote:
>> Use dev_err_probe() when returning an error in the probe function.
> 
> ...
> 
>>  	mutex_init(&st->lock);
> 
> Side note. Switch to devm?

Yup, missed that.

> 
> ...
> 
>>  	ret = ada4250_init(st);
> 
> Is this used only in ->probe() stage? If so, please also move to
> dev_err_probe() there.

After all of the other changes, there was nothing left to convert
to dev_err_probe() there.

> 
>> -	if (ret) {
>> -		dev_err(&spi->dev, "ADA4250 init failed\n");
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		return dev_err_probe(&spi->dev, ret, "ADA4250 init failed\n");
>>  
>>  	return devm_iio_device_register(&spi->dev, indio_dev);
> 

Re: [PATCH v3 0/5] iio: amplifiers: ada4250: various cleanups

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:33:00PM -0500, David Lechner wrote:
> While investigating some potential bugs, we noticed quite a few
> opportunities for small improvements in the ada4250 driver.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

for non-commented patches. Otherwise, feel free to add it there
if my suggestion / assumptions are wrong.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/5] iio: amplifiers: ada4250: various cleanups

[Jonathan Cameron]

On Thu, 12 Jun 2025 16:07:00 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Jun 11, 2025 at 04:33:00PM -0500, David Lechner wrote:
> > While investigating some potential bugs, we noticed quite a few
> > opportunities for small improvements in the ada4250 driver.  
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> for non-commented patches. Otherwise, feel free to add it there
> if my suggestion / assumptions are wrong.
> 

I think, with exception of request for pahole (which is a nice to have
to my eyes rather than a requirement) I think all addressed by David
so applied and tag picked up.

For the pahole thing only thing I can imagine it doing is showing
a less obvious additional move is useful as this one was about as
obvious as they come if IIO_DMA_MINALIGN isn't huge (in which case
it makes no difference as we have a big hole!)

Jonathan