> -----Original Message----- > From: K, Mythri P > Sent: Thursday, June 23, 2011 11:21 AM > To: Premi, Sanjeev > Cc: linux-omap@xxxxxxxxxxxxxxx; Valkeinen, Tomi > Subject: Re: [PATCH 1/8] OMAP4: DSS: HDMI: HDMI clean up to > pass base_address > > Hi Sanjeev, > > On Mon, Jun 20, 2011 at 7:03 PM, Premi, Sanjeev <premi@xxxxxx> wrote: > >> -----Original Message----- > >> From: linux-omap-owner@xxxxxxxxxxxxxxx > >> [mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of K, Mythri P > >> Sent: Friday, June 17, 2011 1:47 PM > >> To: linux-omap@xxxxxxxxxxxxxxx; Valkeinen, Tomi > >> Cc: K, Mythri P > >> Subject: [PATCH 1/8] OMAP4: DSS: HDMI: HDMI clean up to pass > >> base_address > > > > [sp] HDMI appears twice on the subject. One of these can be removed. > > Also, it isn't clear (though implied) whose base > address is being > > passed. > > > > 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... > >> > >> - 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. > > [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 -- 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
