diff mbox

[1/7,RFC] regmap: add regmap_raw_update_bits() and merge all regmap_update_bits_xxx()

Message ID 87ziv9s2yf.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto Feb. 10, 2016, 2:44 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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.
	regmap_update_bits()
	regmap_update_bits_async()
	regmap_update_bits_check()
	regmap_update_bits_check_async()

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/base/regmap/regmap.c | 113 ++++++-------------------------------------
 include/linux/regmap.h       |  54 ++++++---------------
 2 files changed, 31 insertions(+), 136 deletions(-)

Comments

Mark Brown Feb. 10, 2016, 9:38 a.m. UTC | #1 
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.
Kuninori Morimoto Feb. 15, 2016, 4:48 a.m. UTC | #2 
Hi Mark

OK, I will send v2 patch soon

> 
> > 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.
> [2 signature.asc <application/pgp-signature (7bit)>]
> No public key for 24D68B725D5487D0 created at 2016-02-10T18:38:15+0900 using RSA
diff mbox

Patch

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index ee54e84..c91e67b 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2648,76 +2648,34 @@  static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 }
 
 /**
- * regmap_update_bits: Perform a read/modify/write cycle on the register map
- *
- * @map: Register map to update
- * @reg: Register to update
- * @mask: Bitmask to change
- * @val: New value for bitmask
- *
- * Returns zero for success, a negative number on error.
- */
-int regmap_update_bits(struct regmap *map, unsigned int reg,
-		       unsigned int mask, unsigned int val)
-{
-	int ret;
-
-	map->lock(map->lock_arg);
-	ret = _regmap_update_bits(map, reg, mask, val, NULL, false);
-	map->unlock(map->lock_arg);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(regmap_update_bits);
-
-/**
- * regmap_write_bits: Perform a read/modify/write cycle on the register map
- *
- * @map: Register map to update
- * @reg: Register to update
- * @mask: Bitmask to change
- * @val: New value for bitmask
- *
- * Returns zero for success, a negative number on error.
- */
-int regmap_write_bits(struct regmap *map, unsigned int reg,
-		      unsigned int mask, unsigned int val)
-{
-	int ret;
-
-	map->lock(map->lock_arg);
-	ret = _regmap_update_bits(map, reg, mask, val, NULL, true);
-	map->unlock(map->lock_arg);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(regmap_write_bits);
-
-/**
- * regmap_update_bits_async: Perform a read/modify/write cycle on the register
- *                           map asynchronously
+ * regmap_raw_update_bits: Perform a read/modify/write cycle on the register map
  *
  * @map: Register map to update
  * @reg: Register to update
  * @mask: Bitmask to change
  * @val: New value for bitmask
+ * @change: Boolean indicating if a write was done
+ * @async: Boolean indicating asynchronously
+ * @force: Boolean indicating use force update
  *
+ * if async was true,
  * With most buses the read must be done synchronously so this is most
  * useful for devices with a cache which do not need to interact with
  * the hardware to determine the current register value.
  *
  * Returns zero for success, a negative number on error.
  */
-int regmap_update_bits_async(struct regmap *map, unsigned int reg,
-			     unsigned int mask, unsigned int val)
+int regmap_raw_update_bits(struct regmap *map, unsigned int reg,
+			   unsigned int mask, unsigned int val,
+			   bool *change, bool async, bool force)
 {
 	int ret;
 
 	map->lock(map->lock_arg);
 
-	map->async = true;
+	map->async = async ? true : false;
 
-	ret = _regmap_update_bits(map, reg, mask, val, NULL, false);
+	ret = _regmap_update_bits(map, reg, mask, val, change, force);
 
 	map->async = false;
 
@@ -2725,69 +2683,30 @@  int regmap_update_bits_async(struct regmap *map, unsigned int reg,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(regmap_update_bits_async);
-
-/**
- * regmap_update_bits_check: Perform a read/modify/write cycle on the
- *                           register map and report if updated
- *
- * @map: Register map to update
- * @reg: Register to update
- * @mask: Bitmask to change
- * @val: New value for bitmask
- * @change: Boolean indicating if a write was done
- *
- * Returns zero for success, a negative number on error.
- */
-int regmap_update_bits_check(struct regmap *map, unsigned int reg,
-			     unsigned int mask, unsigned int val,
-			     bool *change)
-{
-	int ret;
-
-	map->lock(map->lock_arg);
-	ret = _regmap_update_bits(map, reg, mask, val, change, false);
-	map->unlock(map->lock_arg);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(regmap_update_bits_check);
+EXPORT_SYMBOL_GPL(regmap_raw_update_bits);
 
 /**
- * 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
  *
  * @map: Register map to update
  * @reg: Register to update
  * @mask: Bitmask to change
  * @val: New value for bitmask
- * @change: Boolean indicating if a write was done
- *
- * With most buses the read must be done synchronously so this is most
- * useful for devices with a cache which do not need to interact with
- * the hardware to determine the current register value.
  *
  * Returns zero for success, a negative number on error.
  */
-int regmap_update_bits_check_async(struct regmap *map, unsigned int reg,
-				   unsigned int mask, unsigned int val,
-				   bool *change)
+int regmap_write_bits(struct regmap *map, unsigned int reg,
+		      unsigned int mask, unsigned int val)
 {
 	int ret;
 
 	map->lock(map->lock_arg);
-
-	map->async = true;
-
-	ret = _regmap_update_bits(map, reg, mask, val, change, false);
-
-	map->async = false;
-
+	ret = _regmap_update_bits(map, reg, mask, val, NULL, true);
 	map->unlock(map->lock_arg);
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(regmap_update_bits_check_async);
+EXPORT_SYMBOL_GPL(regmap_write_bits);
 
 void regmap_async_complete_cb(struct regmap_async *async, int ret)
 {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1839434..0b8b76a 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -65,6 +65,15 @@  struct reg_sequence {
 	unsigned int delay_us;
 };
 
+#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)
+
 #ifdef CONFIG_REGMAP
 
 enum regmap_endian {
@@ -691,18 +700,11 @@  int regmap_raw_read(struct regmap *map, unsigned int reg,
 		    void *val, size_t val_len);
 int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		     size_t val_count);
-int regmap_update_bits(struct regmap *map, unsigned int reg,
-		       unsigned int mask, unsigned int val);
+int regmap_raw_update_bits(struct regmap *map, unsigned int reg,
+			   unsigned int mask, unsigned int val,
+			   bool *change, bool async, bool force);
 int regmap_write_bits(struct regmap *map, unsigned int reg,
 		       unsigned int mask, unsigned int val);
-int regmap_update_bits_async(struct regmap *map, unsigned int reg,
-			     unsigned int mask, unsigned int val);
-int regmap_update_bits_check(struct regmap *map, unsigned int reg,
-			     unsigned int mask, unsigned int val,
-			     bool *change);
-int regmap_update_bits_check_async(struct regmap *map, unsigned int reg,
-				   unsigned int mask, unsigned int val,
-				   bool *change);
 int regmap_get_val_bytes(struct regmap *map);
 int regmap_get_max_register(struct regmap *map);
 int regmap_get_reg_stride(struct regmap *map);
@@ -937,8 +939,9 @@  static inline int regmap_bulk_read(struct regmap *map, unsigned int reg,
 	return -EINVAL;
 }
 
-static inline int regmap_update_bits(struct regmap *map, unsigned int reg,
-				     unsigned int mask, unsigned int val)
+static inline int regmap_raw_update_bits(struct regmap *map, unsigned int reg,
+					 unsigned int mask, unsigned int val,
+					 bool *change, bool async, bool force)
 {
 	WARN_ONCE(1, "regmap API is disabled");
 	return -EINVAL;
@@ -951,33 +954,6 @@  static inline int regmap_write_bits(struct regmap *map, unsigned int reg,
 	return -EINVAL;
 }
 
-static inline int regmap_update_bits_async(struct regmap *map,
-					   unsigned int reg,
-					   unsigned int mask, unsigned int val)
-{
-	WARN_ONCE(1, "regmap API is disabled");
-	return -EINVAL;
-}
-
-static inline int regmap_update_bits_check(struct regmap *map,
-					   unsigned int reg,
-					   unsigned int mask, unsigned int val,
-					   bool *change)
-{
-	WARN_ONCE(1, "regmap API is disabled");
-	return -EINVAL;
-}
-
-static inline int regmap_update_bits_check_async(struct regmap *map,
-						 unsigned int reg,
-						 unsigned int mask,
-						 unsigned int val,
-						 bool *change)
-{
-	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");