[PATCH] drivers: iio: accel: fix ADX355 startup race condition

[PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

From: Valek Andrej <andrej.v@skyrain.eu>

There is an race-condition where device is not full working after SW reset.
Therefore it's necessary to wait some time after reset and verify shadow
registers values by reading and comparing the values before/after reset.
This mechanism is described in datasheet at least from revision D.

Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
---
 drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index 2e00fd51b4d51..5386cd4766def 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -56,6 +56,8 @@
 #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
 #define ADXL355_SELF_TEST_REG		0x2E
 #define ADXL355_RESET_REG		0x2F
+#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
+#define ADXL355_SHADOW_REG_COUNT	5
 
 #define ADXL355_DEVID_AD_VAL		0xAD
 #define ADXL355_DEVID_MST_VAL		0x1D
@@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
 static int adxl355_setup(struct adxl355_data *data)
 {
 	unsigned int regval;
+	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
+	bool shadow_regs_valid = false;
+	int retries = 5; /* retries for reading shadow registers */
 	int ret;
 
 	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
@@ -321,14 +326,47 @@ static int adxl355_setup(struct adxl355_data *data)
 	if (regval != ADXL355_PARTID_VAL)
 		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
 
-	/*
-	 * Perform a software reset to make sure the device is in a consistent
-	 * state after start-up.
-	 */
-	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
+	/* Read shadow registers to be compared after reset */
+	ret = regmap_bulk_read(data->regmap,
+			       ADXL355_BASE_ADDR_SHADOW_REG,
+			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
 	if (ret)
 		return ret;
 
+	/* Do software reset and check validity of the shadow registers */
+	while (!shadow_regs_valid && (retries > 0)) {
+		/*
+		 * Perform a software reset to make sure the device is in a consistent
+		 * state after start-up.
+		 */
+		ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
+		if (ret)
+			return ret;
+
+		/* Wait at least 5ms after software reset */
+		usleep_range(5000, 10000);
+
+		/* Read shadow registers for comparison */
+		ret = regmap_bulk_read(data->regmap,
+				       ADXL355_BASE_ADDR_SHADOW_REG,
+				       data->buffer.buf, ADXL355_SHADOW_REG_COUNT);
+		if (ret)
+			return ret;
+
+		/* Check if shadow registers have expected values */
+		shadow_regs_valid = !memcmp(shadow_regs, data->buffer.buf,
+					    ADXL355_SHADOW_REG_COUNT);
+		if (!shadow_regs_valid) {
+			retries--;
+			dev_warn(data->dev, "Shadow registers mismatch, retrying...\n");
+		}
+	}
+
+	if (!shadow_regs_valid) {
+		dev_err(data->dev, "Invalid shadow registers values\n");
+		return -EIO;
+	}
+
 	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
 				 ADXL355_POWER_CTL_DRDY_MSK,
 				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
-- 
2.34.1

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron]

On Tue,  9 Sep 2025 10:55:28 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> From: Valek Andrej <andrej.v@skyrain.eu>
Hi Valek,

Thanks for the patch. Small thing on patch title, don't include drivers.
It's a pain but you need to look at other patches to a given subsystem
to find out the preferred style.

> 
> There is an race-condition where device is not full working after SW reset.
> Therefore it's necessary to wait some time after reset and verify shadow
> registers values by reading and comparing the values before/after reset.
> This mechanism is described in datasheet at least from revision D.

I'm curious about the retries. Does the datasheet given any explanation for
why a reset might fail a few times before succeeding? 

> 
> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> ---
>  drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 2e00fd51b4d51..5386cd4766def 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -56,6 +56,8 @@
>  #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>  #define ADXL355_SELF_TEST_REG		0x2E
>  #define ADXL355_RESET_REG		0x2F
> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> +#define ADXL355_SHADOW_REG_COUNT	5
>  
>  #define ADXL355_DEVID_AD_VAL		0xAD
>  #define ADXL355_DEVID_MST_VAL		0x1D
> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>  static int adxl355_setup(struct adxl355_data *data)
>  {
>  	unsigned int regval;
> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];

Needs to be a DMA safe buffer.  We can't assume that regmap will always
bounce the data through one before passing it to the SPI controllers
that do sometimes require DMA safe buffers.    Add a buffer to end of
struct adxl355_data where you can take advantage of the forcing of appropriate
padding that is already going on there.

> +	bool shadow_regs_valid = false;
> +	int retries = 5; /* retries for reading shadow registers */

If this is an empirical number of retries say so. If it's on the datasheet
then mention that in the comment instead.

>  	int ret;
>  
>  	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
> @@ -321,14 +326,47 @@ static int adxl355_setup(struct adxl355_data *data)
>  	if (regval != ADXL355_PARTID_VAL)
>  		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>  
> -	/*
> -	 * Perform a software reset to make sure the device is in a consistent
> -	 * state after start-up.
> -	 */
> -	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
> +	/* Read shadow registers to be compared after reset */
> +	ret = regmap_bulk_read(data->regmap,
> +			       ADXL355_BASE_ADDR_SHADOW_REG,
> +			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
>  	if (ret)
>  		return ret;
>  
> +	/* Do software reset and check validity of the shadow registers */
> +	while (!shadow_regs_valid && (retries > 0)) {
I would use a do { } while here.

> +		/*
> +		 * Perform a software reset to make sure the device is in a consistent
> +		 * state after start-up.
> +		 */
> +		ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
> +		if (ret)
> +			return ret;
> +
> +		/* Wait at least 5ms after software reset */
> +		usleep_range(5000, 10000);
> +
> +		/* Read shadow registers for comparison */
> +		ret = regmap_bulk_read(data->regmap,
> +				       ADXL355_BASE_ADDR_SHADOW_REG,
> +				       data->buffer.buf, ADXL355_SHADOW_REG_COUNT);
> +		if (ret)
> +			return ret;
> +
> +		/* Check if shadow registers have expected values */
> +		shadow_regs_valid = !memcmp(shadow_regs, data->buffer.buf,
> +					    ADXL355_SHADOW_REG_COUNT);
The reason for the do { } while () suggestion above is that then you can move
the memcpy into the while condition and just decrement retries unconditionally
at the top of the loop.   Then if retries is 0 in the loop exit directly.
That way we avoid need for the shadow_regs_valid variable and generally end up with
slightly shorter code.

So in all that is something like.

	do {
		retries--;
		if (!retries)
			return dev_err_probe();

		ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
		if (ret)
			return ret;

		/* Wait at least 5ms after software reset */
		usleep_range(5000, 10000);

		/* Read shadow registers for comparison */
		ret = regmap_bulk_read(data->regmap,
				       ADXL355_BASE_ADDR_SHADOW_REG,
				       data->buffer.buf, ADXL355_SHADOW_REG_COUNT);
		if (ret)
			return ret;
	} while(memcmp(shadow_regs, data->buffer.buf,
		       ADXL355_SHADOW_REG_COUNT));

> +		if (!shadow_regs_valid) {
> +			retries--;
> +			dev_warn(data->dev, "Shadow registers mismatch, retrying...\n");
I'd drop this as rather noisy. If the retry is a reasonable thing to do, we probably don't
want to bother userspace with it. If it indicates a hardware problem, just fail if
it doesn't work first time.

> +		}
> +	}
> +
> +	if (!shadow_regs_valid) {
> +		dev_err(data->dev, "Invalid shadow registers values\n");
> +		return -EIO;
> +	}
> +
>  	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
>  				 ADXL355_POWER_CTL_DRDY_MSK,
>  				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

Hi Jonathan,

First, I would like to thanks for your feedback.

On 10.09.2025 20:30, Jonathan Cameron wrote:
> On Tue,  9 Sep 2025 10:55:28 +0200
> Andrej Valek <andrej.v@skyrain.eu> wrote:
>
>> From: Valek Andrej <andrej.v@skyrain.eu>
> Hi Valek,
>
> Thanks for the patch. Small thing on patch title, don't include drivers.
> It's a pain but you need to look at other patches to a given subsystem
> to find out the preferred style.
Valek is my surname 🙂.
Yes, I realized, that I missed "L" in title, will fix it in next rev, 
same as removing "drivers".
>> There is an race-condition where device is not full working after SW reset.
>> Therefore it's necessary to wait some time after reset and verify shadow
>> registers values by reading and comparing the values before/after reset.
>> This mechanism is described in datasheet at least from revision D.
> I'm curious about the retries. Does the datasheet given any explanation for
> why a reset might fail a few times before succeeding?
Yes, as I mentioned in the commit message. Revision "D" of DS 
(https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf) 
page 43:

| In case of a software reset, an unlikely race condition may occur in 
products with REVID = 0x01 or earlier. If the race condition occurs, 
some factory settings in the NVM load incorrectly to shadow registers.
| 1. Read the shadow registers, Register 0x50 to Register 0x54 (five 
8-bit registers) after power-up, but before any software reset.
| 2. Store these values in a host device (for example, a host 
microprocessor).
| 3. After each software reset, read the same five registers. If the 
values differ, perform a software reset again until they match
As I don't want to reset forever, I chose 5 as a max number and seems to 
be working fine.
>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
>> ---
>>   drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
>>   1 file changed, 43 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
>> index 2e00fd51b4d51..5386cd4766def 100644
>> --- a/drivers/iio/accel/adxl355_core.c
>> +++ b/drivers/iio/accel/adxl355_core.c
>> @@ -56,6 +56,8 @@
>>   #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>>   #define ADXL355_SELF_TEST_REG		0x2E
>>   #define ADXL355_RESET_REG		0x2F
>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
>> +#define ADXL355_SHADOW_REG_COUNT	5
>>   
>>   #define ADXL355_DEVID_AD_VAL		0xAD
>>   #define ADXL355_DEVID_MST_VAL		0x1D
>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>>   static int adxl355_setup(struct adxl355_data *data)
>>   {
>>   	unsigned int regval;
>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
> Needs to be a DMA safe buffer.  We can't assume that regmap will always
> bounce the data through one before passing it to the SPI controllers
> that do sometimes require DMA safe buffers.    Add a buffer to end of
> struct adxl355_data where you can take advantage of the forcing of appropriate
> padding that is already going on there.
I see, but I don't like extending the adxl355_data just for one time 
usage. The suggested approach is more similar to what is done with the 
ID checking.
| if (regval != ADXL355_DEVID_MST_VAL) {
|  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
|  return -ENODEV;
| }
>> +	bool shadow_regs_valid = false;
>> +	int retries = 5; /* retries for reading shadow registers */
> If this is an empirical number of retries say so. If it's on the datasheet
> then mention that in the comment instead.
Basically yes, as I said, I didn't want to loop forever. And yes, will 
change the comment to: /* the number of retries is chosen based on 
empirical reasons. */
>
>>   	int ret;
>>   
>>   	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
>> @@ -321,14 +326,47 @@ static int adxl355_setup(struct adxl355_data *data)
>>   	if (regval != ADXL355_PARTID_VAL)
>>   		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>>   
>> -	/*
>> -	 * Perform a software reset to make sure the device is in a consistent
>> -	 * state after start-up.
>> -	 */
>> -	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
>> +	/* Read shadow registers to be compared after reset */
>> +	ret = regmap_bulk_read(data->regmap,
>> +			       ADXL355_BASE_ADDR_SHADOW_REG,
>> +			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
>>   	if (ret)
>>   		return ret;
>>   
>> +	/* Do software reset and check validity of the shadow registers */
>> +	while (!shadow_regs_valid && (retries > 0)) {
> I would use a do { } while here.
>
>> +		/*
>> +		 * Perform a software reset to make sure the device is in a consistent
>> +		 * state after start-up.
>> +		 */
>> +		ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Wait at least 5ms after software reset */
>> +		usleep_range(5000, 10000);
>> +
>> +		/* Read shadow registers for comparison */
>> +		ret = regmap_bulk_read(data->regmap,
>> +				       ADXL355_BASE_ADDR_SHADOW_REG,
>> +				       data->buffer.buf, ADXL355_SHADOW_REG_COUNT);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Check if shadow registers have expected values */
>> +		shadow_regs_valid = !memcmp(shadow_regs, data->buffer.buf,
>> +					    ADXL355_SHADOW_REG_COUNT);
> The reason for the do { } while () suggestion above is that then you can move
> the memcpy into the while condition and just decrement retries unconditionally
> at the top of the loop.   Then if retries is 0 in the loop exit directly.
> That way we avoid need for the shadow_regs_valid variable and generally end up with
> slightly shorter code.
>
> So in all that is something like.
>
> 	do {
> 		retries--;
> 		if (!retries)
> 			return dev_err_probe();
>
> 		ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
> 		if (ret)
> 			return ret;
>
> 		/* Wait at least 5ms after software reset */
> 		usleep_range(5000, 10000);
>
> 		/* Read shadow registers for comparison */
> 		ret = regmap_bulk_read(data->regmap,
> 				       ADXL355_BASE_ADDR_SHADOW_REG,
> 				       data->buffer.buf, ADXL355_SHADOW_REG_COUNT);
> 		if (ret)
> 			return ret;
> 	} while(memcmp(shadow_regs, data->buffer.buf,
> 		       ADXL355_SHADOW_REG_COUNT));
Ok, will change this.
>> +		if (!shadow_regs_valid) {
>> +			retries--;
>> +			dev_warn(data->dev, "Shadow registers mismatch, retrying...\n");
> I'd drop this as rather noisy. If the retry is a reasonable thing to do, we probably don't
> want to bother userspace with it. If it indicates a hardware problem, just fail if
> it doesn't work first time.
Will change it as well. There is no need to log warnings to the user space.
>> +		}
>> +	}
>> +
>> +	if (!shadow_regs_valid) {
>> +		dev_err(data->dev, "Invalid shadow registers values\n");
>> +		return -EIO;
>> +	}
>> +
>>   	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
>>   				 ADXL355_POWER_CTL_DRDY_MSK,
>>   				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
>
Will wait for the response and then send V2.

BR,
Andy

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron]

On Thu, 11 Sep 2025 11:33:49 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> Hi Jonathan,
> 
> First, I would like to thanks for your feedback.
> 
> On 10.09.2025 20:30, Jonathan Cameron wrote:
> > On Tue,  9 Sep 2025 10:55:28 +0200
> > Andrej Valek <andrej.v@skyrain.eu> wrote:
> >  
> >> From: Valek Andrej <andrej.v@skyrain.eu>  
> > Hi Valek,
> >
> > Thanks for the patch. Small thing on patch title, don't include drivers.
> > It's a pain but you need to look at other patches to a given subsystem
> > to find out the preferred style.  
> Valek is my surname 🙂.

Oops. Sorry! 

Hi Andrej,

> >> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> >> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> >> ---
> >>   drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
> >>   1 file changed, 43 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> >> index 2e00fd51b4d51..5386cd4766def 100644
> >> --- a/drivers/iio/accel/adxl355_core.c
> >> +++ b/drivers/iio/accel/adxl355_core.c
> >> @@ -56,6 +56,8 @@
> >>   #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
> >>   #define ADXL355_SELF_TEST_REG		0x2E
> >>   #define ADXL355_RESET_REG		0x2F
> >> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> >> +#define ADXL355_SHADOW_REG_COUNT	5
> >>   
> >>   #define ADXL355_DEVID_AD_VAL		0xAD
> >>   #define ADXL355_DEVID_MST_VAL		0x1D
> >> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> >>   static int adxl355_setup(struct adxl355_data *data)
> >>   {
> >>   	unsigned int regval;
> >> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];  
> > Needs to be a DMA safe buffer.  We can't assume that regmap will always
> > bounce the data through one before passing it to the SPI controllers
> > that do sometimes require DMA safe buffers.    Add a buffer to end of
> > struct adxl355_data where you can take advantage of the forcing of appropriate
> > padding that is already going on there.  

> I see, but I don't like extending the adxl355_data just for one time 
> usage. The suggested approach is more similar to what is done with the 
> ID checking.
> | if (regval != ADXL355_DEVID_MST_VAL) {
> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
> |  return -ENODEV;
> | }

That's a single read.  Regmap always bounces those so a local variable should
be fine. Not so for a bulk read (or at least no one guarantees it)

https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).

The path for a single read goes through:
https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
_regmap_bus_read() which uses map->work_buf which is DMA safe.

Jonathan

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

Hi Jonathan,

I submitted the version 2 with applied your suggestions.

On 12.09.2025 16:12, Jonathan Cameron wrote:
> On Thu, 11 Sep 2025 11:33:49 +0200
> Andrej Valek <andrej.v@skyrain.eu> wrote:
>
>> Hi Jonathan,
>>
>> First, I would like to thanks for your feedback.
>>
>> On 10.09.2025 20:30, Jonathan Cameron wrote:
>>> On Tue,  9 Sep 2025 10:55:28 +0200
>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>>   
>>>> From: Valek Andrej <andrej.v@skyrain.eu>
>>> Hi Valek,
>>>
>>> Thanks for the patch. Small thing on patch title, don't include drivers.
>>> It's a pain but you need to look at other patches to a given subsystem
>>> to find out the preferred style.
>> Valek is my surname 🙂.
> Oops. Sorry!
>
> Hi Andrej,
>
>>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
>>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
>>>> ---
>>>>    drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
>>>>    1 file changed, 43 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
>>>> index 2e00fd51b4d51..5386cd4766def 100644
>>>> --- a/drivers/iio/accel/adxl355_core.c
>>>> +++ b/drivers/iio/accel/adxl355_core.c
>>>> @@ -56,6 +56,8 @@
>>>>    #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>>>>    #define ADXL355_SELF_TEST_REG		0x2E
>>>>    #define ADXL355_RESET_REG		0x2F
>>>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
>>>> +#define ADXL355_SHADOW_REG_COUNT	5
>>>>    
>>>>    #define ADXL355_DEVID_AD_VAL		0xAD
>>>>    #define ADXL355_DEVID_MST_VAL		0x1D
>>>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>>>>    static int adxl355_setup(struct adxl355_data *data)
>>>>    {
>>>>    	unsigned int regval;
>>>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
>>> Needs to be a DMA safe buffer.  We can't assume that regmap will always
>>> bounce the data through one before passing it to the SPI controllers
>>> that do sometimes require DMA safe buffers.    Add a buffer to end of
>>> struct adxl355_data where you can take advantage of the forcing of appropriate
>>> padding that is already going on there.
>> I see, but I don't like extending the adxl355_data just for one time
>> usage. The suggested approach is more similar to what is done with the
>> ID checking.
>> | if (regval != ADXL355_DEVID_MST_VAL) {
>> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
>> |  return -ENODEV;
>> | }
> That's a single read.  Regmap always bounces those so a local variable should
> be fine. Not so for a bulk read (or at least no one guarantees it)
>
> https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).
>
> The path for a single read goes through:
> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
> _regmap_bus_read() which uses map->work_buf which is DMA safe.
regmap_bulk_read is using the same _regmap_read 
https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137 
so it should be safe, too I guess.
>
> Jonathan
>
>
Andy

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron]

On Mon, 15 Sep 2025 14:03:47 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> Hi Jonathan,
> 
> I submitted the version 2 with applied your suggestions.
> 
> On 12.09.2025 16:12, Jonathan Cameron wrote:
> > On Thu, 11 Sep 2025 11:33:49 +0200
> > Andrej Valek <andrej.v@skyrain.eu> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> First, I would like to thanks for your feedback.
> >>
> >> On 10.09.2025 20:30, Jonathan Cameron wrote:  
> >>> On Tue,  9 Sep 2025 10:55:28 +0200
> >>> Andrej Valek <andrej.v@skyrain.eu> wrote:
> >>>     
> >>>> From: Valek Andrej <andrej.v@skyrain.eu>  
> >>> Hi Valek,
> >>>
> >>> Thanks for the patch. Small thing on patch title, don't include drivers.
> >>> It's a pain but you need to look at other patches to a given subsystem
> >>> to find out the preferred style.  
> >> Valek is my surname 🙂.  
> > Oops. Sorry!
> >
> > Hi Andrej,
> >  
> >>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> >>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> >>>> ---
> >>>>    drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
> >>>>    1 file changed, 43 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> >>>> index 2e00fd51b4d51..5386cd4766def 100644
> >>>> --- a/drivers/iio/accel/adxl355_core.c
> >>>> +++ b/drivers/iio/accel/adxl355_core.c
> >>>> @@ -56,6 +56,8 @@
> >>>>    #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
> >>>>    #define ADXL355_SELF_TEST_REG		0x2E
> >>>>    #define ADXL355_RESET_REG		0x2F
> >>>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> >>>> +#define ADXL355_SHADOW_REG_COUNT	5
> >>>>    
> >>>>    #define ADXL355_DEVID_AD_VAL		0xAD
> >>>>    #define ADXL355_DEVID_MST_VAL		0x1D
> >>>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> >>>>    static int adxl355_setup(struct adxl355_data *data)
> >>>>    {
> >>>>    	unsigned int regval;
> >>>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];  
> >>> Needs to be a DMA safe buffer.  We can't assume that regmap will always
> >>> bounce the data through one before passing it to the SPI controllers
> >>> that do sometimes require DMA safe buffers.    Add a buffer to end of
> >>> struct adxl355_data where you can take advantage of the forcing of appropriate
> >>> padding that is already going on there.  
> >> I see, but I don't like extending the adxl355_data just for one time
> >> usage. The suggested approach is more similar to what is done with the
> >> ID checking.
> >> | if (regval != ADXL355_DEVID_MST_VAL) {
> >> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
> >> |  return -ENODEV;
> >> | }  
> > That's a single read.  Regmap always bounces those so a local variable should
> > be fine. Not so for a bulk read (or at least no one guarantees it)
> >
> > https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> > Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).
> >
> > The path for a single read goes through:
> > https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
> > _regmap_bus_read() which uses map->work_buf which is DMA safe.  
> regmap_bulk_read is using the same _regmap_read 
> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137 
> so it should be safe, too I guess.

Whilst true, the repeated statement from the regmap maintainer that
is mentioned in the talk is that there is no guarantee that will continue
to be the case :(  Whereas the single reads are fine and will remain so.
I guess one reason for that is that regmap_read() take an unsigned int *val
so the type is almost always going to be wrong and a copy necessary. 

Anyhow, the whole thing is a bit silly but upshot is dma safe buffers only to bulk
accessors unless the ABI is ever documented as not needing them.

Jonathan

> >
> > Jonathan
> >
> >  
> Andy
> 

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

On 15.09.2025 17:53, Jonathan Cameron wrote:
> On Mon, 15 Sep 2025 14:03:47 +0200
> Andrej Valek <andrej.v@skyrain.eu> wrote:
>
>> Hi Jonathan,
>>
>> I submitted the version 2 with applied your suggestions.
>>
>> On 12.09.2025 16:12, Jonathan Cameron wrote:
>>> On Thu, 11 Sep 2025 11:33:49 +0200
>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>>   
>>>> Hi Jonathan,
>>>>
>>>> First, I would like to thanks for your feedback.
>>>>
>>>> On 10.09.2025 20:30, Jonathan Cameron wrote:
>>>>> On Tue,  9 Sep 2025 10:55:28 +0200
>>>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>>>>      
>>>>>> From: Valek Andrej <andrej.v@skyrain.eu>
>>>>> Hi Valek,
>>>>>
>>>>> Thanks for the patch. Small thing on patch title, don't include drivers.
>>>>> It's a pain but you need to look at other patches to a given subsystem
>>>>> to find out the preferred style.
>>>> Valek is my surname 🙂.
>>> Oops. Sorry!
>>>
>>> Hi Andrej,
>>>   
>>>>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
>>>>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
>>>>>> ---
>>>>>>     drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
>>>>>>     1 file changed, 43 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
>>>>>> index 2e00fd51b4d51..5386cd4766def 100644
>>>>>> --- a/drivers/iio/accel/adxl355_core.c
>>>>>> +++ b/drivers/iio/accel/adxl355_core.c
>>>>>> @@ -56,6 +56,8 @@
>>>>>>     #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>>>>>>     #define ADXL355_SELF_TEST_REG		0x2E
>>>>>>     #define ADXL355_RESET_REG		0x2F
>>>>>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
>>>>>> +#define ADXL355_SHADOW_REG_COUNT	5
>>>>>>     
>>>>>>     #define ADXL355_DEVID_AD_VAL		0xAD
>>>>>>     #define ADXL355_DEVID_MST_VAL		0x1D
>>>>>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>>>>>>     static int adxl355_setup(struct adxl355_data *data)
>>>>>>     {
>>>>>>     	unsigned int regval;
>>>>>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
>>>>> Needs to be a DMA safe buffer.  We can't assume that regmap will always
>>>>> bounce the data through one before passing it to the SPI controllers
>>>>> that do sometimes require DMA safe buffers.    Add a buffer to end of
>>>>> struct adxl355_data where you can take advantage of the forcing of appropriate
>>>>> padding that is already going on there.
>>>> I see, but I don't like extending the adxl355_data just for one time
>>>> usage. The suggested approach is more similar to what is done with the
>>>> ID checking.
>>>> | if (regval != ADXL355_DEVID_MST_VAL) {
>>>> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
>>>> |  return -ENODEV;
>>>> | }
>>> That's a single read.  Regmap always bounces those so a local variable should
>>> be fine. Not so for a bulk read (or at least no one guarantees it)
>>>
>>> https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
>>> Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).
>>>
>>> The path for a single read goes through:
>>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
>>> _regmap_bus_read() which uses map->work_buf which is DMA safe.
>> regmap_bulk_read is using the same _regmap_read
>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137
>> so it should be safe, too I guess.
> Whilst true, the repeated statement from the regmap maintainer that
> is mentioned in the talk is that there is no guarantee that will continue
> to be the case :(  Whereas the single reads are fine and will remain so.
> I guess one reason for that is that regmap_read() take an unsigned int *val
> so the type is almost always going to be wrong and a copy necessary.
>
> Anyhow, the whole thing is a bit silly but upshot is dma safe buffers only to bulk
> accessors unless the ABI is ever documented as not needing them.
>
> Jonathan
Ok, I see. So can we live with the variant I did?
>>> Jonathan
>>>
>>>   
>> Andy
>>
>
In other words, is there anything pending from my side?

Andy

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron]

On Tue, 16 Sep 2025 09:07:18 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> On 15.09.2025 17:53, Jonathan Cameron wrote:
> > On Mon, 15 Sep 2025 14:03:47 +0200
> > Andrej Valek <andrej.v@skyrain.eu> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> I submitted the version 2 with applied your suggestions.
> >>
> >> On 12.09.2025 16:12, Jonathan Cameron wrote:  
> >>> On Thu, 11 Sep 2025 11:33:49 +0200
> >>> Andrej Valek <andrej.v@skyrain.eu> wrote:
> >>>     
> >>>> Hi Jonathan,
> >>>>
> >>>> First, I would like to thanks for your feedback.
> >>>>
> >>>> On 10.09.2025 20:30, Jonathan Cameron wrote:  
> >>>>> On Tue,  9 Sep 2025 10:55:28 +0200
> >>>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
> >>>>>        
> >>>>>> From: Valek Andrej <andrej.v@skyrain.eu>  
> >>>>> Hi Valek,
> >>>>>
> >>>>> Thanks for the patch. Small thing on patch title, don't include drivers.
> >>>>> It's a pain but you need to look at other patches to a given subsystem
> >>>>> to find out the preferred style.  
> >>>> Valek is my surname 🙂.  
> >>> Oops. Sorry!
> >>>
> >>> Hi Andrej,
> >>>     
> >>>>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> >>>>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> >>>>>> ---
> >>>>>>     drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
> >>>>>>     1 file changed, 43 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> >>>>>> index 2e00fd51b4d51..5386cd4766def 100644
> >>>>>> --- a/drivers/iio/accel/adxl355_core.c
> >>>>>> +++ b/drivers/iio/accel/adxl355_core.c
> >>>>>> @@ -56,6 +56,8 @@
> >>>>>>     #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
> >>>>>>     #define ADXL355_SELF_TEST_REG		0x2E
> >>>>>>     #define ADXL355_RESET_REG		0x2F
> >>>>>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> >>>>>> +#define ADXL355_SHADOW_REG_COUNT	5
> >>>>>>     
> >>>>>>     #define ADXL355_DEVID_AD_VAL		0xAD
> >>>>>>     #define ADXL355_DEVID_MST_VAL		0x1D
> >>>>>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> >>>>>>     static int adxl355_setup(struct adxl355_data *data)
> >>>>>>     {
> >>>>>>     	unsigned int regval;
> >>>>>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];  
> >>>>> Needs to be a DMA safe buffer.  We can't assume that regmap will always
> >>>>> bounce the data through one before passing it to the SPI controllers
> >>>>> that do sometimes require DMA safe buffers.    Add a buffer to end of
> >>>>> struct adxl355_data where you can take advantage of the forcing of appropriate
> >>>>> padding that is already going on there.  
> >>>> I see, but I don't like extending the adxl355_data just for one time
> >>>> usage. The suggested approach is more similar to what is done with the
> >>>> ID checking.
> >>>> | if (regval != ADXL355_DEVID_MST_VAL) {
> >>>> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
> >>>> |  return -ENODEV;
> >>>> | }  
> >>> That's a single read.  Regmap always bounces those so a local variable should
> >>> be fine. Not so for a bulk read (or at least no one guarantees it)
> >>>
> >>> https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> >>> Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).
> >>>
> >>> The path for a single read goes through:
> >>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
> >>> _regmap_bus_read() which uses map->work_buf which is DMA safe.  
> >> regmap_bulk_read is using the same _regmap_read
> >> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137
> >> so it should be safe, too I guess.  
> > Whilst true, the repeated statement from the regmap maintainer that
> > is mentioned in the talk is that there is no guarantee that will continue
> > to be the case :(  Whereas the single reads are fine and will remain so.
> > I guess one reason for that is that regmap_read() take an unsigned int *val
> > so the type is almost always going to be wrong and a copy necessary.
> >
> > Anyhow, the whole thing is a bit silly but upshot is dma safe buffers only to bulk
> > accessors unless the ABI is ever documented as not needing them.
> >
> > Jonathan  
> Ok, I see. So can we live with the variant I did?
> >>> Jonathan
> >>>
> >>>     
> >> Andy
> >>  
> >  
> In other words, is there anything pending from my side?

Use a dma safe buffer for the bulk read. It's not a bug today
but it is relying on undocumented behaviour of the regmap
implementation that may change.

Jonathan

> 
> Andy

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

Hello Jonathan,

Just a friendly reminder :).

Is there anything blocking before going to be merged?

Regards,
Andrej

On 16.09.2025 09:07, Andrej Valek wrote:
>
>
> On 15.09.2025 17:53, Jonathan Cameron wrote:
>> On Mon, 15 Sep 2025 14:03:47 +0200
>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>
>>> Hi Jonathan,
>>>
>>> I submitted the version 2 with applied your suggestions.
>>>
>>> On 12.09.2025 16:12, Jonathan Cameron wrote:
>>>> On Thu, 11 Sep 2025 11:33:49 +0200
>>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>>>> Hi Jonathan,
>>>>>
>>>>> First, I would like to thanks for your feedback.
>>>>>
>>>>> On 10.09.2025 20:30, Jonathan Cameron wrote:
>>>>>> On Tue,  9 Sep 2025 10:55:28 +0200
>>>>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
>>>>>>> From: Valek Andrej <andrej.v@skyrain.eu>
>>>>>> Hi Valek,
>>>>>>
>>>>>> Thanks for the patch. Small thing on patch title, don't include 
>>>>>> drivers.
>>>>>> It's a pain but you need to look at other patches to a given 
>>>>>> subsystem
>>>>>> to find out the preferred style.
>>>>> Valek is my surname 🙂.
>>>> Oops. Sorry!
>>>>
>>>> Hi Andrej,
>>>>>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
>>>>>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
>>>>>>> ---
>>>>>>>     drivers/iio/accel/adxl355_core.c | 48 
>>>>>>> ++++++++++++++++++++++++++++----
>>>>>>>     1 file changed, 43 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/accel/adxl355_core.c 
>>>>>>> b/drivers/iio/accel/adxl355_core.c
>>>>>>> index 2e00fd51b4d51..5386cd4766def 100644
>>>>>>> --- a/drivers/iio/accel/adxl355_core.c
>>>>>>> +++ b/drivers/iio/accel/adxl355_core.c
>>>>>>> @@ -56,6 +56,8 @@
>>>>>>>     #define  ADXL355_POWER_CTL_DRDY_MSK    BIT(2)
>>>>>>>     #define ADXL355_SELF_TEST_REG        0x2E
>>>>>>>     #define ADXL355_RESET_REG        0x2F
>>>>>>> +#define ADXL355_BASE_ADDR_SHADOW_REG    0x50
>>>>>>> +#define ADXL355_SHADOW_REG_COUNT    5
>>>>>>>         #define ADXL355_DEVID_AD_VAL        0xAD
>>>>>>>     #define ADXL355_DEVID_MST_VAL        0x1D
>>>>>>> @@ -294,6 +296,9 @@ static void 
>>>>>>> adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>>>>>>>     static int adxl355_setup(struct adxl355_data *data)
>>>>>>>     {
>>>>>>>         unsigned int regval;
>>>>>>> +    u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
>>>>>> Needs to be a DMA safe buffer.  We can't assume that regmap will 
>>>>>> always
>>>>>> bounce the data through one before passing it to the SPI controllers
>>>>>> that do sometimes require DMA safe buffers.    Add a buffer to 
>>>>>> end of
>>>>>> struct adxl355_data where you can take advantage of the forcing 
>>>>>> of appropriate
>>>>>> padding that is already going on there.
>>>>> I see, but I don't like extending the adxl355_data just for one time
>>>>> usage. The suggested approach is more similar to what is done with 
>>>>> the
>>>>> ID checking.
>>>>> | if (regval != ADXL355_DEVID_MST_VAL) {
>>>>> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
>>>>> |  return -ENODEV;
>>>>> | }
>>>> That's a single read.  Regmap always bounces those so a local 
>>>> variable should
>>>> be fine. Not so for a bulk read (or at least no one guarantees it)
>>>>
>>>> https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf 
>>>>
>>>> Slide 11 (in general this slide deck is a good introduction to the 
>>>> fun of DMA safety).
>>>>
>>>> The path for a single read goes through:
>>>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779 
>>>>
>>>> _regmap_bus_read() which uses map->work_buf which is DMA safe.
>>> regmap_bulk_read is using the same _regmap_read
>>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137 
>>>
>>> so it should be safe, too I guess.
>> Whilst true, the repeated statement from the regmap maintainer that
>> is mentioned in the talk is that there is no guarantee that will 
>> continue
>> to be the case :(  Whereas the single reads are fine and will remain so.
>> I guess one reason for that is that regmap_read() take an unsigned 
>> int *val
>> so the type is almost always going to be wrong and a copy necessary.
>>
>> Anyhow, the whole thing is a bit silly but upshot is dma safe buffers 
>> only to bulk
>> accessors unless the ABI is ever documented as not needing them.
>>
>> Jonathan
> Ok, I see. So can we live with the variant I did?
>>>> Jonathan
>>>>
>>> Andy
>>>
>>
> In other words, is there anything pending from my side?
>
> Andy

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[David Lechner]

On Mon, Sep 29, 2025 at 8:24 AM Andrej Valek <andrej.v@skyrain.eu> wrote:
>
> Hello Jonathan,
>
> Just a friendly reminder :).
>
> Is there anything blocking before going to be merged?
>

There were some changes requested already [1] to add a Fixes tag and
use a DMA-safe buffer. There is already such a buffer we can use in
struct adxl355_data..

[1]: https://lore.kernel.org/linux-iio/20250927145515.26692e60@jic23-huawei/

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Andrej Valek]

Hello David,

Ah, my fault, I overlooked the response from him. Should be applied in 
version 3.

BR,
Andy

On 01.10.2025 15:29, David Lechner wrote:
> On Mon, Sep 29, 2025 at 8:24 AM Andrej Valek <andrej.v@skyrain.eu> wrote:
>> Hello Jonathan,
>>
>> Just a friendly reminder :).
>>
>> Is there anything blocking before going to be merged?
>>
> There were some changes requested already [1] to add a Fixes tag and
> use a DMA-safe buffer. There is already such a buffer we can use in
> struct adxl355_data..
>
> [1]: https://lore.kernel.org/linux-iio/20250927145515.26692e60@jic23-huawei/
>

Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron]

