Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
From: Matti Vaittinen
Date: Mon Oct 30 2023 - 06:24:20 EST
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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
...
+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! ~~