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

[Jonathan Cameron <jic23@kernel.org>]

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;