[PATCH 0/5] Add support for adis16545/47

[PATCH 0/5] Add support for adis16545/47

[Ramona Gradinariu]

Add support for delta angle and delta velocity channels in adis16480 driver.
Add support for ADIS16545/47 devices in already existing adis16480 driver.
Add documentation for adis16480 driver.

Ramona Gradinariu (5):
  iio: adis16480: make the burst_max_speed configurable
  iio: imu: adis16480.c: Add delta angle and delta velocity channels
  dt-bindings: iio: imu: Add docs for ADIS16545/47
  iio: adis16480: add support for adis16545/7 families
  docs: iio: add documentation for adis16480 driver

 .../bindings/iio/imu/adi,adis16480.yaml       |   6 +
 Documentation/iio/adis16480.rst               | 461 ++++++++++++++++++
 Documentation/iio/index.rst                   |   1 +
 drivers/iio/imu/adis16480.c                   | 384 +++++++++++++--
 4 files changed, 803 insertions(+), 49 deletions(-)
 create mode 100644 Documentation/iio/adis16480.rst

--
2.34.1

[PATCH 1/5] iio: adis16480: make the burst_max_speed configurable

[Ramona Gradinariu]

With this, we can pass the maxixum spi burst speed to the
'ADIS16480_DATA()' macro. This is in preparation to support new devices
that have a different speed than the one used so far.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 drivers/iio/imu/adis16480.c | 84 +++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index b40a55bba30c..bc6cbd00cd4b 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -107,8 +107,6 @@
 #define ADIS16495_BURST_ID			0xA5A5
 /* total number of segments in burst */
 #define ADIS16495_BURST_MAX_DATA		20
-/* spi max speed in burst mode */
-#define ADIS16495_BURST_MAX_SPEED              6000000
 
 #define ADIS16480_REG_SERIAL_NUM		ADIS16480_REG(0x04, 0x20)
 
@@ -872,33 +870,33 @@ static const char * const adis16480_status_error_msgs[] = {
 
 static int adis16480_enable_irq(struct adis *adis, bool enable);
 
-#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len)			\
-{									\
-	.diag_stat_reg = ADIS16480_REG_DIAG_STS,			\
-	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,				\
-	.prod_id_reg = ADIS16480_REG_PROD_ID,				\
-	.prod_id = (_prod_id),						\
-	.has_paging = true,						\
-	.read_delay = 5,						\
-	.write_delay = 5,						\
-	.self_test_mask = BIT(1),					\
-	.self_test_reg = ADIS16480_REG_GLOB_CMD,			\
-	.status_error_msgs = adis16480_status_error_msgs,		\
-	.status_error_mask = BIT(ADIS16480_DIAG_STAT_XGYRO_FAIL) |	\
-		BIT(ADIS16480_DIAG_STAT_YGYRO_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_ZGYRO_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_XACCL_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_YACCL_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_ZACCL_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_XMAGN_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_YMAGN_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_ZMAGN_FAIL) |			\
-		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),			\
-	.enable_irq = adis16480_enable_irq,				\
-	.timeouts = (_timeouts),					\
-	.burst_reg_cmd = ADIS16495_REG_BURST_CMD,			\
-	.burst_len = (_burst_len),					\
-	.burst_max_speed_hz = ADIS16495_BURST_MAX_SPEED			\
+#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len, _burst_max_speed)	\
+{										\
+	.diag_stat_reg = ADIS16480_REG_DIAG_STS,				\
+	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,					\
+	.prod_id_reg = ADIS16480_REG_PROD_ID,					\
+	.prod_id = (_prod_id),							\
+	.has_paging = true,							\
+	.read_delay = 5,							\
+	.write_delay = 5,							\
+	.self_test_mask = BIT(1),						\
+	.self_test_reg = ADIS16480_REG_GLOB_CMD,				\
+	.status_error_msgs = adis16480_status_error_msgs,			\
+	.status_error_mask = BIT(ADIS16480_DIAG_STAT_XGYRO_FAIL) |		\
+		BIT(ADIS16480_DIAG_STAT_YGYRO_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_ZGYRO_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_XACCL_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_YACCL_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_ZACCL_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_XMAGN_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_YMAGN_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_ZMAGN_FAIL) |				\
+		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),				\
+	.enable_irq = adis16480_enable_irq,					\
+	.timeouts = (_timeouts),						\
+	.burst_reg_cmd = ADIS16495_REG_BURST_CMD,				\
+	.burst_len = (_burst_len),						\
+	.burst_max_speed_hz = _burst_max_speed					\
 }
 
 static const struct adis_timeout adis16485_timeouts = {
@@ -944,7 +942,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts, 0),
+		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts, 0, 0),
 	},
 	[ADIS16480] = {
 		.channels = adis16480_channels,
@@ -958,7 +956,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts, 0),
+		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts, 0, 0),
 	},
 	[ADIS16485] = {
 		.channels = adis16485_channels,
@@ -972,7 +970,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts, 0),
+		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts, 0, 0),
 	},
 	[ADIS16488] = {
 		.channels = adis16480_channels,
@@ -986,7 +984,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts, 0),
+		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts, 0, 0),
 	},
 	[ADIS16490] = {
 		.channels = adis16485_channels,
@@ -1000,7 +998,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts, 0),
+		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts, 0, 0),
 	},
 	[ADIS16495_1] = {
 		.channels = adis16485_channels,
@@ -1016,7 +1014,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 	[ADIS16495_2] = {
 		.channels = adis16485_channels,
@@ -1032,7 +1031,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 	[ADIS16495_3] = {
 		.channels = adis16485_channels,
@@ -1048,7 +1048,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 	[ADIS16497_1] = {
 		.channels = adis16485_channels,
@@ -1064,7 +1065,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 	[ADIS16497_2] = {
 		.channels = adis16485_channels,
@@ -1080,7 +1082,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 	[ADIS16497_3] = {
 		.channels = adis16485_channels,
@@ -1096,7 +1099,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.has_pps_clk_mode = true,
 		/* 20 elements of 16bits */
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
-					    ADIS16495_BURST_MAX_DATA * 2),
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    6000000),
 	},
 };
 
-- 
2.34.1

[PATCH 2/5] iio: imu: adis16480.c: Add delta angle and delta velocity channels

[Ramona Gradinariu]

Add support for delta angle and delta velocity raw readings to
adis16480 driver.

Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 drivers/iio/imu/adis16480.c | 78 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index bc6cbd00cd4b..4adc2244a4ef 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -140,6 +140,8 @@ struct adis16480_chip_info {
 	unsigned int accel_max_val;
 	unsigned int accel_max_scale;
 	unsigned int temp_scale;
+	unsigned int deltang_max_val;
+	unsigned int deltvel_max_val;
 	unsigned int int_clk;
 	unsigned int max_dec_rate;
 	const unsigned int *filter_freqs;
@@ -445,6 +447,12 @@ enum {
 	ADIS16480_SCAN_MAGN_Z,
 	ADIS16480_SCAN_BARO,
 	ADIS16480_SCAN_TEMP,
+	ADIS16480_SCAN_DELTANG_X,
+	ADIS16480_SCAN_DELTANG_Y,
+	ADIS16480_SCAN_DELTANG_Z,
+	ADIS16480_SCAN_DELTVEL_X,
+	ADIS16480_SCAN_DELTVEL_Y,
+	ADIS16480_SCAN_DELTVEL_Z,
 };
 
 static const unsigned int adis16480_calibbias_regs[] = {
@@ -688,6 +696,14 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
 			*val = 131; /* 1310mbar = 131 kPa */
 			*val2 = 32767 << 16;
 			return IIO_VAL_FRACTIONAL;
+		case IIO_DELTA_ANGL:
+			*val = st->chip_info->deltang_max_val;
+			*val2 = 31;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_DELTA_VELOCITY:
+			*val = st->chip_info->deltvel_max_val;
+			*val2 = 31;
+			return IIO_VAL_FRACTIONAL_LOG2;
 		default:
 			return -EINVAL;
 		}
@@ -761,6 +777,30 @@ static int adis16480_write_raw(struct iio_dev *indio_dev,
 	BIT(IIO_CHAN_INFO_CALIBSCALE), \
 	32)
 
+#define ADIS16480_DELTANG_CHANNEL(_mod) \
+	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
+	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, ADIS16480_SCAN_DELTANG_ ## _mod, \
+	0, \
+	32)
+
+#define ADIS16480_DELTANG_CHANNEL_NO_SCAN(_mod) \
+	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
+	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, -1, \
+	0, \
+	32)
+
+#define ADIS16480_DELTVEL_CHANNEL(_mod) \
+	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
+	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, ADIS16480_SCAN_DELTVEL_ ## _mod, \
+	0, \
+	32)
+
+#define ADIS16480_DELTVEL_CHANNEL_NO_SCAN(_mod) \
+	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
+	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, -1, \
+	0, \
+	32)
+
 #define ADIS16480_MAGN_CHANNEL(_mod) \
 	ADIS16480_MOD_CHANNEL(IIO_MAGN, IIO_MOD_ ## _mod, \
 	ADIS16480_REG_ ## _mod ## _MAGN_OUT, ADIS16480_SCAN_MAGN_ ## _mod, \
@@ -816,7 +856,13 @@ static const struct iio_chan_spec adis16480_channels[] = {
 	ADIS16480_MAGN_CHANNEL(Z),
 	ADIS16480_PRESSURE_CHANNEL(),
 	ADIS16480_TEMP_CHANNEL(),
-	IIO_CHAN_SOFT_TIMESTAMP(11)
+	IIO_CHAN_SOFT_TIMESTAMP(11),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(X),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(Y),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(Z),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(X),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Y),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Z),
 };
 
 static const struct iio_chan_spec adis16485_channels[] = {
@@ -827,7 +873,13 @@ static const struct iio_chan_spec adis16485_channels[] = {
 	ADIS16480_ACCEL_CHANNEL(Y),
 	ADIS16480_ACCEL_CHANNEL(Z),
 	ADIS16480_TEMP_CHANNEL(),
-	IIO_CHAN_SOFT_TIMESTAMP(7)
+	IIO_CHAN_SOFT_TIMESTAMP(7),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(X),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(Y),
+	ADIS16480_DELTANG_CHANNEL_NO_SCAN(Z),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(X),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Y),
+	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Z),
 };
 
 enum adis16480_variant {
@@ -938,6 +990,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(21973 << 16),
 		.accel_max_scale = 18,
 		.temp_scale = 5650, /* 5.65 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(180),
+		.deltvel_max_val = 100,
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
@@ -952,6 +1006,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(12500 << 16),
 		.accel_max_scale = 10,
 		.temp_scale = 5650, /* 5.65 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 200,
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
@@ -966,6 +1022,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(20000 << 16),
 		.accel_max_scale = 5,
 		.temp_scale = 5650, /* 5.65 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 50,
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
@@ -980,6 +1038,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(22500 << 16),
 		.accel_max_scale = 18,
 		.temp_scale = 5650, /* 5.65 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 200,
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.has_sleep_cnt = true,
@@ -994,6 +1054,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(16000 << 16),
 		.accel_max_scale = 8,
 		.temp_scale = 14285, /* 14.285 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 200,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1008,6 +1070,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 8,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(360),
+		.deltvel_max_val = 100,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1025,6 +1089,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 8,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 100,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1042,6 +1108,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 8,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(2160),
+		.deltvel_max_val = 100,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1059,6 +1127,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 40,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(360),
+		.deltvel_max_val = 400,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1076,6 +1146,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 40,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 400,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
@@ -1093,6 +1165,8 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
 		.accel_max_scale = 40,
 		.temp_scale = 12500, /* 12.5 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(2160),
+		.deltvel_max_val = 400,
 		.int_clk = 4250000,
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
-- 
2.34.1

Re: [PATCH 2/5] iio: imu: adis16480.c: Add delta angle and delta velocity channels

[Jonathan Cameron]

On Tue, 23 Apr 2024 11:42:07 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

> Add support for delta angle and delta velocity raw readings to
> adis16480 driver.

Why are these not allowed via the buffer interface?   Normally
my expectation of delta values is they are more or less useless
without buffered capture. The intent of providing those channels
is that they are gathered over time and summed up to give the
angle difference (for example) between start of capture and now.
Note the formula on the datasheet 
https://www.analog.com/media/en/technical-documentation/data-sheets/adis16545-16547.pdf
looks wrong (formula 3) as it's adding the signals at time
nD + d and at nD + D - 1 whereas for a delta you'd subtract those
(maybe I'm reading that wrong).

If we are providing these values as raw readings I'd expect them
to be presented as delta_angle / time (e.g. rate of change of angle) and
delta_velocity / time = acceleration (be it slightly distorted vs
the acceleration measured as a result of oversampling.).
So basically spot measurements of delta values are normally pretty
useless.

My guess is that you did this because the device either seems
to allow burst reads of the main channels or of these delta
values?

If so consider using available_scan_masks to allow one or the
other set of channels rather than not allowing capture of these
via the buffered interfaces.

Jonathan



> 
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> ---
>  drivers/iio/imu/adis16480.c | 78 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index bc6cbd00cd4b..4adc2244a4ef 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -140,6 +140,8 @@ struct adis16480_chip_info {
>  	unsigned int accel_max_val;
>  	unsigned int accel_max_scale;
>  	unsigned int temp_scale;
> +	unsigned int deltang_max_val;
> +	unsigned int deltvel_max_val;
>  	unsigned int int_clk;
>  	unsigned int max_dec_rate;
>  	const unsigned int *filter_freqs;
> @@ -445,6 +447,12 @@ enum {
>  	ADIS16480_SCAN_MAGN_Z,
>  	ADIS16480_SCAN_BARO,
>  	ADIS16480_SCAN_TEMP,
> +	ADIS16480_SCAN_DELTANG_X,
> +	ADIS16480_SCAN_DELTANG_Y,
> +	ADIS16480_SCAN_DELTANG_Z,
> +	ADIS16480_SCAN_DELTVEL_X,
> +	ADIS16480_SCAN_DELTVEL_Y,
> +	ADIS16480_SCAN_DELTVEL_Z,
>  };
>  
>  static const unsigned int adis16480_calibbias_regs[] = {
> @@ -688,6 +696,14 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
>  			*val = 131; /* 1310mbar = 131 kPa */
>  			*val2 = 32767 << 16;
>  			return IIO_VAL_FRACTIONAL;
> +		case IIO_DELTA_ANGL:
> +			*val = st->chip_info->deltang_max_val;
> +			*val2 = 31;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_DELTA_VELOCITY:
> +			*val = st->chip_info->deltvel_max_val;
> +			*val2 = 31;
> +			return IIO_VAL_FRACTIONAL_LOG2;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -761,6 +777,30 @@ static int adis16480_write_raw(struct iio_dev *indio_dev,
>  	BIT(IIO_CHAN_INFO_CALIBSCALE), \
>  	32)
>  
> +#define ADIS16480_DELTANG_CHANNEL(_mod) \
> +	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
> +	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, ADIS16480_SCAN_DELTANG_ ## _mod, \
> +	0, \

Trivial but why this line wrap?  I'd push 32 onto the line above at least.

> +	32)
> +
> +#define ADIS16480_DELTANG_CHANNEL_NO_SCAN(_mod) \
> +	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
> +	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, -1, \
> +	0, \
> +	32)
> +
> +#define ADIS16480_DELTVEL_CHANNEL(_mod) \
> +	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
> +	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, ADIS16480_SCAN_DELTVEL_ ## _mod, \
> +	0, \
> +	32)
> +
> +#define ADIS16480_DELTVEL_CHANNEL_NO_SCAN(_mod) \
> +	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
> +	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, -1, \
> +	0, \
> +	32)

Re: [PATCH 2/5] iio: imu: adis16480.c: Add delta angle and delta velocity channels

[Jonathan Cameron]

On Sun, 28 Apr 2024 16:04:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 23 Apr 2024 11:42:07 +0300
> Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> 
> > Add support for delta angle and delta velocity raw readings to
> > adis16480 driver.  
> 
> Why are these not allowed via the buffer interface?   Normally
> my expectation of delta values is they are more or less useless
> without buffered capture. The intent of providing those channels
> is that they are gathered over time and summed up to give the
> angle difference (for example) between start of capture and now.
> Note the formula on the datasheet 
> https://www.analog.com/media/en/technical-documentation/data-sheets/adis16545-16547.pdf
> looks wrong (formula 3) as it's adding the signals at time
> nD + d and at nD + D - 1 whereas for a delta you'd subtract those
> (maybe I'm reading that wrong).
> 
> If we are providing these values as raw readings I'd expect them
> to be presented as delta_angle / time (e.g. rate of change of angle) and
> delta_velocity / time = acceleration (be it slightly distorted vs
> the acceleration measured as a result of oversampling.).
> So basically spot measurements of delta values are normally pretty
> useless.
> 
> My guess is that you did this because the device either seems
> to allow burst reads of the main channels or of these delta
> values?
> 
> If so consider using available_scan_masks to allow one or the
> other set of channels rather than not allowing capture of these
> via the buffered interfaces.

I see from later patch that this was adding support to parts that
don't support grabbing these in burst mode.  Please make that clearer
in this patch description and list the parts to which this support
applied.

I'm a bit dubious of it being a useful feature, but maybe it is
worth supporting.

Jonathan

> 
> Jonathan
> 
> 
> 
> > 
> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> > ---
> >  drivers/iio/imu/adis16480.c | 78 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 76 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index bc6cbd00cd4b..4adc2244a4ef 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -140,6 +140,8 @@ struct adis16480_chip_info {
> >  	unsigned int accel_max_val;
> >  	unsigned int accel_max_scale;
> >  	unsigned int temp_scale;
> > +	unsigned int deltang_max_val;
> > +	unsigned int deltvel_max_val;
> >  	unsigned int int_clk;
> >  	unsigned int max_dec_rate;
> >  	const unsigned int *filter_freqs;
> > @@ -445,6 +447,12 @@ enum {
> >  	ADIS16480_SCAN_MAGN_Z,
> >  	ADIS16480_SCAN_BARO,
> >  	ADIS16480_SCAN_TEMP,
> > +	ADIS16480_SCAN_DELTANG_X,
> > +	ADIS16480_SCAN_DELTANG_Y,
> > +	ADIS16480_SCAN_DELTANG_Z,
> > +	ADIS16480_SCAN_DELTVEL_X,
> > +	ADIS16480_SCAN_DELTVEL_Y,
> > +	ADIS16480_SCAN_DELTVEL_Z,
> >  };
> >  
> >  static const unsigned int adis16480_calibbias_regs[] = {
> > @@ -688,6 +696,14 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
> >  			*val = 131; /* 1310mbar = 131 kPa */
> >  			*val2 = 32767 << 16;
> >  			return IIO_VAL_FRACTIONAL;
> > +		case IIO_DELTA_ANGL:
> > +			*val = st->chip_info->deltang_max_val;
> > +			*val2 = 31;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> > +		case IIO_DELTA_VELOCITY:
> > +			*val = st->chip_info->deltvel_max_val;
> > +			*val2 = 31;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> >  		default:
> >  			return -EINVAL;
> >  		}
> > @@ -761,6 +777,30 @@ static int adis16480_write_raw(struct iio_dev *indio_dev,
> >  	BIT(IIO_CHAN_INFO_CALIBSCALE), \
> >  	32)
> >  
> > +#define ADIS16480_DELTANG_CHANNEL(_mod) \
> > +	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
> > +	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, ADIS16480_SCAN_DELTANG_ ## _mod, \
> > +	0, \  
> 
> Trivial but why this line wrap?  I'd push 32 onto the line above at least.
> 
> > +	32)
> > +
> > +#define ADIS16480_DELTANG_CHANNEL_NO_SCAN(_mod) \
> > +	ADIS16480_MOD_CHANNEL(IIO_DELTA_ANGL, IIO_MOD_ ## _mod, \
> > +	ADIS16480_REG_ ## _mod ## _DELTAANG_OUT, -1, \
> > +	0, \
> > +	32)
> > +
> > +#define ADIS16480_DELTVEL_CHANNEL(_mod) \
> > +	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
> > +	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, ADIS16480_SCAN_DELTVEL_ ## _mod, \
> > +	0, \
> > +	32)
> > +
> > +#define ADIS16480_DELTVEL_CHANNEL_NO_SCAN(_mod) \
> > +	ADIS16480_MOD_CHANNEL(IIO_DELTA_VELOCITY, IIO_MOD_ ## _mod, \
> > +	ADIS16480_REG_ ## _mod ## _DELTAVEL_OUT, -1, \
> > +	0, \
> > +	32)  
> 

[PATCH 3/5] dt-bindings: iio: imu: Add docs for ADIS16545/47

[Ramona Gradinariu]

Update ADIS16480 existing documentation with documentation for
ADIS16545/47 devices.

Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 .../devicetree/bindings/iio/imu/adi,adis16480.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16480.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.yaml
index 56e0dc20f5e4..e3eec38897bf 100644
--- a/Documentation/devicetree/bindings/iio/imu/adi,adis16480.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.yaml
@@ -23,6 +23,12 @@ properties:
       - adi,adis16497-1
       - adi,adis16497-2
       - adi,adis16497-3
+      - adi,adis16545-1
+      - adi,adis16545-2
+      - adi,adis16545-3
+      - adi,adis16547-1
+      - adi,adis16547-2
+      - adi,adis16547-3
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH 3/5] dt-bindings: iio: imu: Add docs for ADIS16545/47

[Krzysztof Kozlowski]

On 23/04/2024 10:42, Ramona Gradinariu wrote:
> Update ADIS16480 existing documentation with documentation for
> ADIS16545/47 devices.

Nothing in this commit msg explains me why they are not compatible,
while driver suggests a bit they are.

A nit, subject: drop second/last, redundant "docs for". The
"dt-bindings" prefix is already stating that these are bindings which is
documentation.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Best regards,
Krzysztof

[PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Ramona Gradinariu]

The ADIS16545 and ADIS16547 are a complete inertial system that
includes a triaxis gyroscope and a triaxis accelerometer.
The serial peripheral interface (SPI) and register structure provide a
simple interface for data collection and configuration control.

These devices are similar to the ones already supported in the driver,
with changes in the scales, timings and the max spi speed in burst
mode.
Also, they support delta angle and delta velocity readings in burst
mode, for which support was added in the trigger handler.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 drivers/iio/imu/adis16480.c | 222 ++++++++++++++++++++++++++++++++++--
 1 file changed, 215 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 4adc2244a4ef..e0020b7b5fb5 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -132,6 +132,10 @@
 #define ADIS16480_SYNC_MODE_MSK		BIT(8)
 #define ADIS16480_SYNC_MODE(x)		FIELD_PREP(ADIS16480_SYNC_MODE_MSK, x)
 
+#define ADIS16545_BURST_DATA_SEL_0_CHN_MASK	GENMASK(5, 0)
+#define ADIS16545_BURST_DATA_SEL_1_CHN_MASK	GENMASK(16, 11)
+#define ADIS16545_BURST_DATA_SEL_MASK		BIT(8)
+
 struct adis16480_chip_info {
 	unsigned int num_channels;
 	const struct iio_chan_spec *channels;
@@ -147,6 +151,7 @@ struct adis16480_chip_info {
 	const unsigned int *filter_freqs;
 	bool has_pps_clk_mode;
 	bool has_sleep_cnt;
+	bool has_burst_delta_data;
 	const struct adis_data adis_data;
 };
 
@@ -170,6 +175,7 @@ struct adis16480 {
 	struct clk *ext_clk;
 	enum adis16480_clock_mode clk_mode;
 	unsigned int clk_freq;
+	u16 burst_id;
 	/* Alignment needed for the timestamp */
 	__be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8);
 };
@@ -882,6 +888,23 @@ static const struct iio_chan_spec adis16485_channels[] = {
 	ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Z),
 };
 
+static const struct iio_chan_spec adis16545_channels[] = {
+	ADIS16480_GYRO_CHANNEL(X),
+	ADIS16480_GYRO_CHANNEL(Y),
+	ADIS16480_GYRO_CHANNEL(Z),
+	ADIS16480_ACCEL_CHANNEL(X),
+	ADIS16480_ACCEL_CHANNEL(Y),
+	ADIS16480_ACCEL_CHANNEL(Z),
+	ADIS16480_TEMP_CHANNEL(),
+	ADIS16480_DELTANG_CHANNEL(X),
+	ADIS16480_DELTANG_CHANNEL(Y),
+	ADIS16480_DELTANG_CHANNEL(Z),
+	ADIS16480_DELTVEL_CHANNEL(X),
+	ADIS16480_DELTVEL_CHANNEL(Y),
+	ADIS16480_DELTVEL_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(13),
+};
+
 enum adis16480_variant {
 	ADIS16375,
 	ADIS16480,
@@ -894,6 +917,12 @@ enum adis16480_variant {
 	ADIS16497_1,
 	ADIS16497_2,
 	ADIS16497_3,
+	ADIS16545_1,
+	ADIS16545_2,
+	ADIS16545_3,
+	ADIS16547_1,
+	ADIS16547_2,
+	ADIS16547_3
 };
 
 #define ADIS16480_DIAG_STAT_XGYRO_FAIL 0
@@ -975,6 +1004,12 @@ static const struct adis_timeout adis16495_1_timeouts = {
 	.self_test_ms = 20,
 };
 
+static const struct adis_timeout adis16545_timeouts = {
+	.reset_ms = 315,
+	.sw_reset_ms = 270,
+	.self_test_ms = 35,
+};
+
 static const struct adis16480_chip_info adis16480_chip_info[] = {
 	[ADIS16375] = {
 		.channels = adis16485_channels,
@@ -1176,6 +1211,126 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 					    ADIS16495_BURST_MAX_DATA * 2,
 					    6000000),
 	},
+	[ADIS16545_1] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 20000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(125),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 8,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(360),
+		.deltvel_max_val = 100,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16545, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
+	[ADIS16545_2] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 18000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(450),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 8,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 100,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16545, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
+	[ADIS16545_3] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 20000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(2000),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 8,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(2160),
+		.deltvel_max_val = 100,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16545, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
+	[ADIS16547_1] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 20000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(125),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 40,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(360),
+		.deltvel_max_val = 400,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16547, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
+	[ADIS16547_2] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 18000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(450),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 40,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(720),
+		.deltvel_max_val = 400,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16547, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
+	[ADIS16547_3] = {
+		.channels = adis16545_channels,
+		.num_channels = ARRAY_SIZE(adis16545_channels),
+		.gyro_max_val = 20000 << 16,
+		.gyro_max_scale = IIO_DEGREE_TO_RAD(2000),
+		.accel_max_val = IIO_M_S_2_TO_G(32000 << 16),
+		.accel_max_scale = 40,
+		.temp_scale = 7000, /* 7 milli degree Celsius */
+		.deltang_max_val = IIO_DEGREE_TO_RAD(2160),
+		.deltvel_max_val = 400,
+		.int_clk = 4250000,
+		.max_dec_rate = 4250,
+		.filter_freqs = adis16495_def_filter_freqs,
+		.has_pps_clk_mode = true,
+		.has_burst_delta_data = true,
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16547, &adis16545_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2,
+					    3200000),
+	},
 };
 
 static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc)
@@ -1200,7 +1355,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	struct adis16480 *st = iio_priv(indio_dev);
 	struct adis *adis = &st->adis;
 	struct device *dev = &adis->spi->dev;
-	int ret, bit, offset, i = 0;
+	int ret, bit, offset, i = 0, buff_offset = 0;
 	__be16 *buffer;
 	u32 crc;
 	bool valid;
@@ -1233,8 +1388,8 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	 * 16-bit responses containing the BURST_ID depending on the sclk. If
 	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
 	 * we have only one. To manage that variation, we use the transition from the
-	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If
-	 * we not find this variation in the first 4 segments, then the data should
+	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3.
+	 * If we not find this variation in the first 4 segments, then the data should
 	 * not be valid.
 	 */
 	buffer = adis->buffer;
@@ -1242,7 +1397,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 		u16 curr = be16_to_cpu(buffer[offset]);
 		u16 next = be16_to_cpu(buffer[offset + 1]);
 
-		if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) {
+		if (curr == st->burst_id && next != st->burst_id) {
 			offset++;
 			break;
 		}
@@ -1269,11 +1424,22 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 		switch (bit) {
 		case ADIS16480_SCAN_TEMP:
 			st->data[i++] = buffer[offset + 1];
+			/*
+			 * The temperature channel has 16-bit storage size.
+			 * We need to perform the padding to have the buffer
+			 * elements naturally aligned in case there are any
+			 * 32-bit storage size channels enabled which have a
+			 * scan index higher than the temperature channel scan
+			 * index.
+			 */
+			if (*indio_dev->active_scan_mask &
+			    GENMASK(ADIS16480_SCAN_DELTVEL_Z, ADIS16480_SCAN_DELTANG_X))
+				st->data[i++] = 0;
 			break;
 		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
 			/* The lower register data is sequenced first */
-			st->data[i++] = buffer[2 * bit + offset + 3];
-			st->data[i++] = buffer[2 * bit + offset + 2];
+			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3];
+			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2];
 			break;
 		}
 	}
@@ -1285,10 +1451,38 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int adis16480_update_scan_mode(struct iio_dev *indio_dev,
+				      const unsigned long *scan_mask)
+{
+	u16 en;
+	int ret;
+	struct adis16480 *st = iio_priv(indio_dev);
+
+	if (st->chip_info->has_burst_delta_data) {
+		if ((*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) &&
+		    (*scan_mask & ADIS16545_BURST_DATA_SEL_1_CHN_MASK))
+			return -EINVAL;
+		if (*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) {
+			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 0);
+			st->burst_id = 0xA5A5;
+		} else {
+			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 1);
+			st->burst_id = 0xC3C3;
+		}
+
+		ret = __adis_update_bits(&st->adis, ADIS16480_REG_CONFIG,
+					 ADIS16545_BURST_DATA_SEL_MASK, en);
+		if (ret)
+			return ret;
+	}
+
+	return adis_update_scan_mode(indio_dev, scan_mask);
+}
+
 static const struct iio_info adis16480_info = {
 	.read_raw = &adis16480_read_raw,
 	.write_raw = &adis16480_write_raw,
-	.update_scan_mode = adis_update_scan_mode,
+	.update_scan_mode = &adis16480_update_scan_mode,
 	.debugfs_reg_access = adis_debugfs_reg_access,
 };
 
@@ -1498,6 +1692,8 @@ static int adis16480_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	st->burst_id = ADIS16495_BURST_ID;
+
 	if (st->chip_info->has_sleep_cnt) {
 		ret = devm_add_action_or_reset(dev, adis16480_stop, indio_dev);
 		if (ret)
@@ -1571,6 +1767,12 @@ static const struct spi_device_id adis16480_ids[] = {
 	{ "adis16497-1", ADIS16497_1 },
 	{ "adis16497-2", ADIS16497_2 },
 	{ "adis16497-3", ADIS16497_3 },
+	{ "adis16545-1", ADIS16545_1 },
+	{ "adis16545-2", ADIS16545_2 },
+	{ "adis16545-3", ADIS16545_3 },
+	{ "adis16547-1", ADIS16547_1 },
+	{ "adis16547-2", ADIS16547_2 },
+	{ "adis16547-3", ADIS16547_3 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adis16480_ids);
@@ -1587,6 +1789,12 @@ static const struct of_device_id adis16480_of_match[] = {
 	{ .compatible = "adi,adis16497-1" },
 	{ .compatible = "adi,adis16497-2" },
 	{ .compatible = "adi,adis16497-3" },
+	{ .compatible = "adi,adis16545-1" },
+	{ .compatible = "adi,adis16545-2" },
+	{ .compatible = "adi,adis16545-3" },
+	{ .compatible = "adi,adis16547-1" },
+	{ .compatible = "adi,adis16547-2" },
+	{ .compatible = "adi,adis16547-3" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, adis16480_of_match);
-- 
2.34.1

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Jonathan Cameron]

On Tue, 23 Apr 2024 11:42:09 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

> The ADIS16545 and ADIS16547 are a complete inertial system that
> includes a triaxis gyroscope and a triaxis accelerometer.
> The serial peripheral interface (SPI) and register structure provide a
> simple interface for data collection and configuration control.
> 
> These devices are similar to the ones already supported in the driver,
> with changes in the scales, timings and the max spi speed in burst
> mode.
> Also, they support delta angle and delta velocity readings in burst
> mode, for which support was added in the trigger handler.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

What is Nuno's relationship to this patch?  You are author and the sender
which is fine, but in that case you need to make Nuno's involvement explicit.
Perhaps a Co-developed-by or similar is appropriate?

> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
A few comments inline.  Biggest one is I'd like a clear statement of why you
can't do a burst of one type, then a burst of other.  My guess is that the
transition is very time consuming?  If so, that is fine, but you should be able
to let available_scan_masks handle the disjoint channel sets.

From what I recall a similar case was why that got added in the first place.
The uses in optimizing for devices that 'want' to grab lots of channels was
a later use case.

Jonathan



>  
>  static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc)
> @@ -1200,7 +1355,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	struct adis16480 *st = iio_priv(indio_dev);
>  	struct adis *adis = &st->adis;
>  	struct device *dev = &adis->spi->dev;
> -	int ret, bit, offset, i = 0;
> +	int ret, bit, offset, i = 0, buff_offset = 0;
>  	__be16 *buffer;
>  	u32 crc;
>  	bool valid;
> @@ -1233,8 +1388,8 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	 * 16-bit responses containing the BURST_ID depending on the sclk. If
>  	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
>  	 * we have only one. To manage that variation, we use the transition from the
> -	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If
> -	 * we not find this variation in the first 4 segments, then the data should
> +	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3.
> +	 * If we not find this variation in the first 4 segments, then the data should
>  	 * not be valid.
>  	 */
>  	buffer = adis->buffer;
> @@ -1242,7 +1397,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  		u16 curr = be16_to_cpu(buffer[offset]);
>  		u16 next = be16_to_cpu(buffer[offset + 1]);
>  
> -		if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) {
> +		if (curr == st->burst_id && next != st->burst_id) {
>  			offset++;
>  			break;
>  		}
> @@ -1269,11 +1424,22 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  		switch (bit) {
>  		case ADIS16480_SCAN_TEMP:
>  			st->data[i++] = buffer[offset + 1];
> +			/*
> +			 * The temperature channel has 16-bit storage size.
> +			 * We need to perform the padding to have the buffer
> +			 * elements naturally aligned in case there are any
> +			 * 32-bit storage size channels enabled which have a
> +			 * scan index higher than the temperature channel scan
> +			 * index.
> +			 */
> +			if (*indio_dev->active_scan_mask &
> +			    GENMASK(ADIS16480_SCAN_DELTVEL_Z, ADIS16480_SCAN_DELTANG_X))
> +				st->data[i++] = 0;

I think it is harmless to always do this. If there is no data after this channel
then we are writing data that won't be copied anywhere. If there is data after
it is needed.

So I think you can drop the condition but do add a comment on it being necessary
if there is data afterwards and harmless if there isn't.
	

>  			break;
>  		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
>  			/* The lower register data is sequenced first */
> -			st->data[i++] = buffer[2 * bit + offset + 3];
> -			st->data[i++] = buffer[2 * bit + offset + 2];
> +			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3];
> +			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2];
>  			break;
>  		}
>  	}
> @@ -1285,10 +1451,38 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int adis16480_update_scan_mode(struct iio_dev *indio_dev,
> +				      const unsigned long *scan_mask)
> +{
> +	u16 en;
> +	int ret;
> +	struct adis16480 *st = iio_priv(indio_dev);
> +
> +	if (st->chip_info->has_burst_delta_data) {
> +		if ((*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) &&
> +		    (*scan_mask & ADIS16545_BURST_DATA_SEL_1_CHN_MASK))
> +			return -EINVAL;

Use available scan_masks to enforce this.  That can have mutually exclusive
sets like you have here.

However, what stops 2 burst reads - so do them one after the other if needed.
You'd still want available_scan_masks to have the subsets though but added
to that the combination of the two.

> +		if (*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) {
> +			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 0);
> +			st->burst_id = 0xA5A5;

Give the magic value a name via a define.

> +		} else {
> +			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 1);
> +			st->burst_id = 0xC3C3;
> +		}
> +
> +		ret = __adis_update_bits(&st->adis, ADIS16480_REG_CONFIG,
> +					 ADIS16545_BURST_DATA_SEL_MASK, en);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return adis_update_scan_mode(indio_dev, scan_mask);
> +}

>  
> @@ -1498,6 +1692,8 @@ static int adis16480_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	st->burst_id = ADIS16495_BURST_ID;

Why this default? Probably wants an explanatory comment.

> +

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Nuno Sá]

On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:
> On Tue, 23 Apr 2024 11:42:09 +0300
> Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> 
> > The ADIS16545 and ADIS16547 are a complete inertial system that
> > includes a triaxis gyroscope and a triaxis accelerometer.
> > The serial peripheral interface (SPI) and register structure provide a
> > simple interface for data collection and configuration control.
> > 
> > These devices are similar to the ones already supported in the driver,
> > with changes in the scales, timings and the max spi speed in burst
> > mode.
> > Also, they support delta angle and delta velocity readings in burst
> > mode, for which support was added in the trigger handler.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> What is Nuno's relationship to this patch?  You are author and the sender
> which is fine, but in that case you need to make Nuno's involvement explicit.
> Perhaps a Co-developed-by or similar is appropriate?
> 
> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> A few comments inline.  Biggest one is I'd like a clear statement of why you
> can't do a burst of one type, then a burst of other.  My guess is that the
> transition is very time consuming?  If so, that is fine, but you should be
> able
> to let available_scan_masks handle the disjoint channel sets.

Yeah, the burst message is a special spi transfer that brings you all of the
channels data at once but for the accel/gyro you need to explicitly configure
the chip to either give you the "normal vs "delta" readings. Re-configuring the
chip and then do another burst would destroy performance I think. We could do
the manual readings as we do in adis16475 for chips not supporting burst32. But
in the burst32 case those manual readings should be minimal while in here we
could have to do 6 of them which could also be very time consuming...

Now, why we don't use available_scan_masks is something I can't really remember
but this implementation goes in line with what we have in the adis16475 driver.

- Nuno Sá

RE: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Gradinariu, Ramona]

> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Monday, April 29, 2024 10:59 AM
> To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> <ramona.bolboaca13@gmail.com>
> Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org;
> Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> <Nuno.Sa@analog.com>
> Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
> 
> [External]
> 
> On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:
> > On Tue, 23 Apr 2024 11:42:09 +0300
> > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> >
> > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > The serial peripheral interface (SPI) and register structure provide a
> > > simple interface for data collection and configuration control.
> > >
> > > These devices are similar to the ones already supported in the driver,
> > > with changes in the scales, timings and the max spi speed in burst
> > > mode.
> > > Also, they support delta angle and delta velocity readings in burst
> > > mode, for which support was added in the trigger handler.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> >
> > What is Nuno's relationship to this patch?  You are author and the sender
> > which is fine, but in that case you need to make Nuno's involvement explicit.
> > Perhaps a Co-developed-by or similar is appropriate?
> >
> > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> > A few comments inline.  Biggest one is I'd like a clear statement of why you
> > can't do a burst of one type, then a burst of other.  My guess is that the
> > transition is very time consuming?  If so, that is fine, but you should be
> > able
> > to let available_scan_masks handle the disjoint channel sets.
> 
> Yeah, the burst message is a special spi transfer that brings you all of the
> channels data at once but for the accel/gyro you need to explicitly configure
> the chip to either give you the "normal vs "delta" readings. Re-configuring the
> chip and then do another burst would destroy performance I think. We could
> do
> the manual readings as we do in adis16475 for chips not supporting burst32.
> But
> in the burst32 case those manual readings should be minimal while in here we
> could have to do 6 of them which could also be very time consuming...
> 
> Now, why we don't use available_scan_masks is something I can't really
> remember
> but this implementation goes in line with what we have in the adis16475
> driver.
> 
> - Nuno Sá
> 

Thank you Nuno for all the additional explanations.
Regarding the use of available_scan_masks, the idea is to have any possible
combination for accel, gyro, temp and timestamp channels or delta angle, delta 
velocity, temp and  timestamp channels. There are a lot of combinations for 
this and it does not seem like a good idea to write them all manually. That is 
why adis16480_update_scan_mode is used for checking the correct channels 
selection.

Ramona G.

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Jonathan Cameron]

On Mon, 29 Apr 2024 13:17:42 +0000
"Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:

> > -----Original Message-----
> > From: Nuno Sá <noname.nuno@gmail.com>
> > Sent: Monday, April 29, 2024 10:59 AM
> > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > <ramona.bolboaca13@gmail.com>
> > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org;
> > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > <Nuno.Sa@analog.com>
> > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
> > 
> > [External]
> > 
> > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:  
> > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > >  
> > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > The serial peripheral interface (SPI) and register structure provide a
> > > > simple interface for data collection and configuration control.
> > > >
> > > > These devices are similar to the ones already supported in the driver,
> > > > with changes in the scales, timings and the max spi speed in burst
> > > > mode.
> > > > Also, they support delta angle and delta velocity readings in burst
> > > > mode, for which support was added in the trigger handler.
> > > >
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > >
> > > What is Nuno's relationship to this patch?  You are author and the sender
> > > which is fine, but in that case you need to make Nuno's involvement explicit.
> > > Perhaps a Co-developed-by or similar is appropriate?
> > >  
> > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>  
> > > A few comments inline.  Biggest one is I'd like a clear statement of why you
> > > can't do a burst of one type, then a burst of other.  My guess is that the
> > > transition is very time consuming?  If so, that is fine, but you should be
> > > able
> > > to let available_scan_masks handle the disjoint channel sets.  
> > 
> > Yeah, the burst message is a special spi transfer that brings you all of the
> > channels data at once but for the accel/gyro you need to explicitly configure
> > the chip to either give you the "normal vs "delta" readings. Re-configuring the
> > chip and then do another burst would destroy performance I think. We could
> > do
> > the manual readings as we do in adis16475 for chips not supporting burst32.
> > But
> > in the burst32 case those manual readings should be minimal while in here we
> > could have to do 6 of them which could also be very time consuming...
> > 
> > Now, why we don't use available_scan_masks is something I can't really
> > remember
> > but this implementation goes in line with what we have in the adis16475
> > driver.
> > 
> > - Nuno Sá
> >   
> 
> Thank you Nuno for all the additional explanations.
> Regarding the use of available_scan_masks, the idea is to have any possible
> combination for accel, gyro, temp and timestamp channels or delta angle, delta 
> velocity, temp and  timestamp channels. There are a lot of combinations for 
> this and it does not seem like a good idea to write them all manually. That is 
> why adis16480_update_scan_mode is used for checking the correct channels 
> selection.

If you are using bursts, the data is getting read anyway - which is the main
cost here. The real question becomes what are you actually saving by supporting all
the combinations of the the two subsets of channels in the pollfunc?
Currently you have to pick the channels out and repack them, if pushing them all
looks to me like a mempcy and a single value being set (unconditionally).

Then it's a question of what the overhead of the channel demux in the core is.
What you pass out of the driver via iio_push_to_buffers*()
is not what ends up in the buffer if you allow the IIO core to do data demuxing
for you - that is enabled by providing available_scan_masks.  At buffer
start up the demux code computes a fairly optimal set of copies to repack
the incoming data to match with what channels the consumer (here probably
the kfifo on the way to userspace) is expecting.

That demux adds a small overhead but it should be small as long
as the channels wanted aren't pathological (i.e. every other one).

Advantage is the driver ends up simpler and in the common case of turn
on all the channels (why else did you buy a device with those measurements
if you didn't want them!) the demux is zerocopy so effectively free which
is not going to be the case for the bitmap walk and element copy in the
driver.

Jonathan

> 
> Ramona G.

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Nuno Sá]

On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:
> On Mon, 29 Apr 2024 13:17:42 +0000
> "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Nuno Sá <noname.nuno@gmail.com>
> > > Sent: Monday, April 29, 2024 10:59 AM
> > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > <ramona.bolboaca13@gmail.com>
> > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org;
> > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > <Nuno.Sa@analog.com>
> > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
> > > 
> > > [External]
> > > 
> > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:  
> > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > >  
> > > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > The serial peripheral interface (SPI) and register structure provide a
> > > > > simple interface for data collection and configuration control.
> > > > > 
> > > > > These devices are similar to the ones already supported in the driver,
> > > > > with changes in the scales, timings and the max spi speed in burst
> > > > > mode.
> > > > > Also, they support delta angle and delta velocity readings in burst
> > > > > mode, for which support was added in the trigger handler.
> > > > > 
> > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > > > 
> > > > What is Nuno's relationship to this patch?  You are author and the sender
> > > > which is fine, but in that case you need to make Nuno's involvement explicit.
> > > > Perhaps a Co-developed-by or similar is appropriate?
> > > >  
> > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>  
> > > > A few comments inline.  Biggest one is I'd like a clear statement of why you
> > > > can't do a burst of one type, then a burst of other.  My guess is that the
> > > > transition is very time consuming?  If so, that is fine, but you should be
> > > > able
> > > > to let available_scan_masks handle the disjoint channel sets.  
> > > 
> > > Yeah, the burst message is a special spi transfer that brings you all of the
> > > channels data at once but for the accel/gyro you need to explicitly configure
> > > the chip to either give you the "normal vs "delta" readings. Re-configuring the
> > > chip and then do another burst would destroy performance I think. We could
> > > do
> > > the manual readings as we do in adis16475 for chips not supporting burst32.
> > > But
> > > in the burst32 case those manual readings should be minimal while in here we
> > > could have to do 6 of them which could also be very time consuming...
> > > 
> > > Now, why we don't use available_scan_masks is something I can't really
> > > remember
> > > but this implementation goes in line with what we have in the adis16475
> > > driver.
> > > 
> > > - Nuno Sá
> > >   
> > 
> > Thank you Nuno for all the additional explanations.
> > Regarding the use of available_scan_masks, the idea is to have any possible
> > combination for accel, gyro, temp and timestamp channels or delta angle, delta 
> > velocity, temp and  timestamp channels. There are a lot of combinations for 
> > this and it does not seem like a good idea to write them all manually. That is 
> > why adis16480_update_scan_mode is used for checking the correct channels 
> > selection.
> 
> If you are using bursts, the data is getting read anyway - which is the main
> cost here. The real question becomes what are you actually saving by supporting all
> the combinations of the the two subsets of channels in the pollfunc?
> Currently you have to pick the channels out and repack them, if pushing them all
> looks to me like a mempcy and a single value being set (unconditionally).

> Then it's a question of what the overhead of the channel demux in the core is.
> What you pass out of the driver via iio_push_to_buffers*()
> is not what ends up in the buffer if you allow the IIO core to do data demuxing
> for you - that is enabled by providing available_scan_masks.  At buffer
> start up the demux code computes a fairly optimal set of copies to repack
> the incoming data to match with what channels the consumer (here probably
> the kfifo on the way to userspace) is expecting.
> 
> That demux adds a small overhead but it should be small as long
> as the channels wanted aren't pathological (i.e. every other one).
> 
> Advantage is the driver ends up simpler and in the common case of turn
> on all the channels (why else did you buy a device with those measurements
> if you didn't want them!) the demux is zerocopy so effectively free which
> is not going to be the case for the bitmap walk and element copy in the
> driver.
> 

Maybe my younger me was smarter but reading again the validation of the scan mask
code (when available_scan_masks is available), I'm not sure why we're not using them.
I think that having one mask with delta values + temperature and another one with
normal + temperature would be enough for what we want in here. The code in
adis16480_update_scan_mode() could then be simpler I think.

Now, what I'm still not following is the straight memcpy(). I may be missing
something but the demux code only appears to kick in when we have compound masks
resulting of multiple buffers being enabled. So I'm not seeing how we can get away
without picking the channels and place them correctly in the buffer passed to IIO?
What we could do in the future (for a similar device) is to maybe have a fastpath in
the handler. Something like:

if (bitmap_full(scan_mask, masklength)) {
	memcpy(iio_buff, burst + data_off, size);
	goto push_to_iio;
}

Right now we would always have to do some "manual" work as the temperature scan index
does not match the position on the received burst data.

Some devices with the burst32 (which I think do not exist in this driver) would also
make the plain memcpy() harder.

- Nuno Sá

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Jonathan Cameron]

