| Message ID | 20190806013723.4047-8-alistair@popple.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Split backends from system description | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (d0a0d919cccbd69631aaef0d37f1dba8d53e86e0) |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
We definitely need a function to create a new node without assigning the driver (or initializing the hw_unit part of the structure). We also need to separate the assignment of hw drivers to a target. So in the first pass, we simply parse fdt and create device tree "node" without assigning any driver. And in the second pass of the device tree, we can assign the drivers. This will be helpful later when we want do dynamically insert common sub-trees. Question is would you like to do that as part of this series? On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > Simplify the control logic for dt_new_node() by splitting out the > initialisation from the HW unit. Should be no functional change. > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > --- > libpdbg/device.c | 89 +++++++++++++++++++++++++++++----------------- > -- > 1 file changed, 54 insertions(+), 35 deletions(-) > > diff --git a/libpdbg/device.c b/libpdbg/device.c > index 7a913b2..21ec3a3 100644 > --- a/libpdbg/device.c > +++ b/libpdbg/device.c > @@ -65,60 +65,79 @@ static const char *take_name(const char *name) > return name; > } > > -static struct pdbg_target *dt_new_node(const char *name, const void > *fdt, int node_offset) > +/* Adds information representing an actual target */ > +static struct pdbg_target *pdbg_target_new(const void *fdt, int > node_offset) > { > + struct pdbg_target *target; > + struct pdbg_target_class *target_class; > const struct hw_unit_info *hw_info = NULL; > const struct fdt_property *prop; > - struct pdbg_target *node; > - size_t size = sizeof(*node); > - > - if (fdt) { > - prop = fdt_get_property(fdt, node_offset, "compatible", > NULL); > - if (prop) { > - int i, prop_len = fdt32_to_cpu(prop->len); > - > - /* > - * If I understand correctly, the property we > have > - * here can be a stringlist with a few > compatible > - * strings > - */ > - i = 0; > - while (i < prop_len) { > - hw_info = > pdbg_hwunit_find_compatible(&prop->data[i]); > - if (hw_info) { > - size = hw_info->size; > - break; > - } > - > - i += strlen(&prop->data[i]) + 1; > + size_t size; > + > + prop = fdt_get_property(fdt, node_offset, "compatible", NULL); > + if (prop) { > + int i, prop_len = fdt32_to_cpu(prop->len); > + > + /* > + * If I understand correctly, the property we have > + * here can be a stringlist with a few compatible > + * strings > + */ > + i = 0; > + while (i < prop_len) { > + hw_info = pdbg_hwunit_find_compatible(&prop- > >data[i]); > + if (hw_info) { > + size = hw_info->size; > + break; > } > + > + i += strlen(&prop->data[i]) + 1; > } > } > > - node = calloc(1, size); > - if (!node) { > + if (!hw_info) > + /* Couldn't find anything implementing this target */ > + return NULL; > + > + target = calloc(1, size); > + if (!target) { > prerror("Failed to allocate node\n"); > abort(); > } > > - if (hw_info) { > - struct pdbg_target_class *target_class; > + /* hw_info->hw_unit points to a per-target struct type. This > + * works because the first member in the per-target struct is > + * guaranteed to be the struct pdbg_target (see the comment > + * above DECLARE_HW_UNIT). */ > + memcpy(target, hw_info->hw_unit, size); > + target_class = get_target_class(target->class); > + list_add_tail(&target_class->targets, &target->class_link); > + > + return target; > +} > > - /* hw_info->hw_unit points to a per-target struct type. > This > - * works because the first member in the per-target > struct is > - * guaranteed to be the struct pdbg_target (see the > comment > - * above DECLARE_HW_UNIT). */ > - memcpy(node, hw_info->hw_unit, size); > - target_class = get_target_class(node->class); > - list_add_tail(&target_class->targets, &node- > >class_link); > +static struct pdbg_target *dt_new_node(const char *name, const void > *fdt, int node_offset) > +{ > + struct pdbg_target *node = NULL; > + size_t size = sizeof(*node); > + > + if (fdt) > + node = pdbg_target_new(fdt, node_offset); > + > + if (!node) > + node = calloc(1, size); > + > + if (!node) { > + prerror("Failed to allocate node\n"); > + abort(); > } > > node->dn_name = take_name(name); > node->parent = NULL; > list_head_init(&node->properties); > list_head_init(&node->children); > - /* FIXME: locking? */ > node->phandle = ++last_phandle; > + > return node; > } > > -- > 2.20.1 > Amitay.
I think it makes the code a bit easier to follow anyway, so if you think it will be useful in future and agree that it doesn't make any functional changes then I am happy to take it as part of this series. - Alistair On Tuesday, 20 August 2019 2:00:51 PM AEST Amitay Isaacs wrote: > We definitely need a function to create a new node without assigning > the driver (or initializing the hw_unit part of the structure). > > We also need to separate the assignment of hw drivers to a target. So > in the first pass, we simply parse fdt and create device tree "node" > without assigning any driver. And in the second pass of the device > tree, we can assign the drivers. This will be helpful later when we > want do dynamically insert common sub-trees. > > Question is would you like to do that as part of this series? > > On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > > Simplify the control logic for dt_new_node() by splitting out the > > initialisation from the HW unit. Should be no functional change. > > > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > --- > > libpdbg/device.c | 89 +++++++++++++++++++++++++++++----------------- > > -- > > 1 file changed, 54 insertions(+), 35 deletions(-) > > > > diff --git a/libpdbg/device.c b/libpdbg/device.c > > index 7a913b2..21ec3a3 100644 > > --- a/libpdbg/device.c > > +++ b/libpdbg/device.c > > @@ -65,60 +65,79 @@ static const char *take_name(const char *name) > > return name; > > } > > > > -static struct pdbg_target *dt_new_node(const char *name, const void > > *fdt, int node_offset) > > +/* Adds information representing an actual target */ > > +static struct pdbg_target *pdbg_target_new(const void *fdt, int > > node_offset) > > { > > + struct pdbg_target *target; > > + struct pdbg_target_class *target_class; > > const struct hw_unit_info *hw_info = NULL; > > const struct fdt_property *prop; > > - struct pdbg_target *node; > > - size_t size = sizeof(*node); > > - > > - if (fdt) { > > - prop = fdt_get_property(fdt, node_offset, "compatible", > > NULL); > > - if (prop) { > > - int i, prop_len = fdt32_to_cpu(prop->len); > > - > > - /* > > - * If I understand correctly, the property we > > have > > - * here can be a stringlist with a few > > compatible > > - * strings > > - */ > > - i = 0; > > - while (i < prop_len) { > > - hw_info = > > pdbg_hwunit_find_compatible(&prop->data[i]); > > - if (hw_info) { > > - size = hw_info->size; > > - break; > > - } > > - > > - i += strlen(&prop->data[i]) + 1; > > + size_t size; > > + > > + prop = fdt_get_property(fdt, node_offset, "compatible", NULL); > > + if (prop) { > > + int i, prop_len = fdt32_to_cpu(prop->len); > > + > > + /* > > + * If I understand correctly, the property we have > > + * here can be a stringlist with a few compatible > > + * strings > > + */ > > + i = 0; > > + while (i < prop_len) { > > + hw_info = pdbg_hwunit_find_compatible(&prop- > > >data[i]); > > + if (hw_info) { > > + size = hw_info->size; > > + break; > > } > > + > > + i += strlen(&prop->data[i]) + 1; > > } > > } > > > > - node = calloc(1, size); > > - if (!node) { > > + if (!hw_info) > > + /* Couldn't find anything implementing this target */ > > + return NULL; > > + > > + target = calloc(1, size); > > + if (!target) { > > prerror("Failed to allocate node\n"); > > abort(); > > } > > > > - if (hw_info) { > > - struct pdbg_target_class *target_class; > > + /* hw_info->hw_unit points to a per-target struct type. This > > + * works because the first member in the per-target struct is > > + * guaranteed to be the struct pdbg_target (see the comment > > + * above DECLARE_HW_UNIT). */ > > + memcpy(target, hw_info->hw_unit, size); > > + target_class = get_target_class(target->class); > > + list_add_tail(&target_class->targets, &target->class_link); > > + > > + return target; > > +} > > > > - /* hw_info->hw_unit points to a per-target struct type. > > This > > - * works because the first member in the per-target > > struct is > > - * guaranteed to be the struct pdbg_target (see the > > comment > > - * above DECLARE_HW_UNIT). */ > > - memcpy(node, hw_info->hw_unit, size); > > - target_class = get_target_class(node->class); > > - list_add_tail(&target_class->targets, &node- > > >class_link); > > +static struct pdbg_target *dt_new_node(const char *name, const void > > *fdt, int node_offset) > > +{ > > + struct pdbg_target *node = NULL; > > + size_t size = sizeof(*node); > > + > > + if (fdt) > > + node = pdbg_target_new(fdt, node_offset); > > + > > + if (!node) > > + node = calloc(1, size); > > + > > + if (!node) { > > + prerror("Failed to allocate node\n"); > > + abort(); > > } > > > > node->dn_name = take_name(name); > > node->parent = NULL; > > list_head_init(&node->properties); > > list_head_init(&node->children); > > - /* FIXME: locking? */ > > node->phandle = ++last_phandle; > > + > > return node; > > } > > > > Amitay. >
diff --git a/libpdbg/device.c b/libpdbg/device.c index 7a913b2..21ec3a3 100644 --- a/libpdbg/device.c +++ b/libpdbg/device.c @@ -65,60 +65,79 @@ static const char *take_name(const char *name) return name; } -static struct pdbg_target *dt_new_node(const char *name, const void *fdt, int node_offset) +/* Adds information representing an actual target */ +static struct pdbg_target *pdbg_target_new(const void *fdt, int node_offset) { + struct pdbg_target *target; + struct pdbg_target_class *target_class; const struct hw_unit_info *hw_info = NULL; const struct fdt_property *prop; - struct pdbg_target *node; - size_t size = sizeof(*node); - - if (fdt) { - prop = fdt_get_property(fdt, node_offset, "compatible", NULL); - if (prop) { - int i, prop_len = fdt32_to_cpu(prop->len); - - /* - * If I understand correctly, the property we have - * here can be a stringlist with a few compatible - * strings - */ - i = 0; - while (i < prop_len) { - hw_info = pdbg_hwunit_find_compatible(&prop->data[i]); - if (hw_info) { - size = hw_info->size; - break; - } - - i += strlen(&prop->data[i]) + 1; + size_t size; + + prop = fdt_get_property(fdt, node_offset, "compatible", NULL); + if (prop) { + int i, prop_len = fdt32_to_cpu(prop->len); + + /* + * If I understand correctly, the property we have + * here can be a stringlist with a few compatible + * strings + */ + i = 0; + while (i < prop_len) { + hw_info = pdbg_hwunit_find_compatible(&prop->data[i]); + if (hw_info) { + size = hw_info->size; + break; } + + i += strlen(&prop->data[i]) + 1; } } - node = calloc(1, size); - if (!node) { + if (!hw_info) + /* Couldn't find anything implementing this target */ + return NULL; + + target = calloc(1, size); + if (!target) { prerror("Failed to allocate node\n"); abort(); } - if (hw_info) { - struct pdbg_target_class *target_class; + /* hw_info->hw_unit points to a per-target struct type. This + * works because the first member in the per-target struct is + * guaranteed to be the struct pdbg_target (see the comment + * above DECLARE_HW_UNIT). */ + memcpy(target, hw_info->hw_unit, size); + target_class = get_target_class(target->class); + list_add_tail(&target_class->targets, &target->class_link); + + return target; +} - /* hw_info->hw_unit points to a per-target struct type. This - * works because the first member in the per-target struct is - * guaranteed to be the struct pdbg_target (see the comment - * above DECLARE_HW_UNIT). */ - memcpy(node, hw_info->hw_unit, size); - target_class = get_target_class(node->class); - list_add_tail(&target_class->targets, &node->class_link); +static struct pdbg_target *dt_new_node(const char *name, const void *fdt, int node_offset) +{ + struct pdbg_target *node = NULL; + size_t size = sizeof(*node); + + if (fdt) + node = pdbg_target_new(fdt, node_offset); + + if (!node) + node = calloc(1, size); + + if (!node) { + prerror("Failed to allocate node\n"); + abort(); } node->dn_name = take_name(name); node->parent = NULL; list_head_init(&node->properties); list_head_init(&node->children); - /* FIXME: locking? */ node->phandle = ++last_phandle; + return node; }
Simplify the control logic for dt_new_node() by splitting out the initialisation from the HW unit. Should be no functional change. Signed-off-by: Alistair Popple <alistair@popple.id.au> --- libpdbg/device.c | 89 +++++++++++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 35 deletions(-)