Re: [PATCH v3 6/7] iio: imu: adis16550: add adis16550 support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Dec 2024 16:48:12 +0200
Robert Budai <robert.budai@analog.com> 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@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Robert Budai <robert.budai@analog.com>
> ---
> 
> v3:
> - freed xbuf
> - fixed code styling related issues
> - removed the sensor type enum and used separate structures for providing chip info data

Hi

A few comments inline to add to what Nuno noted.

Thanks

Jonathan


>  adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_trigger.o
> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> new file mode 100644
> index 000000000000..6e9d98dee0d8
> --- /dev/null
> +++ b/drivers/iio/imu/adis16550.c




> +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)
How would we be getting here if these weren't set?  Should be fine to
assume they are.

> +		memset(adis->xfer, 0, 2 * sizeof(*adis->xfer));
> +	if (adis->buffer)
> +		memset(adis->buffer, 0, burst_length + sizeof(u32));
> +
> +	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_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);
> +	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);
I guess this was me explaining something badly.
You need to look at all the infrastructure in cleanup.h.
This only makes sense if you are using auto freeing of the variables
That is
	void *buffer_tmp __free(kfree) =
		kzalloc(ADIS16550_BURST_DATA_LEN + sizeof(u32), GFP_KERNEL);
etc
Then get rid of the kfree below as they will be freed on exiting scope if
no_free_ptr() hasn't been called on them (which sets the pointer to NULL).
Then you can avoid the goto's.

However as Nuno pointed out, devm_kzalloc() may well be a better choice.
We need alignement for DMA, but dma_kzalloc does guarantee that so it is all fine.

That would also fix actually freeing these on driver remove which is currently
missing.


> +
> +	return 0;
> +
> +free_on_error:
> +	kfree(xfer_tmp);
> +	kfree(buffer_tmp);
> +
> +	return ret;
> +}