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

Open Source and information security mailing list archives

Message-ID: <20150527082725.GA28236@opensource.wolfsonmicro.com>
Date:	Wed, 27 May 2015 09:27:25 +0100
From:	Nariman Poushin <nariman@...nsource.wolfsonmicro.com>
To:	Richard Fitzgerald <rf@...nsource.wolfsonmicro.com>
Cc:	Mark Brown <broonie@...nel.org>, gregkh@...uxfoundation.org,
	patches@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] regmap: Add support for sequences of writes with
	specified delays

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@...nsource.wolfsonmicro.com
> > http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives