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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 30/08/2026 04:16, Jonathan Cameron wrote:
> On Fri, 28 Aug 2026 14:01:46 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> 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.

Which, in this case, is perfectly fine because it does not matter what 
the cause of the failure is. There is no need to distinguish between 
read error and 'returned value is below the minimum time' - in both 
cases we just use the minimum time.

> 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");
I do both agree and disagree. This is not the first time when I am 
pondering if a "getter API" should only return an error, and fill the 
desired output in location pointed by a parameter (your suggestion here).

In many cases it is simpler in the calling site to just use the return 
value, and error handling can still be reliably done when all legitimate 
values are positive (in this case, also greater than minimum). Using the 
pointer argument for value retrieval typically brings potential issues 
with value initialization if call is unsuccessful.

Hence, I am often using this "dual meaning" for return value (negative 
error code or positive valid value) in cases, where all successful 
values are guaranteed to be positive.

I would definitely favour your suggested signature if retrieved value 
could be negative, or was a pointer. I have never liked the IS_ERR() and 
others - but check:
if "value < allowed minimum" and decision "then, something is wrong - 
use default" is really quite clear to me.

Yours,
	-- Matti

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

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