[RFC][PATCH] regmap: Add reg_sequence for use with multi_reg_write / register_patch

[RFC][PATCH] regmap: Add reg_sequence for use with multi_reg_write / register_patch

[Nariman Poushin]

Support write sequences / patches with specified delays (in microseconds)
after some (or all) writes. Logically separate reg_default from the new
reg_sequence structure (which has an additional delay_us member) as the
reg_default tables can run in to the thousands (for modern devices) and the
additional memory usage due to an added unsigned int could be large
(especially on 64-bit systems).

We treat a delay in a sequence the same as we treat a page change as they
are logically similar (you can coalesce all writes within the same page or
if none of them require a delay after being written).

Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
---

 I am not sure how best to update all the users of this patch (should
 it be accepted), should I:
	- Squash all the updates in to this patch (I suppose the benefit
	  there is that we don't break the kernel build from one patch
	  to the other)
	- Provide separate patch(es) for the users

 Thanks
 Nariman

 drivers/base/regmap/internal.h |  2 +-
 drivers/base/regmap/regmap.c   | 56 ++++++++++++++++++++++++++++++------------
 include/linux/regmap.h         | 21 +++++++++++++---
 3 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index b2b2849..873ddf9 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -136,7 +136,7 @@ struct regmap {
 	/* if set, the HW registers are known to match map->reg_defaults */
 	bool no_sync_defaults;
 
-	struct reg_default *patch;
+	struct reg_sequence *patch;
 	int patch_regs;
 
 	/* if set, converts bulk rw to single rw */
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6273ff0..75773fd 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
+#include <linux/delay.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -47,6 +48,17 @@ static int _regmap_bus_reg_write(void *context, unsigned int reg,
 static int _regmap_bus_raw_write(void *context, unsigned int reg,
 				 unsigned int val);
 
+void regmap_sequence_delay(unsigned int delay_us)
+{
+	/* For small delays it isn't worth setting up the hrtimers
+	 * so fall back on udelay
+	 */
+	if (delay_us < 10)
+		udelay(delay_us);
+	else
+		usleep_range(delay_us, delay_us * 2);
+}
+
 bool regmap_reg_in_ranges(unsigned int reg,
 			  const struct regmap_range *ranges,
 			  unsigned int nranges)
@@ -1744,7 +1756,7 @@ EXPORT_SYMBOL_GPL(regmap_bulk_write);
  * relative. The page register has been written if that was neccessary.
  */
 static int _regmap_raw_multi_reg_write(struct regmap *map,
-				       const struct reg_default *regs,
+				       const struct reg_sequence *regs,
 				       size_t num_regs)
 {
 	int ret;
@@ -1801,17 +1813,18 @@ static unsigned int _regmap_register_page(struct regmap *map,
 }
 
 static int _regmap_range_multi_paged_reg_write(struct regmap *map,
-					       struct reg_default *regs,
+					       struct reg_sequence *regs,
 					       size_t num_regs)
 {
 	int ret;
 	int i, n;
-	struct reg_default *base;
+	struct reg_sequence *base;
 	unsigned int this_page = 0;
 	/*
 	 * the set of registers are not neccessarily in order, but
 	 * since the order of write must be preserved this algorithm
-	 * chops the set each time the page changes
+	 * chops the set each time the page changes. This also applies
+	 * if there is a delay required at any point in the sequence.
 	 */
 	base = regs;
 	for (i = 0, n = 0; i < num_regs; i++, n++) {
@@ -1819,17 +1832,21 @@ static int _regmap_range_multi_paged_reg_write(struct regmap *map,
 		struct regmap_range_node *range;
 
 		range = _regmap_range_lookup(map, reg);
-		if (range) {
+		if (range || regs[i].delay_us) {
 			unsigned int win_page = _regmap_register_page(map, reg,
 								      range);
 
 			if (i == 0)
 				this_page = win_page;
-			if (win_page != this_page) {
+			if (win_page != this_page || regs[i].delay_us) {
 				this_page = win_page;
 				ret = _regmap_raw_multi_reg_write(map, base, n);
 				if (ret != 0)
 					return ret;
+
+				if (regs[i].delay_us)
+					regmap_sequence_delay(regs[i].delay_us);
+
 				base += n;
 				n = 0;
 			}
@@ -1844,7 +1861,7 @@ static int _regmap_range_multi_paged_reg_write(struct regmap *map,
 }
 
 static int _regmap_multi_reg_write(struct regmap *map,
-				   const struct reg_default *regs,
+				   const struct reg_sequence *regs,
 				   size_t num_regs)
 {
 	int i;
@@ -1855,6 +1872,9 @@ static int _regmap_multi_reg_write(struct regmap *map,
 			ret = _regmap_write(map, regs[i].reg, regs[i].def);
 			if (ret != 0)
 				return ret;
+
+			if (regs[i].delay_us)
+				regmap_sequence_delay(regs[i].delay_us);
 		}
 		return 0;
 	}
@@ -1894,11 +1914,15 @@ static int _regmap_multi_reg_write(struct regmap *map,
 	for (i = 0; i < num_regs; i++) {
 		unsigned int reg = regs[i].reg;
 		struct regmap_range_node *range;
+
+		/* Coalesce all the writes between a page break or a delay
+		 * in a sequence
+		 */
 		range = _regmap_range_lookup(map, reg);
-		if (range) {
-			size_t len = sizeof(struct reg_default)*num_regs;
-			struct reg_default *base = kmemdup(regs, len,
-							   GFP_KERNEL);
+		if (range || regs[i].delay_us) {
+			size_t len = sizeof(struct reg_sequence)*num_regs;
+			struct reg_sequence *base = kmemdup(regs, len,
+							    GFP_KERNEL);
 			if (!base)
 				return -ENOMEM;
 			ret = _regmap_range_multi_paged_reg_write(map, base,
@@ -1930,7 +1954,7 @@ static int _regmap_multi_reg_write(struct regmap *map,
  * A value of zero will be returned on success, a negative errno will be
  * returned in error cases.
  */
-int regmap_multi_reg_write(struct regmap *map, const struct reg_default *regs,
+int regmap_multi_reg_write(struct regmap *map, const struct reg_sequence *regs,
 			   int num_regs)
 {
 	int ret;
@@ -1963,7 +1987,7 @@ EXPORT_SYMBOL_GPL(regmap_multi_reg_write);
  * be returned in error cases.
  */
 int regmap_multi_reg_write_bypassed(struct regmap *map,
-				    const struct reg_default *regs,
+				    const struct reg_sequence *regs,
 				    int num_regs)
 {
 	int ret;
@@ -2553,10 +2577,10 @@ EXPORT_SYMBOL_GPL(regmap_async_complete);
  * The caller must ensure that this function cannot be called
  * concurrently with either itself or regcache_sync().
  */
-int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
+int regmap_register_patch(struct regmap *map, const struct reg_sequence *regs,
 			  int num_regs)
 {
-	struct reg_default *p;
+	struct reg_sequence *p;
 	int ret;
 	bool bypass;
 
@@ -2565,7 +2589,7 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 		return 0;
 
 	p = krealloc(map->patch,
-		     sizeof(struct reg_default) * (map->patch_regs + num_regs),
+		     sizeof(struct reg_sequence) * (map->patch_regs + num_regs),
 		     GFP_KERNEL);
 	if (p) {
 		memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs));
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d..c3bf366 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -50,6 +50,21 @@ struct reg_default {
 	unsigned int def;
 };
 
+/**
+ * Register / Value pairs for sequences of writes, incorporating an optional
+ * delay in microseconds.
+ *
+ * @reg: Register address.
+ * @def: Register default value.
+ * @delay_us: Delay in microseconds
+ */
+
+struct reg_sequence {
+	unsigned reg;
+	unsigned def;
+	unsigned delay_us;
+};
+
 #ifdef CONFIG_REGMAP
 
 enum regmap_endian {
@@ -410,10 +425,10 @@ int regmap_raw_write(struct regmap *map, unsigned int reg,
 		     const void *val, size_t val_len);
 int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 			size_t val_count);
-int regmap_multi_reg_write(struct regmap *map, const struct reg_default *regs,
+int regmap_multi_reg_write(struct regmap *map, const struct reg_sequence *regs,
 			int num_regs);
 int regmap_multi_reg_write_bypassed(struct regmap *map,
-				    const struct reg_default *regs,
+				    const struct reg_sequence *regs,
 				    int num_regs);
 int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 			   const void *val, size_t val_len);
@@ -448,7 +463,7 @@ void regcache_mark_dirty(struct regmap *map);
 bool regmap_check_range_table(struct regmap *map, unsigned int reg,
 			      const struct regmap_access_table *table);
 
-int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
+int regmap_register_patch(struct regmap *map, const struct reg_sequence *regs,
 			  int num_regs);
 int regmap_parse_val(struct regmap *map, const void *buf,
 				unsigned int *val);
-- 
2.1.4

Re: [RFC][PATCH] regmap: Add reg_sequence for use with multi_reg_write / register_patch

[Mark Brown]

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

On Mon, Jun 01, 2015 at 10:20:12AM +0100, Nariman Poushin wrote:

>  it be accepted), should I:
> 	- Squash all the updates in to this patch (I suppose the benefit
> 	  there is that we don't break the kernel build from one patch
> 	  to the other)

You need to squash the changes in since they break bisection if handled
separately.  It would be better to do this by having a separate patch to
add the newly named structure rather than adding the new functionality
at the same time.  That makes the patch more mechanical and easier to
review.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC][PATCH] regmap: Add reg_sequence for use with multi_reg_write / register_patch

[Nariman Poushin]

On Tue, Jun 02, 2015 at 07:15:13PM +0100, Mark Brown wrote:
> On Mon, Jun 01, 2015 at 10:20:12AM +0100, Nariman Poushin wrote:
> 
> >  it be accepted), should I:
> > 	- Squash all the updates in to this patch (I suppose the benefit
> > 	  there is that we don't break the kernel build from one patch
> > 	  to the other)
> 
> You need to squash the changes in since they break bisection if handled
> separately.  It would be better to do this by having a separate patch to
> add the newly named structure rather than adding the new functionality
> at the same time.  That makes the patch more mechanical and easier to
> review.

Ok, I have a patch set ready (as you described) but I am having some
problem deciding on the correct distribution, the squashed patch that
touches a whole bunch of subsystems ends up with a monstrous
get_maintainer.pl output, so even going through and checking
MAINTAINERS I have ended up with a large list (26 individuals and lists).

Is this ok? I am not sure if it is going to get bounced by mail servers
as spam or whether it's bad etiquette to do this, but as you say
we don't want to break the bisection.

Thanks
Nariman

Re: [RFC][PATCH] regmap: Add reg_sequence for use with multi_reg_write / register_patch

[Mark Brown]

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

On Thu, Jun 04, 2015 at 03:21:19PM +0100, Nariman Poushin wrote:
> On Tue, Jun 02, 2015 at 07:15:13PM +0100, Mark Brown wrote:

> > You need to squash the changes in since they break bisection if handled
> > separately.  It would be better to do this by having a separate patch to
> > add the newly named structure rather than adding the new functionality
> > at the same time.  That makes the patch more mechanical and easier to
> > review.

> Ok, I have a patch set ready (as you described) but I am having some
> problem deciding on the correct distribution, the squashed patch that
> touches a whole bunch of subsystems ends up with a monstrous
> get_maintainer.pl output, so even going through and checking
> MAINTAINERS I have ended up with a large list (26 individuals and lists).

> Is this ok? I am not sure if it is going to get bounced by mail servers
> as spam or whether it's bad etiquette to do this, but as you say
> we don't want to break the bisection.

Probably post individual patches for people to look at but make it clear
that they need to go in as a single API change patch rather than be
applied in the commit message.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]