* [PATCH v2] iio: filter: admv8818: Force initialization of SDO
@ 2025-01-25 14:54 Sam Winchenbach
2025-02-01 11:18 ` Jonathan Cameron
2025-02-03 13:34 ` [PATCH v3] " Sam Winchenbach
0 siblings, 2 replies; 8+ messages in thread
From: Sam Winchenbach @ 2025-01-25 14:54 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org
Cc: linux-iio@vger.kernel.org, antoniu.miclaus@analog.com,
lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org
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.
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")
Signed-off-by: Sam Winchenbach <swinchenbach@arka.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;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO
2025-01-25 14:54 [PATCH v2] iio: filter: admv8818: Force initialization of SDO Sam Winchenbach
@ 2025-02-01 11:18 ` Jonathan Cameron
2025-02-01 14:00 ` Sam Winchenbach
2025-02-03 12:14 ` Miclaus, Antoniu
2025-02-03 13:34 ` [PATCH v3] " Sam Winchenbach
1 sibling, 2 replies; 8+ messages in thread
From: Jonathan Cameron @ 2025-02-01 11:18 UTC (permalink / raw)
To: Sam Winchenbach
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
antoniu.miclaus@analog.com, lars@metafoo.de,
Michael.Hennerich@analog.com
On Sat, 25 Jan 2025 14:54:44 +0000
Sam Winchenbach <swinchenbach@arka.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@arka.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;
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO
2025-02-01 11:18 ` Jonathan Cameron
@ 2025-02-01 14:00 ` Sam Winchenbach
2025-02-02 15:15 ` Jonathan Cameron
2025-02-03 12:14 ` Miclaus, Antoniu
1 sibling, 1 reply; 8+ messages in thread
From: Sam Winchenbach @ 2025-02-01 14:00 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
antoniu.miclaus@analog.com, lars@metafoo.de,
Michael.Hennerich@analog.com
> > 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!
>
On power-up the device is configured without SDO enabled. This, alone, makes the read-modify-write during initialization incorrect - It is not possible to read from the device seeing it is not driving the output.
If the SPI bus, like in our situation, has a weak pull-up the situation is compounded. When the device initializes it reads backs all 1's as part of the read-modify-write sequence, falsely determines that the soft-reset bit is already set and skips resetting the device. The next step is to enable the SDO line. It reads back all 1's, falsely determines that the SDO is enabled and then skips writing to enable it. This leaves the device in a non-functioning state.
By writing directly to register it will always perform the reset, and it will always enable SDO regardless of the invalid values read back during initialization.
> >
> > 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.
Is this the blank line after the fixes? I need to do some research to understand the tag block.
>
> > Signed-off-by: Sam Winchenbach <swinchenbach@arka.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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO
2025-02-01 14:00 ` Sam Winchenbach
@ 2025-02-02 15:15 ` Jonathan Cameron
2025-02-03 13:37 ` Sam Winchenbach
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2025-02-02 15:15 UTC (permalink / raw)
To: Sam Winchenbach
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
antoniu.miclaus@analog.com, lars@metafoo.de,
Michael.Hennerich@analog.com
On Sat, 1 Feb 2025 14:00:58 +0000
Sam Winchenbach <swinchenbach@arka.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!
> >
>
> On power-up the device is configured without SDO enabled. This, alone, makes the read-modify-write during initialization incorrect - It is not possible to read from the device seeing it is not driving the output.
>
> If the SPI bus, like in our situation, has a weak pull-up the situation is compounded. When the device initializes it reads backs all 1's as part of the read-modify-write sequence, falsely determines that the soft-reset bit is already set and skips resetting the device. The next step is to enable the SDO line. It reads back all 1's, falsely determines that the SDO is enabled and then skips writing to enable it. This leaves the device in a non-functioning state.
>
> By writing directly to register it will always perform the reset, and it will always enable SDO regardless of the invalid values read back during initialization.
Ah ok. So it's a bug with or without appropriate pull up.
Just happens to be worse if that is missing.
>
> > >
> > > 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.
>
> Is this the blank line after the fixes? I need to do some research to understand the tag block.
Yes - The block of all tags needs to be contiguous. No blank lines.
Greg has a script that moans about this, not sure if it also runs on linux-next
Jonathan
>
> >
> > > Signed-off-by: Sam Winchenbach <swinchenbach@arka.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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Re: [PATCH v2] iio: filter: admv8818: Force initialization of SDO
2025-02-02 15:15 ` Jonathan Cameron
@ 2025-02-03 13:37 ` Sam Winchenbach
0 siblings, 0 replies; 8+ messages in thread
From: Sam Winchenbach @ 2025-02-03 13:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
antoniu.miclaus@analog.com, lars@metafoo.de,
Michael.Hennerich@analog.com
> > > > 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!
> > >
> >
> > On power-up the device is configured without SDO enabled. This, alone, makes the read-modify-write during initialization incorrect - It is not possible to read from the device seeing it is not driving the output.
> >
> > If the SPI bus, like in our situation, has a weak pull-up the situation is compounded. When the device initializes it reads backs all 1's as part of the read-modify-write sequence, falsely determines that the soft-reset bit is already set and skips resetting the device. The next step is to enable the SDO line. It reads back all 1's, falsely determines that the SDO is enabled and then skips writing to enable it. This leaves the device in a non-functioning state.
> >
> > By writing directly to register it will always perform the reset, and it will always enable SDO regardless of the invalid values read back during initialization.
> Ah ok. So it's a bug with or without appropriate pull up.
> Just happens to be worse if that is missing.
> >
> > > >
> > > > 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.
> >
> > Is this the blank line after the fixes? I need to do some research to understand the tag block.
> Yes - The block of all tags needs to be contiguous. No blank lines.
>
> Greg has a script that moans about this, not sure if it also runs on linux-next
Okay. I submitted v3 with this fix. I also setup mutt/send-email so the next time I submit a patch it won't be quite so painful.
Thank you for the guidance.
>
> Jonathan
> >
> > >
> > > > Signed-off-by: Sam Winchenbach <swinchenbach@arka.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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] iio: filter: admv8818: Force initialization of SDO
2025-02-01 11:18 ` Jonathan Cameron
2025-02-01 14:00 ` Sam Winchenbach
@ 2025-02-03 12:14 ` Miclaus, Antoniu
1 sibling, 0 replies; 8+ messages in thread
From: Miclaus, Antoniu @ 2025-02-03 12:14 UTC (permalink / raw)
To: Jonathan Cameron, Sam Winchenbach
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
lars@metafoo.de, Hennerich, Michael
> On Sat, 25 Jan 2025 14:54:44 +0000
> Sam Winchenbach <swinchenbach@arka.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!
>
Indeed, this can be considered as a "bug". I had a look over the datasheet and the SDO line is disabled by default.
So the `update_bits` doesn't quite make sense in this scenario.
> >
> > 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@arka.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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] iio: filter: admv8818: Force initialization of SDO
2025-01-25 14:54 [PATCH v2] iio: filter: admv8818: Force initialization of SDO Sam Winchenbach
2025-02-01 11:18 ` Jonathan Cameron
@ 2025-02-03 13:34 ` Sam Winchenbach
2025-02-08 12:47 ` Jonathan Cameron
1 sibling, 1 reply; 8+ messages in thread
From: Sam Winchenbach @ 2025-02-03 13:34 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org
Cc: linux-iio@vger.kernel.org, antoniu.miclaus@analog.com,
lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org
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.
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")
Signed-off-by: Sam Winchenbach <swinchenbach@arka.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;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3] iio: filter: admv8818: Force initialization of SDO
2025-02-03 13:34 ` [PATCH v3] " Sam Winchenbach
@ 2025-02-08 12:47 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2025-02-08 12:47 UTC (permalink / raw)
To: Sam Winchenbach
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
antoniu.miclaus@analog.com, lars@metafoo.de,
Michael.Hennerich@analog.com
On Mon, 3 Feb 2025 13:34:34 +0000
Sam Winchenbach <swinchenbach@arka.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.
>
> 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")
> Signed-off-by: Sam Winchenbach <swinchenbach@arka.org>
Applied and marked for stable.
Thanks,
Jonathan
> ---
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-02-08 12:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-25 14:54 [PATCH v2] iio: filter: admv8818: Force initialization of SDO Sam Winchenbach
2025-02-01 11:18 ` Jonathan Cameron
2025-02-01 14:00 ` Sam Winchenbach
2025-02-02 15:15 ` Jonathan Cameron
2025-02-03 13:37 ` Sam Winchenbach
2025-02-03 12:14 ` Miclaus, Antoniu
2025-02-03 13:34 ` [PATCH v3] " Sam Winchenbach
2025-02-08 12:47 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox