| 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: <25680568c9477e8db5cf533ff2a9f117ed8de440.camel@gmail.com>
Date: Thu, 20 Jul 2023 10:58:51 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Ramona Bolboaca <ramona.bolboaca@...log.com>, jic23@...nel.org,
nuno.sa@...log.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] iio: imu: adis16475.c: Add delta angle and delta
velocity channels
On Wed, 2023-07-19 at 15:31 +0300, Ramona Bolboaca wrote:
> Add support for delta angle and delta velocity raw and buffer
> readings to adis16475 driver.
>
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@...log.com>
> ---
> drivers/iio/imu/adis16475.c | 210 +++++++++++++++++++++++++++++++-----
> 1 file changed, 184 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 17275a53ca2c..e92b42974ee6 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -31,6 +31,12 @@
> #define ADIS16475_REG_Y_ACCEL_L 0x14
> #define ADIS16475_REG_Z_ACCEL_L 0x18
> #define ADIS16475_REG_TEMP_OUT 0x1c
> +#define ADIS16475_REG_X_DELTANG_L 0x24
> +#define ADIS16475_REG_Y_DELTANG_L 0x28
> +#define ADIS16475_REG_Z_DELTANG_L 0x2C
> +#define ADIS16475_REG_X_DELTVEL_L 0x30
> +#define ADIS16475_REG_Y_DELTVEL_L 0x34
> +#define ADIS16475_REG_Z_DELTVEL_L 0x38
> #define ADIS16475_REG_X_GYRO_BIAS_L 0x40
> #define ADIS16475_REG_Y_GYRO_BIAS_L 0x44
> #define ADIS16475_REG_Z_GYRO_BIAS_L 0x48
> @@ -55,6 +61,8 @@
> #define ADIS16475_REG_PROD_ID 0x72
> #define ADIS16475_REG_SERIAL_NUM 0x74
> #define ADIS16475_REG_FLASH_CNT 0x7c
> +#define ADIS16500_BURST_DATA_SEL_MASK BIT(8)
> +#define
> ADIS16500_BURST_DATA_SEL(x) FIELD_PREP(ADIS16500_BURST_DATA_SEL_MASK, x)
> #define ADIS16500_BURST32_MASK BIT(9)
> #define ADIS16500_BURST32(x) FIELD_PREP(ADIS16500_BURST32_MASK, x)
> /* number of data elements in burst mode */
> @@ -65,6 +73,8 @@
> #define ADIS16475_BURST_MAX_SPEED 1000000
> #define ADIS16475_LSB_DEC_MASK BIT(0)
> #define ADIS16475_LSB_FIR_MASK BIT(1)
> +#define ADIS16500_BURST_DATA_SEL_0_CHN_MASK (GENMASK(6, 0) | BIT(13))
> +#define ADIS16500_BURST_DATA_SEL_1_CHN_MASK GENMASK(13, 6)
>
> enum {
> ADIS16475_SYNC_DIRECT = 1,
> @@ -90,6 +100,8 @@ struct adis16475_chip_info {
> u32 accel_max_val;
> u32 accel_max_scale;
> u32 temp_scale;
> + u32 deltang_max_val;
> + u32 deltvel_max_val;
> u32 int_clk;
> u16 max_dec;
> u8 num_sync;
> @@ -115,6 +127,12 @@ enum {
> ADIS16475_SCAN_ACCEL_Y,
> ADIS16475_SCAN_ACCEL_Z,
> ADIS16475_SCAN_TEMP,
> + ADIS16475_SCAN_DELTANG_X,
> + ADIS16475_SCAN_DELTANG_Y,
> + ADIS16475_SCAN_DELTANG_Z,
> + ADIS16475_SCAN_DELTVEL_X,
> + ADIS16475_SCAN_DELTVEL_Y,
> + ADIS16475_SCAN_DELTVEL_Z,
> };
>
> static bool low_rate_allow;
> @@ -451,6 +469,14 @@ static int adis16475_read_raw(struct iio_dev *indio_dev,
> case IIO_TEMP:
> *val = st->info->temp_scale;
> return IIO_VAL_INT;
> + case IIO_ROT:
> + *val = st->info->deltang_max_val;
> + *val2 = 31;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_VELOCITY:
> + *val = st->info->deltvel_max_val;
> + *val2 = 31;
> + return IIO_VAL_FRACTIONAL_LOG2;
> default:
> return -EINVAL;
> }
> @@ -551,6 +577,40 @@ static int adis16475_write_raw(struct iio_dev *indio_dev,
> }, \
> }
>
> +#define ADIS16475_MOD_CHAN_DELTA(_type, _mod, _address, _si, _r_bits,
> _s_bits) { \
> + .type = (_type), \
> + .modified = 1, \
> + .channel2 = (_mod), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
> + .address = (_address), \
> + .scan_index = _si, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = (_r_bits), \
> + .storagebits = (_s_bits), \
> + .endianness = IIO_BE, \
> + }, \
> + }
> +
> +#define ADIS16475_DELTANG_CHAN(_mod) \
> + ADIS16475_MOD_CHAN_DELTA(IIO_ROT, IIO_MOD_ ## _mod, \
> + ADIS16475_REG_ ## _mod ## _DELTANG_L,
> ADIS16475_SCAN_DELTANG_ ## _mod, 32, 32)
> +
> +#define ADIS16475_DELTVEL_CHAN(_mod) \
> + ADIS16475_MOD_CHAN_DELTA(IIO_VELOCITY, IIO_MOD_ ## _mod, \
> + ADIS16475_REG_ ## _mod ## _DELTVEL_L,
> ADIS16475_SCAN_DELTVEL_ ## _mod, 32, 32)
> +
> +#define ADIS16475_DELTANG_CHAN_NO_SCAN(_mod) \
> + ADIS16475_MOD_CHAN_DELTA(IIO_ROT, IIO_MOD_ ## _mod, \
> + ADIS16475_REG_ ## _mod ## _DELTANG_L, -1, 32, 32)
> +
> +#define ADIS16475_DELTVEL_CHAN_NO_SCAN(_mod) \
> + ADIS16475_MOD_CHAN_DELTA(IIO_VELOCITY, IIO_MOD_ ## _mod, \
> + ADIS16475_REG_ ## _mod ## _DELTVEL_L, -1, 32, 32)
> +
> static const struct iio_chan_spec adis16475_channels[] = {
> ADIS16475_GYRO_CHANNEL(X),
> ADIS16475_GYRO_CHANNEL(Y),
> @@ -559,7 +619,30 @@ static const struct iio_chan_spec adis16475_channels[] =
> {
> ADIS16475_ACCEL_CHANNEL(Y),
> ADIS16475_ACCEL_CHANNEL(Z),
> ADIS16475_TEMP_CHANNEL(),
> - IIO_CHAN_SOFT_TIMESTAMP(7)
> + IIO_CHAN_SOFT_TIMESTAMP(7),
> + ADIS16475_DELTANG_CHAN_NO_SCAN(X),
> + ADIS16475_DELTANG_CHAN_NO_SCAN(Y),
> + ADIS16475_DELTANG_CHAN_NO_SCAN(Z),
> + ADIS16475_DELTVEL_CHAN_NO_SCAN(X),
> + ADIS16475_DELTVEL_CHAN_NO_SCAN(Y),
> + ADIS16475_DELTVEL_CHAN_NO_SCAN(Z)
> +};
> +
> +static const struct iio_chan_spec adis16500_channels[] = {
> + ADIS16475_GYRO_CHANNEL(X),
> + ADIS16475_GYRO_CHANNEL(Y),
> + ADIS16475_GYRO_CHANNEL(Z),
> + ADIS16475_ACCEL_CHANNEL(X),
> + ADIS16475_ACCEL_CHANNEL(Y),
> + ADIS16475_ACCEL_CHANNEL(Z),
> + ADIS16475_TEMP_CHANNEL(),
> + ADIS16475_DELTANG_CHAN(X),
> + ADIS16475_DELTANG_CHAN(Y),
> + ADIS16475_DELTANG_CHAN(Z),
> + ADIS16475_DELTVEL_CHAN(X),
> + ADIS16475_DELTVEL_CHAN(Y),
> + ADIS16475_DELTVEL_CHAN(Z),
> + IIO_CHAN_SOFT_TIMESTAMP(13)
> };
Hmm, IIRC I suggested this to you so apologizes. But I'm not sure we should do
this. In theory, I think we provide all the interfaces so an userspace app would
still work by changing the timestamp index but who knows what apps might be
assuming... So maybe, it's just safer to keep timestamp at index 7.
I guess it will also make the available scan_masks neater.
>
> enum adis16475_variant {
> @@ -662,6 +745,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -677,6 +762,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -692,6 +779,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -707,6 +796,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -715,13 +806,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16477_1] = {
> .name = "adis16477-1",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -731,13 +824,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16477_2] = {
> .name = "adis16477-2",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -747,13 +842,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16477_3] = {
> .name = "adis16477-3",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -770,6 +867,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -785,6 +884,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -800,6 +901,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -815,6 +918,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -830,6 +935,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -845,6 +952,8 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> .accel_max_val = 1,
> .accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -853,13 +962,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16500] = {
> .name = "adis16500",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
> .accel_max_val = 392,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -870,13 +981,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16505_1] = {
> .name = "adis16505-1",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
> .accel_max_val = 78,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -887,13 +1000,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16505_2] = {
> .name = "adis16505-2",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
> .accel_max_val = 78,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -904,13 +1019,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16505_3] = {
> .name = "adis16505-3",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
> .accel_max_val = 78,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 100,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -921,13 +1038,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16507_1] = {
> .name = "adis16507-1",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
> .accel_max_val = 392,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 360,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -938,13 +1057,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16507_2] = {
> .name = "adis16507-2",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
> .accel_max_val = 392,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 720,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -955,13 +1076,15 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> [ADIS16507_3] = {
> .name = "adis16507-3",
> - .num_channels = ARRAY_SIZE(adis16475_channels),
> - .channels = adis16475_channels,
> + .num_channels = ARRAY_SIZE(adis16500_channels),
> + .channels = adis16500_channels,
> .gyro_max_val = 1,
> .gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
> .accel_max_val = 392,
> .accel_max_scale = 32000 << 16,
> .temp_scale = 100,
> + .deltang_max_val = 2160,
> + .deltvel_max_val = 400,
> .int_clk = 2000,
> .max_dec = 1999,
> .sync = adis16475_sync_mode,
> @@ -972,10 +1095,40 @@ static const struct adis16475_chip_info
> adis16475_chip_info[] = {
> },
> };
>
> +static const unsigned long adis16475_avail_scan_masks[] = {
> + ADIS16500_BURST_DATA_SEL_0_CHN_MASK,
> + ADIS16500_BURST_DATA_SEL_1_CHN_MASK,
> + 0
> +};
> +
Hmm I guess this ok if we don't have strict scan_mask match. AFAICT, that's only
the case for HARDWARE buffering and not likely to change... right? Jonathan?
> +static int adis16475_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + u16 en;
> + int ret;
> + struct adis16475 *st = iio_priv(indio_dev);
> +
> + if (st->info->has_burst32) {
Hmm, I get that buffering these channels are only available in devices that have
burst32. But I would not assume that as there's no guarantee that will always be
the case. Given that's straight to just have another flag for the presence of
these channels in the burst data, I would prefer to have one right now.
As a nit I would flip the logic in here:
if (!st->info->has_burst32)
return adis_update_scan_mode(indio_dev, scan_mask);
Then you could keep indentation and even save some lines of code. Anyways, just
my personal preference.
> + if (*scan_mask & ADIS16500_BURST_DATA_SEL_0_CHN_MASK)
> + en = ADIS16500_BURST_DATA_SEL(0);
> + else if (*scan_mask & ADIS16500_BURST_DATA_SEL_1_CHN_MASK)
> + en = ADIS16500_BURST_DATA_SEL(1);
> + else
> + return -EINVAL;
> +
I don't think the above is ever possible since you're guaranteed that a subset
of the available_scan_masks is set. So, if() .. else should be enough.
Moreover will this actually work with BIT(13) or BIT(6) set? Won't we always
fall in the first case if one of those bits are set even if we want
ADIS16500_BURST_DATA_SEL(1)? So I guess you should explicitly check against
GENMASK(5, 0) for normal accel/gyro or GENMASK(13, 8) for delta stuff. Right?
- Nuno Sá
Powered by blists - more mailing lists