| Message ID | 20210408075643.70183-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: adis16480: support burst read function | expand |
On 4/8/21 9:56 AM, 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@analog.com> Looks good to me. Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
On Thu, 8 Apr 2021 09:56:43 +0200 Nuno Sa <nuno.sa@analog.com> 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@analog.com> 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. > #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. > + } > + } > + > + 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? If something more complex is going on, perhaps a comment here? > + 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. > + */ > + 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; > + } > + } > + > + if (offset == 4) { > + dev_err(&adis->spi->dev, "Invalid burst data\n"); > + goto irq_done; > + } > + > + crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]); > + valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc); > + if (!valid) { > + dev_err(&adis->spi->dev, "Invalid crc\n"); > + goto irq_done; > + } > + > + for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) { > + /* > + * When burst mode is used, temperature is the first data > + * channel in the sequence, but the temperature scan index > + * is 10. > + */ > + switch (bit) { > + case ADIS16480_SCAN_TEMP: > + st->data[i++] = buffer[offset + 1]; > + break; > + case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z: > + /* The lower register data is sequenced first */ > + st->data[i++] = buffer[2 * bit + offset + 3]; > + st->data[i++] = buffer[2 * bit + offset + 2]; > + break; > + } > + } > + > + iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp); > +irq_done: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > static const struct iio_info adis16480_info = { > .read_raw = &adis16480_read_raw, > .write_raw = &adis16480_write_raw, > @@ -1341,7 +1476,7 @@ static int adis16480_probe(struct spi_device *spi) > st->clk_freq = st->chip_info->int_clk; > } > > - ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL); > + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, adis16480_trigger_handler); Whilst 100 chars is fine, it's preferred to stay under 80 when it doesn't hurt readability so I'd like to see this on two lines. > if (ret) > return ret; >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, April 11, 2021 4:40 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio@vger.kernel.org; Hennerich, Michael > <Michael.Hennerich@analog.com>; Lars-Peter Clausen > <lars@metafoo.de> > Subject: Re: [PATCH v2] iio: adis16480: support burst read function > > > On Thu, 8 Apr 2021 09:56:43 +0200 > Nuno Sa <nuno.sa@analog.com> 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@analog.com> > 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á
... > > > +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á
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> #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; + } + } + + 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. + */ + 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; + } + } + + if (offset == 4) { + dev_err(&adis->spi->dev, "Invalid burst data\n"); + goto irq_done; + } + + crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]); + valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc); + if (!valid) { + dev_err(&adis->spi->dev, "Invalid crc\n"); + goto irq_done; + } + + for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) { + /* + * When burst mode is used, temperature is the first data + * channel in the sequence, but the temperature scan index + * is 10. + */ + switch (bit) { + case ADIS16480_SCAN_TEMP: + st->data[i++] = buffer[offset + 1]; + break; + case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z: + /* The lower register data is sequenced first */ + st->data[i++] = buffer[2 * bit + offset + 3]; + st->data[i++] = buffer[2 * bit + offset + 2]; + break; + } + } + + iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp); +irq_done: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + static const struct iio_info adis16480_info = { .read_raw = &adis16480_read_raw, .write_raw = &adis16480_write_raw, @@ -1341,7 +1476,7 @@ static int adis16480_probe(struct spi_device *spi) st->clk_freq = st->chip_info->int_clk; } - ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL); + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, adis16480_trigger_handler); if (ret) return ret;
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@analog.com> --- 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(-)