* [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors
@ 2024-11-21 13:04 Matti Vaittinen
2024-11-21 13:05 ` [PATCH 1/2] iio: bu27034: simplify using guard(mutex) Matti Vaittinen
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Matti Vaittinen @ 2024-11-21 13:04 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Jonathan Cameron, Lars-Peter Clausen, linux-iio,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
Use __cleanup.
The series converts the rest of the ROHM sensors (maintained by me) to
use guard(mutex). This simplifies the error paths.
As a note, kx022a accelerometer driver is handled in another series,
which also adds support for two new accelerometers. I did also patch the
driver for the BU27008 and BU27010 - but when I was testing the changes
I found that the BU27008 status is set to "obsolete". I'll try to dig
some information about the BU27010 and decide if having the driver
in-tree is still worth the effort, or if I should just send out patches
to drop it all. Hence patch to rohm-bu27008.c is not included in the
series. If someone is actually using the BU27008 or BU27010 and wants
to patch it - feel free to pick
131315de97ff ("iio: bu27008: simplify using guard(mutex)")
from
https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
---
Matti Vaittinen (2):
iio: bu27034: simplify using guard(mutex)
iio: bm1390: simplify using guard(mutex)
drivers/iio/light/rohm-bu27034.c | 73 ++++++++++------------------
drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
2 files changed, 55 insertions(+), 96 deletions(-)
base-commit: adc218676eef25575469234709c2d87185ca223a
--
2.47.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] iio: bu27034: simplify using guard(mutex)
2024-11-21 13:04 [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Matti Vaittinen
@ 2024-11-21 13:05 ` Matti Vaittinen
2024-11-22 6:40 ` Javier Carrasco
2024-11-21 13:05 ` [PATCH 2/2] iio: bm1390: " Matti Vaittinen
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2024-11-21 13:05 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Jonathan Cameron, Lars-Peter Clausen, linux-iio,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5449 bytes --]
The BU27034 uses mutex for protecting the gain / time / scale changes.
The clean-up for a few of the functions can be slightly simplified by
removing the goto-based error handling/unlocking and by utilizing the
guard(mutex) scoped mutex handling instead.
Simplify driver by using the scoped mutexes.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
drivers/iio/light/rohm-bu27034.c | 73 +++++++++++---------------------
1 file changed, 25 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
index 76711c3cdf7c..d2ed92d5ec92 100644
--- a/drivers/iio/light/rohm-bu27034.c
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -7,6 +7,7 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/i2c.h>
#include <linux/module.h>
@@ -395,30 +396,26 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
int numg = ARRAY_SIZE(gains);
int ret, int_time_old, i;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bu27034_get_int_time(data);
if (ret < 0)
- goto unlock_out;
+ return ret;
int_time_old = ret;
if (!iio_gts_valid_time(&data->gts, time_us)) {
dev_err(data->dev, "Unsupported integration time %u\n",
time_us);
- ret = -EINVAL;
-
- goto unlock_out;
+ return -EINVAL;
}
- if (time_us == int_time_old) {
- ret = 0;
- goto unlock_out;
- }
+ if (time_us == int_time_old)
+ return 0;
for (i = 0; i < numg; i++) {
ret = bu27034_get_gain(data, gains[i].chan, &gains[i].old_gain);
if (ret)
- goto unlock_out;
+ return 0;
ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts,
gains[i].old_gain,
@@ -434,7 +431,7 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
gains[i].chan, time_us, scale1, scale2);
if (gains[i].new_gain < 0)
- goto unlock_out;
+ return ret;
/*
* If caller requests for integration time change and we
@@ -455,7 +452,7 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
"Total gain increase. Risk of saturation");
ret = iio_gts_get_min_gain(&data->gts);
if (ret < 0)
- goto unlock_out;
+ return ret;
}
dev_dbg(data->dev, "chan %u scale changed\n",
gains[i].chan);
@@ -468,15 +465,10 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
for (i = 0; i < numg; i++) {
ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
if (ret)
- goto unlock_out;
+ return ret;
}
- ret = bu27034_set_int_time(data, time_us);
-
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bu27034_set_int_time(data, time_us);
}
static int bu27034_set_scale(struct bu27034_data *data, int chan,
@@ -492,10 +484,10 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
return -EINVAL;
}
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel);
if (ret)
- goto unlock_out;
+ return ret;
ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
val, val2, &gain_sel);
@@ -518,7 +510,7 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
ret = bu27034_get_gain(data, gain.chan, &gain.old_gain);
if (ret)
- goto unlock_out;
+ return ret;
/*
* Iterate through all the times to see if we find one which
@@ -551,26 +543,20 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
if (!found) {
dev_dbg(data->dev,
"Can't set scale maintaining other channel\n");
- ret = -EINVAL;
-
- goto unlock_out;
+ return -EINVAL;
}
ret = bu27034_set_gain(data, gain.chan, gain.new_gain);
if (ret)
- goto unlock_out;
+ return ret;
ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1,
BU27034_MASK_MEAS_MODE, new_time_sel);
if (ret)
- goto unlock_out;
+ return ret;
}
- ret = bu27034_write_gain_sel(data, chan, gain_sel);
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bu27034_write_gain_sel(data, chan, gain_sel);
}
/*
@@ -1221,42 +1207,33 @@ static int bu27034_buffer_enable(struct iio_dev *idev)
struct task_struct *task;
int ret;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bu27034_meas_set(data, true);
if (ret)
- goto unlock_out;
+ return ret;
task = kthread_run(bu27034_buffer_thread, idev,
"bu27034-buffering-%u",
iio_device_id(idev));
- if (IS_ERR(task)) {
- ret = PTR_ERR(task);
- goto unlock_out;
- }
+ if (IS_ERR(task))
+ return PTR_ERR(task);
data->task = task;
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
+ return 0;
}
static int bu27034_buffer_disable(struct iio_dev *idev)
{
struct bu27034_data *data = iio_priv(idev);
- int ret;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
if (data->task) {
kthread_stop(data->task);
data->task = NULL;
}
- ret = bu27034_meas_set(data, false);
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bu27034_meas_set(data, false);
}
static const struct iio_buffer_setup_ops bu27034_buffer_ops = {
--
2.47.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] iio: bm1390: simplify using guard(mutex)
2024-11-21 13:04 [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Matti Vaittinen
2024-11-21 13:05 ` [PATCH 1/2] iio: bu27034: simplify using guard(mutex) Matti Vaittinen
@ 2024-11-21 13:05 ` Matti Vaittinen
2024-11-22 6:41 ` Javier Carrasco
2024-11-21 13:54 ` [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Javier Carrasco
2024-11-23 16:46 ` Jonathan Cameron
3 siblings, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2024-11-21 13:05 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Jonathan Cameron, Lars-Peter Clausen, linux-iio,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]
The BM1390 uses mutex for protecting the fifo read sequence. The clean-up
for a few of the functions can be slightly simplified by removing the
goto-based error handling/unlocking and by utilizing the guard(mutex)
scoped mutex handling instead.
Simplify driver by using the scoped mutexes.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
1 file changed, 30 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
index ccaa07a569c9..fa1a477151d0 100644
--- a/drivers/iio/pressure/rohm-bm1390.c
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -8,6 +8,7 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/i2c.h>
#include <linux/module.h>
@@ -263,14 +264,14 @@ static int bm1390_read_data(struct bm1390_data *data,
{
int ret, warn;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
/*
* We use 'continuous mode' even for raw read because according to the
* data-sheet an one-shot mode can't be used with IIR filter.
*/
ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
if (ret)
- goto unlock_out;
+ return ret;
switch (chan->type) {
case IIO_PRESSURE:
@@ -287,10 +288,8 @@ static int bm1390_read_data(struct bm1390_data *data,
warn = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
if (warn)
dev_warn(data->dev, "Failed to stop measurement (%d)\n", warn);
-unlock_out:
- mutex_unlock(&data->mutex);
- return ret;
+ return 0;
}
static int bm1390_read_raw(struct iio_dev *idev,
@@ -546,38 +545,33 @@ static int bm1390_fifo_enable(struct iio_dev *idev)
if (data->irq <= 0)
return -EINVAL;
- mutex_lock(&data->mutex);
- if (data->trigger_enabled) {
- ret = -EBUSY;
- goto unlock_out;
- }
+ guard(mutex)(&data->mutex);
+
+ if (data->trigger_enabled)
+ return -EBUSY;
/* Update watermark to HW */
ret = bm1390_fifo_set_wmi(data);
if (ret)
- goto unlock_out;
+ return ret;
/* Enable WMI_IRQ */
ret = regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
BM1390_MASK_WMI_EN);
if (ret)
- goto unlock_out;
+ return ret;
/* Enable FIFO */
ret = regmap_set_bits(data->regmap, BM1390_REG_FIFO_CTRL,
BM1390_MASK_FIFO_EN);
if (ret)
- goto unlock_out;
+ return ret;
data->state = BM1390_STATE_FIFO;
data->old_timestamp = iio_get_time_ns(idev);
- ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
}
static int bm1390_fifo_disable(struct iio_dev *idev)
@@ -587,27 +581,22 @@ static int bm1390_fifo_disable(struct iio_dev *idev)
msleep(1);
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
if (ret)
- goto unlock_out;
+ return ret;
/* Disable FIFO */
ret = regmap_clear_bits(data->regmap, BM1390_REG_FIFO_CTRL,
BM1390_MASK_FIFO_EN);
if (ret)
- goto unlock_out;
+ return ret;
data->state = BM1390_STATE_SAMPLE;
/* Disable WMI_IRQ */
- ret = regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
+ return regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
BM1390_MASK_WMI_EN);
-
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
}
static int bm1390_buffer_postenable(struct iio_dev *idev)
@@ -691,25 +680,24 @@ static irqreturn_t bm1390_irq_thread_handler(int irq, void *private)
{
struct iio_dev *idev = private;
struct bm1390_data *data = iio_priv(idev);
- int ret = IRQ_NONE;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
if (data->trigger_enabled) {
iio_trigger_poll_nested(data->trig);
- ret = IRQ_HANDLED;
- } else if (data->state == BM1390_STATE_FIFO) {
+ return IRQ_HANDLED;
+ }
+
+ if (data->state == BM1390_STATE_FIFO) {
int ok;
ok = __bm1390_fifo_flush(idev, BM1390_FIFO_LENGTH,
data->timestamp);
if (ok > 0)
- ret = IRQ_HANDLED;
+ return IRQ_HANDLED;
}
- mutex_unlock(&data->mutex);
-
- return ret;
+ return IRQ_NONE;
}
static int bm1390_set_drdy_irq(struct bm1390_data *data, bool en)
@@ -725,17 +713,16 @@ static int bm1390_trigger_set_state(struct iio_trigger *trig,
bool state)
{
struct bm1390_data *data = iio_trigger_get_drvdata(trig);
- int ret = 0;
+ int ret;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
if (data->trigger_enabled == state)
- goto unlock_out;
+ return 0;
if (data->state == BM1390_STATE_FIFO) {
dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
- ret = -EBUSY;
- goto unlock_out;
+ return -EBUSY;
}
data->trigger_enabled = state;
@@ -743,13 +730,13 @@ static int bm1390_trigger_set_state(struct iio_trigger *trig,
if (state) {
ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
if (ret)
- goto unlock_out;
+ return ret;
} else {
int dummy;
ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
if (ret)
- goto unlock_out;
+ return ret;
/*
* We need to read the status register in order to ACK the
@@ -761,12 +748,7 @@ static int bm1390_trigger_set_state(struct iio_trigger *trig,
dev_warn(data->dev, "status read failed\n");
}
- ret = bm1390_set_drdy_irq(data, state);
-
-unlock_out:
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bm1390_set_drdy_irq(data, state);
}
static const struct iio_trigger_ops bm1390_trigger_ops = {
--
2.47.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors
2024-11-21 13:04 [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Matti Vaittinen
2024-11-21 13:05 ` [PATCH 1/2] iio: bu27034: simplify using guard(mutex) Matti Vaittinen
2024-11-21 13:05 ` [PATCH 2/2] iio: bm1390: " Matti Vaittinen
@ 2024-11-21 13:54 ` Javier Carrasco
2024-11-22 6:10 ` Matti Vaittinen
2024-11-23 16:46 ` Jonathan Cameron
3 siblings, 1 reply; 9+ messages in thread
From: Javier Carrasco @ 2024-11-21 13:54 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
On 21/11/2024 14:04, Matti Vaittinen wrote:
> Use __cleanup.
>
> The series converts the rest of the ROHM sensors (maintained by me) to
> use guard(mutex). This simplifies the error paths.
>
> As a note, kx022a accelerometer driver is handled in another series,
> which also adds support for two new accelerometers. I did also patch the
> driver for the BU27008 and BU27010 - but when I was testing the changes
> I found that the BU27008 status is set to "obsolete". I'll try to dig
> some information about the BU27010 and decide if having the driver
> in-tree is still worth the effort, or if I should just send out patches
> to drop it all. Hence patch to rohm-bu27008.c is not included in the
> series. If someone is actually using the BU27008 or BU27010 and wants
> to patch it - feel free to pick
> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
> from
> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>
> ---
>
> Matti Vaittinen (2):
> iio: bu27034: simplify using guard(mutex)
> iio: bm1390: simplify using guard(mutex)
>
> drivers/iio/light/rohm-bu27034.c | 73 ++++++++++------------------
> drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
> 2 files changed, 55 insertions(+), 96 deletions(-)
>
>
> base-commit: adc218676eef25575469234709c2d87185ca223a
Hi Matti,
Both patches look good to me, but I noticed that you kept a few
mutex_lock() + mutex_unlock() in both drivers, in particular in the
cases where a scoped_guard() could simplify the code. Did you leave
those cases untouched on purpose?
Best regards,
Javier Carrasco
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors
2024-11-21 13:54 ` [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Javier Carrasco
@ 2024-11-22 6:10 ` Matti Vaittinen
2024-11-22 6:40 ` Javier Carrasco
0 siblings, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2024-11-22 6:10 UTC (permalink / raw)
To: Javier Carrasco, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
Hi Javier,
On 21/11/2024 15:54, Javier Carrasco wrote:
> On 21/11/2024 14:04, Matti Vaittinen wrote:
>> Use __cleanup.
>>
>> The series converts the rest of the ROHM sensors (maintained by me) to
>> use guard(mutex). This simplifies the error paths.
>>
>> As a note, kx022a accelerometer driver is handled in another series,
>> which also adds support for two new accelerometers. I did also patch the
>> driver for the BU27008 and BU27010 - but when I was testing the changes
>> I found that the BU27008 status is set to "obsolete". I'll try to dig
>> some information about the BU27010 and decide if having the driver
>> in-tree is still worth the effort, or if I should just send out patches
>> to drop it all. Hence patch to rohm-bu27008.c is not included in the
>> series. If someone is actually using the BU27008 or BU27010 and wants
>> to patch it - feel free to pick
>> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
>> from
>> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>>
>> ---
>>
>> Matti Vaittinen (2):
>> iio: bu27034: simplify using guard(mutex)
>> iio: bm1390: simplify using guard(mutex)
>>
>> drivers/iio/light/rohm-bu27034.c | 73 ++++++++++------------------
>> drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>> 2 files changed, 55 insertions(+), 96 deletions(-)
>>
>>
>> base-commit: adc218676eef25575469234709c2d87185ca223a
>
> Hi Matti,
>
> Both patches look good to me, but I noticed that you kept a few
> mutex_lock() + mutex_unlock() in both drivers, in particular in the
> cases where a scoped_guard() could simplify the code. Did you leave
> those cases untouched on purpose?
Thanks for taking a look at the patches. Much appreciated :)
I remember leaving couple of direct calls to mutex_lock() and
mutex_unlock() - but I think I left them only to places where I saw no
real improvement by the use of guard() or scoped_guard(). It is likely I
considered the locking in these cases being trivial. (Probably only for
a duration of one or couple of function calls, with no error handling
when a lock is held). The direct mutex_lock()/mutex_unlock() has no real
room for usual errors (like leaving the function while lock was taken)
in such case.
For me,
mutex_lock();
ret = foo();
mutex_unlock();
is as clear as it gets. I don't think scoped_guard() has benefits there.
On the contrary, for me the scoped_guard() would be more complex and
less obvious :)
Yours,
-- Matti
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors
2024-11-22 6:10 ` Matti Vaittinen
@ 2024-11-22 6:40 ` Javier Carrasco
0 siblings, 0 replies; 9+ messages in thread
From: Javier Carrasco @ 2024-11-22 6:40 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
On 22/11/2024 07:10, Matti Vaittinen wrote:
> Hi Javier,
>
> On 21/11/2024 15:54, Javier Carrasco wrote:
>> On 21/11/2024 14:04, Matti Vaittinen wrote:
>>> Use __cleanup.
>>>
>>> The series converts the rest of the ROHM sensors (maintained by me) to
>>> use guard(mutex). This simplifies the error paths.
>>>
>>> As a note, kx022a accelerometer driver is handled in another series,
>>> which also adds support for two new accelerometers. I did also patch the
>>> driver for the BU27008 and BU27010 - but when I was testing the changes
>>> I found that the BU27008 status is set to "obsolete". I'll try to dig
>>> some information about the BU27010 and decide if having the driver
>>> in-tree is still worth the effort, or if I should just send out patches
>>> to drop it all. Hence patch to rohm-bu27008.c is not included in the
>>> series. If someone is actually using the BU27008 or BU27010 and wants
>>> to patch it - feel free to pick
>>> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
>>> from
>>> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>>>
>>> ---
>>>
>>> Matti Vaittinen (2):
>>> iio: bu27034: simplify using guard(mutex)
>>> iio: bm1390: simplify using guard(mutex)
>>>
>>> drivers/iio/light/rohm-bu27034.c | 73 ++++++++++------------------
>>> drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
>>> 2 files changed, 55 insertions(+), 96 deletions(-)
>>>
>>>
>>> base-commit: adc218676eef25575469234709c2d87185ca223a
>>
>> Hi Matti,
>>
>> Both patches look good to me, but I noticed that you kept a few
>> mutex_lock() + mutex_unlock() in both drivers, in particular in the
>> cases where a scoped_guard() could simplify the code. Did you leave
>> those cases untouched on purpose?
>
> Thanks for taking a look at the patches. Much appreciated :)
>
> I remember leaving couple of direct calls to mutex_lock() and
> mutex_unlock() - but I think I left them only to places where I saw no
> real improvement by the use of guard() or scoped_guard(). It is likely I
> considered the locking in these cases being trivial. (Probably only for
> a duration of one or couple of function calls, with no error handling
> when a lock is held). The direct mutex_lock()/mutex_unlock() has no real
> room for usual errors (like leaving the function while lock was taken)
> in such case.
>
> For me,
>
> mutex_lock();
> ret = foo();
> mutex_unlock();
>
> is as clear as it gets. I don't think scoped_guard() has benefits there.
> On the contrary, for me the scoped_guard() would be more complex and
> less obvious :)
>
> Yours,
> -- Matti
>
Yes, the cases I saw had very restricted scope. I just wanted to make
sure that you left them untouched on purpose. Often such refactoring of
mutex handling opts for removing all calls of mutex_lock/unlock to avoid
mixing both approaches in the same driver.
Personally, I like the scoped_guard() for short scopes too because it is
more robust if new code is added in that scope. But that is just a
preference :)
Best regards,
Javier Carrasco
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iio: bu27034: simplify using guard(mutex)
2024-11-21 13:05 ` [PATCH 1/2] iio: bu27034: simplify using guard(mutex) Matti Vaittinen
@ 2024-11-22 6:40 ` Javier Carrasco
0 siblings, 0 replies; 9+ messages in thread
From: Javier Carrasco @ 2024-11-22 6:40 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
On 21/11/2024 14:05, Matti Vaittinen wrote:
> The BU27034 uses mutex for protecting the gain / time / scale changes.
> The clean-up for a few of the functions can be slightly simplified by
> removing the goto-based error handling/unlocking and by utilizing the
> guard(mutex) scoped mutex handling instead.
>
> Simplify driver by using the scoped mutexes.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> drivers/iio/light/rohm-bu27034.c | 73 +++++++++++---------------------
> 1 file changed, 25 insertions(+), 48 deletions(-)
>
Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: bm1390: simplify using guard(mutex)
2024-11-21 13:05 ` [PATCH 2/2] iio: bm1390: " Matti Vaittinen
@ 2024-11-22 6:41 ` Javier Carrasco
0 siblings, 0 replies; 9+ messages in thread
From: Javier Carrasco @ 2024-11-22 6:41 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
On 21/11/2024 14:05, Matti Vaittinen wrote:
> The BM1390 uses mutex for protecting the fifo read sequence. The clean-up
> for a few of the functions can be slightly simplified by removing the
> goto-based error handling/unlocking and by utilizing the guard(mutex)
> scoped mutex handling instead.
>
> Simplify driver by using the scoped mutexes.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
> 1 file changed, 30 insertions(+), 48 deletions(-)
>
Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors
2024-11-21 13:04 [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Matti Vaittinen
` (2 preceding siblings ...)
2024-11-21 13:54 ` [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Javier Carrasco
@ 2024-11-23 16:46 ` Jonathan Cameron
3 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2024-11-23 16:46 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lars-Peter Clausen, linux-iio, linux-kernel
On Thu, 21 Nov 2024 15:04:53 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Use __cleanup.
>
> The series converts the rest of the ROHM sensors (maintained by me) to
> use guard(mutex). This simplifies the error paths.
>
> As a note, kx022a accelerometer driver is handled in another series,
> which also adds support for two new accelerometers. I did also patch the
> driver for the BU27008 and BU27010 - but when I was testing the changes
> I found that the BU27008 status is set to "obsolete". I'll try to dig
> some information about the BU27010 and decide if having the driver
> in-tree is still worth the effort, or if I should just send out patches
> to drop it all. Hence patch to rohm-bu27008.c is not included in the
> series. If someone is actually using the BU27008 or BU27010 and wants
> to patch it - feel free to pick
> 131315de97ff ("iio: bu27008: simplify using guard(mutex)")
> from
> https://github.com/M-Vaittinen/linux/tree/bu27008-cleanup
>
Applied. Thanks,
J
> ---
>
> Matti Vaittinen (2):
> iio: bu27034: simplify using guard(mutex)
> iio: bm1390: simplify using guard(mutex)
>
> drivers/iio/light/rohm-bu27034.c | 73 ++++++++++------------------
> drivers/iio/pressure/rohm-bm1390.c | 78 ++++++++++++------------------
> 2 files changed, 55 insertions(+), 96 deletions(-)
>
>
> base-commit: adc218676eef25575469234709c2d87185ca223a
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-11-23 16:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-21 13:04 [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Matti Vaittinen
2024-11-21 13:05 ` [PATCH 1/2] iio: bu27034: simplify using guard(mutex) Matti Vaittinen
2024-11-22 6:40 ` Javier Carrasco
2024-11-21 13:05 ` [PATCH 2/2] iio: bm1390: " Matti Vaittinen
2024-11-22 6:41 ` Javier Carrasco
2024-11-21 13:54 ` [PATCH 0/2] iio: Use __cleanup for a few ROHM sensors Javier Carrasco
2024-11-22 6:10 ` Matti Vaittinen
2024-11-22 6:40 ` Javier Carrasco
2024-11-23 16:46 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox