linux-kernel - Re: [PATCH v2] iio: amplifiers: ada4250: clean up ada4250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250421122409.79f5580c@jic23-huawei>
Date: Mon, 21 Apr 2025 12:24:09 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: amplifiers: ada4250: clean up ada4250_init()

On Fri, 18 Apr 2025 15:37:54 -0500
David Lechner <dlechner@...libre.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@...libre.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,