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

[Maxwell Doose]

Hi Jonathan,

On Wed, Apr 29, 2026 at 5:42 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> 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!
>

Glad the helper worked out, I'm also on a similar page with you about
the code complexity. I feel that if there are similar patterns in the
code, we could probably condens those down into either inlined
functions or macros (we want to avoid macros for obvious reasons but I
digress). If I find other patterns that do this kind of:
lock(&my_mutex);
ret = my_function(params);
unlock(&my_mutex);
then it may be a good idea to consider writing a helper-wrapper in a
header that takes in a function pointer and its params. It would take
more thought, and we'd probably have to avoid variadic arguments, but
it may be a good idea just so we can keep the same scope and keep the
logic clean. Just thinking ahead here.

> >  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.
>

Thanks for pointing that out, I didn't realize that. I don't have much
experience with the bitops.h macros, but I think that would be sound
for a separate patch. I'd ordinarily include that in this patch, but I
feel at that point we might be deviating from the original purpose of
the patch.

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

I'll consider it for the v4, but I might send it as a separate patch
as well since again, at that point we might be deviating from the
original purpose.

best regards,
maxwell