Messages in this thread | | | Date | Thu, 9 Oct 2008 10:02:55 +0200 | From | "John Kacur" <> | Subject | Re: Fwd: [PATCH 7/9] ASoC: Blackfin: I2S CPU DAI driver |
| |
On Thu, Oct 9, 2008 at 9:35 AM, <Valdis.Kletnieks@vt.edu> wrote: > 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.. :) >
Well, of course you only want to make things shorter when it increases clarity, you don't want to make things shorter for the sake of making them shorter, so yeah, I would nix that non-standard default position too.
| |