Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error
From: Matti Vaittinen
Date: Sun Aug 30 2026 - 05:32:51 EST
On 30/08/2026 04:16, Jonathan Cameron wrote:
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.
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 parameterI 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).
+ 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");
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! ~~