lkml.org 
[lkml]   [2023]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
    From
    On 10/30/23 12:21, Matti Vaittinen wrote:
    > Hi dee Ho peeps,
    >
    > On 10/29/23 17:51, Matti Vaittinen wrote:
    >> On 10/28/23 18:20, Jonathan Cameron wrote:
    >>> On Fri, 27 Oct 2023 18:15:45 +1030
    >>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
    >>>
    >>>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor
    >>>> with als
    >>>> and clear channels with i2c interface. Hardware interrupt
    >>>> configuration is
    >>>> optional. It is a low power device with 20 bit resolution and has
    >>>> configurable adaptive interrupt mode and interrupt persistence mode.
    >>>> The device also features inbuilt hardware gain, multiple integration
    >>>> time
    >>>> selection options and sampling frequency selection options.

    ...

    >>>> +static int apds9306_scale_set(struct apds9306_data *data, int val,
    >>>> int val2)
    >>>> +{
    >>>> +    int i, ret, time_sel, gain_sel;
    >>>> +
    >>>> +    /* Rounding up the last digit by one, otherwise matching table
    >>>> fails! */
    >>>
    >>> Interesting.  Sounds like a question for Matti?
    >>
    >> Sounds odd indeed. I assume this happens when scale setting is
    >> requested using one of the exact values advertised by the available
    >> scales from the GTS? This does not feel right and the +1 does not ring
    >> a bell to me. I need to investigate what's going on. It would help if
    >> you could provide the values used as val and val2 for the setting.
    >>
    >> This will take a while from me though - I'll try to get to this next
    >> week. Thanks for pointing out the anomaly!
    >>
    >
    > I think I have a rough understanding. I did a Kunit test which goes
    > through all the available scales values from the
    > gts->avail_all_scales_table and all integration times, and feeds them to
    > the logic below. It seems the first culprit is hit by:
    > val = 0, val2 = 125025502.
    >
    > Problem is that the 125025502 is rounded. The exact linearized NANO
    > scale resulting from time multiplier 128, gain multiplier 1 is
    > 125025502.5 - which means we will see rounding.
    >
    >>>
    >>>> +    if (val2 % 10)
    >>>> +        val2 += 1;
    >
    > For a while I was unsure if this check works for all cases because I see
    > linearized scales:
    > 250051005 - multipliers 1x, 64x
    > 83350335 - multipliers 3x, 64x and 6x, 32x
    > 27783445 - multipliers 9x, 64x.
    >
    > For those we will get + 1 added to val2 even though there is no
    > rounding. It appears this is not a problem because the
    > iio_gts_get_gain() (which is used to figure out the required total gain
    > to get the desired scale) does not require the scale to be formed by
    > exact multiples of gain.

    ...

    > I think it would be very nice if the gts-helpers could do the rounding
    > when computing the available scales, but that'd require some thinking.
    > Fixup patch is still very welcome ;)

    I did some further experimenting. Basically, I did a "hack" which always
    rounds up the available-scales values if division results a remainder.
    This way the values advertised by the available_scales did find the
    matching table.

    It is a tiny bit icky because for example the scale 6945861.25 becomes
    6945862 in available-scales. Also, I assume that if we "hack" just the
    available-scales and don't fix the rest of the logic, setting 6945862
    will read back as 6945861 (I haven't tested this though). Also, the
    20837583.75 will be 20837583 in available-scales but 20837582 when read
    back, resulting small error. (I haven't tested this either but I assume
    the current GTS code is flooring the 20837583.75 to 20837583.

    I am wondering if changing the iio_gts_get_gain() to do rounding instead
    of flooring and changing also the iio_gts_total_gain_to_scale() to
    something like:

    int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
    int *scale_int, int *scale_nano)
    {
    u64 tmp;
    int rem;

    tmp = gts->max_scale;

    rem = do_div(tmp, total_gain);
    if (total_gain > 1 && rem >= total_gain / 2)
    tmp += 1ULL;

    return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
    }

    would do the trick. It's just that I'm a bit afraid of touching the
    iio_gts_get_gain() - by the very least I need to fire up the GTS tests
    which I implemented but are not in-tree due to the test-device
    dependency... :/

    Any thoughts?

    >>>> +
    >>>> +    ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
    >>>> +                     data->intg_time_idx, val, val2, &gain_sel);
    >>>> +    if (ret) {
    >>>> +        for (i = 0; i < data->gts.num_itime; i++) {
    >>>> +            time_sel = data->gts.itime_table[i].sel;
    >>>> +
    >>>> +            if (time_sel == data->intg_time_idx)
    >>>> +                continue;
    >>>> +
    >>>> +            ret =
    >>>> iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
    >>>> +                        time_sel, val, val2, &gain_sel);
    >>>> +            if (!ret)
    >>>> +                break;
    >>>> +        }
    >>>> +        if (ret)
    >>>> +            return -EINVAL;
    >>>> +
    >>>> +        ret = apds9306_intg_time_set_hw(data, time_sel);
    >>>> +        if (ret)
    >>>> +            return ret;
    >>>> +    }
    >>>> +
    >>>> +    return apds9306_gain_set_hw(data, gain_sel);
    >>>> +}

    Yours,
    -- Matti

    --
    Matti Vaittinen
    Linux kernel developer at ROHM Semiconductors
    Oulu Finland

    ~~ When things go utterly wrong vim users can always type :help! ~~

    \
     
     \ /
      Last update: 2023-10-31 08:12    [W:2.536 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site