* [PATCH] iio: accel: adis16203: Fix single-axis representation and CALIBBIAS handling
@ 2025-04-15 22:26 Gabriel Shahrouzi
2025-04-18 15:50 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-15 22:26 UTC (permalink / raw)
To: gregkh, jic23, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich
Cc: gshahrouzi, skhan, kernelmentees
The ADIS16203 is a single-axis 360 degree inclinometer. The previous
driver code incorrectly represented this by defining separate X and Y
inclination channels based on the two different output format registers
(0x0C for 0-360 deg, 0x0E for +/-180 deg). This violated IIO conventions
and misrepresented the hardware's single angle output. The 'Fixme'
comment on the original Y channel definition indicated this known issue.
Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
---
Not sure to put a fixes tag here or not because the driver used to be
spread out across multiple files until it was whittled down to one file
using a common interface for similar devices.
---
drivers/staging/iio/accel/adis16203.c | 52 ++++++++++++++++-----------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
index c1c73308800c5..73288121bf0bd 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -28,11 +28,11 @@
/* Output, temperature */
#define ADIS16203_TEMP_OUT 0x0A
-/* Output, x-axis inclination */
-#define ADIS16203_XINCL_OUT 0x0C
+/* Output, 360 deg format */
+#define ADIS16203_INCL_OUT 0x0C
-/* Output, y-axis inclination */
-#define ADIS16203_YINCL_OUT 0x0E
+/* Output, +/-180 deg format */
+#define ADIS16203_INCL_180_OUT 0x0E
/* Incline null calibration */
#define ADIS16203_INCL_NULL 0x18
@@ -128,8 +128,7 @@
#define ADIS16203_ERROR_ACTIVE BIT(14)
enum adis16203_scan {
- ADIS16203_SCAN_INCLI_X,
- ADIS16203_SCAN_INCLI_Y,
+ ADIS16203_SCAN_INCLI,
ADIS16203_SCAN_SUPPLY,
ADIS16203_SCAN_AUX_ADC,
ADIS16203_SCAN_TEMP,
@@ -137,10 +136,6 @@ enum adis16203_scan {
#define DRIVER_NAME "adis16203"
-static const u8 adis16203_addresses[] = {
- [ADIS16203_SCAN_INCLI_X] = ADIS16203_INCL_NULL,
-};
-
static int adis16203_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val,
@@ -148,10 +143,15 @@ static int adis16203_write_raw(struct iio_dev *indio_dev,
long mask)
{
struct adis *st = iio_priv(indio_dev);
- /* currently only one writable parameter which keeps this simple */
- u8 addr = adis16203_addresses[chan->scan_index];
- return adis_write_reg_16(st, addr, val & 0x3FFF);
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBBIAS:
+ if (chan->scan_index != ADIS16203_SCAN_INCLI)
+ return -EINVAL;
+ return adis_write_reg_16(st, ADIS16203_INCL_NULL, val & 0x3FFF);
+ default:
+ return -EINVAL;
+ }
}
static int adis16203_read_raw(struct iio_dev *indio_dev,
@@ -161,7 +161,6 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
{
struct adis *st = iio_priv(indio_dev);
int ret;
- u8 addr;
s16 val16;
switch (mask) {
@@ -194,8 +193,9 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
*val = 25000 / -470 - 1278; /* 25 C = 1278 */
return IIO_VAL_INT;
case IIO_CHAN_INFO_CALIBBIAS:
- addr = adis16203_addresses[chan->scan_index];
- ret = adis_read_reg_16(st, addr, &val16);
+ if (chan->scan_index != ADIS16203_SCAN_INCLI)
+ return -EINVAL;
+ ret = adis_read_reg_16(st, ADIS16203_INCL_NULL, &val16);
if (ret)
return ret;
*val = sign_extend32(val16, 13);
@@ -206,13 +206,23 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
}
static const struct iio_chan_spec adis16203_channels[] = {
+ {
+ .type = IIO_INCLI,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_CALIBBIAS),
+ .address = ADIS16203_INCL_180_OUT,
+ .scan_index = ADIS16203_SCAN_INCLI,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 14,
+ .storagebits = 16,
+ .shift = 0,
+ .endianness = IIO_CPU,
+ },
+ },
ADIS_SUPPLY_CHAN(ADIS16203_SUPPLY_OUT, ADIS16203_SCAN_SUPPLY, 0, 12),
ADIS_AUX_ADC_CHAN(ADIS16203_AUX_ADC, ADIS16203_SCAN_AUX_ADC, 0, 12),
- ADIS_INCLI_CHAN(X, ADIS16203_XINCL_OUT, ADIS16203_SCAN_INCLI_X,
- BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
- /* Fixme: Not what it appears to be - see data sheet */
- ADIS_INCLI_CHAN(Y, ADIS16203_YINCL_OUT, ADIS16203_SCAN_INCLI_Y,
- 0, 0, 14),
ADIS_TEMP_CHAN(ADIS16203_TEMP_OUT, ADIS16203_SCAN_TEMP, 0, 12),
IIO_CHAN_SOFT_TIMESTAMP(5),
};
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] iio: accel: adis16203: Fix single-axis representation and CALIBBIAS handling
2025-04-15 22:26 [PATCH] iio: accel: adis16203: Fix single-axis representation and CALIBBIAS handling Gabriel Shahrouzi
@ 2025-04-18 15:50 ` Jonathan Cameron
2025-04-18 17:38 ` Gabriel Shahrouzi
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2025-04-18 15:50 UTC (permalink / raw)
To: Gabriel Shahrouzi
Cc: gregkh, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees
On Tue, 15 Apr 2025 18:26:52 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
> The ADIS16203 is a single-axis 360 degree inclinometer. The previous
> driver code incorrectly represented this by defining separate X and Y
> inclination channels based on the two different output format registers
> (0x0C for 0-360 deg, 0x0E for +/-180 deg). This violated IIO conventions
> and misrepresented the hardware's single angle output. The 'Fixme'
> comment on the original Y channel definition indicated this known issue.
>
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> ---
> Not sure to put a fixes tag here or not because the driver used to be
> spread out across multiple files until it was whittled down to one file
> using a common interface for similar devices.
No fixes tag for this one is the right choice. It is a complex bit of
ABI abuse.
> ---
> drivers/staging/iio/accel/adis16203.c | 52 ++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
> index c1c73308800c5..73288121bf0bd 100644
> --- a/drivers/staging/iio/accel/adis16203.c
> +++ b/drivers/staging/iio/accel/adis16203.c
> @@ -28,11 +28,11 @@
> /* Output, temperature */
> #define ADIS16203_TEMP_OUT 0x0A
>
> -/* Output, x-axis inclination */
> -#define ADIS16203_XINCL_OUT 0x0C
> +/* Output, 360 deg format */
> +#define ADIS16203_INCL_OUT 0x0C
>
> -/* Output, y-axis inclination */
> -#define ADIS16203_YINCL_OUT 0x0E
> +/* Output, +/-180 deg format */
> +#define ADIS16203_INCL_180_OUT 0x0E
>
> /* Incline null calibration */
> #define ADIS16203_INCL_NULL 0x18
> @@ -128,8 +128,7 @@
> #define ADIS16203_ERROR_ACTIVE BIT(14)
>
> enum adis16203_scan {
> - ADIS16203_SCAN_INCLI_X,
> - ADIS16203_SCAN_INCLI_Y,
> + ADIS16203_SCAN_INCLI,
> ADIS16203_SCAN_SUPPLY,
> ADIS16203_SCAN_AUX_ADC,
> ADIS16203_SCAN_TEMP,
> @@ -137,10 +136,6 @@ enum adis16203_scan {
>
> #define DRIVER_NAME "adis16203"
>
> -static const u8 adis16203_addresses[] = {
> - [ADIS16203_SCAN_INCLI_X] = ADIS16203_INCL_NULL,
> -};
> -
> static int adis16203_write_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int val,
> @@ -148,10 +143,15 @@ static int adis16203_write_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct adis *st = iio_priv(indio_dev);
> - /* currently only one writable parameter which keeps this simple */
> - u8 addr = adis16203_addresses[chan->scan_index];
>
> - return adis_write_reg_16(st, addr, val & 0x3FFF);
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBBIAS:
> + if (chan->scan_index != ADIS16203_SCAN_INCLI)
> + return -EINVAL;
> + return adis_write_reg_16(st, ADIS16203_INCL_NULL, val & 0x3FFF);
I would check for out of range before you get here rather than masking.
Clearly the old code just masked, but we can do better given you are refactoring
it. If an invalid setting is requested best thing is normally to just return
an error so userspace can see it was ignored.
> + default:
> + return -EINVAL;
> + }
> }
>
> static int adis16203_read_raw(struct iio_dev *indio_dev,
> @@ -161,7 +161,6 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> {
> struct adis *st = iio_priv(indio_dev);
> int ret;
> - u8 addr;
> s16 val16;
>
> switch (mask) {
> @@ -194,8 +193,9 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> *val = 25000 / -470 - 1278; /* 25 C = 1278 */
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_CALIBBIAS:
> - addr = adis16203_addresses[chan->scan_index];
> - ret = adis_read_reg_16(st, addr, &val16);
> + if (chan->scan_index != ADIS16203_SCAN_INCLI)
> + return -EINVAL;
> + ret = adis_read_reg_16(st, ADIS16203_INCL_NULL, &val16);
> if (ret)
> return ret;
> *val = sign_extend32(val16, 13);
> @@ -206,13 +206,23 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> }
>
> static const struct iio_chan_spec adis16203_channels[] = {
> + {
> + .type = IIO_INCLI,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_CALIBBIAS),
> + .address = ADIS16203_INCL_180_OUT,
> + .scan_index = ADIS16203_SCAN_INCLI,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 14,
> + .storagebits = 16,
> + .shift = 0,
No need for setting shift to 0 explicitly. It will happen anyway and
a shift of 0 is a fairly natural default.
> + .endianness = IIO_CPU,
> + },
> + },
> ADIS_SUPPLY_CHAN(ADIS16203_SUPPLY_OUT, ADIS16203_SCAN_SUPPLY, 0, 12),
> ADIS_AUX_ADC_CHAN(ADIS16203_AUX_ADC, ADIS16203_SCAN_AUX_ADC, 0, 12),
> - ADIS_INCLI_CHAN(X, ADIS16203_XINCL_OUT, ADIS16203_SCAN_INCLI_X,
> - BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> - /* Fixme: Not what it appears to be - see data sheet */
> - ADIS_INCLI_CHAN(Y, ADIS16203_YINCL_OUT, ADIS16203_SCAN_INCLI_Y,
> - 0, 0, 14),
Why the ordering change? I don't think it matters in practice, but easier to
review of we keep that ordering the same as then no need to think about it at
all!
Jonathan
> ADIS_TEMP_CHAN(ADIS16203_TEMP_OUT, ADIS16203_SCAN_TEMP, 0, 12),
> IIO_CHAN_SOFT_TIMESTAMP(5),
> };
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] iio: accel: adis16203: Fix single-axis representation and CALIBBIAS handling
2025-04-18 15:50 ` Jonathan Cameron
@ 2025-04-18 17:38 ` Gabriel Shahrouzi
0 siblings, 0 replies; 3+ messages in thread
From: Gabriel Shahrouzi @ 2025-04-18 17:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: gregkh, lars, linux-iio, linux-kernel, linux-staging,
Michael.Hennerich, skhan, kernelmentees
On Fri, Apr 18, 2025 at 11:50 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 15 Apr 2025 18:26:52 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
>
> > The ADIS16203 is a single-axis 360 degree inclinometer. The previous
> > driver code incorrectly represented this by defining separate X and Y
> > inclination channels based on the two different output format registers
> > (0x0C for 0-360 deg, 0x0E for +/-180 deg). This violated IIO conventions
> > and misrepresented the hardware's single angle output. The 'Fixme'
> > comment on the original Y channel definition indicated this known issue.
> >
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > ---
> > Not sure to put a fixes tag here or not because the driver used to be
> > spread out across multiple files until it was whittled down to one file
> > using a common interface for similar devices.
>
> No fixes tag for this one is the right choice. It is a complex bit of
> ABI abuse.
Got it.
>
> > ---
> > drivers/staging/iio/accel/adis16203.c | 52 ++++++++++++++++-----------
> > 1 file changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
> > index c1c73308800c5..73288121bf0bd 100644
> > --- a/drivers/staging/iio/accel/adis16203.c
> > +++ b/drivers/staging/iio/accel/adis16203.c
> > @@ -28,11 +28,11 @@
> > /* Output, temperature */
> > #define ADIS16203_TEMP_OUT 0x0A
> >
> > -/* Output, x-axis inclination */
> > -#define ADIS16203_XINCL_OUT 0x0C
> > +/* Output, 360 deg format */
> > +#define ADIS16203_INCL_OUT 0x0C
> >
> > -/* Output, y-axis inclination */
> > -#define ADIS16203_YINCL_OUT 0x0E
> > +/* Output, +/-180 deg format */
> > +#define ADIS16203_INCL_180_OUT 0x0E
> >
> > /* Incline null calibration */
> > #define ADIS16203_INCL_NULL 0x18
> > @@ -128,8 +128,7 @@
> > #define ADIS16203_ERROR_ACTIVE BIT(14)
> >
> > enum adis16203_scan {
> > - ADIS16203_SCAN_INCLI_X,
> > - ADIS16203_SCAN_INCLI_Y,
> > + ADIS16203_SCAN_INCLI,
> > ADIS16203_SCAN_SUPPLY,
> > ADIS16203_SCAN_AUX_ADC,
> > ADIS16203_SCAN_TEMP,
> > @@ -137,10 +136,6 @@ enum adis16203_scan {
> >
> > #define DRIVER_NAME "adis16203"
> >
> > -static const u8 adis16203_addresses[] = {
> > - [ADIS16203_SCAN_INCLI_X] = ADIS16203_INCL_NULL,
> > -};
> > -
> > static int adis16203_write_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int val,
> > @@ -148,10 +143,15 @@ static int adis16203_write_raw(struct iio_dev *indio_dev,
> > long mask)
> > {
> > struct adis *st = iio_priv(indio_dev);
> > - /* currently only one writable parameter which keeps this simple */
> > - u8 addr = adis16203_addresses[chan->scan_index];
> >
> > - return adis_write_reg_16(st, addr, val & 0x3FFF);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + if (chan->scan_index != ADIS16203_SCAN_INCLI)
> > + return -EINVAL;
> > + return adis_write_reg_16(st, ADIS16203_INCL_NULL, val & 0x3FFF);
>
> I would check for out of range before you get here rather than masking.
> Clearly the old code just masked, but we can do better given you are refactoring
> it. If an invalid setting is requested best thing is normally to just return
> an error so userspace can see it was ignored.
Got it. Added these changes to V2.
>
>
> > + default:
> > + return -EINVAL;
> > + }
> > }
> >
> > static int adis16203_read_raw(struct iio_dev *indio_dev,
> > @@ -161,7 +161,6 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> > {
> > struct adis *st = iio_priv(indio_dev);
> > int ret;
> > - u8 addr;
> > s16 val16;
> >
> > switch (mask) {
> > @@ -194,8 +193,9 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> > *val = 25000 / -470 - 1278; /* 25 C = 1278 */
> > return IIO_VAL_INT;
> > case IIO_CHAN_INFO_CALIBBIAS:
> > - addr = adis16203_addresses[chan->scan_index];
> > - ret = adis_read_reg_16(st, addr, &val16);
> > + if (chan->scan_index != ADIS16203_SCAN_INCLI)
> > + return -EINVAL;
> > + ret = adis_read_reg_16(st, ADIS16203_INCL_NULL, &val16);
> > if (ret)
> > return ret;
> > *val = sign_extend32(val16, 13);
> > @@ -206,13 +206,23 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> > }
> >
> > static const struct iio_chan_spec adis16203_channels[] = {
> > + {
> > + .type = IIO_INCLI,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_CALIBBIAS),
> > + .address = ADIS16203_INCL_180_OUT,
> > + .scan_index = ADIS16203_SCAN_INCLI,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 14,
> > + .storagebits = 16,
> > + .shift = 0,
>
> No need for setting shift to 0 explicitly. It will happen anyway and
> a shift of 0 is a fairly natural default.
Got it. Added to V2.
>
> > + .endianness = IIO_CPU,
> > + },
> > + },
> > ADIS_SUPPLY_CHAN(ADIS16203_SUPPLY_OUT, ADIS16203_SCAN_SUPPLY, 0, 12),
> > ADIS_AUX_ADC_CHAN(ADIS16203_AUX_ADC, ADIS16203_SCAN_AUX_ADC, 0, 12),
> > - ADIS_INCLI_CHAN(X, ADIS16203_XINCL_OUT, ADIS16203_SCAN_INCLI_X,
> > - BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> > - /* Fixme: Not what it appears to be - see data sheet */
> > - ADIS_INCLI_CHAN(Y, ADIS16203_YINCL_OUT, ADIS16203_SCAN_INCLI_Y,
> > - 0, 0, 14),
> Why the ordering change? I don't think it matters in practice, but easier to
> review of we keep that ordering the same as then no need to think about it at
> all!
You're right, maintaining the original order simplifies reviewing this
specific functional change. I have restored the original relative
order of the channels in v2.
My initial thought process for moving the inclinometer channel was to
place the device's primary function first in the list, but I agree
that separating functional changes (like this patch) from
organizational changes (like reordering) is better practice. Any
reordering can be proposed separately.
>
> Jonathan
>
> > ADIS_TEMP_CHAN(ADIS16203_TEMP_OUT, ADIS16203_SCAN_TEMP, 0, 12),
> > IIO_CHAN_SOFT_TIMESTAMP(5),
> > };
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-18 17:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-15 22:26 [PATCH] iio: accel: adis16203: Fix single-axis representation and CALIBBIAS handling Gabriel Shahrouzi
2025-04-18 15:50 ` Jonathan Cameron
2025-04-18 17:38 ` Gabriel Shahrouzi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox