Messages in this thread | | | Date | Mon, 6 Mar 2023 14:52:57 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers |
| |
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...
> + * 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.
> + } > + > + while (max > scale * (u64) tmp)
No space for castings?
> + 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?
...
> + *lin_scale = (u64) scale_whole * (u64)scaler +
No space for casting?
> + (u64)(scale_nano / (NANO / scaler));
...
> +EXPORT_SYMBOL_NS_GPL(iio_gts_total_gain_to_scale, IIO_GTS_HELPER);
I would say _HELPER part is too much, but fine with me.
...
> + 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?
...
> + /* > + * 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?
...
> + 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?
> + 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).
> + 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.
> + 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.
> +/** > + * 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? 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.
> + 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.
> + 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.
> + if (ret) > + return -EINVAL;
...
> +++ b/drivers/iio/light/iio-gts-helper.h
Is it _only_ for a Light type of sensors?
...
> +#ifndef __IIO_GTS_HELPER__ > +#define __IIO_GTS_HELPER__
If yes, perhaps adding LIGHT here?
-- With Best Regards, Andy Shevchenko
| |