| 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: <20250418165012.53c9bb85@jic23-huawei>
Date: Fri, 18 Apr 2025 16:50:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Gabriel Shahrouzi <gshahrouzi@...il.com>
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 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.
> ---
> 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),
> };
Powered by blists - more mailing lists