Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux