iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

[PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by Antoniu Miclaus 1 month, 1 week ago

FIELD_PREP(BIT(0), fifo_samples & BIT(8)) produces either 0 or 256,
and since FIELD_PREP masks to bit 0, 256 & 1 evaluates to 0. Use !!
to convert the result to a proper 0-or-1 value.

Fixes: df36de13677a ("iio: accel: add ADXL380 driver")
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/accel/adxl380.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
index 8fab2fdbe147..a51d1d61c412 100644
--- a/drivers/iio/accel/adxl380.c
+++ b/drivers/iio/accel/adxl380.c
@@ -877,7 +877,7 @@ static int adxl380_set_fifo_samples(struct adxl380_state *st)
 	ret = regmap_update_bits(st->regmap, ADXL380_FIFO_CONFIG_0_REG,
 				 ADXL380_FIFO_SAMPLES_8_MSK,
 				 FIELD_PREP(ADXL380_FIFO_SAMPLES_8_MSK,
-					    (fifo_samples & BIT(8))));
+					    !!(fifo_samples & BIT(8))));
 	if (ret)
 		return ret;
 
-- 
2.43.0

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by David Lechner 1 month, 1 week ago

On 2/27/26 6:43 AM, Antoniu Miclaus wrote:
> FIELD_PREP(BIT(0), fifo_samples & BIT(8)) produces either 0 or 256,
> and since FIELD_PREP masks to bit 0, 256 & 1 evaluates to 0. Use !!
> to convert the result to a proper 0-or-1 value.
> 
> Fixes: df36de13677a ("iio: accel: add ADXL380 driver")
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  drivers/iio/accel/adxl380.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
> index 8fab2fdbe147..a51d1d61c412 100644
> --- a/drivers/iio/accel/adxl380.c
> +++ b/drivers/iio/accel/adxl380.c
> @@ -877,7 +877,7 @@ static int adxl380_set_fifo_samples(struct adxl380_state *st)

Some relevant context...

	u16 fifo_samples = st->watermark * st->fifo_set_size;

>  	ret = regmap_update_bits(st->regmap, ADXL380_FIFO_CONFIG_0_REG,
>  				 ADXL380_FIFO_SAMPLES_8_MSK,
>  				 FIELD_PREP(ADXL380_FIFO_SAMPLES_8_MSK,
> -					    (fifo_samples & BIT(8))));
> +					    !!(fifo_samples & BIT(8))));

Technically, this works, but in terms of understanding the code I think
fifo_samples >= BIT(8) would make more sense.

fifo_samples is a count, not bit flags.


>  	if (ret)
>  		return ret;
>

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by Andy Shevchenko 1 month ago

On Sat, Feb 28, 2026 at 10:50:00AM -0600, David Lechner wrote:
> On 2/27/26 6:43 AM, Antoniu Miclaus wrote:

...

> > +					    !!(fifo_samples & BIT(8))));
> 
> Technically, this works, but in terms of understanding the code I think
> fifo_samples >= BIT(8) would make more sense.
> 
> fifo_samples is a count, not bit flags.

I even would prefer to see in such a case

	fifo_samples > (BIT(8) - 1)

that it will define the maximum that fits the HW, or plain number

	fifo_samples > 127

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by David Lechner 1 month ago

On 3/2/26 1:56 AM, Andy Shevchenko wrote:
> On Sat, Feb 28, 2026 at 10:50:00AM -0600, David Lechner wrote:
>> On 2/27/26 6:43 AM, Antoniu Miclaus wrote:
> 
> ...
> 
>>> +					    !!(fifo_samples & BIT(8))));
>>
>> Technically, this works, but in terms of understanding the code I think
>> fifo_samples >= BIT(8) would make more sense.
>>
>> fifo_samples is a count, not bit flags.
> 
> I even would prefer to see in such a case
> 
> 	fifo_samples > (BIT(8) - 1)
> 
> that it will define the maximum that fits the HW, or plain number
> 
> 	fifo_samples > 127
> 

Now that I looked at the dataheet, I get Jonathan's point.

The reason this is here is that we are filling in a 9-bit value
using 8-bit registers, so the MSB goes in a different register.

So actually, it would probably make the most sense to write
it as `fifo_samples >> 8`.

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by Andy Shevchenko 1 month ago

On Mon, Mar 02, 2026 at 08:54:52AM -0600, David Lechner wrote:
> On 3/2/26 1:56 AM, Andy Shevchenko wrote:
> > On Sat, Feb 28, 2026 at 10:50:00AM -0600, David Lechner wrote:
> >> On 2/27/26 6:43 AM, Antoniu Miclaus wrote:

