[Pdbg] [RFC 07/12] libpdbg: Rework dt_new_node()

[Amitay Isaacs]

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

To get irritated is the same as accepting the stupidness of others
upon yourself.