Replace usage of error_ret labels by returning directly when handling
errors. Cases that do a mutex unlock were not changed.
Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
drivers/iio/accel/sca3000.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index aabe4491efd7..bfa8a3f5a92f 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -369,23 +369,20 @@ static int sca3000_write_ctrl_reg(struct sca3000_state *st,
ret = sca3000_reg_lock_on(st);
if (ret < 0)
- goto error_ret;
+ return ret;
if (ret) {
ret = __sca3000_unlock_reg_lock(st);
if (ret)
- goto error_ret;
+ return ret;
}
/* Set the control select register */
ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, sel);
if (ret)
- goto error_ret;
+ return ret;
/* Write the actual value into the register */
- ret = sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
-
-error_ret:
- return ret;
+ return sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
}
/**
@@ -402,22 +399,20 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
ret = sca3000_reg_lock_on(st);
if (ret < 0)
- goto error_ret;
+ return ret;
if (ret) {
ret = __sca3000_unlock_reg_lock(st);
if (ret)
- goto error_ret;
+ return ret;
}
/* Set the control select register */
ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
if (ret)
- goto error_ret;
+ return ret;
ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
if (ret)
- goto error_ret;
+ return ret;
return st->rx[0];
-error_ret:
- return ret;
}
/**
@@ -577,7 +572,8 @@ static inline int __sca3000_get_base_freq(struct sca3000_state *st,
ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
if (ret)
- goto error_ret;
+ return ret;
+
switch (SCA3000_REG_MODE_MODE_MASK & st->rx[0]) {
case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
*base_freq = info->measurement_mode_freq;
@@ -591,7 +587,6 @@ static inline int __sca3000_get_base_freq(struct sca3000_state *st,
default:
ret = -EINVAL;
}
-error_ret:
return ret;
}
@@ -834,7 +829,7 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
val = st->rx[0];
mutex_unlock(&st->lock);
if (ret)
- goto error_ret;
+ return ret;
switch (val & SCA3000_REG_MODE_MODE_MASK) {
case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
@@ -857,8 +852,6 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
break;
}
return len;
-error_ret:
- return ret;
}
/*
--
2.49.0On Wed, Jun 11, 2025 at 04:39:19PM -0300, Andrew Ijano wrote: > Replace usage of error_ret labels by returning directly when handling > errors. Cases that do a mutex unlock were not changed. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> -- With Best Regards, Andy Shevchenko
On Thu, 12 Jun 2025 15:56:26 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Jun 11, 2025 at 04:39:19PM -0300, Andrew Ijano wrote: > > Replace usage of error_ret labels by returning directly when handling > > errors. Cases that do a mutex unlock were not changed. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Applied this patch to the togreg branch of iio.git which is initially pushed out as testing. Thanks, Jonathan >
On Sat, Jun 14, 2025 at 8:48 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 12 Jun 2025 15:56:26 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > ... > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Applied this patch to the togreg branch of iio.git which is > initially pushed out as testing. > Thank you, Nino, Andy and Jonathan for the review! I'll address the comments and send a new version of the patchset for paches #2 and #3 then. Thanks, Andrew
On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> Replace usage of error_ret labels by returning directly when handling
> errors. Cases that do a mutex unlock were not changed.
>
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> ---
Code looks good. But since you're doing this you could cleanup some of the switch()
cases. Some return in every case statement while other don't (even think I saw one
one place where 'return' in the end was not needed). IIRC, there's preference for
returning in place.
- Nuno Sá
> drivers/iio/accel/sca3000.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index aabe4491efd7..bfa8a3f5a92f 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -369,23 +369,20 @@ static int sca3000_write_ctrl_reg(struct sca3000_state *st,
>
> ret = sca3000_reg_lock_on(st);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> if (ret) {
> ret = __sca3000_unlock_reg_lock(st);
> if (ret)
> - goto error_ret;
> + return ret;
> }
>
> /* Set the control select register */
> ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, sel);
> if (ret)
> - goto error_ret;
> + return ret;
>
> /* Write the actual value into the register */
> - ret = sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
> -
> -error_ret:
> - return ret;
> + return sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
> }
>
> /**
> @@ -402,22 +399,20 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
>
> ret = sca3000_reg_lock_on(st);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> if (ret) {
> ret = __sca3000_unlock_reg_lock(st);
> if (ret)
> - goto error_ret;
> + return ret;
> }
> /* Set the control select register */
> ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
> if (ret)
> - goto error_ret;
> + return ret;
> ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
> if (ret)
> - goto error_ret;
> + return ret;
> return st->rx[0];
> -error_ret:
> - return ret;
> }
>
> /**
> @@ -577,7 +572,8 @@ static inline int __sca3000_get_base_freq(struct sca3000_state
> *st,
>
> ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> if (ret)
> - goto error_ret;
> + return ret;
> +
> switch (SCA3000_REG_MODE_MODE_MASK & st->rx[0]) {
> case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
> *base_freq = info->measurement_mode_freq;
> @@ -591,7 +587,6 @@ static inline int __sca3000_get_base_freq(struct sca3000_state
> *st,
> default:
> ret = -EINVAL;
> }
> -error_ret:
> return ret;
> }
>
> @@ -834,7 +829,7 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> val = st->rx[0];
> mutex_unlock(&st->lock);
> if (ret)
> - goto error_ret;
> + return ret;
>
> switch (val & SCA3000_REG_MODE_MODE_MASK) {
> case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
> @@ -857,8 +852,6 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> break;
> }
> return len;
> -error_ret:
> - return ret;
> }
>
> /*
On Thu, 2025-06-12 at 07:20 +0100, Nuno Sá wrote:
> On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> > Replace usage of error_ret labels by returning directly when handling
> > errors. Cases that do a mutex unlock were not changed.
> >
> > Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> > Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> > Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > ---
>
> Code looks good. But since you're doing this you could cleanup some of the switch()
> cases. Some return in every case statement while other don't (even think I saw one
> one place where 'return' in the end was not needed). IIRC, there's preference for
> returning in place.
>
I see the above could be a bit cumbersome in cases there's locking (which get's
cleaned up in patch 3). So, nevermind the above. If there's any leftover, you can
send a follow up patch or introduce a new patch if you need to re-spin.
For this one:
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
>
> > drivers/iio/accel/sca3000.c | 29 +++++++++++------------------
> > 1 file changed, 11 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index aabe4491efd7..bfa8a3f5a92f 100644
> > --- a/drivers/iio/accel/sca3000.c
> > +++ b/drivers/iio/accel/sca3000.c
> > @@ -369,23 +369,20 @@ static int sca3000_write_ctrl_reg(struct sca3000_state *st,
> >
> > ret = sca3000_reg_lock_on(st);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > if (ret) {
> > ret = __sca3000_unlock_reg_lock(st);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > }
> >
> > /* Set the control select register */
> > ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, sel);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > /* Write the actual value into the register */
> > - ret = sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
> > -
> > -error_ret:
> > - return ret;
> > + return sca3000_write_reg(st, SCA3000_REG_CTRL_DATA_ADDR, val);
> > }
> >
> > /**
> > @@ -402,22 +399,20 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
> >
> > ret = sca3000_reg_lock_on(st);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > if (ret) {
> > ret = __sca3000_unlock_reg_lock(st);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > }
> > /* Set the control select register */
> > ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > return st->rx[0];
> > -error_ret:
> > - return ret;
> > }
> >
> > /**
> > @@ -577,7 +572,8 @@ static inline int __sca3000_get_base_freq(struct
> > sca3000_state
> > *st,
> >
> > ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > +
> > switch (SCA3000_REG_MODE_MODE_MASK & st->rx[0]) {
> > case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
> > *base_freq = info->measurement_mode_freq;
> > @@ -591,7 +587,6 @@ static inline int __sca3000_get_base_freq(struct
> > sca3000_state
> > *st,
> > default:
> > ret = -EINVAL;
> > }
> > -error_ret:
> > return ret;
> > }
> >
> > @@ -834,7 +829,7 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> > val = st->rx[0];
> > mutex_unlock(&st->lock);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > switch (val & SCA3000_REG_MODE_MODE_MASK) {
> > case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
> > @@ -857,8 +852,6 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> > break;
> > }
> > return len;
> > -error_ret:
> > - return ret;
> > }
> >
> > /*
>
On Thu, Jun 12, 2025 at 4:41 AM Nuno Sá <noname.nuno@gmail.com> wrote: > ... > > > > Code looks good. But since you're doing this you could cleanup some of the switch() > > cases. Some return in every case statement while other don't (even think I saw one > > one place where 'return' in the end was not needed). IIRC, there's preference for > > returning in place. > > > > I see the above could be a bit cumbersome in cases there's locking (which get's > cleaned up in patch 3). So, nevermind the above. If there's any leftover, you can > send a follow up patch or introduce a new patch if you need to re-spin. > Great! That's the idea, I addressed these cases in patch #3, but I'll double check if there is any leftover! Thanks, Andrew
© 2016 - 2025 Red Hat, Inc.