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; + }