On Fri, Sep 20, 2024 at 3:24 PM Nechita, Ramona
<Ramona.Nechita@xxxxxxxxxx> 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.)
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.
