[RFC PATCH v2 0/2] regmap: add APIs to bypass regmap_field and work with reg_field directly

[RFC PATCH v2 0/2] regmap: add APIs to bypass regmap_field and work with reg_field directly

[Victor Chernyakin]

Working with the existing regmap_field_* APIs requires you to define
a reg_field, then use one of the regmap_field_*_alloc functions to turn
it into a dynamically allocated regmap_field. But reg_field and
regmap_field are practically the same struct; all the *_alloc functions do
is shuffle the data around. So this RFC proposes adding functions that
cut out the middleman and take reg_field directly. Patch 1 adds two
of them: regmap_reg_field_read and regmap_reg_field_write.

Patch 2 ports a driver to use these new functions. The result is
shorter and simpler, since all the code to perform the allocations
and manage the resulting regmap_fields goes away. In theory,
there should also be a performance benefit (we're doing fewer allocations
and have less duplicated state), but I haven't benchmarked it, so take that
with a grain of salt.

If we like this approach, I can post a v3 that adds regmap_reg_field_*
analogs for all the regmap_field_* functions, not just the two you see
here.


This is not actually a new idea! Both it and the current *_alloc approach
were suggested as alternatives right when the regmap_field_* APIs were
being added [1]. The *_alloc approach was chosen without much
discussion, but I think (and Patch 2 demonstrates, I hope) that
this approach is the cleaner one.

[1] https://lore.kernel.org/20130604210137.GV31367@sirena.org.uk

---
This series depends on:
	cbd74ec34c06 ("regmap: Add REG_FIELD_GENMASK() helper macro")

---
Changes in v2:
  - Used REG_FIELD_GENMASK where possible
  - Removed stale doc comment for the 'fields' struct member in Patch 2

Link to v1: https://lore.kernel.org/SN1PR12MB2528754CEF9D8E5103ECAE28A5772@SN1PR12MB2528.namprd12.prod.outlook.com

---
Victor Chernyakin (2):
  regmap: add regmap_reg_field_* APIs
  iio: accel: msa311: use regmap_reg_field_* functions

 drivers/iio/accel/msa311.c | 297 +++++++++++++------------------------
 include/linux/regmap.h     |  64 ++++++--
 2 files changed, 158 insertions(+), 203 deletions(-)

-- 
2.53.0

[RFC PATCH v2 1/2] regmap: add regmap_reg_field_* APIs

[Victor Chernyakin]

These functions are equivalent to their regmap_field_* counterparts,
but allow working with reg_field directly, without having to
allocate and manage regmap_fields.

Signed-off-by: Victor Chernyakin <chernyakin.victor.j@outlook.com>
---
 include/linux/regmap.h | 64 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 186581e96b13..71df96b9da7a 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -10,6 +10,7 @@
  * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/bug.h>
 #include <linux/cleanup.h>
@@ -1464,19 +1465,19 @@ struct reg_field {
 	unsigned int id_offset;
 };
 
-#define REG_FIELD(_reg, _lsb, _msb) {		\
-				.reg = _reg,	\
-				.lsb = _lsb,	\
-				.msb = _msb,	\
-				}
+#define REG_FIELD(_reg, _lsb, _msb) ((struct reg_field){ \
+				.reg = _reg,		 \
+				.lsb = _lsb,		 \
+				.msb = _msb,		 \
+				})
 
-#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) {	\
-				.reg = _reg,			\
-				.lsb = _lsb,			\
-				.msb = _msb,			\
-				.id_size = _size,		\
-				.id_offset = _offset,		\
-				}
+#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) ((struct reg_field){ \
+				.reg = _reg,				    \
+				.lsb = _lsb,				    \
+				.msb = _msb,				    \
+				.id_size = _size,			    \
+				.id_offset = _offset,			    \
+				})
 
 #define REG_FIELD_GENMASK(_reg_field)	GENMASK(_reg_field.msb, _reg_field.lsb)
 
@@ -1586,6 +1587,29 @@ regmap_fields_force_update_bits(struct regmap_field *field, unsigned int id,
 					      NULL, false, true);
 }
 
