| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKUZ0zJ5ZCmkJOESYpbp=a6odgLyt4Qxt_0cucsHk7FCYqoumw@mail.gmail.com>
Date: Fri, 18 Apr 2025 13:38:24 -0400
From: Gabriel Shahrouzi <gshahrouzi@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: gregkh@...uxfoundation.org, lars@...afoo.de, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
Michael.Hennerich@...log.com, skhan@...uxfoundation.org,
kernelmentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] iio: accel: adis16203: Fix single-axis representation and
CALIBBIAS handling
On Fri, Apr 18, 2025 at 11:50 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 15 Apr 2025 18:26:52 -0400
> Gabriel Shahrouzi <gshahrouzi@...il.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@...il.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),
> > };
>
Powered by blists - more mailing lists