lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9d810e5c-c7a5-41e5-8073-b703717faf3d@gmail.com>
Date: Mon, 2 Dec 2024 10:22:19 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>,
 Mikael Gonella-Bolduc <mgonellabolduc@...onoff.com>
Cc: Mikael Gonella-Bolduc via B4 Relay
 <devnull+mgonellabolduc.dimonoff.com@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling
 <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 Mikael Gonella-Bolduc <m.gonella.bolduc@...il.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
 Hugo Villeneuve <hvilleneuve@...onoff.com>
Subject: Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor
 driver

Hi Jonathan & Mikael,

On 01/12/2024 15:20, Jonathan Cameron wrote:
> 
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
>>>> +
>>>> +static struct i2c_driver apds9160_driver = {
>>>> +	.driver	  = {
>>>> +		.name	= APDS9160_DRIVER_NAME,
>>>> +		.owner = THIS_MODULE,
>>>> +		.of_match_table = apds9160_of_match,
>>>> +	},
>>>> +	.probe    = apds9160_probe,
>>>> +	.remove	  = apds9160_remove,
>>>> +	.id_table = apds9160_id,
>>>> +};
>> First, regarding the integration time/gain/scale parameters. I took a look at the datasheet again as there is a table
>> provided to get lux/count (scale?) for the ALS sensor depending on gain and integration time.
>>
>> It looks like the correlation in the table is almost linear but it's not as there is a loss of precision.
>> For example, at 1x gain with integration time 100ms the lux/count is 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.
>>
>> Is it still possible to use the gts helpers in that case?
> 
> Ah. Probably not if it goes non linear.  Matti? (+CC)

Disclaimer - I didn't go through the patch and I just respond from the 
top of my head :) So, please take my words with a pinch of salt.

AFAIR, it is not required that the impact of integration time is 
_linear_ through the range. The "multiplication factor" can be set for 
each integration time separately. So, it is perfectly Ok to say:

time 1 => multiply by 1
time 2 => multiply by 2
time 10 => multiply by 9 <= not linear, as linear would be 10.
time 15 => multiply by 15

...

The notable limitation of _current_ implementation is that the 
"multiplication factor" needs to be integer. So, this may result loss of 
accuracy.

>> Second, regarding the use of the IIO_CHAN_INFO_HARDWAREGAIN channel info.
>> I took a look at a couple of recent drivers, some use the IIO_CHAN_INFO_SCALE to ajust gain type registers.
>>
>> In my use case, it feels like the scale is read-only as it is affected by the gain and integration time and both can be set independently
>> with their respective available values. How should I handle this?
> The general preference is for the scale to be the primary control.
> For a light sensor assuming the device doesn't support very long integration times, the
> trade off is normally set the integration time as high as possible (as that gives lowest
> noise) then tune the gain as necessary.
> 
> Another model is to let the integration time be controllable and then try and adjust
> the gain to keep as close as possible to a requested scale.  Matti has spent more
> brain power on this than anyone so I'll over to him for more precise suggestions!

This is the very reason why I wrote the gts helpers. The recently 
removed rohm-bu27008.c light-sensor driver and the rohm-bu27034.c allow 
setting integration time. When time is set the driver will also set the 
gain, and does this so that the scale is maintained. (Eg, if gain caused 
by time is doubled by the user request, the hw-gain is halved by the 
driver).

For scale setting I used logic where the scale setting tries to maintain 
the integration time when possible, and only set the hardware gain. 
Ideology is that when the driver allows user to set the time, the user 
is expected to know whether to prefer longer time (and typically better 
accuracy), or faster measurement with more hw-gain.

There are still some corner cases in these drivers where the scale can't 
be maintained when time is set, or time can't be maintained when scale 
is set. AFAIR, These drivers chose to in any case set the requested 
time/scale - which means the userland needs to be prepared to "pick up 
the pieces" and deal with the other entity changing. Other option would 
be to deny such changes, but it would limit the use of the hardware - 
and I hate drivers trying to outsmart the users.

Where this gets really hairy is when there are multiple channels and 
some of the controls can be set per channel, and some are common for all 
channels. As an example, I've faced sensors where gain can be set 
separately for the channels but the time is common for all. Here we may 
end up in situations where part of the channels can compensate the 
changes while others can't. Furthermore, there were situations where 
some of the scales could only be achieved with a specific integration 
times. This can get very confusing for users.

In order to make it somehow tolerable the gts helpers support also 
advertising only scales which can be supported with currently selected 
integration time via the "available_scales". Also, I strongly recommend 
having at least a read-only hardwaregain to help understanding which 
part of the scale is contributed by time, and which by hardwaregain. I 
think this is very helpful when debugging ;)

I know Jonathan (and maybe rest of the world :) ) prefers keeping the 
scale as the single main way of changing the gain. Still, my personal 
opinion on this is that there are cases, where having both the 
hardwaregain and integration time directly changeable would be the 
simplest and clearest solution.

Putting my opinions aside (as this is not a battle I feel like fighting 
- nor do I feel I am the best expert on this!), with the current 'one 
scale to rule them all' approach, my recommendation is to consider if 
always using largest integration time and smallest hardwaregain to 
achieve the requested scale fits your users. (Also, I think your case 
where the impact of integration time is not strictly linear will cause 
some scales to be 'almost same' but not quite. I am not sure if it makes 
sense to support all of these, or just always round to the scale where 
the integration time is longest). If yes, then maybe do it.

If you think the longest times may cause unacceptable wait times for 
some users, or that the small scale differences really matter, then you 
may want to go the route bu27008 / bu27034 drivers took - but it may get 
hairy.

Finally, maybe taking a look if the integration time multiplier in gts 
helpers could support fixed point wouldn't be completely futile(?) No 
promises on that route though, I haven't considered this when writing 
those so it may lead to dead end - but one never knows before taking a 
look, right? :)

Ha. Looking all the letters I typed, I feel I helped really little while 
being very verbose ... Sorry bout that but I really don't have a great 
recommendation :(

Yours,
	-- Matti

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