On Fri, 10 Jan 2025 09:42:53 +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
>
> 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>
> ---
>
> 4:
> - reorganized channels to match the order in the datasheet
> - removed extra checks and goto statements
> - for buffer memory allocation used only kfree, since adis library already takes care of freeing the buffer
That last bit makes for a mess wrt to who owns the buffer and lifetime
management. Suggestions inline.
Jonathan
> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> new file mode 100644
> index 000000000000..49c3ff9ef1e2
> --- /dev/null
> +++ b/drivers/iio/imu/adis16550.c
> @@ -0,0 +1,1202 @@
...
> +static int adis16550_set_accl_filter_freq(struct adis16550 *st, int freq_hz)
> +{
> + bool en = false;
> +
> + if (freq_hz)
> + en = true;
> +
> + return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> + ADIS16550_ACCL_FIR_EN_MASK,
> + (u32)FIELD_PREP(ADIS16550_ACCL_FIR_EN_MASK, en));
Why is the cast needed? Only bit 3 is set.
> +}
> +static irqreturn_t adis16550_trigger_handler(int irq, void *p)
> +{
> + u32 crc;
> + u16 dummy;
> + bool valid;
> + int ret, i = 0;
> + u8 data_offset = 3;
> + 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 = adis->buffer;
Related to below lifetime comment. Move this buffer into iio_priv()
given you aren't using the lifetime management in the adis library code.
> +
> + 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],
> + ADIS16550_BURST_N_ELEM - 2, crc);
> + if (!valid) {
> + dev_err(&adis->spi->dev, "Burst Invalid crc!\n");
> + goto done;
> + }
> +
> + memcpy(&data[i], &buffer[data_offset], (ADIS16550_SCAN_ACCEL_Z -
i is always 0? If so just use data here and get rid of i.
If data_offset is always 3, encode that directly here as well instead
of confusing local variable.
> + 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 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;
> + u8 burst_cmd;
> + u8 *tx;
> +
> + 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;
> +
> + memset(adis->xfer, 0, 2 * sizeof(*adis->xfer));
> + memset(adis->buffer, 0, burst_length + sizeof(u32));
This overlaps with the comment on lifetime management below.
I would move the xfer and buffer pointers into iio_priv leaving
adis->xfer and adis->buffer == NULL so that the core frees nothing.
> +
> + tx = adis->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));
> +
> + adis->xfer[0].tx_buf = tx;
> + adis->xfer[0].len = 4;
> + adis->xfer[0].cs_change = 1;
> + adis->xfer[0].delay.value = 8;
> + adis->xfer[0].delay.unit = SPI_DELAY_UNIT_USECS;
> + adis->xfer[1].rx_buf = adis->buffer;
> + adis->xfer[1].len = burst_length;
> +
> + spi_message_init_with_transfers(&adis->msg, adis->xfer, 2);
> +
> + return 0;
> +}
> +static int adis16550_config_sync(struct adis16550 *st)
> +{
> + struct device *dev = &st->adis.spi->dev;
> + const struct adis16550_sync *sync;
> + struct clk *clk;
> + u32 sync_mode;
> + u16 mode;
> + int ret;
> +
> + ret = device_property_read_u32(dev, "adi,sync-mode", &sync_mode);
> + if (ret) {
> + st->clk_freq_hz = st->info->int_clk * 1000;
> + return 0;
> + }
> +
> + if (sync_mode >= st->info->num_sync) {
> + dev_err(dev, "Invalid sync mode: %u for %s\n", sync_mode,
> + st->info->name);
> + return -EINVAL;
> + }
> +
> + sync = &st->info->sync_mode[sync_mode];
> + st->sync_mode = sync->sync_mode;
> +
> + clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + st->clk_freq_hz = clk_get_rate(clk);
> + if (st->clk_freq_hz > sync->max_rate || st->clk_freq_hz < sync->min_rate)
> + return dev_err_probe(dev, -EINVAL,
> + "Clk rate: %lu not in a valid range:[%u %u]\n",
> + st->clk_freq_hz, sync->min_rate, sync->max_rate);
> +
> + if (sync->sync_mode == ADIS16550_SYNC_MODE_SCALED) {
Similar to DT binding discussion. You can see from the clock frequency if you
need to need to use sync_mode_scaled or not. I don't see a need for a binding
to control that.
> + u16 sync_scale;
> + u32 scaled_freq = 0;
> + /*
> + * In pps scaled sync we must scale the input clock to a range
> + * of [3000 45000].
> + */
> + ret = device_property_read_u32(dev, "adi,scaled-output-hz",
> + &scaled_freq);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "adi,scaled-output-hz property not found");
> +
> + if (scaled_freq < 3000 || scaled_freq > 4500)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid value:%u for adi,scaled-output-hz",
> + scaled_freq);
> +
> + sync_scale = DIV_ROUND_CLOSEST(scaled_freq, st->clk_freq_hz);
> +
> + ret = adis_write_reg_16(&st->adis, ADIS16550_REG_SYNC_SCALE,
> + sync_scale);
> + if (ret)
> + return ret;
> +
> + st->clk_freq_hz = scaled_freq;
> + }
> +
> + st->clk_freq_hz *= 1000;
> +
> + mode = FIELD_PREP(ADIS16550_SYNC_MODE_MASK, sync->sync_mode) |
> + FIELD_PREP(ADIS16550_SYNC_EN_MASK, true);
> +
> + return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> + ADIS16550_SYNC_MASK, mode);
> +}
> +
> +static int adis16550_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct adis16550 *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->info = spi_get_device_match_data(spi);
> + if (!st->info)
> + return -EINVAL;
> +
> + indio_dev->name = st->info->name;
> + indio_dev->channels = st->info->channels;
> + indio_dev->num_channels = st->info->num_channels;
> + indio_dev->available_scan_masks = adis16550_channel_masks;
> + indio_dev->info = &adis16550_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->adis.ops = &adis16550_ops;
> +
> + /*
> + * Allocate the xfer and buffer for the burst read operation. The buffer
> + * is used to store the burst data and the xfer is used to send the burst
> + * command and receive the data. On device remove the adis libraary is going
library.
> + * to free the xfer and buffer.
That is a horrible lifetime control. They should either be allocated in the library
and freed in the library or allocated and freed in the driver. Mixing that doesn't
make sense.
Can you store them somewhere else so that the ownership is never in the adis
library?
> + */
> + st->adis.xfer = kzalloc(2 * sizeof(*st->adis.xfer),
> + if (!st->adis.xfer)
> + return -ENOMEM;
> +
> + st->adis.buffer = kzalloc(ADIS16550_BURST_DATA_LEN + sizeof(u32),
> + GFP_KERNEL);
> + if (!st->adis.buffer)
> + return -ENOMEM;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret) {
> + dev_err_probe(dev, ret,
> + "Failed to get vdd regulator\n");
> + return ret;
return dev_err_probe()
and drop the brackets.
> + }
> +
> + ret = adis_init(&st->adis, indio_dev, spi, &adis16550_data);
> + if (ret)
> + return ret;
> +
> + ret = __adis_initial_startup(&st->adis);
> + if (ret)
> + return ret;
> +
> + ret = adis16550_config_sync(st);
> + if (ret)
> + return ret;
> +
> + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> + adis16550_trigger_handler);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + adis16550_debugfs_init(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id adis16550_id[] = {
> + { "adis16550", (kernel_ulong_t)&adis16550_chip_info},
> + { "adis16550w", (kernel_ulong_t)&adis16550w_chip_info},
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, adis16550_id);
> +
> +static const struct of_device_id adis16550_of_match[] = {
> + { .compatible = "adi,adis16550",
> + .data = &adis16550_chip_info },
> + { .compatible = "adi,adis16550w",
> + .data = &adis16550w_chip_info },
Under 80 chars on one line, so don't wrap these.
> + { }
> +};
