Messages in this thread Patch in this message | | | Date | Mon, 19 Sep 2011 17:13:59 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap |
| |
On Mon, Sep 19, 2011 at 05:57:49PM +0200, Lars-Peter Clausen wrote: > On 09/19/2011 03:34 PM, Dimitris Papastamos wrote:
> > + if (!map->cache_bypass) { > > + ret = regcache_write(map, reg, val); > > + if (!ret || map->cache_only) > > + return 0;
> The hw write shouldn't be skipped if the cache write is successful. We should > only exit here if cache_only is set.
There's a couple of other issues too. I've already got the following patch for this one locally:
--- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -304,7 +304,9 @@ static int _regmap_write(struct regmap *map, unsigned int re if (!map->cache_bypass) { ret = regcache_write(map, reg, val); - if (!ret || map->cache_only) + if (ret != 0) + return ret; + if (map->cache_only) return 0; } > I also wonder if we should pass the return value of regcache_write on to the > caller if cache_only is set. Yup.
> Btw. what should happen if both cache_bypass and cache_only are set? Or is that > an invalid configuration?
That's not sensible. Probably BUG_ON().
> > @@ -428,6 +446,14 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
> > mutex_lock(&map->lock);
> > + if (!map->cache_bypass) { > > + ret = regcache_read(map, reg, val); > > + if (!ret) { > > + mutex_unlock(&map->lock); > > + return 0; > > + } > > + }
> This should go into _regmap_read. Otherwise regmap_update_bits will always use > a hw read.
Yup, got that too.
> Also if cache_only is set I guess we shouldn't fallback to a hw read.
Probably. Cache only is *mostly* there for write side stuff, but it'd be useful to add this. I've got a patch to do this.
| |