On 13/05/16 19:43, Crestez Dan Leonard wrote: > As far as I can tell DRDY for ST sensors behaves as a level rather than > edge interrupt. Registering for IRQF_TRIGGER_RISING instead of > IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is > high enough that new samples come before the new ones are read > completely. In that case the interrupt line remains high, no more rising > edges occur and the iio buffer stalls. > > Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as > expected. This patch makes it so that st_sensors_trigger interrupt > request code doesn't mangle the request flags into IRQF_TRIGGER_RISING. > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> > Cc: Giuseppe Barba <giuseppe.barba@xxxxxx> > Cc: Denis Ciocca <denis.ciocca@xxxxxx> > Signed-off-by: Crestez Dan Leonard <leonard.crestez@xxxxxxxxx> > --- > This is an alternative fix to this patch: > https://www.spinics.net/lists/linux-iio/msg24722.html > [PATCH 2/2 v6] iio: st_sensors: read surplus samples in trigger. > > It's not clear if all st_sensors behave this way on DRDY. If they do it might > be better to always request HIGH even if the interrupt is initially configured > for RISING. > > I tested on MinnowBoard Max which uses pinctrl-baytrail for gpio irqs. I > initially tested hacking the irq_trig to IRQF_TRIGGER_HIGH with an usb adaptor > (gpio-dln2) and it didn't work but I think that's because that driver doesn't > handle persistently high irqs properly. Hi Leonard / Linus, I thought my memory was failing me a little when I read Linus' patch description where he mentioned that we were missing pulsed interrupts. Years ago (probably at least 8) I ran into this exact issue with the lis3l02dq and spent sometime with a scope tracking down the cause (this was way before threaded interrupt etc were in mainline). Anyhow, my recollection was that we didn't always get a gap between the pulses. Hence it was possible to cross reading the registers with new data coming in for other channels and never see it drop. Distinctly remember scratching my head over how to deal with this for some time. The snag I had was that I was working with an intel stargate 2 with a pxa27x which didn't have any level interrupt support. Hence I ended up with a cludge that checked the gpio itself (was easier back then as you could get the gpio from the IRQ :) and did a bonus read with no processing to hammer it down and get out of being stuck. Linus' checking the data ready register and firing another trigger was the equivalent of this (back then we didn't have the two polling methods). You can still see my hack in staging/iio/accel/lis3l02dq_ring.c in try_reenable. Note the tryreenable was an attempt to bring sanity checking whether we need a repeat poll into one place and push the repeat polling into the core - which only works if all drivers hanging off the trigger can work fine with threaded interrupts only. Right now I think the try_reenable stuff has been broken for a long time... It will call iio_trigger_poll from a thread (typically). Obviously missed this one when we converted over to threaded interrupts (hmm) Need to audit if that can ever be called from interrupt context (to avoid making things worse!) If that was fixed then we could use it for Linus's reread but it suffers the same issue with no limit on the number of times it can go around (so that would want fixing as well!) This patch doesn't make things worse as I read it - except in the case where no interrupt type is specified... In that case it will try to make it high which may not be supported. Note that (if I'd merged the lis3l02dq driver in as I've been meaning too for a long time) this would just have broken my boards.. (can we check if the interrupt supports it and fall back to the original default if not? - it's even more comic on my board as IIRC the interrupt is shared via a discrete NAND gate with a tsl2561 so everything is flipped as well) The other question is how many users have one of these hooked up to an irq that doesn't support level interrupts (such as my pxa27x - which I fire up every few months - or your gpio-dln2 adapter). If so I guess we aren't making things any worse for them but the issue you had still exists + I still can't port my lis3l02dq driver over... I wonder if we ultimately end up having to support both fixes (though the one I did originally and Linus effectively reinvented is hideous :) If level interrupts are specified then yours is great and simple, if we edge interrupts are specified we have to play games to fake a level interrupt. Note that way back when I asked if anyone had any generic code for doing exactly this and got the answer 'It's too fiddly' IIRC - and I don't think anyone has ever taken it on since... I'm also inclined on this fix in general to send it the slow route (next merge window) rather than rushing it through. It's invasive and it's not as though we are looking at a regression here. The other fix is a totally different matter and I'll pick that up shortly. So in conclusion, in the first instance I'm keener on your solution which I think is the technically correct one. However, I fear we have enough crazy hardware types who have this wired up to an in appropriate interrupt controller, that we need not to make things worse than they are. We also have the potential to make them better perhaps by including the guts of Linus' suggestion as well (when not in level interrupt mode). Jonathan > > drivers/iio/common/st_sensors/st_sensors_trigger.c | 26 ++++++++++++---------- > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c > index 7c2e6ab..ffd6edb 100644 > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c > @@ -99,13 +99,16 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, > * If the IRQ is triggered on falling edge, we need to mark the > * interrupt as active low, if the hardware supports this. > */ > - if (irq_trig == IRQF_TRIGGER_FALLING) { > + if (irq_trig == IRQF_TRIGGER_FALLING || irq_trig == IRQF_TRIGGER_LOW) { > if (!sdata->sensor_settings->drdy_irq.addr_ihl) { > dev_err(&indio_dev->dev, > - "falling edge specified for IRQ but hardware " > - "only support rising edge, will request " > - "rising edge\n"); > - irq_trig = IRQF_TRIGGER_RISING; > + "active low specified for IRQ but hardware " > + "only support active high, will request " > + "active high\n"); > + if (irq_trig == IRQF_TRIGGER_FALLING) > + irq_trig = IRQF_TRIGGER_RISING; > + else if (irq_trig == IRQF_TRIGGER_LOW) > + irq_trig = IRQF_TRIGGER_HIGH; > } else { > /* Set up INT active low i.e. falling edge */ > err = st_sensors_write_data_with_mask(indio_dev, > @@ -114,18 +117,17 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, > if (err < 0) > goto iio_trigger_free; > dev_info(&indio_dev->dev, > - "interrupts on the falling edge\n"); > + "interrupts are active low\n"); > } > - } else if (irq_trig == IRQF_TRIGGER_RISING) { > + } else if (irq_trig == IRQF_TRIGGER_RISING || irq_trig == IRQF_TRIGGER_HIGH) { > dev_info(&indio_dev->dev, > - "interrupts on the rising edge\n"); > + "interrupts are active high\n"); > > } else { > dev_err(&indio_dev->dev, > - "unsupported IRQ trigger specified (%lx), only " > - "rising and falling edges supported, enforce " > - "rising edge\n", irq_trig); > - irq_trig = IRQF_TRIGGER_RISING; > + "unsupported IRQ trigger specified (%lx) " > + "enforce level high\n", irq_trig); > + irq_trig = IRQF_TRIGGER_HIGH; > } > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
