Re: [alsa-devel] [RFC 1/2] ASoC: core: Reordered DAPM update power on widgets

On Thu, Dec 02, 2010 at 02:03:09PM +0200, Peter Ujfalusi wrote:

• unsigned int dapm_reorder_pupdate:1; /* Reordered pupdate in widgets */

Please use a more meaningful name than pupdate, I can't parse what that stands for easily (I'm guessing power update?) and even with that it's not clear what the option actually does.

• unsigned int change, dapm_pupdate_first = 1;

Please split the new variable onto a separate line - mixed initialised and uninitialised variables are confusing to read.

• if (snd_soc_test_bits(widget->codec, reg, val_mask, val)) {

• if (val)
  

• 	/* new connection */
  

• 	connect = invert ? 0:1;
  

• else
  

• 	/* old connection must be powered down */
  

• 	connect = invert ? 1:0;
  

• change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
• if (val)

• /* new connection */
  

• connect = invert ? 0 : 1;
  

• else

• /* old connection must be powered down */
  

This is really confusing - if there's no change why are we not just exiting the function immediately? It makes the rest of the code much harder to follow as the conditionals all get more complex.