Re: [PATCH v3 3/3] iio: imu: adis16475.c: Add delta angle and delta velocity channels

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 4 Aug 2023 09:45:59 +0300
Ramona Bolboaca <ramona.bolboaca@analog.com> wrote:

> Add support for delta angle and delta velocity raw and buffer
> readings to adis16475 driver.
> 
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>

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);
> +}