+static inline int
+regmap_reg_field_read(struct regmap *regmap, struct reg_field field,
+		      unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(regmap, field.reg, val);
+	if (ret)
+		return ret;
+
+	*val = field_get(REG_FIELD_GENMASK(field), *val);
+
+	return ret;
+}
+
+static inline int
+regmap_reg_field_write(struct regmap *regmap, struct reg_field field,
+		       unsigned int val)
+{
+	return regmap_update_bits(regmap, field.reg,
+				  REG_FIELD_GENMASK(field), val << field.lsb);
+}
+
 /**
  * struct regmap_irq_type - IRQ type definitions.
  *
@@ -2074,6 +2098,22 @@ regmap_fields_force_update_bits(struct regmap_field *field, unsigned int id,
 	return -EINVAL;
 }
 
+static inline int
+regmap_reg_field_read(struct regmap *regmap, struct reg_field field,
+		      unsigned int *val)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
+static inline int
+regmap_reg_field_write(struct regmap *regmap, struct reg_field field,
+		       unsigned int val)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
 static inline int regmap_get_val_bytes(struct regmap *map)
 {
 	WARN_ONCE(1, "regmap API is disabled");
-- 
2.53.0

[RFC PATCH v2 2/2] iio: accel: msa311: use regmap_reg_field_* functions

[Victor Chernyakin]

Simplify the driver by using the new regmap_reg_field_* functions.
regmap_reg_field_FOO is equivalent to regmap_field_FOO, but takes
a reg_field directly instead of a regmap_field.

Tested by compiling.

Signed-off-by: Victor Chernyakin <chernyakin.victor.j@outlook.com>
---
 drivers/iio/accel/msa311.c | 298 +++++++++++++------------------------
 1 file changed, 106 insertions(+), 192 deletions(-)

diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
index caf9b1b6397f..63c795d6bc99 100644
--- a/drivers/iio/accel/msa311.c
+++ b/drivers/iio/accel/msa311.c
@@ -75,137 +75,85 @@
 #define MSA311_OFFSET_Y_REG       0x39
 #define MSA311_OFFSET_Z_REG       0x3A
 
-enum msa311_fields {
-	/* Soft_Reset */
-	F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
-	/* Motion_Interrupt */
-	F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
-	/* Data_Interrupt */
-	F_NEW_DATA_INT,
-	/* Tap_Active_Status */
-	F_TAP_SIGN, F_TAP_FIRST_X, F_TAP_FIRST_Y, F_TAP_FIRST_Z, F_ACTV_SIGN,
-	F_ACTV_FIRST_X, F_ACTV_FIRST_Y, F_ACTV_FIRST_Z,
-	/* Orientation_Status */
-	F_ORIENT_Z, F_ORIENT_X_Y,
-	/* Range */
-	F_FS,
-	/* ODR */
-	F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
-	/* Power Mode/Bandwidth */
-	F_PWR_MODE, F_LOW_POWER_BW,
-	/* Swap_Polarity */
-	F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
-	/* Int_Set_0 */
-	F_ORIENT_INT_EN, F_S_TAP_INT_EN, F_D_TAP_INT_EN, F_ACTIVE_INT_EN_Z,
-	F_ACTIVE_INT_EN_Y, F_ACTIVE_INT_EN_X,
-	/* Int_Set_1 */
-	F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
-	/* Int_Map_0 */
-	F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
-	F_INT1_FREEFALL,
-	/* Int_Map_1 */
-	F_INT1_NEW_DATA,
-	/* Int_Config */
-	F_INT1_OD, F_INT1_LVL,
-	/* Int_Latch */
-	F_RESET_INT, F_LATCH_INT,
-	/* Freefall_Hy */
-	F_FREEFALL_MODE, F_FREEFALL_HY,
-	/* Active_Dur */
-	F_ACTIVE_DUR,
-	/* Tap_Dur */
-	F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
-	/* Tap_Th */
-	F_TAP_TH,
-	/* Orient_Hy */
-	F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
-	/* Z_Block */
-	F_Z_BLOCKING,
-	/* End of register map */
-	F_MAX_FIELDS,
-};
-
-static const struct reg_field msa311_reg_fields[] = {
-	/* Soft_Reset */
-	[F_SOFT_RESET_I2C] = REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2),
-	[F_SOFT_RESET_SPI] = REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5),
-	/* Motion_Interrupt */
-	[F_ORIENT_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 6, 6),
-	[F_S_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 5, 5),
-	[F_D_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 4, 4),
-	[F_ACTIVE_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 2, 2),
-	[F_FREEFALL_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 0, 0),
-	/* Data_Interrupt */
-	[F_NEW_DATA_INT] = REG_FIELD(MSA311_DATA_INT_REG, 0, 0),
-	/* Tap_Active_Status */
-	[F_TAP_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 7, 7),
-	[F_TAP_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 6, 6),
-	[F_TAP_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 5, 5),
-	[F_TAP_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 4, 4),
-	[F_ACTV_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 3, 3),
-	[F_ACTV_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 2, 2),
-	[F_ACTV_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 1, 1),
-	[F_ACTV_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 0, 0),
-	/* Orientation_Status */
-	[F_ORIENT_Z] = REG_FIELD(MSA311_ORIENT_STS_REG, 6, 6),
-	[F_ORIENT_X_Y] = REG_FIELD(MSA311_ORIENT_STS_REG, 4, 5),
-	/* Range */
-	[F_FS] = REG_FIELD(MSA311_RANGE_REG, 0, 1),
-	/* ODR */
-	[F_X_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 7, 7),
-	[F_Y_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 6, 6),
-	[F_Z_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 5, 5),
-	[F_ODR] = REG_FIELD(MSA311_ODR_REG, 0, 3),
-	/* Power Mode/Bandwidth */
-	[F_PWR_MODE] = REG_FIELD(MSA311_PWR_MODE_REG, 6, 7),
-	[F_LOW_POWER_BW] = REG_FIELD(MSA311_PWR_MODE_REG, 1, 4),
-	/* Swap_Polarity */
-	[F_X_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 3, 3),
-	[F_Y_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 2, 2),
-	[F_Z_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 1, 1),
-	[F_X_Y_SWAP] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 0, 0),
-	/* Int_Set_0 */
-	[F_ORIENT_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 6, 6),
-	[F_S_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 5, 5),
-	[F_D_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 4, 4),
-	[F_ACTIVE_INT_EN_Z] = REG_FIELD(MSA311_INT_SET_0_REG, 2, 2),
-	[F_ACTIVE_INT_EN_Y] = REG_FIELD(MSA311_INT_SET_0_REG, 1, 1),
-	[F_ACTIVE_INT_EN_X] = REG_FIELD(MSA311_INT_SET_0_REG, 0, 0),
-	/* Int_Set_1 */
-	[F_NEW_DATA_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 4, 4),
-	[F_FREEFALL_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 3, 3),
-	/* Int_Map_0 */
-	[F_INT1_ORIENT] = REG_FIELD(MSA311_INT_MAP_0_REG, 6, 6),
-	[F_INT1_S_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 5, 5),
-	[F_INT1_D_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 4, 4),
-	[F_INT1_ACTIVE] = REG_FIELD(MSA311_INT_MAP_0_REG, 2, 2),
-	[F_INT1_FREEFALL] = REG_FIELD(MSA311_INT_MAP_0_REG, 0, 0),
-	/* Int_Map_1 */
-	[F_INT1_NEW_DATA] = REG_FIELD(MSA311_INT_MAP_1_REG, 0, 0),
-	/* Int_Config */
-	[F_INT1_OD] = REG_FIELD(MSA311_INT_CONFIG_REG, 1, 1),
-	[F_INT1_LVL] = REG_FIELD(MSA311_INT_CONFIG_REG, 0, 0),
-	/* Int_Latch */
-	[F_RESET_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 7, 7),
-	[F_LATCH_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 0, 3),
-	/* Freefall_Hy */
-	[F_FREEFALL_MODE] = REG_FIELD(MSA311_FREEFALL_HY_REG, 2, 2),
-	[F_FREEFALL_HY] = REG_FIELD(MSA311_FREEFALL_HY_REG, 0, 1),
-	/* Active_Dur */
-	[F_ACTIVE_DUR] = REG_FIELD(MSA311_ACTIVE_DUR_REG, 0, 1),
-	/* Tap_Dur */
-	[F_TAP_QUIET] = REG_FIELD(MSA311_TAP_DUR_REG, 7, 7),
-	[F_TAP_SHOCK] = REG_FIELD(MSA311_TAP_DUR_REG, 6, 6),
-	[F_TAP_DUR] = REG_FIELD(MSA311_TAP_DUR_REG, 0, 2),
-	/* Tap_Th */
-	[F_TAP_TH] = REG_FIELD(MSA311_TAP_TH_REG, 0, 4),
-	/* Orient_Hy */
-	[F_ORIENT_HYST] = REG_FIELD(MSA311_ORIENT_HY_REG, 4, 6),
-	[F_ORIENT_BLOCKING] = REG_FIELD(MSA311_ORIENT_HY_REG, 2, 3),
-	[F_ORIENT_MODE] = REG_FIELD(MSA311_ORIENT_HY_REG, 0, 1),
-	/* Z_Block */
-	[F_Z_BLOCKING] = REG_FIELD(MSA311_Z_BLOCK_REG, 0, 3),
-};
+/* Soft_Reset */
+#define MSA311_F_SOFT_RESET_I2C	REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2)
+#define MSA311_F_SOFT_RESET_SPI	REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5)
+/* Motion_Interrupt */
+#define MSA311_F_ORIENT_INT	REG_FIELD(MSA311_MOTION_INT_REG, 6, 6)
+#define MSA311_F_S_TAP_INT	REG_FIELD(MSA311_MOTION_INT_REG, 5, 5)
+#define MSA311_F_D_TAP_INT	REG_FIELD(MSA311_MOTION_INT_REG, 4, 4)
+#define MSA311_F_ACTIVE_INT	REG_FIELD(MSA311_MOTION_INT_REG, 2, 2)
+#define MSA311_F_FREEFALL_INT	REG_FIELD(MSA311_MOTION_INT_REG, 0, 0)
+/* Data_Interrupt */
+#define MSA311_F_NEW_DATA_INT	REG_FIELD(MSA311_DATA_INT_REG, 0, 0)
+/* Tap_Active_Status */
+#define MSA311_F_TAP_SIGN	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 7, 7)
+#define MSA311_F_TAP_FIRST_X	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 6, 6)
+#define MSA311_F_TAP_FIRST_Y	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 5, 5)
+#define MSA311_F_TAP_FIRST_Z	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 4, 4)
+#define MSA311_F_ACTV_SIGN	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 3, 3)
+#define MSA311_F_ACTV_FIRST_X	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 2, 2)
+#define MSA311_F_ACTV_FIRST_Y	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 1, 1)
+#define MSA311_F_ACTV_FIRST_Z	REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 0, 0)
+/* Orientation_Status */
+#define MSA311_F_ORIENT_Z	REG_FIELD(MSA311_ORIENT_STS_REG, 6, 6)
+#define MSA311_F_ORIENT_X_Y	REG_FIELD(MSA311_ORIENT_STS_REG, 4, 5)
+/* Range */
+#define MSA311_F_FS		REG_FIELD(MSA311_RANGE_REG, 0, 1)
+/* ODR */
+#define MSA311_F_X_AXIS_DIS	REG_FIELD(MSA311_ODR_REG, 7, 7)
+#define MSA311_F_Y_AXIS_DIS	REG_FIELD(MSA311_ODR_REG, 6, 6)
+#define MSA311_F_Z_AXIS_DIS	REG_FIELD(MSA311_ODR_REG, 5, 5)
+#define MSA311_F_ODR		REG_FIELD(MSA311_ODR_REG, 0, 3)
+/* Power Mode/Bandwidth */
+#define MSA311_F_PWR_MODE	REG_FIELD(MSA311_PWR_MODE_REG, 6, 7)
+#define MSA311_F_LOW_POWER_BW	REG_FIELD(MSA311_PWR_MODE_REG, 1, 4)
+/* Swap_Polarity */
+#define MSA311_F_X_POLARITY	REG_FIELD(MSA311_SWAP_POLARITY_REG, 3, 3)
+#define MSA311_F_Y_POLARITY	REG_FIELD(MSA311_SWAP_POLARITY_REG, 2, 2)
+#define MSA311_F_Z_POLARITY	REG_FIELD(MSA311_SWAP_POLARITY_REG, 1, 1)
+#define MSA311_F_X_Y_SWAP	REG_FIELD(MSA311_SWAP_POLARITY_REG, 0, 0)
+/* Int_Set_0 */
+#define MSA311_F_ORIENT_INT_EN		REG_FIELD(MSA311_INT_SET_0_REG, 6, 6)
+#define MSA311_F_S_TAP_INT_EN		REG_FIELD(MSA311_INT_SET_0_REG, 5, 5)
+#define MSA311_F_D_TAP_INT_EN		REG_FIELD(MSA311_INT_SET_0_REG, 4, 4)
+#define MSA311_F_ACTIVE_INT_EN_Z	REG_FIELD(MSA311_INT_SET_0_REG, 2, 2)
+#define MSA311_F_ACTIVE_INT_EN_Y	REG_FIELD(MSA311_INT_SET_0_REG, 1, 1)
+#define MSA311_F_ACTIVE_INT_EN_X	REG_FIELD(MSA311_INT_SET_0_REG, 0, 0)
+/* Int_Set_1 */
+#define MSA311_F_NEW_DATA_INT_EN	REG_FIELD(MSA311_INT_SET_1_REG, 4, 4)
+#define MSA311_F_FREEFALL_INT_EN	REG_FIELD(MSA311_INT_SET_1_REG, 3, 3)
+/* Int_Map_0 */
+#define MSA311_F_INT1_ORIENT	REG_FIELD(MSA311_INT_MAP_0_REG, 6, 6)
+#define MSA311_F_INT1_S_TAP	REG_FIELD(MSA311_INT_MAP_0_REG, 5, 5)
+#define MSA311_F_INT1_D_TAP	REG_FIELD(MSA311_INT_MAP_0_REG, 4, 4)
+#define MSA311_F_INT1_ACTIVE	REG_FIELD(MSA311_INT_MAP_0_REG, 2, 2)
+#define MSA311_F_INT1_FREEFALL	REG_FIELD(MSA311_INT_MAP_0_REG, 0, 0)
+/* Int_Map_1 */
+#define MSA311_F_INT1_NEW_DATA	REG_FIELD(MSA311_INT_MAP_1_REG, 0, 0)
+/* Int_Config */
+#define MSA311_F_INT1_OD	REG_FIELD(MSA311_INT_CONFIG_REG, 1, 1)
+#define MSA311_F_INT1_LVL	REG_FIELD(MSA311_INT_CONFIG_REG, 0, 0)
+/* Int_Latch */
+#define MSA311_F_RESET_INT	REG_FIELD(MSA311_INT_LATCH_REG, 7, 7)
+#define MSA311_F_LATCH_INT	REG_FIELD(MSA311_INT_LATCH_REG, 0, 3)
+/* Freefall_Hy */
+#define MSA311_F_FREEFALL_MODE	REG_FIELD(MSA311_FREEFALL_HY_REG, 2, 2)
+#define MSA311_F_FREEFALL_HY	REG_FIELD(MSA311_FREEFALL_HY_REG, 0, 1)
+/* Active_Dur */
+#define MSA311_F_ACTIVE_DUR	REG_FIELD(MSA311_ACTIVE_DUR_REG, 0, 1)
+/* Tap_Dur */
+#define MSA311_F_TAP_QUIET	REG_FIELD(MSA311_TAP_DUR_REG, 7, 7)
+#define MSA311_F_TAP_SHOCK	REG_FIELD(MSA311_TAP_DUR_REG, 6, 6)
+#define MSA311_F_TAP_DUR	REG_FIELD(MSA311_TAP_DUR_REG, 0, 2)
+/* Tap_Th */
+#define MSA311_F_TAP_TH		REG_FIELD(MSA311_TAP_TH_REG, 0, 4)
+/* Orient_Hy */
+#define MSA311_F_ORIENT_HYST		REG_FIELD(MSA311_ORIENT_HY_REG, 4, 6)
+#define MSA311_F_ORIENT_BLOCKING	REG_FIELD(MSA311_ORIENT_HY_REG, 2, 3)
+#define MSA311_F_ORIENT_MODE		REG_FIELD(MSA311_ORIENT_HY_REG, 0, 1)
+/* Z_Block */
+#define MSA311_F_Z_BLOCKING	REG_FIELD(MSA311_Z_BLOCK_REG, 0, 3)
 
 #define MSA311_WHO_AM_I 0x13
 
@@ -334,17 +282,10 @@ static const struct regmap_config msa311_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
-#define MSA311_GENMASK(field) ({                \
-	typeof(&(msa311_reg_fields)[0]) _field; \
-	_field = &msa311_reg_fields[(field)];   \
-	GENMASK(_field->msb, _field->lsb);      \
-})
-
 /**
  * struct msa311_priv - MSA311 internal private state
  * @regs: Underlying I2C bus adapter used to abstract slave
  *        register accesses
- * @fields: Abstract objects for each registers fields access
  * @dev: Device handler associated with appropriate bus client
  * @lock: Protects msa311 device state between setup and data access routines
  *        (power transitions, samp_freq/scale tune, retrieving axes data, etc)
@@ -354,7 +295,6 @@ static const struct regmap_config msa311_regmap_config = {
  */
 struct msa311_priv {
 	struct regmap *regs;
-	struct regmap_field *fields[F_MAX_FIELDS];
 
 	struct device *dev;
 	struct mutex lock;
@@ -410,7 +350,7 @@ static int msa311_get_odr(struct msa311_priv *msa311, unsigned int *odr)
 {
 	int err;
 
-	err = regmap_field_read(msa311->fields[F_ODR], odr);
+	err = regmap_reg_field_read(msa311->regs, MSA311_F_ODR, odr);
 	if (err)
 		return err;
 
@@ -453,7 +393,7 @@ static int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
 	bool good_odr;
 	int err;
 
-	err = regmap_field_read(msa311->fields[F_PWR_MODE], &pwr_mode);
+	err = regmap_reg_field_read(msa311->regs, MSA311_F_PWR_MODE, &pwr_mode);
 	if (err)
 		return err;
 
@@ -472,7 +412,7 @@ static int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
 		return -EINVAL;
 	}
 
-	return regmap_field_write(msa311->fields[F_ODR], odr);
+	return regmap_reg_field_write(msa311->regs, MSA311_F_ODR, odr);
 }
 
 /**
@@ -538,11 +478,11 @@ static int msa311_set_pwr_mode(struct msa311_priv *msa311, unsigned int mode)
 
 	dev_dbg(dev, "transition to %s mode\n", msa311_pwr_modes[mode]);
 
-	err = regmap_field_read(msa311->fields[F_PWR_MODE], &prev_mode);
+	err = regmap_reg_field_read(msa311->regs, MSA311_F_PWR_MODE, &prev_mode);
 	if (err)
 		return err;
 
-	err = regmap_field_write(msa311->fields[F_PWR_MODE], mode);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_PWR_MODE, mode);
 	if (err)
 		return err;
 
@@ -634,7 +574,7 @@ static int msa311_read_scale(struct iio_dev *indio_dev, int *val, int *val2)
 	int err;
 
 	mutex_lock(&msa311->lock);
-	err = regmap_field_read(msa311->fields[F_FS], &fs);
+	err = regmap_reg_field_read(msa311->regs, MSA311_F_FS, &fs);
 	mutex_unlock(&msa311->lock);
 	if (err) {
 		dev_err(dev, "can't get actual scale (%pe)\n", ERR_PTR(err));
@@ -734,7 +674,7 @@ static int msa311_write_scale(struct iio_dev *indio_dev, int val, int val2)
 		/* Do not check msa311_fs_table[fs].integral, it's always 0 */
 		if (val2 == msa311_fs_table[fs].microfract) {
 			mutex_lock(&msa311->lock);
-			err = regmap_field_write(msa311->fields[F_FS], fs);
+			err = regmap_reg_field_write(msa311->regs, MSA311_F_FS, fs);
 			mutex_unlock(&msa311->lock);
 			break;
 		}
