[PATCH v3 0/5] iio: accel: bma400: Refactor GENINTR config and register macros

[PATCH v3 0/5] iio: accel: bma400: Refactor GENINTR config and register macros

[Akshay Jindal]

This series refactors the BMA400 driver with a focus on generic interrupt
configuration and related register usage. The main changes reduce
usage of hard-coded values by introducing macros and formula-based
register addressing, and add a centralized lookup indexed on iio event
direction.

Alongside these updates, the series also reorganizes and renames register
and field macros for consistency with the datasheet, and extends comments
for additional clarity.

All patches are pure refactoring. No functional changes are intended.

Akshay Jindal (5):
  iio: accel: bma400: Reorganize and rename register and field macros
  iio: accel: bma400: Use macros for generic event configuration values
  iio: accel: bma400: Use index-based register addressing and lookup
  iio: accel: bma400: Rename activity_event_en() to generic_event_en()
  iio: accel: bma400: Add detail to comments in GEN INTR configuration

Changes since v2:
- Split single patch into five smaller patches as suggested
- Addressed review comments related to trailing comma [Patch 2/5]
- Extended renaming of macros to TAP_CONFIG registers [Patch 1/5]
- Addressed review comment received regarding write then replace in
  activity_event_en() [Patch 3/5]

Testing Summary:
- Tested on raspberrypi 4b and 7-semi bma400 sensor breakout board.
- Since no functional impact is there, so before functionality is
  expected to be equal to after change functionality.
- Tested mapping of GEN1 and GEN2 both on INT1 pin as before.
- Tested both activity and inactivity detection by setting attributes
  events/in_accel_mag_falling_en as well as events/in_accel_mag_rising_en.
- Did read and writes on various attributes such that write_event_config(),
  write_event_value() and read_event_value() callbacks are triggered.

 drivers/iio/accel/bma400.h      | 151 ++++++++------
 drivers/iio/accel/bma400_core.c | 345 ++++++++++++++++++--------------
 2 files changed, 286 insertions(+), 210 deletions(-)

-- 
2.43.0

[PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register and field macros

[Akshay Jindal]

Reorganize register and field macros to improve consistency with the
datasheet and naming style:

- Move field macros next to their corresponding register macros
- Reorder register macros to follow address order from the datasheet
- Rename field macros to include the register name in the macro name
- Add a _REG suffix to register macros where missing

No functional changes are intended.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
 drivers/iio/accel/bma400.h      | 110 +++++++++--------
 drivers/iio/accel/bma400_core.c | 212 ++++++++++++++++----------------
 2 files changed, 163 insertions(+), 159 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 932358b45f17..ae3411c090c9 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -16,31 +16,37 @@
  * Read-Only Registers
  */
 
+/* Chip ID of BMA 400 devices found in the chip ID register. */
+#define BMA400_ID_REG_VAL           0x90
+
 /* Status and ID registers */
 #define BMA400_CHIP_ID_REG          0x00
 #define BMA400_ERR_REG              0x02
 #define BMA400_STATUS_REG           0x03
 
 /* Acceleration registers */
-#define BMA400_X_AXIS_LSB_REG       0x04
-#define BMA400_X_AXIS_MSB_REG       0x05
-#define BMA400_Y_AXIS_LSB_REG       0x06
-#define BMA400_Y_AXIS_MSB_REG       0x07
-#define BMA400_Z_AXIS_LSB_REG       0x08
-#define BMA400_Z_AXIS_MSB_REG       0x09
+#define BMA400_ACC_X_LSB_REG		0x04
+#define BMA400_ACC_X_MSB_REG		0x05
+#define BMA400_ACC_Y_LSB_REG		0x06
+#define BMA400_ACC_Y_MSB_REG		0x07
+#define BMA400_ACC_Z_LSB_REG		0x08
+#define BMA400_ACC_Z_MSB_REG		0x09
 
 /* Sensor time registers */
-#define BMA400_SENSOR_TIME0         0x0a
-#define BMA400_SENSOR_TIME1         0x0b
-#define BMA400_SENSOR_TIME2         0x0c
+#define BMA400_SENSOR_TIME0_REG         0x0a
+#define BMA400_SENSOR_TIME1_REG         0x0b
+#define BMA400_SENSOR_TIME2_REG         0x0c
 
 /* Event and interrupt registers */
 #define BMA400_EVENT_REG            0x0d
+
 #define BMA400_INT_STAT0_REG        0x0e
 #define BMA400_INT_STAT1_REG        0x0f
 #define BMA400_INT_STAT2_REG        0x10
-#define BMA400_INT12_MAP_REG        0x23
-#define BMA400_INT_ENG_OVRUN_MSK    BIT(4)
+#define BMA400_ENG_OVRUN_INT_STAT_MASK		BIT(4)
+#define BMA400_STEP_INT_STAT_MASK		GENMASK(9, 8)
+#define BMA400_S_TAP_INT_STAT_MASK		BIT(10)
+#define BMA400_D_TAP_INT_STAT_MASK		BIT(11)
 
 /* Temperature register */
 #define BMA400_TEMP_DATA_REG        0x11
@@ -55,70 +61,68 @@
 #define BMA400_STEP_CNT1_REG        0x16
 #define BMA400_STEP_CNT3_REG        0x17
 #define BMA400_STEP_STAT_REG        0x18
-#define BMA400_STEP_INT_MSK         BIT(0)
 #define BMA400_STEP_RAW_LEN         0x03
-#define BMA400_STEP_STAT_MASK       GENMASK(9, 8)
 
 /*
  * Read-write configuration registers
  */
-#define BMA400_ACC_CONFIG0_REG      0x19
-#define BMA400_ACC_CONFIG1_REG      0x1a
+#define BMA400_ACC_CONFIG0_REG		0x19
+#define BMA400_ACC_CONFIG0_LP_OSR_MASK		GENMASK(6, 5)
+#define BMA400_LP_OSR_SHIFT		5
+
+#define BMA400_ACC_CONFIG1_REG		0x1a
+#define BMA400_ACC_CONFIG1_ODR_MASK		GENMASK(3, 0)
+#define BMA400_ACC_CONFIG1_ODR_MIN_RAW		0x05
+#define BMA400_ACC_CONFIG1_ODR_LP_RAW		0x06
+#define BMA400_ACC_CONFIG1_ODR_MAX_RAW		0x0b
+#define BMA400_ACC_CONFIG1_ODR_MAX_HZ		800
+#define BMA400_ACC_CONFIG1_ODR_MIN_WHOLE_HZ	25
+#define BMA400_ACC_CONFIG1_ODR_MIN_HZ		12
+#define BMA400_ACC_CONFIG1_NP_OSR_MASK		GENMASK(5, 4)
+#define BMA400_NP_OSR_SHIFT		4
+#define BMA400_ACC_CONFIG1_ACC_RANGE_MASK	GENMASK(7, 6)
+#define BMA400_ACC_RANGE_SHIFT		6
+
 #define BMA400_ACC_CONFIG2_REG      0x1b
-#define BMA400_CMD_REG              0x7e
 
 /* Interrupt registers */
 #define BMA400_INT_CONFIG0_REG	    0x1f
+#define BMA400_INT_CONFIG0_GEN1_MASK		BIT(2)
+#define BMA400_INT_CONFIG0_GEN2_MASK		BIT(3)
+#define BMA400_INT_CONFIG0_DRDY_MASK		BIT(7)
+
 #define BMA400_INT_CONFIG1_REG	    0x20
+#define BMA400_INT_CONFIG1_STEP_INT_MASK	BIT(0)
+#define BMA400_INT_CONFIG1_S_TAP_MASK		BIT(2)
+#define BMA400_INT_CONFIG1_D_TAP_MASK		BIT(3)
+
 #define BMA400_INT1_MAP_REG	    0x21
+#define BMA400_INT12_MAP_REG        0x23
 #define BMA400_INT_IO_CTRL_REG	    0x24
-#define BMA400_INT_DRDY_MSK	    BIT(7)
-
-/* Chip ID of BMA 400 devices found in the chip ID register. */
-#define BMA400_ID_REG_VAL           0x90
-
-#define BMA400_LP_OSR_SHIFT         5
-#define BMA400_NP_OSR_SHIFT         4
-#define BMA400_SCALE_SHIFT          6
 
 #define BMA400_TWO_BITS_MASK        GENMASK(1, 0)
-#define BMA400_LP_OSR_MASK          GENMASK(6, 5)
-#define BMA400_NP_OSR_MASK          GENMASK(5, 4)
-#define BMA400_ACC_ODR_MASK         GENMASK(3, 0)
-#define BMA400_ACC_SCALE_MASK       GENMASK(7, 6)
-
-#define BMA400_ACC_ODR_MIN_RAW      0x05
-#define BMA400_ACC_ODR_LP_RAW       0x06
-#define BMA400_ACC_ODR_MAX_RAW      0x0b
-
-#define BMA400_ACC_ODR_MAX_HZ       800
-#define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
-#define BMA400_ACC_ODR_MIN_HZ       12
 
 /* Generic interrupts register */
-#define BMA400_GEN1INT_CONFIG0      0x3f
-#define BMA400_GEN2INT_CONFIG0      0x4A
+#define BMA400_GEN1INT_CONFIG0_REG      0x3f
+#define BMA400_GEN2INT_CONFIG0_REG      0x4A
+#define BMA400_GENINT_CONFIG0_HYST_MASK		GENMASK(1, 0)
+
 #define BMA400_GEN_CONFIG1_OFF      0x01
 #define BMA400_GEN_CONFIG2_OFF      0x02
 #define BMA400_GEN_CONFIG3_OFF      0x03
 #define BMA400_GEN_CONFIG31_OFF     0x04
-#define BMA400_INT_GEN1_MSK         BIT(2)
-#define BMA400_INT_GEN2_MSK         BIT(3)
-#define BMA400_GEN_HYST_MSK         GENMASK(1, 0)
 
 /* TAP config registers */
-#define BMA400_TAP_CONFIG           0x57
-#define BMA400_TAP_CONFIG1          0x58
-#define BMA400_S_TAP_MSK            BIT(2)
-#define BMA400_D_TAP_MSK            BIT(3)
-#define BMA400_INT_S_TAP_MSK        BIT(10)
-#define BMA400_INT_D_TAP_MSK        BIT(11)
-#define BMA400_TAP_SEN_MSK          GENMASK(2, 0)
-#define BMA400_TAP_TICSTH_MSK       GENMASK(1, 0)
-#define BMA400_TAP_QUIET_MSK        GENMASK(3, 2)
-#define BMA400_TAP_QUIETDT_MSK      GENMASK(5, 4)
+#define BMA400_TAP_CONFIG_REG		0x57
+#define BMA400_TAP_CONFIG_SEN_MASK	GENMASK(2, 0)
+
+#define BMA400_TAP_CONFIG1_REG          0x58
+#define BMA400_TAP_CONFIG1_TICSTH_MASK		GENMASK(1, 0)
+#define BMA400_TAP_CONFIG1_QUIET_MASK		GENMASK(3, 2)
+#define BMA400_TAP_CONFIG1_QUIETDT_MASK		GENMASK(5, 4)
 #define BMA400_TAP_TIM_LIST_LEN     4
 
+#define BMA400_CMD_REG              0x7e
 /*
  * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
  * converting to micro values for +-2g range.
@@ -138,8 +142,8 @@
  * To select +-8g = 9577 << 2 = raw value to write is 2.
  * To select +-16g = 9577 << 3 = raw value to write is 3.
  */
-#define BMA400_SCALE_MIN            9577
-#define BMA400_SCALE_MAX            76617
+#define BMA400_ACC_SCALE_MIN            9577
+#define BMA400_ACC_SCALE_MAX            76617
 
 extern const struct regmap_config bma400_regmap_config;
 
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 85e23badf733..35d2b90425f8 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -127,15 +127,15 @@ static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
 	case BMA400_CHIP_ID_REG:
 	case BMA400_ERR_REG:
 	case BMA400_STATUS_REG:
-	case BMA400_X_AXIS_LSB_REG:
-	case BMA400_X_AXIS_MSB_REG:
-	case BMA400_Y_AXIS_LSB_REG:
-	case BMA400_Y_AXIS_MSB_REG:
-	case BMA400_Z_AXIS_LSB_REG:
-	case BMA400_Z_AXIS_MSB_REG:
-	case BMA400_SENSOR_TIME0:
-	case BMA400_SENSOR_TIME1:
-	case BMA400_SENSOR_TIME2:
+	case BMA400_ACC_X_LSB_REG:
+	case BMA400_ACC_X_MSB_REG:
+	case BMA400_ACC_Y_LSB_REG:
+	case BMA400_ACC_Y_MSB_REG:
+	case BMA400_ACC_Z_LSB_REG:
+	case BMA400_ACC_Z_MSB_REG:
+	case BMA400_SENSOR_TIME0_REG:
+	case BMA400_SENSOR_TIME1_REG:
+	case BMA400_SENSOR_TIME2_REG:
 	case BMA400_EVENT_REG:
 	case BMA400_INT_STAT0_REG:
 	case BMA400_INT_STAT1_REG:
@@ -159,15 +159,15 @@ static bool bma400_is_volatile_reg(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case BMA400_ERR_REG:
 	case BMA400_STATUS_REG:
-	case BMA400_X_AXIS_LSB_REG:
-	case BMA400_X_AXIS_MSB_REG:
-	case BMA400_Y_AXIS_LSB_REG:
-	case BMA400_Y_AXIS_MSB_REG:
-	case BMA400_Z_AXIS_LSB_REG:
-	case BMA400_Z_AXIS_MSB_REG:
-	case BMA400_SENSOR_TIME0:
-	case BMA400_SENSOR_TIME1:
-	case BMA400_SENSOR_TIME2:
+	case BMA400_ACC_X_LSB_REG:
+	case BMA400_ACC_X_MSB_REG:
+	case BMA400_ACC_Y_LSB_REG:
+	case BMA400_ACC_Y_MSB_REG:
+	case BMA400_ACC_Z_LSB_REG:
+	case BMA400_ACC_Z_MSB_REG:
+	case BMA400_SENSOR_TIME0_REG:
+	case BMA400_SENSOR_TIME1_REG:
+	case BMA400_SENSOR_TIME2_REG:
 	case BMA400_EVENT_REG:
 	case BMA400_INT_STAT0_REG:
 	case BMA400_INT_STAT1_REG:
@@ -275,11 +275,11 @@ static ssize_t in_accel_gesture_tap_maxtomin_time_show(struct device *dev,
 	struct bma400_data *data = iio_priv(indio_dev);
 	int ret, reg_val, raw, vals[2];
 
-	ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1, &reg_val);
+	ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1_REG, &reg_val);
 	if (ret)
 		return ret;
 
-	raw = FIELD_GET(BMA400_TAP_TICSTH_MSK, reg_val);
+	raw = FIELD_GET(BMA400_TAP_CONFIG1_TICSTH_MASK, reg_val);
 	vals[0] = 0;
 	vals[1] = tap_max2min_time[raw];
 
@@ -302,9 +302,9 @@ static ssize_t in_accel_gesture_tap_maxtomin_time_store(struct device *dev,
 	if (raw < 0)
 		return -EINVAL;
 
-	ret = regmap_update_bits(data->regmap, BMA400_TAP_CONFIG1,
-				 BMA400_TAP_TICSTH_MSK,
-				 FIELD_PREP(BMA400_TAP_TICSTH_MSK, raw));
+	ret = regmap_update_bits(data->regmap, BMA400_TAP_CONFIG1_REG,
+				 BMA400_TAP_CONFIG1_TICSTH_MASK,
+				 FIELD_PREP(BMA400_TAP_CONFIG1_TICSTH_MASK, raw));
 	if (ret)
 		return ret;
 
@@ -449,13 +449,13 @@ static int bma400_get_accel_reg(struct bma400_data *data,
 
 	switch (chan->channel2) {
 	case IIO_MOD_X:
-		lsb_reg = BMA400_X_AXIS_LSB_REG;
+		lsb_reg = BMA400_ACC_X_LSB_REG;
 		break;
 	case IIO_MOD_Y:
-		lsb_reg = BMA400_Y_AXIS_LSB_REG;
+		lsb_reg = BMA400_ACC_Y_LSB_REG;
 		break;
 	case IIO_MOD_Z:
-		lsb_reg = BMA400_Z_AXIS_LSB_REG;
+		lsb_reg = BMA400_ACC_Z_LSB_REG;
 		break;
 	default:
 		dev_err(data->dev, "invalid axis channel modifier\n");
@@ -475,8 +475,8 @@ static int bma400_get_accel_reg(struct bma400_data *data,
 static void bma400_output_data_rate_from_raw(int raw, unsigned int *val,
 					     unsigned int *val2)
 {
-	*val = BMA400_ACC_ODR_MAX_HZ >> (BMA400_ACC_ODR_MAX_RAW - raw);
-	if (raw > BMA400_ACC_ODR_MIN_RAW)
+	*val = BMA400_ACC_CONFIG1_ODR_MAX_HZ >> (BMA400_ACC_CONFIG1_ODR_MAX_RAW - raw);
+	if (raw > BMA400_ACC_CONFIG1_ODR_MIN_RAW)
 		*val2 = 0;
 	else
 		*val2 = 500000;
@@ -494,7 +494,7 @@ static int bma400_get_accel_output_data_rate(struct bma400_data *data)
 		 * Runs at a fixed rate in low-power mode. See section 4.3
 		 * in the datasheet.
 		 */
-		bma400_output_data_rate_from_raw(BMA400_ACC_ODR_LP_RAW,
+		bma400_output_data_rate_from_raw(BMA400_ACC_CONFIG1_ODR_LP_RAW,
 						 &data->sample_freq.hz,
 						 &data->sample_freq.uhz);
 		return 0;
@@ -507,9 +507,9 @@ static int bma400_get_accel_output_data_rate(struct bma400_data *data)
 		if (ret)
 			goto error;
 
-		odr = val & BMA400_ACC_ODR_MASK;
-		if (odr < BMA400_ACC_ODR_MIN_RAW ||
-		    odr > BMA400_ACC_ODR_MAX_RAW) {
+		odr = val & BMA400_ACC_CONFIG1_ODR_MASK;
+		if (odr < BMA400_ACC_CONFIG1_ODR_MIN_RAW ||
+		    odr > BMA400_ACC_CONFIG1_ODR_MAX_RAW) {
 			ret = -EINVAL;
 			goto error;
 		}
@@ -539,19 +539,19 @@ static int bma400_set_accel_output_data_rate(struct bma400_data *data,
 	unsigned int val;
 	int ret;
 
-	if (hz >= BMA400_ACC_ODR_MIN_WHOLE_HZ) {
-		if (uhz || hz > BMA400_ACC_ODR_MAX_HZ)
+	if (hz >= BMA400_ACC_CONFIG1_ODR_MIN_WHOLE_HZ) {
+		if (uhz || hz > BMA400_ACC_CONFIG1_ODR_MAX_HZ)
 			return -EINVAL;
 
 		/* Note this works because MIN_WHOLE_HZ is odd */
 		idx = __ffs(hz);
 
-		if (hz >> idx != BMA400_ACC_ODR_MIN_WHOLE_HZ)
+		if (hz >> idx != BMA400_ACC_CONFIG1_ODR_MIN_WHOLE_HZ)
 			return -EINVAL;
 
-		idx += BMA400_ACC_ODR_MIN_RAW + 1;
-	} else if (hz == BMA400_ACC_ODR_MIN_HZ && uhz == 500000) {
-		idx = BMA400_ACC_ODR_MIN_RAW;
+		idx += BMA400_ACC_CONFIG1_ODR_MIN_RAW + 1;
+	} else if (hz == BMA400_ACC_CONFIG1_ODR_MIN_HZ && uhz == 500000) {
+		idx = BMA400_ACC_CONFIG1_ODR_MIN_RAW;
 	} else {
 		return -EINVAL;
 	}
@@ -561,7 +561,7 @@ static int bma400_set_accel_output_data_rate(struct bma400_data *data,
 		return ret;
 
 	/* preserve the range and normal mode osr */
-	odr = (~BMA400_ACC_ODR_MASK & val) | idx;
+	odr = (~BMA400_ACC_CONFIG1_ODR_MASK & val) | idx;
 
 	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
 	if (ret)
@@ -592,7 +592,7 @@ static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
 			return ret;
 		}
 
-		osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
+		osr = (val & BMA400_ACC_CONFIG0_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
 
 		data->oversampling_ratio = osr;
 		return 0;
@@ -603,7 +603,7 @@ static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
 			return ret;
 		}
 
-		osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
+		osr = (val & BMA400_ACC_CONFIG1_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
 
 		data->oversampling_ratio = osr;
 		return 0;
@@ -637,7 +637,7 @@ static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
 			return ret;
 
 		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
-				   (acc_config & ~BMA400_LP_OSR_MASK) |
+				   (acc_config & ~BMA400_ACC_CONFIG0_LP_OSR_MASK) |
 				   (val << BMA400_LP_OSR_SHIFT));
 		if (ret) {
 			dev_err(data->dev, "Failed to write out OSR\n");
@@ -653,7 +653,7 @@ static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
 			return ret;
 
 		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
-				   (acc_config & ~BMA400_NP_OSR_MASK) |
+				   (acc_config & ~BMA400_ACC_CONFIG1_NP_OSR_MASK) |
 				   (val << BMA400_NP_OSR_SHIFT));
 		if (ret) {
 			dev_err(data->dev, "Failed to write out OSR\n");
@@ -679,7 +679,7 @@ static int bma400_accel_scale_to_raw(struct bma400_data *data,
 	/* Note this works because BMA400_SCALE_MIN is odd */
 	raw = __ffs(val);
 
-	if (val >> raw != BMA400_SCALE_MIN)
+	if (val >> raw != BMA400_ACC_SCALE_MIN)
 		return -EINVAL;
 
 	return raw;
@@ -695,11 +695,11 @@ static int bma400_get_accel_scale(struct bma400_data *data)
 	if (ret)
 		return ret;
 
-	raw_scale = (val & BMA400_ACC_SCALE_MASK) >> BMA400_SCALE_SHIFT;
+	raw_scale = (val & BMA400_ACC_CONFIG1_ACC_RANGE_MASK) >> BMA400_ACC_RANGE_SHIFT;
 	if (raw_scale > BMA400_TWO_BITS_MASK)
 		return -EINVAL;
 
-	data->scale = BMA400_SCALE_MIN << raw_scale;
+	data->scale = BMA400_ACC_SCALE_MIN << raw_scale;
 
 	return 0;
 }
@@ -719,8 +719,8 @@ static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
 		return raw;
 
 	ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
-			   (acc_config & ~BMA400_ACC_SCALE_MASK) |
-			   (raw << BMA400_SCALE_SHIFT));
+			   (acc_config & ~BMA400_ACC_CONFIG1_ACC_RANGE_MASK) |
+			   (raw << BMA400_ACC_RANGE_SHIFT));
 	if (ret)
 		return ret;
 
@@ -786,8 +786,8 @@ static int bma400_enable_steps(struct bma400_data *data, int val)
 		return 0;
 
 	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
-				 BMA400_STEP_INT_MSK,
-				 FIELD_PREP(BMA400_STEP_INT_MSK, val ? 1 : 0));
+				 BMA400_INT_CONFIG1_STEP_INT_MASK,
+				 FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK, val ? 1 : 0));
 	if (ret)
 		return ret;
 	data->steps_enabled = val;
@@ -826,7 +826,7 @@ static void bma400_init_tables(void)
 	for (i = 0; i + 1 < ARRAY_SIZE(bma400_scales); i += 2) {
 		raw = i / 2;
 		bma400_scales[i] = 0;
-		bma400_scales[i + 1] = BMA400_SCALE_MIN << raw;
+		bma400_scales[i + 1] = BMA400_ACC_SCALE_MIN << raw;
 	}
 }
 
@@ -1063,7 +1063,7 @@ static int bma400_write_raw(struct iio_dev *indio_dev,
 		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		if (val != 0 ||
-		    val2 < BMA400_SCALE_MIN || val2 > BMA400_SCALE_MAX)
+		    val2 < BMA400_ACC_SCALE_MIN || val2 > BMA400_ACC_SCALE_MAX)
 			return -EINVAL;
 
 		mutex_lock(&data->mutex);
@@ -1114,16 +1114,16 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
 	case IIO_ACCEL:
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
-			return FIELD_GET(BMA400_INT_GEN1_MSK,
+			return FIELD_GET(BMA400_INT_CONFIG0_GEN1_MASK,
 					 data->generic_event_en);
 		case IIO_EV_DIR_FALLING:
-			return FIELD_GET(BMA400_INT_GEN2_MSK,
+			return FIELD_GET(BMA400_INT_CONFIG0_GEN2_MASK,
 					 data->generic_event_en);
 		case IIO_EV_DIR_SINGLETAP:
-			return FIELD_GET(BMA400_S_TAP_MSK,
+			return FIELD_GET(BMA400_INT_CONFIG1_S_TAP_MASK,
 					 data->tap_event_en_bitmask);
 		case IIO_EV_DIR_DOUBLETAP:
-			return FIELD_GET(BMA400_D_TAP_MSK,
+			return FIELD_GET(BMA400_INT_CONFIG1_D_TAP_MASK,
 					 data->tap_event_en_bitmask);
 		default:
 			return -EINVAL;
@@ -1146,8 +1146,8 @@ static int bma400_steps_event_enable(struct bma400_data *data, int state)
 		return ret;
 
 	ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
-				 BMA400_STEP_INT_MSK,
-				 FIELD_PREP(BMA400_STEP_INT_MSK,
+				 BMA400_INT_CONFIG1_STEP_INT_MASK,
+				 FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK,
 					    state));
 	if (ret)
 		return ret;
@@ -1164,18 +1164,18 @@ static int bma400_activity_event_en(struct bma400_data *data,
 
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
-		reg = BMA400_GEN1INT_CONFIG0;
-		msk = BMA400_INT_GEN1_MSK;
+		reg = BMA400_GEN1INT_CONFIG0_REG;
+		msk = BMA400_INT_CONFIG0_GEN1_MASK;
 		value = 2;
-		set_mask_bits(&field_value, BMA400_INT_GEN1_MSK,
-			      FIELD_PREP(BMA400_INT_GEN1_MSK, state));
+		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN1_MASK,
+			      FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state));
 		break;
 	case IIO_EV_DIR_FALLING:
-		reg = BMA400_GEN2INT_CONFIG0;
-		msk = BMA400_INT_GEN2_MSK;
+		reg = BMA400_GEN2INT_CONFIG0_REG;
+		msk = BMA400_INT_CONFIG0_GEN2_MASK;
 		value = 0;
-		set_mask_bits(&field_value, BMA400_INT_GEN2_MSK,
-			      FIELD_PREP(BMA400_INT_GEN2_MSK, state));
+		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN2_MASK,
+			      FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state));
 		break;
 	default:
 		return -EINVAL;
@@ -1240,21 +1240,21 @@ static int bma400_tap_event_en(struct bma400_data *data,
 	}
 
 	ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
-				 BMA400_S_TAP_MSK,
-				 FIELD_PREP(BMA400_S_TAP_MSK, state));
+				 BMA400_INT_CONFIG1_S_TAP_MASK,
+				 FIELD_PREP(BMA400_INT_CONFIG1_S_TAP_MASK, state));
 	if (ret)
 		return ret;
 
 	switch (dir) {
 	case IIO_EV_DIR_SINGLETAP:
-		mask = BMA400_S_TAP_MSK;
-		set_mask_bits(&field_value, BMA400_S_TAP_MSK,
-			      FIELD_PREP(BMA400_S_TAP_MSK, state));
+		mask = BMA400_INT_CONFIG1_S_TAP_MASK;
+		set_mask_bits(&field_value, BMA400_INT_CONFIG1_S_TAP_MASK,
+			      FIELD_PREP(BMA400_INT_CONFIG1_S_TAP_MASK, state));
 		break;
 	case IIO_EV_DIR_DOUBLETAP:
-		mask = BMA400_D_TAP_MSK;
-		set_mask_bits(&field_value, BMA400_D_TAP_MSK,
-			      FIELD_PREP(BMA400_D_TAP_MSK, state));
+		mask = BMA400_INT_CONFIG1_D_TAP_MASK;
+		set_mask_bits(&field_value, BMA400_INT_CONFIG1_D_TAP_MASK,
+			      FIELD_PREP(BMA400_INT_CONFIG1_D_TAP_MASK, state));
 		break;
 	default:
 		return -EINVAL;
@@ -1340,9 +1340,9 @@ static int get_gen_config_reg(enum iio_event_direction dir)
 {
 	switch (dir) {
 	case IIO_EV_DIR_FALLING:
-		return BMA400_GEN2INT_CONFIG0;
+		return BMA400_GEN2INT_CONFIG0_REG;
 	case IIO_EV_DIR_RISING:
-		return BMA400_GEN1INT_CONFIG0;
+		return BMA400_GEN1INT_CONFIG0_REG;
 	default:
 		return -EINVAL;
 	}
@@ -1393,7 +1393,7 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 			ret = regmap_read(data->regmap, reg, val);
 			if (ret)
 				return ret;
-			*val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
+			*val = FIELD_GET(BMA400_GENINT_CONFIG0_HYST_MASK, *val);
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
@@ -1401,30 +1401,30 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
-			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
+			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG_REG,
 					  &reg_val);
 			if (ret)
 				return ret;
 
-			*val = FIELD_GET(BMA400_TAP_SEN_MSK, reg_val);
+			*val = FIELD_GET(BMA400_TAP_CONFIG_SEN_MASK, reg_val);
 			return IIO_VAL_INT;
 		case IIO_EV_INFO_RESET_TIMEOUT:
-			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
+			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1_REG,
 					  &reg_val);
 			if (ret)
 				return ret;
 
-			raw = FIELD_GET(BMA400_TAP_QUIET_MSK, reg_val);
+			raw = FIELD_GET(BMA400_TAP_CONFIG1_QUIET_MASK, reg_val);
 			*val = 0;
 			*val2 = tap_reset_timeout[raw];
 			return IIO_VAL_INT_PLUS_MICRO;
 		case IIO_EV_INFO_TAP2_MIN_DELAY:
-			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
+			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1_REG,
 					  &reg_val);
 			if (ret)
 				return ret;
 
-			raw = FIELD_GET(BMA400_TAP_QUIETDT_MSK, reg_val);
+			raw = FIELD_GET(BMA400_TAP_CONFIG1_QUIETDT_MASK, reg_val);
 			*val = 0;
 			*val2 = double_tap2_min_delay[raw];
 			return IIO_VAL_INT_PLUS_MICRO;
@@ -1480,8 +1480,8 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				return -EINVAL;
 
 			return regmap_update_bits(data->regmap, reg,
-						  BMA400_GEN_HYST_MSK,
-						  FIELD_PREP(BMA400_GEN_HYST_MSK,
+						  BMA400_GENINT_CONFIG0_HYST_MASK,
+						  FIELD_PREP(BMA400_GENINT_CONFIG0_HYST_MASK,
 							     val));
 		default:
 			return -EINVAL;
@@ -1493,9 +1493,9 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				return -EINVAL;
 
 			return regmap_update_bits(data->regmap,
-						  BMA400_TAP_CONFIG,
-						  BMA400_TAP_SEN_MSK,
-						  FIELD_PREP(BMA400_TAP_SEN_MSK,
+						  BMA400_TAP_CONFIG_REG,
+						  BMA400_TAP_CONFIG_SEN_MASK,
+						  FIELD_PREP(BMA400_TAP_CONFIG_SEN_MASK,
 							     val));
 		case IIO_EV_INFO_RESET_TIMEOUT:
 			raw = usec_to_tapreg_raw(val2, tap_reset_timeout);
@@ -1503,9 +1503,9 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				return -EINVAL;
 
 			return regmap_update_bits(data->regmap,
-						  BMA400_TAP_CONFIG1,
-						  BMA400_TAP_QUIET_MSK,
-						  FIELD_PREP(BMA400_TAP_QUIET_MSK,
+						  BMA400_TAP_CONFIG1_REG,
+						  BMA400_TAP_CONFIG1_QUIET_MASK,
+						  FIELD_PREP(BMA400_TAP_CONFIG1_QUIET_MASK,
 							     raw));
 		case IIO_EV_INFO_TAP2_MIN_DELAY:
 			raw = usec_to_tapreg_raw(val2, double_tap2_min_delay);
@@ -1513,9 +1513,9 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				return -EINVAL;
 
 			return regmap_update_bits(data->regmap,
-						  BMA400_TAP_CONFIG1,
-						  BMA400_TAP_QUIETDT_MSK,
-						  FIELD_PREP(BMA400_TAP_QUIETDT_MSK,
+						  BMA400_TAP_CONFIG1_REG,
+						  BMA400_TAP_CONFIG1_QUIETDT_MASK,
+						  FIELD_PREP(BMA400_TAP_CONFIG1_QUIETDT_MASK,
 							     raw));
 		default:
 			return -EINVAL;
@@ -1533,14 +1533,14 @@ static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
 	int ret;
 
 	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
-				 BMA400_INT_DRDY_MSK,
-				 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+				 BMA400_INT_CONFIG0_DRDY_MASK,
+				 FIELD_PREP(BMA400_INT_CONFIG0_DRDY_MASK, state));
 	if (ret)
 		return ret;
 
 	return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
-				  BMA400_INT_DRDY_MSK,
-				  FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+				  BMA400_INT_CONFIG0_DRDY_MASK,
+				  FIELD_PREP(BMA400_INT_CONFIG0_DRDY_MASK, state));
 }
 
 static const unsigned long bma400_avail_scan_masks[] = {
@@ -1578,7 +1578,7 @@ static irqreturn_t bma400_trigger_handler(int irq, void *p)
 	mutex_lock(&data->mutex);
 
 	/* bulk read six registers, with the base being the LSB register */
-	ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
+	ret = regmap_bulk_read(data->regmap, BMA400_ACC_X_LSB_REG,
 			       &data->buffer.buff, sizeof(data->buffer.buff));
 	if (ret)
 		goto unlock_err;
@@ -1628,13 +1628,13 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 	 * Disable all advance interrupts if interrupt engine overrun occurs.
 	 * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
 	 */
-	if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
+	if (FIELD_GET(BMA400_ENG_OVRUN_INT_STAT_MASK, le16_to_cpu(data->status))) {
 		bma400_disable_adv_interrupt(data);
 		dev_err(data->dev, "Interrupt engine overrun\n");
 		goto unlock_err;
 	}
 
-	if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
+	if (FIELD_GET(BMA400_S_TAP_INT_STAT_MASK, le16_to_cpu(data->status)))
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
 						  IIO_MOD_X_OR_Y_OR_Z,
@@ -1642,7 +1642,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 						  IIO_EV_DIR_SINGLETAP),
 			       timestamp);
 
-	if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
+	if (FIELD_GET(BMA400_D_TAP_INT_STAT_MASK, le16_to_cpu(data->status)))
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
 						  IIO_MOD_X_OR_Y_OR_Z,
@@ -1650,10 +1650,10 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 						  IIO_EV_DIR_DOUBLETAP),
 			       timestamp);
 
