...
> > > +static irqreturn_t adis16480_trigger_handler(int irq, void *p)
> > > +{
> > > + struct iio_poll_func *pf = p;
> > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > + struct adis16480 *st = iio_priv(indio_dev);
> > > + struct adis *adis = &st->adis;
> > > + int ret, bit, offset, i = 0;
> > > + __be16 *buffer;
> > > + u32 crc;
> > > + bool valid;
> > > + const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
> > > +
> > > + adis_dev_lock(adis);
> > > + if (adis->current_page != 0) {
> > > + adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> > > + adis->tx[1] = 0;
> > > + ret = spi_write(adis->spi, adis->tx, 2);
> > > + if (ret) {
> > > + dev_err(&adis->spi->dev, "Failed to change
> > device page: %d\n", ret);
> > > + adis_dev_unlock(adis);
> > > + return ret;
> >
> > This is an interrupt handler, you should be careful what you return
> > as they will be treated as irqreturn_t not ints.
> >
> > return IRQ_HANDLED even in error paths.
>
> Hmm, yeah, this is definitely not ok. Also imposes the question if we should
> call ' iio_trigger_notify_done()' in these error paths? I'm pending to do it as
> it might be a big assumption to say the device is 'broken' if some spi transfer
> fails...
Yup, that has always been a bit of an open question in drivers. As likely
as not, any breakage leaves the device in a state from which we can't recover
anyway. I've mostly left whether to call iio_trigger_notify_done() to the
discretion of the driver writers.
>
> Not doing it means we will never receive another irq (I think this is also true if
> we do not return IRQ_HANDLED)...
>
> Also need to check other places as I'm fairly sure we have this problem (at least)
> in the adis16475 driver...
oops. Guess I missed it there ;)
> > > + }
> > > + }
> > > +
> > > + adis->spi->max_speed_hz = ADIS16495_BURST_MAX_SPEED;
> > > +
> > > + ret = spi_sync(adis->spi, &adis->msg);
> > > + if (ret) {
> > > + dev_err(&adis->spi->dev, "Failed to read data: %d\n",
> > ret);
> > > + adis_dev_unlock(adis);
> > > + return ret;
> > > + }
> > > +
> > > + adis->spi->max_speed_hz = cached_spi_speed_hz;
> > > + adis->current_page = 0;
> >
> > Does it make more sense to move this to just after we changed the
> > page?
>
> Yes, it does. If the second spi transfer fails, we already moved to page 0
> but did not updated this variable...
>
> - Nuno Sá
