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

[PATCH v4] 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>
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 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..ffa9b46278ed 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_guarded_regmap_bulk_read() - 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 v4] iio: magnetometer: rm3100: Modernize locking and refactor control flow

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 09:14:39PM -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.

Almost there, a couple of minor issues and feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

(Side note:
 compare this change to ACPI PMIC one, you see what I have told about
 in that thread.)

...

> +/**
> + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex

Always validate kernel-doc

	scripts/kernel-doc -v -none -Wall -- $FILE


You forgot to update function name here.

> + * @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.
> + */

...

>  		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);

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Maxwell Doose]

On Thu, Apr 30, 2026 at 1:31 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> Almost there, a couple of minor issues and feel free to add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>

Alright sounds good, I'll make sure to add the recommended changes
below, but right now it's incredibly late over here, so I'll get back
with that patch later.

> (Side note:
>  compare this change to ACPI PMIC one, you see what I have told about
>  in that thread.)

I probably should stop wasting my time over there, I'm tripping over
my own commit details there and that driver seems to be at the point
of diminishing returns.

>
> ...
>
> > +/**
> > + * rm3100_guarded_regmap_bulk_read() - Wrapper around regmap_bulk_read() with a mutex
>
> Always validate kernel-doc
>
>         scripts/kernel-doc -v -none -Wall -- $FILE

Thanks for informing me about that, I didn't realize there was a
script for that.

>
>
> You forgot to update function name here.

Whoops, nice catch.

Anyways, I think I need to sincerely thank you for your help on this,
it's been great.

Best Regards,
Maxwell Doose




>
> > + * @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.
> > + */
>
> ...
>
> >               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);
>
>                         ret = rm3100_regmap_bulk_read_locked(data,
>                                                              RM3100_REG_MX2 + 3 * bit,
>                                                              data->buffer, 3);
>
> > +                     if (ret < 0)
> >                               goto done;
> > -                     }
> >               }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[kernel test robot]

Hi Maxwell,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v7.1-rc1 next-20260430]
[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/Maxwell-Doose/iio-magnetometer-rm3100-Modernize-locking-and-refactor-control-flow/20260502-014808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20260430021439.67591-1-m32285159%40gmail.com
patch subject: [PATCH v4] iio: magnetometer: rm3100: Modernize locking and refactor control flow
config: s390-randconfig-r071-20260503 (https://download.01.org/0day-ci/archive/20260503/202605030817.kUz9B6MC-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 15.2.0
smatch: v0.5.0-9065-ge9cc34fd
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260503/202605030817.kUz9B6MC-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/202605030817.kUz9B6MC-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/iio/magnetometer/rm3100-core.c:467 expecting prototype for rm3100_guarded_regmap_bulk_read(). Prototype was for rm3100_regmap_bulk_read_locked() instead

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki