Re: [PATCH v4 14/15] ASoC: rockchip/rockchip-hdmi-audio: add sound driver for hdmi audio

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

On Sat, Feb 28, 2015 at 10:04:30PM -0500, Yakir Yang wrote:

> +	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
> +	if (ret < 0) {
> +		dev_err(cpu_dai->dev, "failed to set cpu_dai fmt.\n");
> +		return ret;
> +	}

You've already set this in the dai_link, no need to do it again.

> +	dev_info(&pdev->dev, "hdmi audio init success.\n");

Please remove noisy prints like this.

> +free_cpu_of_node:
> +	hdmi_audio_dai.cpu_of_node = NULL;
> +	hdmi_audio_dai.platform_of_node = NULL;
> +free_priv_data:
> +	snd_soc_card_set_drvdata(card, NULL);
> +	platform_set_drvdata(pdev, NULL);
> +	card->dev = NULL;

If any of these assignments is doing anything there's a problem with the
code.

> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +
> +	snd_soc_unregister_card(card);

devm_snd_soc_register_card() and you can remove this function entirely.

> +static const struct of_device_id rockchip_hdmi_audio_of_match[] = {
> +	{ .compatible = "rockchip,rk3288-hdmi-audio", },
> +	{},
> +};

There is no documentation for this binding, binding documentation is
mandatory.  Based on the compatible string this looks like it's specific
to the SoC rather than a design for a board - is the whole card part of
the SoC?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip