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
