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.
Attachment:
signature.asc
Description: PGP signature
