linux-kernel - [PATCH] regmap: provide regmap_assign

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20241108-assign-bits-v1-1-382790562d99@ideasonboard.com>
Date: Fri, 08 Nov 2024 16:07:37 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Mark Brown <broonie@...nel.org>
Cc: linux-kernel@...r.kernel.org, 
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
 Bartosz Golaszewski <brgl@...ev.pl>, 
 Andy Shevchenko <andy.shevchenko@...il.com>, 
 Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, 
 Bartosz Golaszewski <brgl@...ev.pl>
Subject: [PATCH] regmap: provide regmap_assign_bits()

From: Bartosz Golaszewski <brgl@...ev.pl>

Add another bits helper to regmap API: this one sets given bits if value
is true and clears them if it's false.

Suggested-by: Andy Shevchenko <andy.shevchenko@...il.com>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
---
The patch to add regmap_assign_bits() was sent some years back, but not
applied:

https://lore.kernel.org/all/20201104193051.32236-2-brgl@bgdev.pl/

I still feel it would be useful.So, here it's again, this time with a
semantic patch (for discussion, not to be applied).

Running this semantic patch:

@@
expression RMAP, REG, MASK, VAL;
@@

(
-regmap_update_bits(RMAP, REG, MASK, VAL ? MASK : 0)
+regmap_assign_bits(RMAP, REG, MASK, VAL)
|
-regmap_update_bits(RMAP, REG, MASK, VAL ? 0: MASK)
+regmap_assign_bits(RMAP, REG, MASK, !VAL)
)

with:

spatch --no-show-diff --in-place --sp-file assign-bits.cocci --dir drivers/

gives:

$ git status|grep modified|wc -l
130

With some cursory look, many of them are of the pattern:

regmap_update_bits(data->regmap, SI514_REG_CONTROL,
		   SI514_CONTROL_OE,
		   enable ? SI514_CONTROL_OE : 0);

which are then turned into:

regmap_assign_bits(data->regmap, SI514_REG_CONTROL,
		   SI514_CONTROL_OE, enable);

I, at least, find the regmap_assign_bits() more readable and
understandable. Here the mask name is relatively short, but I often name
mask macros as something like "{DEV}_{REG}_{FIELD}", which results in
specific but rather long names. And the "enable" variable is often in
some state struct. So for the sake of discussion, let's lenghten the
names a bit:

regmap_update_bits(data->regmap, SI514_SOME_CONTROL_REG,
		   SI514_SOME_CONTROL_REG_ENABLE_FOOBARING,
		   priv->enable_foobaring ?
			SI514_SOME_CONTROL_REG_ENABLE_FOOBARING : 0);

would turn into:

regmap_assign_bits(data->regmap, SI514_SOME_CONTROL_REG,
		   SI514_SOME_CONTROL_REG_ENABLE_FOOBARING,
		   priv->enable_foobaring);

The above could also be done with:

if (enable)
	regmap_set_bits(data->regmap, SI514_REG_CONTROL,
			SI514_CONTROL_OE);
else
	regmap_clear_bits(data->regmap, SI514_REG_CONTROL,
			  SI514_CONTROL_OE);

This pattern is also used, but I only found 7 files with a quick
semantic patch. And I like the regmap_assign_bits() better: the
operation is an assignment to certain bits, so it's a single operation,
not an if/else situation.

I was also thinking about naming the function as "regmap_toggle_bits",
but maybe "toggle" is similar to "invert", which is not really the case
here.

Another option would be to make it regmap_assign_bit(), and allow only a
single-bit mask. But I'm not sure if that really helps any, although I
would guess that majority of the cases where regmap_assign_bits() can be
used deal with a single bit.
---
 include/linux/regmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f9ccad32fc5c..239659919203 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1328,6 +1328,15 @@ static inline int regmap_clear_bits(struct regmap *map,
 	return regmap_update_bits_base(map, reg, bits, 0, NULL, false, false);
 }
 
+static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
+				     unsigned int bits, bool value)
+{
+	if (value)
+		return regmap_set_bits(map, reg, bits);
+	else
+		return regmap_clear_bits(map, reg, bits);
+}
+
 int regmap_test_bits(struct regmap *map, unsigned int reg, unsigned int bits);
 
 /**
@@ -1796,6 +1805,13 @@ static inline int regmap_clear_bits(struct regmap *map,
 	return -EINVAL;
 }
 
+static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
+				     unsigned int bits, bool value)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
 static inline int regmap_test_bits(struct regmap *map,
 				   unsigned int reg, unsigned int bits)
 {

---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241108-assign-bits-82ecc986a7d5

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>