On 28/06/2024 20:37, Jonathan Cameron wrote:
Hi Mudit,
I'd failed on previous reviews to notice the odd trigger in here.
What is it, because it doesn't seem to be a dataready trigger as the device
doesn't seem to provide such an interrupt?
Hi Jonathan,
Thank you for your review on this.
I've incorrect called it as a dataready trigger, I missed this as part
of my initial cleanup - apologies for the confusion caused by this. I
should potentially call it 'threshold' or 'dev'. Please suggest what you
think would be appropriate here.
The sensor has an active low interrupt pin which is connected to a GPIO
(input, pullup). When the sensor reading crosses value set in threshold
high or threshold low resisters, interrupt signal is generated and the
interrupt gets handled in 'bh1745_interrupt_handler()' (interrupt also
depends on number of consecutive judgements set in BH1745_PERSISTENCE
register)
Various other comments inline.
Will address all for v7
...
+static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct bh1745_data *data = iio_priv(indio_dev);
+ int ret;
+ int value;
+
+ ret = regmap_read(data->regmap, BH1745_INTR, &value);
+ if (ret)
+ return IRQ_NONE;
+
+ if (value & BH1745_INTR_STATUS) {
+ guard(mutex)(&data->lock);
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ iio_get_time_ns(indio_dev));
What is happening here. You always push out the event and use that as
a trigger? This is an unusual trigger if it's appropriate to use it for
one at all. You've called it a dataready trigger but it is not obvious
that this device provides any such signal.
When an interrupt occurs, BH1745_INTR_STATUS bit is set in the
BH1745_INTR register. Event is only pushed out when the
BH1745_INTR_STATUS bit is set.
+
+ iio_trigger_poll_nested(data->trig);
+
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
Best regards,
Mudit Sharma
