[PATCH] regmap: provide regmap_assign_bits()

[PATCH] regmap: provide regmap_assign_bits()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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>
---
Hi Mark,

I'm sending this patch without any user yet but we have a use-case over in
the GPIO subsystem where this will come in handy soon.

Bartosz

 include/linux/regmap.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e7834d98207f..62099e7a3ed6 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1148,6 +1148,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);
 
 /**
@@ -1554,6 +1563,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)
 {
-- 
2.29.1

Re: [PATCH] regmap: provide regmap_assign_bits()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Mon, Oct 26, 2020 at 04:10:15PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add another bits helper to regmap API: this one sets given bits if value
> is true and clears them if it's false.

What's the use case?

> +static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
> +				     unsigned int bits, bool value)

I don't have a great suggestion but this naming feels prone to confusion
with _update_bits().  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: provide regmap_assign_bits()

[Bartosz Golaszewski]

On Thu, Oct 29, 2020 at 4:18 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Oct 26, 2020 at 04:10:15PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Add another bits helper to regmap API: this one sets given bits if value
> > is true and clears them if it's false.
>
> What's the use case?
>

Basically what the function does: set bits if a condition is true,
clear them if false. I think this is a common enough use-case to
warrant a helper.

> > +static inline int regmap_assign_bits(struct regmap *map, unsigned int reg,
> > +                                  unsigned int bits, bool value)
>
> I don't have a great suggestion but this naming feels prone to confusion
> with _update_bits().

This is already used in bitops - we have set_bit(), clear_bit() and
assign_bit(). People are used to it and I thought I'd stay consistent
with this naming.

Bartosz

Re: [PATCH] regmap: provide regmap_assign_bits()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Thu, Oct 29, 2020 at 04:44:16PM +0100, Bartosz Golaszewski wrote:
> On Thu, Oct 29, 2020 at 4:18 PM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Oct 26, 2020 at 04:10:15PM +0100, Bartosz Golaszewski wrote:

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

> > What's the use case?

> Basically what the function does: set bits if a condition is true,
> clear them if false. I think this is a common enough use-case to
> warrant a helper.

I can tell what the function does, I can't tell why you'd want it and
simply stating that it's common isn't helping me here :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: provide regmap_assign_bits()

[Bartosz Golaszewski]

On Thu, Oct 29, 2020 at 4:48 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 29, 2020 at 04:44:16PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Oct 29, 2020 at 4:18 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Oct 26, 2020 at 04:10:15PM +0100, Bartosz Golaszewski wrote:
>
> > > > Add another bits helper to regmap API: this one sets given bits if value
> > > > is true and clears them if it's false.
>
> > > What's the use case?
>
> > Basically what the function does: set bits if a condition is true,
> > clear them if false. I think this is a common enough use-case to
> > warrant a helper.
>
> I can tell what the function does, I can't tell why you'd want it and
> simply stating that it's common isn't helping me here :(

Got it, I'll resend this together with the patches that need it then.

Bartosz

[PATCH] regmap: provide regmap_assign_bits()

[Tomi Valkeinen]

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>

Re: [PATCH] regmap: provide regmap_assign_bits()

[Mark Brown]

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