| 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: <20250421125357.571c429e@jic23-huawei>
Date: Mon, 21 Apr 2025 12:53:57 +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,
linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH v2] staging: iio: accel: adis16203: Fix single-axis
representation
On Fri, 18 Apr 2025 13:33:12 -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>
Hi. A few comments inline.
Mostly that this is doing several unrelated things now.
I missed that in v1 I guess. Sorry about that!
Jonathan
> ---
> Changes in v2:
> - Check write value range in adis16203_write_raw.
> - Remove 0x3FFF mask in adis16203_write_raw.
> - Remove explicit shift = 0 in channel definition.
> - Keep original channel ordering.
> - Add staging prefix to subject line.
> ---
> drivers/staging/iio/accel/adis16203.c | 53 ++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
> index c1c73308800c5..620e0b96d3b22 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,17 @@ 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)
Why is this change related to dropping of channels?
> + return -EINVAL;
> + if (val < -BIT(13) || val >= BIT(13))
Or this one?
Seems to me these should be in a separate patch.
> + return -EINVAL;
> + return adis_write_reg_16(st, ADIS16203_INCL_NULL, val);
> + default:
> + return -EINVAL;
> + }
> }
>
> static int adis16203_read_raw(struct iio_dev *indio_dev,
> @@ -161,7 +163,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 +195,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;
This looks to be defensive coding. Is calibbias registered for any other
channels? If not this can't get called anyway.
I don't mind this sort of defensive check if it adds some level
of readability to the code by making it clear what it is for, but the
fact it uses the INCL_NULL kind of makes that obvious anyway.
> + ret = adis_read_reg_16(st, ADIS16203_INCL_NULL, &val16);
> if (ret)
> return ret;
> *val = sign_extend32(val16, 13);
> @@ -208,11 +210,20 @@ static int adis16203_read_raw(struct iio_dev *indio_dev,
> static const struct iio_chan_spec adis16203_channels[] = {
> 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),
> + {
> + .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,
> + .endianness = IIO_CPU,
> + },
> + },
> ADIS_TEMP_CHAN(ADIS16203_TEMP_OUT, ADIS16203_SCAN_TEMP, 0, 12),
> IIO_CHAN_SOFT_TIMESTAMP(5),
> };
Powered by blists - more mailing lists