@@ -863,7 +803,7 @@ static int msa311_set_new_data_trig_state(struct iio_trigger *trig, bool state)
 	int err;
 
 	mutex_lock(&msa311->lock);
-	err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_NEW_DATA_INT_EN, state);
 	mutex_unlock(&msa311->lock);
 	if (err)
 		dev_err(dev,
@@ -934,8 +874,8 @@ static irqreturn_t msa311_irq_thread(int irq, void *p)
 	 * specification it's cleared automatically after a fixed time.
 	 * So just check that is enabled by driver logic.
 	 */
-	err = regmap_field_read(msa311->fields[F_NEW_DATA_INT_EN],
-				&new_data_int_enabled);
+	err = regmap_reg_field_read(msa311->regs, MSA311_F_NEW_DATA_INT_EN,
+				    &new_data_int_enabled);
 
 	mutex_unlock(&msa311->lock);
 	if (err) {
@@ -995,8 +935,8 @@ static int msa311_soft_reset(struct msa311_priv *msa311)
 	int err;
 
 	err = regmap_write(msa311->regs, MSA311_SOFT_RESET_REG,
-			   MSA311_GENMASK(F_SOFT_RESET_I2C) |
-			   MSA311_GENMASK(F_SOFT_RESET_SPI));
+			   REG_FIELD_GENMASK(MSA311_F_SOFT_RESET_I2C) |
+			   REG_FIELD_GENMASK(MSA311_F_SOFT_RESET_SPI));
 	if (err)
 		return dev_err_probe(dev, err, "can't soft reset all logic\n");
 
@@ -1029,9 +969,9 @@ static int msa311_chip_init(struct msa311_priv *msa311)
 
 	/* Disable all axes by default */
 	err = regmap_clear_bits(msa311->regs, MSA311_ODR_REG,
-				MSA311_GENMASK(F_X_AXIS_DIS) |
-				MSA311_GENMASK(F_Y_AXIS_DIS) |
-				MSA311_GENMASK(F_Z_AXIS_DIS));
+				REG_FIELD_GENMASK(MSA311_F_X_AXIS_DIS) |
+				REG_FIELD_GENMASK(MSA311_F_Y_AXIS_DIS) |
+				REG_FIELD_GENMASK(MSA311_F_Z_AXIS_DIS));
 	if (err)
 		return dev_err_probe(dev, err, "can't enable all axes\n");
 
@@ -1074,30 +1014,30 @@ static int msa311_setup_interrupts(struct msa311_priv *msa311)
 		return dev_err_probe(dev, err,
 				     "can't register newdata trigger\n");
 
-	err = regmap_field_write(msa311->fields[F_INT1_OD],
-				 MSA311_INT1_OD_PUSH_PULL);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_INT1_OD,
+				     MSA311_INT1_OD_PUSH_PULL);
 	if (err)
 		return dev_err_probe(dev, err,
 				     "can't enable push-pull interrupt\n");
 
-	err = regmap_field_write(msa311->fields[F_INT1_LVL],
-				 MSA311_INT1_LVL_HIGH);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_INT1_LVL,
+				     MSA311_INT1_LVL_HIGH);
 	if (err)
 		return dev_err_probe(dev, err,
 				     "can't set active interrupt level\n");
 
-	err = regmap_field_write(msa311->fields[F_LATCH_INT],
-				 MSA311_LATCH_INT_LATCHED);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_LATCH_INT,
+				     MSA311_LATCH_INT_LATCHED);
 	if (err)
 		return dev_err_probe(dev, err,
 				     "can't latch interrupt\n");
 
-	err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_RESET_INT, 1);
 	if (err)
 		return dev_err_probe(dev, err,
 				     "can't reset interrupt\n");
 
