| Message ID | 20241122113322.242875-16-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad7124: Various fixes | expand |
On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > Some of the ADCs by Analog signal their irq condition on the MISO line. > So typically that line is connected to an SPI controller and a GPIO. The > GPIO is used as input and the respective interrupt is enabled when the > last SPI transfer is completed. > > Depending on the GPIO controller the toggling MISO line might make the > interrupt pending even while it's masked. In that case the irq handler > is called immediately after irq_enable() and so before the device > actually pulls that line low which results in non-sense values being > reported to the upper layers. > > The only way to find out if the line was actually pulled low is to read > the GPIO. (There is a flag in AD7124's status register that also signals > if an interrupt was asserted, but reading that register toggles the MISO > line and so might trigger another spurious interrupt.) > > Add the possibility to specify an interrupt GPIO in the machine > description in addition to the plain interrupt. This GPIO is used then > to check if the irq line is actually active in the irq handler. ... > + if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) { > + complete(&sigma_delta->completion); > + disable_irq_nosync(irq); > + sigma_delta->irq_dis = true; > + iio_trigger_poll(sigma_delta->trig); > + > + return IRQ_HANDLED; > + } else { Redundant 'else'. > + return IRQ_NONE; > + } Can we actually invert the conditional? > } ... > + if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line) Do you need the first check? (I haven't remember what gpiod_to_irq() will return on NULL, though) > + sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod); ... > --- a/include/linux/iio/adc/ad_sigma_delta.h > +++ b/include/linux/iio/adc/ad_sigma_delta.h > @@ -96,6 +96,7 @@ struct ad_sigma_delta { > unsigned int active_slots; > unsigned int current_slot; > unsigned int num_slots; > + struct gpio_desc *rdy_gpiod; Do you need a type forward declaration? > int irq_line; > bool status_appended;
On Fri, Nov 22, 2024 at 09:16:24PM +0200, Andy Shevchenko wrote: > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > > > Some of the ADCs by Analog signal their irq condition on the MISO line. > > So typically that line is connected to an SPI controller and a GPIO. The > > GPIO is used as input and the respective interrupt is enabled when the > > last SPI transfer is completed. > > > > Depending on the GPIO controller the toggling MISO line might make the > > interrupt pending even while it's masked. In that case the irq handler > > is called immediately after irq_enable() and so before the device > > actually pulls that line low which results in non-sense values being > > reported to the upper layers. > > > > The only way to find out if the line was actually pulled low is to read > > the GPIO. (There is a flag in AD7124's status register that also signals > > if an interrupt was asserted, but reading that register toggles the MISO > > line and so might trigger another spurious interrupt.) > > > > Add the possibility to specify an interrupt GPIO in the machine > > description in addition to the plain interrupt. This GPIO is used then > > to check if the irq line is actually active in the irq handler. > > ... > > > + if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) { > > + complete(&sigma_delta->completion); > > + disable_irq_nosync(irq); > > + sigma_delta->irq_dis = true; > > + iio_trigger_poll(sigma_delta->trig); > > + > > + return IRQ_HANDLED; > > > + } else { > > Redundant 'else'. > > > + return IRQ_NONE; > > + } > > Can we actually invert the conditional? I thought about that and I prefer it that way because like this is better matches the code comment before the if. I dropped the 'else' though for the next submission. > > } > > ... > > > + if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line) > > Do you need the first check? (I haven't remember what gpiod_to_irq() > will return on NULL, though) > > > + sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod); gpiod_to_irq() returns -EINVAL then. I added an error path for that condition. > > --- a/include/linux/iio/adc/ad_sigma_delta.h > > +++ b/include/linux/iio/adc/ad_sigma_delta.h > > @@ -96,6 +96,7 @@ struct ad_sigma_delta { > > unsigned int active_slots; > > unsigned int current_slot; > > unsigned int num_slots; > > + struct gpio_desc *rdy_gpiod; > > Do you need a type forward declaration? That would indeed be more robust. It compiles fine currently because all consumers of linux/iio/adc/ad_sigma_delta.h also include linux/spi/spi.h before which in turn includes linux/gpio/consumer.h and so knows about struct gpio_desc. I'll add a declaration to be on the safe side. > > int irq_line; > > bool status_appended; Thanks for your feedback! Uwe
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index ea4aabd3960a..4c8d986b6609 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -539,12 +539,29 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private) { struct ad_sigma_delta *sigma_delta = private; - complete(&sigma_delta->completion); - disable_irq_nosync(irq); - sigma_delta->irq_dis = true; - iio_trigger_poll(sigma_delta->trig); + /* + * AD7124 and a few others use the same physical line for interrupt + * reporting (nRDY) and MISO. + * As MISO toggles when reading a register, this likely results in a + * pending interrupt. This has two consequences: a) The irq might + * trigger immediately after it's enabled even though the conversion + * isn't done yet; and b) checking the STATUS register's nRDY flag is + * off-limits as reading that would trigger another irq event. + * + * So read the MOSI line as GPIO (if available) and only trigger the irq + * if the line is active. + */ - return IRQ_HANDLED; + if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) { + complete(&sigma_delta->completion); + disable_irq_nosync(irq); + sigma_delta->irq_dis = true; + iio_trigger_poll(sigma_delta->trig); + + return IRQ_HANDLED; + } else { + return IRQ_NONE; + } } /** @@ -679,6 +696,14 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev, else sigma_delta->irq_line = spi->irq; + sigma_delta->rdy_gpiod = devm_gpiod_get_optional(&spi->dev, "rdy", GPIOD_IN); + if (IS_ERR(sigma_delta->rdy_gpiod)) + return dev_err_probe(&spi->dev, PTR_ERR(sigma_delta->rdy_gpiod), + "Failed to find rdy gpio\n"); + + if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line) + sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod); + iio_device_set_drvdata(indio_dev, sigma_delta); return 0; diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h index f8c1d2505940..866b4c21794b 100644 --- a/include/linux/iio/adc/ad_sigma_delta.h +++ b/include/linux/iio/adc/ad_sigma_delta.h @@ -96,6 +96,7 @@ struct ad_sigma_delta { unsigned int active_slots; unsigned int current_slot; unsigned int num_slots; + struct gpio_desc *rdy_gpiod; int irq_line; bool status_appended; /* map slots to channels in order to know what to expect from devices */
Some of the ADCs by Analog signal their irq condition on the MISO line. So typically that line is connected to an SPI controller and a GPIO. The GPIO is used as input and the respective interrupt is enabled when the last SPI transfer is completed. Depending on the GPIO controller the toggling MISO line might make the interrupt pending even while it's masked. In that case the irq handler is called immediately after irq_enable() and so before the device actually pulls that line low which results in non-sense values being reported to the upper layers. The only way to find out if the line was actually pulled low is to read the GPIO. (There is a flag in AD7124's status register that also signals if an interrupt was asserted, but reading that register toggles the MISO line and so might trigger another spurious interrupt.) Add the possibility to specify an interrupt GPIO in the machine description in addition to the plain interrupt. This GPIO is used then to check if the irq line is actually active in the irq handler. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad_sigma_delta.c | 35 ++++++++++++++++++++++---- include/linux/iio/adc/ad_sigma_delta.h | 1 + 2 files changed, 31 insertions(+), 5 deletions(-)