lkml.org 
[lkml]   [2023]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
    On Thu, Mar 02, 2023 at 12:57:54PM +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.

    ...

    > +/*

    Is it intentionally _not_ a kernel doc?

    > + * iio_gts_get_gain - Convert scale to total gain

    > + * Internal helper for converting scale to total gain.

    Otherwise this line should go after the fields, I remember kernel doc warnings
    on the similar cases.

    > + * @max: Maximum linearized scale. As an example, when scale is creted in

    creted?

    IIRC I already pointed out to the very same mistake in your code in the past
    (sorry, if my memory doesn't serve me well).

    > + * 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 scales. -EINVAL if scale

    scales? (Plural?)

    > + * is invalid.
    > + */

    Same remark to all of the comments.

    > +{
    > + 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)

    Space is not required after casting.

    > + tmp++;
    > +
    > + return tmp + 1;

    Wondering why you can't simplify this to

    max -= scale;
    tmp++;

    > + }
    > +
    > + while (max > scale * (u64) tmp)
    > + tmp++;
    > +
    > + return tmp;
    > +}

    Missing blank line.

    > +/*
    > + * gain_get_scale_fraction - get the gain or time based on scale and known one
    > + *
    > + * Internal helper for computing unknown fraction of total gain.
    > + * Compute either gain or time based on scale and either the gain or time
    > + * depending on which one is known.
    > + *
    > + * @max: Maximum linearized scale. As an example, when scale is creted in

    creted?

    Is it mistakenly stored in your spellcheck database? Or is it simply
    copy'n'paste typo?

    > + * magnitude of NANOs and max scale is 64.1 - The linearized
    > + * scale is 64 100 000 000.
    > + * @scale: Linearized scale to compute the gain/time for.
    > + * @known: Either integration time or gain depending on which one is known
    > + * @unknown: Pointer to variable where the computed gain/time is stored
    > + *
    > + * Return: 0 on success
    > + */

    ...

    > +static const struct iio_itime_sel_mul *
    > + iio_gts_find_itime_by_time(struct iio_gts *gts, int time)

    Strange indentation.

    Ditto for all these types of cases.

    ...

    > + *lin_scale = (u64) scale_whole * (u64)scaler + (u64)(scale_nano
    > + / (NANO / scaler));

    Strange indentation. Split on the logical (math) parts better.

    ...

    > +EXPORT_SYMBOL_GPL(iio_init_iio_gts);

    I haven't noticed if you put these all exports into a proper namespace.
    If no, please do.

    ...

    > + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
    > + NULL);

    One line is okay.

    ...

    > + all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int),

    Something from overflow.h is very suitable here.

    > + GFP_KERNEL);
    > + if (!all_gains)
    > + return -ENOMEM;

    ...

    > + memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int));

    Maybe it's better to have a temporary which will be calculated as array_size()
    for allocation and reused here?

    ...

    > + for (i = gts->num_itime - 2; i >= 0; i--)

    Yeah, if you put this into temporary, like

    i = gts->num_itime - 1;

    this becomes

    while (i--) {

    Note, you missed {} for better reading.

    Note, you may re-use that i (maybe renamed to something better in the memcpy()
    above as well).

    > + for (j = 0; j < gts->num_hwgain; j++) {
    > + int candidate = gains[i][j];
    > + int chk;
    > +
    > + if (candidate > all_gains[new_idx - 1]) {
    > + all_gains[new_idx] = candidate;
    > + new_idx++;
    > +
    > + continue;
    > + }
    > + for (chk = 0; chk < new_idx; chk++)
    > + if (candidate <= all_gains[chk])
    > + break;
    > +
    > + if (candidate == all_gains[chk])
    > + continue;
    > +
    > + memmove(&all_gains[chk + 1], &all_gains[chk],
    > + (new_idx - chk) * sizeof(int));
    > + all_gains[chk] = candidate;
    > + new_idx++;
    > + }

    ...

    > + gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
    > + 2 * sizeof(int), GFP_KERNEL);
    > + if (!gts->avail_all_scales_table)
    > + ret = -ENOMEM;
    > + else
    > + for (i = 0; !ret && i < gts->num_avail_all_scales; i++)

    Much easier to read if you move this...

    > + ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
    > + &gts->avail_all_scales_table[i * 2],
    > + &gts->avail_all_scales_table[i * 2 + 1]);

    ...here as

    if (ret)
    break;

    > + kfree(all_gains);
    > + if (ret && gts->avail_all_scales_table)
    > + kfree(gts->avail_all_scales_table);
    > +
    > + return ret;

    But Wouldn't be better to use goto labels?

    ...

    > + while (i) {

    Instead of doing standard

    while (i--) {

    > + /*
    > + * It does not matter if i'th alloc was not succesfull as
    > + * kfree(NULL) is safe.
    > + */

    You add this comment, ...

    > + kfree(per_time_gains[i]);
    > + kfree(per_time_scales[i]);

    ...an additional loop, ...

    > +
    > + i--;

    ...and a line of code.

    > + }

    Why?

    > + for (i = gts->num_itime - 1; i >= 0; i--) {

    i = gts->num_itime;

    while (i--) {

    > + int new = gts->itime_table[i].time_us;
    > +
    > + if (times[idx] < new) {
    > + times[idx++] = new;
    > + continue;
    > + }
    > +
    > + for (j = 0; j <= idx; j++) {
    > + if (times[j] > new) {
    > + memmove(&times[j + 1], &times[j], (idx - j) * sizeof(int));
    > + times[j] = new;
    > + idx++;
    > + }
    > + }
    > + }

    ...

    > +void iio_gts_purge_avail_time_table(struct iio_gts *gts)
    > +{
    > + if (gts->num_avail_time_tables) {

    if (!...)
    return;

    > + kfree(gts->avail_time_tables);
    > + gts->avail_time_tables = NULL;
    > + gts->num_avail_time_tables = 0;
    > + }
    > +}

    ...

    > + if (!diff) {

    Why not positive conditional?

    if (diff) {
    ...
    } else {
    ...
    }

    > + 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;
    > + }
    > + }

    ...

    > + ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);

    > +

    Redundant blank line.

    > + if (ret || !iio_gts_valid_gain(gts, *gain))

    Why error code is shadowed?

    > + return -EINVAL;
    > +

    ...

    > + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
    > + &scale);

    Single line if fine.

    > + if (ret)
    > + return ret;
    > +
    > + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
    > + new_gain);

    Ditto.

    > + if (ret)
    > + return -EINVAL;

    ...

    > +#ifndef __GAIN_TIME_SCALE_HELPER__
    > +#define __GAIN_TIME_SCALE_HELPER__

    __IIO_... ?

    Missing types.h (at least, haven't checked for more).

    Missing some forward declarations, at least for struct device.

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2023-03-27 00:42    [W:8.451 / U:0.780 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site