-	err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
+	err = regmap_reg_field_write(msa311->regs, MSA311_F_INT1_NEW_DATA, 1);
 	if (err)
 		return dev_err_probe(dev, err,
 				     "can't map new data interrupt\n");
@@ -1105,33 +1045,6 @@ static int msa311_setup_interrupts(struct msa311_priv *msa311)
 	return 0;
 }
 
-static int msa311_regmap_init(struct msa311_priv *msa311)
-{
-	struct regmap_field **fields = msa311->fields;
-	struct device *dev = msa311->dev;
-	struct i2c_client *i2c = to_i2c_client(dev);
-	struct regmap *regmap;
-	int i;
-
-	regmap = devm_regmap_init_i2c(i2c, &msa311_regmap_config);
-	if (IS_ERR(regmap))
-		return dev_err_probe(dev, PTR_ERR(regmap),
-				     "failed to register i2c regmap\n");
-
-	msa311->regs = regmap;
-
-	for (i = 0; i < F_MAX_FIELDS; i++) {
-		fields[i] = devm_regmap_field_alloc(dev,
-						    msa311->regs,
-						    msa311_reg_fields[i]);
-		if (IS_ERR(msa311->fields[i]))
-			return dev_err_probe(dev, PTR_ERR(msa311->fields[i]),
-					     "can't alloc field[%d]\n", i);
-	}
-
-	return 0;
-}
-
 static void msa311_powerdown(void *msa311)
 {
 	msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
@@ -1152,9 +1065,10 @@ static int msa311_probe(struct i2c_client *i2c)
 	msa311->dev = dev;
 	i2c_set_clientdata(i2c, indio_dev);
 
-	err = msa311_regmap_init(msa311);
-	if (err)
-		return err;
+	msa311->regs = devm_regmap_init_i2c(i2c, &msa311_regmap_config);
+	if (IS_ERR(msa311->regs))
+		return dev_err_probe(dev, PTR_ERR(msa311->regs),
+				     "failed to register i2c regmap\n");
 
 	mutex_init(&msa311->lock);
 
-- 
2.53.0

Re: [RFC PATCH v2 1/2] regmap: add regmap_reg_field_* APIs

[Andy Shevchenko]

On Fri, Nov 13, 2026 at 9:05 AM Victor Chernyakin
<chernyakin.victor.j@outlook.com> wrote:
>
> These functions are equivalent to their regmap_field_* counterparts,
> but allow working with reg_field directly, without having to
> allocate and manage regmap_fields.

...

> -#define REG_FIELD(_reg, _lsb, _msb) {          \
> -                               .reg = _reg,    \
> -                               .lsb = _lsb,    \
> -                               .msb = _msb,    \
> -                               }
> +#define REG_FIELD(_reg, _lsb, _msb) ((struct reg_field){ \
> +                               .reg = _reg,             \
> +                               .lsb = _lsb,             \
> +                               .msb = _msb,             \
> +                               })
>
> -#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) {       \
> -                               .reg = _reg,                    \
> -                               .lsb = _lsb,                    \
> -                               .msb = _msb,                    \
> -                               .id_size = _size,               \
> -                               .id_offset = _offset,           \
> -                               }
> +#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) ((struct reg_field){ \
> +                               .reg = _reg,                                \
> +                               .lsb = _lsb,                                \
> +                               .msb = _msb,                                \
> +                               .id_size = _size,                           \
> +                               .id_offset = _offset,                       \
> +                               })

