Re: [PATCH v5 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1324 bytes --]

On Fri, Jul 27, 2018 at 06:05:49PM -0500, Pierre-Louis Bossart wrote:

> new file mode 100644
> index 000000000000..b2a9a6c6b147
> --- /dev/null
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2015-18 Intel Corporation.
> + */

Please make the entire comment a C++ one in the C files so it looks more
intentional.

> +int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
> +{
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +	struct skl_hda_hdmi_pcm *pcm;
> +	char dai_name[NAME_SIZE];
> +	static int i = 1;	/* hdmi codec dai name starts from index 1 */
> +
> +	pcm = devm_kzalloc(card->dev, sizeof(*pcm), GFP_KERNEL);
> +	if (!pcm)
> +		return -ENOMEM;
> +
> +	snprintf(dai_name, sizeof(dai_name), "intel-hdmi-hifi%d", i++);
> +	pcm->codec_dai = snd_soc_card_get_codec_dai(card, dai_name);

I'm confused, i is unconditionally 1 before the print and we then
increment it even though we never reference i again?

> +	if (codec_count == 1 && pdata->codec_mask & IDISP_CODEC_MASK) {
> +		num_links = IDISP_DAI_COUNT;
> +		num_route = IDISP_ROUTE_COUNT;
> +	} else {
> +		return -EINVAL;
> +	}

A warning about unsupported configurations might be nice if the code
ever gets executed on some shiny new laptop.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]