On Thu, 02 May 2024 13:31:55 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:
> > On Mon, 29 Apr 2024 13:17:42 +0000
> > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Nuno Sá <noname.nuno@gmail.com>
> > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > > <ramona.bolboaca13@gmail.com>
> > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org;
> > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > > <Nuno.Sa@analog.com>
> > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
> > > > 
> > > > [External]
> > > > 
> > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:    
> > > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > >    
> > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > > The serial peripheral interface (SPI) and register structure provide a
> > > > > > simple interface for data collection and configuration control.
> > > > > > 
> > > > > > These devices are similar to the ones already supported in the driver,
> > > > > > with changes in the scales, timings and the max spi speed in burst
> > > > > > mode.
> > > > > > Also, they support delta angle and delta velocity readings in burst
> > > > > > mode, for which support was added in the trigger handler.
> > > > > > 
> > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>    
> > > > > 
> > > > > What is Nuno's relationship to this patch?  You are author and the sender
> > > > > which is fine, but in that case you need to make Nuno's involvement explicit.
> > > > > Perhaps a Co-developed-by or similar is appropriate?
> > > > >    
> > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>    
> > > > > A few comments inline.  Biggest one is I'd like a clear statement of why you
> > > > > can't do a burst of one type, then a burst of other.  My guess is that the
> > > > > transition is very time consuming?  If so, that is fine, but you should be
> > > > > able
> > > > > to let available_scan_masks handle the disjoint channel sets.    
> > > > 
> > > > Yeah, the burst message is a special spi transfer that brings you all of the
> > > > channels data at once but for the accel/gyro you need to explicitly configure
> > > > the chip to either give you the "normal vs "delta" readings. Re-configuring the
> > > > chip and then do another burst would destroy performance I think. We could
> > > > do
> > > > the manual readings as we do in adis16475 for chips not supporting burst32.
> > > > But
> > > > in the burst32 case those manual readings should be minimal while in here we
> > > > could have to do 6 of them which could also be very time consuming...
> > > > 
> > > > Now, why we don't use available_scan_masks is something I can't really
> > > > remember
> > > > but this implementation goes in line with what we have in the adis16475
> > > > driver.
> > > > 
> > > > - Nuno Sá
> > > >     
> > > 
> > > Thank you Nuno for all the additional explanations.
> > > Regarding the use of available_scan_masks, the idea is to have any possible
> > > combination for accel, gyro, temp and timestamp channels or delta angle, delta 
> > > velocity, temp and  timestamp channels. There are a lot of combinations for 
> > > this and it does not seem like a good idea to write them all manually. That is 
> > > why adis16480_update_scan_mode is used for checking the correct channels 
> > > selection.  
> > 
> > If you are using bursts, the data is getting read anyway - which is the main
> > cost here. The real question becomes what are you actually saving by supporting all
> > the combinations of the the two subsets of channels in the pollfunc?
> > Currently you have to pick the channels out and repack them, if pushing them all
> > looks to me like a mempcy and a single value being set (unconditionally).  
> 
> > Then it's a question of what the overhead of the channel demux in the core is.
> > What you pass out of the driver via iio_push_to_buffers*()
> > is not what ends up in the buffer if you allow the IIO core to do data demuxing
> > for you - that is enabled by providing available_scan_masks.  At buffer
> > start up the demux code computes a fairly optimal set of copies to repack
> > the incoming data to match with what channels the consumer (here probably
> > the kfifo on the way to userspace) is expecting.
> > 
> > That demux adds a small overhead but it should be small as long
> > as the channels wanted aren't pathological (i.e. every other one).
> > 
> > Advantage is the driver ends up simpler and in the common case of turn
> > on all the channels (why else did you buy a device with those measurements
> > if you didn't want them!) the demux is zerocopy so effectively free which
> > is not going to be the case for the bitmap walk and element copy in the
> > driver.
> >   
> 
> Maybe my younger me was smarter but reading again the validation of the scan mask
> code (when available_scan_masks is available), I'm not sure why we're not using them.
> I think that having one mask with delta values + temperature and another one with
> normal + temperature would be enough for what we want in here. The code in
> adis16480_update_scan_mode() could then be simpler I think.
> 
> Now, what I'm still not following is the straight memcpy(). I may be missing
> something but the demux code only appears to kick in when we have compound masks
> resulting of multiple buffers being enabled. So I'm not seeing how we can get away
> without picking the channels and place them correctly in the buffer passed to IIO?

It runs whenever the enabled mask requested (the channels that are enabled) is
different from the active_scan_mask. It only sends channels in one
direction if there is only one user but it only sends the ones enabled by that consumer.
It's called unconditionally from iio_enable_buffers()

That iterates over all enabled buffers (often there is only 1)

and then checks if the active scan mask is the same as the one for this buffer.
https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006

The setup for whether to find a superset from available_scan_masks is here
https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928

Key is that if it happens to match, then we don't actually do anything in the demux.
It just gets passed straight on through.  That does the fast path you mention below.

> What we could do in the future (for a similar device) is to maybe have a fastpath in
> the handler. Something like:
> 
> if (bitmap_full(scan_mask, masklength)) {
> 	memcpy(iio_buff, burst + data_off, size);
> 	goto push_to_iio;
> }
> 
> Right now we would always have to do some "manual" work as the temperature scan index
> does not match the position on the received burst data.

Agreed -that one is needed to shuffle the temperature to the right place.


> 
> Some devices with the burst32 (which I think do not exist in this driver) would also
> make the plain memcpy() harder.
> 
> - Nuno Sá
> 

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Nuno Sá]

On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote:
> On Thu, 02 May 2024 13:31:55 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:
> > > On Mon, 29 Apr 2024 13:17:42 +0000
> > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
> > >   
> > > > > -----Original Message-----
> > > > > From: Nuno Sá <noname.nuno@gmail.com>
> > > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > > > <ramona.bolboaca13@gmail.com>
> > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > robh@kernel.org;
> > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > > > <Nuno.Sa@analog.com>
> > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7
> > > > > families
> > > > > 
> > > > > [External]
> > > > > 
> > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:    
> > > > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > > >    
> > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > > > The serial peripheral interface (SPI) and register structure
> > > > > > > provide a
> > > > > > > simple interface for data collection and configuration control.
> > > > > > > 
> > > > > > > These devices are similar to the ones already supported in the
> > > > > > > driver,
> > > > > > > with changes in the scales, timings and the max spi speed in burst
> > > > > > > mode.
> > > > > > > Also, they support delta angle and delta velocity readings in
> > > > > > > burst
> > > > > > > mode, for which support was added in the trigger handler.
> > > > > > > 
> > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>    
> > > > > > 
> > > > > > What is Nuno's relationship to this patch?  You are author and the
> > > > > > sender
> > > > > > which is fine, but in that case you need to make Nuno's involvement
> > > > > > explicit.
> > > > > > Perhaps a Co-developed-by or similar is appropriate?
> > > > > >    
> > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>   
> > > > > > A few comments inline.  Biggest one is I'd like a clear statement of
> > > > > > why you
> > > > > > can't do a burst of one type, then a burst of other.  My guess is
> > > > > > that the
> > > > > > transition is very time consuming?  If so, that is fine, but you
> > > > > > should be
> > > > > > able
> > > > > > to let available_scan_masks handle the disjoint channel sets.    
> > > > > 
> > > > > Yeah, the burst message is a special spi transfer that brings you all
> > > > > of the
> > > > > channels data at once but for the accel/gyro you need to explicitly
> > > > > configure
> > > > > the chip to either give you the "normal vs "delta" readings. Re-
> > > > > configuring the
> > > > > chip and then do another burst would destroy performance I think. We
> > > > > could
> > > > > do
> > > > > the manual readings as we do in adis16475 for chips not supporting
> > > > > burst32.
> > > > > But
> > > > > in the burst32 case those manual readings should be minimal while in
> > > > > here we
> > > > > could have to do 6 of them which could also be very time consuming...
> > > > > 
> > > > > Now, why we don't use available_scan_masks is something I can't really
> > > > > remember
> > > > > but this implementation goes in line with what we have in the
> > > > > adis16475
> > > > > driver.
> > > > > 
> > > > > - Nuno Sá
> > > > >     
> > > > 
> > > > Thank you Nuno for all the additional explanations.
> > > > Regarding the use of available_scan_masks, the idea is to have any
> > > > possible
> > > > combination for accel, gyro, temp and timestamp channels or delta angle,
> > > > delta 
> > > > velocity, temp and  timestamp channels. There are a lot of combinations
> > > > for 
> > > > this and it does not seem like a good idea to write them all manually.
> > > > That is 
> > > > why adis16480_update_scan_mode is used for checking the correct channels
> > > > selection.  
> > > 
> > > If you are using bursts, the data is getting read anyway - which is the
> > > main
> > > cost here. The real question becomes what are you actually saving by
> > > supporting all
> > > the combinations of the the two subsets of channels in the pollfunc?
> > > Currently you have to pick the channels out and repack them, if pushing
> > > them all
> > > looks to me like a mempcy and a single value being set (unconditionally). 
> > 
> > > Then it's a question of what the overhead of the channel demux in the core
> > > is.
> > > What you pass out of the driver via iio_push_to_buffers*()
> > > is not what ends up in the buffer if you allow the IIO core to do data
> > > demuxing
> > > for you - that is enabled by providing available_scan_masks.  At buffer
> > > start up the demux code computes a fairly optimal set of copies to repack
> > > the incoming data to match with what channels the consumer (here probably
> > > the kfifo on the way to userspace) is expecting.
> > > 
> > > That demux adds a small overhead but it should be small as long
> > > as the channels wanted aren't pathological (i.e. every other one).
> > > 
> > > Advantage is the driver ends up simpler and in the common case of turn
> > > on all the channels (why else did you buy a device with those measurements
> > > if you didn't want them!) the demux is zerocopy so effectively free which
> > > is not going to be the case for the bitmap walk and element copy in the
> > > driver.
> > >   
> > 
> > Maybe my younger me was smarter but reading again the validation of the scan
> > mask
> > code (when available_scan_masks is available), I'm not sure why we're not
> > using them.
> > I think that having one mask with delta values + temperature and another one
> > with
> > normal + temperature would be enough for what we want in here. The code in
> > adis16480_update_scan_mode() could then be simpler I think.
> > 
> > Now, what I'm still not following is the straight memcpy(). I may be missing
> > something but the demux code only appears to kick in when we have compound
> > masks
> > resulting of multiple buffers being enabled. So I'm not seeing how we can
> > get away
> > without picking the channels and place them correctly in the buffer passed
> > to IIO?
> 
> It runs whenever the enabled mask requested (the channels that are enabled) is
> different from the active_scan_mask. It only sends channels in one
> direction if there is only one user but it only sends the ones enabled by that
> consumer.
> It's called unconditionally from iio_enable_buffers()
> 
> That iterates over all enabled buffers (often there is only 1)
> 
> and then checks if the active scan mask is the same as the one for this
> buffer.
> https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006
> 
> The setup for whether to find a superset from available_scan_masks is here
> https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928
> 
> Key is that if it happens to match, then we don't actually do anything in the
> demux.
> It just gets passed straight on through.  That does the fast path you mention
> below.

Ahh got it... May failure was not realizing that iio_scan_mask_match() returns
the available masks so I was assuming that the bitmap_equal() check would only
differ when multiple buffers are enabled.

Oh well, I think then this should work... I'm not sure it will be more
performant for the case where we don't enable all the channels because the demux
is a linked list which is far from being a performance friend (maybe we can do
some trials with something like xarray...). Still, for this to work the buffer
we pass into IIO has to be bigger than it needs to be (so bigger memset())
because, AFAIU, we will have to pass on all the scan elements and, as I said,
the burst data will either have delta or normal measurements (not both). I guess
the code will still look simpler so if there's no visible difference in
performance I'm fine with the change. I guess Ramona can give it a try to see
how it looks like.

- Nuno Sá
> 

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Jonathan Cameron]

On Fri, 03 May 2024 08:09:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

Resend as the to / cc entries were garbled. No idea why!

