devicetree-compiler.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Add capability to create property from old property
@ 2025-03-01 13:25 Ayush Singh
  2025-03-01 13:25 ` [PATCH 1/3] Add alloc_marker Ayush Singh
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Ayush Singh @ 2025-03-01 13:25 UTC (permalink / raw)
  To: David Gibson, Andreas Gnau, d-gole, lorforlinux, jkridner,
	robertcnelson, Andrew Davis, Geert Uytterhoeven, Simon Glass
  Cc: devicetree-compiler, Ayush Singh

Allow construction of new values for a property using old property
value.

This was originally suggested in /append-property/ Patch series [0] and
can be used to achieve the same results (and much more).

In practice, it looks as follows:

dts-v1/;

/ {
	str-prop = "0";
	int-prop = <0>;
};

/ {
	str-prop = /./, "1", /./;
	int-prop = /./, <1>, /./;
};

Open Items:

- Integer array:

  Currently, the use with integer arrays looks as follows:

  /dts-v1/;
  
  / {
          int-prop = <0 1>;
  };
  
  / {
          int-prop = /./, <2 3>, /./;
  };

  This will produce the correct code, and I personally prefer the
  current syntax to ``</./ 1 /./>``.

- dts to source output can look somewhat weird

  The above dts produces the following with annoation:

  /dts-v1/;
  
  / { /* base.dts:3:3-5:3, base.dts:7:3-9:3 */
          int-prop = <0x00 0x01>, <0x02 0x03>, <0x00 0x01>; /* base.dts:4:9-4:26, base.dts:8:9-8:36 */
  }; /* base.dts:3:3-5:3, base.dts:7:3-9:3 */

  It isn't a real problem since it allows easily tracing that some parts
  were constructed at other places, but is differnt from how a normal
  array would appear.

[0]: https://lore.kernel.org/devicetree-compiler/Z24nldCpXpoT7RaK@zatzit/T/#t

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
Ayush Singh (3):
      Add alloc_marker
      dtc: Add /./
      tests: Add test for /./

 data.c              | 20 +++++++++++++++-----
 dtc-lexer.l         |  5 +++++
 dtc-parser.y        |  5 +++++
 dtc.h               |  3 +++
 livetree.c          | 44 ++++++++++++++++++++++++++++++++++++++++++--
 tests/prev_prop.dts | 21 +++++++++++++++++++++
 tests/run_tests.sh  |  7 +++++++
 7 files changed, 98 insertions(+), 7 deletions(-)
---
base-commit: ce1d8588880aecd7af264e422a16a8b33617cef7
change-id: 20241227-previous-value-c22d4eeaae72

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] Add alloc_marker
  2025-03-01 13:25 [PATCH 0/3] Add capability to create property from old property Ayush Singh
@ 2025-03-01 13:25 ` Ayush Singh
  2025-03-03  9:35   ` David Gibson
  2025-03-01 13:25 ` [PATCH 2/3] dtc: Add /./ Ayush Singh
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Ayush Singh @ 2025-03-01 13:25 UTC (permalink / raw)
  To: David Gibson, Andreas Gnau, d-gole, lorforlinux, jkridner,
	robertcnelson, Andrew Davis, Geert Uytterhoeven, Simon Glass
  Cc: devicetree-compiler, Ayush Singh

- Add helper to allocate new marker

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 data.c | 20 +++++++++++++++-----
 dtc.h  |  2 ++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/data.c b/data.c
index 14734233ad8b7ebd38c3e62442b81aae66601806..913796f2e664d07cdc48e0cbf2ab5d6fe9978072 100644
--- a/data.c
+++ b/data.c
@@ -228,11 +228,7 @@ struct data data_add_marker(struct data d, enum markertype type, char *ref)
 {
 	struct marker *m;
 
-	m = xmalloc(sizeof(*m));
-	m->offset = d.len;
-	m->type = type;
-	m->ref = ref;
-	m->next = NULL;
+	m = alloc_marker(d.len, type, ref);
 
 	return data_append_markers(d, m);
 }
@@ -254,3 +250,17 @@ bool data_is_one_string(struct data d)
 
 	return true;
 }
+
+struct marker *alloc_marker(unsigned int offset, enum markertype type,
+			    char *ref)
+{
+	struct marker *m;
+
+	m = xmalloc(sizeof(*m));
+	m->offset = offset;
+	m->type = type;
+	m->ref = ref;
+	m->next = NULL;
+
+	return m;
+}
diff --git a/dtc.h b/dtc.h
index 4c4aaca1fc417c9d93e904e64b2c40216ee1b093..86928e1eea9764fe5d74d6dbb987589d65d54b66 100644
--- a/dtc.h
+++ b/dtc.h
@@ -183,6 +183,8 @@ struct data data_append_byte(struct data d, uint8_t byte);
 struct data data_append_zeroes(struct data d, int len);
 struct data data_append_align(struct data d, int align);
 
+struct marker *alloc_marker(unsigned int offset, enum markertype type,
+			    char *ref);
 struct data data_add_marker(struct data d, enum markertype type, char *ref);
 
 bool data_is_one_string(struct data d);

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] dtc: Add /./
  2025-03-01 13:25 [PATCH 0/3] Add capability to create property from old property Ayush Singh
  2025-03-01 13:25 ` [PATCH 1/3] Add alloc_marker Ayush Singh
@ 2025-03-01 13:25 ` Ayush Singh
  2025-03-05  4:14   ` David Gibson
  2025-03-01 13:25 ` [PATCH 3/3] tests: Add test for /./ Ayush Singh
  2025-03-03  9:02 ` [PATCH 0/3] Add capability to create property from old property David Gibson
  3 siblings, 1 reply; 8+ messages in thread
From: Ayush Singh @ 2025-03-01 13:25 UTC (permalink / raw)
  To: David Gibson, Andreas Gnau, d-gole, lorforlinux, jkridner,
	robertcnelson, Andrew Davis, Geert Uytterhoeven, Simon Glass
  Cc: devicetree-compiler, Ayush Singh

Allow construction new values for a property using old property values.
Can be used to append, pre-append, duplicate, etc.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 dtc-lexer.l  |  5 +++++
 dtc-parser.y |  5 +++++
 dtc.h        |  1 +
 livetree.c   | 44 ++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index de60a70b6bdbcb5ae4336ea4171ad6f645e91b36..5efeca10363e0c9c338b6578be9240c3f42249f0 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -144,6 +144,11 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
 			return DT_OMIT_NO_REF;
 		}
 
+<*>"/./" {
+			DPRINT("Keyword: /./\n");
+			return DT_PREV_PROP;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
diff --git a/dtc-parser.y b/dtc-parser.y
index 4d5eece526243460203157464e3cd75f781e50e7..c34eb10a1068b5eb6a0f08e5a1db8066217b16bf 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -57,6 +57,7 @@ static bool is_ref_relative(const char *ref)
 %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
 %token DT_BITS
 %token DT_DEL_PROP
+%token DT_PREV_PROP
 %token DT_DEL_NODE
 %token DT_OMIT_NO_REF
 %token <propnodename> DT_PROPNODENAME
@@ -308,6 +309,10 @@ propdata:
 		{
 			$$ = data_merge($1, $2);
 		}
+        | propdataprefix DT_PREV_PROP
+                {
+                        $$ = data_add_marker($1, PREV_VALUE, NULL);
+                }
 	| propdataprefix arrayprefix '>'
 		{
 			$$ = data_merge($1, $2.data);
diff --git a/dtc.h b/dtc.h
index 86928e1eea9764fe5d74d6dbb987589d65d54b66..175fe3637a31cc28453dc27980f315a171763b49 100644
--- a/dtc.h
+++ b/dtc.h
@@ -110,6 +110,7 @@ enum markertype {
 	REF_PHANDLE,
 	REF_PATH,
 	LABEL,
+	PREV_VALUE,
 	TYPE_UINT8,
 	TYPE_UINT16,
 	TYPE_UINT32,
diff --git a/livetree.c b/livetree.c
index 93c77d95a320ec05aa355e12920cef9e1c91c26a..89e15c39e9eadb87cf8376fe167a4d9c6002031a 100644
--- a/livetree.c
+++ b/livetree.c
@@ -139,10 +139,34 @@ struct node *reference_node(struct node *node)
 	return node;
 }
 
+static struct data data_insert_old_value(struct data d, struct marker *m,
+					 const struct data *old)
+{
+	unsigned int offset = m->offset;
+	struct marker *next = m->next;
+	struct marker *marker = m;
+	struct data new_data;
+
+	new_data = data_insert_at_marker(d, marker, old->val, old->len);
+
+	/* Copy all markers from old value */
+	marker = old->markers;
+	for_each_marker(marker) {
+		m->next = alloc_marker(marker->offset + offset, marker->type,
+				       marker->ref);
+		m = m->next;
+	}
+	m->next = next;
+
+	return new_data;
+}
+
 struct node *merge_nodes(struct node *old_node, struct node *new_node)
 {
 	struct property *new_prop, *old_prop;
 	struct node *new_child, *old_child;
+	bool prev_value_used = false;
+	struct marker *marker;
 	struct label *l;
 
 	old_node->deleted = 0;
@@ -172,10 +196,26 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 				for_each_label_withdel(new_prop->labels, l)
 					add_label(&old_prop->labels, l->label);
 
+				marker = new_prop->val.markers;
+				for_each_marker_of_type(marker, PREV_VALUE) {
+					new_prop->val = data_insert_old_value(
+						new_prop->val, marker,
+						&old_prop->val);
+					prev_value_used = true;
+				}
+
 				old_prop->val = new_prop->val;
 				old_prop->deleted = 0;
-				free(old_prop->srcpos);
-				old_prop->srcpos = new_prop->srcpos;
+
+				if (prev_value_used) {
+					old_prop->srcpos =
+						srcpos_extend(old_prop->srcpos,
+							      new_prop->srcpos);
+				} else {
+					free(old_prop->srcpos);
+					old_prop->srcpos = new_prop->srcpos;
+				}
+
 				free(new_prop);
 				new_prop = NULL;
 				break;

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] tests: Add test for /./
  2025-03-01 13:25 [PATCH 0/3] Add capability to create property from old property Ayush Singh
  2025-03-01 13:25 ` [PATCH 1/3] Add alloc_marker Ayush Singh
  2025-03-01 13:25 ` [PATCH 2/3] dtc: Add /./ Ayush Singh
@ 2025-03-01 13:25 ` Ayush Singh
  2025-03-05  4:17   ` David Gibson
  2025-03-03  9:02 ` [PATCH 0/3] Add capability to create property from old property David Gibson
  3 siblings, 1 reply; 8+ messages in thread
