>
> ...
>
> >
> >> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> >> +{
> >> + int ret, i_hi, i_lo, i;
> >> + struct iio_dev *idev = priv;
> >> + struct bd79124_data *d = iio_priv(idev);
> >> +
> >> + /*
> >> + * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> >> + * subsystem to disable the offending IRQ line if we get a hardware
> >> + * problem. This behaviour has saved my poor bottom a few times in the
> >> + * past as, instead of getting unusably unresponsive, the system has
> >> + * spilled out the magic words "...nobody cared".
> > *laughs*. Maybe the comment isn't strictly necessary but it cheered
> > up my Friday.
> >> + */
> >> + ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> >> + if (ret)
> >> + return IRQ_NONE;
> >> +
> >> + ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> >> + if (ret)
> >> + return IRQ_NONE;
> >> +
> >> + if (!i_lo && !i_hi)
> >> + return IRQ_NONE;
> >> +
> >> + for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> >> + u64 ecode;
> >> +
> >> + if (BIT(i) & i_hi) {
> > Maybe cleaner as a pair of
> >
> > for_each_set_bit() loops.
> >
>
> I kept the original for 2 reasons.
>
> 1. the main reason is that the for_each_set_bit() would want the value
> read from a register to be in long. Regmap wants to use int. Solving
> this produced (in my 'humblish' opinion) less readable code.
>
> 2. The current implementation has only one loop, which should perhaps be
> a tiny bit more efficient.
OK.
>
> >> + ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> >> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
> >> +
> >> + iio_push_event(idev, ecode, d->timestamp);
> >> + /*
> >> + * The BD79124 keeps the IRQ asserted for as long as
> >> + * the voltage exceeds the threshold. It may not serve
> >> + * the purpose to keep the IRQ firing and events
> >> + * generated in a loop because it may yield the
> >> + * userspace to have some problems when event handling
> >> + * there is slow.
> >> + *
> >> + * Thus, we disable the event for the channel. Userspace
> >> + * needs to re-enable the event.
> >
> > That's not pretty. So I'd prefer a timeout and autoreenable if we can.
>
> And I did this, but with constant 1 sec 'grace time' instead of
> modifiable time-out. I believe this suffices and keeps it simpler.
We might want to present that value to userspace anyway at somepoint, but
a fixed value is fine.
Jonathan
>
>
> Yours,
> -- Matti
