[Pdbg] [PATCH 4/5] libpdbg: Rework dt_new_node()

[Amitay Isaacs]

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 at 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 at 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.
-- 

There are two infinite things. The universe and stupidity of men, but I'm
not sure with the universe.  - Albert Einstein