Re: [PATCH v8 5/5] iio: light: Add support for APDS9306 Light Sensor
From: Subhajit Ghosh
Date: Mon Mar 04 2024 - 07:48:55 EST
On 4/3/24 01:44, Jonathan Cameron wrote:
On Wed, 28 Feb 2024 22:54:08 +1030
Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote:
Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
channel approximates the response of the human-eye providing direct
read out where the output count is proportional to ambient light levels.
It is internally temperature compensated and rejects 50Hz and 60Hz flicker
caused by artificial light sources. 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.
This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
Scales, Gains and Integration time implementation.
Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
---
v7 -> v8:
- Renamed APDS9306_INT_CH_CLEAR to APDS9306_INT_SRC_CLEAR macro for higher
readability
- Removed APDS9306_CHANNEL macro for higher readability
- Updated iio_push_event() functions with correct type of events (Light or Intensity)
Partly right. Need to push the modified part for the intensity channel.
The event should match the channel description.
I also noted some missing elements in the event specs (sorry missed those
before!). Whilst what you have will work, that's just because the error checking
is relaxed in the IIO core and we don't complain if they aren't fully specified.
What you have creates the correct attributes, but that's a side effect of how
we use the data, not what data should be provided.
Thanks,
Jonathan
- Updated variable name "event_ch_is_light" to "int_src" and change as per
review to fix compiler warning
- Used scope for guard() functions
- Other fixes as per reviews
https://lore.kernel.org/all/20240224151340.3f2f51e8@jic23-huawei/
https://lore.kernel.org/all/ZdycR6nr3rtrnuth@xxxxxxxxxxxxxxxxxx/
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 2e5fdb33e0e9..a30f906e91ba 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020) += adux1020.o
...
+ GAIN_SCALE_ITIME_US(3125, APDS9306_MEAS_MODE_3125US, BIT(0)),
+};
+
+static struct iio_event_spec apds9306_event_spec_als[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+static struct iio_event_spec apds9306_event_spec_clear[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
Can't configure the threshold for this channel?
Same threshold regs for both als and clear channels.
Whilst the IIO core doesn't check these for missing entries in
shared attributes, you driver should replicate the parts that
are in mask_shared_by_all above. The code that builds the attributes
expects duplication of entries so they are here to provide an easy
place for us to visually check what is supported.
I understand this approach now.
I think that means this event spec will be identical to that for the
als channel. So reuse that.
Yes, correct.
I tried using struct iio_event_spec apds9306_event_spec_als[] for both light
and clear channels and the ABI is identical to the previous version.
Let us know if you copied this pattern from another driver as we
should fix any that have gotten through review doing this.
Initially I referenced many drivers but after so many iterations it
does not resemble anything that I have looked at previously.
Thank you for reviewing.
Regards,
Subhajit Ghosh
+ },
+};
+
+static struct iio_chan_spec apds9306_channels_with_events[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+ .event_spec = apds9306_event_spec_als,
+ .num_event_specs = ARRAY_SIZE(apds9306_event_spec_als),
+ }, {
+ .type = IIO_INTENSITY,
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .modified = 1,
+ .event_spec = apds9306_event_spec_clear,
+ .num_event_specs = ARRAY_SIZE(apds9306_event_spec_clear),
+ },
+};
+
+static struct iio_chan_spec apds9306_channels_without_events[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+ }, {
+ .type = IIO_INTENSITY,
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .modified = 1,
+ },
+};