drivers/iio/accel/bma400.h | 71 +++++++++++++++--- drivers/iio/accel/bma400_core.c | 128 ++++++++++++++++---------------- 2 files changed, 125 insertions(+), 74 deletions(-)
Refactor the generic interrupt configuration logic to improve readability
and reduce repetitive code. Replace hard-coded register values with macros
and enums, making it clearer what configuration is written to hardware.
Introduce a const struct to map event direction to the corresponding
generic interrupt. This removes the need for switch statements in multiple
callbacks, including activity event setup, read_event_value, and
write_event_value, while still performing the selection at runtime.
This change has no functional impact but simplifies the code and makes it
easier to maintain and extend in future updates.
Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
drivers/iio/accel/bma400.h | 71 +++++++++++++++---
drivers/iio/accel/bma400_core.c | 128 ++++++++++++++++----------------
2 files changed, 125 insertions(+), 74 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 932358b45f17..ab7d1d139b66 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -68,7 +68,19 @@
#define BMA400_CMD_REG 0x7e
/* Interrupt registers */
-#define BMA400_INT_CONFIG0_REG 0x1f
+#define BMA400_INT_CONFIG0_REG 0x1f
+#define BMA400_INT_CONFIG0_ORTN_CHG_MASK BIT(1)
+#define BMA400_INT_CONFIG0_GEN1_MASK BIT(2)
+#define BMA400_INT_CONFIG0_GEN2_MASK BIT(3)
+#define BMA400_INT_CONFIG0_FIFO_FULL_MASK BIT(5)
+#define BMA400_INT_CONFIG0_FIFO_WTRMRK_MASK BIT(6)
+#define BMA400_INT_CONFIG0_DRDY_MASK BIT(7)
+
+enum generic_intr {
+ GEN1_INTR,
+ GEN2_INTR
+};
+
#define BMA400_INT_CONFIG1_REG 0x20
#define BMA400_INT1_MAP_REG 0x21
#define BMA400_INT_IO_CTRL_REG 0x24
@@ -96,15 +108,53 @@
#define BMA400_ACC_ODR_MIN_HZ 12
/* Generic interrupts register */
-#define BMA400_GEN1INT_CONFIG0 0x3f
-#define BMA400_GEN2INT_CONFIG0 0x4A
-#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)
+#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) * BMA400_NUM_GENINT_CONFIG_REGS + \
+ (config_idx))
+
+/* Generic Interrupt Config0 register */
+#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_hysteresis_config {
+ NO_HYSTERESIS,
+ HYSTERESIS_24MG,
+ HYSTERESIS_48MG,
+ HYSTERESIS_96MG
+};
+
+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
+};
+
+/* Generic Interrupt Config1 register */
+#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,
+};
/* TAP config registers */
#define BMA400_TAP_CONFIG 0x57
@@ -119,6 +169,7 @@
#define BMA400_TAP_QUIETDT_MSK GENMASK(5, 4)
#define BMA400_TAP_TIM_LIST_LEN 4
+
/*
* BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
* converting to micro values for +-2g range.
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 85e23badf733..d59c26b8a57f 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -121,6 +121,29 @@ struct bma400_data {
__be16 duration;
};
+struct bma400_genintr_info {
+ u8 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 = GEN1_INTR, /* 0 for GEN1 */
+ .intrmask = BMA400_INT_CONFIG0_GEN1_MASK,
+ .dir = IIO_EV_DIR_RISING,
+ .detect_mode = BMA400_DETECT_ACTIVITY,
+ },
+ [IIO_EV_DIR_FALLING] = {
+ .genintr = GEN2_INTR, /* 1 for GEN2 */
+ .intrmask = BMA400_INT_CONFIG0_GEN2_MASK,
+ .dir = IIO_EV_DIR_FALLING,
+ .detect_mode = BMA400_DETECT_INACTIVITY,
+ }
+};
+
static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
@@ -1114,10 +1137,10 @@ 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,
@@ -1159,59 +1182,50 @@ 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;
-
- switch (dir) {
- case IIO_EV_DIR_RISING:
- reg = BMA400_GEN1INT_CONFIG0;
- msk = BMA400_INT_GEN1_MSK;
- value = 2;
- set_mask_bits(&field_value, BMA400_INT_GEN1_MSK,
- FIELD_PREP(BMA400_INT_GEN1_MSK, state));
- break;
- case IIO_EV_DIR_FALLING:
- reg = BMA400_GEN2INT_CONFIG0;
- msk = BMA400_INT_GEN2_MSK;
- value = 0;
- set_mask_bits(&field_value, BMA400_INT_GEN2_MSK,
- FIELD_PREP(BMA400_INT_GEN2_MSK, state));
- break;
- default:
- return -EINVAL;
- }
+ int ret, regval;
+ u8 genintr = bma400_genintrs[dir].genintr;
+ u8 detect_criterion = bma400_genintrs[dir].detect_mode;
+ unsigned int intrmask = bma400_genintrs[dir].intrmask;
/* Enabling all axis for interrupt evaluation */
- ret = regmap_write(data->regmap, reg, 0xF8);
+ 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|
+ BMA400_REF_EVERYTIME_UPDT_MODE);
if (ret)
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 = FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state);
+ if (genintr)
+ regval = FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state);
+
+ 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;
}
@@ -1336,18 +1350,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;
- case IIO_EV_DIR_RISING:
- return BMA400_GEN1INT_CONFIG0;
- 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,
@@ -1356,22 +1358,20 @@ 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, genintr, reg_val, raw;
if (chan->type != IIO_ACCEL)
return -EINVAL;
switch (type) {
case IIO_EV_TYPE_MAG:
- reg = get_gen_config_reg(dir);
- if (reg < 0)
- return -EINVAL;
+ genintr = bma400_genintrs[dir].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;
@@ -1379,7 +1379,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) {
@@ -1390,10 +1390,12 @@ 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_GEN_HYST_MSK, *val);
+ *val = FIELD_GET(BMA400_GENINT_CONFIG0_HYST_MASK, *val);
return IIO_VAL_INT;
default:
return -EINVAL;
@@ -1444,16 +1446,14 @@ 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 genintr, ret, raw;
if (chan->type != IIO_ACCEL)
return -EINVAL;
switch (type) {
case IIO_EV_TYPE_MAG:
- reg = get_gen_config_reg(dir);
- if (reg < 0)
- return -EINVAL;
+ genintr = bma400_genintrs[dir].genintr;
switch (info) {
case IIO_EV_INFO_VALUE:
@@ -1461,7 +1461,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)
@@ -1470,7 +1470,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);
@@ -1479,10 +1479,10 @@ 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,
- BMA400_GEN_HYST_MSK,
- FIELD_PREP(BMA400_GEN_HYST_MSK,
- val));
+ 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));
default:
return -EINVAL;
}
@@ -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) {
--
2.43.0Hi,
Thanks for your patch.
On Fri, 2025-09-19 at 00:37 +0530, Akshay Jindal wrote:
> Refactor the generic interrupt configuration logic to improve readability
> and reduce repetitive code. Replace hard-coded register values with macros
> and enums, making it clearer what configuration is written to hardware.
>
> Introduce a const struct to map event direction to the corresponding
> generic interrupt. This removes the need for switch statements in multiple
> callbacks, including activity event setup, read_event_value, and
> write_event_value, while still performing the selection at runtime.
>
> This change has no functional impact but simplifies the code and makes it
> easier to maintain and extend in future updates.
>
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
You may be trying to refactor too much in one single patch. I would maybe think
about splitting this into 2/3 logical changes.
> drivers/iio/accel/bma400.h | 71 +++++++++++++++---
> drivers/iio/accel/bma400_core.c | 128 ++++++++++++++++----------------
> 2 files changed, 125 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 932358b45f17..ab7d1d139b66 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -68,7 +68,19 @@
> #define BMA400_CMD_REG 0x7e
>
> /* Interrupt registers */
> -#define BMA400_INT_CONFIG0_REG 0x1f
> +#define BMA400_INT_CONFIG0_REG 0x1f
> +#define BMA400_INT_CONFIG0_ORTN_CHG_MASK BIT(1)
> +#define BMA400_INT_CONFIG0_GEN1_MASK BIT(2)
> +#define BMA400_INT_CONFIG0_GEN2_MASK BIT(3)
> +#define BMA400_INT_CONFIG0_FIFO_FULL_MASK BIT(5)
> +#define BMA400_INT_CONFIG0_FIFO_WTRMRK_MASK BIT(6)
> +#define BMA400_INT_CONFIG0_DRDY_MASK BIT(7)
> +
> +enum generic_intr {
> + GEN1_INTR,
> + GEN2_INTR
> +};
> +
> #define BMA400_INT_CONFIG1_REG 0x20
> #define BMA400_INT1_MAP_REG 0x21
> #define BMA400_INT_IO_CTRL_REG 0x24
> @@ -96,15 +108,53 @@
> #define BMA400_ACC_ODR_MIN_HZ 12
>
> /* Generic interrupts register */
> -#define BMA400_GEN1INT_CONFIG0 0x3f
> -#define BMA400_GEN2INT_CONFIG0 0x4A
> -#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)
> +#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) * BMA400_NUM_GENINT_CONFIG_REGS + \
> + (config_idx))
Not sure the above macro helps that much. More on this below...
> +
> +/* Generic Interrupt Config0 register */
> +#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_hysteresis_config {
> + NO_HYSTERESIS,
> + HYSTERESIS_24MG,
> + HYSTERESIS_48MG,
> + HYSTERESIS_96MG
> +};
> +
> +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
> +};
> +
> +/* Generic Interrupt Config1 register */
> +#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,
> +};
>
> /* TAP config registers */
> #define BMA400_TAP_CONFIG 0x57
> @@ -119,6 +169,7 @@
> #define BMA400_TAP_QUIETDT_MSK GENMASK(5, 4)
> #define BMA400_TAP_TIM_LIST_LEN 4
>
> +
> /*
> * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> * converting to micro values for +-2g range.
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 85e23badf733..d59c26b8a57f 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -121,6 +121,29 @@ struct bma400_data {
> __be16 duration;
> };
>
> +struct bma400_genintr_info {
> + u8 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 = GEN1_INTR, /* 0 for GEN1 */
> + .intrmask = BMA400_INT_CONFIG0_GEN1_MASK,
> + .dir = IIO_EV_DIR_RISING,
> + .detect_mode = BMA400_DETECT_ACTIVITY,
> + },
> + [IIO_EV_DIR_FALLING] = {
> + .genintr = GEN2_INTR, /* 1 for GEN2 */
> + .intrmask = BMA400_INT_CONFIG0_GEN2_MASK,
> + .dir = IIO_EV_DIR_FALLING,
> + .detect_mode = BMA400_DETECT_INACTIVITY,
> + }
> +};
> +
> static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> {
> switch (reg) {
> @@ -1114,10 +1137,10 @@ 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);
Like the above is above renaming defines in a more logical name. Not going to
opinate whether this name is better or not but you could put that (and other
similar changes) in one patch.
> 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,
> @@ -1159,59 +1182,50 @@ 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;
> -
> - switch (dir) {
> - case IIO_EV_DIR_RISING:
> - reg = BMA400_GEN1INT_CONFIG0;
> - msk = BMA400_INT_GEN1_MSK;
> - value = 2;
> - set_mask_bits(&field_value, BMA400_INT_GEN1_MSK,
> - FIELD_PREP(BMA400_INT_GEN1_MSK, state));
> - break;
> - case IIO_EV_DIR_FALLING:
> - reg = BMA400_GEN2INT_CONFIG0;
> - msk = BMA400_INT_GEN2_MSK;
> - value = 0;
> - set_mask_bits(&field_value, BMA400_INT_GEN2_MSK,
> - FIELD_PREP(BMA400_INT_GEN2_MSK, state));
> - break;
> - default:
> - return -EINVAL;
> - }
> + int ret, regval;
> + u8 genintr = bma400_genintrs[dir].genintr;
> + u8 detect_criterion = bma400_genintrs[dir].detect_mode;
> + unsigned int intrmask = bma400_genintrs[dir].intrmask;
>
Hmm, you should still have the switch case. Again, not sure the const struct
adds that much but I'm also fine with it. But I would do:
switch (dir) {
case IIO_EV_DIR_RISING:
case IIO_EV_DIR_FALLING:
genintr = bma400_genintrs[dir].genintr;
detect_criterion = bma400_genintrs[dir].detect_mode;
intrmask = bma400_genintrs[dir].intrmask;
default:
return -EINVAL;
> /* Enabling all axis for interrupt evaluation */
> - ret = regmap_write(data->regmap, reg, 0xF8);
> + 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|
> + BMA400_REF_EVERYTIME_UPDT_MODE);
> if (ret)
> 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);
I do not think readability is improved here. The former is way easier to read
IMO. Also, we still have the magic 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 = FIELD_PREP(BMA400_INT_CONFIG0_GEN1_MASK, state);
> + if (genintr)
> + regval = FIELD_PREP(BMA400_INT_CONFIG0_GEN2_MASK, state);
> +
> + 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;
> }
>
> @@ -1336,18 +1350,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;
> - case IIO_EV_DIR_RISING:
> - return BMA400_GEN1INT_CONFIG0;
> - 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,
> @@ -1356,22 +1358,20 @@ 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, genintr, reg_val, raw;
>
> if (chan->type != IIO_ACCEL)
> return -EINVAL;
>
> switch (type) {
> case IIO_EV_TYPE_MAG:
> - reg = get_gen_config_reg(dir);
> - if (reg < 0)
> - return -EINVAL;
> + genintr = bma400_genintrs[dir].genintr;
Again you should make sure dir is valid. You could keep get_gen_config_reg() and
do something similar with what I suggested above.
>
> *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;
> @@ -1379,7 +1379,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) {
> @@ -1390,10 +1390,12 @@ 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);
One more case where I don't really think this macro is doing readability any
better.
- Nuno Sá
Hi Nuno,
Thank you so much for your review. Please find follow-ups inline.
On Fri, Sep 19, 2025 at 5:34 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> Hi,
>
> Thanks for your patch.
>
> On Fri, 2025-09-19 at 00:37 +0530, Akshay Jindal wrote:
> > Refactor the generic interrupt configuration logic to improve readability
> > and reduce repetitive code. Replace hard-coded register values with macros
> > and enums, making it clearer what configuration is written to hardware.
> >
> > Introduce a const struct to map event direction to the corresponding
> > generic interrupt. This removes the need for switch statements in multiple
> > callbacks, including activity event setup, read_event_value, and
> > write_event_value, while still performing the selection at runtime.
> >
> > This change has no functional impact but simplifies the code and makes it
> > easier to maintain and extend in future updates.
> >
> > Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> > ---
>
> You may be trying to refactor too much in one single patch. I would maybe think
> about splitting this into 2/3 logical changes.
>
> > drivers/iio/accel/bma400.h | 71 +++++++++++++++---
> > drivers/iio/accel/bma400_core.c | 128 ++++++++++++++++----------------
> > 2 files changed, 125 insertions(+), 74 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index 932358b45f17..ab7d1d139b66 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -68,7 +68,19 @@
> > #define BMA400_CMD_REG 0x7e
> >
> > /* Interrupt registers */
> > -#define BMA400_INT_CONFIG0_REG 0x1f
> > +#define BMA400_INT_CONFIG0_REG 0x1f
> > +#define BMA400_INT_CONFIG0_ORTN_CHG_MASK BIT(1)
> > +#define BMA400_INT_CONFIG0_GEN1_MASK BIT(2)
> > +#define BMA400_INT_CONFIG0_GEN2_MASK BIT(3)
> > +#define BMA400_INT_CONFIG0_FIFO_FULL_MASK BIT(5)
> > +#define BMA400_INT_CONFIG0_FIFO_WTRMRK_MASK BIT(6)
> > +#define BMA400_INT_CONFIG0_DRDY_MASK BIT(7)
> > +
> > +enum generic_intr {
> > + GEN1_INTR,
> > + GEN2_INTR
> > +};
> > +
> > #define BMA400_INT_CONFIG1_REG 0x20
> > #define BMA400_INT1_MAP_REG 0x21
> > #define BMA400_INT_IO_CTRL_REG 0x24
> > @@ -96,15 +108,53 @@
> > #define BMA400_ACC_ODR_MIN_HZ 12
> >
> > /* Generic interrupts register */
> > -#define BMA400_GEN1INT_CONFIG0 0x3f
> > -#define BMA400_GEN2INT_CONFIG0 0x4A
> > -#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)
> > +#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) * BMA400_NUM_GENINT_CONFIG_REGS + \
> > + (config_idx))
>
> Not sure the above macro helps that much. More on this below...
In my last patch regarding ltr390, I was given feedback towards
reducing the number of macro definitions. This may look little cryptic,
but it enables me to access GEN1(11) + GEN2 (11) registers with only
2 definitions. I see it as a reasonable tradeoff between readability and
code volume.
> > static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> > {
> > switch (reg) {
> > @@ -1114,10 +1137,10 @@ 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);
>
> Like the above is above renaming defines in a more logical name. Not going to
> opinate whether this name is better or not but you could put that (and other
> similar changes) in one patch.
Initially, I thought of doing this, but if you look at the diff in bma400.h,
only GEN1_MASK and GEN2_MASK have been renamed and re-placed closer
to their register definition macro. Rest all of them are either new
introductions
or have gone for good. Hence I felt it might be an overkill to create
a new patch
just for 2 reg bit renaming. At the same time, I feel new
introductions should be
clubbed with their usage. Ofcourse, all of them are not being used, but even if
the code is using 1 bit of a reg, then all of its fields should be defined.
Due to the above logic, I kept it in a single patch. Although happy to change if
you still feel otherwise.
>
> > 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,
> > @@ -1159,59 +1182,50 @@ 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;
> > -
> > - switch (dir) {
> > - case IIO_EV_DIR_RISING:
> > - reg = BMA400_GEN1INT_CONFIG0;
> > - msk = BMA400_INT_GEN1_MSK;
> > - value = 2;
> > - set_mask_bits(&field_value, BMA400_INT_GEN1_MSK,
> > - FIELD_PREP(BMA400_INT_GEN1_MSK, state));
> > - break;
> > - case IIO_EV_DIR_FALLING:
> > - reg = BMA400_GEN2INT_CONFIG0;
> > - msk = BMA400_INT_GEN2_MSK;
> > - value = 0;
> > - set_mask_bits(&field_value, BMA400_INT_GEN2_MSK,
> > - FIELD_PREP(BMA400_INT_GEN2_MSK, state));
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > + int ret, regval;
> > + u8 genintr = bma400_genintrs[dir].genintr;
> > + u8 detect_criterion = bma400_genintrs[dir].detect_mode;
> > + unsigned int intrmask = bma400_genintrs[dir].intrmask;
> >
>
> Hmm, you should still have the switch case. Again, not sure the const struct
> adds that much but I'm also fine with it. But I would do:
>
> switch (dir) {
> case IIO_EV_DIR_RISING:
> case IIO_EV_DIR_FALLING:
> genintr = bma400_genintrs[dir].genintr;
> detect_criterion = bma400_genintrs[dir].detect_mode;
> intrmask = bma400_genintrs[dir].intrmask;
> default:
> return -EINVAL;
>
Thank you for detailing it out. Yes this gives the best of both worlds.
Uses the centralized lookup struct as well as makes sure dir is sanitized.
Will wrap it up in a function and apply in the v2.
Thanks,
Akshay.
© 2016 - 2025 Red Hat, Inc.