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

[Matti Vaittinen]

On 28/08/2026 10:53, Andy Shevchenko wrote:
> On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:
>
> > When reading an integration-time fails, the code will use error code to
> > compute the sleep time.
> >
> > Fix this by using the smallest integration time as a default if
> > reading fails.
>
> ...
>
> >   	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:

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;

(but for me this feels like unnecessary bikeshedding than anything else.)

Well, I'll change this if I respin.

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~