Re: [RFC][PATCH] regmap: Add support for sequences of writes with specified delays

[Nariman Poushin <nariman@opensource.wolfsonmicro.com>]

On Tue, May 26, 2015 at 04:36:54PM +0100, Richard Fitzgerald wrote:
> On Tue, May 26, 2015 at 04:21:00PM +0100, Mark Brown wrote:
> > On Tue, May 26, 2015 at 01:39:21PM +0100, Nariman Poushin wrote:
> > 
> > > Change-Id:Ie9e77aa48f258b353ffa7406d02e19c28d5f2a44

My bad, should have removed it.

> > 
> > Please don't include noise like this in upstream patches.
> > 
> > > +		if (regs[i].delay_us)
> > > +			udelay(regs[i].delay_us);
> > 
> > This should be a usleep_range() at least (as checkpatch should have told
> > you).

Checkpatch did not complain, but I take your point.

> > 
> > > +int regmap_sequence_write(struct regmap *map, const struct reg_sequence *regs,
> > > +			  int num_regs);
> > 
> > It's a bit sad that this is a separate interface to the existing
> > sequence writing interface (_multi_reg_write() and _patch()), and
> > especially that it's a separate implementation.  This means that if
> > something needs a delay in the sequence it won't get to take advantage
> > of any optimisations that the rest of the implementations get.
> > 
> > Of course the fact that we used the same struct for both sequences and
> > the register defaults makes this a bit annoying.  We could either just
> > add the extra field to the defaults and ignore it (we don't have *that*
> > many defaults) or just update the existing users to use the new struct
> > with the additional delay field (which is also fairly straightforward as
> > we have few users right now).
> 
> If we're going to do something to avoid having another API, I prefer the
> second option of updating the existing multi write to use the new structure.
> The list of register default tables for the Arizona codecs is getting quite
> large and adding a delay field to the defaults struct ends up with several
> kBytes of wasted entries in the tables. In any case it makes some sense
> in that a list of writes to be performed is not necessarily the same
> conceptually as a list of register defaults.
> 

Yes, the initial discussion was that it was increasing the memory usage
of the register defaults table (like Richard says some arizona tables have
thousands of defaults).

I am happy to resend using an updated implementation of _multi_reg_write
to use the reg_sequence struct, and update the current users.

Thanks
Nariman

> > _______________________________________________
> > patches mailing list
> > patches@opensource.wolfsonmicro.com
> > http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches
>