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

[Jonathan Cameron]

On Fri, 28 Aug 2026 14:01:46 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

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

Except that wait_ms can be negative due to a read error and that is obscure
by what now looks like 
	wait_ms = max(wait_ms, BU27034_INT_TIME_MIN_MS);
so ensuring we clamp an out of range value.

I'd just go a touch further and split return value and parameter
+ provide a default.

wait_us = BU27034_INT_TIME_US;
ret = bu27034_get_int_time(data, &wait_us);
if (ret)
	dev_warn(dev, "Failed to read int time, muddling on\n");






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