Re: [PATCH v3 1/3] omap: opp: add OMAP3 OPP table data and common init

[Nishanth Menon <nm@ti.com>]

Thomas Petazzoni wrote, on 11/16/2010 06:42 AM:
> Hello,
>
> On Tue, 16 Nov 2010 05:54:50 -0600
> Nishanth Menon<nm@ti.com>  wrote:
>
>>> Do we really need this ? I personaly don't really like this quite of
>>> "Hey, I'm already initialized, let's do nothing silently then". Unless
>>> there are strong reasons for which this function could be called twice,
>>> I'd rather not have this, or turn this into a BUG_ON(omap_table_init ==
>>> 1).
>> Yes, it is needed. The intent here is different. See the documentation
>> that I put along with this patch - At times, board files may need to do
>> customization to opps - like enable 1GHz on that platform alone ->  it
>> can do it *only if* the defaults are registered, following which it can
>> call opp_enable. when device_initcall follows this at a later point, it
>> is still valid.
>
> Ah, right, I didn't catch that omapX_init_opp() could be called first
> from the board init function, and then later on as a device_initcall()
> callback.
>
> But, sorry, I find this even uglier than I thought it was :) What about
> adding the obligation to boards file to call the omapX_init_opp()
> function and then do their customization (if needed), then no call to
> omapX_init_opp() would be needed as a device_initcall() callback. Or,
> add a mechanism that allows the board file to register its
> customizations, that are later taken into account by the
> omapX_init_opp() function.
>
> Maybe it's just a matter of personal taste, but I really don't like
> this kind of "let's call this function once, do some stuff, then call
> it again, since it'll know that it shouldn't do anything".,

I feel you may have misunderstood the code, we DONOT oblige all boards 
to *have* to call omapX_init_opp. It is a device_initcall - so for the 
boards that dont call it, device_initcall will trigger and initialzie 
it. the hooks for the customization of the OPPs is in OPP layer itself.

the need we satisfy is this: if you need to support two sets of boards:
a) boards that are happy with the defaults - most of the boards - dont 
do anything in the board file. (device_init_call with auto register the 
defaults)
b) boards that need customization - these guys need to call 
omapX_init_opp(to register the defaults) before customizing the defaults.

Does this explain the code and reason for the logic? if you do have a 
better mechanism, lets know.

>
>> btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok
>> fine, system can still survive without cpufreq, no need to stop system
>> operations because of that.
>
> I don't see why replacing:
>
> +	if (omap_table_init)
> +		return 0;
> +	omap_table_init = 1;
>
> by
>
> 	BUG_ON(omap_table_init == 1);
> 	omap_table_init = 1;
>
> would prevent you from having no OPP table (the case where a NULL OPP
> table is passed is tested *before* in omapX_init_opp()).
HUH?? NULL table to a static function - what code are you talking 
about?? why are you so behind BUG_ON, when there are valid reasons for 
reentry into code.

-- 
Regards,
Nishanth Menon