Re: [PATCH v5 07/18] iio: magnetometer: ak8975: modernize polling loops with iopoll() macros

[Joshua Crofts]

On Wed, 6 May 2026 at 09:01, Joshua Crofts <joshua.crofts1@xxxxxxxxx> wrote:
>
> On Wed, 6 May 2026 at 08:53, Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, May 05, 2026 at 01:46:03PM +0200, Joshua Crofts via B4 Relay wrote:
> >
> > > The driver currently uses while loops and msleep() for polling during
> > > conversion waits.
> > >
> > > Replace the custom polling loops with readx_poll_timeout() and
> > > read_poll_timeout() macros from <linux/iopoll.h>. This reduces
> > > boilerplate, standardizes timeout handling and improves overall code
> > > readability, keeping the original timing and error behaviour. Add
> > > <linux/time.h> for USEC_PER_MSEC macro instead of using magic numbers.
> >
> > ...
> >
> > > +     ret = read_poll_timeout(i2c_smbus_read_byte_data, val, val != 0,
> > > +                             poll_ms * USEC_PER_MSEC,
> > > +                             timeout_ms * USEC_PER_MSEC,
> > > +                             true,
> > > +                             client, data->def->ctrl_regs[ST1]);
> > > +     if (ret)
> > > +             return ret;
> > > +     if (val < 0) {
> > > +             dev_err(&client->dev, "Error in reading ST1\n");
> > > +             return val;
> > >       }
> > > -     if (!timeout_ms)
> > > -             return -ETIMEDOUT;
> > >
> > > -     return read_status;
> > > +     return val;
> >
> > Besides the unneeded return val duplication, I think this is the wrong location
> > for this check and it changes behaviour (really subtle change!).

Hmm, rechecking this, I agree with the unnecessary return, but I don't think it
changes the behaviour of the function.

> > Before if the last iteration gives an error from the device, we return
> > -ETIMEDOUT instead of the whatever the i2c_smbus_read_byte_data() returns.

Looking at the original function, it always prioritized returning the
i2c_smbus_read_byte_data() error code before returning -ETIMEDOUT (even
on the final iteration). In the new version, if the i2c read is bad,
the read_poll_timeout()
code will still be zero, therefore allowing the code to jump to the
i2c value check
and then return if bad (this is still the same behaviour IMO).

Maybe I'm looking at it from the wrong angle though. AFAIC Sashiko nitpicked
everything except this (but yes, it's still an AI so we have to tread lightly).

Same goes for the other patch where you raised this issue.

-- 
Kind regards

CJD