Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

On Fri, Aug 28, 2026 at 12:46:24PM +0300, Matti Vaittinen wrote:
> On 28/08/2026 10:53, Andy Shevchenko wrote:
> > On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:


...

> > >   	wait_ms = bu27034_get_int_time(data);
> > > +
> > > +	/*
> > > +	 * If reading the integration time fails, default to the minimum so we
> > > +	 * don't lose samples. This may waste CPU cycles, but as a hardening
> > > +	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
> > > +	 */
> > > +	if (wait_ms < 0)
> > > +		wait_ms = BU27034_INT_TIME_US_MIN;
> > > +
> > >   	wait_ms /= 1000;
> > 
> > With the above being open coded the _ms feels not right.
> > I would expect the TIME_MIN to be in MS from the start
> > (and for the consistency's sake with the below) and having
> > all this to be written like
> > 
> > 	ret = bu27034_get_int_time(data);
> > 	if (ret < 0)
> > 		wait_ms = _MS_MIN;
> > 	else
> > 		wait_ms = ret / USEC_PER_MSEC;
> > 
> > >   	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
> 
> I don't like using 'ret' there.
> 
> At first glance, the
> ret = bu27034_get_int_time(data);
> 
> looks like ret is containing just the success status. Furthermore,
> > 		wait_ms = ret / USEC_PER_MSEC;
> 
> forces one to go back and see WTF the 'ret' is (even if just couple of lines
> - but this is not an improvement, using ret is obfuscation).
> 
> I could change this to:

I suggested without knowing the possible ranges of the returned value.

> wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC;
> if (wait_ms < BU27034_INT_TIME_MIN_MS)
>                 wait_ms = BU27034_INT_TIME_MIN_MS;

This looks sane to me and  removes the confusion I was talking about.

> (but for me this feels like unnecessary bikeshedding than anything else.)
> 
> Well, I'll change this if I respin.

-- 
With Best Regards,
Andy Shevchenko