Messages in this thread | | | From | "Opensource [Anthony Olech]" <> | Subject | RE: [RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write | Date | Fri, 28 Feb 2014 15:58:34 +0000 |
| |
> -----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
| |