[PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with
the more modern guard(mutex)() family. This will help modernize the
driver and bring it up-to-date with modern available macros/functions.

While replacing mutex_lock() and mutex_unlock(), the critical sections
of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
include negligible operations for cleaner logic.

Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
help keep rm3100_trigger_handler() switch-cases clean while maintaining
mutex locking and avoiding re-entrancy risks from potential callbacks.

While at it, remove redundant gotos where applicable, and use direct
returns instead. In addition, remove regmap variable in
rm3100_trigger_handler() as its references have been replaced with
variable data.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
 - The following excerpts are from the original cover letter:
 - Added small style fixes per Andy's suggestions (Adding blank lines,
    moving an if statement in a scoped_guard block).
 - Switched out scoped_guard() for guard(mutex)() in certain commits.
 - Fixed error in commit 4 where deadlocks could occur, as goto
    ignores __attribute__((cleanup)). This has been fixed by the above.

 v3:
 - Added new helper-wrapper function rm3100_guarded_regmap_bulk_read(),
   see commit message.
 - Squashed commit into one commit rather than four.

 v4:
 - Changed function signature of rm3100_guarded_regmap_bulk_read() to
   supply struct rm3100_data *data instead of struct mutex *lock per
   Andy.
 - Renamed rm3100_guarded_regmap_bulk_read() to
   rm3100_regmap_bulk_read_locked() per Andy.

 v5:
 - Fix kernel doc of rm3100_regmap_bulk_read_locked() per Andy.
 - Added Andy Shevchenko's reviewed-by tag.

 drivers/iio/magnetometer/rm3100-core.c | 93 ++++++++++++++------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 2b2884425746..c0ce2784e97e 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -204,27 +204,23 @@ static int rm3100_read_mag(struct rm3100_data *data, int idx, int *val)
 	u8 buffer[3];
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	ret = regmap_write(regmap, RM3100_REG_POLL, BIT(4 + idx));
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	ret = rm3100_wait_measurement(data);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * idx, buffer, 3);
 	if (ret < 0)
-		goto unlock_return;
-	mutex_unlock(&data->lock);
+		return ret;
 
 	*val = sign_extend32(get_unaligned_be24(&buffer[0]), 23);
 
 	return IIO_VAL_INT;
-
-unlock_return:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 #define RM3100_CHANNEL(axis, idx)					\
@@ -284,11 +280,12 @@ static int rm3100_get_samp_freq(struct rm3100_data *data, int *val, int *val2)
 	unsigned int tmp;
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
-	mutex_unlock(&data->lock);
 	if (ret < 0)
 		return ret;
+
 	*val = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][0];
 	*val2 = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][1];
 
@@ -338,56 +335,50 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 	int ret;
 	int i;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
+
 	/* All cycle count registers use the same value. */
 	ret = regmap_read(regmap, RM3100_REG_CC_X, &cycle_count);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	for (i = 0; i < RM3100_SAMP_NUM; i++) {
 		if (val == rm3100_samp_rates[i][0] &&
 		    val2 == rm3100_samp_rates[i][1])
 			break;
 	}
-	if (i == RM3100_SAMP_NUM) {
-		ret = -EINVAL;
-		goto unlock_return;
-	}
+	if (i == RM3100_SAMP_NUM)
+		return -EINVAL;
 
 	ret = regmap_write(regmap, RM3100_REG_TMRC, i + RM3100_TMRC_OFFSET);
 	if (ret < 0)
-		goto unlock_return;
+		return ret;
 
 	/* Checking if cycle count registers need changing. */
 	if (val == 600 && cycle_count == 200) {
 		ret = rm3100_set_cycle_count(data, 100);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	} else if (val != 600 && cycle_count == 100) {
 		ret = rm3100_set_cycle_count(data, 200);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	}
 
 	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 		ret = regmap_write(data->regmap, RM3100_REG_CMM,
 			(*indio_dev->active_scan_mask & 0x7) <<
 			RM3100_CMM_AXIS_SHIFT | RM3100_CMM_START);
 		if (ret < 0)
