guard() notation allows early returns when encountering errors, making
control flow more obvious. Use it.
Also variables that now only hold error codes (or 0) are renamed to
"error" to make their purpose clearer.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/iio/adc/ti-ads7950.c | 105 ++++++++++++++++-------------------
1 file changed, 48 insertions(+), 57 deletions(-)
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 2a7d4a1d9fa9..d31397f37ec4 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -306,18 +306,17 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
struct ti_ads7950_state *st = iio_priv(indio_dev);
- int ret;
+ int error;
- mutex_lock(&st->slock);
- ret = spi_sync(st->spi, &st->ring_msg);
- if (ret < 0)
- goto out;
+ scoped_guard(mutex, &st->slock) {
+ error = spi_sync(st->spi, &st->ring_msg);
+ if (error)
+ break;
- iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
- iio_get_time_ns(indio_dev));
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
+ iio_get_time_ns(indio_dev));
+ }
-out:
- mutex_unlock(&st->slock);
iio_trigger_notify_done(indio_dev->trig);
return IRQ_HANDLED;
@@ -326,22 +325,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
{
struct ti_ads7950_state *st = iio_priv(indio_dev);
- int ret, cmd;
+ int error;
+ int cmd;
+
+ guard(mutex)(&st->slock);
- mutex_lock(&st->slock);
cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
st->single_tx = cmd;
- ret = spi_sync(st->spi, &st->scan_single_msg);
- if (ret)
- goto out;
-
- ret = st->single_rx;
-
-out:
- mutex_unlock(&st->slock);
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
- return ret;
+ return st->single_rx;
}
static int ti_ads7950_get_range(struct ti_ads7950_state *st)
@@ -407,9 +403,9 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
int value)
{
struct ti_ads7950_state *st = gpiochip_get_data(chip);
- int ret;
+ int error;
- mutex_lock(&st->slock);
+ guard(mutex)(&st->slock);
if (value)
st->cmd_settings_bitmask |= BIT(offset);
@@ -417,47 +413,44 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
st->cmd_settings_bitmask &= ~BIT(offset);
st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
- ret = spi_sync(st->spi, &st->scan_single_msg);
-
- mutex_unlock(&st->slock);
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
- return ret;
+ return 0;
}
static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
{
struct ti_ads7950_state *st = gpiochip_get_data(chip);
- int ret = 0;
bool state;
+ int error;
- mutex_lock(&st->slock);
+ guard(mutex)(&st->slock);
/* 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;
}
/* GPIO data bit sets SDO bits 12-15 to GPIO input */
st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
- ret = spi_sync(st->spi, &st->scan_single_msg);
- if (ret)
- goto out;
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
state = (st->single_rx >> 12) & BIT(offset);
/* Revert back to original settings */
st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
- ret = spi_sync(st->spi, &st->scan_single_msg);
- if (ret)
- goto out;
-
-out:
- mutex_unlock(&st->slock);
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
- return ret ?: state;
+ return state;
}
static int ti_ads7950_get_direction(struct gpio_chip *chip,
@@ -473,9 +466,9 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
int input)
{
struct ti_ads7950_state *st = gpiochip_get_data(chip);
- int ret = 0;
+ int error;
- mutex_lock(&st->slock);
+ guard(mutex)(&st->slock);
/* Only change direction if needed */
if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
@@ -483,15 +476,14 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
st->gpio_cmd_settings_bitmask |= BIT(offset);
else
- goto out;
+ return 0;
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);
+ if (error)
+ return error;
-out:
- mutex_unlock(&st->slock);
-
- return ret;
+ return 0;
}
static int ti_ads7950_direction_input(struct gpio_chip *chip,
@@ -514,27 +506,26 @@ static int ti_ads7950_direction_output(struct gpio_chip *chip,
static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
{
- int ret = 0;
+ int error;
- mutex_lock(&st->slock);
+ guard(mutex)(&st->slock);
/* Settings for Manual/Auto1/Auto2 commands */
/* Default to 5v ref */
st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
- ret = spi_sync(st->spi, &st->scan_single_msg);
- if (ret)
- goto out;
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
/* Settings for GPIO command */
st->gpio_cmd_settings_bitmask = 0x0;
st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
- ret = spi_sync(st->spi, &st->scan_single_msg);
-
-out:
- mutex_unlock(&st->slock);
+ error = spi_sync(st->spi, &st->scan_single_msg);
+ if (error)
+ return error;
- return ret;
+ return 0;
}
static int ti_ads7950_probe(struct spi_device *spi)
--
2.53.0.335.g19a08e0c02-googOn 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> guard() notation allows early returns when encountering errors, making
> control flow more obvious. Use it.
>
> Also variables that now only hold error codes (or 0) are renamed to
> "error" to make their purpose clearer.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/iio/adc/ti-ads7950.c | 105 ++++++++++++++++-------------------
> 1 file changed, 48 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 2a7d4a1d9fa9..d31397f37ec4 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -306,18 +306,17 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct ti_ads7950_state *st = iio_priv(indio_dev);
> - int ret;
> + int error;
>
> - mutex_lock(&st->slock);
> - ret = spi_sync(st->spi, &st->ring_msg);
> - if (ret < 0)
> - goto out;
> + 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);
>
> - iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> - iio_get_time_ns(indio_dev));
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> + iio_get_time_ns(indio_dev));
> + }
>
> -out:
> - mutex_unlock(&st->slock);
> iio_trigger_notify_done(indio_dev->trig);
>
> return IRQ_HANDLED;
> @@ -326,22 +325,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
> static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
> {
> struct ti_ads7950_state *st = iio_priv(indio_dev);
> - int ret, cmd;
> + int error;
> + int cmd;
> +
> + guard(mutex)(&st->slock);
>
> - mutex_lock(&st->slock);
> cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
> st->single_tx = cmd;
>
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> - if (ret)
> - goto out;
> -
> - ret = st->single_rx;
> -
> -out:
> - mutex_unlock(&st->slock);
> + error = spi_sync(st->spi, &st->scan_single_msg);
> + if (error)
> + return error;
>
> - return ret;
> + return st->single_rx;
> }
>
> static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> @@ -407,9 +403,9 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
> int value)
> {
> struct ti_ads7950_state *st = gpiochip_get_data(chip);
> - int ret;
> + int error;
>
> - mutex_lock(&st->slock);
> + guard(mutex)(&st->slock);
>
> if (value)
> st->cmd_settings_bitmask |= BIT(offset);
> @@ -417,47 +413,44 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
> st->cmd_settings_bitmask &= ~BIT(offset);
>
> st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> -
> - mutex_unlock(&st->slock);
> + error = spi_sync(st->spi, &st->scan_single_msg);
> + if (error)
> + return error;
>
> - return ret;
> + return 0;
> }
>
> static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
> {
> struct ti_ads7950_state *st = gpiochip_get_data(chip);
> - int ret = 0;
> bool state;
> + int error;
>
> - mutex_lock(&st->slock);
> + guard(mutex)(&st->slock);
>
> /* 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.
> }
>
> /* GPIO data bit sets SDO bits 12-15 to GPIO input */
> st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
> st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> - if (ret)
> - goto out;
> + error = spi_sync(st->spi, &st->scan_single_msg);
> + if (error)
> + return error;
>
> state = (st->single_rx >> 12) & BIT(offset);
>
> /* Revert back to original settings */
> st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
> st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> - if (ret)
> - goto out;
> -
> -out:
> - mutex_unlock(&st->slock);
> + error = spi_sync(st->spi, &st->scan_single_msg);
> + if (error)
> + return error;
>
> - return ret ?: state;
> + return state;
> }
>
> static int ti_ads7950_get_direction(struct gpio_chip *chip,
> @@ -473,9 +466,9 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
> int input)
> {
> struct ti_ads7950_state *st = gpiochip_get_data(chip);
> - int ret = 0;
> + int error;
>
> - mutex_lock(&st->slock);
> + guard(mutex)(&st->slock);
>
> /* Only change direction if needed */
> if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
> @@ -483,15 +476,14 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
> else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
> st->gpio_cmd_settings_bitmask |= BIT(offset);
> else
> - goto out;
> + return 0;
>
> 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.
> + if (error)
> + return error;
>
> -out:
> - mutex_unlock(&st->slock);
> -
> - return ret;
> + return 0;
> }
>
> static int ti_ads7950_direction_input(struct gpio_chip *chip,
> @@ -514,27 +506,26 @@ static int ti_ads7950_direction_output(struct gpio_chip *chip,
>
> static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
> {
> - int ret = 0;
> + int error;
>
> - mutex_lock(&st->slock);
> + guard(mutex)(&st->slock);
>
> /* Settings for Manual/Auto1/Auto2 commands */
> /* Default to 5v ref */
> st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
> st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> - if (ret)
> - goto out;
> + error = spi_sync(st->spi, &st->scan_single_msg);
> + if (error)
> + return error;
>
> /* Settings for GPIO command */
> st->gpio_cmd_settings_bitmask = 0x0;
> st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> - ret = spi_sync(st->spi, &st->scan_single_msg);
> -
> -out:
> - mutex_unlock(&st->slock);
> + error = spi_sync(st->spi, &st->scan_single_msg);
And can return directly here.
> + if (error)
> + return error;
>
> - return ret;
> + return 0;
> }
>
> static int ti_ads7950_probe(struct spi_device *spi)
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));
}
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.
> >
> > /* 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.
Thanks.
--
Dmitry
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.
>
On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote: > guard() notation allows early returns when encountering errors, making guard()() // strictly speaking > control flow more obvious. Use it. I like the change, but... > Also variables that now only hold error codes (or 0) are renamed to > "error" to make their purpose clearer. ...this does not belong to the patch. If you wish to rename, better doing it separately. This, in particular, adds undesired churn in the change making it unclear. -- With Best Regards, Andy Shevchenko
On 2/19/26 1:51 AM, Andy Shevchenko wrote: > On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote: >> guard() notation allows early returns when encountering errors, making > > guard()() > > // strictly speaking > >> control flow more obvious. Use it. > > I like the change, but... > >> Also variables that now only hold error codes (or 0) are renamed to >> "error" to make their purpose clearer. Normally I would not give my opinion on this, but since I wrote the driver originally, I will say please don't rename. I prefer to always use "ret". > > ...this does not belong to the patch. If you wish to rename, better doing > it separately. This, in particular, adds undesired churn in the change making > it unclear. >
On Sat, Feb 21, 2026 at 11:20:42AM -0600, David Lechner wrote:
> On 2/19/26 1:51 AM, Andy Shevchenko wrote:
> > On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
> >> guard() notation allows early returns when encountering errors, making
> >
> > guard()()
> >
> > // strictly speaking
> >
> >> control flow more obvious. Use it.
> >
> > I like the change, but...
> >
> >> Also variables that now only hold error codes (or 0) are renamed to
> >> "error" to make their purpose clearer.
>
> Normally I would not give my opinion on this, but since I wrote the driver
> originally, I will say please don't rename. I prefer to always use "ret".
I hope I can convince you otherwise.
IMO "ret" or "retval" should be used when the returned value is intended
to be used during normal operation. For cases where we only expect to
have an error or 0 for success "error" or "err" is more appropriate.
This allows you to write
error = do_action(...);
if (error) {
// handle error somehow, typically simply report.
}
This also helps when reading the code as you know that there is usually
no reason to care about the specific value in this variable (maybe
except -EPROBE_DEFER).
I will push the conversion ret -> error to the very last patch so it can
easily be dropped if I am unsuccessful in swaying your opinion.
Thanks.
--
Dmitry
On 2/22/26 3:31 PM, Dmitry Torokhov wrote:
> On Sat, Feb 21, 2026 at 11:20:42AM -0600, David Lechner wrote:
>> On 2/19/26 1:51 AM, Andy Shevchenko wrote:
>>> On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
>>>> guard() notation allows early returns when encountering errors, making
>>>
>>> guard()()
>>>
>>> // strictly speaking
>>>
>>>> control flow more obvious. Use it.
>>>
>>> I like the change, but...
>>>
>>>> Also variables that now only hold error codes (or 0) are renamed to
>>>> "error" to make their purpose clearer.
>>
>> Normally I would not give my opinion on this, but since I wrote the driver
>> originally, I will say please don't rename. I prefer to always use "ret".
>
> I hope I can convince you otherwise.
I'm afraid not. "ret" is used > 35k times in IIO and err is used < 3k times
and error < 1k times. So I am really used to seeing only "ret" for errors and
"error" looks very unusual to my eyes because of this.
You mentioned valuing the common pattern in your other reply, so I hope
you can understand that this is what I value more here.
>
> IMO "ret" or "retval" should be used when the returned value is intended
> to be used during normal operation. For cases where we only expect to
> have an error or 0 for success "error" or "err" is more appropriate.
> This allows you to write
>
> error = do_action(...);
> if (error) {
> // handle error somehow, typically simply report.
> }
>
> This also helps when reading the code as you know that there is usually
> no reason to care about the specific value in this variable (maybe
> except -EPROBE_DEFER).
>
> I will push the conversion ret -> error to the very last patch so it can
> easily be dropped if I am unsuccessful in swaying your opinion.
>
> Thanks.
>
To give you fair warning, it will still be NAK from me.
© 2016 - 2026 Red Hat, Inc.