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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Apr 2026 17:26:00 -0500
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.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
Hi Maxwell.

The use of the helper worked out well but it also made me look
at some existing code and wonder why it is so complex!

>  static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -468,11 +481,10 @@ static irqreturn_t rm3100_trigger_handler(int irq, void *p)
>  	struct regmap *regmap = data->regmap;
>  	int ret, i, bit;
>  
> -	mutex_lock(&data->lock);
>  	switch (scan_mask) {

>  	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_guarded_regmap_bulk_read(&data->lock, regmap,
> +							      RM3100_REG_MX2 + 3 * bit,
> +							      data->buffer, 3);

Hmm. Not one for this patch, but the only reason this doesn't
hammer the lock is the bitmap is one hot.  Which is why the read is always to the same
location in the buffer. That could have been a lot easier to spot if instead
of a loop find_first_bit() was used.

I don't think we can ever get here with no bits set but maybe check that as well
(it's not supposed to be possible as doesn't make any sense to measure nothing ;)
 




> +			if (ret < 0)
>  				goto done;
> -			}
>  		}
> -		mutex_unlock(&data->lock);
>  	}
>  	/*
>  	 * Always using the same buffer so that we wouldn't need to set the