On Wed, 29 Jan 2025 10:20:45 +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 com-
> pensation formulas that provide accurate sensor measurements
Oddly short wrap and don't break compensation across lines.
Wrap for commit messages should be 75 chars approximately.
>
> 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>
Various comments inline.
Jonathan
> ---
>
> v5:
> - corrected module MODULE_IMPORT_NS
> - memory allocation for buffer handled by device entirely now
> - changed scaled sync mode logic to be based on external clock-frequency
> - corrected formating according to suggestions
>
> drivers/iio/imu/Kconfig | 13 +
> drivers/iio/imu/Makefile | 1 +
> drivers/iio/imu/adis16550.c | 1182 +++++++++++++++++++++++++++++++++++
> 3 files changed, 1196 insertions(+)
> create mode 100644 drivers/iio/imu/adis16550.c
>
> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> index ca0efecb5b5c..c4e06ebbe058 100644
> --- a/drivers/iio/imu/Kconfig
> +++ b/drivers/iio/imu/Kconfig
> @@ -52,6 +52,19 @@ config ADIS16480
> Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
> ADIS16485, ADIS16488 inertial sensors.
>
> +config ADIS16550
> + tristate "Analog Devices ADIS16550 and similar IMU driver"
> + depends on SPI
> + select IIO_ADIS_LIB
> + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> + select CRC32
> + help
> + Say yes here to build support for Analog Devices ADIS16550 inertial
> + sensor.
Add a little more detail on what data it measures etc.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adis16550.
> +
> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> new file mode 100644
> index 000000000000..57b666db141d
> --- /dev/null
> +++ b/drivers/iio/imu/adis16550.c
> @@ -0,0 +1,1182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADIS16550 IMU driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/crc32.h>
> +#include <linux/debugfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/imu/adis.h>
> +#include <linux/iio/sysfs.h>
That is very rarely needed in a modern driver. Why is it here?
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/lcm.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/swab.h>
> +#include <linux/unaligned.h>
> +
> +#define ADIS16550_REG_BURST_GYRO_ACCEL 0x0a
> +#define ADIS16550_REG_BURST_DELTA_ANG_VEL 0x0b
> +#define ADIS16550_BURST_DATA_GYRO_ACCEL_MASK GENMASK(6, 1)
> +#define ADIS16550_BURST_DATA_DELTA_ANG_VEL_MASK GENMASK(12, 7)
> +
> +#define ADIS16550_REG_STATUS 0x0e
> +#define ADIS16550_REG_TEMP 0x10
> +#define ADIS16550_REG_X_GYRO 0x12
> +#define ADIS16550_REG_Y_GYRO 0x14
> +#define ADIS16550_REG_Z_GYRO 0x16
> +#define ADIS16550_REG_X_ACCEL 0x18
> +#define ADIS16550_REG_Y_ACCEL 0x1a
> +#define ADIS16550_REG_Z_ACCEL 0x1c
> +#define ADIS16550_REG_X_DELTANG_L 0x1E
> +#define ADIS16550_REG_Y_DELTANG_L 0x20
> +#define ADIS16550_REG_Z_DELTANG_L 0x22
> +#define ADIS16550_REG_X_DELTVEL_L 0x24
> +#define ADIS16550_REG_Y_DELTVEL_L 0x26
> +#define ADIS16550_REG_Z_DELTVEL_L 0x28
> +#define ADIS16550_REG_X_GYRO_SCALE 0x30
> +#define ADIS16550_REG_Y_GYRO_SCALE 0x32
> +#define ADIS16550_REG_Z_GYRO_SCALE 0x34
> +#define ADIS16550_REG_X_ACCEL_SCALE 0x36
> +#define ADIS16550_REG_Y_ACCEL_SCALE 0x38
> +#define ADIS16550_REG_Z_ACCEL_SCALE 0x3a
> +#define ADIS16550_REG_X_GYRO_BIAS 0x40
> +#define ADIS16550_REG_Y_GYRO_BIAS 0x42
> +#define ADIS16550_REG_Z_GYRO_BIAS 0x44
> +#define ADIS16550_REG_X_ACCEL_BIAS 0x46
> +#define ADIS16550_REG_Y_ACCEL_BIAS 0x48
> +#define ADIS16550_REG_Z_ACCEL_BIAS 0x4a
> +#define ADIS16550_REG_COMMAND 0x50
> +#define ADIS16550_REG_CONFIG 0x52
> +#define ADIS16550_GYRO_FIR_EN_MASK BIT(3)
> +#define ADIS16550_ACCL_FIR_EN_MASK BIT(2)
> +#define ADIS16550_SYNC_MASK \
> + (ADIS16550_SYNC_EN_MASK | ADIS16550_SYNC_MODE_MASK)
Indent just one tab.
> +#define ADIS16550_SYNC_MODE_MASK BIT(1)
> +#define ADIS16550_SYNC_EN_MASK BIT(0)
> +/* max of 4000 SPS in scale sync */
> +#define ADIS16550_SYNC_SCALE_MAX_RATE (4000 * 1000)
> +#define ADIS16550_REG_DEC_RATE 0x54
> +#define ADIS16550_REG_SYNC_SCALE 0x56
> +#define ADIS16550_REG_SERIAL_NUM 0x76
> +#define ADIS16550_REG_FW_REV 0x7A
> +#define ADIS16550_REG_FW_DATE 0x7C
> +#define ADIS16550_REG_PROD_ID 0x7E
> +#define ADIS16550_REG_FLASH_CNT 0x72
> +/* spi protocol*/
/* SPI protocol */
> +#define ADIS16550_SPI_DATA_MASK GENMASK(31, 16)
> +#define ADIS16550_SPI_REG_MASK GENMASK(14, 8)
> +#define ADIS16550_SPI_R_W_MASK BIT(7)
> +#define ADIS16550_SPI_CRC_MASK GENMASK(3, 0)
> +#define ADIS16550_SPI_SV_MASK GENMASK(7, 6)
> +/* burst read */
> +#define ADIS16550_BURST_N_ELEM 12
> +#define ADIS16550_BURST_DATA_LEN (ADIS16550_BURST_N_ELEM * 4)
This is not very big. Can you use an __aligned(IIO_DMA_MINALIGN) buffer
at the end of iio_priv() for this instead and avoid the separate
allocation?
> +#define ADIS16550_MAX_SCAN_DATA 12
> +
> +struct adis16550_sync {
> + u16 sync_mode;
> + u16 min_rate;
> + u16 max_rate;
> +};
> +
> +struct adis16550_chip_info {
> + const struct iio_chan_spec *channels;
> + const struct adis16550_sync *sync_mode;
> + char *name;
> + u32 num_channels;
> + u32 gyro_max_val;
> + u32 gyro_max_scale;
> + u32 accel_max_val;
> + u32 accel_max_scale;
> + u32 temp_scale;
> + u32 deltang_max_val;
> + u32 deltvel_max_val;
> + u32 int_clk;
> + u16 max_dec;
> + u16 num_sync;
> +};
> +
> +struct adis16550 {
> + const struct adis16550_chip_info *info;
> + struct adis adis;
> + unsigned long clk_freq_hz;
> + u32 sync_mode;
> + struct spi_transfer *xfer;
> + void *buffer;
> +
This blank line should go. It doesn't add anything useful.
> +};
...
> +static void adis16550_spi_msg_prepare(const u32 reg, const bool write,
> + const u16 data, __be32 *din)
> +{
> + u8 crc;
> + u32 __din;
> +
> + __din = FIELD_PREP(ADIS16550_SPI_REG_MASK, reg);
> +
> + if (write) {
> + __din |= FIELD_PREP(ADIS16550_SPI_R_W_MASK, true);
I'd put 1 in there. FIELD_PREP takes an integer not a boolean.
Ends up the same but still good to make the code show it is an integer.
> + __din |= FIELD_PREP(ADIS16550_SPI_DATA_MASK, data);
> + }
> +
> + crc = spi_crc4(__din);
> + __din |= FIELD_PREP(ADIS16550_SPI_CRC_MASK, crc);
> +
> + *din = cpu_to_be32(__din);
> +}
> +
> +static int adis16550_spi_xfer(const struct adis *adis, u32 reg, u32 len,
> + u32 *readval, u32 writeval)
> +{
> + int ret;
> + u16 data = 0;
> + struct spi_message msg;
> + __be32 __din[2], __dout[2];
These are not DMA safe buffers. You can't use them like this for
SPI transfers. There are two paths to dma safe buffers, either
kzalloc them or __aligned(IIO_DMA_MINALIGN) to create a new cacheline
at the end of the iio_priv() structure.
> + bool wr = readval ? false : true;
> + struct spi_device *spi = adis->spi;
> + struct spi_transfer xfers[] = {
> + {
> + .tx_buf = &__din[0],
> + .len = 4,
> + .cs_change = 1,
> + }, {
> + .tx_buf = &__din[1],
> + .len = 4,
> + .cs_change = 1,
> + .rx_buf = __dout,
> + }, {
> + .tx_buf = &__din[1],
> + .rx_buf = &__dout[1],
> + .len = 4,
> + },
> + };
> +
> + spi_message_init(&msg);
> +
> + switch (len) {
> + case 4:
> + adis16550_spi_msg_prepare(reg + 1, wr, writeval >> 16,
> + &__din[0]);
> + spi_message_add_tail(&xfers[0], &msg);
> + fallthrough;
> + case 2:
> + adis16550_spi_msg_prepare(reg, wr, writeval, &__din[1]);
> + spi_message_add_tail(&xfers[1], &msg);
> + spi_message_add_tail(&xfers[2], &msg);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = spi_sync(spi, &msg);
> + if (ret) {
> + dev_err(&spi->dev, "Spi failure %d\n", ret);
> + return ret;
> + }
> + /*
> + * When writing a register, the device will reply with a readback on the
> + * transfer so that we can validate if our data was actually written..
> + */
> + switch (len) {
> + case 4:
> + ret = adis16550_spi_validate(adis, __dout[0], &data);
> + if (ret)
> + return ret;
> +
> + if (readval) {
> + *readval = data << 16;
> + } else if ((writeval >> 16) != data && reg != ADIS16550_REG_COMMAND) {
> + dev_err(&spi->dev,
> + "Data not written: wr: 0x%04X, rcv: 0x%04X\n",
> + writeval >> 16, data);
> + return -EIO;
> + }
> +
> + fallthrough;
> + case 2:
> + ret = adis16550_spi_validate(adis, __dout[1], &data);
> + if (ret)
> + return ret;
> +
> + if (readval) {
> + *readval = (*readval & GENMASK(31, 16)) | data;
> + } else if ((writeval & GENMASK(15, 0)) != data && reg != ADIS16550_REG_COMMAND) {
> + dev_err(&spi->dev,
> + "Data not written: wr: 0x%04X, rcv: 0x%04X\n",
> + (u16)writeval, data);
> + return -EIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int adis16550_spi_read(struct adis *adis, const u32 reg,
> + u32 *value, const u32 len)
> +{
> + return adis16550_spi_xfer(adis, reg, len, value, 0);
> +}
> +
> +static int adis16550_spi_write(struct adis *adis, const u32 reg,
> + const u32 value, const u32 len)
> +{
> + return adis16550_spi_xfer(adis, reg, len, NULL, value);
> +}
> +
> +static ssize_t adis16550_show_firmware_date(struct file *file,
> + char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct adis16550 *st = file->private_data;
> + u32 date;
> + char buf[12];
> + size_t len;
> + int ret;
It is normally a good idea to pick an ordering convention and stick
to it. I don't really care what it is, but if nothing else comes
to mind reverse xmas at least looks prettier!
> +
> + ret = adis_read_reg_32(&st->adis, ADIS16550_REG_FW_DATE, &date);
> + if (ret)
> + return ret;
> +
> + len = scnprintf(buf, sizeof(buf), "%.2x-%.2x-%.4x\n", date & 0xff,
> + (date >> 8) & 0xff, date >> 16);
> +
> + return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> +}
> +static void adis16550_debugfs_init(struct iio_dev *indio_dev)
> +{
> + struct adis16550 *st = iio_priv(indio_dev);
> + struct dentry *d = iio_get_debugfs_dentry(indio_dev);
> +
> + debugfs_create_file_unsafe("serial_number", 0400,
> + d, st, &adis16550_serial_number_fops);
Strange wrap. Why not drag d and st to the line above?
> + debugfs_create_file_unsafe("product_id", 0400,
> + d, st, &adis16550_product_id_fops);
> + debugfs_create_file("firmware_revision", 0400,
> + d, st, &adis16550_firmware_revision_fops);
> + debugfs_create_file("firmware_date", 0400, d,
> + st, &adis16550_firmware_date_fops);
> + debugfs_create_file_unsafe("flash_count", 0400,
> + d, st, &adis16550_flash_count_fops);
> +}
> +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),
This need to be terminated with a 0.
> +};
> +
> +static int adis16550_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct adis *adis = iio_device_get_drvdata(indio_dev);
> + u16 burst_length = ADIS16550_BURST_DATA_LEN;
> + struct adis16550 *st = iio_priv(indio_dev);
> + u8 burst_cmd;
> + u8 *tx;
> +
> + memset(st->buffer, 0, burst_length + sizeof(u32));
Given you treat tx as a set of 4 u8, sizeof(u32) doesn't make much
sense. Can you just use the size define that was used to allocate this
buffer?
> +
> + if (*scan_mask & ADIS16550_BURST_DATA_GYRO_ACCEL_MASK)
> + burst_cmd = ADIS16550_REG_BURST_GYRO_ACCEL;
> + else
> + burst_cmd = ADIS16550_REG_BURST_DELTA_ANG_VEL;
> +
> + tx = st->buffer + burst_length;
> + tx[0] = 0x00;
> + tx[1] = 0x00;
> + tx[2] = burst_cmd;
> + /* crc4 is 0 on burst command */
> + tx[3] = spi_crc4(get_unaligned_le32(tx));
> +
> + spi_message_init_with_transfers(&adis->msg, st->xfer, 2);
