Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux