Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Arnaud Pouliquen <arnaud.pouliquen@st.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "kernel@stlinux.com" <kernel@stlinux.com>,
	Takashi Iwai <tiwai@suse.de>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	Jyri Sarha <jsarha@ti.com>, David Airlie <airlied@linux.ie>,
	"broonie@kernel.org" <broonie@kernel.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control
Date: Tue, 13 Dec 2016 22:23:54 +0900	[thread overview]
Message-ID: <636ed854-e80c-e18c-314d-54c870d2c1bc@sakamocchi.jp> (raw)
In-Reply-To: <6589c8e0-f918-10c3-3d7b-39d50b429850@st.com>

Hi Arnaud,

On Dec 12 2016 22:46, Arnaud Pouliquen wrote:
> On 12/12/2016 01:03 PM, Takashi Sakamoto wrote:
>> On 2016年12月12日 18:38, Arnaud Pouliquen wrote:
>>>>> + */
>>>>> +static struct hdmi_codec_cea_spk_alloc hdmi_codec_channel_alloc[] = {
>>>>> +/*			  channel:   7     6    5    4    3     2    1    0  */
>>>>> +{ .ca_index = 0x00,  .speakers = {   0,    0,   0,   0,   0,    0,  FR,  FL } },
>>>>> +				 /* 2.1 */
>>>>> +{ .ca_index = 0x01,  .speakers = {   0,    0,   0,   0,   0,  LFE,  FR,  FL } },
>>>>> +				 /* Dolby Surround */
>>>>> +{ .ca_index = 0x02,  .speakers = {   0,    0,   0,   0,  FC,    0,  FR,  FL } },
>>>>> +				 /* surround51 */
>>>>> +{ .ca_index = 0x0b,  .speakers = {   0,    0,  RR,  RL,  FC,  LFE,  FR,  FL } },
>>>>> +				 /* surround40 */
>>>>> +{ .ca_index = 0x08,  .speakers = {   0,    0,  RR,  RL,   0,    0,  FR,  FL } },
>>>>> +				 /* surround41 */
>>>>> +{ .ca_index = 0x09,  .speakers = {   0,    0,  RR,  RL,   0,  LFE,  FR,  FL } },
>>>>> +				 /* surround50 */
>>>>> +{ .ca_index = 0x0a,  .speakers = {   0,    0,  RR,  RL,  FC,    0,  FR,  FL } },
>>>>> +				 /* 6.1 */
>>>>> +{ .ca_index = 0x0f,  .speakers = {   0,   RC,  RR,  RL,  FC,  LFE,  FR,  FL } },
>>>>> +				 /* surround71 */
>>>>> +{ .ca_index = 0x13,  .speakers = { RRC,  RLC,  RR,  RL,  FC,  LFE,  FR,  FL } },
>>>>> +
>>>>> +{ .ca_index = 0x03,  .speakers = {   0,    0,   0,   0,  FC,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x04,  .speakers = {   0,    0,   0,  RC,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x05,  .speakers = {   0,    0,   0,  RC,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x06,  .speakers = {   0,    0,   0,  RC,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x07,  .speakers = {   0,    0,   0,  RC,  FC,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x0c,  .speakers = {   0,   RC,  RR,  RL,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x0d,  .speakers = {   0,   RC,  RR,  RL,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x0e,  .speakers = {   0,   RC,  RR,  RL,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x10,  .speakers = { RRC,  RLC,  RR,  RL,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x11,  .speakers = { RRC,  RLC,  RR,  RL,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x12,  .speakers = { RRC,  RLC,  RR,  RL,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x14,  .speakers = { FRC,  FLC,   0,   0,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x15,  .speakers = { FRC,  FLC,   0,   0,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x16,  .speakers = { FRC,  FLC,   0,   0,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x17,  .speakers = { FRC,  FLC,   0,   0,  FC,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x18,  .speakers = { FRC,  FLC,   0,  RC,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x19,  .speakers = { FRC,  FLC,   0,  RC,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x1a,  .speakers = { FRC,  FLC,   0,  RC,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x1b,  .speakers = { FRC,  FLC,   0,  RC,  FC,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x1c,  .speakers = { FRC,  FLC,  RR,  RL,   0,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x1d,  .speakers = { FRC,  FLC,  RR,  RL,   0,  LFE,  FR,  FL } },
>>>>> +{ .ca_index = 0x1e,  .speakers = { FRC,  FLC,  RR,  RL,  FC,    0,  FR,  FL } },
>>>>> +{ .ca_index = 0x1f,  .speakers = { FRC,  FLC,  RR,  RL,  FC,  LFE,  FR,  FL } },
>>>>> +};
>>>>
>>>> Ditto.
>>> Sorry not understand this comment vs the previous one, could you please
>>> clarify?
>>
>> This table is invariant in lifetime of the storage object, as well.
>> Let's put into .rodata section, too.
>>
> This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
> constant. In theory i could declare all field instead of computing some.
> But for lisibility, i would prefer to just declare ca_index  and
> speakers allocation field in this table (i will declared both as const)

You should pay enough attention to a case that one system has several 
GPUs to which relevant GPU drivers register HDMI_CODEC_DRV_NAME platform 
device. The 'static' modifier has an effect to keep just one storage 
object, thus your code causes bugs in the case.

Regards

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel