* [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure
@ 2025-06-11 19:39 Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-11 19:39 UTC (permalink / raw)
To: jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
The sca3000 driver is old and could be simplified by using newer
infrastructure.
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>
---
v4 -> v5:
- break up the changes in three patches
- replace error_ret labels by simple returns
- use spi_w8r16be() for be16 reads
- use guard(mutex) for handling mutex lock
v3 -> v4:
- clean the code and remove redundant operations
v2 -> v3:
- replace usages of internal read data helpers by spi helpers
v1 -> v2:
- simplify the return of the internal read data function
---
Andrew Ijano (3):
iio: accel: sca3000: replace error_ret labels by simple returns
iio: accel: sca3000: replace usages of internal read data helpers by
spi helpers
iio: accel: sca3000: use guard(mutex)() for handling mutex lock
drivers/iio/accel/sca3000.c | 366 +++++++++++++-----------------------
1 file changed, 133 insertions(+), 233 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-11 19:39 [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure Andrew Ijano
@ 2025-06-11 19:39 ` Andrew Ijano
2025-06-12 6:20 ` Nuno Sá
2025-06-12 12:56 ` Andy Shevchenko
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2 siblings, 2 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-11 19:39 UTC (permalink / raw)
To: jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
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.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-11 19:39 [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
@ 2025-06-11 19:39 ` Andrew Ijano
2025-06-12 6:29 ` Nuno Sá
2025-06-12 13:22 ` Andy Shevchenko
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2 siblings, 2 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-11 19:39 UTC (permalink / raw)
To: jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
Remove usages of sca3000_read_data() and sca3000_read_data_short()
functions, replacing it by spi_w8r8() and spi_w8r16be() helpers. Just
one case that reads large buffers is left using an internal helper.
This is an old driver that was not making full use of the newer
infrastructure.
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 | 166 +++++++++++++++---------------------
1 file changed, 68 insertions(+), 98 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index bfa8a3f5a92f..d41759c68fb4 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -281,24 +281,6 @@ static int sca3000_write_reg(struct sca3000_state *st, u8 address, u8 val)
return spi_write(st->us, st->tx, 2);
}
-static int sca3000_read_data_short(struct sca3000_state *st,
- u8 reg_address_high,
- int len)
-{
- struct spi_transfer xfer[2] = {
- {
- .len = 1,
- .tx_buf = st->tx,
- }, {
- .len = len,
- .rx_buf = st->rx,
- }
- };
- st->tx[0] = SCA3000_READ_REG(reg_address_high);
-
- return spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
-}
-
/**
* sca3000_reg_lock_on() - test if the ctrl register lock is on
* @st: Driver specific device instance data.
@@ -309,11 +291,11 @@ static int sca3000_reg_lock_on(struct sca3000_state *st)
{
int ret;
- ret = sca3000_read_data_short(st, SCA3000_REG_STATUS_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_STATUS_ADDR));
if (ret < 0)
return ret;
- return !(st->rx[0] & SCA3000_LOCKED);
+ return !(ret & SCA3000_LOCKED);
}
/**
@@ -409,10 +391,7 @@ static int sca3000_read_ctrl_reg(struct sca3000_state *st,
ret = sca3000_write_reg(st, SCA3000_REG_CTRL_SEL_ADDR, ctrl_reg);
if (ret)
return ret;
- ret = sca3000_read_data_short(st, SCA3000_REG_CTRL_DATA_ADDR, 1);
- if (ret)
- return ret;
- return st->rx[0];
+ return spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_CTRL_DATA_ADDR));
}
/**
@@ -427,13 +406,13 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
struct sca3000_state *st = iio_priv(indio_dev);
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_REVID_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
if (ret < 0)
goto error_ret;
dev_info(&indio_dev->dev,
"sca3000 revision major=%lu, minor=%lu\n",
- st->rx[0] & SCA3000_REG_REVID_MAJOR_MASK,
- st->rx[0] & SCA3000_REG_REVID_MINOR_MASK);
+ ret & SCA3000_REG_REVID_MAJOR_MASK,
+ ret & SCA3000_REG_REVID_MINOR_MASK);
error_ret:
mutex_unlock(&st->lock);
@@ -570,7 +549,7 @@ static inline int __sca3000_get_base_freq(struct sca3000_state *st,
{
int ret;
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
return ret;
@@ -660,13 +639,13 @@ static int sca3000_read_3db_freq(struct sca3000_state *st, int *val)
{
int ret;
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
return ret;
/* mask bottom 2 bits - only ones that are relevant */
- st->rx[0] &= SCA3000_REG_MODE_MODE_MASK;
- switch (st->rx[0]) {
+ ret &= SCA3000_REG_MODE_MODE_MASK;
+ switch (ret) {
case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
*val = st->info->measurement_mode_3db_freq;
return IIO_VAL_INT;
@@ -698,14 +677,14 @@ static int sca3000_write_3db_freq(struct sca3000_state *st, int val)
mode = SCA3000_REG_MODE_MEAS_MODE_OP_2;
else
return -EINVAL;
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
return ret;
- st->rx[0] &= ~SCA3000_REG_MODE_MODE_MASK;
- st->rx[0] |= (mode & SCA3000_REG_MODE_MODE_MASK);
+ ret &= ~SCA3000_REG_MODE_MODE_MASK;
+ ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
- return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, st->rx[0]);
+ return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR, ret);
}
static int sca3000_read_raw(struct iio_dev *indio_dev,
@@ -727,25 +706,22 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
return -EBUSY;
}
address = sca3000_addresses[chan->address][0];
- ret = sca3000_read_data_short(st, address, 2);
+ ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
if (ret < 0) {
mutex_unlock(&st->lock);
return ret;
}
- *val = sign_extend32(be16_to_cpup((__be16 *)st->rx) >>
- chan->scan_type.shift,
+ *val = sign_extend32(ret >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
} else {
/* get the temperature when available */
- ret = sca3000_read_data_short(st,
- SCA3000_REG_TEMP_MSB_ADDR,
- 2);
+ ret = spi_w8r16be(st->us,
+ SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
if (ret < 0) {
mutex_unlock(&st->lock);
return ret;
}
- *val = (be16_to_cpup((__be16 *)st->rx) >>
- chan->scan_type.shift) &
+ *val = (ret >> chan->scan_type.shift) &
GENMASK(chan->scan_type.realbits - 1, 0);
}
mutex_unlock(&st->lock);
@@ -822,16 +798,15 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct sca3000_state *st = iio_priv(indio_dev);
- int len = 0, ret, val;
+ int len = 0, ret;
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
- val = st->rx[0];
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
mutex_unlock(&st->lock);
if (ret)
return ret;
- switch (val & SCA3000_REG_MODE_MODE_MASK) {
+ switch (ret & SCA3000_REG_MODE_MODE_MASK) {
case SCA3000_REG_MODE_MEAS_MODE_NORMAL:
len += sprintf(buf + len, "%d %d %d\n",
st->info->measurement_mode_freq,
@@ -964,7 +939,6 @@ static const struct attribute_group sca3000_attribute_group = {
static int sca3000_read_data(struct sca3000_state *st,
u8 reg_address_high,
- u8 *rx,
int len)
{
int ret;
@@ -974,18 +948,15 @@ static int sca3000_read_data(struct sca3000_state *st,
.tx_buf = st->tx,
}, {
.len = len,
- .rx_buf = rx,
+ .rx_buf = st->rx,
}
};
-
st->tx[0] = SCA3000_READ_REG(reg_address_high);
+
ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
- if (ret) {
+ if (ret)
dev_err(&st->us->dev, "problem reading register\n");
- return ret;
- }
-
- return 0;
+ return ret;
}
/**
@@ -1001,16 +972,15 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
mutex_lock(&st->lock);
if (val & SCA3000_REG_INT_STATUS_HALF) {
- ret = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR,
- 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
if (ret)
goto error_ret;
- num_available = st->rx[0];
+ num_available = ret;
/*
* num_available is the total number of samples available
* i.e. number of time points * number of channels.
*/
- ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
+ ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
num_available * 2);
if (ret)
goto error_ret;
@@ -1045,7 +1015,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
{
struct iio_dev *indio_dev = private;
struct sca3000_state *st = iio_priv(indio_dev);
- int ret, val;
+ int ret;
s64 last_timestamp = iio_get_time_ns(indio_dev);
/*
@@ -1053,15 +1023,14 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
* but ensures no interrupt is missed.
*/
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
- val = st->rx[0];
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
mutex_unlock(&st->lock);
if (ret)
goto done;
- sca3000_ring_int_process(val, indio_dev);
+ sca3000_ring_int_process(ret, indio_dev);
- if (val & SCA3000_INT_STATUS_FREE_FALL)
+ if (ret & SCA3000_INT_STATUS_FREE_FALL)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL,
0,
@@ -1070,7 +1039,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
IIO_EV_DIR_FALLING),
last_timestamp);
- if (val & SCA3000_INT_STATUS_Y_TRIGGER)
+ if (ret & SCA3000_INT_STATUS_Y_TRIGGER)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL,
0,
@@ -1079,7 +1048,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
IIO_EV_DIR_RISING),
last_timestamp);
- if (val & SCA3000_INT_STATUS_X_TRIGGER)
+ if (ret & SCA3000_INT_STATUS_X_TRIGGER)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL,
0,
@@ -1088,7 +1057,7 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
IIO_EV_DIR_RISING),
last_timestamp);
- if (val & SCA3000_INT_STATUS_Z_TRIGGER)
+ if (ret & SCA3000_INT_STATUS_Z_TRIGGER)
iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL,
0,
@@ -1114,13 +1083,13 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
/* read current value of mode register */
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
goto error_ret;
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
- ret = !!(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT);
+ ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
break;
case IIO_MOD_X:
case IIO_MOD_Y:
@@ -1129,7 +1098,7 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
* Motion detection mode cannot run at the same time as
* acceleration data being read.
*/
- if ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+ if ((ret & SCA3000_REG_MODE_MODE_MASK)
!= SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
ret = 0;
} else {
@@ -1157,20 +1126,20 @@ static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
int ret;
/* read current value of mode register */
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
return ret;
/* if off and should be on */
- if (state && !(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
+ if (state && !(ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- st->rx[0] | SCA3000_REG_MODE_FREE_FALL_DETECT);
+ ret | SCA3000_REG_MODE_FREE_FALL_DETECT);
/* if on and should be off */
- else if (!state && (st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
+ if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- st->rx[0] & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
- else
- return 0;
+ ret & ~SCA3000_REG_MODE_FREE_FALL_DETECT);
+
+ return 0;
}
static int sca3000_motion_detect_set_state(struct iio_dev *indio_dev, int axis,
@@ -1207,22 +1176,22 @@ static int sca3000_motion_detect_set_state(struct iio_dev *indio_dev, int axis,
}
/* read current value of mode register */
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
return ret;
/* if off and should be on */
if ((st->mo_det_use_count) &&
- ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+ ((ret & SCA3000_REG_MODE_MODE_MASK)
!= SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- (st->rx[0] & ~SCA3000_REG_MODE_MODE_MASK)
+ (ret & ~SCA3000_REG_MODE_MODE_MASK)
| SCA3000_REG_MODE_MEAS_MODE_MOT_DET);
/* if on and should be off */
else if (!(st->mo_det_use_count) &&
- ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
+ ((ret & SCA3000_REG_MODE_MODE_MASK)
== SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- st->rx[0] & SCA3000_REG_MODE_MODE_MASK);
+ ret & SCA3000_REG_MODE_MODE_MASK);
else
return 0;
}
@@ -1280,18 +1249,18 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
int ret;
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
goto error_ret;
if (state) {
dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
ret = sca3000_write_reg(st,
SCA3000_REG_MODE_ADDR,
- (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
+ (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
} else
ret = sca3000_write_reg(st,
SCA3000_REG_MODE_ADDR,
- (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
+ (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
error_ret:
mutex_unlock(&st->lock);
@@ -1315,12 +1284,12 @@ static int sca3000_hw_ring_preenable(struct iio_dev *indio_dev)
mutex_lock(&st->lock);
/* Enable the 50% full interrupt */
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
goto error_unlock;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
- st->rx[0] | SCA3000_REG_INT_MASK_RING_HALF);
+ ret | SCA3000_REG_INT_MASK_RING_HALF);
if (ret)
goto error_unlock;
@@ -1346,12 +1315,12 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
/* Disable the 50% full interrupt */
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
goto unlock;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
- st->rx[0] & ~SCA3000_REG_INT_MASK_RING_HALF);
+ ret & ~SCA3000_REG_INT_MASK_RING_HALF);
unlock:
mutex_unlock(&st->lock);
return ret;
@@ -1376,7 +1345,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
mutex_lock(&st->lock);
/* Ensure all interrupts have been acknowledged */
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
goto error_ret;
@@ -1402,7 +1371,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
if (ret)
goto error_ret;
/* Enable interrupts, relevant to mode and set up as active low */
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
goto error_ret;
ret = sca3000_write_reg(st,
@@ -1416,11 +1385,11 @@ static int sca3000_clean_setup(struct sca3000_state *st)
* Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
* as that occurs in one of the example on the datasheet
*/
- ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
goto error_ret;
ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
- (st->rx[0] & SCA3000_MODE_PROT_MASK));
+ ret & SCA3000_MODE_PROT_MASK);
error_ret:
mutex_unlock(&st->lock);
@@ -1503,14 +1472,15 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
int ret;
mutex_lock(&st->lock);
- ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+ ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
goto error_ret;
+
ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
- (st->rx[0] &
- ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
- SCA3000_REG_INT_MASK_RING_HALF |
- SCA3000_REG_INT_MASK_ALL_INTS)));
+ ret &
+ ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
+ SCA3000_REG_INT_MASK_RING_HALF |
+ SCA3000_REG_INT_MASK_ALL_INTS));
error_ret:
mutex_unlock(&st->lock);
return ret;
--
2.49.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-11 19:39 [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
@ 2025-06-11 19:39 ` Andrew Ijano
2025-06-12 6:38 ` Nuno Sá
` (3 more replies)
2 siblings, 4 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-11 19:39 UTC (permalink / raw)
To: jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
Use guard(mutex)(&st->lock) for handling mutex lock instead of
manually locking and unlocking the mutex. This prevents forgotten
locks due to early exits and remove the need of gotos.
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>
---
For this one, there are two cases where the previous implementation
was a smalllocking portion of the code and now it's locking the whole
function. I don't know if this is a desired behavior.
drivers/iio/accel/sca3000.c | 177 ++++++++++++------------------------
1 file changed, 57 insertions(+), 120 deletions(-)
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index d41759c68fb4..098d45bad389 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -405,17 +405,14 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
if (ret < 0)
- goto error_ret;
+ return ret;
dev_info(&indio_dev->dev,
"sca3000 revision major=%lu, minor=%lu\n",
ret & SCA3000_REG_REVID_MAJOR_MASK,
ret & SCA3000_REG_REVID_MINOR_MASK);
-error_ret:
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -699,32 +696,25 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (chan->type == IIO_ACCEL) {
- if (st->mo_det_use_count) {
- mutex_unlock(&st->lock);
+ if (st->mo_det_use_count)
return -EBUSY;
- }
address = sca3000_addresses[chan->address][0];
ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
- if (ret < 0) {
- mutex_unlock(&st->lock);
+ if (ret < 0)
return ret;
- }
*val = sign_extend32(ret >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
} else {
/* get the temperature when available */
ret = spi_w8r16be(st->us,
SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
- if (ret < 0) {
- mutex_unlock(&st->lock);
+ if (ret < 0)
return ret;
- }
*val = (ret >> chan->scan_type.shift) &
GENMASK(chan->scan_type.realbits - 1, 0);
}
- mutex_unlock(&st->lock);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = 0;
@@ -738,14 +728,12 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
*val2 = 600000;
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_SAMP_FREQ:
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = sca3000_read_raw_samp_freq(st, val);
- mutex_unlock(&st->lock);
return ret ? ret : IIO_VAL_INT;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = sca3000_read_3db_freq(st, val);
- mutex_unlock(&st->lock);
return ret;
default:
return -EINVAL;
@@ -763,22 +751,16 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_SAMP_FREQ:
if (val2)
return -EINVAL;
- mutex_lock(&st->lock);
- ret = sca3000_write_raw_samp_freq(st, val);
- mutex_unlock(&st->lock);
- return ret;
+ guard(mutex)(&st->lock);
+ return sca3000_write_raw_samp_freq(st, val);
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
if (val2)
return -EINVAL;
- mutex_lock(&st->lock);
- ret = sca3000_write_3db_freq(st, val);
- mutex_unlock(&st->lock);
- return ret;
+ guard(mutex)(&st->lock);
+ return sca3000_write_3db_freq(st, val);
default:
return -EINVAL;
}
-
- return ret;
}
/**
@@ -800,9 +782,8 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
struct sca3000_state *st = iio_priv(indio_dev);
int len = 0, ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
- mutex_unlock(&st->lock);
if (ret)
return ret;
@@ -851,10 +832,9 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
switch (info) {
case IIO_EV_INFO_VALUE:
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = sca3000_read_ctrl_reg(st,
sca3000_addresses[chan->address][1]);
- mutex_unlock(&st->lock);
if (ret < 0)
return ret;
*val = 0;
@@ -918,13 +898,10 @@ static int sca3000_write_event_value(struct iio_dev *indio_dev,
}
}
- mutex_lock(&st->lock);
- ret = sca3000_write_ctrl_reg(st,
+ guard(mutex)(&st->lock);
+ return sca3000_write_ctrl_reg(st,
sca3000_addresses[chan->address][1],
nonlinear);
- mutex_unlock(&st->lock);
-
- return ret;
}
static struct attribute *sca3000_attributes[] = {
@@ -969,12 +946,12 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
struct sca3000_state *st = iio_priv(indio_dev);
int ret, i, num_available;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (val & SCA3000_REG_INT_STATUS_HALF) {
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
if (ret)
- goto error_ret;
+ return;
num_available = ret;
/*
* num_available is the total number of samples available
@@ -983,7 +960,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
num_available * 2);
if (ret)
- goto error_ret;
+ return;
for (i = 0; i < num_available / 3; i++) {
/*
* Dirty hack to cover for 11 bit in fifo, 13 bit
@@ -995,8 +972,6 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
}
}
-error_ret:
- mutex_unlock(&st->lock);
}
/**
@@ -1022,9 +997,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
* Could lead if badly timed to an extra read of status reg,
* but ensures no interrupt is missed.
*/
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
- mutex_unlock(&st->lock);
if (ret)
goto done;
@@ -1081,16 +1055,15 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
struct sca3000_state *st = iio_priv(indio_dev);
int ret;
/* read current value of mode register */
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
+ return ret;
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
- ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
- break;
+ return !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
case IIO_MOD_X:
case IIO_MOD_Y:
case IIO_MOD_Z:
@@ -1100,24 +1073,18 @@ static int sca3000_read_event_config(struct iio_dev *indio_dev,
*/
if ((ret & SCA3000_REG_MODE_MODE_MASK)
!= SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
- ret = 0;
+ return 0;
} else {
ret = sca3000_read_ctrl_reg(st,
SCA3000_REG_CTRL_SEL_MD_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
/* only supporting logical or's for now */
- ret = !!(ret & sca3000_addresses[chan->address][2]);
+ return !!(ret & sca3000_addresses[chan->address][2]);
}
- break;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
-
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
}
static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
@@ -1220,26 +1187,19 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
struct sca3000_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
switch (chan->channel2) {
case IIO_MOD_X_AND_Y_AND_Z:
- ret = sca3000_freefall_set_state(indio_dev, state);
- break;
-
+ return sca3000_freefall_set_state(indio_dev, state);
case IIO_MOD_X:
case IIO_MOD_Y:
case IIO_MOD_Z:
- ret = sca3000_motion_detect_set_state(indio_dev,
+ return sca3000_motion_detect_set_state(indio_dev,
chan->address,
state);
- break;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
- mutex_unlock(&st->lock);
-
- return ret;
}
static inline
@@ -1248,23 +1208,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
struct sca3000_state *st = iio_priv(indio_dev);
int ret;
- mutex_lock(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
+ return ret;
+
if (state) {
dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
- ret = sca3000_write_reg(st,
+ return sca3000_write_reg(st,
SCA3000_REG_MODE_ADDR,
(ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
- } else
- ret = sca3000_write_reg(st,
- SCA3000_REG_MODE_ADDR,
- (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
+ }
+ return sca3000_write_reg(st,
+ SCA3000_REG_MODE_ADDR,
+ (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
}
/**
@@ -1281,26 +1237,18 @@ static int sca3000_hw_ring_preenable(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
- mutex_lock(&st->lock);
-
+ guard(mutex)(&st->lock);
/* Enable the 50% full interrupt */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_unlock;
+ return ret;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
ret | SCA3000_REG_INT_MASK_RING_HALF);
if (ret)
- goto error_unlock;
-
- mutex_unlock(&st->lock);
+ return ret;
return __sca3000_hw_ring_state_set(indio_dev, 1);
-
-error_unlock:
- mutex_unlock(&st->lock);
-
- return ret;
}
static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
@@ -1308,22 +1256,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
int ret;
struct sca3000_state *st = iio_priv(indio_dev);
+ guard(mutex)(&st->lock);
ret = __sca3000_hw_ring_state_set(indio_dev, 0);
if (ret)
return ret;
/* Disable the 50% full interrupt */
- mutex_lock(&st->lock);
-
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto unlock;
- ret = sca3000_write_reg(st,
+ return ret;
+ return sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
ret & ~SCA3000_REG_INT_MASK_RING_HALF);
-unlock:
- mutex_unlock(&st->lock);
- return ret;
}
static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
@@ -1343,25 +1287,25 @@ static int sca3000_clean_setup(struct sca3000_state *st)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* Ensure all interrupts have been acknowledged */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
/* Turn off all motion detection channels */
ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL,
ret & SCA3000_MD_CTRL_PROT_MASK);
if (ret)
- goto error_ret;
+ return ret;
/* Disable ring buffer */
ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL,
(ret & SCA3000_REG_OUT_CTRL_PROT_MASK)
| SCA3000_REG_OUT_CTRL_BUF_X_EN
@@ -1369,17 +1313,17 @@ static int sca3000_clean_setup(struct sca3000_state *st)
| SCA3000_REG_OUT_CTRL_BUF_Z_EN
| SCA3000_REG_OUT_CTRL_BUF_DIV_4);
if (ret)
- goto error_ret;
+ return ret;
/* Enable interrupts, relevant to mode and set up as active low */
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
ret = sca3000_write_reg(st,
SCA3000_REG_INT_MASK_ADDR,
(ret & SCA3000_REG_INT_MASK_PROT_MASK)
| SCA3000_REG_INT_MASK_ACTIVE_LOW);
if (ret)
- goto error_ret;
+ return ret;
/*
* Select normal measurement mode, free fall off, ring off
* Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
@@ -1387,13 +1331,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
*/
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
if (ret)
- goto error_ret;
- ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
+ return ret;
+ return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
ret & SCA3000_MODE_PROT_MASK);
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
}
static const struct iio_info sca3000_info = {
@@ -1471,19 +1411,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
if (ret)
- goto error_ret;
+ return ret;
- ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
+ return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
ret &
~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
SCA3000_REG_INT_MASK_RING_HALF |
SCA3000_REG_INT_MASK_ALL_INTS));
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
}
static void sca3000_remove(struct spi_device *spi)
--
2.49.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
@ 2025-06-12 6:20 ` Nuno Sá
2025-06-12 6:41 ` Nuno Sá
2025-06-12 12:56 ` Andy Shevchenko
1 sibling, 1 reply; 26+ messages in thread
From: Nuno Sá @ 2025-06-12 6:20 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
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;
> }
>
> /*
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
@ 2025-06-12 6:29 ` Nuno Sá
2025-06-14 19:33 ` Andrew Ijano
2025-06-12 13:22 ` Andy Shevchenko
1 sibling, 1 reply; 26+ messages in thread
From: Nuno Sá @ 2025-06-12 6:29 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16be() helpers. Just
> one case that reads large buffers is left using an internal helper.
>
> This is an old driver that was not making full use of the newer
> infrastructure.
>
> 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>
> ---
Looks good. Just one comment from me...
> drivers/iio/accel/sca3000.c | 166 +++++++++++++++---------------------
> 1 file changed, 68 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index bfa8a3f5a92f..d41759c68fb4 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -281,24 +281,6 @@ static int sca3000_write_reg(struct sca3000_state *st, u8
> address, u8 val)
> return spi_write(st->us, st->tx, 2);
> }
>
...
>
> static int sca3000_read_data(struct sca3000_state *st,
> u8 reg_address_high,
> - u8 *rx,
> int len)
> {
> int ret;
> @@ -974,18 +948,15 @@ static int sca3000_read_data(struct sca3000_state *st,
> .tx_buf = st->tx,
> }, {
> .len = len,
> - .rx_buf = rx,
> + .rx_buf = st->rx,
> }
> };
> -
> st->tx[0] = SCA3000_READ_REG(reg_address_high);
> +
> ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
> - if (ret) {
> + if (ret)
> dev_err(&st->us->dev, "problem reading register\n");
> - return ret;
> - }
> -
> - return 0;
> + return ret;
Unless I'm missing something, the above seems unrelated to the rest of the patch.
- Nuno Sá
> }
>
> /**
> @@ -1001,16 +972,15 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> *indio_dev)
> mutex_lock(&st->lock);
>
> if (val & SCA3000_REG_INT_STATUS_HALF) {
> - ret = sca3000_read_data_short(st, SCA3000_REG_BUF_COUNT_ADDR,
> - 1);
> + ret = spi_w8r8(st->us,
> SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> if (ret)
> goto error_ret;
> - num_available = st->rx[0];
> + num_available = ret;
> /*
> * num_available is the total number of samples available
> * i.e. number of time points * number of channels.
> */
> - ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, st->rx,
> + ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
> num_available * 2);
> if (ret)
> goto error_ret;
> @@ -1045,7 +1015,7 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> {
> struct iio_dev *indio_dev = private;
> struct sca3000_state *st = iio_priv(indio_dev);
> - int ret, val;
> + int ret;
> s64 last_timestamp = iio_get_time_ns(indio_dev);
>
> /*
> @@ -1053,15 +1023,14 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> * but ensures no interrupt is missed.
> */
> mutex_lock(&st->lock);
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
> - val = st->rx[0];
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> mutex_unlock(&st->lock);
> if (ret)
> goto done;
>
> - sca3000_ring_int_process(val, indio_dev);
> + sca3000_ring_int_process(ret, indio_dev);
>
> - if (val & SCA3000_INT_STATUS_FREE_FALL)
> + if (ret & SCA3000_INT_STATUS_FREE_FALL)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> @@ -1070,7 +1039,7 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> IIO_EV_DIR_FALLING),
> last_timestamp);
>
> - if (val & SCA3000_INT_STATUS_Y_TRIGGER)
> + if (ret & SCA3000_INT_STATUS_Y_TRIGGER)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> @@ -1079,7 +1048,7 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> IIO_EV_DIR_RISING),
> last_timestamp);
>
> - if (val & SCA3000_INT_STATUS_X_TRIGGER)
> + if (ret & SCA3000_INT_STATUS_X_TRIGGER)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> @@ -1088,7 +1057,7 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> IIO_EV_DIR_RISING),
> last_timestamp);
>
> - if (val & SCA3000_INT_STATUS_Z_TRIGGER)
> + if (ret & SCA3000_INT_STATUS_Z_TRIGGER)
> iio_push_event(indio_dev,
> IIO_MOD_EVENT_CODE(IIO_ACCEL,
> 0,
> @@ -1114,13 +1083,13 @@ static int sca3000_read_event_config(struct iio_dev
> *indio_dev,
> /* read current value of mode register */
> mutex_lock(&st->lock);
>
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> goto error_ret;
>
> switch (chan->channel2) {
> case IIO_MOD_X_AND_Y_AND_Z:
> - ret = !!(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT);
> + ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
> break;
> case IIO_MOD_X:
> case IIO_MOD_Y:
> @@ -1129,7 +1098,7 @@ static int sca3000_read_event_config(struct iio_dev
> *indio_dev,
> * Motion detection mode cannot run at the same time as
> * acceleration data being read.
> */
> - if ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
> + if ((ret & SCA3000_REG_MODE_MODE_MASK)
> != SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
> ret = 0;
> } else {
> @@ -1157,20 +1126,20 @@ static int sca3000_freefall_set_state(struct iio_dev
> *indio_dev, bool state)
> int ret;
>
> /* read current value of mode register */
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> return ret;
>
> /* if off and should be on */
> - if (state && !(st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
> + if (state && !(ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - st->rx[0] |
> SCA3000_REG_MODE_FREE_FALL_DETECT);
> + ret | SCA3000_REG_MODE_FREE_FALL_DETECT);
> /* if on and should be off */
> - else if (!state && (st->rx[0] & SCA3000_REG_MODE_FREE_FALL_DETECT))
> + if (!state && (ret & SCA3000_REG_MODE_FREE_FALL_DETECT))
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - st->rx[0] &
> ~SCA3000_REG_MODE_FREE_FALL_DETECT);
> - else
> - return 0;
> + ret &
> ~SCA3000_REG_MODE_FREE_FALL_DETECT);
> +
> + return 0;
> }
>
> static int sca3000_motion_detect_set_state(struct iio_dev *indio_dev, int axis,
> @@ -1207,22 +1176,22 @@ static int sca3000_motion_detect_set_state(struct iio_dev
> *indio_dev, int axis,
> }
>
> /* read current value of mode register */
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> return ret;
> /* if off and should be on */
> if ((st->mo_det_use_count) &&
> - ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
> + ((ret & SCA3000_REG_MODE_MODE_MASK)
> != SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - (st->rx[0] & ~SCA3000_REG_MODE_MODE_MASK)
> + (ret & ~SCA3000_REG_MODE_MODE_MASK)
> | SCA3000_REG_MODE_MEAS_MODE_MOT_DET);
> /* if on and should be off */
> else if (!(st->mo_det_use_count) &&
> - ((st->rx[0] & SCA3000_REG_MODE_MODE_MASK)
> + ((ret & SCA3000_REG_MODE_MODE_MASK)
> == SCA3000_REG_MODE_MEAS_MODE_MOT_DET))
> return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - st->rx[0] & SCA3000_REG_MODE_MODE_MASK);
> + ret & SCA3000_REG_MODE_MODE_MASK);
> else
> return 0;
> }
> @@ -1280,18 +1249,18 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev,
> bool state)
> int ret;
>
> mutex_lock(&st->lock);
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> goto error_ret;
> if (state) {
> dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> ret = sca3000_write_reg(st,
> SCA3000_REG_MODE_ADDR,
> - (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
> } else
> ret = sca3000_write_reg(st,
> SCA3000_REG_MODE_ADDR,
> - (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> error_ret:
> mutex_unlock(&st->lock);
>
> @@ -1315,12 +1284,12 @@ static int sca3000_hw_ring_preenable(struct iio_dev
> *indio_dev)
> mutex_lock(&st->lock);
>
> /* Enable the 50% full interrupt */
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> goto error_unlock;
> ret = sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> - st->rx[0] | SCA3000_REG_INT_MASK_RING_HALF);
> + ret | SCA3000_REG_INT_MASK_RING_HALF);
> if (ret)
> goto error_unlock;
>
> @@ -1346,12 +1315,12 @@ static int sca3000_hw_ring_postdisable(struct iio_dev
> *indio_dev)
> /* Disable the 50% full interrupt */
> mutex_lock(&st->lock);
>
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> goto unlock;
> ret = sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> - st->rx[0] & ~SCA3000_REG_INT_MASK_RING_HALF);
> + ret & ~SCA3000_REG_INT_MASK_RING_HALF);
> unlock:
> mutex_unlock(&st->lock);
> return ret;
> @@ -1376,7 +1345,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
>
> mutex_lock(&st->lock);
> /* Ensure all interrupts have been acknowledged */
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_STATUS_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> goto error_ret;
>
> @@ -1402,7 +1371,7 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> if (ret)
> goto error_ret;
> /* Enable interrupts, relevant to mode and set up as active low */
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> goto error_ret;
> ret = sca3000_write_reg(st,
> @@ -1416,11 +1385,11 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> * Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
> * as that occurs in one of the example on the datasheet
> */
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> goto error_ret;
> ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> - (st->rx[0] & SCA3000_MODE_PROT_MASK));
> + ret & SCA3000_MODE_PROT_MASK);
>
> error_ret:
> mutex_unlock(&st->lock);
> @@ -1503,14 +1472,15 @@ static int sca3000_stop_all_interrupts(struct sca3000_state
> *st)
> int ret;
>
> mutex_lock(&st->lock);
> - ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> goto error_ret;
> +
> ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> - (st->rx[0] &
> - ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> - SCA3000_REG_INT_MASK_RING_HALF |
> - SCA3000_REG_INT_MASK_ALL_INTS)));
> + ret &
> + ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> + SCA3000_REG_INT_MASK_RING_HALF |
> + SCA3000_REG_INT_MASK_ALL_INTS));
> error_ret:
> mutex_unlock(&st->lock);
> return ret;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
@ 2025-06-12 6:38 ` Nuno Sá
2025-06-14 11:54 ` Jonathan Cameron
2025-06-14 21:17 ` Andrew Ijano
2025-06-12 14:48 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 2 replies; 26+ messages in thread
From: Nuno Sá @ 2025-06-12 6:38 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> Use guard(mutex)(&st->lock) for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.
>
> 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>
> ---
> For this one, there are two cases where the previous implementation
> was a smalllocking portion of the code and now it's locking the whole
> function. I don't know if this is a desired behavior.
>
In theory, it should not break anything. But you can always refactor things (like
small helpers) to lock only the code you want. There's also scoped_guard(). I would
say, up to you for re-spinning a new version because of the above :).
Just have something that I'm not totatlly sure... Did you made sure to compile?
AFAIR, guard() had some complains when used in switch() case statements. Maybe that
got improved.
If the above is not an issue:
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> drivers/iio/accel/sca3000.c | 177 ++++++++++++------------------------
> 1 file changed, 57 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index d41759c68fb4..098d45bad389 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -405,17 +405,14 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
> int ret;
> struct sca3000_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
> if (ret < 0)
> - goto error_ret;
> + return ret;
> dev_info(&indio_dev->dev,
> "sca3000 revision major=%lu, minor=%lu\n",
> ret & SCA3000_REG_REVID_MAJOR_MASK,
> ret & SCA3000_REG_REVID_MINOR_MASK);
> -error_ret:
> - mutex_unlock(&st->lock);
> -
> return ret;
> }
>
> @@ -699,32 +696,25 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> if (chan->type == IIO_ACCEL) {
> - if (st->mo_det_use_count) {
> - mutex_unlock(&st->lock);
> + if (st->mo_det_use_count)
> return -EBUSY;
> - }
> address = sca3000_addresses[chan->address][0];
> ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
> - if (ret < 0) {
> - mutex_unlock(&st->lock);
> + if (ret < 0)
> return ret;
> - }
> *val = sign_extend32(ret >> chan->scan_type.shift,
> chan->scan_type.realbits - 1);
> } else {
> /* get the temperature when available */
> ret = spi_w8r16be(st->us,
> SCA3000_READ_REG(SCA3000_REG_TEMP_
> MSB_ADDR));
> - if (ret < 0) {
> - mutex_unlock(&st->lock);
> + if (ret < 0)
> return ret;
> - }
> *val = (ret >> chan->scan_type.shift) &
> GENMASK(chan->scan_type.realbits - 1, 0);
> }
> - mutex_unlock(&st->lock);
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> *val = 0;
> @@ -738,14 +728,12 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
> *val2 = 600000;
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = sca3000_read_raw_samp_freq(st, val);
> - mutex_unlock(&st->lock);
> return ret ? ret : IIO_VAL_INT;
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = sca3000_read_3db_freq(st, val);
> - mutex_unlock(&st->lock);
> return ret;
> default:
> return -EINVAL;
> @@ -763,22 +751,16 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_SAMP_FREQ:
> if (val2)
> return -EINVAL;
> - mutex_lock(&st->lock);
> - ret = sca3000_write_raw_samp_freq(st, val);
> - mutex_unlock(&st->lock);
> - return ret;
> + guard(mutex)(&st->lock);
> + return sca3000_write_raw_samp_freq(st, val);
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> if (val2)
> return -EINVAL;
> - mutex_lock(&st->lock);
> - ret = sca3000_write_3db_freq(st, val);
> - mutex_unlock(&st->lock);
> - return ret;
> + guard(mutex)(&st->lock);
> + return sca3000_write_3db_freq(st, val);
> default:
> return -EINVAL;
> }
> -
> - return ret;
> }
>
> /**
> @@ -800,9 +782,8 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> struct sca3000_state *st = iio_priv(indio_dev);
> int len = 0, ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> - mutex_unlock(&st->lock);
> if (ret)
> return ret;
>
> @@ -851,10 +832,9 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
>
> switch (info) {
> case IIO_EV_INFO_VALUE:
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = sca3000_read_ctrl_reg(st,
> sca3000_addresses[chan->address][1]);
> - mutex_unlock(&st->lock);
> if (ret < 0)
> return ret;
> *val = 0;
> @@ -918,13 +898,10 @@ static int sca3000_write_event_value(struct iio_dev
> *indio_dev,
> }
> }
>
> - mutex_lock(&st->lock);
> - ret = sca3000_write_ctrl_reg(st,
> + guard(mutex)(&st->lock);
> + return sca3000_write_ctrl_reg(st,
> sca3000_addresses[chan->address][1],
> nonlinear);
> - mutex_unlock(&st->lock);
> -
> - return ret;
> }
>
> static struct attribute *sca3000_attributes[] = {
> @@ -969,12 +946,12 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> *indio_dev)
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret, i, num_available;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
>
> if (val & SCA3000_REG_INT_STATUS_HALF) {
> ret = spi_w8r8(st->us,
> SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> if (ret)
> - goto error_ret;
> + return;
> num_available = ret;
> /*
> * num_available is the total number of samples available
> @@ -983,7 +960,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> *indio_dev)
> ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
> num_available * 2);
> if (ret)
> - goto error_ret;
> + return;
> for (i = 0; i < num_available / 3; i++) {
> /*
> * Dirty hack to cover for 11 bit in fifo, 13 bit
> @@ -995,8 +972,6 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> *indio_dev)
> iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
> }
> }
> -error_ret:
> - mutex_unlock(&st->lock);
> }
>
> /**
> @@ -1022,9 +997,8 @@ static irqreturn_t sca3000_event_handler(int irq, void
> *private)
> * Could lead if badly timed to an extra read of status reg,
> * but ensures no interrupt is missed.
> */
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> - mutex_unlock(&st->lock);
> if (ret)
> goto done;
>
> @@ -1081,16 +1055,15 @@ static int sca3000_read_event_config(struct iio_dev
> *indio_dev,
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
> /* read current value of mode register */
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
>
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
>
> switch (chan->channel2) {
> case IIO_MOD_X_AND_Y_AND_Z:
> - ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
> - break;
> + return !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
> case IIO_MOD_X:
> case IIO_MOD_Y:
> case IIO_MOD_Z:
> @@ -1100,24 +1073,18 @@ static int sca3000_read_event_config(struct iio_dev
> *indio_dev,
> */
> if ((ret & SCA3000_REG_MODE_MODE_MASK)
> != SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
> - ret = 0;
> + return 0;
> } else {
> ret = sca3000_read_ctrl_reg(st,
> SCA3000_REG_CTRL_SEL_MD_CTRL);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> /* only supporting logical or's for now */
> - ret = !!(ret & sca3000_addresses[chan->address][2]);
> + return !!(ret & sca3000_addresses[chan->address][2]);
> }
> - break;
> default:
> - ret = -EINVAL;
> + return -EINVAL;
> }
> -
> -error_ret:
> - mutex_unlock(&st->lock);
> -
> - return ret;
> }
>
> static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
> @@ -1220,26 +1187,19 @@ static int sca3000_write_event_config(struct iio_dev
> *indio_dev,
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> switch (chan->channel2) {
> case IIO_MOD_X_AND_Y_AND_Z:
> - ret = sca3000_freefall_set_state(indio_dev, state);
> - break;
> -
> + return sca3000_freefall_set_state(indio_dev, state);
> case IIO_MOD_X:
> case IIO_MOD_Y:
> case IIO_MOD_Z:
> - ret = sca3000_motion_detect_set_state(indio_dev,
> + return sca3000_motion_detect_set_state(indio_dev,
> chan->address,
> state);
> - break;
> default:
> - ret = -EINVAL;
> - break;
> + return -EINVAL;
> }
> - mutex_unlock(&st->lock);
> -
> - return ret;
> }
>
> static inline
> @@ -1248,23 +1208,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev,
> bool state)
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
> +
> if (state) {
> dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> - ret = sca3000_write_reg(st,
> + return sca3000_write_reg(st,
> SCA3000_REG_MODE_ADDR,
> (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
> - } else
> - ret = sca3000_write_reg(st,
> - SCA3000_REG_MODE_ADDR,
> - (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> -error_ret:
> - mutex_unlock(&st->lock);
> -
> - return ret;
> + }
> + return sca3000_write_reg(st,
> + SCA3000_REG_MODE_ADDR,
> + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> }
>
> /**
> @@ -1281,26 +1237,18 @@ static int sca3000_hw_ring_preenable(struct iio_dev
> *indio_dev)
> int ret;
> struct sca3000_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->lock);
> -
> + guard(mutex)(&st->lock);
> /* Enable the 50% full interrupt */
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto error_unlock;
> + return ret;
> ret = sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> ret | SCA3000_REG_INT_MASK_RING_HALF);
> if (ret)
> - goto error_unlock;
> -
> - mutex_unlock(&st->lock);
> + return ret;
>
> return __sca3000_hw_ring_state_set(indio_dev, 1);
> -
> -error_unlock:
> - mutex_unlock(&st->lock);
> -
> - return ret;
> }
>
> static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> @@ -1308,22 +1256,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev
> *indio_dev)
> int ret;
> struct sca3000_state *st = iio_priv(indio_dev);
>
> + guard(mutex)(&st->lock);
> ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> if (ret)
> return ret;
>
> /* Disable the 50% full interrupt */
> - mutex_lock(&st->lock);
> -
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto unlock;
> - ret = sca3000_write_reg(st,
> + return ret;
> + return sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> ret & ~SCA3000_REG_INT_MASK_RING_HALF);
> -unlock:
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
> @@ -1343,25 +1287,25 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> {
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> /* Ensure all interrupts have been acknowledged */
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
>
> /* Turn off all motion detection channels */
> ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL,
> ret & SCA3000_MD_CTRL_PROT_MASK);
> if (ret)
> - goto error_ret;
> + return ret;
>
> /* Disable ring buffer */
> ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL);
> if (ret < 0)
> - goto error_ret;
> + return ret;
> ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL,
> (ret & SCA3000_REG_OUT_CTRL_PROT_MASK)
> | SCA3000_REG_OUT_CTRL_BUF_X_EN
> @@ -1369,17 +1313,17 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> | SCA3000_REG_OUT_CTRL_BUF_Z_EN
> | SCA3000_REG_OUT_CTRL_BUF_DIV_4);
> if (ret)
> - goto error_ret;
> + return ret;
> /* Enable interrupts, relevant to mode and set up as active low */
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
> ret = sca3000_write_reg(st,
> SCA3000_REG_INT_MASK_ADDR,
> (ret & SCA3000_REG_INT_MASK_PROT_MASK)
> | SCA3000_REG_INT_MASK_ACTIVE_LOW);
> if (ret)
> - goto error_ret;
> + return ret;
> /*
> * Select normal measurement mode, free fall off, ring off
> * Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
> @@ -1387,13 +1331,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> */
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> if (ret)
> - goto error_ret;
> - ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> + return ret;
> + return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> ret & SCA3000_MODE_PROT_MASK);
> -
> -error_ret:
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> static const struct iio_info sca3000_info = {
> @@ -1471,19 +1411,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state
> *st)
> {
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> if (ret)
> - goto error_ret;
> + return ret;
>
> - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> ret &
> ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> SCA3000_REG_INT_MASK_RING_HALF |
> SCA3000_REG_INT_MASK_ALL_INTS));
> -error_ret:
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> static void sca3000_remove(struct spi_device *spi)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-12 6:20 ` Nuno Sá
@ 2025-06-12 6:41 ` Nuno Sá
2025-06-14 19:12 ` Andrew Ijano
0 siblings, 1 reply; 26+ messages in thread
From: Nuno Sá @ 2025-06-12 6:41 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
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;
> > }
> >
> > /*
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
2025-06-12 6:20 ` Nuno Sá
@ 2025-06-12 12:56 ` Andy Shevchenko
2025-06-14 11:48 ` Jonathan Cameron
1 sibling, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2025-06-12 12:56 UTC (permalink / raw)
To: Andrew Ijano
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
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>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
2025-06-12 6:29 ` Nuno Sá
@ 2025-06-12 13:22 ` Andy Shevchenko
2025-06-14 21:06 ` Andrew Ijano
1 sibling, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2025-06-12 13:22 UTC (permalink / raw)
To: Andrew Ijano
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On Wed, Jun 11, 2025 at 04:39:20PM -0300, Andrew Ijano wrote:
> Remove usages of sca3000_read_data() and sca3000_read_data_short()
> functions, replacing it by spi_w8r8() and spi_w8r16be() helpers. Just
> one case that reads large buffers is left using an internal helper.
>
> This is an old driver that was not making full use of the newer
> infrastructure.
...
> + ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
Unneeded parentheses.
...
> + ret = spi_w8r16be(st->us,
> + SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
Make it simply one line. The above formatting is ugly.
...
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct sca3000_state *st = iio_priv(indio_dev);
> - int len = 0, ret, val;
> + int len = 0, ret;
Ideally it's better to split them and len should never be signed.
Moreover, the function should be switched to sysfs_emit_at() if this is part
of ABI.
> mutex_lock(&st->lock);
> - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> - val = st->rx[0];
> + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> mutex_unlock(&st->lock);
> if (ret)
> return ret;
...
> }, {
> .len = len,
> - .rx_buf = rx,
> + .rx_buf = st->rx,
> }
> };
> -
Stray change. Doesn't checkpatch complain on this?
> - (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
> - (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
In the original code and still now too many parentheses.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2025-06-12 6:38 ` Nuno Sá
@ 2025-06-12 14:48 ` kernel test robot
2025-06-12 15:52 ` kernel test robot
2025-06-14 12:01 ` Jonathan Cameron
3 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2025-06-12 14:48 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: oe-kbuild-all, andrew.lopes, gustavobastos, dlechner, nuno.sa,
andy, jstephan, linux-iio, linux-kernel
Hi Andrew,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.16-rc1 next-20250612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrew-Ijano/iio-accel-sca3000-replace-error_ret-labels-by-simple-returns/20250612-034940
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20250611194648.18133-4-andrew.lopes%40alumni.usp.br
patch subject: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
config: x86_64-randconfig-001-20250612 (https://download.01.org/0day-ci/archive/20250612/202506122259.4MrANF8h-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250612/202506122259.4MrANF8h-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506122259.4MrANF8h-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/iio/accel/sca3000.c: In function 'sca3000_write_raw':
>> drivers/iio/accel/sca3000.c:748:13: warning: unused variable 'ret' [-Wunused-variable]
748 | int ret;
| ^~~
drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_value':
drivers/iio/accel/sca3000.c:881:13: warning: unused variable 'ret' [-Wunused-variable]
881 | int ret;
| ^~~
drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_config':
drivers/iio/accel/sca3000.c:1188:13: warning: unused variable 'ret' [-Wunused-variable]
1188 | int ret;
| ^~~
vim +/ret +748 drivers/iio/accel/sca3000.c
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 742
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 743 static int sca3000_write_raw(struct iio_dev *indio_dev,
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 744 struct iio_chan_spec const *chan,
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 745 int val, int val2, long mask)
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 746 {
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 747 struct sca3000_state *st = iio_priv(indio_dev);
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 @748 int ret;
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 749
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 750 switch (mask) {
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 751 case IIO_CHAN_INFO_SAMP_FREQ:
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 752 if (val2)
e0f3fc9b47e61b drivers/staging/iio/accel/sca3000_core.c Ico Doornekamp 2016-09-13 753 return -EINVAL;
926150211ef327 drivers/iio/accel/sca3000.c Andrew Ijano 2025-06-11 754 guard(mutex)(&st->lock);
926150211ef327 drivers/iio/accel/sca3000.c Andrew Ijano 2025-06-11 755 return sca3000_write_raw_samp_freq(st, val);
626f971b5b0729 drivers/staging/iio/accel/sca3000.c Jonathan Cameron 2016-10-08 756 case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
626f971b5b0729 drivers/staging/iio/accel/sca3000.c Jonathan Cameron 2016-10-08 757 if (val2)
626f971b5b0729 drivers/staging/iio/accel/sca3000.c Jonathan Cameron 2016-10-08 758 return -EINVAL;
926150211ef327 drivers/iio/accel/sca3000.c Andrew Ijano 2025-06-11 759 guard(mutex)(&st->lock);
926150211ef327 drivers/iio/accel/sca3000.c Andrew Ijano 2025-06-11 760 return sca3000_write_3db_freq(st, val);
25888dc51163a5 drivers/staging/iio/accel/sca3000_core.c Jonathan Cameron 2011-05-18 761 default:
25888dc51163a5 drivers/staging/iio/accel/sca3000_core.c Jonathan Cameron 2011-05-18 762 return -EINVAL;
25888dc51163a5 drivers/staging/iio/accel/sca3000_core.c Jonathan Cameron 2011-05-18 763 }
25888dc51163a5 drivers/staging/iio/accel/sca3000_core.c Jonathan Cameron 2011-05-18 764 }
574fb258d63658 drivers/staging/iio/accel/sca3000_core.c Jonathan Cameron 2009-08-18 765
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2025-06-12 6:38 ` Nuno Sá
2025-06-12 14:48 ` kernel test robot
@ 2025-06-12 15:52 ` kernel test robot
2025-06-12 16:06 ` David Lechner
2025-06-14 12:01 ` Jonathan Cameron
3 siblings, 1 reply; 26+ messages in thread
From: kernel test robot @ 2025-06-12 15:52 UTC (permalink / raw)
To: Andrew Ijano, jic23
Cc: oe-kbuild-all, andrew.lopes, gustavobastos, dlechner, nuno.sa,
andy, jstephan, linux-iio, linux-kernel
Hi Andrew,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.16-rc1 next-20250612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrew-Ijano/iio-accel-sca3000-replace-error_ret-labels-by-simple-returns/20250612-034940
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20250611194648.18133-4-andrew.lopes%40alumni.usp.br
patch subject: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
config: nios2-randconfig-002-20250612 (https://download.01.org/0day-ci/archive/20250612/202506122309.FvJPaMhh-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250612/202506122309.FvJPaMhh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506122309.FvJPaMhh-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/irqflags.h:17,
from include/asm-generic/bitops.h:14,
from ./arch/nios2/include/generated/asm/bitops.h:1,
from include/linux/bitops.h:68,
from include/linux/kernel.h:23,
from include/linux/interrupt.h:6,
from drivers/iio/accel/sca3000.c:10:
drivers/iio/accel/sca3000.c: In function 'sca3000_read_raw':
>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
class_##_name##_t var __cleanup(class_##_name##_destructor) = \
^~~~~~
include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
CLASS(_name, __UNIQUE_ID(guard))
^~~~~
drivers/iio/accel/sca3000.c:699:3: note: in expansion of macro 'guard'
guard(mutex)(&st->lock);
^~~~~
>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
class_##_name##_t var __cleanup(class_##_name##_destructor) = \
^~~~~~
include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
CLASS(_name, __UNIQUE_ID(guard))
^~~~~
drivers/iio/accel/sca3000.c:731:3: note: in expansion of macro 'guard'
guard(mutex)(&st->lock);
^~~~~
>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
class_##_name##_t var __cleanup(class_##_name##_destructor) = \
^~~~~~
include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
CLASS(_name, __UNIQUE_ID(guard))
^~~~~
drivers/iio/accel/sca3000.c:735:3: note: in expansion of macro 'guard'
guard(mutex)(&st->lock);
^~~~~
drivers/iio/accel/sca3000.c: In function 'sca3000_write_raw':
drivers/iio/accel/sca3000.c:748:6: warning: unused variable 'ret' [-Wunused-variable]
int ret;
^~~
In file included from include/linux/irqflags.h:17,
from include/asm-generic/bitops.h:14,
from ./arch/nios2/include/generated/asm/bitops.h:1,
from include/linux/bitops.h:68,
from include/linux/kernel.h:23,
from include/linux/interrupt.h:6,
from drivers/iio/accel/sca3000.c:10:
drivers/iio/accel/sca3000.c: In function 'sca3000_read_event_value':
>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
class_##_name##_t var __cleanup(class_##_name##_destructor) = \
^~~~~~
include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
CLASS(_name, __UNIQUE_ID(guard))
^~~~~
drivers/iio/accel/sca3000.c:835:3: note: in expansion of macro 'guard'
guard(mutex)(&st->lock);
^~~~~
drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_value':
drivers/iio/accel/sca3000.c:881:6: warning: unused variable 'ret' [-Wunused-variable]
int ret;
^~~
drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_config':
drivers/iio/accel/sca3000.c:1188:6: warning: unused variable 'ret' [-Wunused-variable]
int ret;
^~~
vim +258 include/linux/cleanup.h
54da6a0924311c Peter Zijlstra 2023-05-26 256
54da6a0924311c Peter Zijlstra 2023-05-26 257 #define CLASS(_name, var) \
54da6a0924311c Peter Zijlstra 2023-05-26 @258 class_##_name##_t var __cleanup(class_##_name##_destructor) = \
54da6a0924311c Peter Zijlstra 2023-05-26 259 class_##_name##_constructor
54da6a0924311c Peter Zijlstra 2023-05-26 260
54da6a0924311c Peter Zijlstra 2023-05-26 261
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-12 15:52 ` kernel test robot
@ 2025-06-12 16:06 ` David Lechner
2025-06-14 21:21 ` Andrew Ijano
0 siblings, 1 reply; 26+ messages in thread
From: David Lechner @ 2025-06-12 16:06 UTC (permalink / raw)
To: kernel test robot, Andrew Ijano, jic23
Cc: oe-kbuild-all, andrew.lopes, gustavobastos, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On 6/12/25 10:52 AM, kernel test robot wrote:
> Hi Andrew,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on jic23-iio/togreg]
> [also build test ERROR on linus/master v6.16-rc1 next-20250612]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Andrew-Ijano/iio-accel-sca3000-replace-error_ret-labels-by-simple-returns/20250612-034940
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> patch link: https://lore.kernel.org/r/20250611194648.18133-4-andrew.lopes%40alumni.usp.br
> patch subject: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
> config: nios2-randconfig-002-20250612 (https://download.01.org/0day-ci/archive/20250612/202506122309.FvJPaMhh-lkp@intel.com/config)
> compiler: nios2-linux-gcc (GCC) 8.5.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250612/202506122309.FvJPaMhh-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202506122309.FvJPaMhh-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> In file included from include/linux/irqflags.h:17,
> from include/asm-generic/bitops.h:14,
> from ./arch/nios2/include/generated/asm/bitops.h:1,
> from include/linux/bitops.h:68,
> from include/linux/kernel.h:23,
> from include/linux/interrupt.h:6,
> from drivers/iio/accel/sca3000.c:10:
> drivers/iio/accel/sca3000.c: In function 'sca3000_read_raw':
>>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
> class_##_name##_t var __cleanup(class_##_name##_destructor) = \
> ^~~~~~
> include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
> CLASS(_name, __UNIQUE_ID(guard))
> ^~~~~
> drivers/iio/accel/sca3000.c:699:3: note: in expansion of macro 'guard'
> guard(mutex)(&st->lock);
> ^~~~~
>>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
> class_##_name##_t var __cleanup(class_##_name##_destructor) = \
> ^~~~~~
> include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
> CLASS(_name, __UNIQUE_ID(guard))
> ^~~~~
> drivers/iio/accel/sca3000.c:731:3: note: in expansion of macro 'guard'
> guard(mutex)(&st->lock);
> ^~~~~
>>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
> class_##_name##_t var __cleanup(class_##_name##_destructor) = \
> ^~~~~~
> include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
> CLASS(_name, __UNIQUE_ID(guard))
> ^~~~~
> drivers/iio/accel/sca3000.c:735:3: note: in expansion of macro 'guard'
> guard(mutex)(&st->lock);
> ^~~~~
> drivers/iio/accel/sca3000.c: In function 'sca3000_write_raw':
> drivers/iio/accel/sca3000.c:748:6: warning: unused variable 'ret' [-Wunused-variable]
> int ret;
> ^~~
> In file included from include/linux/irqflags.h:17,
> from include/asm-generic/bitops.h:14,
> from ./arch/nios2/include/generated/asm/bitops.h:1,
> from include/linux/bitops.h:68,
> from include/linux/kernel.h:23,
> from include/linux/interrupt.h:6,
> from drivers/iio/accel/sca3000.c:10:
> drivers/iio/accel/sca3000.c: In function 'sca3000_read_event_value':
>>> include/linux/cleanup.h:258:2: error: a label can only be part of a statement and a declaration is not a statement
> class_##_name##_t var __cleanup(class_##_name##_destructor) = \
> ^~~~~~
> include/linux/cleanup.h:319:2: note: in expansion of macro 'CLASS'
> CLASS(_name, __UNIQUE_ID(guard))
> ^~~~~
> drivers/iio/accel/sca3000.c:835:3: note: in expansion of macro 'guard'
> guard(mutex)(&st->lock);
> ^~~~~
> drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_value':
> drivers/iio/accel/sca3000.c:881:6: warning: unused variable 'ret' [-Wunused-variable]
> int ret;
> ^~~
> drivers/iio/accel/sca3000.c: In function 'sca3000_write_event_config':
> drivers/iio/accel/sca3000.c:1188:6: warning: unused variable 'ret' [-Wunused-variable]
> int ret;
> ^~~
>
>
> vim +258 include/linux/cleanup.h
>
> 54da6a0924311c Peter Zijlstra 2023-05-26 256
> 54da6a0924311c Peter Zijlstra 2023-05-26 257 #define CLASS(_name, var) \
> 54da6a0924311c Peter Zijlstra 2023-05-26 @258 class_##_name##_t var __cleanup(class_##_name##_destructor) = \
> 54da6a0924311c Peter Zijlstra 2023-05-26 259 class_##_name##_constructor
> 54da6a0924311c Peter Zijlstra 2023-05-26 260
> 54da6a0924311c Peter Zijlstra 2023-05-26 261
>
These error messages aren't particularity helpful, but what I think
this is try to say is that you have to be careful with guard() in
switch statements.
The guard() macro is declaring a new local variable, which shouldn't
be done in a case: statement without enclosing it in a separate scope.
Some compilers complain and some don't so even if it worked for you
locally, we need to make it work for all supported compilers.
So the code needs to looks something like this:
case IIO_CHAN_INFO_SAMP_FREQ: {
guard(mutex)(&st->lock);
ret = sca3000_read_raw_samp_freq(st, val);
return ret ? ret : IIO_VAL_INT;
}
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-12 12:56 ` Andy Shevchenko
@ 2025-06-14 11:48 ` Jonathan Cameron
2025-06-14 19:16 ` Andrew Ijano
0 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2025-06-14 11:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Ijano, andrew.lopes, gustavobastos, dlechner, nuno.sa,
andy, jstephan, linux-iio, linux-kernel
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-12 6:38 ` Nuno Sá
@ 2025-06-14 11:54 ` Jonathan Cameron
2025-06-14 21:17 ` Andrew Ijano
1 sibling, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2025-06-14 11:54 UTC (permalink / raw)
To: Nuno Sá
Cc: Andrew Ijano, andrew.lopes, gustavobastos, dlechner, nuno.sa,
andy, jstephan, linux-iio, linux-kernel
On Thu, 12 Jun 2025 07:38:18 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> > Use guard(mutex)(&st->lock) for handling mutex lock instead of
> > manually locking and unlocking the mutex. This prevents forgotten
> > locks due to early exits and remove the need of gotos.
> >
> > 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>
> > ---
> > For this one, there are two cases where the previous implementation
> > was a smalllocking portion of the code and now it's locking the whole
> > function. I don't know if this is a desired behavior.
> >
>
> In theory, it should not break anything. But you can always refactor things (like
> small helpers) to lock only the code you want. There's also scoped_guard(). I would
> say, up to you for re-spinning a new version because of the above :).
>
> Just have something that I'm not totatlly sure... Did you made sure to compile?
> AFAIR, guard() had some complains when used in switch() case statements. Maybe that
> got improved.
yes, - Needs scope {} to be defined to limit it to the case.
>
> If the above is not an issue:
>
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
>
> > drivers/iio/accel/sca3000.c | 177 ++++++++++++------------------------
> > 1 file changed, 57 insertions(+), 120 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index d41759c68fb4..098d45bad389 100644
> > --- a/drivers/iio/accel/sca3000.c
> > +++ b/drivers/iio/accel/sca3000.c
> > @@ -405,17 +405,14 @@ static int sca3000_print_rev(struct iio_dev *indio_dev)
> > int ret;
> > struct sca3000_state *st = iio_priv(indio_dev);
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_REVID_ADDR));
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > dev_info(&indio_dev->dev,
> > "sca3000 revision major=%lu, minor=%lu\n",
> > ret & SCA3000_REG_REVID_MAJOR_MASK,
> > ret & SCA3000_REG_REVID_MINOR_MASK);
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > -
> > return ret;
> > }
> >
> > @@ -699,32 +696,25 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > if (chan->type == IIO_ACCEL) {
> > - if (st->mo_det_use_count) {
> > - mutex_unlock(&st->lock);
> > + if (st->mo_det_use_count)
> > return -EBUSY;
> > - }
> > address = sca3000_addresses[chan->address][0];
> > ret = spi_w8r16be(st->us, SCA3000_READ_REG(address));
> > - if (ret < 0) {
> > - mutex_unlock(&st->lock);
> > + if (ret < 0)
> > return ret;
> > - }
> > *val = sign_extend32(ret >> chan->scan_type.shift,
> > chan->scan_type.realbits - 1);
> > } else {
> > /* get the temperature when available */
> > ret = spi_w8r16be(st->us,
> > SCA3000_READ_REG(SCA3000_REG_TEMP_
> > MSB_ADDR));
> > - if (ret < 0) {
> > - mutex_unlock(&st->lock);
> > + if (ret < 0)
> > return ret;
> > - }
> > *val = (ret >> chan->scan_type.shift) &
> > GENMASK(chan->scan_type.realbits - 1, 0);
> > }
> > - mutex_unlock(&st->lock);
> > return IIO_VAL_INT;
> > case IIO_CHAN_INFO_SCALE:
> > *val = 0;
> > @@ -738,14 +728,12 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
> > *val2 = 600000;
> > return IIO_VAL_INT_PLUS_MICRO;
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = sca3000_read_raw_samp_freq(st, val);
> > - mutex_unlock(&st->lock);
> > return ret ? ret : IIO_VAL_INT;
> > case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = sca3000_read_3db_freq(st, val);
> > - mutex_unlock(&st->lock);
> > return ret;
> > default:
> > return -EINVAL;
> > @@ -763,22 +751,16 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > if (val2)
> > return -EINVAL;
> > - mutex_lock(&st->lock);
> > - ret = sca3000_write_raw_samp_freq(st, val);
> > - mutex_unlock(&st->lock);
> > - return ret;
> > + guard(mutex)(&st->lock);
> > + return sca3000_write_raw_samp_freq(st, val);
> > case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > if (val2)
> > return -EINVAL;
> > - mutex_lock(&st->lock);
> > - ret = sca3000_write_3db_freq(st, val);
> > - mutex_unlock(&st->lock);
> > - return ret;
> > + guard(mutex)(&st->lock);
> > + return sca3000_write_3db_freq(st, val);
> > default:
> > return -EINVAL;
> > }
> > -
> > - return ret;
> > }
> >
> > /**
> > @@ -800,9 +782,8 @@ static ssize_t sca3000_read_av_freq(struct device *dev,
> > struct sca3000_state *st = iio_priv(indio_dev);
> > int len = 0, ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > - mutex_unlock(&st->lock);
> > if (ret)
> > return ret;
> >
> > @@ -851,10 +832,9 @@ static int sca3000_read_event_value(struct iio_dev *indio_dev,
> >
> > switch (info) {
> > case IIO_EV_INFO_VALUE:
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = sca3000_read_ctrl_reg(st,
> > sca3000_addresses[chan->address][1]);
> > - mutex_unlock(&st->lock);
> > if (ret < 0)
> > return ret;
> > *val = 0;
> > @@ -918,13 +898,10 @@ static int sca3000_write_event_value(struct iio_dev
> > *indio_dev,
> > }
> > }
> >
> > - mutex_lock(&st->lock);
> > - ret = sca3000_write_ctrl_reg(st,
> > + guard(mutex)(&st->lock);
> > + return sca3000_write_ctrl_reg(st,
> > sca3000_addresses[chan->address][1],
> > nonlinear);
> > - mutex_unlock(&st->lock);
> > -
> > - return ret;
> > }
> >
> > static struct attribute *sca3000_attributes[] = {
> > @@ -969,12 +946,12 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> > *indio_dev)
> > struct sca3000_state *st = iio_priv(indio_dev);
> > int ret, i, num_available;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> >
> > if (val & SCA3000_REG_INT_STATUS_HALF) {
> > ret = spi_w8r8(st->us,
> > SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return;
> > num_available = ret;
> > /*
> > * num_available is the total number of samples available
> > @@ -983,7 +960,7 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> > *indio_dev)
> > ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR,
> > num_available * 2);
> > if (ret)
> > - goto error_ret;
> > + return;
> > for (i = 0; i < num_available / 3; i++) {
> > /*
> > * Dirty hack to cover for 11 bit in fifo, 13 bit
> > @@ -995,8 +972,6 @@ static void sca3000_ring_int_process(u8 val, struct iio_dev
> > *indio_dev)
> > iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
> > }
> > }
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > }
> >
> > /**
> > @@ -1022,9 +997,8 @@ static irqreturn_t sca3000_event_handler(int irq, void
> > *private)
> > * Could lead if badly timed to an extra read of status reg,
> > * but ensures no interrupt is missed.
> > */
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> > - mutex_unlock(&st->lock);
> > if (ret)
> > goto done;
> >
> > @@ -1081,16 +1055,15 @@ static int sca3000_read_event_config(struct iio_dev
> > *indio_dev,
> > struct sca3000_state *st = iio_priv(indio_dev);
> > int ret;
> > /* read current value of mode register */
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> >
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > switch (chan->channel2) {
> > case IIO_MOD_X_AND_Y_AND_Z:
> > - ret = !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
> > - break;
> > + return !!(ret & SCA3000_REG_MODE_FREE_FALL_DETECT);
> > case IIO_MOD_X:
> > case IIO_MOD_Y:
> > case IIO_MOD_Z:
> > @@ -1100,24 +1073,18 @@ static int sca3000_read_event_config(struct iio_dev
> > *indio_dev,
> > */
> > if ((ret & SCA3000_REG_MODE_MODE_MASK)
> > != SCA3000_REG_MODE_MEAS_MODE_MOT_DET) {
> > - ret = 0;
> > + return 0;
> > } else {
> > ret = sca3000_read_ctrl_reg(st,
> > SCA3000_REG_CTRL_SEL_MD_CTRL);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > /* only supporting logical or's for now */
> > - ret = !!(ret & sca3000_addresses[chan->address][2]);
> > + return !!(ret & sca3000_addresses[chan->address][2]);
> > }
> > - break;
> > default:
> > - ret = -EINVAL;
> > + return -EINVAL;
> > }
> > -
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > -
> > - return ret;
> > }
> >
> > static int sca3000_freefall_set_state(struct iio_dev *indio_dev, bool state)
> > @@ -1220,26 +1187,19 @@ static int sca3000_write_event_config(struct iio_dev
> > *indio_dev,
> > struct sca3000_state *st = iio_priv(indio_dev);
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > switch (chan->channel2) {
> > case IIO_MOD_X_AND_Y_AND_Z:
> > - ret = sca3000_freefall_set_state(indio_dev, state);
> > - break;
> > -
> > + return sca3000_freefall_set_state(indio_dev, state);
> > case IIO_MOD_X:
> > case IIO_MOD_Y:
> > case IIO_MOD_Z:
> > - ret = sca3000_motion_detect_set_state(indio_dev,
> > + return sca3000_motion_detect_set_state(indio_dev,
> > chan->address,
> > state);
> > - break;
> > default:
> > - ret = -EINVAL;
> > - break;
> > + return -EINVAL;
> > }
> > - mutex_unlock(&st->lock);
> > -
> > - return ret;
> > }
> >
> > static inline
> > @@ -1248,23 +1208,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev,
> > bool state)
> > struct sca3000_state *st = iio_priv(indio_dev);
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > +
> > if (state) {
> > dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> > - ret = sca3000_write_reg(st,
> > + return sca3000_write_reg(st,
> > SCA3000_REG_MODE_ADDR,
> > (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
> > - } else
> > - ret = sca3000_write_reg(st,
> > - SCA3000_REG_MODE_ADDR,
> > - (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > -
> > - return ret;
> > + }
> > + return sca3000_write_reg(st,
> > + SCA3000_REG_MODE_ADDR,
> > + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> > }
> >
> > /**
> > @@ -1281,26 +1237,18 @@ static int sca3000_hw_ring_preenable(struct iio_dev
> > *indio_dev)
> > int ret;
> > struct sca3000_state *st = iio_priv(indio_dev);
> >
> > - mutex_lock(&st->lock);
> > -
> > + guard(mutex)(&st->lock);
> > /* Enable the 50% full interrupt */
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto error_unlock;
> > + return ret;
> > ret = sca3000_write_reg(st,
> > SCA3000_REG_INT_MASK_ADDR,
> > ret | SCA3000_REG_INT_MASK_RING_HALF);
> > if (ret)
> > - goto error_unlock;
> > -
> > - mutex_unlock(&st->lock);
> > + return ret;
> >
> > return __sca3000_hw_ring_state_set(indio_dev, 1);
> > -
> > -error_unlock:
> > - mutex_unlock(&st->lock);
> > -
> > - return ret;
> > }
> >
> > static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> > @@ -1308,22 +1256,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev
> > *indio_dev)
> > int ret;
> > struct sca3000_state *st = iio_priv(indio_dev);
> >
> > + guard(mutex)(&st->lock);
> > ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> > if (ret)
> > return ret;
> >
> > /* Disable the 50% full interrupt */
> > - mutex_lock(&st->lock);
> > -
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto unlock;
> > - ret = sca3000_write_reg(st,
> > + return ret;
> > + return sca3000_write_reg(st,
> > SCA3000_REG_INT_MASK_ADDR,
> > ret & ~SCA3000_REG_INT_MASK_RING_HALF);
> > -unlock:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > }
> >
> > static const struct iio_buffer_setup_ops sca3000_ring_setup_ops = {
> > @@ -1343,25 +1287,25 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> > {
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > /* Ensure all interrupts have been acknowledged */
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > /* Turn off all motion detection channels */
> > ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_MD_CTRL,
> > ret & SCA3000_MD_CTRL_PROT_MASK);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > /* Disable ring buffer */
> > ret = sca3000_read_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > ret = sca3000_write_ctrl_reg(st, SCA3000_REG_CTRL_SEL_OUT_CTRL,
> > (ret & SCA3000_REG_OUT_CTRL_PROT_MASK)
> > | SCA3000_REG_OUT_CTRL_BUF_X_EN
> > @@ -1369,17 +1313,17 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> > | SCA3000_REG_OUT_CTRL_BUF_Z_EN
> > | SCA3000_REG_OUT_CTRL_BUF_DIV_4);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > /* Enable interrupts, relevant to mode and set up as active low */
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > ret = sca3000_write_reg(st,
> > SCA3000_REG_INT_MASK_ADDR,
> > (ret & SCA3000_REG_INT_MASK_PROT_MASK)
> > | SCA3000_REG_INT_MASK_ACTIVE_LOW);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > /*
> > * Select normal measurement mode, free fall off, ring off
> > * Ring in 12 bit mode - it is fine to overwrite reserved bits 3,5
> > @@ -1387,13 +1331,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> > */
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > if (ret)
> > - goto error_ret;
> > - ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > + return ret;
> > + return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > ret & SCA3000_MODE_PROT_MASK);
> > -
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > }
> >
> > static const struct iio_info sca3000_info = {
> > @@ -1471,19 +1411,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state
> > *st)
> > {
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> > if (ret)
> > - goto error_ret;
> > + return ret;
> >
> > - ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > + return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > ret &
> > ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> > SCA3000_REG_INT_MASK_RING_HALF |
> > SCA3000_REG_INT_MASK_ALL_INTS));
> > -error_ret:
> > - mutex_unlock(&st->lock);
> > - return ret;
> > }
> >
> > static void sca3000_remove(struct spi_device *spi)
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
` (2 preceding siblings ...)
2025-06-12 15:52 ` kernel test robot
@ 2025-06-14 12:01 ` Jonathan Cameron
2025-06-14 21:40 ` Andrew Ijano
3 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2025-06-14 12:01 UTC (permalink / raw)
To: Andrew Ijano
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
On Wed, 11 Jun 2025 16:39:21 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:
> Use guard(mutex)(&st->lock) for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.
>
> 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>
> ---
> For this one, there are two cases where the previous implementation
> was a smalllocking portion of the code and now it's locking the whole
> function. I don't know if this is a desired behavior.
I'd call out more specifically that you are going from
lock
stuff
unlock
call which contains lock over whole useful scope
to
lock
stuff
call with lock no longer done inside
unlock
In at least one case. Looks cleaner to me. I'd guess this is a silly
bit of code evolution that you are tidying up as that lock dance looks
pointless to me.
Otherwise just the {} for cases thing needs fixing up.
Jonathan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-12 6:41 ` Nuno Sá
@ 2025-06-14 19:12 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 19:12 UTC (permalink / raw)
To: Nuno Sá
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns
2025-06-14 11:48 ` Jonathan Cameron
@ 2025-06-14 19:16 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 19:16 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, andrew.lopes, gustavobastos, dlechner, nuno.sa,
andy, jstephan, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-12 6:29 ` Nuno Sá
@ 2025-06-14 19:33 ` Andrew Ijano
2025-06-18 2:36 ` Andrew Ijano
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 19:33 UTC (permalink / raw)
To: Nuno Sá
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On Thu, Jun 12, 2025 at 4:29 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
...
>
> Looks good. Just one comment from me...
>
> > drivers/iio/accel/sca3000.c | 166 +++++++++++++++---------------------
> > 1 file changed, 68 insertions(+), 98 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index bfa8a3f5a92f..d41759c68fb4 100644
> > --- a/drivers/iio/accel/sca3000.c
> > +++ b/drivers/iio/accel/sca3000.c
> > @@ -281,24 +281,6 @@ static int sca3000_write_reg(struct sca3000_state *st, u8
> > address, u8 val)
> > return spi_write(st->us, st->tx, 2);
> > }
> >
>
> ...
>
> >
> > static int sca3000_read_data(struct sca3000_state *st,
> > u8 reg_address_high,
> > - u8 *rx,
> > int len)
> > {
> > int ret;
> > @@ -974,18 +948,15 @@ static int sca3000_read_data(struct sca3000_state *st,
> > .tx_buf = st->tx,
> > }, {
> > .len = len,
> > - .rx_buf = rx,
> > + .rx_buf = st->rx,
> > }
> > };
> > -
> > st->tx[0] = SCA3000_READ_REG(reg_address_high);
> > +
> > ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
> > - if (ret) {
> > + if (ret)
> > dev_err(&st->us->dev, "problem reading register\n");
> > - return ret;
> > - }
> > -
> > - return 0;
> > + return ret;
>
> Unless I'm missing something, the above seems unrelated to the rest of the patch.
>
Oh, I can see why that might feel unrelated. Actually, this was
related to the first version of the patch which was focused on
removing the duplicated behavior of sca3000_read_data() and
sca3000_read_data_short(), unifying it in only one function. Also,
with Andy's suggestions we cleaned up the function as you're seeing
here. However, we later replaced all usages of
sca3000_read_data_short() by spi helpers, leaving just one place
calling sca3000_read_data(), so this change isn't as necessary as
before.
Do you think it's still a valid cleanup for this patch or would you
prefer moving it to a separated one?
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-12 13:22 ` Andy Shevchenko
@ 2025-06-14 21:06 ` Andrew Ijano
2025-06-14 21:50 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 21:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > + ret |= (mode & SCA3000_REG_MODE_MODE_MASK);
>
> Unneeded parentheses.
>
...
>
> > + ret = spi_w8r16be(st->us,
> > + SCA3000_READ_REG(SCA3000_REG_TEMP_MSB_ADDR));
>
> Make it simply one line. The above formatting is ugly.
That's right! I'll fix them.
>
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > struct sca3000_state *st = iio_priv(indio_dev);
> > - int len = 0, ret, val;
> > + int len = 0, ret;
>
> Ideally it's better to split them and len should never be signed.
Nice! I can make this change.
> Moreover, the function should be switched to sysfs_emit_at() if this is part
> of ABI.
Great! I didn't know that.
In this case, sca3000_read_av_freq() is described as a "sysfs function
to get available frequencies", so I guess it's the case, right?
Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
then? If so, I could do that in a following patch, it seems that
sca3000_show_available_3db_freqs() is also using sprintf().
>
> > mutex_lock(&st->lock);
> > - ret = sca3000_read_data_short(st, SCA3000_REG_MODE_ADDR, 1);
> > - val = st->rx[0];
> > + ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> > mutex_unlock(&st->lock);
> > if (ret)
> > return ret;
>
> ...
>
> > }, {
> > .len = len,
> > - .rx_buf = rx,
> > + .rx_buf = st->rx,
> > }
> > };
>
> > -
>
> Stray change. Doesn't checkpatch complain on this?
I don't recall getting any warning from checkpatch but I can check
again for this next version.
> > - (st->rx[0] | SCA3000_REG_MODE_RING_BUF_ENABLE));
> > + (ret | SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> > - (st->rx[0] & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
> > + (ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE));
>
> In the original code and still now too many parentheses.
Ok! I'll remove them.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-12 6:38 ` Nuno Sá
2025-06-14 11:54 ` Jonathan Cameron
@ 2025-06-14 21:17 ` Andrew Ijano
1 sibling, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 21:17 UTC (permalink / raw)
To: Nuno Sá
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On Thu, Jun 12, 2025 at 4:37 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2025-06-11 at 16:39 -0300, Andrew Ijano wrote:
> > ---
> > For this one, there are two cases where the previous implementation
> > was a smalllocking portion of the code and now it's locking the whole
> > function. I don't know if this is a desired behavior.
> >
>
> In theory, it should not break anything. But you can always refactor things (like
> small helpers) to lock only the code you want. There's also scoped_guard(). I would
> say, up to you for re-spinning a new version because of the above :).
Oh, thanks for the suggestion! scoped_guard() is exactly what I was looking for.
> Just have something that I'm not totatlly sure... Did you made sure to compile?
> AFAIR, guard() had some complains when used in switch() case statements. Maybe that
> got improved.
>
Well, it did compile for me, but as David and Jonathan suggested, it's
better to create a scope for that.
In fact, when implementing this I looked to other examples that used
guard() in switch() case statements, but I didn't notice that they
added a local scope too.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-12 16:06 ` David Lechner
@ 2025-06-14 21:21 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 21:21 UTC (permalink / raw)
To: David Lechner
Cc: kernel test robot, jic23, oe-kbuild-all, andrew.lopes,
gustavobastos, nuno.sa, andy, jstephan, linux-iio, linux-kernel
On Thu, Jun 12, 2025 at 1:06 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 6/12/25 10:52 AM, kernel test robot wrote:
> > Hi Andrew,
...
>
> These error messages aren't particularity helpful, but what I think
> this is try to say is that you have to be careful with guard() in
> switch statements.
>
> The guard() macro is declaring a new local variable, which shouldn't
> be done in a case: statement without enclosing it in a separate scope.
> Some compilers complain and some don't so even if it worked for you
> locally, we need to make it work for all supported compilers.
>
> So the code needs to looks something like this:
>
> case IIO_CHAN_INFO_SAMP_FREQ: {
> guard(mutex)(&st->lock);
> ret = sca3000_read_raw_samp_freq(st, val);
> return ret ? ret : IIO_VAL_INT;
> }
Hi, David! Thanks a lot for the explanation. I'll make this change right away.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock
2025-06-14 12:01 ` Jonathan Cameron
@ 2025-06-14 21:40 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-14 21:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: andrew.lopes, gustavobastos, dlechner, nuno.sa, andy, jstephan,
linux-iio, linux-kernel
On Sat, Jun 14, 2025 at 9:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 11 Jun 2025 16:39:21 -0300
> Andrew Ijano <andrew.ijano@gmail.com> wrote:
>
> > Use guard(mutex)(&st->lock) for handling mutex lock instead of
> > manually locking and unlocking the mutex. This prevents forgotten
> > locks due to early exits and remove the need of gotos.
> >
> > 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>
> > ---
> > For this one, there are two cases where the previous implementation
> > was a smalllocking portion of the code and now it's locking the whole
> > function. I don't know if this is a desired behavior.
>
> I'd call out more specifically that you are going from
>
> lock
> stuff
> unlock
> call which contains lock over whole useful scope
>
> to
> lock
> stuff
> call with lock no longer done inside
> unlock
>
> In at least one case. Looks cleaner to me. I'd guess this is a silly
> bit of code evolution that you are tidying up as that lock dance looks
> pointless to me.
Yes! That's something I noticed when making this change and it looked
like a clean up for me too.
What bothered me were cases where we originally had a lock /
spi_w8r8() / unlock and then several operations like iio_push_event()
or sprintf(). I found these cases in sca3000_event_handler() and
sca3000_read_av_freq().
Maybe this isn't as problematic as I'm making them up to be, but it
seems that applying the suggestion of Nino to use scoped_guard()
instead in these cases would already solve this problem.
>
> Otherwise just the {} for cases thing needs fixing up.
Great! I'll fix that too.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-14 21:06 ` Andrew Ijano
@ 2025-06-14 21:50 ` Andy Shevchenko
2025-06-15 3:21 ` Andrew Ijano
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2025-06-14 21:50 UTC (permalink / raw)
To: Andrew Ijano
Cc: Andy Shevchenko, jic23, andrew.lopes, gustavobastos, dlechner,
nuno.sa, andy, jstephan, linux-iio, linux-kernel
On Sun, Jun 15, 2025 at 12:06 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
...
> > Moreover, the function should be switched to sysfs_emit_at() if this is part
> > of ABI.
>
> Great! I didn't know that.
>
> In this case, sca3000_read_av_freq() is described as a "sysfs function
> to get available frequencies", so I guess it's the case, right?
> Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
> then? If so, I could do that in a following patch, it seems that
> sca3000_show_available_3db_freqs() is also using sprintf().
Yes. This is written in the Documentation.
...
> > > }, {
> > > .len = len,
> > > - .rx_buf = rx,
> > > + .rx_buf = st->rx,
> > > }
> > > };
> >
> > > -
> >
> > Stray change. Doesn't checkpatch complain on this?
>
> I don't recall getting any warning from checkpatch but I can check
> again for this next version.
The problem here is the absence of a blank line between the definition
block (where variables are declared/defined) and the first line of the
actual code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-14 21:50 ` Andy Shevchenko
@ 2025-06-15 3:21 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-15 3:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, jic23, andrew.lopes, gustavobastos, dlechner,
nuno.sa, andy, jstephan, linux-iio, linux-kernel
On Sat, Jun 14, 2025 at 6:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 15, 2025 at 12:06 AM Andrew Ijano <andrew.ijano@gmail.com> wrote:
> > On Thu, Jun 12, 2025 at 10:22 AM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
>
> ...
>
> > > Moreover, the function should be switched to sysfs_emit_at() if this is part
> > > of ABI.
> >
> > Great! I didn't know that.
> >
> > In this case, sca3000_read_av_freq() is described as a "sysfs function
> > to get available frequencies", so I guess it's the case, right?
> > Is your suggestion to replace cases of sprintf() by sysfs_emit_at()
> > then? If so, I could do that in a following patch, it seems that
> > sca3000_show_available_3db_freqs() is also using sprintf().
>
> Yes. This is written in the Documentation.
Nice! I'll try to make a new patch later to address that then.
...
> > > > }, {
> > > > .len = len,
> > > > - .rx_buf = rx,
> > > > + .rx_buf = st->rx,
> > > > }
> > > > };
> > >
> > > > -
> > >
> > > Stray change. Doesn't checkpatch complain on this?
> >
> > I don't recall getting any warning from checkpatch but I can check
> > again for this next version.
>
> The problem here is the absence of a blank line between the definition
> block (where variables are declared/defined) and the first line of the
> actual code.
Thanks for the explanation. I double checked and checkpatch doesn't
seem to warn about that, but I fixed it anyway.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers
2025-06-14 19:33 ` Andrew Ijano
@ 2025-06-18 2:36 ` Andrew Ijano
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Ijano @ 2025-06-18 2:36 UTC (permalink / raw)
To: Nuno Sá
Cc: jic23, andrew.lopes, gustavobastos, dlechner, nuno.sa, andy,
jstephan, linux-iio, linux-kernel
On Sat, Jun 14, 2025 at 4:33 PM Andrew Ijano <andrew.ijano@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 4:29 AM Nuno Sá <noname.nuno@gmail.com> wrote:
...
> >
> > Unless I'm missing something, the above seems unrelated to the rest of the patch.
> >
> Oh, I can see why that might feel unrelated. Actually, this was
> related to the first version of the patch which was focused on
> removing the duplicated behavior of sca3000_read_data() and
> sca3000_read_data_short(), unifying it in only one function. Also,
> with Andy's suggestions we cleaned up the function as you're seeing
> here. However, we later replaced all usages of
> sca3000_read_data_short() by spi helpers, leaving just one place
> calling sca3000_read_data(), so this change isn't as necessary as
> before.
>
> Do you think it's still a valid cleanup for this patch or would you
> prefer moving it to a separated one?
Looking back to this, I think we really could benefit from having two
separate patches to handle these different changes.
I'll try to make a new version that addresses all the comments for the
current patchset.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2025-06-18 2:36 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-11 19:39 [PATCH v5 0/3] iio: accel: sca3000: simplify by using newer infrastructure Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 1/3] iio: accel: sca3000: replace error_ret labels by simple returns Andrew Ijano
2025-06-12 6:20 ` Nuno Sá
2025-06-12 6:41 ` Nuno Sá
2025-06-14 19:12 ` Andrew Ijano
2025-06-12 12:56 ` Andy Shevchenko
2025-06-14 11:48 ` Jonathan Cameron
2025-06-14 19:16 ` Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 2/3] iio: accel: sca3000: replace usages of internal read data helpers by spi helpers Andrew Ijano
2025-06-12 6:29 ` Nuno Sá
2025-06-14 19:33 ` Andrew Ijano
2025-06-18 2:36 ` Andrew Ijano
2025-06-12 13:22 ` Andy Shevchenko
2025-06-14 21:06 ` Andrew Ijano
2025-06-14 21:50 ` Andy Shevchenko
2025-06-15 3:21 ` Andrew Ijano
2025-06-11 19:39 ` [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock Andrew Ijano
2025-06-12 6:38 ` Nuno Sá
2025-06-14 11:54 ` Jonathan Cameron
2025-06-14 21:17 ` Andrew Ijano
2025-06-12 14:48 ` kernel test robot
2025-06-12 15:52 ` kernel test robot
2025-06-12 16:06 ` David Lechner
2025-06-14 21:21 ` Andrew Ijano
2025-06-14 12:01 ` Jonathan Cameron
2025-06-14 21:40 ` Andrew Ijano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).