iio: filter: admv8818: Bug fix and other improvements

[PATCH 2/3] iio: filter: admv8818: Simplify locking with guard()

Posted by Ethan Tidmore 1 month, 1 week ago

Use guard() instead of manual locking to simplify code.

Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
 drivers/iio/filter/admv8818.c | 59 ++++++++++++-----------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
index 4d5e7a9d806a..2d12700d717f 100644
--- a/drivers/iio/filter/admv8818.c
+++ b/drivers/iio/filter/admv8818.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/iio/iio.h>
@@ -205,13 +206,8 @@ static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 
 static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 {
-	int ret;
-
-	mutex_lock(&st->lock);
-	ret = __admv8818_hpf_select(st, freq);
-	mutex_unlock(&st->lock);
-
-	return ret;
+	guard(mutex)(&st->lock);
+	return __admv8818_hpf_select(st, freq);
 }
 
 static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
@@ -281,13 +277,8 @@ static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 
 static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 {
-	int ret;
-
-	mutex_lock(&st->lock);
-	ret = __admv8818_lpf_select(st, freq);
-	mutex_unlock(&st->lock);
-
-	return ret;
+	guard(mutex)(&st->lock);
+	return __admv8818_lpf_select(st, freq);
 }
 
 static int admv8818_rfin_band_select(struct admv8818_state *st)
@@ -308,16 +299,17 @@ static int admv8818_rfin_band_select(struct admv8818_state *st)
 	if (lpf_corner_target < st->cf_hz)
 		lpf_corner_target = U64_MAX;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = __admv8818_hpf_select(st, hpf_corner_target);
 	if (ret)
-		goto exit;
+		return ret;
 
 	ret = __admv8818_lpf_select(st, lpf_corner_target);
-exit:
-	mutex_unlock(&st->lock);
-	return ret;
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
@@ -352,13 +344,8 @@ static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
 
 static int admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
 {
-	int ret;
-
-	mutex_lock(&st->lock);
-	ret = __admv8818_read_hpf_freq(st, hpf_freq);
-	mutex_unlock(&st->lock);
-
-	return ret;
+	guard(mutex)(&st->lock);
+	return __admv8818_read_hpf_freq(st, hpf_freq);
 }
 
 static int __admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
@@ -393,13 +380,8 @@ static int __admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
 
 static int admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
 {
-	int ret;
-
-	mutex_lock(&st->lock);
-	ret = __admv8818_read_lpf_freq(st, lpf_freq);
-	mutex_unlock(&st->lock);
-
-	return ret;
+	guard(mutex)(&st->lock);
+	return __admv8818_read_lpf_freq(st, lpf_freq);
 }
 
 static int admv8818_write_raw_get_fmt(struct iio_dev *indio_dev,
@@ -485,7 +467,7 @@ static int admv8818_filter_bypass(struct admv8818_state *st)
 {
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
 				 ADMV8818_SW_IN_SET_WR0_MSK |
@@ -497,18 +479,17 @@ static int admv8818_filter_bypass(struct admv8818_state *st)
 				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
 				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, 0));
 	if (ret)
-		goto exit;
+		return ret;
 
 	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
 				 ADMV8818_HPF_WR0_MSK |
 				 ADMV8818_LPF_WR0_MSK,
 				 FIELD_PREP(ADMV8818_HPF_WR0_MSK, 0) |
 				 FIELD_PREP(ADMV8818_LPF_WR0_MSK, 0));
+	if (ret)
+		return ret;
 
-exit:
-	mutex_unlock(&st->lock);
-
-	return ret;
+	return 0;
 }
 
 static int admv8818_get_mode(struct iio_dev *indio_dev,
-- 
2.53.0

Re: [PATCH 2/3] iio: filter: admv8818: Simplify locking with guard()

Posted by David Lechner 1 month, 1 week ago

On 2/27/26 12:14 AM, Ethan Tidmore wrote:
> Use guard() instead of manual locking to simplify code.
> 
> Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
> ---

> @@ -352,13 +344,8 @@ static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
>  
>  static int admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
>  {
> -	int ret;
> -
> -	mutex_lock(&st->lock);
> -	ret = __admv8818_read_hpf_freq(st, hpf_freq);
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> +	guard(mutex)(&st->lock);
> +	return __admv8818_read_hpf_freq(st, hpf_freq);
>  }

admv8818_read_hpf_freq() is the only caller of __admv8818_read_hpf_freq()
so we can drop the wrapper, move the guard to __admv8818_read_hpf_freq()
and rename it to admv8818_read_hpf_freq().

I didn't check all functions, but might be others places where we can do
this as well.

Re: [PATCH 2/3] iio: filter: admv8818: Simplify locking with guard()

Posted by Ethan Tidmore 1 month, 1 week ago

On Fri Feb 27, 2026 at 10:08 AM CST, David Lechner wrote:
> On 2/27/26 12:14 AM, Ethan Tidmore wrote:
>> Use guard() instead of manual locking to simplify code.
>> 
>> Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
>> ---
>
>
>> @@ -352,13 +344,8 @@ static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
>>  
>>  static int admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
>>  {
>> -	int ret;
>> -
>> -	mutex_lock(&st->lock);
>> -	ret = __admv8818_read_hpf_freq(st, hpf_freq);
>> -	mutex_unlock(&st->lock);
>> -
>> -	return ret;
>> +	guard(mutex)(&st->lock);
>> +	return __admv8818_read_hpf_freq(st, hpf_freq);
>>  }
>
> admv8818_read_hpf_freq() is the only caller of __admv8818_read_hpf_freq()
> so we can drop the wrapper, move the guard to __admv8818_read_hpf_freq()
> and rename it to admv8818_read_hpf_freq().
>
> I didn't check all functions, but might be others places where we can do
> this as well.

So this can be easily done with a few wrappers but these two functions
have this odd scenario:

	guard(mutex)(&st->lock);

	ret = __admv8818_hpf_select(st, hpf_corner_target);
	if (ret)
		return ret;

	return __admv8818_lpf_select(st, lpf_corner_target);

Where if I deleted that guard() and just made those hold the lock
separately this would be a behaviour change. Wondering if there was a
nice way to get around this?

Thanks,

ET

Re: [PATCH 2/3] iio: filter: admv8818: Simplify locking with guard()

Posted by Andy Shevchenko 1 month, 1 week ago

On Fri, Feb 27, 2026 at 12:14:23AM -0600, Ethan Tidmore wrote:
> Use guard() instead of manual locking to simplify code.

...

>  	ret = __admv8818_lpf_select(st, lpf_corner_target);
> -exit:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Why not

	return __admv8818_lpf_select(...);

?

...

>  	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
>  				 ADMV8818_HPF_WR0_MSK |
>  				 ADMV8818_LPF_WR0_MSK,
>  				 FIELD_PREP(ADMV8818_HPF_WR0_MSK, 0) |
>  				 FIELD_PREP(ADMV8818_LPF_WR0_MSK, 0));
> +	if (ret)
> +		return ret;
>  
> -exit:
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> +	return 0;

In the similar way?

-- 
With Best Regards,
Andy Shevchenko