Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor

Matti Vaittinen
2023-04-26T10:19:44Z

On 4/26/23 13:12, Andi Shyti wrote:
> Hi Matti,
>
>> Thanks for the review! It's nice to see you're still keeping an eye on ROHM
>> / Kionix senor drivers ;)
>
> yeah... this is fun... if I just had a bit more time :)
>
>>>> +static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
>>>> + struct iio_chan_spec const *chan, int *val, int *val2)
>>>> +{
>>>> + int ret, int_time;
>>>> +
>>>> + ret = bu27008_chan_cfg(data, chan);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = bu27008_meas_set(data, BU27008_MEAS_EN);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + int_time = bu27008_get_int_time(data);
>>>> + if (int_time < 0)
>>>> + int_time = 400000;
>>>> +
>>>> + msleep((int_time + 500) / 1000);
>>>
>>> What is this 500 doing? Is it making a real difference? it's
>>> 0.5ms.
>>
>> Thanks for the question, having extra pairs of eyes helps spotting
>> brainfarts :)
>>
>> The 500 here is half of the value of the divider - idea was to do rounding
>> correctly upwards to prevent premature wake-up. Well, this is incorrect
>> because we should always round up the sleep time, not just 'mathematically
>> correctly' (Eg, not only upwards when value >= 0.5 but upwards always when
>> the division is not even).
>>
>> After this being said, integration times for this device are full milli
>> seconds so they can all be divided by 1000 uS.
>>
>> Nevertheless, it's good to note that the sensor is definitely not being
>> clocked by the same clock as CPU and I assume the timing for it will be
>> drifting quite a bit from the CPU clock. This means some sensors will for
>> sure complete the measurement later than this wake-up. In order to tackle
>> this we have the valid-bit polling in bu27008_chan_read_data(). So, at the
>> end of the day, this rounding correction is lkely to be just some
>> unnecessary noise.
>
> I understand the logic of the waiting, but msleep is not the
> right function as waiting with msleep is always very approximate,
> that's why it's recommended to use it for a large waiting period,
> where the error is smaller.
>
> If int_time is 1ms, waiting 1.5 or 2 or 1, is the same thing,
> most probably you will end up waiting more.

Well, the light sensors are slow. The shortest time with BU27008 is 55
mS, longest being 400 mS. I think msleep fits Ok for this purpose.

>>> What's the minimum int_time? Can we set a minimum, as well, just
>>> for the sake of the msleep?
>>
>> Can you please elaborate what you mean by this? The minimum integration time
>> for bu27008 is 55 mS and this is set in the time tables for the gts-helpers.
>> The bu27008_get_int_time() should never return valid time smaller than that.
>
> Witha minimum i mean a minimum value for the msleep to start
> working decently. E.g. what if int_time is lower than 1ms? Can we
> have msleep(0)?

As mentioned, int_time for bu27008 will be in a range of 55 ... 400 mS.

>
> [...]
>
>>>> +static int bu27008_chip_init(struct bu27008_data *data)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
>>>> + BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
>>>> + if (ret)
>>>> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
>>>> +
>>>> + /*
>>>> + * The data-sheet does not tell how long performing the IC reset takes.
>>>> + * However, the data-sheet says the minimum time it takes the IC to be
>>>> + * able to take inputs after power is applied, is 100 uS. I'd assume
>>>> + * > 1 mS is enough.
>>>> + */
>>>> + msleep(1);
>>>
>>> please use usleep_range().
>>
>> I prefer to not require setting up hrtimers as we have no real requirements
>> for the duration of this sleep. I know the msleep() is likely to exceed the
>> 1 mS, potentially a lot if there is things to do - but we don't really care
>> at this point. The main thing is to give the HW time to reset while allowing
>> other things to be scheduled.
>
> For the reason above, msleep(1) is quite a meaningless
> instruction. If you need to wait around 1ms, then usleep_range is
> the function to be used.
>
> Refer, also, to the Documentation/timers/timers-howto.rst

I know the msleep() may sleep much longer. It still is not meaningless.
Here we use the msleep() in a purpose:

"Sleep at least 1 mS, but actually I don't care if that is 20 mS or more
- as long as you allow scheduling other things with as little overhead
as possible".

For that purpose msleep() works just perfectly :)

I actually had a comment clarifying this in previous IIO driver I wrote
(just to avoid confusing reviewers) but Jonathan asked me to remove the
comment ;)

Yours,
-- Matti

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

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