-	if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
+	if (FIELD_GET(BMA400_INT_CONFIG0_GEN1_MASK, le16_to_cpu(data->status)))
 		ev_dir = IIO_EV_DIR_RISING;
 
-	if (FIELD_GET(BMA400_INT_GEN2_MSK, le16_to_cpu(data->status)))
+	if (FIELD_GET(BMA400_INT_CONFIG0_GEN2_MASK, le16_to_cpu(data->status)))
 		ev_dir = IIO_EV_DIR_FALLING;
 
 	if (ev_dir != IIO_EV_DIR_NONE) {
@@ -1664,7 +1664,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 			       timestamp);
 	}
 
-	if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(data->status))) {
+	if (FIELD_GET(BMA400_STEP_INT_STAT_MASK, le16_to_cpu(data->status))) {
 		iio_push_event(indio_dev,
 			       IIO_MOD_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
 						  IIO_EV_TYPE_CHANGE,
@@ -1686,7 +1686,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 		}
 	}
 
-	if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(data->status))) {
+	if (FIELD_GET(BMA400_INT_CONFIG0_DRDY_MASK, le16_to_cpu(data->status))) {
 		mutex_unlock(&data->mutex);
 		iio_trigger_poll_nested(data->trig);
 		return IRQ_HANDLED;
-- 
2.43.0

Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register and field macros

[Jonathan Cameron]

On Fri,  3 Oct 2025 00:11:02 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Reorganize register and field macros to improve consistency with the
> datasheet and naming style:
> 
> - Move field macros next to their corresponding register macros
> - Reorder register macros to follow address order from the datasheet
> - Rename field macros to include the register name in the macro name
> - Add a _REG suffix to register macros where missing
> 
> No functional changes are intended.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>

This is much easier to review. Thanks for breaking it all up.

> ---
>  drivers/iio/accel/bma400.h      | 110 +++++++++--------
>  drivers/iio/accel/bma400_core.c | 212 ++++++++++++++++----------------
>  2 files changed, 163 insertions(+), 159 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 932358b45f17..ae3411c090c9 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -16,31 +16,37 @@

>  /* Event and interrupt registers */
>  #define BMA400_EVENT_REG            0x0d
> +
>  #define BMA400_INT_STAT0_REG        0x0e
>  #define BMA400_INT_STAT1_REG        0x0f
>  #define BMA400_INT_STAT2_REG        0x10
> -#define BMA400_INT12_MAP_REG        0x23
> -#define BMA400_INT_ENG_OVRUN_MSK    BIT(4)
> +#define BMA400_ENG_OVRUN_INT_STAT_MASK		BIT(4)

This is an odd field as it applies to all the INT_STATX registers
However  I would still try to make that connection with a name
such as BMA500_INT_STAT_OVRUN_MASK

> +#define BMA400_STEP_INT_STAT_MASK		GENMASK(9, 8)

This bit is a little odd.  We are treating INT_STAT0 and INT_STAT1
(but not 2) as a single 16 bit register. That makes it hard to
associate the field with the register name. I wonder if we shouldn't
break that and just handle it as a pair of u8 instead.

> +#define BMA400_S_TAP_INT_STAT_MASK		BIT(10)
> +#define BMA400_D_TAP_INT_STAT_MASK		BIT(11)
>  
>  /* Temperature register */
>  #define BMA400_TEMP_DATA_REG        0x11
> @@ -55,70 +61,68 @@
>  #define BMA400_STEP_CNT1_REG        0x16
>  #define BMA400_STEP_CNT3_REG        0x17
>  #define BMA400_STEP_STAT_REG        0x18
> -#define BMA400_STEP_INT_MSK         BIT(0)
>  #define BMA400_STEP_RAW_LEN         0x03
> -#define BMA400_STEP_STAT_MASK       GENMASK(9, 8)
>  
>  /*
>   * Read-write configuration registers
>   */
> -#define BMA400_ACC_CONFIG0_REG      0x19
> -#define BMA400_ACC_CONFIG1_REG      0x1a
> +#define BMA400_ACC_CONFIG0_REG		0x19
> +#define BMA400_ACC_CONFIG0_LP_OSR_MASK		GENMASK(6, 5)
> +#define BMA400_LP_OSR_SHIFT		5
#
Should never need a explicit shift. Use FIELD_PREP() and FIELD_GET() to
allow the MASK to be used in all cases.

> +
> +#define BMA400_ACC_CONFIG1_REG		0x1a
> +#define BMA400_ACC_CONFIG1_ODR_MASK		GENMASK(3, 0)
> +#define BMA400_ACC_CONFIG1_ODR_MIN_RAW		0x05
> +#define BMA400_ACC_CONFIG1_ODR_LP_RAW		0x06
> +#define BMA400_ACC_CONFIG1_ODR_MAX_RAW		0x0b
> +#define BMA400_ACC_CONFIG1_ODR_MAX_HZ		800
> +#define BMA400_ACC_CONFIG1_ODR_MIN_WHOLE_HZ	25
> +#define BMA400_ACC_CONFIG1_ODR_MIN_HZ		12
> +#define BMA400_ACC_CONFIG1_NP_OSR_MASK		GENMASK(5, 4)

> +#define BMA400_NP_OSR_SHIFT		4
Similarly on this shift.

> +#define BMA400_ACC_CONFIG1_ACC_RANGE_MASK	GENMASK(7, 6)
> +#define BMA400_ACC_RANGE_SHIFT		6

and this one.  Might be a good idea to switch away from using the shifts
as a precursor patch as it's really a different sort of change from
the rest of this.

> +
>  #define BMA400_ACC_CONFIG2_REG      0x1b
> -#define BMA400_CMD_REG              0x7e
>

...
  
> -		osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
> +		osr = (val & BMA400_ACC_CONFIG0_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;

Here is one of those cases with the shift that could just be
		osr = FIELD_GET(BMA400_ACC_CONFIG0_LP_OSR_MASK, val);

>  
>  		data->oversampling_ratio = osr;
>  		return 0;
> @@ -603,7 +603,7 @@ static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
>  			return ret;
>  		}
>  
> -		osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> +		osr = (val & BMA400_ACC_CONFIG1_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
her as well.
>  
>  		data->oversampling_ratio = osr;
>  		return 0;
> @@ -637,7 +637,7 @@ static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
>  			return ret;
>  
>  		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> -				   (acc_config & ~BMA400_LP_OSR_MASK) |
> +				   (acc_config & ~BMA400_ACC_CONFIG0_LP_OSR_MASK) |
>  				   (val << BMA400_LP_OSR_SHIFT));
FIELD_PREP for this one.  
>  		if (ret) {
>  			dev_err(data->dev, "Failed to write out OSR\n");
> @@ -653,7 +653,7 @@ static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
>  			return ret;
>  
>  		ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> -				   (acc_config & ~BMA400_NP_OSR_MASK) |
> +				   (acc_config & ~BMA400_ACC_CONFIG1_NP_OSR_MASK) |
>  				   (val << BMA400_NP_OSR_SHIFT));
here as well.  Anyhow, from a quick look it appears that getting rid of the _SHIFT defines
should be easy.


>  		if (ret) {
>  			dev_err(data->dev, "Failed to write out OSR\n");
>

>  
> @@ -786,8 +786,8 @@ static int bma400_enable_steps(struct bma400_data *data, int val)
>  		return 0;
>  
>  	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
> -				 BMA400_STEP_INT_MSK,
> -				 FIELD_PREP(BMA400_STEP_INT_MSK, val ? 1 : 0));
> +				 BMA400_INT_CONFIG1_STEP_INT_MASK,
> +				 FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK, val ? 1 : 0));

Could use regmap_assign_bits() to simplify this a bit - but separate change
so different patch.

>  	if (ret)
>  		return ret;

Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register and field macros

[Akshay Jindal]

On Sat, Oct 4, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
>This is much easier to review. Thanks for breaking it all up.

Hi Jonathan,
Thank you for the review.
Keeping v3 feedback in mind, I have floated a v4 patch series.
Have some follow-ups in some comments. Please read below
for those.

Thanks,
Akshay.

> >  #define BMA400_INT_STAT0_REG        0x0e
> >  #define BMA400_INT_STAT1_REG        0x0f
> >  #define BMA400_INT_STAT2_REG        0x10
> > -#define BMA400_INT12_MAP_REG        0x23
> > -#define BMA400_INT_ENG_OVRUN_MSK    BIT(4)
> > +#define BMA400_ENG_OVRUN_INT_STAT_MASK               BIT(4)
>
> This is an odd field as it applies to all the INT_STATX registers
> However  I would still try to make that connection with a name
> such as BMA500_INT_STAT_OVRUN_MASK
The connection is still there Jonathan.
The name in the spec is Interrupt Engine Overrun.
BMA400_ENG_OVRUN_INT_STAT_MASK can be read as
Engine Overrun Interrupt Status Mask.
Here for Interrupt Status fields, I have intentionally taken a little
deviation from the naming convention established.
Original convention:BMA400_<reg_name>_<field_name>_<suffix>
Convention here: BMA400_<INT NAME>_<INT_STAT>_<suffix>
so that it can be read as <INT_NAME> Interrupt Status mask.

