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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 6 May 2026 16:08:21 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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@gmail.com> wrote:  
> > > On Wed, 6 May 2026 at 08:53, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> 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.
> 

I'll leave this one for now as the return val duplication can be tidied up.
I think the rest of the patch looks good tome.

Thanks,

Jonathan