Re: [PATCH 6/6 v3] regmap: Incorporate the regcache core into regmap

[Dimitris Papastamos <dp@opensource.wolfsonmicro.com>]

On Thu, Sep 15, 2011 at 05:19:32PM +0200, Lars-Peter Clausen wrote:
> > [...]
> > @@ -321,6 +333,18 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
> >  
> >  	mutex_lock(&map->lock);
> >  
> > +	if (!map->cache_bypass) {
> > +		ret = regcache_write(map, reg, val);
> > +		if (ret < 0) {
> > +			mutex_unlock(&map->lock);
> > +			return ret;
> > +		}
> > +		if (map->cache_only) {
> > +			mutex_unlock(&map->lock);
> > +			return 0;
> > +		}
> > +	}
> > +
> 
> Would it make sense to move this into _regmap_write ? In that case the code
> wouldn't have to be duplicated in regmap_update_bits and as a bonus it wouldn't
> have to deal with the mutex either.

Hmm, yes from the point of view of simplifying and avoiding code
duplication.

> >  	ret = _regmap_write(map, reg, val);
> >  
> >  	mutex_unlock(&map->lock);
> > @@ -422,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;
> > +		}
> 
> So in case regmap_readable is not true for this register regcache_read will
> return -EIO and we'll fallback to an uncached read. This doesn't make sense in
> my opinion. Or what are the except semantics regmap_readable supposed to be?

I agree the semantics are fuzzy here.

At the moment, we basically have one means of signaling that the cache
can't handle the read request, that is, by returning an error.  We
should probably extend that or simply use fixed error codes with a
specially meaning or something similar.  I guess there is a more
elegant means of doing this though.

> > +	}
> > +
> >  	ret = _regmap_read(map, reg, val);
> >  
> >  	mutex_unlock(&map->lock);
>