Hi Tomi, On Thu, Jun 23, 2011 at 6:01 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: > 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. sure i shall fix it in a separate patch. > >> > - 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? Yes , When the core block is replaced in future OMAP's same PLL and PHY blocks are used. > >> > - 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? The calculation of the PLL would depend on the sysclk freq right ? but only the TMDS is the concern of HDMI IP block , so i would configure only the TMDS portion in HDMI IP and calculation in DSS. > >> > - 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. As the pll_info in needed only for config of the PLL block it wouldnt make sense to store it.[even from API point of view]. > >> > - 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. > Please see the comment above. The PHY , PLL and core blocks are interchangeable. >> > - 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? Well it is just to distinguish , yes the user doesn't have to know. > >> > - 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? Please see above comment, we could still use the same PLL block in future OMAP's only replacing the core block so is the case with PHY.But then it doesnt make sense to split the driver for every 2/3 functions right ? User could still access the PLL and PHY from this IP where as use the new API's for core? > >> > - 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. > There is a disable and enable called , This is on the same lines as other panels, I shall re check and post different patch in case of any correction, as i don't see it as a part of this patch series. > Tomi > > > -- 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
