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

[Jyri Sarha <jsarha@ti.com>]

On 08/14/15 12:57, Russell King - ARM Linux wrote:
> 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.
>

True, I just tried to be explicit.

>> +		.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.
>

Must admit, that I have not read it end to end. Obviously I have missed 
a relevant piece of information here. I'll fix that and check the 
related items too.

>> +	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.
>

Thanks, I did not know that exist. I'll make use of that.

Best regards,
Jyri