From: Ayush Singh @ 2025-03-01 13:25 UTC (permalink / raw)
  To: David Gibson, Andreas Gnau, d-gole, lorforlinux, jkridner,
	robertcnelson, Andrew Davis, Geert Uytterhoeven, Simon Glass
  Cc: devicetree-compiler, Ayush Singh

- Test /./ on a string and int array.
- Also test on subnode property.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 tests/prev_prop.dts | 21 +++++++++++++++++++++
 tests/run_tests.sh  |  7 +++++++
 2 files changed, 28 insertions(+)

diff --git a/tests/prev_prop.dts b/tests/prev_prop.dts
new file mode 100644
index 0000000000000000000000000000000000000000..41a0e19015b006d78a3ec35a26db63982ec4c90a
--- /dev/null
+++ b/tests/prev_prop.dts
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/ {
+        str-prop = "1", "2";
+        int-prop = <1 2>;
+
+        subnode {
+                str-prop = "1", "2";
+                int-prop = <1 2>;
+        };
+};
+
+/ {
+        str-prop = /./, "3", "4", /./, "6";
+        int-prop = /./, <3 4>, /./, <6>;
+
+        subnode {
+                str-prop = /./, "3", "4", /./;
+                int-prop = /./, <3 4>, /./;
+        };
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index f0b51c04bf0af69f1df483b185f3aefa5d0bae27..30aa99f55570ac10b85c94353711361c19a5b479 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -708,6 +708,13 @@ dtc_tests () {
     run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
     tree1_tests dtc_tree1_delete.test.dtb
 
+    # Check previous property functionality
+    run_dtc_test -I dts -O dtb -o prev_prop.test.dtb "$SRCDIR/prev_prop.dts"
+    run_fdtget_test "1 2 3 4 1 2 6" prev_prop.test.dtb "/" "str-prop"
+    run_fdtget_test "1 2 3 4 1 2 6" prev_prop.test.dtb "/" "int-prop"
+    run_fdtget_test "1 2 3 4 1 2" prev_prop.test.dtb "/subnode" "str-prop"
+    run_fdtget_test "1 2 3 4 1 2" prev_prop.test.dtb "/subnode" "int-prop"
+
     # Check omit-if-no-ref functionality
     run_dtc_test -I dts -O dtb -o omit-no-ref.test.dtb "$SRCDIR/omit-no-ref.dts"
     run_test check_path omit-no-ref.test.dtb not-exists "/node1"

-- 
2.48.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] Add capability to create property from old property
  2025-03-01 13:25 [PATCH 0/3] Add capability to create property from old property Ayush Singh
                   ` (2 preceding siblings ...)
  2025-03-01 13:25 ` [PATCH 3/3] tests: Add test for /./ Ayush Singh
@ 2025-03-03  9:02 ` David Gibson
  3 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2025-03-03  9:02 UTC (permalink / raw)
  To: Ayush Singh
  Cc: Andreas Gnau, d-gole, lorforlinux, jkridner, robertcnelson,
	Andrew Davis, Geert Uytterhoeven, Simon Glass,
	devicetree-compiler

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

On Sat, Mar 01, 2025 at 06:55:01PM +0530, Ayush Singh wrote:
> Allow construction of new values for a property using old property
> value.
> 
> This was originally suggested in /append-property/ Patch series [0] and
> can be used to achieve the same results (and much more).
> 
> In practice, it looks as follows:
> 
> dts-v1/;
> 
> / {
> 	str-prop = "0";
> 	int-prop = <0>;
> };
> 
> / {
> 	str-prop = /./, "1", /./;
> 	int-prop = /./, <1>, /./;
> };
> 
> Open Items:
> 
> - Integer array:
> 
>   Currently, the use with integer arrays looks as follows:
> 
>   /dts-v1/;
>   
>   / {
>           int-prop = <0 1>;
>   };
>   
>   / {
>           int-prop = /./, <2 3>, /./;
>   };
> 
>   This will produce the correct code, and I personally prefer the
>   current syntax to ``</./ 1 /./>``.

You've made the correct choice.  We don't exactly keep track of types
in dtc, but we do some things close enough that you can kind of think
of things in terms of types.  </./ 1 /./> would make no sense from a
typing point of view, because you're putting a bytestring value /./ in
an operator that expects integers < ... >.

> - dts to source output can look somewhat weird
> 
>   The above dts produces the following with annoation:
> 
>   /dts-v1/;
>   
>   / { /* base.dts:3:3-5:3, base.dts:7:3-9:3 */
>           int-prop = <0x00 0x01>, <0x02 0x03>, <0x00 0x01>; /* base.dts:4:9-4:26, base.dts:8:9-8:36 */
>   }; /* base.dts:3:3-5:3, base.dts:7:3-9:3 */
> 
>   It isn't a real problem since it allows easily tracing that some parts
>   were constructed at other places, but is differnt from how a normal
>   array would appear.

Right.  This is just another example of the fact that properties are
just bytestrings, so how we format them for -Odts is a guess as to
type.  I sometimes think that preserving some "type" information
during the run so that -I dts -O dts will give output more similar to
the input was a mistake, since it can increase the false impression
that the dtb format is self-describing.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] Add alloc_marker
  2025-03-01 13:25 ` [PATCH 1/3] Add alloc_marker Ayush Singh
@ 2025-03-03  9:35   ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2025-03-03  9:35 UTC (permalink / raw)
  To: Ayush Singh
  Cc: Andreas Gnau, d-gole, lorforlinux, jkridner, robertcnelson,
	Andrew Davis, Geert Uytterhoeven, Simon Glass,
	devicetree-compiler

[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]

On Sat, Mar 01, 2025 at 06:55:02PM +0530, Ayush Singh wrote:
> - Add helper to allocate new marker
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

This patch is a reasonable cleanup, regardless of what happens with
the rest of the series.

> ---
>  data.c | 20 +++++++++++++++-----
>  dtc.h  |  2 ++
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/data.c b/data.c
> index 14734233ad8b7ebd38c3e62442b81aae66601806..913796f2e664d07cdc48e0cbf2ab5d6fe9978072 100644
> --- a/data.c
> +++ b/data.c
> @@ -228,11 +228,7 @@ struct data data_add_marker(struct data d, enum markertype type, char *ref)
>  {
>  	struct marker *m;
>  
> -	m = xmalloc(sizeof(*m));
> -	m->offset = d.len;
> -	m->type = type;
> -	m->ref = ref;
> -	m->next = NULL;
> +	m = alloc_marker(d.len, type, ref);
>  
>  	return data_append_markers(d, m);
>  }
> @@ -254,3 +250,17 @@ bool data_is_one_string(struct data d)
>  
>  	return true;
>  }
> +
> +struct marker *alloc_marker(unsigned int offset, enum markertype type,
> +			    char *ref)
> +{
> +	struct marker *m;
> +
> +	m = xmalloc(sizeof(*m));
> +	m->offset = offset;
> +	m->type = type;
> +	m->ref = ref;
> +	m->next = NULL;
> +
> +	return m;
> +}
> diff --git a/dtc.h b/dtc.h
> index 4c4aaca1fc417c9d93e904e64b2c40216ee1b093..86928e1eea9764fe5d74d6dbb987589d65d54b66 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -183,6 +183,8 @@ struct data data_append_byte(struct data d, uint8_t byte);
>  struct data data_append_zeroes(struct data d, int len);
>  struct data data_append_align(struct data d, int align);
>  
> +struct marker *alloc_marker(unsigned int offset, enum markertype type,
> +			    char *ref);
>  struct data data_add_marker(struct data d, enum markertype type, char *ref);
>  
>  bool data_is_one_string(struct data d);
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] dtc: Add /./
  2025-03-01 13:25 ` [PATCH 2/3] dtc: Add /./ Ayush Singh
@ 2025-03-05  4:14   ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2025-03-05  4:14 UTC (permalink / raw)
  To: Ayush Singh
  Cc: Andreas Gnau, d-gole, lorforlinux, jkridner, robertcnelson,
	Andrew Davis, Geert Uytterhoeven, Simon Glass,
	devicetree-compiler

[-- Attachment #1: Type: text/plain, Size: 5387 bytes --]

On Sat, Mar 01, 2025 at 06:55:03PM +0530, Ayush Singh wrote:
> Allow construction new values for a property using old property values.
> Can be used to append, pre-append, duplicate, etc.

Some examples from your series cover letter would make sense in the
commit message.

This mostly LGTM, a few notes below.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  dtc-lexer.l  |  5 +++++
>  dtc-parser.y |  5 +++++
>  dtc.h        |  1 +
>  livetree.c   | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index de60a70b6bdbcb5ae4336ea4171ad6f645e91b36..5efeca10363e0c9c338b6578be9240c3f42249f0 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -144,6 +144,11 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
>  			return DT_OMIT_NO_REF;
>  		}
>  
> +<*>"/./" {
> +			DPRINT("Keyword: /./\n");
> +			return DT_PREV_PROP;
> +		}
> +
>  <*>{LABEL}:	{
>  			DPRINT("Label: %s\n", yytext);
>  			yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4d5eece526243460203157464e3cd75f781e50e7..c34eb10a1068b5eb6a0f08e5a1db8066217b16bf 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -57,6 +57,7 @@ static bool is_ref_relative(const char *ref)
>  %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>  %token DT_BITS
>  %token DT_DEL_PROP
> +%token DT_PREV_PROP
>  %token DT_DEL_NODE
>  %token DT_OMIT_NO_REF
>  %token <propnodename> DT_PROPNODENAME
> @@ -308,6 +309,10 @@ propdata:
>  		{
>  			$$ = data_merge($1, $2);
>  		}
> +        | propdataprefix DT_PREV_PROP
> +                {
> +                        $$ = data_add_marker($1, PREV_VALUE, NULL);
> +                }
>  	| propdataprefix arrayprefix '>'
>  		{
>  			$$ = data_merge($1, $2.data);
> diff --git a/dtc.h b/dtc.h
> index 86928e1eea9764fe5d74d6dbb987589d65d54b66..175fe3637a31cc28453dc27980f315a171763b49 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -110,6 +110,7 @@ enum markertype {
>  	REF_PHANDLE,
>  	REF_PATH,
>  	LABEL,
> +	PREV_VALUE,
>  	TYPE_UINT8,
>  	TYPE_UINT16,
>  	TYPE_UINT32,
> diff --git a/livetree.c b/livetree.c
> index 93c77d95a320ec05aa355e12920cef9e1c91c26a..89e15c39e9eadb87cf8376fe167a4d9c6002031a 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -139,10 +139,34 @@ struct node *reference_node(struct node *node)
>  	return node;
>  }
>  
> +static struct data data_insert_old_value(struct data d, struct marker *m,
> +					 const struct data *old)

The convention is generall to pass struct data by value, not by pointer.

> +{
> +	unsigned int offset = m->offset;
> +	struct marker *next = m->next;
> +	struct marker *marker = m;

There's no point to this initialisation, because..

> +	struct data new_data;
> +
> +	new_data = data_insert_at_marker(d, marker, old->val, old->len);

.. you could just use 'm' here..

> +
> +	/* Copy all markers from old value */
> +	marker = old->markers;

.. then it is overwritten.

> +	for_each_marker(marker) {
> +		m->next = alloc_marker(marker->offset + offset, marker->type,
> +				       marker->ref);

This will make the new marker have marker->ref aliased with the old
marker.  That's... probably ok... but it makes me nervous.  Might be
better to xstrdup() ref here so each marker has its own copy.

If the marker is a label, and /./ appears multiple times, you'll now
have a duplicate label.  That probably warrants a specific warning, at
least.


> +		m = m->next;
> +	}
> +	m->next = next;
> +
> +	return new_data;
> +}
> +
>  struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  {
>  	struct property *new_prop, *old_prop;
>  	struct node *new_child, *old_child;
> +	bool prev_value_used = false;
> +	struct marker *marker;
>  	struct label *l;
>  
>  	old_node->deleted = 0;
> @@ -172,10 +196,26 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  				for_each_label_withdel(new_prop->labels, l)
>  					add_label(&old_prop->labels, l->label);
>  
> +				marker = new_prop->val.markers;
> +				for_each_marker_of_type(marker, PREV_VALUE) {
> +					new_prop->val = data_insert_old_value(
> +						new_prop->val, marker,
> +						&old_prop->val);
> +					prev_value_used = true;
> +				}
> +
>  				old_prop->val = new_prop->val;
>  				old_prop->deleted = 0;
> -				free(old_prop->srcpos);
> -				old_prop->srcpos = new_prop->srcpos;
> +
> +				if (prev_value_used) {
> +					old_prop->srcpos =
> +						srcpos_extend(old_prop->srcpos,
> +							      new_prop->srcpos);
> +				} else {
> +					free(old_prop->srcpos);
> +					old_prop->srcpos = new_prop->srcpos;
> +				}
> +

Hrm.  I realised there are some pre-existing bugs here: if srcpos has
multiple things chained onto it, we only free the first one.  Likewise
I think we don't properly free the old property value or its markers.

Then again, dtc is a short-lived process, so to an extent we don't
really care about memory leaks.

>  				free(new_prop);
>  				new_prop = NULL;
>  				break;
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] tests: Add test for /./
  2025-03-01 13:25 ` [PATCH 3/3] tests: Add test for /./ Ayush Singh
