Hi Mark, On 09.08.2013 19:16, Ionut Nicu wrote: > On 09.08.2013 13:44, ext Mark Brown wrote: >> On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote: >>> As opposed to the other regmap cache implementations, >>> regcache_flat didn't use the cache_present bitmap for >>> figuring out whether a register was cached or not, nor >>> did it mark a register as present in the cache when >>> regcache_flat_write() was called. >> >>> This caused incorrect behaviour, such as returning >>> value 0 for non-volatile registers without first reading >>> their value from the device and storing it in the cache. >> >> The goal with the flat cache is to do as little work as possible for >> things like memory mapped devices where the cache operations are >> actually noticable in comparison with the I/O costs. I would therefore >> exapect that anything using the flat cache would want to ensure that the >> cache is fully populated at init time, reading back from the device if >> nothing else (by setting num_reg_defaults_raw but not providing values), >> rather than do additional operations in the data path. >> > > Ok, I get your point. I've tried using this approach, but the thing is > that I have a device which supports only single rw operations. The > regcache_hw_init() function calls regmap_raw_read which in turn calls > _regmap_raw_read() when cache_bypass is enabled. But _regmap_raw_read() > doesn't take the use_single_rw flag into account and tries to read > num_reg_defaults_raw registers in a single bus transfer. Unfortunately > this doesn't work with my device. > > I thought about changing regcache_hw_init() to check for this flag in > the same way regmap_bulk_read() does, but I think this is pretty ugly. > > What do you think about moving the check to use_single_rw inside > _regmap_raw_read(), right where the bus accesses are done? > > Something like in the following patch: > > > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c > index 42b45ac..de7f353 100644 > --- a/drivers/base/regmap/regmap.c > +++ b/drivers/base/regmap/regmap.c > @@ -1486,14 +1486,16 @@ static int _regmap_raw_read(struct regmap *map, > unsigned int reg, void *val, > { > struct regmap_range_node *range; > u8 *u8 = map->work_buf; > - int ret; > + size_t val_bytes = map->format.val_bytes; > + size_t reg_size = map->format.reg_bytes + map->format.pad_bytes; > + int ret, i; > > WARN_ON(!map->bus); > > range = _regmap_range_lookup(map, reg); > if (range) { > ret = _regmap_select_page(map, ®, range, > - val_len / map->format.val_bytes); > + val_len / val_bytes); > if (ret != 0) > return ret; > } > @@ -1508,15 +1510,41 @@ static int _regmap_raw_read(struct regmap *map, > unsigned int reg, void *val, > */ > u8[0] |= map->read_flag_mask; > > - trace_regmap_hw_read_start(map->dev, reg, > - val_len / map->format.val_bytes); > + /* > + * Some devices does not support bulk read, for > + * them we have a series of single read operations. > + */ > + if (map->use_single_rw) { > + for (i = 0; i < val_len; i++) { > + trace_regmap_hw_read_start(map->dev, > + reg + (i * map->reg_stride), 1); > + > + map->format.format_reg(map->work_buf, > + reg + (i * map->reg_stride), > + map->reg_shift); > + u8[0] |= map->read_flag_mask; > + > + ret = map->bus->read(map->bus_context, > + map->work_buf, > + reg_size, > + val + (i * val_bytes), > + val_bytes); > + if (ret != 0) > + return ret; > > - ret = map->bus->read(map->bus_context, map->work_buf, > - map->format.reg_bytes + map->format.pad_bytes, > - val, val_len); > + trace_regmap_hw_read_done(map->dev, > + reg + (i * map->reg_stride), 1); > + } > + } else { > + trace_regmap_hw_read_start(map->dev, reg, val_len / val_bytes); > > - trace_regmap_hw_read_done(map->dev, reg, > - val_len / map->format.val_bytes); > + ret = map->bus->read(map->bus_context, map->work_buf, > + reg_size, val, val_len); > + if (ret != 0) > + return ret; > + > + trace_regmap_hw_read_done(map->dev, reg, val_len / val_bytes); > + } > > return ret; > } > @@ -1702,25 +1730,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int > reg, void *val, > return -EINVAL; > > if (vol || map->cache_type == REGCACHE_NONE) { > - /* > - * Some devices does not support bulk read, for > - * them we have a series of single read operations. > - */ > - if (map->use_single_rw) { > - for (i = 0; i < val_count; i++) { > - ret = regmap_raw_read(map, > - reg + (i * map->reg_stride), > - val + (i * val_bytes), > - val_bytes); > - if (ret != 0) > - return ret; > - } > - } else { > - ret = regmap_raw_read(map, reg, val, > - val_bytes * val_count); > - if (ret != 0) > - return ret; > - } > + ret = regmap_raw_read(map, reg, val, val_bytes * val_count); > + if (ret != 0) > + return ret; > > for (i = 0; i < val_count * val_bytes; i += val_bytes) > map->format.parse_inplace(val + i); >
Do you have any feedback for this patch? If you think the change is too complex, I have a simpler solution that could make things work with devices that set use_single_rw. Instead of enabling cache_bypass in regcache_hw_init(), we could temporarily set map->cache_type to REGCACHE_NONE and then call regmap_bulk_read() instead of regmap_raw_read(). Please let me know your opinion on any of the two solutions. Best regards, Ionut Nicu. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/