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