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

[Peter Ujfalusi]

On Thursday 02 December 2010 14:21:28 ext Mark Brown wrote:
> 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.

I'll try to figure out better name.

> 
> > +	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.

True.
 
> > -	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.

Yes, in that case we do not even need the change variable, since if there is no 
change I just: goto out;
Makes it cleaner.

Thanks,
Péter