[alsa-devel] [PATCH v4 2/4] ASoC: es8328: Add support for slave mode

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20170123/dbeefabb/attachment.sig>