Hi Robert,
Some comments from my side...
On Mon, 2024-12-16 at 16:48 +0200, Robert Budai 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>
> ---
>
> v3:
> - freed xbuf
> - fixed code styling related issues
> - removed the sensor type enum and used separate structures for providing chip
> info data
>
>
...
> +
> +static irqreturn_t adis16550_trigger_handler(int irq, void *p)
> +{
> + u32 crc;
> + u16 dummy;
> + bool valid;
> + int ret, i = 0;
> + u8 data_offset = 4;
> + 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;
> +
> + 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;
> + }
> +
> + if (*indio_dev->active_scan_mask & BIT(ADIS16550_SCAN_GYRO_X)) {
> + memcpy(&data[i], &buffer[data_offset],
> (ADIS16550_SCAN_ACCEL_Z -
> + ADIS16550_SCAN_GYRO_X + 1) *
> + sizeof(__be32));
> + i += ADIS16550_SCAN_ACCEL_Z - ADIS16550_SCAN_GYRO_X + 1;
> + }
> + if (*indio_dev->active_scan_mask & BIT(ADIS16550_SCAN_TEMP))
> + data[i++] = buffer[data_offset - 1];
> + if (*indio_dev->active_scan_mask & BIT(ADIS16550_SCAN_DELTANG_X))
> + memcpy(&data[i], &buffer[data_offset],
> + (ADIS16550_SCAN_DELTVEL_Z - ADIS16550_SCAN_DELTANG_X +
> 1) *
> + sizeof(__be32));
>
Can't we just reorder the channels in adis16550_channels so that we have a plain
memcpy()? Like having the temperature before the accel + gyro or the delta
stuff? Then you should only need to care about the initial offset (to account
for DATA_CNT and STATUS). Right?
> + 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),
> +};
> +
> +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;
> +
> + if (adis->xfer)
> + memset(adis->xfer, 0, 2 * sizeof(*adis->xfer));
> + if (adis->buffer)
> + memset(adis->buffer, 0, burst_length + sizeof(u32));
>
The above checks should be removed. Likely from a previous version...
> + 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_reset(struct adis *adis)
> +{
> + return __adis_write_reg_16(adis, ADIS16550_REG_COMMAND, BIT(15));
> +}
> +
> +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;
> +
> + if (!device_property_present(dev, "adi,sync-mode")) {
> + st->clk_freq_hz = st->info->int_clk * 1000;
> + return 0;
> + }
>
I would do it in another way...
ret = device_property_read_u32(dev, "adi,sync-mode", &sync_mode);
if (ret) {
st->clk_freq_hz = st->info->int_clk * 1000;
return 0;
}
/* then proceed */
> + ret = device_property_read_u32(dev, "adi,sync-mode", &sync_mode);
> + if (ret)
> + 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) {
> + 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 const struct iio_info adis16550_info = {
> + .read_raw = &adis16550_read_raw,
> + .write_raw = &adis16550_write_raw,
> + .update_scan_mode = adis16550_update_scan_mode,
> + .debugfs_reg_access = adis_debugfs_reg_access,
> +};
> +
> +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
> +};
> +
> +static const char * const adis16550_status_error_msgs[] = {
> + [ADIS16550_STATUS_CRC_CODE] = "Code CRC Error",
> + [ADIS16550_STATUS_CRC_CONFIG] = "Configuration/Calibration CRC
> Error",
> + [ADIS16550_STATUS_FLASH_UPDATE] = "Flash Update Error",
> + [ADIS16550_STATUS_INERIAL] = "Overrange for Inertial Signals",
> + [ADIS16550_STATUS_SENSOR] = "Sensor failure",
> + [ADIS16550_STATUS_TEMPERATURE] = "Temperature Error",
> + [ADIS16550_STATUS_SPI] = "SPI Communication Error",
> + [ADIS16550_STATUS_PROCESSING] = "Processing Overrun Error",
> + [ADIS16550_STATUS_POWER] = "Power Supply Failure",
> + [ADIS16550_STATUS_BOOT] = "Boot Memory Failure",
> + [ADIS16550_STATUS_WATCHDOG] = "Watchdog timer flag",
> + [ADIS16550_STATUS_REGULATOR] = "Internal Regulator Error",
> + [ADIS16550_STATUS_SENSOR_SUPPLY] = "Internal Sensor Supply Error.",
> + [ADIS16550_STATUS_CPU_SUPPLY] = "Internal Processor Supply Error.",
> + [ADIS16550_STATUS_5V_SUPPLY] = "External 5V Supply Error",
> +};
> +
> +static const struct adis_timeout adis16550_timeouts = {
> + .reset_ms = 1000,
> + .sw_reset_ms = 1000,
> + .self_test_ms = 1000,
> +};
> +
> +static const struct adis_data adis16550_data = {
> + .diag_stat_reg = ADIS16550_REG_STATUS,
> + .diag_stat_size = 4,
> + .prod_id_reg = ADIS16550_REG_PROD_ID,
> + .prod_id = 16550,
> + .self_test_mask = BIT(1),
> + .self_test_reg = ADIS16550_REG_COMMAND,
> + .cs_change_delay = 5,
> + .unmasked_drdy = true,
> + .status_error_msgs = adis16550_status_error_msgs,
> + .status_error_mask = BIT(ADIS16550_STATUS_CRC_CODE) |
> + BIT(ADIS16550_STATUS_CRC_CONFIG) |
> + BIT(ADIS16550_STATUS_FLASH_UPDATE) |
> + BIT(ADIS16550_STATUS_INERIAL) |
> + BIT(ADIS16550_STATUS_SENSOR) |
> + BIT(ADIS16550_STATUS_TEMPERATURE) |
> + BIT(ADIS16550_STATUS_SPI) |
> + BIT(ADIS16550_STATUS_PROCESSING) |
> + BIT(ADIS16550_STATUS_POWER) |
> + BIT(ADIS16550_STATUS_BOOT) |
> + BIT(ADIS16550_STATUS_WATCHDOG) |
> + BIT(ADIS16550_STATUS_REGULATOR) |
> + BIT(ADIS16550_STATUS_SENSOR_SUPPLY) |
> + BIT(ADIS16550_STATUS_CPU_SUPPLY) |
> + BIT(ADIS16550_STATUS_5V_SUPPLY) |
> + BIT(ADIS16550_STATUS_CRC_CODE),
> + .timeouts = &adis16550_timeouts,
> +};
> +
> +static const struct adis_ops adis16550_ops = {
> + .write = adis16550_spi_write,
> + .read = adis16550_spi_read,
> + .reset = adis16550_reset,
> +};
> +
> +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;
> +
> + struct spi_transfer *xfer_tmp = kcalloc(2, sizeof(*st->adis.xfer),
> GFP_KERNEL);
> +
> + if (!xfer_tmp)
> + return -ENOMEM;
> +
> + void *buffer_tmp = kzalloc(ADIS16550_BURST_DATA_LEN + sizeof(u32),
> + GFP_KERNEL);
I'm bit a puzzeled... Why do we have it like this? I guess we are missing
__free(kfree) in here? But I would not go with this trouble in here. Why not
devm_kzalloc()? should be safe to be used in here...
> + if (!buffer_tmp) {
> + kfree(st->adis.xfer);
> + return -ENOMEM;
> + }
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret) {
> + dev_err_probe(dev, ret,
> + "Failed to get vdd regulator\n");
> + goto free_on_error;
> + }
> +
> + ret = adis_init(&st->adis, indio_dev, spi, &adis16550_data);
> + if (ret)
> + goto free_on_error;
> +
> + ret = __adis_initial_startup(&st->adis);
> + if (ret)
> + goto free_on_error;
> +
> + ret = adis16550_config_sync(st);
> + if (ret)
> + goto free_on_error;
> +
> + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> + adis16550_trigger_handler);
> + if (ret)
> + goto free_on_error;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + goto free_on_error;
> +
> + adis16550_debugfs_init(indio_dev);
> +
> + st->adis.xfer = no_free_ptr(xfer_tmp);
> + st->adis.buffer = no_free_ptr(buffer_tmp);
> +
> + return 0;
> +
> +free_on_error:
> + kfree(xfer_tmp);
> + kfree(buffer_tmp);
>
The above should not be needed...
> + return ret;
> +}
> +
> +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 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, adis16550_of_match);
> +
> +static struct spi_driver adis16550_driver = {
> + .driver = {
> + .name = "adis16550",
> + .of_match_table = adis16550_of_match,
> + },
> + .probe = adis16550_probe,
> + .id_table = adis16550_id,
> +};
> +module_spi_driver(adis16550_driver);
> +
> +MODULE_AUTHOR("Nuno Sa <nuno.sa@xxxxxxxxxx>");
> +MODULE_AUTHOR("Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx>");
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices ADIS16550 IMU driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_ADISLIB);
