Message-ID: <20241130173156.4e260944@jic23-huawei>
Date: Sat, 30 Nov 2024 17:31:56 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Robert Budai <robert.budai@...log.com>
Cc: 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>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Jonathan Corbet <corbet@....net>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 6/7] iio: imu: adis16550: add adis16550 support

On Mon, 25 Nov 2024 15:35:13 +0200
Robert Budai <robert.budai@...log.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@...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>

A few additional comments below.

Thanks,

Jonathan

> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> new file mode 100644
> index 000000000000..db0e9dcadca2
> --- /dev/null
> +++ b/drivers/iio/imu/adis16550.c
> @@ -0,0 +1,1203 @@

> +
> +static int adis16550_set_freq_hz(struct adis16550 *st, u32 freq_hz)
> +{
> +	u16 dec;
> +	int ret;
> +	u32 sample_rate = st->clk_freq_hz;
> +	/*
> +	 * The optimal sample rate for the supported IMUs is between
> +	 * int_clk - 1000 and int_clk + 500.
> +	 */
> +	u32 max_sample_rate = st->info->int_clk * 1000 + 500000;
> +	u32 min_sample_rate = st->info->int_clk * 1000 - 1000000;
> +
> +	if (!freq_hz)
> +		return -EINVAL;
> +
> +	adis_dev_auto_lock(&st->adis);
> +
> +	if (st->sync_mode == ADIS16550_SYNC_MODE_SCALED) {
> +		unsigned long scaled_rate = lcm(st->clk_freq_hz, freq_hz);
> +		int sync_scale;
> +
> +		if (scaled_rate > max_sample_rate)
> +			scaled_rate = max_sample_rate / st->clk_freq_hz * st->clk_freq_hz;
> +		else
> +			scaled_rate = max_sample_rate / scaled_rate * scaled_rate;
> +
> +		if (scaled_rate < min_sample_rate)
> +			scaled_rate = roundup(min_sample_rate, st->clk_freq_hz);
> +
> +		sync_scale = scaled_rate / st->clk_freq_hz;
> +		ret = __adis_write_reg_16(&st->adis, ADIS16550_REG_SYNC_SCALE,
> +					  sync_scale);
> +		if (ret)
> +			return ret;
> +
> +		sample_rate = scaled_rate;
> +	}
> +
> +	dec = DIV_ROUND_CLOSEST(sample_rate, freq_hz);
> +
> +	if (dec)
> +		dec--;
> +
> +	dec = min(dec, st->info->max_dec);
> +
> +	ret = __adis_write_reg_16(&st->adis, ADIS16550_REG_DEC_RATE, dec);
> +
> +	return ret;

return __adis_write ..

> +}

> +
> +static int adis16550_set_gyro_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_GYRO_FIR_EN_MASK,
> +				  (u32)FIELD_PREP(ADIS16550_GYRO_FIR_EN_MASK, en));
> +}
> +
> +enum adis16550_variants {
> +	ADIS16550,
> +	ADIS16550W
These enums are usually a sign that something is not going to extend well.
Code should never need to match on such an enum value, but instead using 
structures with all the information this conveys as flags and callbacks etc.

> +};

> +
> +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 -
I would break the line after offset],
> +							ADIS16550_SCAN_DELTANG_X + 1) *
> +							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 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),
> +	0,
This 0 is effectively a terminating entry. So no need for trailing 0 as we should
never add anything after this.

> +};

> +
> +static inline void void_cleaner(void *v) {kfree(v); }
> +
> +DEFINE_FREE(void, void *, if (_T) void_cleaner(_T))
If this made sense why not

__free(kfree)?  Only difference is that it will not call
the free if ERR_PTR() which is more general than what you have
here and does your usecase no harm.

Having __free(void) is not useful.


> +
> +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 = &adis16550_chip_info_ip;
> +	if (!st->info)
> +		return -EINVAL;
> +
> +	indio_dev->name = spi_get_device_id(spi)->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;
> +
> +	st->adis.xfer = kcalloc(2, sizeof(*st->adis.xfer), GFP_KERNEL);
> +	if (!st->adis.xfer)
> +		return -ENOMEM;
> +
> +	void *buffer_tmp __free(void) = kzalloc(ADIS16550_BURST_DATA_LEN + sizeof(u32),
> +				  GFP_KERNEL);

For such a short lived thing I'm not immediately seeing why __free is useful.


> +	if (!buffer_tmp) {
> +		kfree(st->adis.xfer);
> +		return -ENOMEM;
> +	}
> +	st->adis.buffer = no_free_ptr(buffer_tmp);
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get vdd regulator\n");
> +
> +	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", ADIS16550 },
> +	{ "adis16550w", ADIS16550W },
I've no idea what the difference between these is.  Assuming there is one that matters
to software then use a per type structure, not an enum.  That way as you add
lots of additional device support over time it is easy to specify new combinations of
features as data, not code

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adis16550_id);
> +
> +static const struct of_device_id adis16550_of_match[] = {
> +	{ .compatible = "adi,adis16550" },
> +	{ .compatible = "adi,adis16550w" },

Should have data as the pointers to chip_info type structs as suggested above.

> +	{ }
> +};
> +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