> On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote:
> > On Thu, 02 May 2024 13:31:55 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:  
> > > > On Mon, 29 Apr 2024 13:17:42 +0000
> > > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Nuno Sá <noname.nuno@gmail.com>
> > > > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > > > > <ramona.bolboaca13@gmail.com>
> > > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > > robh@kernel.org;
> > > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > > > > <Nuno.Sa@analog.com>
> > > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7
> > > > > > families
> > > > > > 
> > > > > > [External]
> > > > > > 
> > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:      
> > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > > > >      
> > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > > > > The serial peripheral interface (SPI) and register structure
> > > > > > > > provide a
> > > > > > > > simple interface for data collection and configuration control.
> > > > > > > > 
> > > > > > > > These devices are similar to the ones already supported in the
> > > > > > > > driver,
> > > > > > > > with changes in the scales, timings and the max spi speed in burst
> > > > > > > > mode.
> > > > > > > > Also, they support delta angle and delta velocity readings in
> > > > > > > > burst
> > > > > > > > mode, for which support was added in the trigger handler.
> > > > > > > > 
> > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>      
> > > > > > > 
> > > > > > > What is Nuno's relationship to this patch?  You are author and the
> > > > > > > sender
> > > > > > > which is fine, but in that case you need to make Nuno's involvement
> > > > > > > explicit.
> > > > > > > Perhaps a Co-developed-by or similar is appropriate?
> > > > > > >      
> > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>     
> > > > > > > A few comments inline.  Biggest one is I'd like a clear statement of
> > > > > > > why you
> > > > > > > can't do a burst of one type, then a burst of other.  My guess is
> > > > > > > that the
> > > > > > > transition is very time consuming?  If so, that is fine, but you
> > > > > > > should be
> > > > > > > able
> > > > > > > to let available_scan_masks handle the disjoint channel sets.      
> > > > > > 
> > > > > > Yeah, the burst message is a special spi transfer that brings you all
> > > > > > of the
> > > > > > channels data at once but for the accel/gyro you need to explicitly
> > > > > > configure
> > > > > > the chip to either give you the "normal vs "delta" readings. Re-
> > > > > > configuring the
> > > > > > chip and then do another burst would destroy performance I think. We
> > > > > > could
> > > > > > do
> > > > > > the manual readings as we do in adis16475 for chips not supporting
> > > > > > burst32.
> > > > > > But
> > > > > > in the burst32 case those manual readings should be minimal while in
> > > > > > here we
> > > > > > could have to do 6 of them which could also be very time consuming...
> > > > > > 
> > > > > > Now, why we don't use available_scan_masks is something I can't really
> > > > > > remember
> > > > > > but this implementation goes in line with what we have in the
> > > > > > adis16475
> > > > > > driver.
> > > > > > 
> > > > > > - Nuno Sá
> > > > > >       
> > > > > 
> > > > > Thank you Nuno for all the additional explanations.
> > > > > Regarding the use of available_scan_masks, the idea is to have any
> > > > > possible
> > > > > combination for accel, gyro, temp and timestamp channels or delta angle,
> > > > > delta 
> > > > > velocity, temp and  timestamp channels. There are a lot of combinations
> > > > > for 
> > > > > this and it does not seem like a good idea to write them all manually.
> > > > > That is 
> > > > > why adis16480_update_scan_mode is used for checking the correct channels
> > > > > selection.    
> > > > 
> > > > If you are using bursts, the data is getting read anyway - which is the
> > > > main
> > > > cost here. The real question becomes what are you actually saving by
> > > > supporting all
> > > > the combinations of the the two subsets of channels in the pollfunc?
> > > > Currently you have to pick the channels out and repack them, if pushing
> > > > them all
> > > > looks to me like a mempcy and a single value being set (unconditionally).   
> > >   
> > > > Then it's a question of what the overhead of the channel demux in the core
> > > > is.
> > > > What you pass out of the driver via iio_push_to_buffers*()
> > > > is not what ends up in the buffer if you allow the IIO core to do data
> > > > demuxing
> > > > for you - that is enabled by providing available_scan_masks.  At buffer
> > > > start up the demux code computes a fairly optimal set of copies to repack
> > > > the incoming data to match with what channels the consumer (here probably
> > > > the kfifo on the way to userspace) is expecting.
> > > > 
> > > > That demux adds a small overhead but it should be small as long
> > > > as the channels wanted aren't pathological (i.e. every other one).
> > > > 
> > > > Advantage is the driver ends up simpler and in the common case of turn
> > > > on all the channels (why else did you buy a device with those measurements
> > > > if you didn't want them!) the demux is zerocopy so effectively free which
> > > > is not going to be the case for the bitmap walk and element copy in the
> > > > driver.
> > > >     
> > > 
> > > Maybe my younger me was smarter but reading again the validation of the scan
> > > mask
> > > code (when available_scan_masks is available), I'm not sure why we're not
> > > using them.
> > > I think that having one mask with delta values + temperature and another one
> > > with
> > > normal + temperature would be enough for what we want in here. The code in
> > > adis16480_update_scan_mode() could then be simpler I think.
> > > 
> > > Now, what I'm still not following is the straight memcpy(). I may be missing
> > > something but the demux code only appears to kick in when we have compound
> > > masks
> > > resulting of multiple buffers being enabled. So I'm not seeing how we can
> > > get away
> > > without picking the channels and place them correctly in the buffer passed
> > > to IIO?  
> > 
> > It runs whenever the enabled mask requested (the channels that are enabled) is
> > different from the active_scan_mask. It only sends channels in one
> > direction if there is only one user but it only sends the ones enabled by that
> > consumer.
> > It's called unconditionally from iio_enable_buffers()
> > 
> > That iterates over all enabled buffers (often there is only 1)
> > 
> > and then checks if the active scan mask is the same as the one for this
> > buffer.
> > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006
> > 
> > The setup for whether to find a superset from available_scan_masks is here
> > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928
> > 
> > Key is that if it happens to match, then we don't actually do anything in the
> > demux.
> > It just gets passed straight on through.  That does the fast path you mention
> > below.  
> 
> Ahh got it... May failure was not realizing that iio_scan_mask_match() returns
> the available masks so I was assuming that the bitmap_equal() check would only
> differ when multiple buffers are enabled.
> 
> Oh well, I think then this should work... I'm not sure it will be more
> performant for the case where we don't enable all the channels because the demux
> is a linked list which is far from being a performance friend (maybe we can do
> some trials with something like xarray...). Still, for this to work the buffer
> we pass into IIO has to be bigger than it needs to be (so bigger memset())
> because, AFAIU, we will have to pass on all the scan elements and, as I said,
> the burst data will either have delta or normal measurements (not both). I guess
> the code will still look simpler so if there's no visible difference in
> performance I'm fine with the change. I guess Ramona can give it a try to see
> how it looks like.

Would be interesting to look at the performance of that code, but the
real question is what channels do users typically enabled. If they enabled
a full set (and I suspect most do) then that code doesn't nothing at all.

Original thinking was that the non common case didn't really matter much.
If it is worth optimizing I'd suggest a double pass (or cheat - see later)
1st pass works out number of elements, 2nd just saves them in a nice
linear array.  It's typically small however, so maybe just allocate a linear
array big enough for the worst case.

Jonathan

> 
> - Nuno Sá
> >   
> 
> 

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Nuno Sá]

On Fri, 2024-05-03 at 09:42 +0100, Jonathan Cameron wrote:
> On Fri, 03 May 2024 08:09:29 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> Resend as the to / cc entries were garbled. No idea why!
> 
> > On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote:
> > > On Thu, 02 May 2024 13:31:55 +0200
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 29 Apr 2024 13:17:42 +0000
> > > > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
> > > > >     
> > > > > > > -----Original Message-----
> > > > > > > From: Nuno Sá <noname.nuno@gmail.com>
> > > > > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > > > > > <ramona.bolboaca13@gmail.com>
> > > > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org;
> > > > > > > linux-
> > > > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > > > robh@kernel.org;
> > > > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > > > > > <Nuno.Sa@analog.com>
> > > > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for
> > > > > > > adis16545/7
> > > > > > > families
> > > > > > > 
> > > > > > > [External]
> > > > > > > 
> > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:      
> > > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > > > > >      
> > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system
> > > > > > > > > that
> > > > > > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > > > > > The serial peripheral interface (SPI) and register structure
> > > > > > > > > provide a
> > > > > > > > > simple interface for data collection and configuration
> > > > > > > > > control.
> > > > > > > > > 
> > > > > > > > > These devices are similar to the ones already supported in the
> > > > > > > > > driver,
> > > > > > > > > with changes in the scales, timings and the max spi speed in
> > > > > > > > > burst
> > > > > > > > > mode.
> > > > > > > > > Also, they support delta angle and delta velocity readings in
> > > > > > > > > burst
> > > > > > > > > mode, for which support was added in the trigger handler.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>      
> > > > > > > > 
> > > > > > > > What is Nuno's relationship to this patch?  You are author and
> > > > > > > > the
> > > > > > > > sender
> > > > > > > > which is fine, but in that case you need to make Nuno's
> > > > > > > > involvement
> > > > > > > > explicit.
> > > > > > > > Perhaps a Co-developed-by or similar is appropriate?
> > > > > > > >      
> > > > > > > > > Signed-off-by: Ramona Gradinariu
> > > > > > > > > <ramona.gradinariu@analog.com>     
> > > > > > > > A few comments inline.  Biggest one is I'd like a clear
> > > > > > > > statement of
> > > > > > > > why you
> > > > > > > > can't do a burst of one type, then a burst of other.  My guess
> > > > > > > > is
> > > > > > > > that the
> > > > > > > > transition is very time consuming?  If so, that is fine, but you
> > > > > > > > should be
> > > > > > > > able
> > > > > > > > to let available_scan_masks handle the disjoint channel
> > > > > > > > sets.      
> > > > > > > 
> > > > > > > Yeah, the burst message is a special spi transfer that brings you
> > > > > > > all
> > > > > > > of the
> > > > > > > channels data at once but for the accel/gyro you need to
> > > > > > > explicitly
> > > > > > > configure
> > > > > > > the chip to either give you the "normal vs "delta" readings. Re-
> > > > > > > configuring the
> > > > > > > chip and then do another burst would destroy performance I think.
> > > > > > > We
> > > > > > > could
> > > > > > > do
> > > > > > > the manual readings as we do in adis16475 for chips not supporting
> > > > > > > burst32.
> > > > > > > But
> > > > > > > in the burst32 case those manual readings should be minimal while
> > > > > > > in
> > > > > > > here we
> > > > > > > could have to do 6 of them which could also be very time
> > > > > > > consuming...
> > > > > > > 
> > > > > > > Now, why we don't use available_scan_masks is something I can't
> > > > > > > really
> > > > > > > remember
> > > > > > > but this implementation goes in line with what we have in the
> > > > > > > adis16475
> > > > > > > driver.
> > > > > > > 
> > > > > > > - Nuno Sá
> > > > > > >       
> > > > > > 
> > > > > > Thank you Nuno for all the additional explanations.
> > > > > > Regarding the use of available_scan_masks, the idea is to have any
> > > > > > possible
> > > > > > combination for accel, gyro, temp and timestamp channels or delta
> > > > > > angle,
> > > > > > delta 
> > > > > > velocity, temp and  timestamp channels. There are a lot of
> > > > > > combinations
> > > > > > for 
> > > > > > this and it does not seem like a good idea to write them all
> > > > > > manually.
> > > > > > That is 
> > > > > > why adis16480_update_scan_mode is used for checking the correct
> > > > > > channels
> > > > > > selection.    
> > > > > 
> > > > > If you are using bursts, the data is getting read anyway - which is
> > > > > the
> > > > > main
> > > > > cost here. The real question becomes what are you actually saving by
> > > > > supporting all
> > > > > the combinations of the the two subsets of channels in the pollfunc?
> > > > > Currently you have to pick the channels out and repack them, if
> > > > > pushing
> > > > > them all
> > > > > looks to me like a mempcy and a single value being set
> > > > > (unconditionally).   
> > > >   
> > > > > Then it's a question of what the overhead of the channel demux in the
> > > > > core
> > > > > is.
> > > > > What you pass out of the driver via iio_push_to_buffers*()
> > > > > is not what ends up in the buffer if you allow the IIO core to do data
> > > > > demuxing
> > > > > for you - that is enabled by providing available_scan_masks.  At
> > > > > buffer
> > > > > start up the demux code computes a fairly optimal set of copies to
> > > > > repack
> > > > > the incoming data to match with what channels the consumer (here
> > > > > probably
> > > > > the kfifo on the way to userspace) is expecting.
> > > > > 
> > > > > That demux adds a small overhead but it should be small as long
> > > > > as the channels wanted aren't pathological (i.e. every other one).
> > > > > 
> > > > > Advantage is the driver ends up simpler and in the common case of turn
> > > > > on all the channels (why else did you buy a device with those
> > > > > measurements
> > > > > if you didn't want them!) the demux is zerocopy so effectively free
> > > > > which
> > > > > is not going to be the case for the bitmap walk and element copy in
> > > > > the
> > > > > driver.
> > > > >     
> > > > 
> > > > Maybe my younger me was smarter but reading again the validation of the
> > > > scan
> > > > mask
> > > > code (when available_scan_masks is available), I'm not sure why we're
> > > > not
> > > > using them.
> > > > I think that having one mask with delta values + temperature and another
> > > > one
> > > > with
> > > > normal + temperature would be enough for what we want in here. The code
> > > > in
> > > > adis16480_update_scan_mode() could then be simpler I think.
> > > > 
> > > > Now, what I'm still not following is the straight memcpy(). I may be
> > > > missing
> > > > something but the demux code only appears to kick in when we have
> > > > compound
> > > > masks
> > > > resulting of multiple buffers being enabled. So I'm not seeing how we
> > > > can
> > > > get away
> > > > without picking the channels and place them correctly in the buffer
> > > > passed
> > > > to IIO?  
> > > 
> > > It runs whenever the enabled mask requested (the channels that are
> > > enabled) is
> > > different from the active_scan_mask. It only sends channels in one
> > > direction if there is only one user but it only sends the ones enabled by
> > > that
> > > consumer.
> > > It's called unconditionally from iio_enable_buffers()
> > > 
> > > That iterates over all enabled buffers (often there is only 1)
> > > 
> > > and then checks if the active scan mask is the same as the one for this
> > > buffer.
> > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006
> > > 
> > > The setup for whether to find a superset from available_scan_masks is here
> > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928
> > > 
> > > Key is that if it happens to match, then we don't actually do anything in
> > > the
> > > demux.
> > > It just gets passed straight on through.  That does the fast path you
> > > mention
> > > below.  
> > 
> > Ahh got it... May failure was not realizing that iio_scan_mask_match()
> > returns
> > the available masks so I was assuming that the bitmap_equal() check would
> > only
> > differ when multiple buffers are enabled.
> > 
> > Oh well, I think then this should work... I'm not sure it will be more
> > performant for the case where we don't enable all the channels because the
> > demux
> > is a linked list which is far from being a performance friend (maybe we can
> > do
> > some trials with something like xarray...). Still, for this to work the
> > buffer
> > we pass into IIO has to be bigger than it needs to be (so bigger memset())
> > because, AFAIU, we will have to pass on all the scan elements and, as I
> > said,
> > the burst data will either have delta or normal measurements (not both). I
> > guess
> > the code will still look simpler so if there's no visible difference in
> > performance I'm fine with the change. I guess Ramona can give it a try to
> > see
> > how it looks like.
> 
> Would be interesting to look at the performance of that code, but the
> real question is what channels do users typically enabled. If they enabled
> a full set (and I suspect most do) then that code doesn't nothing at all.
> 

The only channel that makes me doubt is the temperature one but yeah, I would
also expect  most users just enable them all...

> Original thinking was that the non common case didn't really matter much.
> If it is worth optimizing I'd suggest a double pass (or cheat - see later)
> 1st pass works out number of elements, 2nd just saves them in a nice
> linear array.  It's typically small however, so maybe just allocate a linear
> array big enough for the worst case.
> 

Yeah, linear array should also be fine and likely simpler. Maybe if I'm bored at
some point I'll run some experiments :)

- Nuno Sá

Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Nuno Sá]