So, you convert them to compound literals. This change may be good on
its own (see other similar changes in the kernel).
The only problem you have is the outer parentheses. They should be
dropped. If you need them, it's a problem on the user's side of this
macros.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 0/2] regmap: add APIs to bypass regmap_field and work with reg_field directly

[Andy Shevchenko]

On Fri, Nov 13, 2026 at 9:01 AM Victor Chernyakin
<chernyakin.victor.j@outlook.com> wrote:
>
> Working with the existing regmap_field_* APIs requires you to define
> a reg_field, then use one of the regmap_field_*_alloc functions to turn
> it into a dynamically allocated regmap_field. But reg_field and
> regmap_field are practically the same struct; all the *_alloc functions do
> is shuffle the data around. So this RFC proposes adding functions that
> cut out the middleman and take reg_field directly. Patch 1 adds two
> of them: regmap_reg_field_read and regmap_reg_field_write.
>
> Patch 2 ports a driver to use these new functions. The result is
> shorter and simpler, since all the code to perform the allocations
> and manage the resulting regmap_fields goes away. In theory,
> there should also be a performance benefit (we're doing fewer allocations
> and have less duplicated state), but I haven't benchmarked it, so take that
> with a grain of salt.
>
> If we like this approach, I can post a v3 that adds regmap_reg_field_*
> analogs for all the regmap_field_* functions, not just the two you see
> here.
>
>
> This is not actually a new idea! Both it and the current *_alloc approach
> were suggested as alternatives right when the regmap_field_* APIs were
> being added [1]. The *_alloc approach was chosen without much
> discussion, but I think (and Patch 2 demonstrates, I hope) that
> this approach is the cleaner one.

It may be good to Cc your attempts to the authors (and active
reviewers) of the mentioned change.

> [1] https://lore.kernel.org/20130604210137.GV31367@sirena.org.uk

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 1/2] regmap: add regmap_reg_field_* APIs

[Victor Chernyakin]

> > -#define REG_FIELD(_reg, _lsb, _msb) {          \
> > -                               .reg = _reg,    \
> > -                               .lsb = _lsb,    \
> > -                               .msb = _msb,    \
> > -                               }
> > +#define REG_FIELD(_reg, _lsb, _msb) ((struct reg_field){ \
> > +                               .reg = _reg,             \
> > +                               .lsb = _lsb,             \
> > +                               .msb = _msb,             \
> > +                               })
> >
> > -#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) {       \
> > -                               .reg = _reg,                    \
> > -                               .lsb = _lsb,                    \
> > -                               .msb = _msb,                    \
> > -                               .id_size = _size,               \
> > -                               .id_offset = _offset,           \
> > -                               }
> > +#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) ((struct reg_field){ \
> > +                               .reg = _reg,                                \
> > +                               .lsb = _lsb,                                \
> > +                               .msb = _msb,                                \
> > +                               .id_size = _size,                           \
> > +                               .id_offset = _offset,                       \
> > +                               })
> 
> So, you convert them to compound literals. This change may be good on
> its own (see other similar changes in the kernel).
> The only problem you have is the outer parentheses. They should be
> dropped. If you need them, it's a problem on the user's side of this
> macros.

In this comment:

https://lore.kernel.org/all/auzCxxkUB9p6WKNC@ashevche-desk.local/

...I got the impresssion that you were *against* dealing with it
on the user's side (in REG_FIELD_GENMASK), and I agreed: if it
can be handled in REG_FIELD, why make users worry about it?
But now it seems you're saying the opposite. Can you clarify
your stance?

Re: [RFC PATCH v2 1/2] regmap: add regmap_reg_field_* APIs

[Andy Shevchenko]

On Fri, Nov 13, 2026 at 10:53 AM Victor Chernyakin
<chernyakin.victor.j@outlook.com> wrote:
>
> > > -#define REG_FIELD(_reg, _lsb, _msb) {          \
> > > -                               .reg = _reg,    \
> > > -                               .lsb = _lsb,    \
> > > -                               .msb = _msb,    \
> > > -                               }
> > > +#define REG_FIELD(_reg, _lsb, _msb) ((struct reg_field){ \
> > > +                               .reg = _reg,             \
> > > +                               .lsb = _lsb,             \
> > > +                               .msb = _msb,             \
> > > +                               })
> > >
> > > -#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) {       \
> > > -                               .reg = _reg,                    \
> > > -                               .lsb = _lsb,                    \
> > > -                               .msb = _msb,                    \
> > > -                               .id_size = _size,               \
> > > -                               .id_offset = _offset,           \
> > > -                               }
> > > +#define REG_FIELD_ID(_reg, _lsb, _msb, _size, _offset) ((struct reg_field){ \
> > > +                               .reg = _reg,                                \
> > > +                               .lsb = _lsb,                                \
> > > +                               .msb = _msb,                                \
> > > +                               .id_size = _size,                           \
> > > +                               .id_offset = _offset,                       \
> > > +                               })
> >
> > So, you convert them to compound literals. This change may be good on
> > its own (see other similar changes in the kernel).
> > The only problem you have is the outer parentheses. They should be
> > dropped. If you need them, it's a problem on the user's side of this
> > macros.
>
> In this comment:
>
> https://lore.kernel.org/all/auzCxxkUB9p6WKNC@ashevche-desk.local/
>
> ...I got the impresssion that you were *against* dealing with it
> on the user's side (in REG_FIELD_GENMASK), and I agreed:

Yes, that's true and I confirm that it's not a business to handle
expressions in REG_FIELD_GENMASK(). We imply that the parameter there
is a regfield.

> if it > can be handled in REG_FIELD, why make users worry about it?

But REG_FIELD is also a macro, it's not their business either.

> But now it seems you're saying the opposite.

Nothing is opposite. The statement is the same, dealing with the
expressions should be done on the caller's side in REG_FIELD_GENMASK()
and REG_FIELD() (as the latter is also a macro).

> Can you clarify
> your stance?

I assume you are trying to have a use of it like in __uart_port_tx().
See also uart_port_tx().

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 2/2] iio: accel: msa311: use regmap_reg_field_* functions

[Jonathan Cameron]

On Fri, 13 Nov 2026 00:55:46 -0600
Victor Chernyakin <chernyakin.victor.j@outlook.com> wrote:

> Simplify the driver by using the new regmap_reg_field_* functions.
> regmap_reg_field_FOO is equivalent to regmap_field_FOO, but takes
> a reg_field directly instead of a regmap_field.

Hi Victor,

In general this seems like a reasonable idea.

One minor suggestion inline on dropping some comments that
add little value and reordering where the defines are to
give better grouping with what they are about.

When sending a small series, just send the whole lot to the list
and all people on the thread.  I had to go find the context
to understand why this was an RFC.

> 
> Tested by compiling.
I'd drop that below the ---

Thanks,

Jonathan

> 
> Signed-off-by: Victor Chernyakin <chernyakin.victor.j@outlook.com>
> ---
>  drivers/iio/accel/msa311.c | 298 +++++++++++++------------------------
>  1 file changed, 106 insertions(+), 192 deletions(-)
> 
> diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
> index caf9b1b6397f..63c795d6bc99 100644
> --- a/drivers/iio/accel/msa311.c
> +++ b/drivers/iio/accel/msa311.c
> @@ -75,137 +75,85 @@
>  #define MSA311_OFFSET_Y_REG       0x39
>  #define MSA311_OFFSET_Z_REG       0x3A
>  

> +/* Soft_Reset */

These comments don't add any value because the register name is over here...

Obviously one set of them in he original code were equally pointless!

I'd be tempted to format this whole thing differently to keep register and
register field definitions together:

#define MSA311_SOFT_RESET_REG     0x00
#define MSA311_F_SOFT_RESET_I2C	REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2)
#define MSA311_F_SOFT_RESET_SPI	REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5)

#define MSA311_PARTID_REG         0x01
#define MSA311_ACC_X_REG          0x02
#define MSA311_ACC_Y_REG          0x04
#define MSA311_ACC_Z_REG          0x06

#define MSA311_MOTION_INT_REG     0x09
#define MSA311_F_ORIENT_INT	REG_FIELD(MSA311_MOTION_INT_REG, 6, 6)
#define MSA311_F_S_TAP_INT	REG_FIELD(MSA311_MOTION_INT_REG, 5, 5)
#define MSA311_F_D_TAP_INT	REG_FIELD(MSA311_MOTION_INT_REG, 4, 4)
#define MSA311_F_ACTIVE_INT	REG_FIELD(MSA311_MOTION_INT_REG, 2, 2)
#define MSA311_F_FREEFALL_INT	REG_FIELD(MSA311_MOTION_INT_REG, 0, 0)

#define MSA311_DATA_INT_REG       0x0A
#define MSA311_F_NEW_DATA_INT	REG_FIELD(MSA311_DATA_INT_REG, 0, 0)

Then even less need for comments.

> +#define MSA311_F_SOFT_RESET_I2C	REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2)
> +#define MSA311_F_SOFT_RESET_SPI	REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5)

Re: [RFC PATCH v2 0/2] regmap: add APIs to bypass regmap_field and work with reg_field directly

[Victor Chernyakin]

> > Working with the existing regmap_field_* APIs requires you to define
> > a reg_field, then use one of the regmap_field_*_alloc functions to turn
> > it into a dynamically allocated regmap_field. But reg_field and
> > regmap_field are practically the same struct; all the *_alloc functions do
> > is shuffle the data around. So this RFC proposes adding functions that
> > cut out the middleman and take reg_field directly. Patch 1 adds two
> > of them: regmap_reg_field_read and regmap_reg_field_write.
> >
> > Patch 2 ports a driver to use these new functions. The result is
> > shorter and simpler, since all the code to perform the allocations
> > and manage the resulting regmap_fields goes away. In theory,
> > there should also be a performance benefit (we're doing fewer allocations
> > and have less duplicated state), but I haven't benchmarked it, so take that
> > with a grain of salt.
> >
> > If we like this approach, I can post a v3 that adds regmap_reg_field_*
> > analogs for all the regmap_field_* functions, not just the two you see
> > here.
> >
> >
> > This is not actually a new idea! Both it and the current *_alloc approach
> > were suggested as alternatives right when the regmap_field_* APIs were
> > being added [1]. The *_alloc approach was chosen without much
> > discussion, but I think (and Patch 2 demonstrates, I hope) that
> > this approach is the cleaner one.
> 
> It may be good to Cc your attempts to the authors (and active
> reviewers) of the mentioned change.
> 
> > [1] https://lore.kernel.org/20130604210137.GV31367@sirena.org.uk

That would make sense, here we go:

Cc: Srinivas Kandagatla <srini@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>