| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251004135312.41858380@jic23-huawei>
Date: Sat, 4 Oct 2025 13:53:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: dan@...obertson.com, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, shuah@...nel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename
register and field macros
On Fri, 3 Oct 2025 00:11:02 +0530
Akshay Jindal <akshayaj.lkd@...il.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@...il.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;
Powered by blists - more mailing lists