linux-kernel - Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250201111813.6b8cbf5b@jic23-huawei>
Date: Sat, 1 Feb 2025 11:18:13 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Sam Winchenbach <swinchenbach@...a.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "antoniu.miclaus@...log.com" <antoniu.miclaus@...log.com>,
 "lars@...afoo.de" <lars@...afoo.de>, "Michael.Hennerich@...log.com"
 <Michael.Hennerich@...log.com>
Subject: Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO

On Sat, 25 Jan 2025 14:54:44 +0000
Sam Winchenbach <swinchenbach@...a.org> wrote:

> When a weak pull-up is present on the SDO line, regmap_update_bits fails
> to write both the SOFTRESET and SDOACTIVE bits because it incorrectly
> reads them as already set.

I can see this as a valid micro optimization but I'm struggling a bit
on how you can use the device if the pull up is weak enough that
you can't read data back from it. Does the reset in some way
solve that?

Having asked for the fixes tag, I'm less sure on whether this is a fix.

Antoniu, I'd also like your input on this one!

> 
> Since the soft reset disables the SDO line, performing a
> read-modify-write operation on ADI_SPI_CONFIG_A to enable the SDO line
> doesn't make sense. This change directly writes to the register instead
> of using regmap_update_bits.
> 
> Fixes: f34fe888ad05 ("iio:filter:admv8818: add support for ADMV8818")
> 

No blank line here.  Fixes is part of the tags block that various scripts
scan.

> Signed-off-by: Sam Winchenbach <swinchenbach@...a.org>
> ---
>  drivers/iio/filter/admv8818.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> index 195e58bc4..9cd1eee84 100644
> --- a/drivers/iio/filter/admv8818.c
> +++ b/drivers/iio/filter/admv8818.c
> @@ -577,21 +577,15 @@ static int admv8818_init(struct admv8818_state *st)
>  	struct spi_device *spi = st->spi;
>  	unsigned int chip_id;
>  
> -	ret = regmap_update_bits(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
> -				 ADMV8818_SOFTRESET_N_MSK |
> -				 ADMV8818_SOFTRESET_MSK,
> -				 FIELD_PREP(ADMV8818_SOFTRESET_N_MSK, 1) |
> -				 FIELD_PREP(ADMV8818_SOFTRESET_MSK, 1));
> +	ret = regmap_write(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
> +			   ADMV8818_SOFTRESET_N_MSK | ADMV8818_SOFTRESET_MSK);
>  	if (ret) {
>  		dev_err(&spi->dev, "ADMV8818 Soft Reset failed.\n");
>  		return ret;
>  	}
>  
> -	ret = regmap_update_bits(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
> -				 ADMV8818_SDOACTIVE_N_MSK |
> -				 ADMV8818_SDOACTIVE_MSK,
> -				 FIELD_PREP(ADMV8818_SDOACTIVE_N_MSK, 1) |
> -				 FIELD_PREP(ADMV8818_SDOACTIVE_MSK, 1));
> +	ret = regmap_write(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
> +			   ADMV8818_SDOACTIVE_N_MSK | ADMV8818_SDOACTIVE_MSK);
>  	if (ret) {
>  		dev_err(&spi->dev, "ADMV8818 SDO Enable failed.\n");
>  		return ret;