On Thu, Aug 21, 2008 at 1:34 AM, Kanigeri, Hari <h-kanigeri2@xxxxxx> wrote: > Felipe, > >> We are trying to understand why this is not just a wrapper. > > -- Thank you :) > >> b) the clock init does the job of obtaining the references to the >> clock ids in its initialization >> >> Those could be initialized in some other place as well. >> > -- Why can't this some other place be the init function in the current clock module in Bridge ? That depends on what is left of clk.c after reorganization. >> How exactly is "clk_enable(BPWR_Clks[clkIdIndex].intClk)" less >> readable than "CLK_Enable(BPWR_Clks[clkIdIndex].intClk)" ? > > -- Did you check the argument of CLK_Enable function ? The argument is not the same as you mentioned above? One more reason to say that this is not a direct wrapper ;). Did you read what I wrote? >> Or if you store the clk handle directly into the BPWR_Clk_t structure: >> >> clk_enable(BPWR_Clks[clkIdIndex].intClk); >> clk_enable(BPWR_Clks[clkIdIndex].funClk); So .intClk wouldn't be "enum SERVICES_ClkId", but "struct clk". > But that's not my point when I mentioned about readability. By removing the code from clk.c and scattering it around will make it difficult to follow and manage the code. I think we can even move the DSPPeripheralClkCtrl function in tiomap_pwr.c to clk.c so that all the clock interfaces from DSP Bridge are in one file, and we can rename this file to bridge_clk_mgr to avoid the impression that clk module in Bridge is implementing its own clock framework or the functions are just acting as wrappers. I think this approach is clean and easy to manage. I see a placeholder for dspbridge clock module considering the number of clocks it has to manage. If you see clk_enable there's nothing more to follow, so it's not hard to follow. Others explained why virtual clocks make sense, which would make the code even easier to follow. The only two places where clock functions are used are tiomap3430.c and tiomap3430_pwr.c. In tiomap3430.c, clk_enable functions can be used directly, there's no translation needed, and I already explained before, you can add debugging quite easily, so no need of clk.c for that. In tiomap3430_pwr.c the translation can happen in DSPPeripheralClkCtrl already, so again, no need of clk.c. If you reorganize the code this way the only thing left in clk.c would be clock initialization. I don't think it makes sense to keep one file for one small function which would be even smaller with virtual clocks. Best regards. -- Felipe Contreras -- 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
