> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, April 11, 2021 4:40 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>
> Subject: Re: [PATCH v2] iio: adis16480: support burst read function
>
>
> On Thu, 8 Apr 2021 09:56:43 +0200
> Nuno Sa <nuno.sa@xxxxxxxxxx> 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>
> Small issue with return values in the trigger handler and
> some stylistic comments.
>
> Otherwise, looks good to me.
>
> > ---
> > Changes in v2:
> > * Return right away if offset == 4 (no valid transition found from
> > brurst_id to sys_flags)
> >
> > drivers/iio/imu/adis16480.c | 161
> +++++++++++++++++++++++++++++++++---
> > 1 file changed, 148 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index f81b86690b76..b782070a27f5 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -5,6 +5,7 @@
> > * Copyright 2012 Analog Devices Inc.
> > */
> >
> > +#include <linux/crc32.h>
>
> Hmm. The ordering here is a bit odd so I guess this doesn't
> really matter, but seems strange to have something so specific as
> crc32.h as the first header included when it's not the first one
> alphabetically.
Yeah, it was not ordered already, so I didn't cared much. I will move more
to the middle...
> > #include <linux/clk.h>
> > #include <linux/bitfield.h>
> > #include <linux/of_irq.h>
> > @@ -19,11 +20,14 @@
> > #include <linux/sysfs.h>
> > #include <linux/module.h>
> > #include <linux/lcm.h>
> > +#include <linux/swab.h>
> >
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/buffer.h>
> > #include <linux/iio/imu/adis.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> >
> > #include <linux/debugfs.h>
> >
> > @@ -103,6 +107,12 @@
> > * Available only for ADIS1649x devices
> > */
> > #define ADIS16495_REG_SYNC_SCALE
> ADIS16480_REG(0x03, 0x10)
> > +#define ADIS16495_REG_BURST_CMD
> ADIS16480_REG(0x00, 0x7C)
> > +#define ADIS16495_BURST_ID 0xA5A5
> > +/* total number of segments in burst */
> > +#define ADIS16495_BURST_MAX_DATA 20
> > +/* spi max speed in burst mode */
> > +#define ADIS16495_BURST_MAX_SPEED 6000000
> >
> > #define ADIS16480_REG_SERIAL_NUM
> ADIS16480_REG(0x04, 0x20)
> >
> > @@ -163,6 +173,8 @@ struct adis16480 {
> > struct clk *ext_clk;
> > enum adis16480_clock_mode clk_mode;
> > unsigned int clk_freq;
> > + /* Alignment needed for the timestamp */
> > + __be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8);
> > };
> >
> > static const char * const adis16480_int_pin_names[4] = {
> > @@ -863,7 +875,7 @@ static const char * const
> adis16480_status_error_msgs[] = {
> >
> > static int adis16480_enable_irq(struct adis *adis, bool enable);
> >
> > -#define ADIS16480_DATA(_prod_id, _timeouts)
> \
> > +#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len)
> \
> > { \
> > .diag_stat_reg = ADIS16480_REG_DIAG_STS, \
> > .glob_cmd_reg = ADIS16480_REG_GLOB_CMD,
> \
> > @@ -887,6 +899,8 @@ static int adis16480_enable_irq(struct adis
> *adis, bool enable);
> > BIT(ADIS16480_DIAG_STAT_BARO_FAIL),
> \
> > .enable_irq = adis16480_enable_irq, \
> > .timeouts = (_timeouts), \
> > + .burst_reg_cmd = ADIS16495_REG_BURST_CMD,
> \
> > + .burst_len = (_burst_len), \
> > }
> >
> > static const struct adis_timeout adis16485_timeouts = {
> > @@ -931,7 +945,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .int_clk = 2460000,
> > .max_dec_rate = 2048,
> > .filter_freqs = adis16480_def_filter_freqs,
> > - .adis_data = ADIS16480_DATA(16375,
> &adis16485_timeouts),
> > + .adis_data = ADIS16480_DATA(16375,
> &adis16485_timeouts, 0),
> > },
> > [ADIS16480] = {
> > .channels = adis16480_channels,
> > @@ -944,7 +958,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .int_clk = 2460000,
> > .max_dec_rate = 2048,
> > .filter_freqs = adis16480_def_filter_freqs,
> > - .adis_data = ADIS16480_DATA(16480,
> &adis16480_timeouts),
> > + .adis_data = ADIS16480_DATA(16480,
> &adis16480_timeouts, 0),
> > },
> > [ADIS16485] = {
> > .channels = adis16485_channels,
> > @@ -957,7 +971,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .int_clk = 2460000,
> > .max_dec_rate = 2048,
> > .filter_freqs = adis16480_def_filter_freqs,
> > - .adis_data = ADIS16480_DATA(16485,
> &adis16485_timeouts),
> > + .adis_data = ADIS16480_DATA(16485,
> &adis16485_timeouts, 0),
> > },
> > [ADIS16488] = {
> > .channels = adis16480_channels,
> > @@ -970,7 +984,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .int_clk = 2460000,
> > .max_dec_rate = 2048,
> > .filter_freqs = adis16480_def_filter_freqs,
> > - .adis_data = ADIS16480_DATA(16488,
> &adis16485_timeouts),
> > + .adis_data = ADIS16480_DATA(16488,
> &adis16485_timeouts, 0),
> > },
> > [ADIS16490] = {
> > .channels = adis16485_channels,
> > @@ -984,7 +998,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16490,
> &adis16495_timeouts),
> > + .adis_data = ADIS16480_DATA(16490,
> &adis16495_timeouts, 0),
> > },
> > [ADIS16495_1] = {
> > .channels = adis16485_channels,
> > @@ -998,7 +1012,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > [ADIS16495_2] = {
> > .channels = adis16485_channels,
> > @@ -1012,7 +1028,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > [ADIS16495_3] = {
> > .channels = adis16485_channels,
> > @@ -1026,7 +1044,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > [ADIS16497_1] = {
> > .channels = adis16485_channels,
> > @@ -1040,7 +1060,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > [ADIS16497_2] = {
> > .channels = adis16485_channels,
> > @@ -1054,7 +1076,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > [ADIS16497_3] = {
> > .channels = adis16485_channels,
> > @@ -1068,10 +1092,121 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> > .max_dec_rate = 4250,
> > .filter_freqs = adis16495_def_filter_freqs,
> > .has_pps_clk_mode = true,
> > - .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > + /* 20 elements of 16bits */
> > + .adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> > },
> > };
> >
> > +static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem,
> const u32 crc)
> > +{
> > + u32 crc_calc;
> > + u16 crc_buf[15];
> > + int j;
> > +
> > + for (j = 0; j < n_elem; j++)
> > + crc_buf[j] = swab16(buf[j]);
> > +
> > + 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;
>
> 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...
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...
> > + }
> > + }
> > +
> > + 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á
