Hello!
On 08/19/2013 10:25 AM, Ionut Nicu wrote:
> -------- Original Message --------
> Subject: Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap
> Date: Fri, 09 Aug 2013 19:16:49 +0200
> From: Ionut Nicu <ioan.nicu....@nsn.com>
> To: ext Mark Brown <broo...@kernel.org>
> CC: Greg Kroah-Hartman <gre...@linuxfoundation.org>,
> linux-kernel@vger.kernel.org
>
> 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:
Acked-by: Alexander Sverdlin <alexander.sverd...@nsn.com>
>
> 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);
>
>
>
>
>
--
Best regards,
Alexander Sverdlin.
--
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/
