Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>,
	Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Paul Gazzillo <paul@pgazz.com>, Matt Ranostay <matt@ranostay.sg>,
	Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
Date: Mon, 30 Oct 2023 12:21:43 +0200	[thread overview]
Message-ID: <6a697c62-6a7c-4b31-bc8e-10f40db0363d@gmail.com> (raw)
In-Reply-To: <84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com>

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

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.

Still, the check:
 >>> +    if (val2 % 10)
 >>> +        val2 += 1;

seems a bit misleading because only the scales where the NANO accuracy 
is not enough need the + 1. I would at least ask for a comment 
explaining this a bit better :)

Another quick'n dirty way is to simply check if requested scale is one 
of the magic scales where the NANO accuracy is not enough:
41675167(.5)
125025502(.5)
20837583(.75)
13891722(.5)

and handle the rounding only for those (instead of the val2 % 10) - 
still with appropriate comment.

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 avoid this issue with BU27* drivers earlier because I was able to 
select the maximum scale so that the NANO accuracy was enough since I 
used these helpers for the intensity channels.

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