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

[Andy Shevchenko]

On Fri, Oct 16, 2026 at 02:17:12PM +0300, Amir Atef via B4 Relay wrote:

> Use guard(mutex) to manage the state lock in ad9834_write() and
> ad9834_store_wavetype(). 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.

...

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

+ blank line.

>  	switch ((u32)this_attr->address) {
>  	case AD9834_REG_FREQ0:
>  	case AD9834_REG_FREQ1:

...

>  			st->control |= this_attr->address;
>  			st->control &= ~AD9834_PIN_SW;
>  		} else {
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}

You can get rid of now redundant 'else' (although I haven't checked the full
context, it might work only in the if-else case, but if-else-if-else requires
different approach).

>  		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
>  		ret = spi_sync(st->spi, &st->msg);

...

> static ssize_t ad9834_store_wavetype(struct device *dev,

>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ad9834_state *st = iio_priv(indio_dev);
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	int ret = 0;
> +	int ret;

While at it, move it to be the last in the definition block.

>  	bool is_ad9833_7 = (st->devid == ID_AD9833) || (st->devid == ID_AD9837);

...

> static ssize_t ad9834_store_wavetype(struct device *dev,

>  			st->control &= ~AD9834_MODE;
>  			st->control |= AD9834_OPBITEN;
>  		} else {
> -			ret = -EINVAL;
> +			return -EINVAL;
>  		}

Try to get rid of 'else'.

>  		break;

...

> static ssize_t ad9834_store_wavetype(struct device *dev,

>  			st->control &= ~AD9834_MODE;
>  			st->control |= AD9834_OPBITEN;
>  		} else {
> -			ret = -EINVAL;
> +			return -EINVAL;
>  		}
>  		break;

Ditto.

>  	default:
> -		ret = -EINVAL;
> -		break;
> +		return -EINVAL;
>  	}
>  
> -	if (!ret) {
> -		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> -		ret = spi_sync(st->spi, &st->msg);
> -	}
> -	mutex_unlock(&st->lock);
> +	st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +	ret = spi_sync(st->spi, &st->msg);
>  
>  	return ret ? ret : len;

Rewrite it with proper if ().

	ret = spi_sync(st->spi, &st->msg);
	if (ret)
		return ret;

	return len;

-- 
With Best Regards,
Andy Shevchenko