...

> >>> +					    !!(fifo_samples & BIT(8))));
> >>
> >> Technically, this works, but in terms of understanding the code I think
> >> fifo_samples >= BIT(8) would make more sense.
> >>
> >> fifo_samples is a count, not bit flags.
> > 
> > I even would prefer to see in such a case
> > 
> > 	fifo_samples > (BIT(8) - 1)
> > 
> > that it will define the maximum that fits the HW, or plain number
> > 
> > 	fifo_samples > 127
> 
> Now that I looked at the dataheet, I get Jonathan's point.
> 
> The reason this is here is that we are filling in a 9-bit value
> using 8-bit registers, so the MSB goes in a different register.
> 
> So actually, it would probably make the most sense to write
> it as `fifo_samples >> 8`.

I agree based on the above justification.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by Jonathan Cameron 1 month, 1 week ago

On Sat, 28 Feb 2026 10:50:00 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 2/27/26 6:43 AM, Antoniu Miclaus wrote:
> > FIELD_PREP(BIT(0), fifo_samples & BIT(8)) produces either 0 or 256,
> > and since FIELD_PREP masks to bit 0, 256 & 1 evaluates to 0. Use !!
> > to convert the result to a proper 0-or-1 value.
> > 
> > Fixes: df36de13677a ("iio: accel: add ADXL380 driver")
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  drivers/iio/accel/adxl380.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
> > index 8fab2fdbe147..a51d1d61c412 100644
> > --- a/drivers/iio/accel/adxl380.c
> > +++ b/drivers/iio/accel/adxl380.c
> > @@ -877,7 +877,7 @@ static int adxl380_set_fifo_samples(struct adxl380_state *st)  
> 
> Some relevant context...
> 
> 	u16 fifo_samples = st->watermark * st->fifo_set_size;
> 
> >  	ret = regmap_update_bits(st->regmap, ADXL380_FIFO_CONFIG_0_REG,
> >  				 ADXL380_FIFO_SAMPLES_8_MSK,
> >  				 FIELD_PREP(ADXL380_FIFO_SAMPLES_8_MSK,
> > -					    (fifo_samples & BIT(8))));
> > +					    !!(fifo_samples & BIT(8))));  
> 
> Technically, this works, but in terms of understanding the code I think
> fifo_samples >= BIT(8) would make more sense.
> 
> fifo_samples is a count, not bit flags.
It's an odd bit of code, but then it's unusual hardware too.
To me it is about bit 8 not the value being big enough to have a bit 8 because
if there were yet another register that was for BIT(9) then
 >= BIT(8) would give the wrong value if BIT(9) was set and BIT(8) was not.

We could express this what is going on in a bit more explicit detail maybe?

	FIELD_PREP(ADXL380_FIFO_SAMPLES_8_MSK,
		   FIELD_GET(BIT(8), fifo_samples));

So there we are extracting the 8th bit and writing to the location for the 8th
bit.
> 
> 
> >  	if (ret)
> >  		return ret;
> >    
>

Re: [PATCH] iio: accel: adxl380: fix FIFO watermark bit 8 always written as 0

Posted by Jonathan Cameron 1 month, 1 week ago

On Fri, 27 Feb 2026 14:43:05 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> FIELD_PREP(BIT(0), fifo_samples & BIT(8)) produces either 0 or 256,
> and since FIELD_PREP masks to bit 0, 256 & 1 evaluates to 0. Use !!
> to convert the result to a proper 0-or-1 value.
> 
> Fixes: df36de13677a ("iio: accel: add ADXL380 driver")
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Applied and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adxl380.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
> index 8fab2fdbe147..a51d1d61c412 100644
> --- a/drivers/iio/accel/adxl380.c
> +++ b/drivers/iio/accel/adxl380.c
> @@ -877,7 +877,7 @@ static int adxl380_set_fifo_samples(struct adxl380_state *st)
>  	ret = regmap_update_bits(st->regmap, ADXL380_FIFO_CONFIG_0_REG,
>  				 ADXL380_FIFO_SAMPLES_8_MSK,
>  				 FIELD_PREP(ADXL380_FIFO_SAMPLES_8_MSK,
> -					    (fifo_samples & BIT(8))));
> +					    !!(fifo_samples & BIT(8))));
>  	if (ret)
>  		return ret;
>