Messages in this thread | | | Date | Thu, 9 Oct 2008 08:58:08 +0200 | From | "John Kacur" <> | Subject | Fwd: [PATCH 7/9] ASoC: Blackfin: I2S CPU DAI driver |
| |
On Thu, Sep 4, 2008 at 4:43 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > > On Thu, Sep 04, 2008 at 04:22:06PM +0800, Bryan Wu wrote: > > From: Cliff Cai <cliff.cai@analog.com> > > > > Signed-off-by: Cliff Cai <cliff.cai@analog.com> > > Signed-off-by: Bryan Wu <cooloney@kernel.org> > > Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > > Please fix the minor issues below as incremental patches for ease of > review. > > > + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { > > + case SND_SOC_DAIFMT_I2S: > > + break; > > + case SND_SOC_DAIFMT_LEFT_J: > > + ret = -EINVAL; > > + break; > > + } > > The SND_SOC_DAFIMT_LEFT_J: ought to be default: instead - there's more > DAI formats than just that. > > > + if (!bf5xx_i2s.master) { > > + /* > > + * TX and RX are not independent,they are enabled at the same time, > > + * even if only one side is running.So,we need to configure both of > > + * them in advance. > > + * > > + * CPU DAI format:I2S, slave mode. > > + */
+ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + ret = -EINVAL; + break; + case SND_SOC_DAIFMT_CBM_CFS: + ret = -EINVAL; + break; + case SND_SOC_DAIFMT_CBM_CFM: + break; + case SND_SOC_DAIFMT_CBS_CFM: + ret = -EINVAL; + break; + default: + break; + }
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; + }
| |