lkml.org 
[lkml]   [2023]   [Oct]   [29]   [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/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.
    >
    > Hi Subhajit,
    >
    >
    >>
    > Change log below the ---
    >
    > We don't generally want to end up with this information in the git log
    > and anything above the --- is used for the commit message.
    >
    > Note that if you want to keep notes in your local git it is fine adding
    >
    > Signed-of-by...
    >
    > ---
    >
    > Version notes
    > etc
    >
    >
    > As then git am will drop them anyway when your patches are picked up.
    >
    >
    >> v1 -> v2
    >> - Renamed probe_new to probe
    >> - Removed module id table
    >>
    >> v0 -> v1
    >> - Fixed errors as per previous review
    >> - Longer commit messages and descriptions
    >> - Updated scale calculations as per iio gts scheme to export proper scale
    >> values and tables to userspace
    >> - Removed processed attribute for the same channel for which raw is
    >> provided, instead, exporting proper scale and scale table to userspace so
    >> that userspace can do "(raw + offset) * scale" and derive Lux values
    >> - Fixed IIO attribute range syntax
    >> - Keeping the regmap lock enabled as the driver uses unlocked regfield
    >> accesses from interrupt handler
    >> - Several levels of cleanups by placing guard mutexes in proper places and
    >> returning immediately in case of an error
    >> - Using iio_device_claim_direct_mode() during raw reads so that
    >> configurations could not be changed during an adc conversion period
    >> - In case of a powerdown error, returning immediately
    >> - Removing the definition of direction of the hardware interrupt and
    >> leaving it on to device tree
    >> - Adding the powerdown callback after doing device initialization
    >> - Removed the regcache_cache_only() implementation
    >>
    >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
    >

    ...

    >
    >> +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!

    >
    >> + if (val2 % 10)
    >> + val2 += 1;
    >> +
    >> + 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);
    >> +}

    --
    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-29 16:53    [W:3.253 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site