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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
