All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH] regmap: Add support for sequences of writes with specified delays
@ 2015-05-26 12:39 Nariman Poushin
  2015-05-26 15:21 ` Mark Brown
  0 siblings, 1 reply; 4+ messages in thread
From: Nariman Poushin @ 2015-05-26 12:39 UTC (permalink / raw
  To: broonie; +Cc: patches, gregkh, linux-kernel

It is common for devices to require delays after a register write
(clock enables/disables, fll inputs etc, power etc.) as a part of
a larger write sequence from the host side. This interface allows
the called to specify a delay in uS to be applied after each write
in the sequence supplied. This also maintains atomicity for the
sequence, which avoids callers needing this type of behaviour from
having to implement their own locking schemes to achieve this when
also requiring delays within a write sequence

Change-Id:Ie9e77aa48f258b353ffa7406d02e19c28d5f2a44
Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regmap.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h       | 20 +++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 58cfb32..ffecc1c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
+#include <linux/delay.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/regmap.h>
@@ -1123,6 +1124,30 @@ static int _regmap_bus_raw_write(void *context, unsigned int reg,
 				 map->format.val_bytes, false);
 }
 
+static int _regmap_sequence_write(struct regmap *map,
+				  const struct reg_sequence *regs,
+				  int num_regs)
+{
+	int i, ret;
+
+	for (i = 0; i < num_regs; i++) {
+		if (regs[i].reg % map->reg_stride)
+			return -EINVAL;
+		ret = _regmap_write(map, regs[i].reg, regs[i].def);
+		if (ret != 0) {
+			dev_err(map->dev, "Failed to write %x = %x: %d\n",
+				regs[i].reg, regs[i].def, ret);
+			return ret;
+		}
+
+		if (regs[i].delay_us)
+			udelay(regs[i].delay_us);
+	}
+
+	return 0;
+
+}
+
 static inline void *_regmap_map_get_context(struct regmap *map)
 {
 	return (map->bus) ? map : map->bus_context;
@@ -1564,6 +1589,40 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 }
 EXPORT_SYMBOL_GPL(regmap_bulk_read);
 
+/* regmap_sequence_write(): Write multiple registers to the device
+* with an optional delay in microseconds after each write.
+*
+* @map: Register map to write to
+* @regs: Array of structures containing register,value, delay to be written
+* @num_regs: Number of registers to write
+*
+* This function is intended to be used for writing a timed sequence
+* of writes to a device for situations where particular register in
+* an overall sequence requires a post-write delay (common examples of
+* this are clock enables, regulator enables) whilst still maintaining
+* atomic access to the register map (to avoid writes from other threads
+* being interleaved with the current sequence)
+*
+* A value of zero will be returned on success, a negative errno will
+* be returned in error cases.
+*/
+
+int regmap_sequence_write(struct regmap *map,
+			  const struct reg_sequence *regs,
+			  int num_regs)
+{
+	int ret;
+
+	map->lock(map->lock_arg);
+
+	ret = _regmap_sequence_write(map, regs, num_regs);
+
+	map->unlock(map->lock_arg);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_sequence_write);
+
 static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 			       unsigned int mask, unsigned int val,
 			       bool *change)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index bf77dfd..fca76e8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -45,6 +45,15 @@ struct reg_default {
 	unsigned int def;
 };
 
+/* For use where the host needs to sequence an array of writes with a
+ * delay in microseconds after some (or all) writes.
+ */
+struct reg_sequence {
+	unsigned int reg;
+	unsigned int def;
+	unsigned int delay_us;
+};
+
 #ifdef CONFIG_REGMAP
 
 enum regmap_endian {
@@ -400,6 +409,9 @@ void regcache_mark_dirty(struct regmap *map);
 int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 			  int num_regs);
 
+int regmap_sequence_write(struct regmap *map, const struct reg_sequence *regs,
+			  int num_regs);
+
 static inline bool regmap_reg_in_range(unsigned int reg,
 				       const struct regmap_range *range)
 {
@@ -595,6 +607,14 @@ static inline struct regmap *dev_get_regmap(struct device *dev,
 	return NULL;
 }
 
+static inline int regmap_sequence_write(struct regmap *map,
+					const struct reg_sequence *regs,
+					int num_regs)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
 #endif
 
 #endif
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] regmap: Add support for sequences of writes with specified delays
  2015-05-26 12:39 [RFC][PATCH] regmap: Add support for sequences of writes with specified delays Nariman Poushin
@ 2015-05-26 15:21 ` Mark Brown
  2015-05-26 15:36   ` Richard Fitzgerald
  0 siblings, 1 reply; 4+ messages in thread
From: Mark Brown @ 2015-05-26 15:21 UTC (permalink / raw
  To: Nariman Poushin; +Cc: patches, gregkh, linux-kernel

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

On Tue, May 26, 2015 at 01:39:21PM +0100, Nariman Poushin wrote:

> Change-Id:Ie9e77aa48f258b353ffa7406d02e19c28d5f2a44

Please don't include noise like this in upstream patches.

> +		if (regs[i].delay_us)
> +			udelay(regs[i].delay_us);

This should be a usleep_range() at least (as checkpatch should have told
you).

> +int regmap_sequence_write(struct regmap *map, const struct reg_sequence *regs,
> +			  int num_regs);

It's a bit sad that this is a separate interface to the existing
sequence writing interface (_multi_reg_write() and _patch()), and
especially that it's a separate implementation.  This means that if
something needs a delay in the sequence it won't get to take advantage
of any optimisations that the rest of the implementations get.

Of course the fact that we used the same struct for both sequences and
the register defaults makes this a bit annoying.  We could either just
add the extra field to the defaults and ignore it (we don't have *that*
many defaults) or just update the existing users to use the new struct
with the additional delay field (which is also fairly straightforward as
we have few users right now).

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] regmap: Add support for sequences of writes with specified delays
  2015-05-26 15:21 ` Mark Brown
