[RFC,07/12] libpdbg: Rework dt_new_node()
diff mbox series

Message ID 20190806013723.4047-8-alistair@popple.id.au
State New
Headers show
Series
  • Split backends from system description
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-multiarch success Test build-multiarch on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (d0a0d919cccbd69631aaef0d37f1dba8d53e86e0)

Commit Message

Alistair Popple Aug. 6, 2019, 1:37 a.m. UTC
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(-)

Comments

Amitay Isaacs Aug. 20, 2019, 4 a.m. UTC | #1
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.
Alistair Popple Aug. 20, 2019, 5:04 a.m. UTC | #2
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.
>

Patch
diff mbox series

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;
 }