Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 23 Sep 2024 14:51:28 +0200
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, Sep 20, 2024 at 3:24 PM Nechita, Ramona
> <Ramona.Nechita@analog.com> wrote:
> >
> > Hello all,
> >
> > Just a minor question
> > ...  
> > >  
> > >> +
> > >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) {
> > >> +    struct iio_poll_func *pf = p;
> > >> +    struct iio_dev *indio_dev = pf->indio_dev;
> > >> +    struct ad7779_state *st = iio_priv(indio_dev);
> > >> +    int ret;
> > >> +    int bit;
> > >> +    int k = 0;
> > >> +    /*
> > >> +     * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32
> > >> +     * for the data and 64 bits for the timestamp
> > >> +     */
> > >> +    u32 tmp[10];
> > >> +
> > >> +    struct spi_transfer sd_readback_tr[] = {
> > >> +            {
> > >> +                    .rx_buf = st->spidata_rx,
> > >> +                    .tx_buf = st->spidata_tx,
> > >> +                    .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE,
> > >> +            }
> > >> +    };
> > >> +
> > >> +    if (!iio_buffer_enabled(indio_dev))
> > >> +            goto exit_handler;  
> > >
> > >If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter.  If it does, what happens?
> > >I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers.  
> >
> > I added that condition rather because the DRDY pulse will keep on being generated even when the buffers are not active,
> > and it would be better to exit the function sooner. I tested it and it does not break to remove the condition, I just
> > thought it made more sense like this. Should I delete it?
> >  
> > >....  
> >
> > Best regards,
> > Ramona Nechita
> >
> >  
> 
> Perhaps a better way to handle this would be to move
> 
>     disable_irq(st->spi->irq);
> 
> to the buffer predisable callback instead of doing it in the buffer
> postdisable callback. Then we will be sure to not get any more DRDY
> interrupts after the buffer is disabled.
> 
> (And to keep things balanced, moved the corresponding irq_enable() to
> the buffer postenable callback.)

That makes logical sense but in reality in the vast majority of cases
it makes no practical difference whether things are in the pre or
post callbacks as the fundamental races with tear down are there in
both cases but shouldn't matter as they correspond to slightly
earlier or later disabling of the buffer.  So this is a nice to
have for readabilty and understanding rather than a required change I think.

> 
> Since ad7779_trigger_handler is the IIO trigger interrupt handler and
> not the DRDY interrupt handler though, it is already not possible for
> this interrupt handler to be called while the IIO buffer is enabled.
> So it should be safe to remove the if
> (!iio_buffere_enabled(indio_dev)) even without the other changes I
> suggested.
Exactly.  

Jonathan