[PATCH v2] iio: amplifiers: ada4250: clean up ada4250_init()

[PATCH v2] iio: amplifiers: ada4250: clean up ada4250_init()

[David Lechner]

There are a few opportunities to simplify the code in ada4250_init():
* Replace local spi variable with dev since spi is not used directly.
* Drop the data variable and use chip_id directly with the regmap bulk
  read (__aligned() and initialization of the data variable were
  unnecessary).
* Don't use get_unaligned_le16() when not needed.
* Use dev_err_probe() instead of dev_err() and return.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
In v1, I though we had a bug, but Andy set me straight. Still, since we
were already looking at this, there is some room for improvement, so I
changed this to a cleanup patch instead.

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
---
 drivers/iio/amplifiers/ada4250.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
index 74f8429d652b17b4d1f38366e23ce6a2b3e9b218..13906e4b4842095717566781ad00cd58f3934510 100644
--- a/drivers/iio/amplifiers/ada4250.c
+++ b/drivers/iio/amplifiers/ada4250.c
@@ -13,8 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
-
-#include <linux/unaligned.h>
+#include <linux/types.h>
 
 /* ADA4250 Register Map */
 #define ADA4250_REG_GAIN_MUX        0x00
@@ -299,25 +298,23 @@ 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;
-	u8 data[2] __aligned(8) = {};
-	struct spi_device *spi = st->spi;
+	__le16 chip_id;
 
-	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");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable specified AVDD supply\n");
 
-	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;
 
@@ -326,16 +323,13 @@ static int ada4250_init(struct ada4250_state *st)
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, data, 2);
+	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, &chip_id,
+			       sizeof(chip_id));
 	if (ret)
 		return ret;
 
-	chip_id = get_unaligned_le16(data);
-
-	if (chip_id != ADA4250_CHIP_ID) {
-		dev_err(&spi->dev, "Invalid chip ID.\n");
-		return -EINVAL;
-	}
+	if (le16_to_cpu(chip_id) != ADA4250_CHIP_ID)
+		return dev_err_probe(dev, -EINVAL, "Invalid chip ID.\n");
 
 	return regmap_write(st->regmap, ADA4250_REG_REFBUF_EN,
 			    FIELD_PREP(ADA4250_REFBUF_MSK, st->refbuf_en));

---
base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
change-id: 20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-93ebb1344295

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

Re: [PATCH v2] iio: amplifiers: ada4250: clean up ada4250_init()

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 03:37:54PM -0500, David Lechner wrote:
> There are a few opportunities to simplify the code in ada4250_init():
> * Replace local spi variable with dev since spi is not used directly.
> * Drop the data variable and use chip_id directly with the regmap bulk
>   read (__aligned() and initialization of the data variable were
>   unnecessary).
> * Don't use get_unaligned_le16() when not needed.
> * Use dev_err_probe() instead of dev_err() and return.

Sorry, but while a good one, this needs a split, at least to three:
1) __le16 type and linked changes;
2) dev_err_probe();
3) the rest...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: amplifiers: ada4250: clean up ada4250_init()

[Jonathan Cameron]

On Fri, 18 Apr 2025 15:37:54 -0500
David Lechner <dlechner@baylibre.com> wrote:

> There are a few opportunities to simplify the code in ada4250_init():
> * Replace local spi variable with dev since spi is not used directly.
> * Drop the data variable and use chip_id directly with the regmap bulk
>   read (__aligned() and initialization of the data variable were
>   unnecessary).
> * Don't use get_unaligned_le16() when not needed.
> * Use dev_err_probe() instead of dev_err() and return.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> In v1, I though we had a bug, but Andy set me straight. Still, since we
> were already looking at this, there is some room for improvement, so I
> changed this to a cleanup patch instead.
> 
> 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
As Andy suggested this wants breaking up.

One additional requested change inline.

> ---
>  drivers/iio/amplifiers/ada4250.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/ada4250.c b/drivers/iio/amplifiers/ada4250.c
> index 74f8429d652b17b4d1f38366e23ce6a2b3e9b218..13906e4b4842095717566781ad00cd58f3934510 100644
> --- a/drivers/iio/amplifiers/ada4250.c
> +++ b/drivers/iio/amplifiers/ada4250.c
> @@ -13,8 +13,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> -
> -#include <linux/unaligned.h>
> +#include <linux/types.h>
>  
>  /* ADA4250 Register Map */
>  #define ADA4250_REG_GAIN_MUX        0x00
> @@ -299,25 +298,23 @@ 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;
> -	u8 data[2] __aligned(8) = {};
> -	struct spi_device *spi = st->spi;
> +	__le16 chip_id;
>  
> -	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");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable specified AVDD supply\n");
>  
> -	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;
>  
> @@ -326,16 +323,13 @@ static int ada4250_init(struct ada4250_state *st)
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, data, 2);
> +	ret = regmap_bulk_read(st->regmap, ADA4250_REG_CHIP_ID, &chip_id,
> +			       sizeof(chip_id));
>  	if (ret)
>  		return ret;
>  
> -	chip_id = get_unaligned_le16(data);
> -
> -	if (chip_id != ADA4250_CHIP_ID) {
> -		dev_err(&spi->dev, "Invalid chip ID.\n");
> -		return -EINVAL;
> -	}
> +	if (le16_to_cpu(chip_id) != ADA4250_CHIP_ID)

Given you are working on this driver, these days we treat an ID match failure
as informational only.  The reason being to allow fallback compatibles to
be used in DT so that an old kernel can in theory support a new compatible
chip that shows up sometime in the future (but has a different chip ID).

So please add a final patch to the series that relaxes this to a dev_info()
print and carry on anyway.

I've considered just changing all rejected chip IDs, but it seems too noisy
unless people are touching the code for other reasons.  Hence I've not done it.
There is also a non zero chance that someone has a broken firmware and
odd error reports will ensue.

Jonathan

> +		return dev_err_probe(dev, -EINVAL, "Invalid chip ID.\n");
>  
>  	return regmap_write(st->regmap, ADA4250_REG_REFBUF_EN,
>  			    FIELD_PREP(ADA4250_REFBUF_MSK, st->refbuf_en));
> 
> ---
> base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
> change-id: 20250418-iio-amplifiers-ada4250-simplify-data-buffer-in-init-93ebb1344295
> 
> Best regards,