linux-kernel - Re: [PATCH 1/7][RFC] regmap: add regmap_raw_update_bits() and merge all regmap_update_bits

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h9hainvh.wl%kuninori.morimoto.gx@renesas.com>
Date:	Mon, 15 Feb 2016 04:48:30 +0000
From:	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Greg KH <gregkh@...uxfoundation.org>,
	Linux-Kernel <linux-kernel@...r.kernel.org>,
	Linux-ALSA <alsa-devel@...a-project.org>
Subject: Re: [PATCH 1/7][RFC] regmap: add regmap_raw_update_bits() and merge all regmap_update_bits_xxx()


Hi Mark

OK, I will send v2 patch soon

> 
> > Current regmap has many similar update functions, but the difference is
> > very few. This patch adds new regmap_raw_update_bits() and merge all
> > update functions into it by macro.
> 
> This is a bit hard to review due to the way the diff comes out, it's not
> entirely clear what the code comes out looking like and I'm a bit
> nervous about what the gains might be since macro conversions often
> obscure things (this is making the macros undocumented for example).
> Creating the new function and then using it in a separate patch would be
> better.
> 
> > +int regmap_raw_update_bits(struct regmap *map, unsigned int reg,
> > +			   unsigned int mask, unsigned int val,
> > +			   bool *change, bool async, bool force)
> 
> _raw specifically means something that works with the direct physical
> data format within regmap, a different name would be better.
> 
> > -	map->async = true;
> > +	map->async = async ? true : false;
> 
> This is abuse of the ternery operator where a normal if statement would
> do, and it's also rewriting a bool into a bool so a simple assignment is
> enough.
> 
> >  /**
> > - * regmap_update_bits_check_async: Perform a read/modify/write cycle on the
> > - *                                 register map asynchronously and report if
> > - *                                 updated
> > + * regmap_write_bits: Perform a read/modify/write cycle on the register map
> 
> This looks like it renames update_bits() to write_bits()...
> 
> > +#define regmap_update_bits(map, reg, mask, val) \
> > +	regmap_raw_update_bits(map, reg, mask, val, NULL, false, false)
> > +#define regmap_update_bits_async(map, reg, mask, val)\
> > +	regmap_raw_update_bits(map, reg, mask, val, NULL, true, false)
> > +#define regmap_update_bits_check(map, reg, mask, val, change)\
> > +	regmap_raw_update_bits(map, reg, mask, val, change, false, false)
> > +#define regmap_update_bits_check_async(map, reg, mask, val, change)\
> > +	regmap_raw_update_bits(map, reg, mask, val, change, true, false)
> > +
> 
> ...but we don't seem to use it?  The new name is also a bit confusing.
> [2 signature.asc <application/pgp-signature (7bit)>]
> No public key for 24D68B725D5487D0 created at 2016-02-10T18:38:15+0900 using RSA