Re: [PATCH v2 3/6] iio: accel: adis16201: prepare driver to support additional parts

[Jonathan Cameron]

On Sun, 13 Sep 2026 13:53:04 +0500
Shehryar Ahmad <shehryar.amd@xxxxxxxxx> wrote:

> Introduce adis16201_chip_info to hold per chip data and adis16201_state
> to wrap struct adis. Move adis16201 to this infrastructure. This
> prepares the driver to support additional chip variants by keeping
> chip-specific parameters in adis16201_chip_info. Additionally,
> adis16201_write_raw applies mask directly on value.
> 
> Signed-off-by: Shehryar Ahmad <shehryar.amd@xxxxxxxxx>

Sashiko spotted what seems to be an interesting bug..  Seems
the adis_buffer library code relies on channel ordering matching
scan_index ordering and that isn't true in this driver.
https://sashiko.dev/#/patchset/20260913085307.13846-1-shehryar.amd%40gmail.com

There should be no negative side effects reordering the
channels so I think that would makes sense to do.

The buffered data will be in a different order but that will at least
be the order the sysfs interface claims it is in!  If you don't
mind doing that as a precursor to this series that would be great.

See the spi transaction building in adis_update_scan_modes() for
where it is going wrong.

One small review comment inline that means making one more thing const
to simplify the code a little.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adis16201.c | 78 ++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
> index 2ce5c409b..e0bf7df50 100644
> --- a/drivers/iio/accel/adis16201.c
> +++ b/drivers/iio/accel/adis16201.c
> @@ -87,6 +87,22 @@ enum adis16201_scan {
>  	ADIS16201_SCAN_TEMP,
>  };
>  
> +struct adis16201_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec *arr_channels;
> +	unsigned int incli_scale_val2;
> +	u16 write_mask_incli;
> +	u16 diag_stat_mask;
> +	unsigned int read_bits_incli;
> +	unsigned int num_channels;
As mentioned below add:
	const struct adis_data *data;
> +};
> +
> +struct adis16201_state {
> +	struct adis adis;
> +	const struct adis16201_chip_info *info;
> +	struct adis_data data;
 and drop this.
> +};

>  
>  static const struct iio_chan_spec adis16201_channels[] = {
> @@ -217,6 +232,20 @@ static const struct iio_chan_spec adis16201_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(7)
>  };
>  
> +static const struct adis16201_chip_info adis16201_chip_data = {
> +	.arr_channels = adis16201_channels,
> +	.incli_scale_val2 = 100000,
> +	.write_mask_incli = GENMASK(8, 0),
> +	.diag_stat_mask =
> +		BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
> +		BIT(ADIS16201_DIAG_STAT_FLASH_UPT_FAIL_BIT) |
> +		BIT(ADIS16201_DIAG_STAT_POWER_HIGH_BIT) |
> +		BIT(ADIS16201_DIAG_STAT_POWER_LOW_BIT),
> +	.read_bits_incli = 9,
> +	.num_channels = ARRAY_SIZE(adis16201_channels),
> +	.name = "adis16201",
> +};
> +
>  static const struct iio_info adis16201_info = {
>  	.read_raw = adis16201_read_raw,
>  	.write_raw = adis16201_write_raw,
> @@ -248,16 +277,12 @@ static const struct adis_data adis16201_data = {
>  	.timeouts = &adis16201_timeouts,
>  
>  	.status_error_msgs = adis16201_status_error_msgs,
> -	.status_error_mask = BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
> -		BIT(ADIS16201_DIAG_STAT_FLASH_UPT_FAIL_BIT) |
> -		BIT(ADIS16201_DIAG_STAT_POWER_HIGH_BIT) |
> -		BIT(ADIS16201_DIAG_STAT_POWER_LOW_BIT),

As below, leave this here. Just look to access this whole structure
via info.

>  };
>  
>  static int adis16201_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> -	struct adis *st;
> +	struct adis16201_state *st;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> @@ -266,22 +291,29 @@ static int adis16201_probe(struct spi_device *spi)
>  
>  	st = iio_priv(indio_dev);
>  
> -	indio_dev->name = spi->dev.driver->name;
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODATA;
> +
> +	indio_dev->name = st->info->name;
>  	indio_dev->info = &adis16201_info;
>  
> -	indio_dev->channels = adis16201_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> +	indio_dev->channels = st->info->arr_channels;
> +	indio_dev->num_channels = st->info->num_channels;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> +	st->data = adis16201_data;
> +	st->data.status_error_mask = st->info->diag_stat_mask;
Given we don't have too many variants, I think rather than
this copy and update pattern I'd just have multiple instances
of the structure type of adis16201_data. Then put a pointer
to the relevant one in the info structure and just pass
in to adis_init() st->info->data.
Then you won't need the diag_stat_mask element.

Costs a little more static const data but simplifies the code
and I think that is the right trade off to make.

> +
> +	ret = adis_init(&st->adis, indio_dev, spi, &st->data);
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> +	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
>  	if (ret)
>  		return ret;
>  
> -	ret = __adis_initial_startup(st);
> +	ret = __adis_initial_startup(&st->adis);
>  	if (ret)
>  		return ret;
>  
> @@ -289,14 +321,14 @@ static int adis16201_probe(struct spi_device *spi)
>  }
>  
>  static const struct of_device_id adis16201_of_match[] = {
> -	{ .compatible = "adi,adis16201" },
> +	{ .compatible = "adi,adis16201", .data = &adis16201_chip_data },
>  	{ },
>  };
>  
>  MODULE_DEVICE_TABLE(of, adis16201_of_match);
>  
>  static const struct spi_device_id adis16201_ids[] = {
> -	{ .name = "adis16201", 0 },
> +	{ .name = "adis16201", .driver_data = (kernel_ulong_t)&adis16201_chip_data },
>  	{ },
>  };
>