@ 2015-05-26 15:36   ` Richard Fitzgerald
  2015-05-27  8:27     ` Nariman Poushin
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Fitzgerald @ 2015-05-26 15:36 UTC (permalink / raw
  To: Mark Brown; +Cc: Nariman Poushin, gregkh, patches, linux-kernel

On Tue, May 26, 2015 at 04:21:00PM +0100, Mark Brown wrote:
> On Tue, May 26, 2015 at 01:39:21PM +0100, Nariman Poushin wrote:
> 
> > Change-Id:Ie9e77aa48f258b353ffa7406d02e19c28d5f2a44
> 
> Please don't include noise like this in upstream patches.
> 
> > +		if (regs[i].delay_us)
> > +			udelay(regs[i].delay_us);
> 
> This should be a usleep_range() at least (as checkpatch should have told
> you).
> 
> > +int regmap_sequence_write(struct regmap *map, const struct reg_sequence *regs,
> > +			  int num_regs);
> 
> It's a bit sad that this is a separate interface to the existing
> sequence writing interface (_multi_reg_write() and _patch()), and
> especially that it's a separate implementation.  This means that if
> something needs a delay in the sequence it won't get to take advantage
> of any optimisations that the rest of the implementations get.
> 
> Of course the fact that we used the same struct for both sequences and
> the register defaults makes this a bit annoying.  We could either just
> add the extra field to the defaults and ignore it (we don't have *that*
> many defaults) or just update the existing users to use the new struct
> with the additional delay field (which is also fairly straightforward as
> we have few users right now).

If we're going to do something to avoid having another API, I prefer the
second option of updating the existing multi write to use the new structure.
The list of register default tables for the Arizona codecs is getting quite
large and adding a delay field to the defaults struct ends up with several
kBytes of wasted entries in the tables. In any case it makes some sense
in that a list of writes to be performed is not necessarily the same
conceptually as a list of register defaults.

> _______________________________________________
> patches mailing list
> patches@opensource.wolfsonmicro.com
> http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] regmap: Add support for sequences of writes with specified delays
  2015-05-26 15:36   ` Richard Fitzgerald
@ 2015-05-27  8:27     ` Nariman Poushin
  0 siblings, 0 replies; 4+ messages in thread
From: Nariman Poushin @ 2015-05-27  8:27 UTC (permalink / raw
  To: Richard Fitzgerald; +Cc: Mark Brown, gregkh, patches, linux-kernel

On Tue, May 26, 2015 at 04:36:54PM +0100, Richard Fitzgerald wrote:
> On Tue, May 26, 2015 at 04:21:00PM +0100, Mark Brown wrote:
> > On Tue, May 26, 2015 at 01:39:21PM +0100, Nariman Poushin wrote:
> > 
> > > Change-Id:Ie9e77aa48f258b353ffa7406d02e19c28d5f2a44

My bad, should have removed it.

> > 
> > Please don't include noise like this in upstream patches.
> > 
> > > +		if (regs[i].delay_us)
> > > +			udelay(regs[i].delay_us);
> > 
> > This should be a usleep_range() at least (as checkpatch should have told
> > you).

Checkpatch did not complain, but I take your point.

> > 
> > > +int regmap_sequence_write(struct regmap *map, const struct reg_sequence *regs,
> > > +			  int num_regs);
> > 
> > It's a bit sad that this is a separate interface to the existing
> > sequence writing interface (_multi_reg_write() and _patch()), and
> > especially that it's a separate implementation.  This means that if
> > something needs a delay in the sequence it won't get to take advantage
> > of any optimisations that the rest of the implementations get.
> > 
> > Of course the fact that we used the same struct for both sequences and
> > the register defaults makes this a bit annoying.  We could either just
> > add the extra field to the defaults and ignore it (we don't have *that*
> > many defaults) or just update the existing users to use the new struct
> > with the additional delay field (which is also fairly straightforward as
> > we have few users right now).
> 
> If we're going to do something to avoid having another API, I prefer the
> second option of updating the existing multi write to use the new structure.
> The list of register default tables for the Arizona codecs is getting quite
> large and adding a delay field to the defaults struct ends up with several
> kBytes of wasted entries in the tables. In any case it makes some sense
> in that a list of writes to be performed is not necessarily the same
> conceptually as a list of register defaults.
> 

Yes, the initial discussion was that it was increasing the memory usage
of the register defaults table (like Richard says some arizona tables have
thousands of defaults).

I am happy to resend using an updated implementation of _multi_reg_write
to use the reg_sequence struct, and update the current users.

Thanks
Nariman

> > _______________________________________________
> > patches mailing list
> > patches@opensource.wolfsonmicro.com
> > http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-05-27  8:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-26 12:39 [RFC][PATCH] regmap: Add support for sequences of writes with specified delays Nariman Poushin
2015-05-26 15:21 ` Mark Brown
2015-05-26 15:36   ` Richard Fitzgerald
2015-05-27  8:27     ` Nariman Poushin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.