-			goto unlock_return;
+			return ret;
 	}
-	mutex_unlock(&data->lock);
 
 	data->conversion_time = rm3100_samp_rates[i][2] * 2;
 	return 0;
-
-unlock_return:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 static int rm3100_read_raw(struct iio_dev *indio_dev,
@@ -458,6 +449,27 @@ static const struct iio_buffer_setup_ops rm3100_buffer_ops = {
 	.postdisable = rm3100_buffer_postdisable,
 };
 
+/**
+ * rm3100_regmap_bulk_read_locked() - Wrapper around regmap_bulk_read() with a mutex
+ *
+ * @data: Data structure containing regmap and mutex
+ * @reg: First register to be read from, passed to regmap_bulk_read()
+ * @val: Pointer to store read value, in native register size for device,
+ * passed to regmap_bulk_read()
+ * @val_count: Number of registers to read, passed to regmap_bulk_read()
+ *
+ * Intended for use only in rm3100_trigger_handler().
+ *
+ * Return:
+ * A value of zero on success, a negative errno in error cases.
+ */
+static int rm3100_regmap_bulk_read_locked(struct rm3100_data *data, unsigned int reg,
+					  void *val, size_t val_count)
+{
+	guard(mutex)(&data->lock);
+	return regmap_bulk_read(data->regmap, reg, val, val_count);
+}
+
 static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -465,14 +477,12 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 	unsigned long scan_mask = *indio_dev->active_scan_mask;
 	unsigned int mask_len = iio_get_masklength(indio_dev);
 	struct rm3100_data *data = iio_priv(indio_dev);
-	struct regmap *regmap = data->regmap;
 	int ret, i, bit;
 
-	mutex_lock(&data->lock);
 	switch (scan_mask) {
 	case BIT(0) | BIT(1) | BIT(2):
-		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
-		mutex_unlock(&data->lock);
+		ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MX2,
+						     data->buffer, 9);
 		if (ret < 0)
 			goto done;
 		/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for paddings. */
@@ -480,36 +490,33 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
 			memmove(data->buffer + i * 4, data->buffer + i * 3, 3);
 		break;
 	case BIT(0) | BIT(1):
-		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
-		mutex_unlock(&data->lock);
+		ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MX2,
+						     data->buffer, 6);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 3, 3);
 		break;
 	case BIT(1) | BIT(2):
-		ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
-		mutex_unlock(&data->lock);
+		ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MY2,
+						     data->buffer, 6);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 3, 3);
 		break;
 	case BIT(0) | BIT(2):
-		ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
-		mutex_unlock(&data->lock);
+		ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MX2,
+						     data->buffer, 9);
 		if (ret < 0)
 			goto done;
 		memmove(data->buffer + 4, data->buffer + 6, 3);
 		break;
 	default:
 		for_each_set_bit(bit, &scan_mask, mask_len) {
-			ret = regmap_bulk_read(regmap, RM3100_REG_MX2 + 3 * bit,
-					       data->buffer, 3);
-			if (ret < 0) {
-				mutex_unlock(&data->lock);
+			ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MX2 + 3 * bit,
+							     data->buffer, 3);
+			if (ret < 0)
 				goto done;
-			}
 		}