On Tue,  9 Sep 2025 10:55:28 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> From: Valek Andrej <andrej.v@skyrain.eu>
Just noticed the part number in the title is missing the L.

[PATCH v2] iio: accel: fix ADXL355 startup race condition

[Andrej Valek]

From: Valek Andrej <andrej.v@skyrain.eu>

There is an race-condition where device is not full working after SW reset.
Therefore it's necessary to wait some time after reset and verify shadow
registers values by reading and comparing the values before/after reset.
This mechanism is described in datasheet at least from revision D.

Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
---
 drivers/iio/accel/adxl355_core.c | 41 ++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index 2e00fd51b4d51..d688841f374da 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -56,6 +56,8 @@
 #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
 #define ADXL355_SELF_TEST_REG		0x2E
 #define ADXL355_RESET_REG		0x2F
+#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
+#define ADXL355_SHADOW_REG_COUNT	5
 
 #define ADXL355_DEVID_AD_VAL		0xAD
 #define ADXL355_DEVID_MST_VAL		0x1D
@@ -294,6 +296,8 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
 static int adxl355_setup(struct adxl355_data *data)
 {
 	unsigned int regval;
+	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
+	int retries = 5; /* the number is chosen based on empirical reasons */
 	int ret;
 
 	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
@@ -321,14 +325,41 @@ static int adxl355_setup(struct adxl355_data *data)
 	if (regval != ADXL355_PARTID_VAL)
 		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
 
-	/*
-	 * Perform a software reset to make sure the device is in a consistent
-	 * state after start-up.
-	 */
-	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
+	/* Read shadow registers to be compared after reset */
+	ret = regmap_bulk_read(data->regmap,
+			       ADXL355_BASE_ADDR_SHADOW_REG,
+			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
 	if (ret)
 		return ret;
 
+	do {
+		if (--retries == 0) {
+			dev_err(data->dev, "Shadow registers mismatch\n");
+			return -EIO;
+		}
+
+		/*
+		 * Perform a software reset to make sure the device is in a consistent
+		 * state after start-up.
+		 */
+		ret = regmap_write(data->regmap, ADXL355_RESET_REG,
+				   ADXL355_RESET_CODE);
+		if (ret)
+			return ret;
+
+		/* Wait at least 5ms after software reset */
+		usleep_range(5000, 10000);
+
+		/* Read shadow registers for comparison */
+		ret = regmap_bulk_read(data->regmap,
+				       ADXL355_BASE_ADDR_SHADOW_REG,
+				       data->buffer.buf,
+				       ADXL355_SHADOW_REG_COUNT);
+		if (ret)
+			return ret;
+	} while (memcmp(shadow_regs, data->buffer.buf,
+			ADXL355_SHADOW_REG_COUNT));
+
 	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
 				 ADXL355_POWER_CTL_DRDY_MSK,
 				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
-- 
2.34.1

Re: [PATCH v2] iio: accel: fix ADXL355 startup race condition

[Jonathan Cameron]

On Mon, 15 Sep 2025 13:58:18 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> From: Valek Andrej <andrej.v@skyrain.eu>
> 
> There is an race-condition where device is not full working after SW reset.
> Therefore it's necessary to wait some time after reset and verify shadow
> registers values by reading and comparing the values before/after reset.
> This mechanism is described in datasheet at least from revision D.
> 
> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> ---
There should be a change lot here.

As per the reply I just sent, even though today regmap doesn't need a dma
safe buffer for bulk reads that is undocumented detail and may not remain
true in the long run.  Hence use a dma safe buffer.

>  drivers/iio/accel/adxl355_core.c | 41 ++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 2e00fd51b4d51..d688841f374da 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -56,6 +56,8 @@
>  #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>  #define ADXL355_SELF_TEST_REG		0x2E
>  #define ADXL355_RESET_REG		0x2F
> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> +#define ADXL355_SHADOW_REG_COUNT	5
>  
>  #define ADXL355_DEVID_AD_VAL		0xAD
>  #define ADXL355_DEVID_MST_VAL		0x1D
> @@ -294,6 +296,8 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>  static int adxl355_setup(struct adxl355_data *data)
>  {
>  	unsigned int regval;
> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];
As above. This needs to be DMA safe. If you want to keep it local use
	u8 *shadow_regs __free(kfree) = kzalloc(ADXL355_SHADOW_REG_COUNT, GFP_KERNEL);
or kcalloc if you prefer.  


> +	int retries = 5; /* the number is chosen based on empirical reasons */
>  	int ret;
>  
>  	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
> @@ -321,14 +325,41 @@ static int adxl355_setup(struct adxl355_data *data)
>  	if (regval != ADXL355_PARTID_VAL)
>  		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>  
> -	/*
> -	 * Perform a software reset to make sure the device is in a consistent
> -	 * state after start-up.
> -	 */
> -	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
> +	/* Read shadow registers to be compared after reset */
> +	ret = regmap_bulk_read(data->regmap,
> +			       ADXL355_BASE_ADDR_SHADOW_REG,
> +			       shadow_regs, ADXL355_SHADOW_REG_COUNT);

Here is the non dma safe buffer usage.

>  	if (ret)
>  		return ret;
>  
> +	do {
> +		if (--retries == 0) {
> +			dev_err(data->dev, "Shadow registers mismatch\n");
> +			return -EIO;
> +		}
> +
> +		/*
> +		 * Perform a software reset to make sure the device is in a consistent
> +		 * state after start-up.
> +		 */
> +		ret = regmap_write(data->regmap, ADXL355_RESET_REG,
> +				   ADXL355_RESET_CODE);
> +		if (ret)
> +			return ret;
> +
> +		/* Wait at least 5ms after software reset */
> +		usleep_range(5000, 10000);
> +
> +		/* Read shadow registers for comparison */
> +		ret = regmap_bulk_read(data->regmap,
> +				       ADXL355_BASE_ADDR_SHADOW_REG,
> +				       data->buffer.buf,
> +				       ADXL355_SHADOW_REG_COUNT);
> +		if (ret)
> +			return ret;
> +	} while (memcmp(shadow_regs, data->buffer.buf,
> +			ADXL355_SHADOW_REG_COUNT));
> +
>  	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
>  				 ADXL355_POWER_CTL_DRDY_MSK,
>  				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));

[PATCH v3] iio: accel: fix ADXL355 startup race condition

[Andrej Valek]

From: Valek Andrej <andrej.v@skyrain.eu>

There is an race-condition where device is not full working after SW reset.
Therefore it's necessary to wait some time after reset and verify shadow
registers values by reading and comparing the values before/after reset.
This mechanism is described in datasheet at least from revision D.

Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
---
 drivers/iio/accel/adxl355_core.c | 41 ++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index 2e00fd51b4d5..782085d74ab8 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -56,6 +56,8 @@
 #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
 #define ADXL355_SELF_TEST_REG		0x2E
 #define ADXL355_RESET_REG		0x2F
+#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
+#define ADXL355_SHADOW_REG_COUNT	5
 
 #define ADXL355_DEVID_AD_VAL		0xAD
 #define ADXL355_DEVID_MST_VAL		0x1D
@@ -294,6 +296,8 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
 static int adxl355_setup(struct adxl355_data *data)
 {
 	unsigned int regval;
+	u8 *shadow_regs __free(kfree) = kzalloc(ADXL355_SHADOW_REG_COUNT, GFP_KERNEL);
+	int retries = 5; /* the number is chosen based on empirical reasons */
 	int ret;
 
 	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
@@ -321,14 +325,41 @@ static int adxl355_setup(struct adxl355_data *data)
 	if (regval != ADXL355_PARTID_VAL)
 		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
 
-	/*
-	 * Perform a software reset to make sure the device is in a consistent
-	 * state after start-up.
-	 */
-	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
+	/* Read shadow registers to be compared after reset */
+	ret = regmap_bulk_read(data->regmap,
+			       ADXL355_BASE_ADDR_SHADOW_REG,
+			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
 	if (ret)
 		return ret;
 
+	do {
+		if (--retries == 0) {
+			dev_err(data->dev, "Shadow registers mismatch\n");
+			return -EIO;
+		}
+
+		/*
+		 * Perform a software reset to make sure the device is in a consistent
+		 * state after start-up.
+		 */
+		ret = regmap_write(data->regmap, ADXL355_RESET_REG,
+				   ADXL355_RESET_CODE);
+		if (ret)
+			return ret;
+
+		/* Wait at least 5ms after software reset */
+		usleep_range(5000, 10000);
+
+		/* Read shadow registers for comparison */
+		ret = regmap_bulk_read(data->regmap,
+				       ADXL355_BASE_ADDR_SHADOW_REG,
+				       data->buffer.buf,
+				       ADXL355_SHADOW_REG_COUNT);
+		if (ret)
+			return ret;
+	} while (memcmp(shadow_regs, data->buffer.buf,
+			ADXL355_SHADOW_REG_COUNT));
+
 	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
 				 ADXL355_POWER_CTL_DRDY_MSK,
 				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
-- 
2.34.1

Re: [PATCH v3] iio: accel: fix ADXL355 startup race condition

[Jonathan Cameron]

On Wed,  1 Oct 2025 16:37:02 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> From: Valek Andrej <andrej.v@skyrain.eu>
> 
> There is an race-condition where device is not full working after SW reset.
> Therefore it's necessary to wait some time after reset and verify shadow
> registers values by reading and comparing the values before/after reset.
> This mechanism is described in datasheet at least from revision D.
> 
> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>

Hi Valek,

One process thing is to not reply to an existing thread with a new version.
Whilst it seems nice and tidy when we only have one patch in a series, it can
get very complex to read if we have deep threads with many versions being commented
on at the same time.  Also tends to end up way off the top of the screen in
threaded email clients.  Hence this is about the last patch I'm replying to today
just because for once I've more or less caught up with all my IIO related emails.

So just send a fresh series that isn't in reply to anything.

Mostly good, but can you reply with a fixes tag and are you fine with me
adding the error check I call out below?


> ---
>  drivers/iio/accel/adxl355_core.c | 41 ++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 2e00fd51b4d5..782085d74ab8 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -56,6 +56,8 @@
>  #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>  #define ADXL355_SELF_TEST_REG		0x2E
>  #define ADXL355_RESET_REG		0x2F
> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> +#define ADXL355_SHADOW_REG_COUNT	5
>  
>  #define ADXL355_DEVID_AD_VAL		0xAD
>  #define ADXL355_DEVID_MST_VAL		0x1D
> @@ -294,6 +296,8 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>  static int adxl355_setup(struct adxl355_data *data)
>  {
>  	unsigned int regval;
> +	u8 *shadow_regs __free(kfree) = kzalloc(ADXL355_SHADOW_REG_COUNT, GFP_KERNEL);
> +	int retries = 5; /* the number is chosen based on empirical reasons */
>  	int ret;

	if (!shadow_regs)
		return -ENOMEM;

Whilst very unlikely to happen we still need to handle memory allocation failure.
>  
>  	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
> @@ -321,14 +325,41 @@ static int adxl355_setup(struct adxl355_data *data)
>  	if (regval != ADXL355_PARTID_VAL)
>  		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>  
> -	/*
> -	 * Perform a software reset to make sure the device is in a consistent
> -	 * state after start-up.
> -	 */
> -	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
> +	/* Read shadow registers to be compared after reset */
> +	ret = regmap_bulk_read(data->regmap,
> +			       ADXL355_BASE_ADDR_SHADOW_REG,
> +			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
>  	if (ret)
>  		return ret;
>  
> +	do {
> +		if (--retries == 0) {
> +			dev_err(data->dev, "Shadow registers mismatch\n");
> +			return -EIO;
> +		}
> +
> +		/*
> +		 * Perform a software reset to make sure the device is in a consistent
> +		 * state after start-up.
> +		 */
> +		ret = regmap_write(data->regmap, ADXL355_RESET_REG,
> +				   ADXL355_RESET_CODE);
> +		if (ret)
> +			return ret;
> +
> +		/* Wait at least 5ms after software reset */
> +		usleep_range(5000, 10000);
> +
> +		/* Read shadow registers for comparison */
> +		ret = regmap_bulk_read(data->regmap,
> +				       ADXL355_BASE_ADDR_SHADOW_REG,
> +				       data->buffer.buf,
> +				       ADXL355_SHADOW_REG_COUNT);
> +		if (ret)
> +			return ret;
> +	} while (memcmp(shadow_regs, data->buffer.buf,
> +			ADXL355_SHADOW_REG_COUNT));
> +
>  	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
>  				 ADXL355_POWER_CTL_DRDY_MSK,
>  				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));

Re: [PATCH v3] iio: accel: fix ADXL355 startup race condition

[Andrej Valek]

Hello Jonathan,

sorry, I didn't want to make something misleading.

On 04.10.2025 17:47, Jonathan Cameron wrote:
> On Wed,  1 Oct 2025 16:37:02 +0200
> Andrej Valek <andrej.v@skyrain.eu> wrote:
>
>> From: Valek Andrej <andrej.v@skyrain.eu>
>>
>> There is an race-condition where device is not full working after SW reset.
>> Therefore it's necessary to wait some time after reset and verify shadow
>> registers values by reading and comparing the values before/after reset.
>> This mechanism is described in datasheet at least from revision D.
>>
>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> Hi Valek,
>
> One process thing is to not reply to an existing thread with a new version.
> Whilst it seems nice and tidy when we only have one patch in a series, it can
> get very complex to read if we have deep threads with many versions being commented
> on at the same time.  Also tends to end up way off the top of the screen in
> threaded email clients.  Hence this is about the last patch I'm replying to today
> just because for once I've more or less caught up with all my IIO related emails.
>
> So just send a fresh series that isn't in reply to anything.
>
> Mostly good, but can you reply with a fixes tag and are you fine with me
> adding the error check I call out below?
I tried to follow the thread and always use the first message as 
reference, but it failed somehow. Anyhow I send a new patch without 
following the previous stuff as requested: 
https://lore.kernel.org/linux-iio/20251006095812.102230-1-andrej.v@skyrain.eu/T/#u
>
>> ---
>>   drivers/iio/accel/adxl355_core.c | 41 ++++++++++++++++++++++++++++----
>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
>> index 2e00fd51b4d5..782085d74ab8 100644
>> --- a/drivers/iio/accel/adxl355_core.c
>> +++ b/drivers/iio/accel/adxl355_core.c
>> @@ -56,6 +56,8 @@
>>   #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
>>   #define ADXL355_SELF_TEST_REG		0x2E
>>   #define ADXL355_RESET_REG		0x2F
>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
>> +#define ADXL355_SHADOW_REG_COUNT	5
>>   
>>   #define ADXL355_DEVID_AD_VAL		0xAD
>>   #define ADXL355_DEVID_MST_VAL		0x1D
>> @@ -294,6 +296,8 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
>>   static int adxl355_setup(struct adxl355_data *data)
>>   {
>>   	unsigned int regval;
>> +	u8 *shadow_regs __free(kfree) = kzalloc(ADXL355_SHADOW_REG_COUNT, GFP_KERNEL);
>> +	int retries = 5; /* the number is chosen based on empirical reasons */
>>   	int ret;
> 	if (!shadow_regs)
> 		return -ENOMEM;
>
> Whilst very unlikely to happen we still need to handle memory allocation failure.
Sure, added.
>>   
>>   	ret = regmap_read(data->regmap, ADXL355_DEVID_AD_REG, &regval);
>> @@ -321,14 +325,41 @@ static int adxl355_setup(struct adxl355_data *data)
>>   	if (regval != ADXL355_PARTID_VAL)
>>   		dev_warn(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>>   
>> -	/*
>> -	 * Perform a software reset to make sure the device is in a consistent
>> -	 * state after start-up.
>> -	 */
>> -	ret = regmap_write(data->regmap, ADXL355_RESET_REG, ADXL355_RESET_CODE);
>> +	/* Read shadow registers to be compared after reset */
>> +	ret = regmap_bulk_read(data->regmap,
>> +			       ADXL355_BASE_ADDR_SHADOW_REG,
>> +			       shadow_regs, ADXL355_SHADOW_REG_COUNT);
>>   	if (ret)
>>   		return ret;
>>   
>> +	do {
>> +		if (--retries == 0) {
>> +			dev_err(data->dev, "Shadow registers mismatch\n");
>> +			return -EIO;
>> +		}
>> +
>> +		/*
>> +		 * Perform a software reset to make sure the device is in a consistent
>> +		 * state after start-up.
>> +		 */
>> +		ret = regmap_write(data->regmap, ADXL355_RESET_REG,
>> +				   ADXL355_RESET_CODE);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Wait at least 5ms after software reset */
>> +		usleep_range(5000, 10000);
>> +
>> +		/* Read shadow registers for comparison */
>> +		ret = regmap_bulk_read(data->regmap,
>> +				       ADXL355_BASE_ADDR_SHADOW_REG,
>> +				       data->buffer.buf,
>> +				       ADXL355_SHADOW_REG_COUNT);
>> +		if (ret)
>> +			return ret;
>> +	} while (memcmp(shadow_regs, data->buffer.buf,
>> +			ADXL355_SHADOW_REG_COUNT));
>> +
>>   	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
>>   				 ADXL355_POWER_CTL_DRDY_MSK,
>>   				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
>
Now should be everything fixed, hopefully... .

BR,
Andy