Message-ID: <b05c905cd003b6107b700628f451eb2ee42342ea.camel@gmail.com>
Date: Tue, 17 Dec 2024 11:29:21 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Robert Budai <robert.budai@...log.com>, Lars-Peter Clausen
 <lars@...afoo.de>,  Michael Hennerich <Michael.Hennerich@...log.com>, Nuno
 Sa <nuno.sa@...log.com>, Ramona Gradinariu	 <ramona.gradinariu@...log.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,  Jonathan Cameron	
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski	
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet	
 <corbet@....net>, Shen Jianping <Jianping.Shen@...bosch.com>, Alex Lanzano	
 <lanzano.alex@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Cc: robi_budai@...oo.com
Subject: Re: [PATCH v3 6/7] iio: imu: adis16550: add adis16550 support

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@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Nuno Sá <nuno.sa@...log.com>
> Signed-off-by: Robert Budai <robert.budai@...log.com>
> ---
> 
> 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@...log.com>");
> +MODULE_AUTHOR("Ramona Gradinariu <ramona.gradinariu@...log.com>");
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices ADIS16550 IMU driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_ADISLIB);

Powered by blists - more mailing lists