Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

NIT: s/merge/consolidate/

On 31/07/2019 11:28, Viktor Mitin wrote:
> Merged make_timer_node and make_timer_domU_node into one function
> make_timer_node.
>
> Kept the domU version for the compatible as it is simpler.
> Kept the hw version for the clock as it is relevant for the both cases.
The commit message needs a bit of rewording:
 - It is not clear why they the two functions are merged
 - This needs more word around so the commit message looks like a coherent text.

> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> ---
> v4 updates:
>     updated "Kept the domU version for the compatible as it is simpler"
>
>   xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 70 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d04a1c3aec..4d7c3411a6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, 
> void *fdt,
>   
>   static int __init make_timer_node(const struct kernel_info *kinfo)
>   {
> +    int res;
>       void *fdt = kinfo->fdt;
> -
Please avoid to add code that you drop in a patch later.

> +    unsigned int irq[MAX_TIMER_PPI];
> +    gic_interrupt_t intrs[3];
> +    u32 clock_frequency;
> +    bool clock_valid;
This is not related to this patch and only increase the complexity of the 
review. If you want to do reshuffling then it should be a separate patch.
But then, I see you real value of the re-ordering here.

>       static const struct dt_device_match timer_ids[] __initconst =
>       {
>           DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> @@ -973,15 +977,6 @@ static int __init make_timer_node(const struct kernel_info 
> *kinfo)
>           { /* sentinel */ },
>       };
>       struct dt_device_node *dev;
> -    u32 len;
> -    const void *compatible;
> -    int res;
> -    unsigned int irq;
> -    gic_interrupt_t intrs[3];
> -    u32 clock_frequency;
> -    bool clock_valid;
> -
> -    dt_dprintk("Create timer node\n");
Why is it dropped?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel