[4/5] libpdbg: Rework dt_new_node()
diff mbox series

Message ID 20190820071906.25950-4-alistair@popple.id.au
State Accepted
Headers show
Series
  • [1/5] libpdbg: Rename adu class to mem
Related show

Checks

Context Check Description
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch

Commit Message

Alistair Popple Aug. 20, 2019, 7:19 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, 7:55 a.m. UTC | #1
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.
Alistair Popple Aug. 21, 2019, 5:53 a.m. UTC | #2
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.
>

Patch
diff mbox series

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