-		mutex_unlock(&data->lock);
 	}
 	/*
 	 * Always using the same buffer so that we wouldn't need to set the
-- 
2.53.0

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Thu, Apr 30, 2026 at 7:41 AM Maxwell Doose <m32285159@gmail.com> wrote:
>
> Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with
> the more modern guard(mutex)() family. This will help modernize the
> driver and bring it up-to-date with modern available macros/functions.
>
> While replacing mutex_lock() and mutex_unlock(), the critical sections
> of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
> include negligible operations for cleaner logic.
>
> Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
> help keep rm3100_trigger_handler() switch-cases clean while maintaining
> mutex locking and avoiding re-entrancy risks from potential callbacks.
>
> While at it, remove redundant gotos where applicable, and use direct
> returns instead. In addition, remove regmap variable in
> rm3100_trigger_handler() as its references have been replaced with
> variable data.
>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
>  v2:
>  - The following excerpts are from the original cover letter:
>  - Added small style fixes per Andy's suggestions (Adding blank lines,
>     moving an if statement in a scoped_guard block).
>  - Switched out scoped_guard() for guard(mutex)() in certain commits.
>  - Fixed error in commit 4 where deadlocks could occur, as goto
>     ignores __attribute__((cleanup)). This has been fixed by the above.
>
>  v3:
>  - Added new helper-wrapper function rm3100_guarded_regmap_bulk_read(),
>    see commit message.
>  - Squashed commit into one commit rather than four.
>
>  v4:
>  - Changed function signature of rm3100_guarded_regmap_bulk_read() to
>    supply struct rm3100_data *data instead of struct mutex *lock per
>    Andy.
>  - Renamed rm3100_guarded_regmap_bulk_read() to
>    rm3100_regmap_bulk_read_locked() per Andy.
>
>  v5:
>  - Fix kernel doc of rm3100_regmap_bulk_read_locked() per Andy.
>  - Added Andy Shevchenko's reviewed-by tag.
>
>  drivers/iio/magnetometer/rm3100-core.c | 93 ++++++++++++++------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
>
[snip]

Hi, just wanted to put the link to where Andy gave me permission to
use his reviewed-by tag just to make sure no confusion arises.
https://lore.kernel.org/linux-iio/CAKqfh0H1tq1eC5h54G1Vft26Qkaa4nYEdhwQAk=1R6CNXgnveQ@mail.gmail.com/T/#m5ad582163d5d9620c59edcd1a78eceee070677f7

best regards,
maxwell

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Thu, Apr 30, 2026 at 07:41:47AM -0500, Maxwell Doose wrote:
> Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with
> the more modern guard(mutex)() family. This will help modernize the
> driver and bring it up-to-date with modern available macros/functions.
> 
> While replacing mutex_lock() and mutex_unlock(), the critical sections
> of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to
> include negligible operations for cleaner logic.
> 
> Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to
> help keep rm3100_trigger_handler() switch-cases clean while maintaining
> mutex locking and avoiding re-entrancy risks from potential callbacks.
> 
> While at it, remove redundant gotos where applicable, and use direct
> returns instead. In addition, remove regmap variable in
> rm3100_trigger_handler() as its references have been replaced with
> variable data.

...

> +			ret = rm3100_regmap_bulk_read_locked(data, RM3100_REG_MX2 + 3 * bit,
> +							     data->buffer, 3);

Jonathan, if you feel to tweak this to make it shorter, I mentioned previously
this:

			ret = rm3100_regmap_bulk_read_locked(data,
							     RM3100_REG_MX2 + 3 * bit,
							     data->buffer, 3);

> +			if (ret < 0)
>  				goto done;

I confirm that my Rb stays with or without this change.
Maxwell, no need to resend for this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Thu, Apr 30, 2026 at 7:58 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> I confirm that my Rb stays with or without this change.
> Maxwell, no need to resend for this.
>

Alright, sounds good.

Best regards,
Maxwell Doose





> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Jonathan Cameron]

On Thu, 30 Apr 2026 10:43:58 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Thu, Apr 30, 2026 at 7:58 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > I confirm that my Rb stays with or without this change.
> > Maxwell, no need to resend for this.
> >  
> 
> Alright, sounds good.
> 
> Best regards,
> Maxwell Doose
Applied to the testing branch of iio.git with that tweak.

Thanks,

Jonathan

> 
> 
> 
> 
> 
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >  

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Tue, May 5, 2026 at 7:45 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Applied to the testing branch of iio.git with that tweak.
>
> Thanks,
>
> Jonathan
>

Hey Jonathan, I just noticed that I made a mistake in the commit
message, it says "Add new helper-wrapper function
rm3100_guarded_regmap_bulk_read()..." when it really should be "Add
new helper-wrapper function rm3100_regmap_bulk_read_locked()..." I'm
unfortunately not in a position to fix it myself since I'm away, but I
just wanted to point it out so it could be fixed.

best regards,
max

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Jonathan Cameron]

On Tue, 5 May 2026 12:05:11 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, May 5, 2026 at 7:45 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > Applied to the testing branch of iio.git with that tweak.
> >
> > Thanks,
> >
> > Jonathan
> >  
> 
> Hey Jonathan, I just noticed that I made a mistake in the commit
> message, it says "Add new helper-wrapper function
> rm3100_guarded_regmap_bulk_read()..." when it really should be "Add
> new helper-wrapper function rm3100_regmap_bulk_read_locked()..." I'm
> unfortunately not in a position to fix it myself since I'm away, but I
> just wanted to point it out so it could be fixed.
fixed up.
> 
> best regards,
> max

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Maxwell Doose]

On Wed May 6, 2026 at 9:43 AM CDT, Jonathan Cameron wrote:
> On Tue, 5 May 2026 12:05:11 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
>
>> On Tue, May 5, 2026 at 7:45 AM Jonathan Cameron <jic23@kernel.org> wrote:
>> >
>> > Applied to the testing branch of iio.git with that tweak.
>> >
>> > Thanks,
>> >
>> > Jonathan
>> >  
>> 
>> Hey Jonathan, I just noticed that I made a mistake in the commit
>> message, it says "Add new helper-wrapper function
>> rm3100_guarded_regmap_bulk_read()..." when it really should be "Add
>> new helper-wrapper function rm3100_regmap_bulk_read_locked()..." I'm
>> unfortunately not in a position to fix it myself since I'm away, but I
>> just wanted to point it out so it could be fixed.
> fixed up.
>> 
>> best regards,
>> max

Gosh, I'm so sorry to keep bothering you, but I noticed a stray change
here:

@@ -7,7 +7,7 @@
  * User Manual available at
  * <https://www.pnicorp.com/download/rm3100-user-manual/>
  *
- * TODO: event generation, pm.
+ * TODO: event generation, pm.da
  */