On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:
> On Tue, 23 Apr 2024 11:42:09 +0300
> Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> 
> > The ADIS16545 and ADIS16547 are a complete inertial system that
> > includes a triaxis gyroscope and a triaxis accelerometer.
> > The serial peripheral interface (SPI) and register structure provide a
> > simple interface for data collection and configuration control.
> > 
> > These devices are similar to the ones already supported in the driver,
> > with changes in the scales, timings and the max spi speed in burst
> > mode.
> > Also, they support delta angle and delta velocity readings in burst
> > mode, for which support was added in the trigger handler.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> What is Nuno's relationship to this patch?  You are author and the sender
> which is fine, but in that case you need to make Nuno's involvement explicit.
> Perhaps a Co-developed-by or similar is appropriate?
> 

Ah and regarding this is that I guess Ramona just modified my internal patches
adding support for these parts long ago (when they were not public yet). Not
sure how much did she changed but a Co-developed-by looks the way to go...

- Nuno Sá
> 

[PATCH 5/5] docs: iio: add documentation for adis16480 driver

[Ramona Gradinariu]

Add documentation for adis16480 driver which describes the driver
device files and shows how the user may use the ABI for various
scenarios (configuration, measurement, etc.).

Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 Documentation/iio/adis16480.rst | 461 ++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst     |   1 +
 2 files changed, 462 insertions(+)
 create mode 100644 Documentation/iio/adis16480.rst

diff --git a/Documentation/iio/adis16480.rst b/Documentation/iio/adis16480.rst
new file mode 100644
index 000000000000..891865cfa712
--- /dev/null
+++ b/Documentation/iio/adis16480.rst
@@ -0,0 +1,461 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================
+ADIS16480 driver
+================
+
+This driver supports Analog Device's IMUs on SPI bus.
+
+1. Supported devices
+====================
+
+* `ADIS16375 <https://www.analog.com/ADIS16375>`_
+* `ADIS16480 <https://www.analog.com/ADIS16480>`_
+* `ADIS16485 <https://www.analog.com/ADIS16485>`_
+* `ADIS16488 <https://www.analog.com/ADIS16488>`_
+* `ADIS16490 <https://www.analog.com/ADIS16490>`_
+* `ADIS16495 <https://www.analog.com/ADIS16495>`_
+* `ADIS16497 <https://www.analog.com/ADIS16497>`_
+* `ADIS16545 <https://www.analog.com/ADIS16545>`_
+* `ADIS16547 <https://www.analog.com/ADIS16547>`_
+
+Each supported device is a complete inertial system that includes a triaxial
+gyroscope and a triaxial accelerometer. Each inertial sensor in device combines
+with signal conditioning that optimizes dynamic performance. The factory
+calibration characterizes each sensor for sensitivity, bias, and alignment. As
+a result, each sensor has its own dynamic compensation formulas that provide
+accurate sensor measurements.
+
+2. Device attributes
+====================
+
+Accelerometer, gyroscope measurements are always provided. Furthermore, the
+driver offers the capability to retrieve the delta angle and the delta velocity
+measurements computed by the device.
+
+The delta angle measurements represent a calculation of angular displacement
+between each sample update, while the delta velocity measurements represent a
+calculation of linear velocity change between each sample update.
+
+Finally, temperature data are provided which show a coarse measurement of
+the temperature inside of the IMU device. This data is most useful for
+monitoring relative changes in the thermal environment.
+
+ADIS16480 and ADIS16488 also provide access to barometric pressure data and
+triaxial magnetometer measurements.
+
+Each IIO device, has a device folder under ``/sys/bus/iio/devices/iio:deviceX``,
+where X is the IIO index of the device. Under these folders reside a set of
+device files, depending on the characteristics and features of the hardware
+device in questions. These files are consistently generalized and documented in
+the IIO ABI documentation.
+
+The following tables show the adis16480 related device files, found in the
+specific device folder path ``/sys/bus/iio/devices/iio:deviceX``.
+
+**Available only for ADIS16480 and ADIS16488:**
+
++------------------------------------------+---------------------------------------------------------+
+| 3-Axis Magnetometer related device files | Description                                             |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_scale                            | Scale for the magnetometer channels.                    |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_x_calibbias                      | Calibration offset for the X-axis magnetometer channel. |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_x_filter_low_pass_3db_frequency  | Bandwidth for the X-axis magnetometer channel.          |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_x_raw                            | Raw X-axis magnetometer channel value.                  |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_y_calibbias                      | Calibration offset for the Y-axis magnetometer channel. |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_y_filter_low_pass_3db_frequency  | Bandwidth for the Y-axis magnetometer channel.          |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_y_raw                            | Raw Y-axis magnetometer channel value.                  |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_z_calibbias                      | Calibration offset for the Z-axis magnetometer channel. |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_z_filter_low_pass_3db_frequency  | Bandwidth for the Z-axis magnetometer channel.          |
++------------------------------------------+---------------------------------------------------------+
+| in_magn_z_raw                            | Raw Z-axis magnetometer channel value.                  |
++------------------------------------------+---------------------------------------------------------+
+
++------------------------------------------+-----------------------------------------------------+
+| Barometric pressure sensor related files | Description                                         |
++------------------------------------------+-----------------------------------------------------+
+| in_pressure0_calibbias                   | Calibration offset for barometric pressure channel. |
++------------------------------------------+-----------------------------------------------------+
+| in_pressure0_raw                         | Raw barometric pressure channel value.              |
++------------------------------------------+-----------------------------------------------------+
+| in_pressure0_scale                       | Scale for the barometric pressure sensor channel.   |
++------------------------------------------+-----------------------------------------------------+
+
+**Available for all supported devices:**
+
++-------------------------------------------+----------------------------------------------------------+
+| 3-Axis Accelerometer related device files | Description                                              |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_scale                            | Scale for the accelerometer channels.                    |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_calibbias                      | Calibration offset for the X-axis accelerometer channel. |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_calibscale                     | Calibration scale for the X-axis accelerometer channel.  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_filter_low_pass_3db_frequency  | Bandwidth for the X-axis accelerometer channel.          |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_raw                            | Raw X-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_calibbias                      | Calibration offset for the Y-axis accelerometer channel. |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_calibscale                     | Calibration scale for the Y-axis accelerometer channel.  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_filter_low_pass_3db_frequency  | Bandwidth for the Y-axis accelerometer channel.          |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_raw                            | Raw Y-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_calibbias                      | Calibration offset for the Z-axis accelerometer channel. |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_calibscale                     | Calibration scale for the Z-axis accelerometer channel.  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_filter_low_pass_3db_frequency  | Bandwidth for the Z-axis accelerometer channel.          |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_raw                            | Raw Z-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+| in_deltavelocity_scale                    | Scale for delta velocity channels.                       |
++-------------------------------------------+----------------------------------------------------------+
+| in_deltavelocity_x_raw                    | Raw X-axis delta velocity channel value.                 |
++-------------------------------------------+----------------------------------------------------------+
+| in_deltavelocity_y_raw                    | Raw Y-axis delta velocity channel value.                 |
++-------------------------------------------+----------------------------------------------------------+
+| in_deltavelocity_z_raw                    | Raw Z-axis delta velocity channel value.                 |
++-------------------------------------------+----------------------------------------------------------+
+
++--------------------------------------------+------------------------------------------------------+
+| 3-Axis Gyroscope related device files      | Description                                          |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_scale                           | Scale for the gyroscope channels.                    |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_x_calibbias                     | Calibration offset for the X-axis gyroscope channel. |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_x_calibscale                    | Calibration scale for the X-axis gyroscope channel.  |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_x_filter_low_pass_3db_frequency | Bandwidth for the X-axis gyroscope channel.          |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_x_raw                           | Raw X-axis gyroscope channel value.                  |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_y_calibbias                     | Calibration offset for the Y-axis gyroscope channel. |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_y_calibscale                    | Calibration scale for the Y-axis gyroscope channel.  |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_y_filter_low_pass_3db_frequency | Bandwidth for the Y-axis gyroscope channel.          |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_y_raw                           | Raw Y-axis gyroscope channel value.                  |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_z_calibbias                     | Calibration offset for the Z-axis gyroscope channel. |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_z_calibscale                    | Calibration scale for the Z-axis gyroscope channel.  |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_z_filter_low_pass_3db_frequency | Bandwidth for the Z-axis gyroscope channel.          |
++--------------------------------------------+------------------------------------------------------+
+| in_anglvel_z_raw                           | Raw Z-axis gyroscope channel value.                  |
++--------------------------------------------+------------------------------------------------------+
+| in_deltaangl_scale                         | Scale for delta angle channels.                      |
++--------------------------------------------+------------------------------------------------------+
+| in_deltaangl_x_raw                         | Raw X-axis delta angle channel value.                |
++--------------------------------------------+------------------------------------------------------+
+| in_deltaangl_y_raw                         | Raw Y-axis delta angle channel value.                |
++--------------------------------------------+------------------------------------------------------+
+| in_deltaangl_z_raw                         | Raw Z-axis delta angle channel value.                |
++--------------------------------------------+------------------------------------------------------+
+
++----------------------------------+-------------------------------------------+
+| Temperature sensor related files | Description                               |
++----------------------------------+-------------------------------------------+
+| in_temp0_raw                     | Raw temperature channel value.            |
++----------------------------------+-------------------------------------------+
+| in_temp0_offset                  | Offset for the temperature sensor channel.|
++----------------------------------+-------------------------------------------+
+| in_temp0_scale                   | Scale for the temperature sensor channel. |
++----------------------------------+-------------------------------------------+
+
++-------------------------------+---------------------------------------------------------+
+| Miscellaneous device files    | Description                                             |
++-------------------------------+---------------------------------------------------------+
+| name                          | Name of the IIO device.                                 |
++-------------------------------+---------------------------------------------------------+
+| sampling_frequency            | Currently selected sample rate.                         |
++-------------------------------+---------------------------------------------------------+
+
+The following table shows the adis16480 related device debug files, found in the
+specific device debug folder path ``/sys/kernel/debug/iio/iio:deviceX``.
+
++----------------------+-------------------------------------------------------------------------+
+| Debugfs device files | Description                                                             |
++----------------------+-------------------------------------------------------------------------+
+| serial_number        | The serial number of the chip in hexadecimal format.                    |
++----------------------+-------------------------------------------------------------------------+
+| product_id           | Chip specific product id (e.g. 16480, 16488, 16545, etc.).              |
++----------------------+-------------------------------------------------------------------------+
+| flash_count          | The number of flash writes performed on the device.                     |
++----------------------+-------------------------------------------------------------------------+
+| firmware_revision    | String containing the firmware revision in the following format ##.##.  |
++----------------------+-------------------------------------------------------------------------+
+| firmware_date        | String containing the firmware date in the following format mm-dd-yyyy. |
++----------------------+-------------------------------------------------------------------------+
+
+Channels processed values
+-------------------------
+
+A channel value can be read from its _raw attribute. The value returned is the
+raw value as reported by the devices. To get the processed value of the channel,
+apply the following formula:
+
+.. code-block:: bash
+
+        processed value = (_raw + _offset) * _scale
+
+Where _offset and _scale are device attributes. If no _offset attribute is
+present, simply assume its value is 0.
+
+The adis16480 driver offers data for 7 types of channels, the table below shows
+the measurement units for the processed value, which are defined by the IIO
+framework:
+
++--------------------------------------+---------------------------+
+| Channel type                         | Measurement unit          |
++--------------------------------------+---------------------------+
+| Acceleration on X, Y, and Z axis     | Meters per Second squared |
++--------------------------------------+---------------------------+
+| Angular velocity on X, Y and Z axis  | Radians per second        |
++--------------------------------------+---------------------------+
+| Delta velocity on X. Y, and Z axis   | Meters per Second         |
++--------------------------------------+---------------------------+
+| Delta angle on X, Y, and Z axis      | Radians                   |
++--------------------------------------+---------------------------+
+| Temperature                          | Millidegrees Celsius      |
++--------------------------------------+---------------------------+
+| Magnetic field along X, Y and Z axis | Gauss                     |
++--------------------------------------+---------------------------+
+| Barometric pressure                  | kilo Pascal               |
++--------------------------------------+---------------------------+
+
+Usage examples
+--------------
+
+Show device name:
+
+.. code-block:: bash
+
+	root:/sys/bus/iio/devices/iio:device0> cat name
+        adis16545-1
+
+Show accelerometer channels value:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_raw
+        1376728
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_y_raw
+        4487621
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_z_raw
+        262773792
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_scale
+        0.000000037
+
+- X-axis acceleration = in_accel_x_raw * in_accel_scale = 0.050938936 m/s^2
+- Y-axis acceleration = in_accel_y_raw * in_accel_scale = 0.166041977 m/s^2
+- Z-axis acceleration = in_accel_z_raw * in_accel_scale = 9.722630304 m/s^2
+
+Show gyroscope channels value:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_x_raw
+        -1041702
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_y_raw
+        -273013
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_z_raw
+        2745116
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_scale
+        0.000000001
+
+- X-axis angular velocity = in_anglvel_x_raw * in_anglvel_scale = −0.001041702 rad/s
+- Y-axis angular velocity = in_anglvel_y_raw * in_anglvel_scale = −0.000273013 rad/s
+- Z-axis angular velocity = in_anglvel_z_raw * in_anglvel_scale = 0.002745116 rad/s
+
+Set calibration offset for accelerometer channels:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        0
+
+        root:/sys/bus/iio/devices/iio:device0> echo 5000 > in_accel_x_calibbias
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        5000
+
+Set calibration offset for gyroscope channels:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_y_calibbias
+        0
+
+        root:/sys/bus/iio/devices/iio:device0> echo -5000 > in_anglvel_y_calibbias
+        root:/sys/bus/iio/devices/iio:device0> cat in_anglvel_y_calibbias
+        -5000
+
+Set sampling frequency:
+
+.. code-block:: bash
+
+	root:/sys/bus/iio/devices/iio:device0> cat sampling_frequency
+        4250.000000
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1000 > sampling_frequency
+        1062.500000
+
+Set bandwidth for accelerometer channels:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_filter_low_pass_3db_frequency
+        0
+
+        root:/sys/bus/iio/devices/iio:device0> echo 300 > in_accel_x_filter_low_pass_3db_frequency
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_filter_low_pass_3db_frequency
+        300
+
+Show serial number:
+
+.. code-block:: bash
+
+        root:/sys/kernel/debug/iio/iio:device0> cat serial_number
+        0x000c
+
+Show product id:
+
+.. code-block:: bash
+
+        root:/sys/kernel/debug/iio/iio:device0> cat product_id
+        16545
+
+Show flash count:
+
+.. code-block:: bash
+
+        root:/sys/kernel/debug/iio/iio:device0> cat flash_count
+        88
+
+Show firmware revision:
+
+.. code-block:: bash
+
+        root:/sys/kernel/debug/iio/iio:device0> cat firmware_revision
+        1.4
+
+Show firmware date:
+
+.. code-block:: bash
+
+        root:/sys/kernel/debug/iio/iio:device0> cat firmware_date
+        09-23-2023
+
+3. Device buffers
+=================
+
+This driver supports IIO buffers.
+
+All devices support retrieving the raw acceleration, gyroscope and temperature
+measurements using buffers.
+
+The following device families also support retrieving the delta velocity, delta
+angle and temperature measurements using buffers:
+
+- ADIS16545
+- ADIS16547
+
+However, when retrieving acceleration or gyroscope data using buffers, delta
+readings will not be available and vice versa.
+
+Usage examples
+--------------
+
+Set device trigger in current_trigger, if not already set:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat trigger/current_trigger
+
+        root:/sys/bus/iio/devices/iio:device0> echo adis16545-1-dev0 > trigger/current_trigger
+        root:/sys/bus/iio/devices/iio:device0> cat trigger/current_trigger
+        adis16545-1-dev0
+
+Select channels for buffer read:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_deltavelocity_x_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_deltavelocity_y_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_deltavelocity_z_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_temp0_en
+
+Set the number of samples to be stored in the buffer:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 10 > buffer/length
+
+Enable buffer readings:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > buffer/enable
+
+Obtain buffered data:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> hexdump -C /dev/iio\:device0
+        ...
+        00006aa0  09 62 00 00 ff ff fc a4  00 00 01 69 00 03 3c 08  |.b.........i..<.|
+        00006ab0  09 61 00 00 00 00 02 96  00 00 02 8f 00 03 37 50  |.a............7P|
+        00006ac0  09 61 00 00 00 00 12 3d  00 00 0b 89 00 03 2c 0b  |.a.....=......,.|
+        00006ad0  09 61 00 00 00 00 1e dc  00 00 16 dd 00 03 25 bf  |.a............%.|
+        00006ae0  09 61 00 00 00 00 1e e3  00 00 1b bf 00 03 27 0b  |.a............'.|
+        00006af0  09 61 00 00 00 00 15 50  00 00 19 44 00 03 30 fd  |.a.....P...D..0.|
+        00006b00  09 61 00 00 00 00 09 0e  00 00 14 41 00 03 3d 7f  |.a.........A..=.|
+        00006b10  09 61 00 00 ff ff ff f0  00 00 0e bc 00 03 48 d0  |.a............H.|
+        00006b20  09 63 00 00 00 00 00 9f  00 00 0f 37 00 03 4c fe  |.c.........7..L.|
+        00006b30  09 64 00 00 00 00 0b f6  00 00 18 92 00 03 43 22  |.d............C"|
+        00006b40  09 64 00 00 00 00 18 df  00 00 22 33 00 03 33 ab  |.d........"3..3.|
+        00006b50  09 63 00 00 00 00 1e 81  00 00 26 be 00 03 29 60  |.c........&...)`|
+        00006b60  09 63 00 00 00 00 1b 13  00 00 22 2f 00 03 23 91  |.c........"/..#.|
+        ...
+
+See ``Documentation/iio/iio_devbuf.rst`` for more information about how buffered
+data is structured.
+
+4. IIO Interfacing Tools
+========================
+
+Linux Kernel Tools
+------------------
+
+Linux Kernel provides some userspace tools that can be used to retrieve data
+from IIO sysfs:
+
+* lsiio: example application that provides a list of IIO devices and triggers
+* iio_event_monitor: example application that reads events from an IIO device
+  and prints them
+* iio_generic_buffer: example application that reads data from buffer
+* iio_utils: set of APIs, typically used to access sysfs files.
+
+LibIIO
+------
+
+LibIIO is a C/C++ library that provides generic access to IIO devices. The
+library abstracts the low-level details of the hardware, and provides a simple
+yet complete programming interface that can be used for advanced projects.
+
+For more information about LibIIO, please see:
+https://github.com/analogdevicesinc/libiio
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index fb6f9d743211..79c348a0284b 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -18,5 +18,6 @@ Industrial I/O Kernel Drivers
 
    ad7944
    adis16475
+   adis16480
    bno055
    ep93xx_adc
-- 
2.34.1

Re: [PATCH 5/5] docs: iio: add documentation for adis16480 driver

[Jonathan Cameron]

On Tue, 23 Apr 2024 11:42:10 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

> Add documentation for adis16480 driver which describes the driver
> device files and shows how the user may use the ABI for various
> scenarios (configuration, measurement, etc.).
> 
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>

LGTM.  A few minor comments, and the final section feels to me like
it belongs somewhere more generic.  I don't want to see much duplication
in these files and that sort of set of pointers to software stacks is
something that either needs to be in some higher level docs, or would
get duplicated.

...

>
> +
> +3. Device buffers
> +=================
> +
> +This driver supports IIO buffers.
> +
> +All devices support retrieving the raw acceleration, gyroscope and temperature
> +measurements using buffers.
> +
> +The following device families also support retrieving the delta velocity, delta
> +angle and temperature measurements using buffers:
> +
> +- ADIS16545
> +- ADIS16547
> +
> +However, when retrieving acceleration or gyroscope data using buffers, delta
> +readings will not be available and vice versa.

I would add a sentence here on why.


> +
> +See ``Documentation/iio/iio_devbuf.rst`` for more information about how buffered
> +data is structured.
> +
> +4. IIO Interfacing Tools
> +========================

This bit looks general.  Good to have, but given we don't want to repeat it in lots
of drivers, perhaps move it somewhere more general?

> +
> +Linux Kernel Tools
> +------------------
> +
> +Linux Kernel provides some userspace tools that can be used to retrieve data
> +from IIO sysfs:
> +
> +* lsiio: example application that provides a list of IIO devices and triggers
> +* iio_event_monitor: example application that reads events from an IIO device
> +  and prints them
> +* iio_generic_buffer: example application that reads data from buffer
> +* iio_utils: set of APIs, typically used to access sysfs files.
> +
> +LibIIO
> +------
> +
> +LibIIO is a C/C++ library that provides generic access to IIO devices. The
> +library abstracts the low-level details of the hardware, and provides a simple
> +yet complete programming interface that can be used for advanced projects.
> +
> +For more information about LibIIO, please see:
> +https://github.com/analogdevicesinc/libiio