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

[Mark Brown]

On Thu, May 31, 2012 at 06:37:00PM +0000, Krystian Garbaciak wrote:

Fix your mailer to word wrap between paragraphs, your mails are not easy
to read.

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

> I expect here to have one or two ranges registered. Do you think,
> rbtree will be more efficient?

It might make the code rather more obvious, right now it's not exactly
clear.

> > > +		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.

> I  am searching for a range configuration with the lowest address
> range, that was not added yet.  I use range_base as a pointer to mark
> the position of base address for the next range to be added.

None of which really addresses what I'm saying at all - the code is very
obscure, especially whatever you're doing with min_base which works out
as an always true comparison with a constant as far as I can tell.

> > > +		if (!range_cfg || range_cfg->base_reg > range_base) {
> > > +			/* Range of registers for direct access */

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

> Any empty space before configured virtual range is filled with range
> used for direct access. Empty address space, after all defined ranges,
> is used for direct access too (If that makes sense?). To mark such
> range (translate_reg==NULL).

I got what it's supposed to do, it's just not at all obvious how it
accomplishes this.  Like I say the fact that the immediately preceeding
code upon which it relies is as clear as mud isn't helping here.

> > > +			/* 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?

> Nothing. _regmap_update_bits() is used, so the cache can be hit here
> and speed up paging.

So why is this so surprising that we need a comment?  The comment seems
like it's flagging something that might be broken but fortunately isn't.

> Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, 
> some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it
> is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, 
> copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
> 
> Please consider the environment before printing this e-mail

You might want to see about removing this...  clearly you can do so
since your patches don't have it?

Attachment: signature.asc
Description: Digital signature