Re: [PATCH 1/7][RFC] regmap: add regmap_raw_update_bits() and merge all regmap_update_bits_xxx()

[Kuninori Morimoto]

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