Message-ID: <20250208153711.1e0215d1@jic23-huawei>
Date: Sat, 8 Feb 2025 15:37:11 +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>, Alexandru Ardelean
 <alexandru.ardelean@...log.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, Nuno Sa
 <nuno.sa@...log.com>, Ramona Gradinariu <ramona.gradinariu@...log.com>,
 Trevor Gamblin <tgamblin@...libre.com>, David Lechner
 <dlechner@...libre.com>, Marcelo Schmitt <marcelo.schmitt@...log.com>, Paul
 Cercueil <paul@...pouillou.net>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v6 5/6] iio: imu: adis16550: add adis16550 support

On Tue, 4 Feb 2025 16:36:09 +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 compensation 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 minor comments inline given you will need to do a v7 for
the DT feedback.  Otherwise I might just have tweaked these
whilst applying.


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

> +
> +struct adis16550 {
> +	const struct adis16550_chip_info *info;
> +	struct adis adis;
> +	unsigned long clk_freq_hz;
> +	u32 sync_mode;
> +	struct spi_transfer xfer[2];
> +	u8 buffer[ADIS16550_BURST_DATA_LEN + sizeof(u32)] __aligned(IIO_DMA_MINALIGN);
> +	__be32 din[2] __aligned(IIO_DMA_MINALIGN);
> +	__be32 dout[2] __aligned(IIO_DMA_MINALIGN);

In most cases only one such marking is needed.  Devices tend not to interfere
with themselves and we tend not to be changing values from software in one of
the buffer DMA is targetting whilst the device is using the others.

So we normally just mark the first one of a set like this.  That avoids
adding a lot of padding when it is not needed.

> +};


> +
> +static int adis16550_set_gyro_filter_freq(struct adis16550 *st, int freq_hz)
> +{
> +	u8 en = 0;
> +
> +	if (freq_hz)
> +		en = 1;
Could save a line or two without greatly affecting readability.
Same thing applies in at least one other place.

	u8 en = freq_hz ? 1 : 0;


> +
> +	return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> +				  ADIS16550_GYRO_FIR_EN_MASK,
> +				  FIELD_PREP(ADIS16550_GYRO_FIR_EN_MASK, en));
> +}

> +
> +static irqreturn_t adis16550_trigger_handler(int irq, void *p)
> +{

> +	memcpy(data, &buffer[3], (ADIS16550_SCAN_ACCEL_Z -
> +						ADIS16550_SCAN_GYRO_X + 2) *
> +						sizeof(__be32));
Strange alignment.
	memcpy(data, &buffer[3],
	       (ADIS16550_SCAN_ACCEL_Z - ADIS16550_SCAN_GYRO_X + 2) *
	       sizeof(__be32));

Is about the best I could come up with.
Good to add a comment on why the + 2 as well.

> +	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_probe(struct spi_device *spi)
> +{
> +	u16 burst_length = ADIS16550_BURST_DATA_LEN;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct adis16550 *st;
> +	struct adis *adis;
> +	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;
> +	adis = &st->adis;
> +	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;
> +	st->xfer[0].tx_buf = st->buffer + burst_length;
> +	st->xfer[0].len = 4;
> +	st->xfer[0].cs_change = 1;
> +	st->xfer[0].delay.value = 8;
> +	st->xfer[0].delay.unit = SPI_DELAY_UNIT_USECS;
> +	st->xfer[1].rx_buf = st->buffer;
> +	st->xfer[1].len = burst_length;
> +
> +	spi_message_init_with_transfers(&adis->msg, st->xfer, 2);
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			      "Failed to get vdd regulator\n");

Totally trivial but preference is to align after ( unless the line ends
up too long. In those cases it is fine to indent just one tab more than
the line above.

Here it actually fits on one line anyway as it's exactly 80 chars.

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

Powered by blists - more mailing lists