Re: [PATCH v2] iio: adis16480: support burst read function

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

...
> > > +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á