@ 2025-03-05  4:17   ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2025-03-05  4:17 UTC (permalink / raw)
  To: Ayush Singh
  Cc: Andreas Gnau, d-gole, lorforlinux, jkridner, robertcnelson,
	Andrew Davis, Geert Uytterhoeven, Simon Glass,
	devicetree-compiler

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On Sat, Mar 01, 2025 at 06:55:04PM +0530, Ayush Singh wrote:
> - Test /./ on a string and int array.
> - Also test on subnode property.

These tests are ok as far as they go.  Testing  a property with mixed
data types would be a good idea for completeness.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  tests/prev_prop.dts | 21 +++++++++++++++++++++
>  tests/run_tests.sh  |  7 +++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/tests/prev_prop.dts b/tests/prev_prop.dts
> new file mode 100644
> index 0000000000000000000000000000000000000000..41a0e19015b006d78a3ec35a26db63982ec4c90a
> --- /dev/null
> +++ b/tests/prev_prop.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +
> +/ {
> +        str-prop = "1", "2";
> +        int-prop = <1 2>;
> +
> +        subnode {
> +                str-prop = "1", "2";
> +                int-prop = <1 2>;
> +        };
> +};
> +
> +/ {
> +        str-prop = /./, "3", "4", /./, "6";
> +        int-prop = /./, <3 4>, /./, <6>;
> +
> +        subnode {
> +                str-prop = /./, "3", "4", /./;
> +                int-prop = /./, <3 4>, /./;
> +        };
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index f0b51c04bf0af69f1df483b185f3aefa5d0bae27..30aa99f55570ac10b85c94353711361c19a5b479 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -708,6 +708,13 @@ dtc_tests () {
>      run_dtc_test -I dts -O dtb -o dtc_tree1_delete.test.dtb "$SRCDIR/test_tree1_delete.dts"
>      tree1_tests dtc_tree1_delete.test.dtb
>  
> +    # Check previous property functionality
> +    run_dtc_test -I dts -O dtb -o prev_prop.test.dtb "$SRCDIR/prev_prop.dts"
> +    run_fdtget_test "1 2 3 4 1 2 6" prev_prop.test.dtb "/" "str-prop"
> +    run_fdtget_test "1 2 3 4 1 2 6" prev_prop.test.dtb "/" "int-prop"
> +    run_fdtget_test "1 2 3 4 1 2" prev_prop.test.dtb "/subnode" "str-prop"
> +    run_fdtget_test "1 2 3 4 1 2" prev_prop.test.dtb "/subnode"
> "int-prop"

Rather than have a batch of separate fdtget tests, I think it would be
cleaner to create a different .dts with the expected output from the
expansions, then compare them with dtbs_equal_ordered.

> +
>      # Check omit-if-no-ref functionality
>      run_dtc_test -I dts -O dtb -o omit-no-ref.test.dtb "$SRCDIR/omit-no-ref.dts"
>      run_test check_path omit-no-ref.test.dtb not-exists "/node1"
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-03-05  4:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-01 13:25 [PATCH 0/3] Add capability to create property from old property Ayush Singh
2025-03-01 13:25 ` [PATCH 1/3] Add alloc_marker Ayush Singh
2025-03-03  9:35   ` David Gibson
2025-03-01 13:25 ` [PATCH 2/3] dtc: Add /./ Ayush Singh
2025-03-05  4:14   ` David Gibson
2025-03-01 13:25 ` [PATCH 3/3] tests: Add test for /./ Ayush Singh
2025-03-05  4:17   ` David Gibson
2025-03-03  9:02 ` [PATCH 0/3] Add capability to create property from old property David Gibson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).