[PATCH 1/7] regmap: Fix regcache_sync generic implementation

[PATCH 1/7] regmap: Fix regcache_sync generic implementation

[Dimitris Papastamos]

We want to use regmap_write() to actually write anything
to the HW.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 6b9efd9..5364dde 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -242,7 +242,7 @@ int regcache_sync(struct regmap *map)
 			if (ret < 0)
 				goto out;
 			regcache_cache_bypass(map, true);
-			ret = regcache_write(map, i, val);
+			ret = regmap_write(map, i, val);
 			regcache_cache_bypass(map, false);
 			if (ret < 0)
 				goto out;
-- 
1.7.6.4

[PATCH 2/7] regmap: Modify map->cache_bypass directly

[Dimitris Papastamos]

In preperation for the upcoming patches, modify map->cache_bypass
directly.  The helper functions will grab an exclusive lock.  Because
we'll have acquired the same lock we need to avoid a deadlock.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 5364dde..f46e247 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -241,9 +241,9 @@ int regcache_sync(struct regmap *map)
 			ret = regcache_read(map, i, &val);
 			if (ret < 0)
 				goto out;
-			regcache_cache_bypass(map, true);
+			map->cache_bypass = 1;
 			ret = regmap_write(map, i, val);
-			regcache_cache_bypass(map, false);
+			map->cache_bypass = 0;
 			if (ret < 0)
 				goto out;
 			dev_dbg(map->dev, "Synced register %#x, value %#x\n",
-- 
1.7.6.4

[PATCH 3/7] regmap: Add a mutex to guard the sync operation

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/internal.h |    1 +
 drivers/base/regmap/regcache.c |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 2d51b1b..a0b931d 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -32,6 +32,7 @@ struct regmap_format {
 
 struct regmap {
 	struct mutex lock;
+	struct mutex sync_lock;
 
 	struct device *dev; /* Device we do I/O on */
 	void *work_buf;     /* Scratch buffer used to format I/O */
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index f46e247..b843b5f 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -130,6 +130,8 @@ int regcache_init(struct regmap *map)
 	if (!map->max_register)
 		map->max_register = map->num_reg_defaults_raw;
 
+	mutex_init(&map->sync_lock);
+
 	if (map->cache_ops->init) {
 		dev_dbg(map->dev, "Initializing %s cache\n",
 			map->cache_ops->name);
@@ -230,6 +232,7 @@ int regcache_sync(struct regmap *map)
 
 	BUG_ON(!map->cache_ops);
 
+	mutex_lock(&map->sync_lock);
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;
@@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	mutex_unlock(&map->sync_lock);
 
 	return ret;
 }
-- 
1.7.6.4

[PATCH 4/7] regmap: Save/restore the bypass state upon syncing

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index b843b5f..695802c 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -229,10 +229,13 @@ int regcache_sync(struct regmap *map)
 	unsigned int val;
 	unsigned int i;
 	const char *name;
+	unsigned int bypass;
 
 	BUG_ON(!map->cache_ops);
 
 	mutex_lock(&map->sync_lock);
+	/* Remember the initial bypass state */
+	bypass = map->cache_bypass;
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;
@@ -257,6 +260,8 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	/* Restore the bypass state */
+	map->cache_bypass = bypass;
 	mutex_unlock(&map->sync_lock);
 
 	return ret;
-- 
1.7.6.4

[PATCH 5/7] regmap: Grab the lock in regcache_cache_only()

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 695802c..8900897 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -282,7 +282,9 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	mutex_lock(&map->sync_lock);
 	map->cache_only = enable;
+	mutex_unlock(&map->sync_lock);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
-- 
1.7.6.4

[PATCH 6/7] regmap: Implement regcache_cache_bypass helper function

[Dimitris Papastamos]

Ensure we've got a function so users can enable/disable the
cache bypass option.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |   19 +++++++++++++++++++
 include/linux/regmap.h         |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 8900897..a3f466c 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -288,6 +288,25 @@ void regcache_cache_only(struct regmap *map, bool enable)
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
+/**
+ * regcache_cache_bypass: Put a register map into cache bypass mode
+ *
+ * @map: map to configure
+ * @cache_only: flag if changes should not be written to the hardware
+ *
+ * When a register map is marked with the cache bypass option, writes
+ * to the register map API will only update the hardware and not the
+ * the cache directly.  This is useful when syncing the cache back to
+ * the hardware.
+ */
+void regcache_cache_bypass(struct regmap *map, bool enable)
+{
+	mutex_lock(&map->sync_lock);
+	map->cache_bypass = enable;
+	mutex_unlock(&map->sync_lock);
+}
+EXPORT_SYMBOL_GPL(regcache_cache_bypass);
+
 bool regcache_set_val(void *base, unsigned int idx,
 		      unsigned int val, unsigned int word_size)
 {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 76ac255..3daac2d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -142,5 +142,6 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 
 int regcache_sync(struct regmap *map);
 void regcache_cache_only(struct regmap *map, bool enable);
+void regcache_cache_bypass(struct regmap *map, bool enable);
 
 #endif
-- 
1.7.6.4

[PATCH 7/7] regmap: Ensure we scream if we enable cache bypass/only at the same time

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index a3f466c..3d59f14 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -282,6 +282,8 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_bypass && enable);
+
 	mutex_lock(&map->sync_lock);
 	map->cache_only = enable;
 	mutex_unlock(&map->sync_lock);
@@ -301,6 +303,8 @@ EXPORT_SYMBOL_GPL(regcache_cache_only);
  */
 void regcache_cache_bypass(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_only && enable);
+
 	mutex_lock(&map->sync_lock);
 	map->cache_bypass = enable;
 	mutex_unlock(&map->sync_lock);
-- 
1.7.6.4

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

[Mark Brown]

On Wed, Sep 28, 2011 at 11:43:43AM +0100, Dimitris Papastamos wrote:

> +	mutex_lock(&map->sync_lock);
>  	dev_dbg(map->dev, "Syncing %s cache\n",
>  		map->cache_ops->name);
>  	name = map->cache_ops->name;
> @@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
>  	}
>  out:
>  	trace_regcache_sync(map->dev, name, "stop");
> +	mutex_unlock(&map->sync_lock);

Shouldn't we either be taking the regmap lock when doing the sync or
otherwise guarding against something other than a cache sync?

Re: [PATCH 1/7] regmap: Fix regcache_sync generic implementation

[Mark Brown]

On Wed, Sep 28, 2011 at 11:43:41AM +0100, Dimitris Papastamos wrote:
> We want to use regmap_write() to actually write anything
> to the HW.

Applied, thanks.

Re: [PATCH 2/7] regmap: Modify map->cache_bypass directly

[Mark Brown]

On Wed, Sep 28, 2011 at 11:43:42AM +0100, Dimitris Papastamos wrote:
> In preperation for the upcoming patches, modify map->cache_bypass
> directly.  The helper functions will grab an exclusive lock.  Because
> we'll have acquired the same lock we need to avoid a deadlock.

Applied, thanks.

Re: [PATCH 5/7] regmap: Grab the lock in regcache_cache_only()

[Mark Brown]

On Wed, Sep 28, 2011 at 11:43:45AM +0100, Dimitris Papastamos wrote:
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Applied, thanks.

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

[Dimitris Papastamos]

On Wed, Sep 28, 2011 at 01:52:50PM +0100, Mark Brown wrote:
> On Wed, Sep 28, 2011 at 11:43:43AM +0100, Dimitris Papastamos wrote:
> 
> > +	mutex_lock(&map->sync_lock);
> >  	dev_dbg(map->dev, "Syncing %s cache\n",
> >  		map->cache_ops->name);
> >  	name = map->cache_ops->name;
> > @@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
> >  	}
> >  out:
> >  	trace_regcache_sync(map->dev, name, "stop");
> > +	mutex_unlock(&map->sync_lock);
> 
> Shouldn't we either be taking the regmap lock when doing the sync or
> otherwise guarding against something other than a cache sync?

Em the main issue is that the sync() implementation will use
regmap_write() which will grab map->lock.  To avoid this we could have
gone and used directly the lockless _regmap_write() but that's
static.  To be honest, it feels cleaner to have only 1 lock to guard
the map so maybe we should get rid of map->sync_lock.

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

[Mark Brown]

On Wed, Sep 28, 2011 at 03:24:06PM +0100, Dimitris Papastamos wrote:
> On Wed, Sep 28, 2011 at 01:52:50PM +0100, Mark Brown wrote:

> > Shouldn't we either be taking the regmap lock when doing the sync or
> > otherwise guarding against something other than a cache sync?

> Em the main issue is that the sync() implementation will use
> regmap_write() which will grab map->lock.  To avoid this we could have
> gone and used directly the lockless _regmap_write() but that's
> static.  To be honest, it feels cleaner to have only 1 lock to guard
> the map so maybe we should get rid of map->sync_lock.

Making _regmap_write() global seems fine, it's not an externally visible
API but the cache code is part of the infrastructure.

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

[Dimitris Papastamos]

On Wed, Sep 28, 2011 at 04:19:37PM +0100, Mark Brown wrote:
> On Wed, Sep 28, 2011 at 03:24:06PM +0100, Dimitris Papastamos wrote:
> > On Wed, Sep 28, 2011 at 01:52:50PM +0100, Mark Brown wrote:
> 
> > > Shouldn't we either be taking the regmap lock when doing the sync or
> > > otherwise guarding against something other than a cache sync?
> 
> > Em the main issue is that the sync() implementation will use
> > regmap_write() which will grab map->lock.  To avoid this we could have
> > gone and used directly the lockless _regmap_write() but that's
> > static.  To be honest, it feels cleaner to have only 1 lock to guard
> > the map so maybe we should get rid of map->sync_lock.
> 
> Making _regmap_write() global seems fine, it's not an externally visible
> API but the cache code is part of the infrastructure.

Okay, so I'll go forth and revert the use of map->sync_lock
and just grab map->lock.

Thanks,
Dimitris