lkml.org 
[lkml]   [2014]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write
Date
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 28 February 2014 03:37
> To: Opensource [Anthony Olech]
> Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; David Dajun Chen
> Subject: Re: [RFC V1] drivers/base/regmap: Implementation for
> regmap_multi_reg_write
> On Thu, Feb 27, 2014 at 11:28:56AM +0000, Opensource [Anthony Olech]
> wrote:
> > This is the implementation of regmap_multi_reg_write()
> > It replaces the first definition, which just defined the API.
> Aside from any review comments this won't apply with the recent patches
> that Charles did to provide a bypassed version of the API, it needs to be
> rebased.

I see that next-20140228 has Charles' patch applied, my next attempt
will be rebased against the latest linux-next.

> > a) should an async operation be allowed? easy in the case where
> > all the changes are in the same page - but if the operation
> > is broken due to changes over several pages not so easy.
> It's fine to support only the simple cases, async operation is just an
> optimisation so we can always just serialise in cases where it gets
> complicated and someone can optimise later if they care. It'd be fine to just
> decay to a series of regmap_reg_write()s if there's paging involved.

The algorithm for splitting up into smaller _multi_reg_writes is easy enough,
so if the calling device driver created a set of (reg,val) pairs for a multi reg
write operation then surely the intention is for the individual pieces to be
handled as multi reg writes.

> > b) the user supplied set (array of struct reg_default) of changes
> > has the register address modified when the target page alters.
> > Would it be better not to do an in-situ change, but rather to
> > alloc a new array of struct reg_default?
> Yes, the user should be able to pass in a const pointer (indeed Charles
> changed the API to do that).

my next attempt will match the API.

> > +++ b/drivers/base/regmap/regmap.c
> > @@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field
> > *field, unsigned int val) } EXPORT_SYMBOL_GPL(regmap_field_write);
> > +
> > /**
> > * regmap_field_update_bits(): Perform a read/modify/write cycle
> Random whitespace change here.

sorry about that - it slipped past my quality control!

> > +static int _switch_register_page(struct regmap *map, unsigned int
> win_page,
> > + struct regmap_range_node *range) {
> > + int ret;
> > + bool page_chg;
> > + void *orig_work_buf = map->work_buf;
> > + unsigned int swp;
> > +
> > + map->work_buf = map->selector_work_buf;
> > +
> > + swp = win_page << range->selector_shift;
> > + ret = _regmap_update_bits(map,
> > + range->selector_reg,
> > + range->selector_mask,
> > + swp, &page_chg);
> > +
> > + map->work_buf = orig_work_buf;
> > +
> I'd expect this to be using _regmap_select_page()? In general there seems
> like quite a bit of duplication to handle paging.

I will try to revamp that part!

> > + return ret;
> > +}
> > +/*
> You need a blank here.

I missed that one as well - I will do better in my next attempt!

> > + buf = kzalloc(len , GFP_KERNEL);
> > +
> > + if (!buf)
> > + return -ENOMEM;
> Coding style - extra blank between the kzalloc and the check and an extra
> space before the comma.

it will be fixed.

> > + /*
> > + * the set of registers are not neccessarily in order, but
> > + * since the order of write must be preserved this algorithm
> > + * chops the set each time the page changes
> > + */
> > + for (i = 0, n = 0, switched = false, base = regs; i < num_regs;
> > + i++, n++) {
> Don't put all this stuff in the for (), just put the iteration in the for ().

all those variables are a fundamental part of the loop, but I will change it.

> > + /*
> > + * Some devices do not support multi write, for
> > + * them we have a series of single write operations.
> > + */
> > + if (map->use_single_rw) {
> > + for (i = 0; i < num_regs; i++) {
> > + ret = _regmap_write(map, regs[i].reg, regs[i].def);
> > + if (ret != 0)
> > + goto out;
> > + }
> > + } else {
> > + ret = _regmap_multi_reg_write(map, regs, num_regs);
>
> I'd expect to see something that devices do to specifically advertise this
> capability, it doesn't follow that a device that a device that only supports
> single writes will support the multi write operation and frameworks may try
> to use the multi write API to help optimise things.

Yes, you are correct - I think a driver needs to pass an extra bit of
information in "struct regmap_config" to indicate that it is capable
of using the multi_req_write mode.

I will invent a new flag

many thanks Mark for your very help review and comments.

Tony Olech
Dialog Semiconductor


\
 
 \ /
  Last update: 2014-02-28 17:41    [W:0.068 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site