On 25/2/24 01:43, Jonathan Cameron wrote:
On Sun, 18 Feb 2024 16:18:26 +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>
I applied this but then got some build warnings that made me look
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
Appreciate that you tried to fix all the issues for me.
...
...
+
+static irqreturn_t apds9306_irq_handler(int irq, void *priv)
+{
+ struct iio_dev *indio_dev = priv;
+ struct apds9306_data *data = iio_priv(indio_dev);
+ struct apds9306_regfields *rf = &data->rf;
+ int ret, status, int_ch;
+
+ /*
+ * The interrupt line is released and the interrupt flag is
+ * cleared as a result of reading the status register. All the
+ * status flags are cleared as a result of this read.
+ */
+ ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS_REG, &status);
+ if (ret < 0) {
+ dev_err_ratelimited(data->dev, "status reg read failed\n");
+ return IRQ_HANDLED;
+ }
+
+ ret = regmap_field_read(rf->int_src, &int_ch);
+ if (ret)
+ return ret;
+
+ if ((status & APDS9306_ALS_INT_STAT_MASK))
+ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+ int_ch, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
+ iio_get_time_ns(indio_dev));
As commented on elsewhere I'm not seeing the relationship between the event
pushed here and the channels this device provides (one of which is modified
for starters).
Yes, I will check which interrupt channel is enabled then push appropriate
event. Earlier versions wrongly had both channels as IIO_LIGHT which were fixed in
later revisions. I forgot to change the event part!
+
+ /*
+ * If a one-shot read through sysfs is underway at the same time
+ * as this interrupt handler is executing and a read data available
+ * flag was set, this flag is set to inform read_poll_timeout()
+ * to exit.
+ */
+ if ((status & APDS9306_ALS_DATA_STAT_MASK))
+ data->read_data_available = 1;
+
+ return IRQ_HANDLED;
+}
...
+static int apds9306_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+ struct apds9306_regfields *rf = &data->rf;
+ int int_en, event_ch_is_light, ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ guard(mutex)(&data->mutex);
+
+ ret = regmap_field_read(rf->int_src, &event_ch_is_light);
Call the local value int_src - it's not obvious to a reviewer what
relationship between that and int_src is. I had to go read the datasheet
to find out.
This unique name was suggested in a previous review:
https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/
I will change it next version. int_src is logical.
+ if (ret)
+ return ret;
+
+ ret = regmap_field_read(rf->int_en, &int_en);
+ if (ret)
+ return ret;
+
+ if (chan->type == IIO_LIGHT)
+ return int_en & event_ch_is_light;
+
+ if (chan->type == IIO_INTENSITY)
+ return int_en & !event_ch_is_light;
This is the specific line the compiler doesn't like
drivers/iio/light/apds9306.c:1036:39: warning: dubious: x & !y
I am using gcc 12.2.0 for cross compiling. I definitely do not want to send
patches with warnings in them. Can you please let me know the gcc version
or flags using which you got the above warning? Should I always use the
latest released version?
I would match int_src against specific values rather than using tricks
based on what those values happen to be.
return int_en && (int_src == APDS9306_INT_SRC_CLEAR);
I will implement this.
Thank you for taking time to review the code in detail and also appreciate
your suggestions.
Regards,
Subhajit Ghosh
