Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers
From: Matti Vaittinen
Date: Mon Mar 13 2023 - 08:47:54 EST
On 3/6/23 14:52, Andy Shevchenko wrote:
On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
Some light sensors can adjust both the HW-gain and integration time.
There are cases where adjusting the integration time has similar impact
to the scale of the reported values as gain setting has.
IIO users do typically expect to handle scale by a single writable 'scale'
entry. Driver should then adjust the gain/time accordingly.
It however is difficult for a driver to know whether it should change
gain or integration time to meet the requested scale. Usually it is
preferred to have longer integration time which usually improves
accuracy, but there may be use-cases where long measurement times can be
an issue. Thus it can be preferable to allow also changing the
integration time - but mitigate the scale impact by also changing the gain
underneath. Eg, if integration time change doubles the measured values,
the driver can reduce the HW-gain to half.
The theory of the computations of gain-time-scale is simple. However,
some people (undersigned) got that implemented wrong for more than once.
Add some gain-time-scale helpers in order to not dublicate errors in all
drivers needing these computations.
...
+/*
If it's deliberately not a kernel doc, why to bother to have it looking as one?
It's really a provocative to some people who will come with a patches to "fix"
this...
I just liked the kernel-doc format. It's a standard way of explaining
the parameters and returned value. Function however is intended to be
internal and thus I don't see a need to make this "official kernel doc".
+ * iio_gts_get_gain - Convert scale to total gain
+ *
+ * Internal helper for converting scale to total gain.
+ *
+ * @max: Maximum linearized scale. As an example, when scale is created
+ * in magnitude of NANOs and max scale is 64.1 - The linearized
+ * scale is 64 100 000 000.
+ * @scale: Linearized scale to compte the gain for.
+ *
+ * Return: (floored) gain corresponding to the scale. -EINVAL if scale
+ * is invalid.
+ */
+static int iio_gts_get_gain(const u64 max, const u64 scale)
+{
+ int tmp = 1;
+
+ if (scale > max || !scale)
+ return -EINVAL;
+
+ if (U64_MAX - max < scale) {
+ /* Risk of overflow */
+ if (max - scale < scale)
+ return 1;
+ while (max - scale > scale * (u64)tmp)
+ tmp++;
+
+ return tmp + 1;
Can you wait for the comments to appear a bit longer, please?
I have answered to your query in the previous discussion.
Yep. Sorry for that. I just wanted to get the v3 out before I left the
computer for a week to allow potential reviewers to check the quite a
bit reworked series.
+ }
+
+ while (max > scale * (u64) tmp)
No space for castings?
Thanks,
+ tmp++;
+
+ return tmp;
+}
...
+ /*
+ * Expect scale to be (mostly) NANO or MICRO. Divide divider instead of
+ * multiplication followed by division to avoid overflow
Missing period.
+ */
+ if (scaler > NANO || !scaler)
+ return -EINVAL;
Shouldn't be OVERFLOW for the first one?
-EOVERFLOW? I guess it could be. Thanks.
...
+ *lin_scale = (u64) scale_whole * (u64)scaler +
No space for casting?
Yes. Thanks
...
+ ret = iio_gts_linearize(max_scale_int, max_scale_nano, NANO,
+ >s->max_scale);
+ if (ret)
+ return ret;
+
+ gts->hwgain_table = gain_tbl;
+ gts->num_hwgain = num_gain;
+ gts->itime_table = tim_tbl;
+ gts->num_itime = num_times;
+ gts->per_time_avail_scale_tables = NULL;
+ gts->avail_time_tables = NULL;
+ gts->avail_all_scales_table = NULL;
+ gts->num_avail_all_scales = 0;
Just wondering why we can't simply
memset(0)
beforehand and drop all these 0 assignments?
I guess we can :)
...
+ /*
+ * Sort the tables for nice output and for easier finding of
+ * unique values
Missing period. Please, check the style of multi-line comments. I believe it's
even mentioned in the documentation.
+ */
...
+ sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
+ NULL);
One line reads better?
I try mostly to keep the good old 80 chars as I often have 3 terminal
windows fitted on my laptop screen. It works best with the short lines.
...
+ if (ret && gts->avail_all_scales_table)
In one case you commented that free(NULL) is okay, in the other, you add
a duplicative check. Why?
Sorry but what do you mean by dublicative check?
Usually I avoid the kfree(NULL). That's why I commented on it in that
another case where it was not explicitly disallowed. I'll change that
for v4 to avoid kfree(NULL) as you suggested.
+ kfree(gts->avail_all_scales_table);
...
+ per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
sizeof(type) is error prone in comparison to sizeof(*var).
Yes and no. In majority of cases where we see sizeof(*var) - the *var is
no longer a pointer as having pointers to pointers is not _that_ common.
When we see sizeof(type *) - we instantly know it is a size of a pointer
and not a size of some other type.
So yes, while having sizeof(*var) makes us tolerant to errors caused by
variable type changes - it makes us prone to human reader errors. Also,
if someone changes type of *var from pointer to some other type - then
he/she is likely to in any case need to revise the array alloactions too.
While I in general agree with you that the sizeof(variable) is better
than sizeof(type) - I see that in cases like this the sizeof(type *) is
clearer.
+ if (!per_time_gains)
+ return ret;
+
+ per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
Ditto.
+ if (!per_time_scales)
+ goto free_gains;
...
+err_free_out:
+ while (i) {
+ /*
+ * It does not matter if i'th alloc was not succesfull as
+ * kfree(NULL) is safe.
+ */
Instead, can be just a free of the known allocated i:th member first followed
by traditional pattern. In that case comment will become redundant.
I replied to this in your comments regarding the v2. Sorry for splitting
the discussion by sending v3 so soon.
+ kfree(per_time_scales[i]);
+ kfree(per_time_gains[i]);
+
+ i--;
+ }
...
+ for (i = gts->num_itime - 1; i >= 0; i--) {
while (i--) {
makes it easier to parse.
This is also something I replied for v2. I think we have a fundamental
disagreement on this one :/
+/**
+ * iio_gts_all_avail_scales - helper for listing all available scales
+ * @gts: Gain time scale descriptor
+ * @vals: Returned array of supported scales
+ * @type: Type of returned scale values
+ * @length: Amount of returned values in array
+ *
+ * Returns a value suitable to be returned from read_avail or a negative error
Missing a return section. Have you run kernel doc to validate this?
No. I think I have never run the kernel doc - probably time to do so :)
Thanks.
Missing period.
Seems these problems occur in many function descriptions.
+ */
...
+ /*
+ * Using this function prior building the tables is a driver-error
+ * which should be fixed when the driver is tested for a first time
Missing period.
+ */
+ if (WARN_ON(!gts->num_avail_all_scales))
Does this justify panic? Note, that any WARN() can become an Oops followed by
panic and reboot.
No. My initial thinking was that this could only happen if a driver
which do not build the tables tries to use the helpers. That, in my
books, would have been 100% reproducible driver error, happening when
ever the available scales were read.
I did overlook the case where freeing of tables is done in wrong order.
That type of bug could well escape the initial testing and no - it
should not bring down the machine. I'll drop the WARN. Thanks.
+ return -EINVAL;
...
+ for (i = 0; i < gts->num_hwgain; i++) {
+ /*
+ * It is not expected this function is called for an exactly
+ * matching gain.
+ */
+ if (unlikely(gain == gts->hwgain_table[i].gain)) {
+ *in_range = true;
+ return gain;
+ }
+ if (!min)
+ min = gts->hwgain_table[i].gain;
+ else
+ min = min(min, gts->hwgain_table[i].gain);
I was staring at this and have got no clue why it's not a dead code.
Nor can I. It seems obvious to me that the one who wrote this had no
idea what he was doing XD
Well, I must have had some initial idea of using the minimum value to
something - but I can't remember what would've been the use. Maybe I was
initially thinking that I'll return the smallest value in-range if the
gain given as a parameter was smaller than any of the supported ones.
Thank you for reading this carefully and pointing it out! Well spotted!
+ if (gain > gts->hwgain_table[i].gain) {
+ if (!diff) {
+ diff = gain - gts->hwgain_table[i].gain;
+ best = i;
+ } else {
+ int tmp = gain - gts->hwgain_table[i].gain;
+
+ if (tmp < diff) {
+ diff = tmp;
+ best = i;
+ }
+ }
int tmp = gain - gts->hwgain_table[i].gain;
if (!diff || tmp < diff) {
diff = tmp;
best = i;
}
?
Or did you miss using 'min'? (And I'm wondering how variable name and min()
macro are not conflicting with each other.
+ } else {
+ /*
+ * We found valid hwgain which is greater than
+ * reference. So, unless we return a failure below we
+ * will have found an in-range gain
+ */
+ *in_range = true;
+ }
+ }
+ /* The requested gain was smaller than anything we support */
+ if (!diff) {
+ *in_range = false;
+
+ return -EINVAL;
+ }
+
+ return gts->hwgain_table[best].gain;
...
+ ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
+ &scale);
Still can be one line.
+ if (ret)
+ return ret;
+
+ ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
+ new_gain);
Ditto.
I still prefer the 80-chars when it does not mean some terribly awkward
split - or really many rows of arguments.
+ if (ret)
+ return -EINVAL;
...
+++ b/drivers/iio/light/iio-gts-helper.h
Is it _only_ for a Light type of sensors?
This is a very good question. I was asking this from myself but as I
don't know better I just decided to put it under the light where I'll
have the use-cases. We can move it to drivers/iio if there will be users
outside the light sensors.
...
+#ifndef __IIO_GTS_HELPER__
+#define __IIO_GTS_HELPER__
If yes, perhaps adding LIGHT here?
I'd like to keep this matching the file name, especially when I don't
know for sure this can't be used elsewhere.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~