Re: Fwd: [PATCH 7/9] ASoC: Blackfin: I2S CPU DAI driver

[Valdis.Kletnieks@vt.edu]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On Thu, 09 Oct 2008 08:58:08 +0200, John Kacur said:

> My eyes fell upon this switch statement, probably I have similar
> criticisms as to what has already been said, but:
> 1. Surely the default case is also an -EINVAL
> 2. Why not let all the EINVALS fall through, it will shorten up the
> code, and IMO make it more readable, something like this?
> +       switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +       case SND_SOC_DAIFMT_CBM_CFM: /* Passing Case */
> +               break;
> +       case SND_SOC_DAIFMT_CBS_CFS: /* Failing Cases */
> +       case SND_SOC_DAIFMT_CBM_CFS:
> +       case SND_SOC_DAIFMT_CBS_CFM:
> +               ret = -EINVAL;
> +               break;
> +       default:
> +               printk(KERN_INFO "Unknown SND_SOC_DAIFMT kind\n");
> +               ret = -EINVAL;
> +               break;
> +       }

Even shorter, but puts the default in a non-standard place:

+       switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+       case SND_SOC_DAIFMT_CBM_CFM: /* Passing Case */
+               break;
+       default:			/* So much Fail we should say something */
+               printk(KERN_INFO "Unknown SND_SOC_DAIFMT kind\n");
+       case SND_SOC_DAIFMT_CBS_CFS: /* Failing Cases */
+       case SND_SOC_DAIFMT_CBM_CFS:
+       case SND_SOC_DAIFMT_CBS_CFM:
+               ret = -EINVAL;
+               break;
+	}

(I see a checkpatch update to check where default: is, coming in 3.. 2.. 1.. :)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]