Re: [Xen-devel] [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

On Tue, Jan 27, 2015 at 8:00 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 27/01/15 17:52, Oleksandr Tyshchenko wrote:
>> On Tue, Jan 27, 2015 at 7:09 PM, Julien Grall <julien.grall@xxxxxxxxxx> 
>> wrote:
>>> Hi Oleksandr,
>> Hi Julien
>>
>>>
>>> On 27/01/15 13:39, Oleksandr Tyshchenko wrote:
>>>> -/* Set up the timer on the boot CPU */
>>>> -int __init init_xen_time(void)
>>>> +static const struct dt_device_match timer_ids[] __initconst =
>>>> +{
>>>> +     DT_MATCH_TIMER,
>>>> +     { /* sentinel */ },
>>>> +};
>>>> +
>>>> +/* Set up the timer on the boot CPU (early init function) */
>>>> +void __init preinit_xen_time(void)
>>>>  {
>>>> -    static const struct dt_device_match timer_ids[] __initconst =
>>>> -    {
>>>> -        DT_MATCH_TIMER,
>>>> -        { /* sentinel */ },
>>>> -    };
>>>
>>> I guess this is a left-over from the previous version?
>>> I would keep the definition of the variable here.
>> Sorry, I am not sure that I fully understand you.
>>
>> in v1 I created separate find_timer_node() a move to it timer_ids,
>> where the reason is to not duplicate it in init_xen_time() and
>> preinit_xen_time().
>>
>> +static __init struct dt_device_node *find_timer_node(void)
>> +{
>> +       static const struct dt_device_match timer_ids[] __initconst =
>> +       {
>> +           DT_MATCH_TIMER,
>> +           { /* sentinel */ },
>> +       };
>> +
>> +       return dt_find_matching_node(NULL, timer_ids);
>> +}
>>
>> in v2 taking into account your comment about "static" I decided to not
>> introduce additional func whose purpose is just to return pointer and
>> drop it.
>
> I think you misunderstood my comment on the V1.
You are right.

> You don't need to look twice for the device node. You can store it outside 
> the function.
>
> What I asked was something like:
>
> static __initdata struct dt_device_node *timer;
>
> void __init preinit_xen_time(void)
> {
>         static const struct dt_device_match timer_ids[] ... =
>                 ...
>
>         timer = dt_find_matching_node(NULL, timer_ids);
>         if (!timer)
>                 panic("...");
>
> }
>
> init __init init_xen_time(void)
> {
>
>         /* Use timer without checking */
> }
Yes, it is really better.


>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel