From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Shehryar Ahmad <shehryar.amd@gmail.com>
Cc: jic23@kernel.org, nuno.sa@analog.com,
Michael.Hennerich@analog.com, dlechner@baylibre.com,
andy@kernel.org, gregkh@linuxfoundation.org, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org, linux@analog.com,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-staging@lists.linux.dev, devicetree@vger.kernel.org,
marcelo.schmitt1@gmail.com, danascape@gmail.com
Subject: Re: [PATCH 1/3] iio: accel: adis16201: merge adis16203 support from staging
Date: Tue, 1 Sep 2026 11:12:10 +0300 [thread overview]
Message-ID: <apaI2vkeBhyx1pQd@ashevche-desk.local> (raw)
In-Reply-To: <20260831184201.34416-2-shehryar.amd@gmail.com>
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
next prev parent reply other threads:[~2026-09-01 8:12 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-31 18:41 [PATCH 0/3] iio: accel: merge adis16203 into mainline adis16201 and remove from staging Shehryar Ahmad
2026-08-31 18:41 ` [PATCH 1/3] iio: accel: adis16201: merge adis16203 support " Shehryar Ahmad
2026-08-31 21:00 ` sashiko-bot
2026-09-01 8:12 ` Andy Shevchenko [this message]
2026-08-31 18:42 ` [PATCH 2/3] staging: iio: accel: remove adis16203, merged into mainline adis16201 driver Shehryar Ahmad
2026-08-31 21:13 ` sashiko-bot
2026-09-01 8:13 ` Andy Shevchenko
2026-08-31 18:42 ` [PATCH 3/3] dt-bindings: iio: accel: adi,adis16201: add adis16203 compatible Shehryar Ahmad
2026-09-01 18:05 ` Conor Dooley
2026-09-01 7:50 ` [PATCH 0/3] iio: accel: merge adis16203 into mainline adis16201 and remove from staging Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=apaI2vkeBhyx1pQd@ashevche-desk.local \
--to=andriy.shevchenko@intel.com \
--cc=Michael.Hennerich@analog.com \
--cc=andy@kernel.org \
--cc=conor+dt@kernel.org \
--cc=danascape@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=gregkh@linuxfoundation.org \
--cc=jic23@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux@analog.com \
--cc=marcelo.schmitt1@gmail.com \
--cc=nuno.sa@analog.com \
--cc=robh@kernel.org \
--cc=shehryar.amd@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox