| Message ID | 20190820071906.25950-4-alistair@popple.id.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/5] libpdbg: Rename adu class to mem | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch master (e005e0863a44f04c931b09804f258fb1c0b6f14c) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On Tue, 2019-08-20 at 17:19 +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 2fcb184..c4cbc44 100644 > --- a/libpdbg/device.c > +++ b/libpdbg/device.c > @@ -64,60 +64,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) Minor nitpick. Even though we are creating new pdbg_target here, it's in the context of dt. May be dt_new_pdbg_target() instead of pdbg_target_new()? Apart from that Reviewed-by: Amitay Isaacs <amitay@ozlabs.org> > { > + 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.
On Tuesday, 20 August 2019 5:55:56 PM AEST Amitay Isaacs wrote: > On Tue, 2019-08-20 at 17:19 +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 2fcb184..c4cbc44 100644 > > --- a/libpdbg/device.c > > +++ b/libpdbg/device.c > > @@ -64,60 +64,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) > > Minor nitpick. Even though we are creating new pdbg_target here, it's > in the context of dt. May be dt_new_pdbg_target() instead of > pdbg_target_new()? Sure. Will update and merge with your reviewed by. Thanks! > Apart from that Reviewed-by: Amitay Isaacs <amitay@ozlabs.org> > > > { > > + 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 2fcb184..c4cbc44 100644 --- a/libpdbg/device.c +++ b/libpdbg/device.c @@ -64,60 +64,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(-)