> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Sent: Wednesday, April 7, 2021 10:25 AM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH] iio: adis16480: support burst read function
>
> [External]
>
> On 4/6/21 5:14 PM, Nuno Sa wrote:
> > Some supported devices support burst read function. This provides a
> method
> > for reading a batch of data (status, temperature, gyroscopes,
> > accelerometers, time stamp/data counter, and CRC code), which
> does not
> > require a stall time between each 16-bit segment and only requires
> one
> > command on the DIN line to initiate. Devices supporting this mode
> > are:
> >
> > * adis16495-1
> > * adis16495-2
> > * adis16495-3
> > * adis16497-1
> > * adis16497-2
> > * adis16497-3
> >
> > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> Looks good to me, just some thoughts on CRC and endiness
> conversion.
>
> > ---
> > drivers/iio/imu/adis16480.c | 157
> +++++++++++++++++++++++++++++++++---
> > 1 file changed, 144 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index f81b86690b76..341945f8339e 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -5,6 +5,7 @@
> > * Copyright 2012 Analog Devices Inc.
> > */
> >
> > [...]
> > +static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem,
> const u32 crc)
> const __be16 *buf
> > +{
> > + u32 crc_calc;
> > + u16 crc_buf[15];
> > + int j;
> > +
> > + for (j = 0; j < n_elem; j++)
> > + crc_buf[j] = swab16(buf[j]);
> be16_to_cpu(buf[j])
I don't think this would work on BE machines. AFAIU, the crc computation
must be done in little endian ("order of bytes low-high"). In BE machines,
crc_buf would be left in BE order which would lead to a wrong crc...
The ' swab16()' was really intentional here (naturally can still be wrong :D).
> > +
> > + crc_calc = crc32(~0, crc_buf, n_elem * 2);
> > + crc_calc ^= ~0;
> > +
> > + return (crc == crc_calc);
> > +}
> > +
> > +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;
> > + }
> > + }
> > +
> > + 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;
> > + adis_dev_unlock(adis);
> > +
> > + /*
> > + * After making the burst request, the response can have one
> or two
> > + * 16-bit responses containing the BURST_ID depending on the
> sclk. If
> > + * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk
> < 3MHZ,
> > + * we have only one. To manage that variation, we use the
> transition from the
> > + * BURST_ID to the SYS_E_FLAG register, which will not be
> equal to 0xA5A5. If
> > + * we not find this variation in the first 4 segments, then the
> data should
> > + * not be valid. We don't return right away since the crc
> validation should
> > + * fail anyways...
> > + */
> > + buffer = adis->buffer;
> > + for (offset = 0; offset < 4; offset++) {
> > + u16 curr = be16_to_cpu(buffer[offset]);
> > + u16 next = be16_to_cpu(buffer[offset + 1]);
> > +
> > + if (curr == ADIS16495_BURST_ID && next !=
> ADIS16495_BURST_ID) {
> > + offset++;
> > + break;
> > + }
> > + }
>
> I think offset can be up to 4 here, in which case offset + 16 is
> out-of-bounds for buffer. Maybe return right away if offset is 4.
>
Auch, good catch!
- Nuno Sá