I would understand, if you want to adhere to original convention.
Will make the change in next version.

>
> > +#define BMA400_STEP_INT_STAT_MASK            GENMASK(9, 8)
>
> This bit is a little odd.  We are treating INT_STAT0 and INT_STAT1
> (but not 2) as a single 16 bit register. That makes it hard to
> associate the field with the register name. I wonder if we shouldn't
> break that and just handle it as a pair of u8 instead.
The spec talks about doing a burst read for such multipart registers
to avoid reading one, while the other one is being updated.
Hence did not touch it.

> >  /*
> >   * Read-write configuration registers
> >   */
> > -#define BMA400_ACC_CONFIG0_REG      0x19
> > -#define BMA400_ACC_CONFIG1_REG      0x1a
> > +#define BMA400_ACC_CONFIG0_REG               0x19
> > +#define BMA400_ACC_CONFIG0_LP_OSR_MASK               GENMASK(6, 5)
> > +#define BMA400_LP_OSR_SHIFT          5
> #
> Should never need a explicit shift. Use FIELD_PREP() and FIELD_GET() to
> allow the MASK to be used in all cases.
>
done

> > +#define BMA400_NP_OSR_SHIFT          4
> Similarly on this shift.
done

> > +#define BMA400_ACC_CONFIG1_ACC_RANGE_MASK    GENMASK(7, 6)
> > +#define BMA400_ACC_RANGE_SHIFT               6
>
> and this one.  Might be a good idea to switch away from using the shifts
> as a precursor patch as it's really a different sort of change from
> the rest of this.
Added a separate patch for this.

>
> > -             osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
> > +             osr = (val & BMA400_ACC_CONFIG0_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
>
> Here is one of those cases with the shift that could just be
yes, fixed as stated above.


> > -             osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> > +             osr = (val & BMA400_ACC_CONFIG1_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> her as well.
yes, fixed.

> >               ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > -                                (acc_config & ~BMA400_LP_OSR_MASK) |
> > +                                (acc_config & ~BMA400_ACC_CONFIG0_LP_OSR_MASK) |
> >                                  (val << BMA400_LP_OSR_SHIFT));
> FIELD_PREP for this one.
fixed.

> >               ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > -                                (acc_config & ~BMA400_NP_OSR_MASK) |
> > +                                (acc_config & ~BMA400_ACC_CONFIG1_NP_OSR_MASK) |
> >                                  (val << BMA400_NP_OSR_SHIFT));
> here as well.  Anyhow, from a quick look it appears that getting rid of the _SHIFT defines
> should be easy.
yes, fixed.


> >       ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
> > -                              BMA400_STEP_INT_MSK,
> > -                              FIELD_PREP(BMA400_STEP_INT_MSK, val ? 1 : 0));
> > +                              BMA400_INT_CONFIG1_STEP_INT_MASK,
> > +                              FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK, val ? 1 : 0));
>
> Could use regmap_assign_bits() to simplify this a bit - but separate change
> so different patch.
regmap_assign_bits calls regmap_set_bits which itself uses
regmap_update_bits_base similar to regmap_update_bits.
Moreover adoption of regmap_assign_bits is not much in drivers.
Hence would request to keep it as it is.

Thanks,
Akshay

Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register and field macros

[Jonathan Cameron]

On Tue, 7 Oct 2025 12:09:43 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sat, Oct 4, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >This is much easier to review. Thanks for breaking it all up.  
> 
> Hi Jonathan,
> Thank you for the review.
> Keeping v3 feedback in mind, I have floated a v4 patch series.
> Have some follow-ups in some comments. Please read below
> for those.
> 
> Thanks,
> Akshay.
> 
> > >  #define BMA400_INT_STAT0_REG        0x0e
> > >  #define BMA400_INT_STAT1_REG        0x0f
> > >  #define BMA400_INT_STAT2_REG        0x10
> > > -#define BMA400_INT12_MAP_REG        0x23
> > > -#define BMA400_INT_ENG_OVRUN_MSK    BIT(4)
> > > +#define BMA400_ENG_OVRUN_INT_STAT_MASK               BIT(4)  
> >
> > This is an odd field as it applies to all the INT_STATX registers
> > However  I would still try to make that connection with a name
> > such as BMA500_INT_STAT_OVRUN_MASK  
> The connection is still there Jonathan.
> The name in the spec is Interrupt Engine Overrun.
> BMA400_ENG_OVRUN_INT_STAT_MASK can be read as
> Engine Overrun Interrupt Status Mask.
> Here for Interrupt Status fields, I have intentionally taken a little
> deviation from the naming convention established.
> Original convention:BMA400_<reg_name>_<field_name>_<suffix>
> Convention here: BMA400_<INT NAME>_<INT_STAT>_<suffix>
> so that it can be read as <INT_NAME> Interrupt Status mask.
> 
> I would understand, if you want to adhere to original convention.
> Will make the change in next version

Yes, I definitely wnt the register name association in there (or a wild
card type thing if it turns up in multiple similar registers.

> 
> >  
> > > +#define BMA400_STEP_INT_STAT_MASK            GENMASK(9, 8)  
> >
> > This bit is a little odd.  We are treating INT_STAT0 and INT_STAT1
> > (but not 2) as a single 16 bit register. That makes it hard to
> > associate the field with the register name. I wonder if we shouldn't
> > break that and just handle it as a pair of u8 instead.  
> The spec talks about doing a burst read for such multipart registers
> to avoid reading one, while the other one is being updated.
> Hence did not touch it.
ok. But then why not INT_STAT2?  Could also do a burst read but still
treat them as separate registers. Bulk read into a u8[2];
  
Cropping out the stuff that needs no more discussion (you could have cropped
the bits you agree with btw!)  Removing them is an implicit 'agreed'

> 
> > >       ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
> > > -                              BMA400_STEP_INT_MSK,
> > > -                              FIELD_PREP(BMA400_STEP_INT_MSK, val ? 1 : 0));
> > > +                              BMA400_INT_CONFIG1_STEP_INT_MASK,
> > > +                              FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK, val ? 1 : 0));  
> >
> > Could use regmap_assign_bits() to simplify this a bit - but separate change
> > so different patch.  
> regmap_assign_bits calls regmap_set_bits which itself uses
> regmap_update_bits_base similar to regmap_update_bits.
> Moreover adoption of regmap_assign_bits is not much in drivers.
> Hence would request to keep it as it is.

I think it is fairly new and the gain here is minor (plus it would be an additional
change anyway) so fine to leave it as it stands.

Thanks,

Jonathan


> 
> Thanks,
> Akshay
> 

[PATCH v3 2/5] iio: accel: bma400: Use macros for generic event configuration values

[Akshay Jindal]

Add macros and enums for configuration values used in generic event
handling for activity and inactivity detection. Replace hard-coded
values in activity_event_en() with the new definitions to make the
configuration explicit.

No functional changes are intended.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
 drivers/iio/accel/bma400.h      | 30 ++++++++++++++++++++++++++++++
 drivers/iio/accel/bma400_core.c | 14 +++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index ae3411c090c9..13fe2e5a3175 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -106,8 +106,38 @@
 #define BMA400_GEN1INT_CONFIG0_REG      0x3f
 #define BMA400_GEN2INT_CONFIG0_REG      0x4A
 #define BMA400_GENINT_CONFIG0_HYST_MASK		GENMASK(1, 0)
+#define BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK	GENMASK(3, 2)
+#define BMA400_GENINT_CONFIG0_DATA_SRC_MASK	BIT(4)
+#define BMA400_GENINT_CONFIG0_X_EN_MASK		BIT(5)
+#define BMA400_GENINT_CONFIG0_Y_EN_MASK		BIT(6)
+#define BMA400_GENINT_CONFIG0_Z_EN_MASK		BIT(7)
+
+enum bma400_accel_data_src {
+	ACCEL_FILT1,
+	ACCEL_FILT2,
+};
+
+enum bma400_ref_updt_mode {
+	BMA400_REF_MANUAL_UPDT_MODE,
+	BMA400_REF_ONETIME_UPDT_MODE,
+	BMA400_REF_EVERYTIME_UPDT_MODE,
+	BMA400_REF_EVERYTIME_LP_UPDT_MODE,
+};
 
 #define BMA400_GEN_CONFIG1_OFF      0x01
+#define BMA400_GENINT_CONFIG1_AXES_COMB_MASK	BIT(0)
+#define BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK	BIT(1)
+
+enum bma400_genintr_acceleval_axescomb {
+	BMA400_EVAL_X_OR_Y_OR_Z,
+	BMA400_EVAL_X_AND_Y_AND_Z,
+};
+
+enum bma400_detect_criterion {
+	BMA400_DETECT_INACTIVITY,
+	BMA400_DETECT_ACTIVITY,
+};
+
 #define BMA400_GEN_CONFIG2_OFF      0x02
 #define BMA400_GEN_CONFIG3_OFF      0x03
 #define BMA400_GEN_CONFIG31_OFF     0x04
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 35d2b90425f8..58c378ba9931 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -1166,14 +1166,16 @@ static int bma400_activity_event_en(struct bma400_data *data,
 	case IIO_EV_DIR_RISING:
 		reg = BMA400_GEN1INT_CONFIG0_REG;
 		msk = BMA400_INT_CONFIG0_GEN1_MASK;
-		value = 2;
+		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
+			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_ACTIVITY);
 		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN1_MASK,
 			      FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state));
 		break;
 	case IIO_EV_DIR_FALLING:
 		reg = BMA400_GEN2INT_CONFIG0_REG;
 		msk = BMA400_INT_CONFIG0_GEN2_MASK;
-		value = 0;
+		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
+			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_INACTIVITY);
 		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN2_MASK,
 			      FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state));
 		break;
@@ -1182,7 +1184,13 @@ static int bma400_activity_event_en(struct bma400_data *data,
 	}
 
 	/* Enabling all axis for interrupt evaluation */
-	ret = regmap_write(data->regmap, reg, 0xF8);
+	ret = regmap_write(data->regmap, reg,
+			   BMA400_GENINT_CONFIG0_X_EN_MASK |
+			   BMA400_GENINT_CONFIG0_Y_EN_MASK |
+			   BMA400_GENINT_CONFIG0_Z_EN_MASK|
+			   FIELD_PREP(BMA400_GENINT_CONFIG0_DATA_SRC_MASK, ACCEL_FILT2)|
+			   FIELD_PREP(BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK,
+				      BMA400_REF_EVERYTIME_UPDT_MODE));
 	if (ret)
 		return ret;
 
-- 
2.43.0

Re: [PATCH v3 2/5] iio: accel: bma400: Use macros for generic event configuration values

[Jonathan Cameron]

On Fri,  3 Oct 2025 00:11:03 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Add macros and enums for configuration values used in generic event
> handling for activity and inactivity detection. Replace hard-coded
> values in activity_event_en() with the new definitions to make the
> configuration explicit.
> 
> No functional changes are intended.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
>  drivers/iio/accel/bma400.h      | 30 ++++++++++++++++++++++++++++++
>  drivers/iio/accel/bma400_core.c | 14 +++++++++++---
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index ae3411c090c9..13fe2e5a3175 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -106,8 +106,38 @@
>  #define BMA400_GEN1INT_CONFIG0_REG      0x3f
>  #define BMA400_GEN2INT_CONFIG0_REG      0x4A
>  #define BMA400_GENINT_CONFIG0_HYST_MASK		GENMASK(1, 0)
> +#define BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK	GENMASK(3, 2)
> +#define BMA400_GENINT_CONFIG0_DATA_SRC_MASK	BIT(4)
> +#define BMA400_GENINT_CONFIG0_X_EN_MASK		BIT(5)
> +#define BMA400_GENINT_CONFIG0_Y_EN_MASK		BIT(6)
> +#define BMA400_GENINT_CONFIG0_Z_EN_MASK		BIT(7)
> +
> +enum bma400_accel_data_src {
> +	ACCEL_FILT1,
> +	ACCEL_FILT2,
> +};
> +
> +enum bma400_ref_updt_mode {
> +	BMA400_REF_MANUAL_UPDT_MODE,
> +	BMA400_REF_ONETIME_UPDT_MODE,
> +	BMA400_REF_EVERYTIME_UPDT_MODE,
> +	BMA400_REF_EVERYTIME_LP_UPDT_MODE,
> +};
>  
>  #define BMA400_GEN_CONFIG1_OFF      0x01
> +#define BMA400_GENINT_CONFIG1_AXES_COMB_MASK	BIT(0)
> +#define BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK	BIT(1)
> +
> +enum bma400_genintr_acceleval_axescomb {
> +	BMA400_EVAL_X_OR_Y_OR_Z,
> +	BMA400_EVAL_X_AND_Y_AND_Z,

An enum for potential field values only makes sense if you are going to
use it as a type somewhere.  If not it is simpler to just
use a define next to the mask for the field.  Also these should be named
with prefixes that align with that mask.
If you do use it as a type in future patches that's fine but the naming
point still remains and you should specify values = 0, = 1 etc
as they matter whereas implication of using an enum is that the actual
values assigned doesn't matter.

Same applies to the other enums introduced in here.
> +};
> +
> +enum bma400_detect_criterion {
> +	BMA400_DETECT_INACTIVITY,
> +	BMA400_DETECT_ACTIVITY,
> +};
> +
>  #define BMA400_GEN_CONFIG2_OFF      0x02
>  #define BMA400_GEN_CONFIG3_OFF      0x03
>  #define BMA400_GEN_CONFIG31_OFF     0x04
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 35d2b90425f8..58c378ba9931 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -1166,14 +1166,16 @@ static int bma400_activity_event_en(struct bma400_data *data,
>  	case IIO_EV_DIR_RISING:
>  		reg = BMA400_GEN1INT_CONFIG0_REG;
>  		msk = BMA400_INT_CONFIG0_GEN1_MASK;
> -		value = 2;
> +		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
> +			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_ACTIVITY);
>  		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN1_MASK,
>  			      FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state));
>  		break;
>  	case IIO_EV_DIR_FALLING:
>  		reg = BMA400_GEN2INT_CONFIG0_REG;
>  		msk = BMA400_INT_CONFIG0_GEN2_MASK;
> -		value = 0;
> +		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
> +			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_INACTIVITY);
>  		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN2_MASK,
>  			      FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state));
>  		break;
> @@ -1182,7 +1184,13 @@ static int bma400_activity_event_en(struct bma400_data *data,
>  	}
>  
>  	/* Enabling all axis for interrupt evaluation */
> -	ret = regmap_write(data->regmap, reg, 0xF8);
> +	ret = regmap_write(data->regmap, reg,
> +			   BMA400_GENINT_CONFIG0_X_EN_MASK |
> +			   BMA400_GENINT_CONFIG0_Y_EN_MASK |
> +			   BMA400_GENINT_CONFIG0_Z_EN_MASK|
> +			   FIELD_PREP(BMA400_GENINT_CONFIG0_DATA_SRC_MASK, ACCEL_FILT2)|
> +			   FIELD_PREP(BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK,
> +				      BMA400_REF_EVERYTIME_UPDT_MODE));
>  	if (ret)
>  		return ret;
>  

Re: [PATCH v3 2/5] iio: accel: bma400: Use macros for generic event configuration values

[Akshay Jindal]

