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

[Andy Shevchenko]

On Wed, May 06, 2026 at 12:50:09PM +0200, Joshua Crofts wrote:
> 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:

...

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

It looks like I stand corrected! Indeed, we have two conditions to follow, one
is provided in a macro parameter, and the other is for timeout. Here the Q is
which one logically should be checked first? More thinking on it tends to your
direction as it follows usual pattern as we check the return values (errors)
from the callee first *then* validate the results.

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

Yep.

-- 
With Best Regards,
Andy Shevchenko