wishing vs code would quit messing with me,
max

Re: [PATCH v5] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Jonathan Cameron]

On Wed, 06 May 2026 10:28:29 -0500
"Maxwell Doose" <m32285159@gmail.com> wrote:

> On Wed May 6, 2026 at 9:43 AM CDT, Jonathan Cameron wrote:
> > On Tue, 5 May 2026 12:05:11 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> >  
> >> On Tue, May 5, 2026 at 7:45 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> >> >
> >> > Applied to the testing branch of iio.git with that tweak.
> >> >
> >> > Thanks,
> >> >
> >> > Jonathan
> >> >    
> >> 
> >> Hey Jonathan, I just noticed that I made a mistake in the commit
> >> message, it says "Add new helper-wrapper function
> >> rm3100_guarded_regmap_bulk_read()..." when it really should be "Add
> >> new helper-wrapper function rm3100_regmap_bulk_read_locked()..." I'm
> >> unfortunately not in a position to fix it myself since I'm away, but I
> >> just wanted to point it out so it could be fixed.  
> > fixed up.  
> >> 
> >> best regards,
> >> max  
> 
> Gosh, I'm so sorry to keep bothering you, but I noticed a stray change
> here:
> 
> @@ -7,7 +7,7 @@
>   * User Manual available at
>   * <https://www.pnicorp.com/download/rm3100-user-manual/>
>   *
> - * TODO: event generation, pm.
> + * TODO: event generation, pm.da
>   */
> 
> wishing vs code would quit messing with me,
> max

Doh.  That wasn't there in the original so I must have messed up.
Good spot.

Fixed up.