Re: [PATCH 6/8] [POWERPC] sysdev,qe_lib: implement FSL GTM support

[Scott Wood <scottwood@freescale.com>]

Anton Vorontsov wrote:
> On Tue, Mar 18, 2008 at 02:55:14PM -0500, Scott Wood wrote:
>> Anton Vorontsov wrote:
>>>>> +arch_initcall(qe_init_gtm);
>>>> If this happens before the gtm_init_gtm(),
>>> "If" isn't possible, order is guaranteed.
>> You use arch_initcall for both, so you're relying on link order.  I 
>> think this at least merits a comment.
>>>> then np->data will not be set. 
>>> It's a bug in the device tree or in the Linux code then.
>> Hmm?  It's set by gtm_init_gtm().  If this code runs before 
>> gtm_init_gtm(), what are you expecting to initialize np->data?
> 
> What code exactly?

Sorry, "this code" == qe_init_gtm().  Obviously, if you assume that 
gtm_init_gtm() will always be linked earlier, then it's not an issue.

>> Also, what if some arch_initcall runs between gtm_init_gtm and 
>> qe_init_gtm, that registers itself as a client of the gtm driver, and 
>> uses the wrong clock value?
> 
> Again, what code exactly?   If it is a driver (for what this API is
> created for), it hardly will run earlier than arch/ code.   If this is
> platform code (arch/powerpc/platform/), then it is hardly will run
> earlier than arch/sysdev/. Inside the arch/sysdev/ fsl_gtm.c is
> guaranteed to run earlier than qe_lib/gtm.c. So, where is the problem?

That's a lot of implicit, undocumented dependency on link order... 
Things can be moved around, and driver-ish code can pop up in surprising 
places.  All I meant was that having the gtm driver present itself as 
ready when it isn't, in a way which isn't readily apparent if it 
happens, is worrysome.

> Since I'll implement clock-frequency inside the timer node, this
> isn't relevant anymore...

OK, good.

> Ah. You need specific timer. No problem. I don't like idea of new arguments
> to the gtm_get_timer() function (complicates things), but we can just
> implement another one. gtm_get_timer_<name>, choice the name please.
> _specific, _2, _for, __gtm_get_timer, ...

How about:

struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm, int timer,
                                          int width);

...with np->data used by the caller to figure out which gtm pointer to 
pass in.

-Scott