From: Bartosz Golaszewski <brgl@bgdev.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@gmail.com>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.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@ideasonboard.com>

On Fri, 08 Nov 2024 16:07:37 +0200, Tomi Valkeinen wrote:
> Add another bits helper to regmap API: this one sets given bits if value
> is true and clears them if it's false.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regmap: provide regmap_assign_bits()
      commit: d1f4390dd28ba110f232615dc4610ac1bb2f39f2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark