Re: [PATCH 1/3] OMAPDSS/ASoC: Relocate ASoC HDMI codec. Part 2

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

On Sat, Feb 11, 2012 at 06:23:15PM -0600, Ricardo Neri wrote:

> The ASoC HDMI codec is to be relocated under sound/soc/codecs. This
> patch places the codec under ASoC. A previous patch removed the code
> from DSS. The purpose of the relocation is to give a more logical
> organization to OMAP HDMI code.

Looks pretty clean, a few things below but mostly fairly nitpicky.

>  sound/soc/codecs/omap-hdmi.c |  422 ++++++++++++++++++++++++++++++++++++++++++

If this is relocating the code shouldn't some old code be being deleted?

> +	switch (sample_freq) {
> +	case 32000:
> +		if ((deep_color == 125) && ((pclk == 54054)
> +				|| (pclk == 74250)))
> +			*n = 8192;
> +		else
> +			*n = 4096;

Use a switch on deep_color too (in all these cases).

> +
> +static int hdmi_audio_startup(struct snd_pcm_substream *substream,
> +				  struct snd_soc_dai *dai)
> +{
> +	if (omapdss_hdmi_get_hdmi_mode() != HDMI_HDMI) {
> +		pr_err("Current video settings do not support audio.\n");

dev_ prints please (throughout the driver).

> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (priv == NULL)
> +		return -ENOMEM;

Use devm_ functions for this and mapping the io region - that way you
don't have to worry about them when cleaning up and doing error
handling...

> +	hdmi_rsrc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (!hdmi_rsrc) {
> +		dev_err(&pdev->dev, "Cannot obtain IORESOURCE_MEM HDMI\n");
> +		return -EINVAL;
> +	}

...like here :)  You should also move the allocation and mapping to the
platform device probe and only register the CODEC once you've got
everything else, this is more idiomatic for the Linux driver stack.

> +static struct snd_soc_dai_driver hdmi_codec_dai_drv = {
> +	.name = "hdmi-audio-codec",

Should proably have an omap- in here somewhere.

> +static int __init hdmi_codec_init(void)
> +{
> +	return platform_driver_register(&hdmi_codec_driver);
> +}
> +module_init(hdmi_codec_init);

module_platform_driver()

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

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