Re: [PATCH 2/5] staging: iio: ad9832: convert to guard(mutex)

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Dec 2025 12:16:32 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> On Thu, Dec 18, 2025 at 11:34:05AM -0300, Marcelo Schmitt wrote:
> > On 12/15, Tomas Borquez wrote:  
> ...
> > > -	mutex_lock(&st->lock);
> > > +	guard(mutex)(&st->lock);
> > >  	switch ((u32)this_attr->address) {
> > >  	case AD9832_FREQ0HM:
> > >  	case AD9832_FREQ1HM:
> > > @@ -203,22 +205,18 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> > >  		ret = spi_sync(st->spi, &st->msg);
> > >  		break;
> > >  	case AD9832_FREQ_SYM:
> > > -		if (val == 1 || val == 0) {
> > > -			st->ctrl_fp &= ~AD9832_FREQ;
> > > -			st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> > > -		} else {
> > > -			ret = -EINVAL;
> > > -			break;
> > > -		}
> > > +		if (val != 1 && val != 0)
> > > +			return -EINVAL;
> > > +
> > > +		st->ctrl_fp &= ~AD9832_FREQ;
> > > +		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> > >  		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
> > >  						  st->ctrl_fp);
> > >  		ret = spi_sync(st->spi, &st->msg);
> > >  		break;  
> > Since we now have the mutex unlock handled by guard, why not returning directly
> > from each case?
> > E.g.
> > 	case AD9832_FREQ1HM:
> > -		ret = ad9832_write_frequency(st, this_attr->address, val);
> > -		break;
> > +		return ad9832_write_frequency(st, this_attr->address, val);
> > I think the last return (outside the default clause) won't be needed anymore.  
> Wouldn't work because we need to return len too, so it would be more like:
> 
>  case AD9832_FREQ1HM:
> 		 ret = ad9832_write_frequency(st, this_attr->address, val);
> +    return ret ?: len;

I'd prefer the error dealt with locally as it's more consistent with the other
paths in the code where we return directly.  The fact this is the last one
in each block doesn't to me mean we should bother to handle it in a special
way.

You could stick to
		if (ret)
			return ret;

		break;
	...
	}

	return len;

If you prefer to keep that shared return of len in the good path. Or just
return it as you suggested in each case: block using the ternary.

Jonathan



> 
> Which is a bit more repetitive than just returning at the end ret ?: len,
> but lemme know what you think.
> 
> >
> > And, since you are touching the lock, you may also update to use
> > devm_mutex_init() (that would probably be best appreciated as a separate patch).  
> 
> Yup, sounds good :)
> 
> Tomas