On Thu, 2011-06-23 at 17:35 +0530, K, Mythri P wrote: > Hi Tomi, > > On Thu, Jun 23, 2011 at 3:28 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: > > On Fri, 2011-06-17 at 13:47 +0530, Mythri P K wrote: > >> HDMI IP block is common between TI OMAP4 Procerssor and Netra processor although > >> the Display subsytem is different. Thus to reuse the code between these two > >> processors , HDMI IP dependant code is seperated out from hdmi.c and moved to > >> new library file hdmi_ti_4xxx_ip.c which now resides in /drivers/video a more > >> generic location out of omap2/dss folder. > >> > >> This patch series does the split and also renames hdmi_omap4_panel.c to > >> hdmi_panel.c as that file has nothing specific to OMAP4 and can be reused for > >> other OMAP family of processors as well. > > > > These comments are based on the branch you have, not to particular > > patches: > > > > include/video/hdmi_ti_4xxx_ip.h: > > > > - This file is in a way the most important one, because it's the public > > API of the component. So you should be extra careful with this, and see > > that everything in it makes sense, and it's clear how it is to be used. > > > > - hdmi_core_hdmi_dvi is defined but not used. Should it be in the > > private header? > > HDMI_HDMI and HDMI_DVI are used everywhere in hdmi.c and hdmi_ti_4xxx_ip.c, > one thing i could do is to add a patch to used this enumerator > instead of int mode ? Oh, so the "int mode" in the header is actually "enum hdmi_core_hdmi_dvi mode"? Using int is obviously wrong, then. > > - hdmi_pll_pwr seems to be very low level thing. It requires the user to > > know HW details about the PLL, I don't think it should be visible to the > > users. It is used in parameters for hdmi_ti_4xxx_set_pll_pwr(). When is > > it supposed to be used? Why is hdmi_ti_4xxx_wp_video_start() not enough? > > > HDMI has core , PLL and PHY blocks, so user of the ip driver should > configure them separately. Why? Is there some requirement/benefit to it? > > - The same goes to hdmi_ti_4xxx_pll_program. Who does the PLL > > calculations, omapdss driver? Shouldn't the HDMI driver do it, the PLL > > is a HDMI HW thing, not DSS HW? Or if they are defined in the board > > file, and omapdss just gives them to the HDMI driver, I think the data > > should be somehow passed through omapdss without omapdss actively > > participating in the PLL programming. This would be easy if the HDMI > > would use a platform device/driver model, the data could be passed via > > the device data. Can you comment on this? Why the PLL calculation is not in the HDMI IP driver? > > - If the PLL calculations have to be done by omapdss, couldn't the > > hdmi_pll_info be part of hdmi_config? > > hdmi_pll_info would be used only once while configuring (power on) , > Why would we have to store the pll_info then in hdmi_config? Whether you store the hdmi_pll_info or not is HDMI IP driver internal thing, not related to the API. We're talking about the API here. Look at the include/video/hdmi_ti_4xxx_ip.h file and think from the users view point: what is the most obvious and easiest way to use the API. Then make the HDMI IP driver work with the API. > > - Is hdmi_ti_4xxx_phy_off the counterpart of hdmi_ti_4xxx_phy_init? If > > so, the naming could be better to clearly show this. Like init/uninit, > > enable/disable, etc. > > I shall rename. yes phy_off is counterpart of init , i shall renmane > it to enable ,disable. > > > > - Is the "phy" relevant (from the user's point of view) in > > hdmi_ti_4xxx_phy_init and off? Or could they be just init and uninit? > > PHY is a seperate block in HDMI and even from the user point of view > user should be aware of PHY, PLL and core blocks. so phy_init is > relevant. Again, why? Why cannot the user just give the configurations to the HDMI IP driver, and start it, without knowing anything about PHY/PLL/core. > > - Perhaps hdmi_ti_4xxx_basic_configure could be just configure, there's > > no advanced configure or similar. > Well, makes sense but this is the init_configuration(default > configuration) is what was intended in the patch. > > > > - is the "wp" important in hdmi_ti_4xxx_wp_video_start? > > > yes wp == wrapper , so it would be clear to have a wp_video_Start. Why does the user need to know wp == wrapper? Isn't the function just about enabling the video output? > > - read_ti_4xxx_edid() is named differently than the other functions. > > > I shall rename to hdmi_ti_4xxx_read_edid. > > > drivers/video/hdmi_ti_4xxx_ip.c: > > > > - EXPORT_SYMBOL(hdmi_ti_4xxx_phy_init); is not below the function. > > > > drivers/video/omap2/dss/hdmi.c: > > > > - hdmi_ti_4xxx_set_pll_pwr() is only called in power_off(), but never > > enabled. I presume the HDMI driver does that internally. That doesn't > > sound like a valid use of the function. > > It is enabled in hdmi_ti_4xxx_pll_program called from power on. Yes, I can see that. But it's bad usage pattern. Never enable something in one component, and disable it in another. The same entity has to do them both. > > - Looking at hdmi_power_on(), I feel that the API of the HDMI driver > > could be simpler. Would it be enough to have just one function to setup > > the HDMI, and another to turn the output on? > No , I suppose it is the responsibility of the user of IP driver to > configure PHY , PLL and CORE blocks seperately as we can reuse the PLL > or PHY block in different SOC's as well. Well, in that case, shouldn't PHY, PLL and CORE be in separate drivers? If they are all in one HDMI IP driver, I don't see why the user would need to know about PHY/PLL/CORE at all. The user just wants to use the HDMI IP as a whole, doesn't it? > > - omapdss_hdmi_display_set_timing() seems to be broken. It just sets > > some variables and calls display_enable(). > It sets a variable custom_set , appropriate action will be taken while > configuring timings based on that parameter. This is the same thing as with the use of hdmi_ti_4xxx_set_pll_pwr. Well, actually not, this is worse. A "set_timing" function which unconditionally calls omapdss_hdmi_display_enable() makes no sense to me. Why would set_timing function enable the display? And even more importantly, omapdss_hdmi_display_enable() won't work correctly if it is called unevenly related to omapdss_hdmi_display_disable(), which will happen here. Tomi -- 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
