Re: [PATCH 1/3] iio: accel: adis16201: merge adis16203 support from staging
From: Andy Shevchenko
Date: Tue Sep 01 2026 - 04:12:59 EST
On Mon, Aug 31, 2026 at 11:41:59PM +0500, Shehryar Ahmad wrote:
> Merge the ADIS16203 360 degree inclinometer driver from staging to
> mainline ADIS16201 driver. Register addresses and external SPI interface
> are identical between both. Some things that differ like write mask, and
> calibbias bit width are handled by per chip struct differ_info, handled
> by of_device_id match data in probe, following the same pattern used in
> adis16475 which selects per-chip data directly via of_device_id.data and
> spi_get_device_match_data(), adis16480 does similar but via a different
> mechanism (index into an array via the SPI ID table).
>
> Channels arrays are kept separate to avoid dropping const and kmemdup
> would be extra failure point if memory allocation fails and is memory
> inefficient.
>
> GENMASK is still same instead of range check to keep adis16201 behaviour
"GENMASK():s are..." ?
> unchanged
Missing period.
...
Yes, it's a good change, but please split it to 3+ patches. See below.
...
> - * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
> + * Analog Devices ADIS16201 Inclinometer/Accelerometer and ADIS16203 Inclinometer Driver
This needs to be written in the more generic form. Also the same text should be
in MODULE_DESCRIPTION() and in the Summary of the Kconfig help text (which is
missing to update). This will be part of the patch that adds the new HW.
...
> * Copyright 2010 Analog Devices Inc.
This probably needs to be merged as well. I haven't checked the actual lines in
the other driver, though.
...
> -#define ADIS16201_STARTUP_DELAY_MS 220
> -#define ADIS16201_FLASH_CNT 0x00
> +#define ADIS16201_STARTUP_DELAY_MS 220
> +#define ADIS16201_FLASH_CNT 0x00
>
> /* Data Output Register Information */
> -#define ADIS16201_SUPPLY_OUT_REG 0x02
> -#define ADIS16201_XACCL_OUT_REG 0x04
> -#define ADIS16201_YACCL_OUT_REG 0x06
> -#define ADIS16201_AUX_ADC_REG 0x08
> -#define ADIS16201_TEMP_OUT_REG 0x0A
> -#define ADIS16201_XINCL_OUT_REG 0x0C
> -#define ADIS16201_YINCL_OUT_REG 0x0E
> +#define ADIS16201_SUPPLY_OUT_REG 0x02
> +#define ADIS16201_XACCL_OUT_REG 0x04
> +#define ADIS16201_YACCL_OUT_REG 0x06
> +#define ADIS16201_AUX_ADC_REG 0x08
> +#define ADIS16201_TEMP_OUT_REG 0x0A
> +#define ADIS16201_INCL_OUT_REG 0x0C
> +#define ADIS16201_YINCL_OUT_REG 0x0E
>
> /* Calibration Register Definition */
> -#define ADIS16201_XACCL_OFFS_REG 0x10
> -#define ADIS16201_YACCL_OFFS_REG 0x12
> -#define ADIS16201_XACCL_SCALE_REG 0x14
> -#define ADIS16201_YACCL_SCALE_REG 0x16
> -#define ADIS16201_XINCL_OFFS_REG 0x18
> -#define ADIS16201_YINCL_OFFS_REG 0x1A
> -#define ADIS16201_XINCL_SCALE_REG 0x1C
> -#define ADIS16201_YINCL_SCALE_REG 0x1E
> +#define ADIS16201_XACCL_OFFS_REG 0x10
> +#define ADIS16201_YACCL_OFFS_REG 0x12
> +#define ADIS16201_XACCL_SCALE_REG 0x14
> +#define ADIS16201_YACCL_SCALE_REG 0x16
> +#define ADIS16201_INCL_OFFS_REG 0x18
> +#define ADIS16201_YINCL_OFFS_REG 0x1A
> +#define ADIS16201_XINCL_SCALE_REG 0x1C
> +#define ADIS16201_YINCL_SCALE_REG 0x1E
>
> /* Alarm Register Definition */
> -#define ADIS16201_ALM_MAG1_REG 0x20
> -#define ADIS16201_ALM_MAG2_REG 0x22
> -#define ADIS16201_ALM_SMPL1_REG 0x24
> -#define ADIS16201_ALM_SMPL2_REG 0x26
> -#define ADIS16201_ALM_CTRL_REG 0x28
> -
> -#define ADIS16201_AUX_DAC_REG 0x30
> -#define ADIS16201_GPIO_CTRL_REG 0x32
> -#define ADIS16201_SMPL_PRD_REG 0x36
> +#define ADIS16201_ALM_MAG1_REG 0x20
> +#define ADIS16201_ALM_MAG2_REG 0x22
> +#define ADIS16201_ALM_SMPL1_REG 0x24
> +#define ADIS16201_ALM_SMPL2_REG 0x26
> +#define ADIS16201_ALM_CTRL_REG 0x28
> +
> +#define ADIS16201_AUX_DAC_REG 0x30
> +#define ADIS16201_GPIO_CTRL_REG 0x32
> +#define ADIS16201_SMPL_PRD_REG 0x36
> +
> /* Operation, filter configuration */
> -#define ADIS16201_AVG_CNT_REG 0x38
> -#define ADIS16201_SLP_CNT_REG 0x3A
> +#define ADIS16201_AVG_CNT_REG 0x38
> +#define ADIS16201_SLP_CNT_REG 0x3A
Make indentation changes either separate or do not touch these lines at all,
it's very hard to check if there were any real changes (in the offsets
and/or names).
...
> enum adis16201_scan {
> - ADIS16201_SCAN_ACC_X,
> - ADIS16201_SCAN_ACC_Y,
> - ADIS16201_SCAN_INCLI_X,
> - ADIS16201_SCAN_INCLI_Y,
> ADIS16201_SCAN_SUPPLY,
> - ADIS16201_SCAN_AUX_ADC,
> ADIS16201_SCAN_TEMP,
> + ADIS16201_SCAN_AUX_ADC,
> + ADIS16201_SCAN_INCLI,
> + ADIS16201_SCAN_INCLI_Y,
> + ADIS16201_SCAN_ACC_X,
> + ADIS16201_SCAN_ACC_Y,
> };
Why do you need to reshuffle the enum? This needs a good justification and
explanation why it's not a problem for the existing support.
...
> +static const struct iio_chan_spec adis16201_channels[] = {
> + ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY,
> + 0, 12),
> + ADIS_TEMP_CHAN(ADIS16201_TEMP_OUT_REG, ADIS16201_SCAN_TEMP, 0, 12),
> + ADIS_ACCEL_CHAN(X, ADIS16201_XACCL_OUT_REG, ADIS16201_SCAN_ACC_X,
> + BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> + ADIS_ACCEL_CHAN(Y, ADIS16201_YACCL_OUT_REG, ADIS16201_SCAN_ACC_Y,
> + BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> + ADIS_AUX_ADC_CHAN(ADIS16201_AUX_ADC_REG, ADIS16201_SCAN_AUX_ADC, 0, 12),
> + ADIS_INCLI_CHAN(X, ADIS16201_INCL_OUT_REG, ADIS16201_SCAN_INCLI,
> + BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 12),
> + ADIS_INCLI_CHAN(Y, ADIS16201_YINCL_OUT_REG, ADIS16201_SCAN_INCLI_Y,
> + BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 12),
> + IIO_CHAN_SOFT_TIMESTAMP(7)
Keep trailing commas in non-termination entries (yes, while it looks like
the end of the list, strictly speaking it's not). Same for the rest of the
similar cases.
> +};
...
> +struct differ_info {
> + u16 write_mask_incli;
> + unsigned int incli_scale_val2;
> + unsigned int read_bits_incli;
> + const struct iio_chan_spec *arr_chans;
> + u16 diag_stat_mask;
> + unsigned int num_chans;
Always run `pahole` when do some data type changes or introduction. This one
may have unneeded gaps.
> +};
...
> +static const struct differ_info adis16201_diff = {
> + .write_mask_incli = GENMASK(8, 0),
> + .incli_scale_val2 = 100000,
Would it make sense to use multipliers from units.h or elsewhere?
> + .read_bits_incli = 9,
> + .arr_chans = adis16201_channels,
> + .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),
Make the BIT() indented in the same way, I recommend to have
.diag_stat_mask =
BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
> + .num_chans = ARRAY_SIZE(adis16201_channels)
> +};
...
> +static const struct differ_info adis16203_diff = {
Same comments as per above.
> +};
...
> +static int adis16201_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long mask)
There is room for all three on the one line.
> +{
> + struct adis16201_state *st = iio_priv(indio_dev);
> + int m;
Why signed?
> + if (mask != IIO_CHAN_INFO_CALIBBIAS)
> + return -EINVAL;
> +
> + switch (chan->type) {
> + case IIO_ACCEL:
> + m = GENMASK(11, 0);
> + break;
> + case IIO_INCLI:
> + m = st->info->write_mask_incli;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return adis_write_reg_16(&st->adis, adis16201_addresses[chan->scan_index],
> + val & m);
Apply mask to the valu directly in the switch case, drop 'm'.
With
struct adis *adis = ...;
and the above suggestion this becomes
return adis_write_reg_16(adis, adis16201_addresses[chan->scan_index], val);
> +}
...
> static int adis16201_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> - int *val, int *val2,
> - long mask)
> + int *val,
> + int *val2, long mask)
This is stray change. Why?!
> {
...
> + case IIO_INCLI:
> + *val = 0;
> + *val2 = st->info->incli_scale_val2;
> + return IIO_VAL_INT_PLUS_MICRO;
> - case IIO_INCLI:
> - *val = 0;
> - *val2 = 100000;
> - return IIO_VAL_INT_PLUS_MICRO;
> default:
> return -EINVAL;
Same Q.
...
Now, looking at the above I see an additional preparatory patch, id est
the new data type (struct adis16201_state) that wraps existing one. In
the following changes you may extend it as required.
...
> -static int adis16201_write_raw(struct iio_dev *indio_dev,
> - struct iio_chan_spec const *chan,
> - int val,
> - int val2,
> - long mask)
> -{
> - struct adis *st = iio_priv(indio_dev);
> - int m;
> -
> - if (mask != IIO_CHAN_INFO_CALIBBIAS)
> - return -EINVAL;
> -
> - switch (chan->type) {
> - case IIO_ACCEL:
> - m = GENMASK(11, 0);
> - break;
> - case IIO_INCLI:
> - m = GENMASK(8, 0);
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - return adis_write_reg_16(st, adis16201_addresses[chan->scan_index],
> - val & m);
> -}
Ah, this code is moved up and being modified. So, split this to a few patches:
- move this function up (no changes, explain why you will need it up)
- modify as I suggested above (apply mast directly to the value)
- add your changes in the final (big) patch
...
> struct iio_dev *indio_dev;
> - struct adis *st;
> + struct adis16201_state *st;
> + struct adis_data data = adis16201_data;
> int ret;
> - indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(struct adis16201_state));
Why?!
> if (!indio_dev)
> return -ENOMEM;
...
> + st->info = spi_get_device_match_data(spi);
This should be checked against NULL and ENODATA to be returned otherwise.
Also switching to chip_info should go separately.
...
> indio_dev->name = spi->dev.driver->name;
This needs to be part of the chip_info structure.
...
> +static const struct of_device_id adis16201_of_match[] = {
> + { .compatible = "adi,adis16201", .data = &adis16201_diff },
> + { .compatible = "adi,adis16203", .data = &adis16203_diff },
> + { }
> +};
Missing MODULE_DEVICE_TABLE() (IIRC the macro name). Also this needs to be
added in a separate patch.
...
> static struct spi_driver adis16201_driver = {
> .driver = {
> - .name = "adis16201",
> + .name = "adis16201_adis16203",
> + .of_match_table = adis16201_of_match,
> },
> .probe = adis16201_probe,
> };
> module_spi_driver(adis16201_driver);
...
> MODULE_ALIAS("spi:adis16201");
> +MODULE_ALIAS("spi:adis16203");
No, make proper SPI ID table to be present instead (in a separate patch)
...
Overall I counted something like 7 patches this one has to be split into.
--
With Best Regards,
Andy Shevchenko