On Sun, 18 Feb 2024 16:18:26 +1030
Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote:
Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.I applied this but then got some build warnings that made me look
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>
more closely at the int_src handling.
This is confusing because of the less than helpful datasheet defintion
of a 2 bit register that takes values 0 and 1 only.
I thought about trying to fix this up whilst applying but the event code
issue is too significant to do without a means to test it.
Jonathan
Hi Jonathan,+static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
+{
+ struct device *dev = data->dev;
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct apds9306_regfields *rf = &data->rf;
+ int ret, delay, intg_time, intg_time_idx, repeat_rate_idx, int_src;
+ int status = 0;
+ u8 buff[3];
+
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret)
+ return ret;
+
+ ret = regmap_field_read(rf->intg_time, &intg_time_idx);
+ if (ret)
+ return ret;
+
+ ret = regmap_field_read(rf->repeat_rate, &repeat_rate_idx);
+ if (ret)
+ return ret;
+
+ ret = regmap_field_read(rf->int_src, &int_src);
+ if (ret)
+ return ret;
+
+ intg_time = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
+ if (intg_time < 0)
+ return intg_time;
+
+ /* Whichever is greater - integration time period or sampling period. */
+ delay = max(intg_time, apds9306_repeat_rate_period[repeat_rate_idx]);
+
+ /*
+ * Clear stale data flag that might have been set by the interrupt
+ * handler if it got data available flag set in the status reg.
+ */
+ data->read_data_available = 0;
+
+ /*
+ * If this function runs parallel with the interrupt handler, either
+ * this reads and clears the status registers or the interrupt handler
+ * does. The interrupt handler sets a flag for read data available
+ * in our private structure which we read here.
+ */
+ ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG,
+ status, data->read_data_available ||
+ (status & (APDS9306_ALS_DATA_STAT_MASK |
+ APDS9306_ALS_INT_STAT_MASK)),
+ APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
+
+ if (ret)
+ return ret;
+
+ /* If we reach here before the interrupt handler we push an event */
+ if ((status & APDS9306_ALS_INT_STAT_MASK))
+ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+ int_src, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
You are pushing an event on channel 0 or 1 (which is non obvious as that
int_src is a 2 bit value again). However you don't use indexed channels
so this is wrong.
It's also pushing IIO_LIGHT for both channels which makes no sense as you
only have one IIO_LIGHT channel.
+ iio_get_time_ns(indio_dev));
+
+ ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
+ if (ret) {
+ dev_err_ratelimited(dev, "read data failed\n");
+ return ret;
+ }
+
+ *val = get_unaligned_le24(&buff);
+
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
+
+ return 0;
+}
+
...