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

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! ~~