Re: [PATCH 3/4] regmap: Add support for register indirect addressing.

From: Mark Brown
Date: Thu May 31 2012 - 13:25:39 EST


On Thu, May 31, 2012 at 04:47:20PM +0200, Krystian Garbaciak wrote:

Adding people who've got some chips with paging, please keep them in the
CCs on this unless they complain (though since I'm cutting context... :/ )

> + /* Partition all accessible registers on address ranges,
> + either to be accessed directly or indirectly. Arrange range
> + list by ascending addresses. */

Wouldn't something naturally sorted like a rbtree be a more direct way
of doing this?

> + range_cfg = NULL;
> + for (n = 0, min_base = UINT_MAX; n < config->n_ranges; n++)
> + if (range_base <= config->ranges[n].base_reg &&
> + config->ranges[n].base_reg <= min_base)
> + range_cfg = &config->ranges[n];
> +

I've stared at this for a little while and I'm really not sure what it's
supposed to do. The whole thing with min_base is just a bit odd, we're
doing comparisons against it but we never update it so why aren't we
using a constant, and in fact the comparison is always going to be true
since we're comparing against UINT_MAX.

I suspect it's supposed to pick the range with the lowest base but I'm
not convinced it does that.

> + if (!range_cfg || range_cfg->base_reg > range_base) {
> + /* Range of registers for direct access */
> + range = kzalloc(sizeof(*range), GFP_KERNEL);
> + if (range == NULL) {
> + ret = -ENOMEM;
> + goto err_range;
> + }
> + range->base_reg = range_base;
> + if (range_cfg)
> + range->max_reg = range_cfg->base_reg - 1;
> + else
> + range->max_reg = UINT_MAX;
> + list_add_tail(&range->list, &map->range_list);
> + }

This is making my head hurt too, possibly because of the lack of clarity
in the above.

> +static int _regmap_update_bits(struct regmap *map, unsigned int reg,
> + unsigned int mask, unsigned int val,
> + bool *change);

Put this up at the top of the file.

> +static int
> +_regmap_range_access(int (*regmap_bus_access)(struct regmap *map,
> + unsigned int reg,
> + void *val, unsigned int val_len),

eew, typedef this!

> + unsigned int _page, _p;
> + unsigned int _reg, _r;
> + unsigned int _num;

These _s aren't helping legibility here.

> + /* Bulk write should not cross single range boundaries */
> + if (val_num != 0 &&
> + reg + val_num - 1 > range->max_reg)
> + return -EINVAL;

When would val_num ever be zero?

> + /* Update page register (may use caching) */
> + ret = _regmap_update_bits(map, range->page_sel_reg,
> + range->page_sel_mask,
> + _page << range->page_sel_shift,
> + &change);
> + if (ret < 0)
> + return ret;

Why the comment about the cache - why would this go wrong?

> + /* There is no point to pass cache for data
> + registers, as they should be volatile anyway */
> + ret = _regmap_range_access(regmap_bus_access,
> + map, _reg, _val, _num);
> + if (ret < 0)
> + return ret;

That comment needs some clarification too...

> +/**
> + * Configuration for indirect accessed register range.
> + * Indirect or paged registers, can be defined with one or more structures.

No , here.

> + * @translate_reg: Function should return indirect address/page number and
> + * register number (out of this range) matching virtual_reg.

Why does the user need to specify this? Shouldn't we just specify a
size for the underlying window and then have a default which does the
obvious translations? I'd imagine an *overwhelming* proportion of users
will want to do that. Allowing an override is fine but requiring code
seems wrong for something like this.

Attachment: signature.asc
Description: Digital signature