On Sat, Oct 4, 2025 at 6:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri,  3 Oct 2025 00:11:03 +0530
> Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> > Add macros and enums for configuration values used in generic event
> > handling for activity and inactivity detection. Replace hard-coded
> > values in activity_event_en() with the new definitions to make the
> > configuration explicit.
> >
> > No functional changes are intended.
> >
> > Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> > ---
> >  drivers/iio/accel/bma400.h      | 30 ++++++++++++++++++++++++++++++
> >  drivers/iio/accel/bma400_core.c | 14 +++++++++++---
> >  2 files changed, 41 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index ae3411c090c9..13fe2e5a3175 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -106,8 +106,38 @@
> >  #define BMA400_GEN1INT_CONFIG0_REG      0x3f
> >  #define BMA400_GEN2INT_CONFIG0_REG      0x4A
> >  #define BMA400_GENINT_CONFIG0_HYST_MASK              GENMASK(1, 0)
> > +#define BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK      GENMASK(3, 2)
> > +#define BMA400_GENINT_CONFIG0_DATA_SRC_MASK  BIT(4)
> > +#define BMA400_GENINT_CONFIG0_X_EN_MASK              BIT(5)
> > +#define BMA400_GENINT_CONFIG0_Y_EN_MASK              BIT(6)
> > +#define BMA400_GENINT_CONFIG0_Z_EN_MASK              BIT(7)
> > +
> > +enum bma400_accel_data_src {
> > +     ACCEL_FILT1,
> > +     ACCEL_FILT2,
> > +};
> > +
> > +enum bma400_ref_updt_mode {
> > +     BMA400_REF_MANUAL_UPDT_MODE,
> > +     BMA400_REF_ONETIME_UPDT_MODE,
> > +     BMA400_REF_EVERYTIME_UPDT_MODE,
> > +     BMA400_REF_EVERYTIME_LP_UPDT_MODE,
> > +};
> >
> >  #define BMA400_GEN_CONFIG1_OFF      0x01
> > +#define BMA400_GENINT_CONFIG1_AXES_COMB_MASK BIT(0)
> > +#define BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK        BIT(1)
> > +
> > +enum bma400_genintr_acceleval_axescomb {
> > +     BMA400_EVAL_X_OR_Y_OR_Z,
> > +     BMA400_EVAL_X_AND_Y_AND_Z,
>
> An enum for potential field values only makes sense if you are going to
> use it as a type somewhere.  If not it is simpler to just
> use a define next to the mask for the field.  Also these should be named
> with prefixes that align with that mask.
> If you do use it as a type in future patches that's fine but the naming
> point still remains and you should specify values = 0, = 1 etc
> as they matter whereas implication of using an enum is that the actual
> values assigned doesn't matter.
>
> Same applies to the other enums introduced in here.
Assigned explicit values to enums in v4 patch series.

Thanks,
Akshay.

Re: [PATCH v3 2/5] iio: accel: bma400: Use macros for generic event configuration values

[Jonathan Cameron]

> > > +enum bma400_genintr_acceleval_axescomb {
> > > +     BMA400_EVAL_X_OR_Y_OR_Z,
> > > +     BMA400_EVAL_X_AND_Y_AND_Z,  
> >
> > An enum for potential field values only makes sense if you are going to
> > use it as a type somewhere.  If not it is simpler to just
> > use a define next to the mask for the field.  Also these should be named
> > with prefixes that align with that mask.
> > If you do use it as a type in future patches that's fine but the naming
> > point still remains and you should specify values = 0, = 1 etc
> > as they matter whereas implication of using an enum is that the actual
> > values assigned doesn't matter.
> >
> > Same applies to the other enums introduced in here.  
> Assigned explicit values to enums in v4 patch series.
> 
Hi Akshay,

Trivial process thing.  This email doesn't need to exist. Just
have the change called out in the change log for v4.  That way it
saves a bit of time for everyone.

Thanks,

Jonathan

> Thanks,
> Akshay.

[PATCH v3 3/5] iio: accel: bma400: Use index-based register addressing and lookup

[Akshay Jindal]

Introduce formula-based macros to compute GEN INTR configuration register
addresses from the interrupt number and register index. This reduces the
need for 22 explicit register macros to three base definitions.

Add a centralized lookup table keyed by IIO event direction and replace
get_gen_config_reg() with a helper integrated with this table.

Apply these changes across the affected callbacks to ensure consistent
access to generic interrupt registers.

No functional changes are intended.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
 drivers/iio/accel/bma400.h      |  17 +++--
 drivers/iio/accel/bma400_core.c | 131 +++++++++++++++++++-------------
 2 files changed, 88 insertions(+), 60 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 13fe2e5a3175..48fcaeeb553d 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -91,6 +91,11 @@
 #define BMA400_INT_CONFIG0_GEN2_MASK		BIT(3)
 #define BMA400_INT_CONFIG0_DRDY_MASK		BIT(7)
 
+enum bma400_generic_intr {
+	BMA400_GEN1_INTR = 1,
+	BMA400_GEN2_INTR,
+};
+
 #define BMA400_INT_CONFIG1_REG	    0x20
 #define BMA400_INT_CONFIG1_STEP_INT_MASK	BIT(0)
 #define BMA400_INT_CONFIG1_S_TAP_MASK		BIT(2)
@@ -103,8 +108,12 @@
 #define BMA400_TWO_BITS_MASK        GENMASK(1, 0)
 
 /* Generic interrupts register */
-#define BMA400_GEN1INT_CONFIG0_REG      0x3f
-#define BMA400_GEN2INT_CONFIG0_REG      0x4A
+#define BMA400_GENINT_CONFIG_REG_BASE		0x3f
+#define BMA400_NUM_GENINT_CONFIG_REGS		11
+#define BMA400_GENINT_CONFIG_REG(gen_intr, config_idx)		\
+	(BMA400_GENINT_CONFIG_REG_BASE +			\
+	(gen_intr - 1) * BMA400_NUM_GENINT_CONFIG_REGS +	\
+	(config_idx))
 #define BMA400_GENINT_CONFIG0_HYST_MASK		GENMASK(1, 0)
 #define BMA400_GENINT_CONFIG0_REF_UPD_MODE_MASK	GENMASK(3, 2)
 #define BMA400_GENINT_CONFIG0_DATA_SRC_MASK	BIT(4)
@@ -138,10 +147,6 @@ enum bma400_detect_criterion {
 	BMA400_DETECT_ACTIVITY,
 };
 
-#define BMA400_GEN_CONFIG2_OFF      0x02
-#define BMA400_GEN_CONFIG3_OFF      0x03
-#define BMA400_GEN_CONFIG31_OFF     0x04
-
 /* TAP config registers */
 #define BMA400_TAP_CONFIG_REG		0x57
 #define BMA400_TAP_CONFIG_SEN_MASK	GENMASK(2, 0)
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 58c378ba9931..a0e994f9882b 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -121,6 +121,41 @@ struct bma400_data {
 	__be16 duration;
 };
 
+struct bma400_genintr_info {
+	enum bma400_generic_intr genintr;
+	unsigned int intrmask;
+	enum iio_event_direction dir;
+	enum bma400_detect_criterion detect_mode;
+};
+
+/* Lookup struct for determining GEN1/GEN2 based on dir */
+static const struct bma400_genintr_info bma400_genintrs[] = {
+	[IIO_EV_DIR_RISING] = {
+		.genintr = BMA400_GEN1_INTR,
+		.intrmask = BMA400_INT_CONFIG0_GEN1_MASK,
+		.dir = IIO_EV_DIR_RISING,
+		.detect_mode = BMA400_DETECT_ACTIVITY,
+	},
+	[IIO_EV_DIR_FALLING] = {
+		.genintr = BMA400_GEN2_INTR,
+		.intrmask = BMA400_INT_CONFIG0_GEN2_MASK,
+		.dir = IIO_EV_DIR_FALLING,
+		.detect_mode = BMA400_DETECT_INACTIVITY,
+	}
+};
+
+static inline const struct bma400_genintr_info *
+get_bma400_genintr_info(enum iio_event_direction dir)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+	case IIO_EV_DIR_FALLING:
+		return &bma400_genintrs[dir];
+	default:
+		return NULL;
+	};
+}
+
 static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -1159,32 +1194,22 @@ static int bma400_activity_event_en(struct bma400_data *data,
 				    enum iio_event_direction dir,
 				    int state)
 {
-	int ret, reg, msk, value;
-	int field_value = 0;
+	int ret;
+	unsigned int intrmask, regval;
+	enum bma400_generic_intr genintr;
+	enum bma400_detect_criterion detect_criterion;
+	const struct bma400_genintr_info *bma400_genintr;
 
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		reg = BMA400_GEN1INT_CONFIG0_REG;
-		msk = BMA400_INT_CONFIG0_GEN1_MASK;
-		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
-			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_ACTIVITY);
-		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN1_MASK,
-			      FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state));
-		break;
-	case IIO_EV_DIR_FALLING:
-		reg = BMA400_GEN2INT_CONFIG0_REG;
-		msk = BMA400_INT_CONFIG0_GEN2_MASK;
-		value = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
-			FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, BMA400_DETECT_INACTIVITY);
-		set_mask_bits(&field_value, BMA400_INT_CONFIG0_GEN2_MASK,
-			      FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state));
-		break;
-	default:
+	bma400_genintr = get_bma400_genintr_info(dir);
+	if (!bma400_genintr)
 		return -EINVAL;
-	}
+
+	genintr = bma400_genintr->genintr;
+	detect_criterion = bma400_genintr->detect_mode;
+	intrmask = bma400_genintr->intrmask;
 
 	/* Enabling all axis for interrupt evaluation */
-	ret = regmap_write(data->regmap, reg,
+	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 0),
 			   BMA400_GENINT_CONFIG0_X_EN_MASK |
 			   BMA400_GENINT_CONFIG0_Y_EN_MASK |
 			   BMA400_GENINT_CONFIG0_Z_EN_MASK|
@@ -1195,31 +1220,32 @@ static int bma400_activity_event_en(struct bma400_data *data,
 		return ret;
 
 	/* OR combination of all axis for interrupt evaluation */
-	ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF, value);
+	regval = FIELD_PREP(BMA400_GENINT_CONFIG1_AXES_COMB_MASK, BMA400_EVAL_X_OR_Y_OR_Z) |
+		 FIELD_PREP(BMA400_GENINT_CONFIG1_DETCT_CRIT_MASK, detect_criterion);
+	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 1), regval);
 	if (ret)
 		return ret;
 
 	/* Initial value to avoid interrupts while enabling*/
-	ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF, 0x0A);
+	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 2), 0x0A);
 	if (ret)
 		return ret;
 
 	/* Initial duration value to avoid interrupts while enabling*/
-	ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF, 0x0F);
+	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 4), 0x0F);
 	if (ret)
 		return ret;
 
-	ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, msk,
-				 field_value);
+	regval = state ? intrmask : 0;
+	ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, intrmask, regval);
 	if (ret)
 		return ret;
 
-	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG, msk,
-				 field_value);
+	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG, intrmask, regval);
 	if (ret)
 		return ret;
 
-	set_mask_bits(&data->generic_event_en, msk, field_value);
+	set_mask_bits(&data->generic_event_en, intrmask, regval);
 	return 0;
 }
 
@@ -1344,18 +1370,6 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
 	}
 }
 
-static int get_gen_config_reg(enum iio_event_direction dir)
-{
-	switch (dir) {
-	case IIO_EV_DIR_FALLING:
-		return BMA400_GEN2INT_CONFIG0_REG;
-	case IIO_EV_DIR_RISING:
-		return BMA400_GEN1INT_CONFIG0_REG;
-	default:
-		return -EINVAL;
-	}
-}
-
 static int bma400_read_event_value(struct iio_dev *indio_dev,
 				   const struct iio_chan_spec *chan,
 				   enum iio_event_type type,
@@ -1364,22 +1378,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 				   int *val, int *val2)
 {
 	struct bma400_data *data = iio_priv(indio_dev);
-	int ret, reg, reg_val, raw;
+	int ret, reg_val, raw;
+	enum bma400_generic_intr genintr;
+	const struct bma400_genintr_info *bma400_genintr;
 
 	if (chan->type != IIO_ACCEL)
 		return -EINVAL;
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
-		reg = get_gen_config_reg(dir);
-		if (reg < 0)
+		bma400_genintr = get_bma400_genintr_info(dir);
+		if (!bma400_genintr)
 			return -EINVAL;
+		genintr = bma400_genintr->genintr;
 
 		*val2 = 0;
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
 			ret = regmap_read(data->regmap,
-					  reg + BMA400_GEN_CONFIG2_OFF,
+					  BMA400_GENINT_CONFIG_REG(genintr, 2),
 					  val);
 			if (ret)
 				return ret;
@@ -1387,7 +1404,7 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 		case IIO_EV_INFO_PERIOD:
 			mutex_lock(&data->mutex);
 			ret = regmap_bulk_read(data->regmap,
-					       reg + BMA400_GEN_CONFIG3_OFF,
+					       BMA400_GENINT_CONFIG_REG(genintr, 3),
 					       &data->duration,
 					       sizeof(data->duration));
 			if (ret) {
@@ -1398,7 +1415,9 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 			mutex_unlock(&data->mutex);
 			return IIO_VAL_INT;
 		case IIO_EV_INFO_HYSTERESIS:
-			ret = regmap_read(data->regmap, reg, val);
+			ret = regmap_read(data->regmap,
+					  BMA400_GENINT_CONFIG_REG(genintr, 0),
+					  val);
 			if (ret)
 				return ret;
 			*val = FIELD_GET(BMA400_GENINT_CONFIG0_HYST_MASK, *val);
@@ -1452,16 +1471,19 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				    int val, int val2)
 {
 	struct bma400_data *data = iio_priv(indio_dev);
-	int reg, ret, raw;
+	int ret, raw;
+	enum bma400_generic_intr genintr;
+	const struct bma400_genintr_info *bma400_genintr;
 
 	if (chan->type != IIO_ACCEL)
 		return -EINVAL;
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
-		reg = get_gen_config_reg(dir);
-		if (reg < 0)
+		bma400_genintr = get_bma400_genintr_info(dir);
+		if (!bma400_genintr)
 			return -EINVAL;
+		genintr = bma400_genintr->genintr;
 
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -1469,7 +1491,7 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				return -EINVAL;
 
 			return regmap_write(data->regmap,
-					    reg + BMA400_GEN_CONFIG2_OFF,
+					    BMA400_GENINT_CONFIG_REG(genintr, 2),
 					    val);
 		case IIO_EV_INFO_PERIOD:
 			if (val < 1 || val > 65535)
@@ -1478,7 +1500,7 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 			mutex_lock(&data->mutex);
 			put_unaligned_be16(val, &data->duration);
 			ret = regmap_bulk_write(data->regmap,
-						reg + BMA400_GEN_CONFIG3_OFF,
+						BMA400_GENINT_CONFIG_REG(genintr, 3),
 						&data->duration,
 						sizeof(data->duration));
 			mutex_unlock(&data->mutex);
@@ -1487,7 +1509,8 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 			if (val < 0 || val > 3)
 				return -EINVAL;
 
-			return regmap_update_bits(data->regmap, reg,
+			return regmap_update_bits(data->regmap,
+						  BMA400_GENINT_CONFIG_REG(genintr, 0),
 						  BMA400_GENINT_CONFIG0_HYST_MASK,
 						  FIELD_PREP(BMA400_GENINT_CONFIG0_HYST_MASK,
 							     val));
-- 
2.43.0

Re: [PATCH v3 3/5] iio: accel: bma400: Use index-based register addressing and lookup

[Jonathan Cameron]

On Fri,  3 Oct 2025 00:11:04 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Introduce formula-based macros to compute GEN INTR configuration register
> addresses from the interrupt number and register index. This reduces the
> need for 22 explicit register macros to three base definitions.
> 
> Add a centralized lookup table keyed by IIO event direction and replace
> get_gen_config_reg() with a helper integrated with this table.
> 
> Apply these changes across the affected callbacks to ensure consistent
> access to generic interrupt registers.
> 
> No functional changes are intended.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
>  drivers/iio/accel/bma400.h      |  17 +++--
>  drivers/iio/accel/bma400_core.c | 131 +++++++++++++++++++-------------
>  2 files changed, 88 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 13fe2e5a3175..48fcaeeb553d 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -91,6 +91,11 @@
>  #define BMA400_INT_CONFIG0_GEN2_MASK		BIT(3)
>  #define BMA400_INT_CONFIG0_DRDY_MASK		BIT(7)
>  
> +enum bma400_generic_intr {
> +	BMA400_GEN1_INTR = 1,
> +	BMA400_GEN2_INTR,
Given the values matter, specify them both.

> +};

Re: [PATCH v3 3/5] iio: accel: bma400: Use index-based register addressing and lookup

[Akshay Jindal]

On Sat, Oct 4, 2025 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> > +enum bma400_generic_intr {
> > +     BMA400_GEN1_INTR = 1,
> > +     BMA400_GEN2_INTR,
> Given the values matter, specify them both.
>
done.

Thanks,
Akshay

[PATCH v3 4/5] iio: accel: bma400: Rename activity_event_en() to generic_event_en()

[Akshay Jindal]

The function activity_event_en() configures the generic interrupts
GEN1 and GEN2, which are used for activity and inactivity detection
as per the datasheet. The existing name is misleading, since the
device also provides activity change and activity recognition
interrupts. Activity change interrupt is not supported yet whereas
Activity recognition interrupt is configured in a different function.

Rename activity_event_en() to generic_event_en() to better reflect its
actual purpose.

No functional changes intended.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
 drivers/iio/accel/bma400_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index a0e994f9882b..41e95291b36b 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -1190,9 +1190,9 @@ static int bma400_steps_event_enable(struct bma400_data *data, int state)
 	return 0;
 }
 
-static int bma400_activity_event_en(struct bma400_data *data,
-				    enum iio_event_direction dir,
-				    int state)
+static int bma400_generic_event_en(struct bma400_data *data,
+				   enum iio_event_direction dir,
+				   int state)
 {
 	int ret;
 	unsigned int intrmask, regval;
@@ -1337,7 +1337,7 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
 		switch (type) {
 		case IIO_EV_TYPE_MAG:
 			mutex_lock(&data->mutex);
-			ret = bma400_activity_event_en(data, dir, state);
+			ret = bma400_generic_event_en(data, dir, state);
 			mutex_unlock(&data->mutex);
 			return ret;
 		case IIO_EV_TYPE_GESTURE:
-- 
2.43.0

[PATCH v3 5/5] iio: accel: bma400: Add detail to comments in GEN INTR configuration

[Akshay Jindal]

Append additional information to existing comments in the generic
interrupt configuration code to provide more context.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
 drivers/iio/accel/bma400_core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 41e95291b36b..c42cdf0a1645 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -1208,7 +1208,10 @@ static int bma400_generic_event_en(struct bma400_data *data,
 	detect_criterion = bma400_genintr->detect_mode;
 	intrmask = bma400_genintr->intrmask;
 
-	/* Enabling all axis for interrupt evaluation */
+	/*
+	 * Enabling all axis for interrupt evaluation
+	 * Acc_filt2 is recommended as data source in datasheet (Section 4.7)
+	 */
 	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 0),
 			   BMA400_GENINT_CONFIG0_X_EN_MASK |
 			   BMA400_GENINT_CONFIG0_Y_EN_MASK |
@@ -1226,7 +1229,10 @@ static int bma400_generic_event_en(struct bma400_data *data,
 	if (ret)
 		return ret;
 
-	/* Initial value to avoid interrupts while enabling*/
+	/*
+	 * Initial value to avoid interrupts while enabling
+	 * Value is in units of 8mg/lsb, i.e. effective val is val * 8mg/lsb
+	 */
 	ret = regmap_write(data->regmap, BMA400_GENINT_CONFIG_REG(genintr, 2), 0x0A);
 	if (ret)
 		return ret;
-- 
2.43.0