Re: [PATCH RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders - Russell King

From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Jyri Sarha <jsarha@ti.com>
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	peter.ujfalusi@ti.com, tomi.valkeinen@ti.com,
	dri-devel@lists.freedesktop.org, liam.r.girdwood@linux.intel.com,
	tony@atomide.com, broonie@kernel.org, bcousson@baylibre.com,
	linux-omap@vger.kernel.org
Subject: Re: [PATCH RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
Date: Fri, 14 Aug 2015 10:57:41 +0100	[thread overview]
Message-ID: <20150814095741.GG7576@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <ed41a6c0b594866c05e695cc066371d0e3862c7e.1439542916.git.jsarha@ti.com>

On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
> +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> +	struct hdmi_codec_params hp = {
> +		.cea = { 0 },

Unnecessary initialisation - because you are initialising this structure,
all unnamed fields will be zeroed.

> +		.iec = {
> +			.status = {
> +				IEC958_AES0_CON_NOT_COPYRIGHT,
> +				IEC958_AES1_CON_GENERAL,
> +				IEC958_AES2_CON_SOURCE_UNSPEC,
> +				IEC958_AES3_CON_CLOCK_VARIABLE,
> +			},

...

> +	hdmi_audio_infoframe_init(&hp.cea);
> +	hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_PCM;

Something tells me here that you haven't read the HDMI specification.
HDMI says that the coding type will be zero (refer to stream header).
The same goes for much of the CEA audio infoframe.  Please see the
Audio InfoFrame details in the HDMI specification.

> +	hp.cea.channels = params_channels(params);
> +
> +	switch (params_width(params)) {
> +	case 16:
> +		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_20_16;
> +		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> +		break;
> +	case 18:
> +		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_22_18;
> +		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
> +		break;
> +	case 20:
> +		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_24_20;
> +		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
> +		break;
> +	case 24:
> +	case 32:
> +		hp.iec.status[4] |= IEC958_AES4_CON_MAX_WORDLEN_24 |
> +			IEC958_AES4_CON_WORDLEN_24_20;

Why not use the generic code to generate the AES channel status bits?
See sound/core/pcm_iec958.c.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel