On Tue, 11 Feb 2025 19:57:02 +0200
Robert Budai <robert.budai@xxxxxxxxxx> wrote:
> The ADIS16550 is a complete inertial system that includes a triaxis
> gyroscope and a triaxis accelerometer. Each inertial sensor in the
> ADIS16550 combines industry leading MEMS only technology with signal
> conditioning that optimizes dynamic performance. The factory calibration
> characterizes each sensor for sensitivity, bias, and alignment. As a
> result, each sensor has its own dynamic compensation formulas that
> provide accurate sensor measurements.
>
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Robert Budai <robert.budai@xxxxxxxxxx>
> ---
>
> v7:
> - minor changes according to PR suggestions
List them next time. Reviewers tend to have limited memories.
>
A few things from a fresh glance through.
Jonathan
> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> new file mode 100644
> index 000000000..b4a88e31c
> --- /dev/null
> +++ b/drivers/iio/imu/adis16550.c
> +
> +struct adis16550 {
> + const struct adis16550_chip_info *info;
> + struct adis adis;
> + unsigned long clk_freq_hz;
> + u32 sync_mode;
> + struct spi_transfer xfer[2];
> + u8 buffer[ADIS16550_BURST_DATA_LEN + sizeof(u32)] __aligned(IIO_DMA_MINALIGN);
> + __be32 din[2] __aligned(IIO_DMA_MINALIGN);
Why do you need the second __aligned(IIO_DMA_MINALIGN)?
Do you access any of these buffers whilst others are in use? If not it
should be fine to just mark the first one (Which is about avoiding overlap
with the earlier fields) and reduce the resulting padding.
> + __be32 dout[2];
> +};
;
> +
> +static u32 adis16550_validate_crc(const u32 *buf, const u8 n_elem,
> + const u32 crc)
> +{
> + int i;
> + u32 crc_calc;
> + u32 crc_buf[ADIS16550_BURST_N_ELEM - 2];
> + /*
> + * The crc calculation of the data is done in little endian. Hence, we
> + * always swap the 32bit elements making sure that the data LSB is
> + * always on address 0...
> + */
> + for (i = 0; i < n_elem; i++)
> + crc_buf[i] = swab32(buf[i]);
Probably makes more sense to pass it in to this function as __be32 *
Also do crc32 implementations really take little endian rather than
cpu endian? I suspect not, so my guess is you should be using
be32_to_cpu() here which will swap on little endian architectures
only.
> +
> + crc_calc = crc32(~0, crc_buf, n_elem * 4);
> + crc_calc ^= ~0;
> +
> + return (crc_calc == crc);
> +}
> +
> +static irqreturn_t adis16550_trigger_handler(int irq, void *p)
> +{
> + u32 crc;
> + int ret;
> + u16 dummy;
> + bool valid;
> + struct iio_poll_func *pf = p;
> + __be32 data[ADIS16550_MAX_SCAN_DATA];
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adis16550 *st = iio_priv(indio_dev);
> + struct adis *adis = iio_device_get_drvdata(indio_dev);
> + __be32 *buffer = (__be32 *)st->buffer;
> +
> + ret = spi_sync(adis->spi, &adis->msg);
> + if (ret)
> + goto done;
> + /*
> + * Validate the header. The header is a normal spi reply with state
> + * vector and crc4.
> + */
> + ret = adis16550_spi_validate(&st->adis, buffer[0], &dummy);
> + if (ret)
> + goto done;
> +
> + crc = be32_to_cpu(buffer[ADIS16550_BURST_N_ELEM - 1]);
> + /* the header is not included in the crc */
> + valid = adis16550_validate_crc((u32 *)&buffer[1],
Why not pass it as __be32 * ?
> + ADIS16550_BURST_N_ELEM - 2, crc);
> + if (!valid) {
> + dev_err(&adis->spi->dev, "Burst Invalid crc!\n");
> + goto done;
> + }
> +
> + /* copy the temperature together with sensor data */
> + memcpy(data, &buffer[3],
> + (ADIS16550_SCAN_ACCEL_Z - ADIS16550_SCAN_GYRO_X + 2) *
> + sizeof(__be32));
> + iio_push_to_buffers_with_timestamp(indio_dev, data, pf->timestamp);
> +done:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static const unsigned long adis16550_channel_masks[] = {
> + ADIS16550_BURST_DATA_GYRO_ACCEL_MASK | BIT(ADIS16550_SCAN_TEMP),
> + ADIS16550_BURST_DATA_DELTA_ANG_VEL_MASK | BIT(ADIS16550_SCAN_TEMP),
> + 0,
This one is a terminator, so no trailing comma (unlike below)
> +};
> +
> +enum {
> + ADIS16550_STATUS_CRC_CODE,
> + ADIS16550_STATUS_CRC_CONFIG,
> + ADIS16550_STATUS_FLASH_UPDATE,
> + ADIS16550_STATUS_INERIAL,
> + ADIS16550_STATUS_SENSOR,
> + ADIS16550_STATUS_TEMPERATURE,
> + ADIS16550_STATUS_SPI,
> + ADIS16550_STATUS_PROCESSING,
> + ADIS16550_STATUS_POWER,
> + ADIS16550_STATUS_BOOT,
> + ADIS16550_STATUS_WATCHDOG = 15,
> + ADIS16550_STATUS_REGULATOR = 28,
> + ADIS16550_STATUS_SENSOR_SUPPLY,
> + ADIS16550_STATUS_CPU_SUPPLY,
> + ADIS16550_STATUS_5V_SUPPLY
Trailing comma. Generally unless the value is such that nothing should ever
follow it (e.g. NULL terminators and similar), you should have trailing commas.
> +};
