On Tue, 30 Jul 2024 14:29:24 +0000 Arnout Diels <Arnout.Diels@xxxxxxxxxx> wrote: > Hello, > > I have a remark regarding ad_sigma_delta's IRQ implementation, especially in conjunction with gpio chips that do no support disabling IRQ, but do support masking them on a HW level. > > > Specifically I was trying to bring up a ad7192 chip on an IMX8 platform. The IMX8 gpio driver (gpio-msc) does not implement interrupt disabling. > I was the same issue as others have seen, namelijk that when doing single ADC measurments, the values are stale, because the driver does not wait correctly for the IRQ to come through. > The fact that this chip muxes an IRQ line with an SPI MISO line is the fundamental cause of all trouble. > > For one, this requires hardware designers to typically route the pin to two MCU peripherals (an SPI pin and a GPIO pin - to avoid having to support both gpio interrupts and doing SPI transactions on the same pin). But ok, that can be done > But the second issue, is that on this pin, per definition, not just IRQ edges will be visible, but also MISO data. The driver has to correctly "time" the enablement of the actual IRQ. > > The ad_sigma_delta driver .. does this, but it can only do so much, if the underlying subsystem is limited. > > Practically, in case of the IMX8, the *hardware* supports falling-edge detection, which is needed and I configured correctly. If this is detetected, this then always calls <handle_level_irq>. > The *AD driver* tries to disable and enable IRQ at the right moments using <disable_irq_nosync> and <enable_irq>. > > But, when reading some single values from sysfs for example, triggering <ad_sigma_delta_single_conversion>, the following scenario plays out: > > - The first time everything goes well: > > SPI command to start sampling > > IRQ enabled (HW unmasked to look for falling edges), to wait for conversion finished result muxed on nRDY/MISO pin > > Falling edge occurs and irq is done, breaking a waiting loop > > During the callback, the interrupt is disabled again. > > > SPi command is sent to read out the data > > > But, during the readout command, things go wrong. > Since the hardware -cannot- be disabled, it WILL see another falling edge at this point. In fact mulitple during that MISO readout. > What happens then, depends on other settings. Assuming lazy interrupt disabling is used, this will actually first trigger another interrupt, which then masks future interrupts on a HW level. (Or, this masking is already done sooner). > However, crucially, MASKING interrupts on a HW level QUEUES them. The subsequent falling edges on the masked hardware will set a bit in the registers, and cause the issue the NEXT readout. > > What then happens on the next readout, is that, when the IRQ is again enabled, it fires IMMEDIATELY (since the previous falling edge was still 'queued' in hardware). > > (This then leads to the waiting loop to break too soon, and a stale value to be returned) > > > > ---- > > The proper way to deal with this, would be to (optionally) -clear- any HW-saved value when enabling the IRQ. (Or wait a while to "flush" it out as a less elegant solution) > > What I find strange is that that seemingly the same issue is supposedly reported to be fixed last year (https://github.com/torvalds/linux/commit/626d312028bec44209d0ecd5beaa9b1aa8945f7d). > However, the "fix" here was to disable lazy IRQ. This does not fix anything when using the IMX8 gpiochip, since this will just mask the HW upfront, rather than waiting another IRQ cycle first. > > > > I'd love to hear your thoughts on this. Thanks for the detailed description. This is a horrible corner case and honestly I'm not sure there will be interest in a comprehensive situation to clear the pending interrupts etc. One option might be to make the interrupt optional. If it's not present then don't allow the device clocked data capture modes (continuous mode) and just rely on sleeping to ensure data capture has occurred after a capture is triggered. Not great performance wise but it should 'work'. Given tight coupling of this driver and the ad_sigma_delta shared code I'm not sure how easy this would be to do. Maybe an early option would be to just stop enabling buffered capture. Then we'd remain in single mode and maybe all that is needed is a time to sleep value from the device specific driver. Jonathan > > Kind regards, > Arnout > >
