Re: [PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Ricardo Neri <ricardo.neri@ti.com>]

On 04/24/2012 10:37 PM, Ricardo Neri wrote:
> On 04/23/2012 08:12 AM, Tomi Valkeinen wrote:
>> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>>> In order to avoid duplication of definitions. Use the definitions
>>> provided
>>> by asoundef.h. Furthermore, as CEA-861 and IEC-60958 are used by both
>>> DisplayPort and HDMI, this helps to make the code more generic.
>>
>> The first two sentences should probably be just one sentence.
> Actually, I just rephrased the whole description :). I will submit it in
> v2 of the patch series.
>>
>>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>>> ---
>>> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 15 +++---
>>> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h | 80
>>> ++---------------------------
>>> 2 files changed, 12 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> index 4740e64..4ab3b19 100644
>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> @@ -29,7 +29,6 @@
>>> #include<linux/string.h>
>>> #include<linux/seq_file.h>
>>> #include<linux/gpio.h>
>>> -
>>
>> Unrelated change.
> I will remove it.
>>
>>> #include "ti_hdmi_4xxx_ip.h"
>>> #include "dss.h"
>>>
>>> @@ -1156,7 +1155,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data
>>> *ip_data,
>>> }
>>>
>>> void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
>>> - struct hdmi_core_infoframe_audio *info_aud)
>>> + struct snd_cea_861_aud_if *info_aud)
>>> {
>>> u8 val;
>>> u8 sum = 0, checksum = 0;
>>> @@ -1172,22 +1171,24 @@ void hdmi_core_audio_infoframe_config(struct
>>> hdmi_ip_data *ip_data,
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUDIO_LEN, 0x0a);
>>> sum += 0x84 + 0x001 + 0x00a;
>>>
>>> - val = (info_aud->db1_coding_type<< 4)
>>> - | (info_aud->db1_channel_count - 1);
>>> + val = (info_aud->coding_type<< CEA861_AUDIO_INFOFRAME_DB1CT_SHIFT)
>>> + | (info_aud->channel_count - 1);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(0), val);
>>> sum += val;
>>>
>>> - val = (info_aud->db2_sample_freq<< 2) | info_aud->db2_sample_size;
>>> + val = (info_aud->sample_freq<< CEA861_AUDIO_INFOFRAME_DB2SF_SHIFT)
>>> + | (info_aud->sample_size);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(1), val);
>>> sum += val;
>>>
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), 0x00);
>>>
>>> - val = info_aud->db4_channel_alloc;
>>> + val = info_aud->channel_alloc;
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), val);
>>> sum += val;
>>>
>>> - val = (info_aud->db5_downmix_inh<< 7) | (info_aud->db5_lsv<< 3);
>>> + val = (info_aud->st_downmix<< CEA861_AUDIO_INFOFRAME_DB5_DM_INH_SHIFT)
>>> + | (info_aud->lsv<< CEA861_AUDIO_INFOFRAME_DB5_LSV_SHIFT);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(4), val);
>>> sum += val;
>>>
>>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> index a442998..d6b49b6 100644
>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> @@ -28,6 +28,8 @@
>>> defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>>> #include<sound/soc.h>
>>> #include<sound/pcm_params.h>
>>> +#include<sound/asound.h>
>>> +#include<sound/asoundef.h>
>>
>> You don't need to include these in the header file.
> I will remove it.
>>
>>> #endif
>>>
>>> /* HDMI Wrapper */
>>> @@ -284,35 +286,6 @@ enum hdmi_core_infoframe {
>>> HDMI_INFOFRAME_AVI_DB5PR_8 = 7,
>>> HDMI_INFOFRAME_AVI_DB5PR_9 = 8,
>>> HDMI_INFOFRAME_AVI_DB5PR_10 = 9,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_IEC60958 = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_AC3 = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MPEG1 = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MP3 = 4,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MPEG2_MULTICH = 5,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_AAC = 6,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DTS = 7,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_ATRAC = 8,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_ONEBIT = 9,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DOLBY_DIGITAL_PLUS = 10,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DTS_HD = 11,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MAT = 12,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DST = 13,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_WMA_PRO = 14,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_32000 = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_44100 = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_48000 = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_88200 = 4,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_96000 = 5,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_176400 = 6,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_192000 = 7,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_16BIT = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_20BIT = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_24BIT = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PERMITTED = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PROHIBITED = 1
>>> };
>>>
>>> enum hdmi_packing_mode {
>>> @@ -322,17 +295,6 @@ enum hdmi_packing_mode {
>>> HDMI_PACK_ALREADYPACKED = 7
>>> };
>>>
>>> -enum hdmi_core_audio_sample_freq {
>>> - HDMI_AUDIO_FS_32000 = 0x3,
>>> - HDMI_AUDIO_FS_44100 = 0x0,
>>> - HDMI_AUDIO_FS_48000 = 0x2,
>>> - HDMI_AUDIO_FS_88200 = 0x8,
>>> - HDMI_AUDIO_FS_96000 = 0xA,
>>> - HDMI_AUDIO_FS_176400 = 0xC,
>>> - HDMI_AUDIO_FS_192000 = 0xE,
>>> - HDMI_AUDIO_FS_NOT_INDICATED = 0x1
>>> -};
>>> -
>>> enum hdmi_core_audio_layout {
>>> HDMI_AUDIO_LAYOUT_2CH = 0,
>>> HDMI_AUDIO_LAYOUT_8CH = 1
>>> @@ -393,31 +355,10 @@ enum hdmi_audio_i2s_config {
>>
>> Are the defines left in the hdmi_audio_i2s_config something that are IP
>> specific? Are they even used? I'm just wondering why many of the defines
>> are in sound headers, but some are left here.
> Some are specific to the OMAP4 HDMI IP, such as HDMI_AUDIO_I2S_SDx_EN.
> Some others refer to generic I2S concepts (such as
> I2S_SCK_EDGE_FALLING/RISING) but defines are used to configure registers
> and such configuration may be different in other hardware. The defines
> that this patch removes are values that are effectively transmitted to
> the sink and are clearly defined in the relevant standards. Anyways, I
> will look at it further to see if some of them can be removed as well.
> Also, the I2S is the same for most of the supported use-cases, if not
> all of them. Maybe I can remove the unused ones.

I took at a second look at the hdmi_audio_i2s_config. As they are used 
to set IP-specific registers, I think they should be kept. Regarding the 
unused defines, they are not too many, do not do harm and let you know 
what other config values are available. I would like to keep them. What 
do you think?

BR,

Ricardo
>>
>>> HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
>>> HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
>>> HDMI_AUDIO_I2S_MAX_WORD_24BITS = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_NOT_SPECIFIED = 0,
>>> - HDMI_AUDIO_I2S_CHST_WORD_16_BITS = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_17_BITS = 6,
>>> - HDMI_AUDIO_I2S_CHST_WORD_18_BITS = 2,
>>> - HDMI_AUDIO_I2S_CHST_WORD_19_BITS = 4,
>>> - HDMI_AUDIO_I2S_CHST_WORD_20_BITS_20MAX = 5,
>>> - HDMI_AUDIO_I2S_CHST_WORD_20_BITS_24MAX = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_21_BITS = 6,
>>> - HDMI_AUDIO_I2S_CHST_WORD_22_BITS = 2,
>>> - HDMI_AUDIO_I2S_CHST_WORD_23_BITS = 4,
>>> - HDMI_AUDIO_I2S_CHST_WORD_24_BITS = 5,
>>> HDMI_AUDIO_I2S_SCK_EDGE_FALLING = 0,
>>> HDMI_AUDIO_I2S_SCK_EDGE_RISING = 1,
>>> HDMI_AUDIO_I2S_VBIT_FOR_PCM = 0,
>>> HDMI_AUDIO_I2S_VBIT_FOR_COMPRESSED = 1,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_NA = 0,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_16 = 2,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_17 = 12,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_18 = 4,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_19 = 8,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_20 = 10,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_21 = 13,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_22 = 5,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_23 = 9,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_24 = 11,
>>> HDMI_AUDIO_I2S_FIRST_BIT_SHIFT = 0,
>>> HDMI_AUDIO_I2S_FIRST_BIT_NO_SHIFT = 1,
>>> HDMI_AUDIO_I2S_SD0_EN = 1,
>>> @@ -486,19 +427,6 @@ struct hdmi_core_infoframe_avi {
>>> /* Pixel number start of right bar */
>>> u16 db12_13_pixel_sofright;
>>> };
>>> -/*
>>> - * Refer to section 8.2 in HDMI 1.3 specification for
>>> - * details about infoframe databytes
>>> - */
>>> -struct hdmi_core_infoframe_audio {
>>> - u8 db1_coding_type;
>>> - u8 db1_channel_count;
>>> - u8 db2_sample_freq;
>>> - u8 db2_sample_size;
>>> - u8 db4_channel_alloc;
>>> - bool db5_downmix_inh;
>>> - u8 db5_lsv; /* Level shift values for downmix */
>>> -};
>>>
>>> struct hdmi_core_packet_enable_repeat {
>>> u32 audio_pkt;
>>> @@ -559,7 +487,7 @@ struct hdmi_core_audio_i2s_config {
>>>
>>> struct hdmi_core_audio_config {
>>> struct hdmi_core_audio_i2s_config i2s_cfg;
>>> - enum hdmi_core_audio_sample_freq freq_sample;
>>> + u32 freq_sample;
>>
>> Try to keep consistent code formatting. If the original code indents the
>> field names, indent your field name also.
>
> I will fix the indentation.
>
> BR,
> Ricardo
>>
>> Tomi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html