| Message ID | 20170123104149.2508-3-romain.perier@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 23, 2017 at 11:41:47AM +0100, Romain Perier wrote: > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBM_CFM: > + master = true; > + break; > + case SND_SOC_DAIFMT_CBS_CFS: > + master = false; > + break; > + default: > + return -EINVAL; > + } Please use the normal kernel coding style. People read in part by pattern matching so this is important to ease review and coding style problems are a big warning sign that the code also has problems with other, more substantial, understanding of how the kernel is expected to work. > - /* Master serial port mode, with BCLK generated automatically */ > - snd_soc_update_bits(codec, ES8328_MASTERMODE, > - ES8328_MASTERMODE_MSC, ES8328_MASTERMODE_MSC); > + if (master) { > + /* Master serial port mode, with BCLK generated automatically */ > + snd_soc_update_bits(codec, ES8328_MASTERMODE, > + ES8328_MASTERMODE_MSC, > + ES8328_MASTERMODE_MSC); > + } else { > + /* Slave serial port mode */ > + snd_soc_update_bits(codec, ES8328_MASTERMODE, > + ES8328_MASTERMODE_MSC, > + 0); > + } Why not just directly do this in the switch statement? This appears to be the only place where we change behaviour so setting a variable at the top of the function then using it at the bottom doesn't seem to add anything.
diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c index 37722194..054e123 100644 --- a/sound/soc/codecs/es8328.c +++ b/sound/soc/codecs/es8328.c @@ -588,10 +588,18 @@ static int es8328_set_dai_fmt(struct snd_soc_dai *codec_dai, struct snd_soc_codec *codec = codec_dai->codec; u8 dac_mode = 0; u8 adc_mode = 0; + bool master; - /* set master/slave audio interface */ - if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBM_CFM) - return -EINVAL; + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBM_CFM: + master = true; + break; + case SND_SOC_DAIFMT_CBS_CFS: + master = false; + break; + default: + return -EINVAL; + } /* interface format */ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -620,9 +628,17 @@ static int es8328_set_dai_fmt(struct snd_soc_dai *codec_dai, snd_soc_update_bits(codec, ES8328_ADCCONTROL4, ES8328_ADCCONTROL4_ADCFORMAT_MASK, adc_mode); - /* Master serial port mode, with BCLK generated automatically */ - snd_soc_update_bits(codec, ES8328_MASTERMODE, - ES8328_MASTERMODE_MSC, ES8328_MASTERMODE_MSC); + if (master) { + /* Master serial port mode, with BCLK generated automatically */ + snd_soc_update_bits(codec, ES8328_MASTERMODE, + ES8328_MASTERMODE_MSC, + ES8328_MASTERMODE_MSC); + } else { + /* Slave serial port mode */ + snd_soc_update_bits(codec, ES8328_MASTERMODE, + ES8328_MASTERMODE_MSC, + 0); + } return 0; }
Currently, the function that changes the DAI format only supports master mode. Trying to use a slave mode exits the function with -EINVAL and leave the codec misconfigured. This commits adds support for enabling the slave mode. Signed-off-by: Romain Perier <romain.perier@collabora.com> --- Changes in v4: None Changes in v3: None Changes in v2: None sound/soc/codecs/es8328.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)