On Sat, 29 Jun 2024 10:10:19 +0100
Mudit Sharma <muditsharma.info@xxxxxxxxx> wrote:
> 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)
This isn't really a trigger. Just report the event and don't register a trigger at
all.
In theory we could have a trigger with these properties (and we did discuss
many years ago how to do this in a generic fashion) but today it isn't
something any standard userspace will understand how to use.
>
> >
> > 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.
That bit is fine. My confusion is more about the trigger. I think
the short answer is drop the next line and indeed all the code registering
a trigger as this device doesn't provide appropriate hardware.
> >> +
> >> + iio_trigger_poll_nested(data->trig);
> >> +
> >> + return IRQ_HANDLED;
> >> + }
> >> +
> >> + return IRQ_NONE;
> >> +}
>
> Best regards,
> Mudit Sharma
>
