On Fri, 4 Aug 2023 09:45:59 +0300 Ramona Bolboaca <ramona.bolboaca@xxxxxxxxxx> wrote: > Add support for delta angle and delta velocity raw and buffer > readings to adis16475 driver. > > Signed-off-by: Ramona Bolboaca <ramona.bolboaca@xxxxxxxxxx> Hi Ramona, A few trivial comments inline given we need to make that unit change so you will be doing a v4. Otherwise I might have just done a bit of tidying up whilst applying. Thanks, Jonathan > --- > drivers/iio/imu/adis16475.c | 165 +++++++++++++++++++++++++++++++----- > 1 file changed, 146 insertions(+), 19 deletions(-) > > diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c > index 17275a53ca2c..dbbeb80c4d23 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) I guess this is consistent with other bits of the driver, but I'm not sure in general that the macro adds anything over directly calling the FIELD_PREP() which is pretty obvious on it's own. > #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,10 @@ > #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(5, 0) > +#define ADIS16500_BURST_DATA_SEL_1_CHN_MASK GENMASK(12, 7) Add a blank line here to separate these flag definitions or see below... > +#define ADIS16475_HAS_BURST32 BIT(0) > +#define ADIS16475_HAS_BURST_DELTA_DATA BIT(1) > > enum { > ADIS16475_SYNC_DIRECT = 1, > @@ -84,16 +96,18 @@ struct adis16475_chip_info { > const struct adis16475_sync *sync; > const struct adis_data adis_data; > const char *name; > + const long flags; I would put the two flag definitions here. Then it's obvious what they are used for. > u32 num_channels; > u32 gyro_max_val; > u32 gyro_max_scale; > 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; > - bool has_burst32; > }; > > struct adis16475 { > @@ -115,6 +129,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, > }; > > [ADIS16507_3] = { > @@ -962,20 +1060,46 @@ static const struct adis16475_chip_info adis16475_chip_info[] = { > .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, > /* pulse sync not supported */ > .num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1, > - .has_burst32 = true, > + .flags = ADIS16475_HAS_BURST32 | ADIS16475_HAS_BURST_DELTA_DATA, In the ideal world, a precursor patch would have made the change to have a flags field, then this one would have just added the new flags. However it would have made a huge difference to readability of this patch so I'm not that bothered about not having it broken out. > .adis_data = ADIS16475_DATA(16507, &adis1650x_timeouts), > }, > }; > > +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->flags & ADIS16475_HAS_BURST_DELTA_DATA) { > + if ((*scan_mask & ADIS16500_BURST_DATA_SEL_0_CHN_MASK) && (*scan_mask & ADIS16500_BURST_DATA_SEL_1_CHN_MASK)) Very long line. No obvious reason not to break it after the && > + return -EINVAL; > + if (*scan_mask & ADIS16500_BURST_DATA_SEL_0_CHN_MASK) > + en = ADIS16500_BURST_DATA_SEL(0); > + else > + en = ADIS16500_BURST_DATA_SEL(1); > + > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL, > + ADIS16500_BURST_DATA_SEL_MASK, en); > + if (ret) > + return ret; > + } > + > + return adis_update_scan_mode(indio_dev, scan_mask); > +}