Re: [PATCH v2] staging: iio: frequency: ad9834: use guard(mutex)

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 17 Oct 2026 02:45:52 +0300
Amir Atef via B4 Relay <devnull+amiratefabdallah1122.gmail.com@kernel.org> wrote:

> From: Amir Atef <amiratefabdallah1122@gmail.com>
> 
> Use guard(mutex) to manage the state lock in ad9834_write() and
> ad9834_store_wavetype(). This simplifies error handling by allowing
> early returns instead of setting the return code and breaking out of the
> switch statement to manually unlock the mutex.
> 
> Signed-off-by: Amir Atef <amiratefabdallah1122@gmail.com>
Hi Amir,

One small additional thing inline.

Thanks,

Jonathan

> ---
> Changes in v2:
> - Add blank line after guard(mutex) in ad9834_write().
> - Move 'int ret;' to the end of the variable declaration block in
>   ad9834_store_wavetype().
> - Drop redundant 'else' in ad9834_store_wavetype() case 1 by inverting
>   the condition (kept 'else' in ad9834_write() and case 0 where they are
>   part of if-else-if-else chains).
> - Replace ternary return with explicit 'if (ret) return ret;' in
>   ad9834_store_wavetype().
> - Link to v1: https://lore.kernel.org/r/179214943270.333.5921549521929682753.20261016-ad9834-guard-mutex@gmail.com
> 
>  drivers/staging/iio/frequency/ad9834.c | 50 ++++++++++++--------------
>  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index 0d20f796e0f0..c45bb388c998 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/dev_printk.h>
>  #include <linux/err.h>
> @@ -154,7 +155,8 @@ static ssize_t ad9834_write(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
> +
>  	switch ((u32)this_attr->address) {
>  	case AD9834_REG_FREQ0:
>  	case AD9834_REG_FREQ1:
> @@ -165,10 +167,8 @@ static ssize_t ad9834_write(struct device *dev,
>  		ret = ad9834_write_phase(st, this_attr->address, val);
>  		break;
>  	case AD9834_OPBITEN:
> -		if (st->control & AD9834_MODE) {
> -			ret = -EINVAL;  /* AD9834 reserved mode */
> -			break;
> -		}
> +		if (st->control & AD9834_MODE)
> +			return -EINVAL;  /* AD9834 reserved mode */
>  
>  		if (val)
>  			st->control |= AD9834_OPBITEN;
> @@ -194,8 +194,7 @@ static ssize_t ad9834_write(struct device *dev,
>  			st->control |= this_attr->address;
>  			st->control &= ~AD9834_PIN_SW;
>  		} else {
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
>  		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
>  		ret = spi_sync(st->spi, &st->msg);
> @@ -210,9 +209,8 @@ static ssize_t ad9834_write(struct device *dev,
>  		ret = spi_sync(st->spi, &st->msg);

I'd rather see all error handled at point where they originate, rather
than only some (with special case for the last call) so here and similar places, do 
		if (ret)
			return ret;

		break;

Or you could even do a return len; here if you prefer.

>  		break;
>  	default:
> -		ret = -ENODEV;
> +		return -ENODEV;
>  	}
> -	mutex_unlock(&st->lock);
>  
>  	return ret ? ret : len;
>  }