[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.