Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[David Lechner]

On 2/22/26 3:37 PM, Dmitry Torokhov wrote:
> On Sat, Feb 21, 2026 at 11:34:33AM -0600, David Lechner wrote:
>> On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
>>> +	scoped_guard(mutex, &st->slock) {
>>> +		error = spi_sync(st->spi, &st->ring_msg);
>>> +		if (error)
>>> +			break;
>>
>> I'm not a fan of scoped_guard() because of the hidden for loop in it.
>> It hides the fact that the break; is breaking out of that for loop.
>>
>> It would be more clear/obvious written as:
>>
>> 	do {
>> 		guard(mutex)(&st->slock);
>>
>> 		ret = spi_sync(st->spi, &st->ring_msg);
>> 		if (ret)
>> 			break;
>>
>> 		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
>> 						   iio_get_time_ns(indio_dev));
>> 	} while (0);
> 
> OK.
> 
> I could also make it
> 
> 	scoped_guard(mutex, &st->slock) {
> 		ret = spi_sync(st->spi, &st->ring_msg);
> 		if (!ret)
> 			iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> 							   iio_get_time_ns(indio_dev));
> 	}

This is OK.

> 
> to avoid using "break".
> 
> I think you will find that scoped_guard() will gain the foothold in the
> kernel so having implementation that does not follow common pattern
> might not be the best option.

In general, I agree with this idea.

In this case, the common pattern unfortunately leads to common bugs.
Since we were discussing this, I audited the kernel and found and 
reported 3 unintentional mistakes with break/continue when scoped_guard()
was introduced. And I have caught more in reviews before they made it
into the kernel.

So I make an exception here to thinking that the common pattern is best.

> 
>>>  
>>>  	/* If set as output, return the output */
>>>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
>>>  		state = st->cmd_settings_bitmask & BIT(offset);
>>> -		goto out;
>>> +		return state;
>>
>> This can return directly instead of using local variable.
> 
> This will require the explicitly normalizing, which we avoided by
> introducing "bool state" to begin with...
> 
>>>  
>>>  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
>>> -	ret = spi_sync(st->spi, &st->scan_single_msg);
>>> +	error = spi_sync(st->spi, &st->scan_single_msg);
>>
>> Can just return directly here now.
> 
> I think there is benefit in explicitly calling out the error paths and
> explicitly return 0 on success. It removes the doubt whether a function
> can return positive value on success.

In the IIO subsystem, the direct return is preferred (maintainer and
other reviewers frequently request this).

> 
> Thanks.
>