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@xxxxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
