Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330860 > unrolled thread

[PATCH 0/6][RFC] cleanup regmap write functions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6][RFC] cleanup regmap write functions Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> - 2016-02-10 03:50 +0100
    Re: [PATCH 1/7][RFC] regmap: add regmap_raw_update_bits() and merge  all regmap_update_bits_xxx() Mark Brown <broonie@kernel.org> - 2016-02-10 10:40 +0100

#1330860 — [PATCH 0/6][RFC] cleanup regmap write functions

Hi Mark

Current regmap has many similar functions
        regmap_update_bits()
        regmap_update_bits_async()
        regmap_update_bits_check()
        regmap_update_bits_check_async()
But difference is very few.
And I would like to have _force_ feature on it.
So, these patches add new regmap_raw_update_bits()
which has _check, _async, _force option.
Above functions are now defined as macro.
4), 5) adds _force_ feature. We can easily add _check, _async.

I used [RFC], because regmap has many effect.

BTW, I noticed #if - #else - #endif on ${LINUX}/include/linux/regmap.h are strange.
Many functions/struct/macro are defined under #ifdef CONFIG_REGMAP,
but few are defined under #else. It can be trouble ?
Do we really need this #if ?

Kuninori Morimoto (7):
      1) regmap: add regmap_raw_update_bits() and merge all regmap_update_bits_xxx()
      2) regmap: regmap_field_xxx() uses regmap_raw_update_bits()
      3) regmap: regmap_fields_xxx() uses regmap_raw_update_bits()
      4) regmap: add regmap_field_force_xxx() functions
      5) regmap: add regmap_fields_force_xxx() functions
      6) regmpa: remove regmap_write_bits()
      7) ASoC: rsnd: rsnd_write() / rsnd_bset() uses regmap _force_ function

 drivers/base/regmap/regmap.c | 191 +++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------
 include/linux/regmap.h       | 102 ++++++++++++++++++++++++++++++----------------------------------------
 sound/soc/sh/rcar/gen.c      |  21 ++-------------
 sound/soc/sh/rcar/rsnd.h     |   2 --
 4 files changed, 88 insertions(+), 228 deletions(-)


Best regards
---
Kuninori Morimoto

[toc] | [next] | [standalone]

#1330970 — Re: [PATCH 1/7][RFC] regmap: add regmap_raw_update_bits() and merge all regmap_update_bits_xxx()

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 10, 2016 at 02:44:11AM +0000, Kuninori Morimoto wrote:

> Current regmap has many similar update functions, but the difference is
> very few. This patch adds new regmap_raw_update_bits() and merge all
> update functions into it by macro.

This is a bit hard to review due to the way the diff comes out, it's not
entirely clear what the code comes out looking like and I'm a bit
nervous about what the gains might be since macro conversions often
obscure things (this is making the macros undocumented for example).
Creating the new function and then using it in a separate patch would be
better.

> +int regmap_raw_update_bits(struct regmap *map, unsigned int reg,
> +			   unsigned int mask, unsigned int val,
> +			   bool *change, bool async, bool force)

_raw specifically means something that works with the direct physical
data format within regmap, a different name would be better.

> -	map->async = true;
> +	map->async = async ? true : false;

This is abuse of the ternery operator where a normal if statement would
do, and it's also rewriting a bool into a bool so a simple assignment is
enough.

>  /**
> - * regmap_update_bits_check_async: Perform a read/modify/write cycle on the
> - *                                 register map asynchronously and report if
> - *                                 updated
> + * regmap_write_bits: Perform a read/modify/write cycle on the register map

This looks like it renames update_bits() to write_bits()...

> +#define regmap_update_bits(map, reg, mask, val) \
> +	regmap_raw_update_bits(map, reg, mask, val, NULL, false, false)
> +#define regmap_update_bits_async(map, reg, mask, val)\
> +	regmap_raw_update_bits(map, reg, mask, val, NULL, true, false)
> +#define regmap_update_bits_check(map, reg, mask, val, change)\
> +	regmap_raw_update_bits(map, reg, mask, val, change, false, false)
> +#define regmap_update_bits_check_async(map, reg, mask, val, change)\
> +	regmap_raw_update_bits(map, reg, mask, val, change, true, false)
> +

...but we don't seem to use it?  The new name is also a bit confusing.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web