Hi Sanjeev, <snip> >> > >> > The changes in the patch aren't limited to DSS alone. >> > See updated to hdmi_core_audio_config(), >> hdmi_wp_audio_config_format(), >> > hdmi_wp_audio_config_dma(), etc. >> > >> The changes are related to the HDMI driver which as of this patch is >> in DSS. You see any concerns ? It is only later that those functions >> are moved out . > > [sp] The subject of mail indicates that changes are meant for DSS. > How are the functions I quoted related to DSS? Unless DSS on OMAP4 > supports audio... > Some of the Audio configuration for HDMI is part of DSS (as of 2.6.40) , This patch series is precisely to separate the DSS portion from non-DSS portion of HDMI. It would be great if you have a look at the 2.6.40 kernel and this gitorious tree with my patch set to get a fair idea. >> >> >> >> - r = hdmi_read_reg(PLLCTRL_CFG1); >> >> + r = hdmi_read_reg(hdmi_pll_base(ip_data), PLLCTRL_CFG1); >> >> r = FLD_MOD(r, fmt->regm, 20, 9); /* CFG1_PLL_REGM */ >> >> r = FLD_MOD(r, fmt->regn, 8, 1); /* CFG1_PLL_REGN */ >> >> >> >> - hdmi_write_reg(PLLCTRL_CFG1, r); >> >> + hdmi_write_reg(hdmi_pll_base(ip_data), PLLCTRL_CFG1, r); >> > >> > [sp] hdmi_pll_base(ip_data) is used at many places withing this >> > function. Have you checked if saving the result in a local >> > variable generates any better code than repetitive ADD operation >> > in the function. >> > >> > Same comment applies to similar use in other functions >> > modified in this patch. >> > >> I had this comment from Tomi as well , Yes in some places it makes >> sense to have a local variable to save , but then in certain functions >> where there are four writes with 3 different base address it doesn't >> make sense to add 3 variables. > > [sp] I don't see base address for the IP changing within the function. > so a local variable still makes sense - instead of using CPU cycles > to do reduntant calculations each time. I shall make those changes where applicable. > >> > [snip]...[snip] >> >> + , >> >> HDMI_CORE_DDC_CMD, 0x9, 3, 0); >> > >> > [sp] Misplaced "," at beginning of the line. >> > >> Not a part of the patch i suppose? > > [sp] Actually the + indicates it is very much part of the patch. > Ideally it should be at end of prev line. > [quote] > /* Clear FIFO */ > - REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0x9, 3, 0); > + REG_FLD_MOD(hdmi_core_sys_base(ip_data) > + , HDMI_CORE_DDC_CMD, 0x9, 3, 0); > [/quote] > > ~sanjeev > -- Thanks and regards, Mythri. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
