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

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

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".

> 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()).

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com