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: Tue, 31 Oct 2023 09:11:37 +0200	[thread overview]
Message-ID: <d528b45c-123d-4ef7-b110-7efbfef91bc5@gmail.com> (raw)
In-Reply-To: <6a697c62-6a7c-4b31-bc8e-10f40db0363d@gmail.com>

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