[PATCH v2 0/2] Add capability to append to property

[PATCH v2 0/2] Add capability to append to property

[Ayush Singh]

Allow appending values to a property instead of overriding the previous
values of property.

Currently, we have /delete-node/ and /delete-property/, but lack
/append-property/. Hence we end up having to repeat all existing values
when appending to a property (e.g. see [1] appending to clocks from
[2]).

This functionality is also important for creating a device tree based
implementation to support different types of addon-boards such as
mikroBUS, Grove [3], etc.

In practice, it looks as follows:

```
dts-v1/;

/ {
	str-prop = "0";
};

/ {
	/append-property/ str-prop = "1";
};
```

Open items
- Better tests: I only started the work today, so well, I still don't
  have a good picture of how to properly write tests.

- Appending to non-existent property: Currently am just adding a new
  property if not found. Not sure if this is the desired behaviour.

- Incompatible values: Not sure how to handle when trying to append
  incompatible values.

[3] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
[2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L39
[1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L3334

Signed-off-by: Ayush Singh <ayush@beagleboard.org>

---
Changes in v2:
- Add comment for append struct member.
- Small code improvements
- Improve test
- Link to v1: https://lore.kernel.org/r/20240827-append-v1-0-d7a126ef1be3@beagleboard.org

---
Ayush Singh (2):
      dtc: Add /append-property/
      tests: Add test for append-property

 dtc-lexer.l           |  7 +++++++
 dtc-parser.y          |  6 ++++++
 dtc.h                 |  5 +++++
 livetree.c            | 29 ++++++++++++++++++++++++-----
 tests/append_prop.dts | 21 +++++++++++++++++++++
 tests/run_tests.sh    |  7 +++++++
 6 files changed, 70 insertions(+), 5 deletions(-)
---
base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
change-id: 20240827-append-f614fe3261f9

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

[PATCH v2 1/2] dtc: Add /append-property/

[Ayush Singh]

Allow appending values to a property instead of overriding the previous
values of property.

Currently, we have /delete-node/ and /delete-property/, but lack
/append-property/. Hence we end up having to repeat all existing values
when appending to a property (e.g. see [1] appending to clocks from
[2]).

This functionality is also important for creating a device tree based
implementation to support different types of addon-boards such as
mikroBUS, Grove [3], etc.

[3] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/
[2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L39
[1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L3334

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 dtc-lexer.l  |  7 +++++++
 dtc-parser.y |  6 ++++++
 dtc.h        |  5 +++++
 livetree.c   | 29 ++++++++++++++++++++++++-----
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index de60a70..5da4ca5 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -137,6 +137,13 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
 			return DT_DEL_NODE;
 		}
 
+<*>"/append-property/"	{
+			DPRINT("Keyword: /append-property/\n");
+			DPRINT("<PROPNODENAME>\n");
+			BEGIN(PROPNODENAME);
+			return DT_APP_PROP;
+		}
+
 <*>"/omit-if-no-ref/"	{
 			DPRINT("Keyword: /omit-if-no-ref/\n");
 			DPRINT("<PROPNODENAME>\n");
diff --git a/dtc-parser.y b/dtc-parser.y
index 4d5eece..94ce405 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -58,6 +58,7 @@ static bool is_ref_relative(const char *ref)
 %token DT_BITS
 %token DT_DEL_PROP
 %token DT_DEL_NODE
+%token DT_APP_PROP
 %token DT_OMIT_NO_REF
 %token <propnodename> DT_PROPNODENAME
 %token <integer> DT_LITERAL
@@ -296,6 +297,11 @@ propdef:
 			$$ = build_property_delete($2);
 			free($2);
 		}
+	| DT_APP_PROP DT_PROPNODENAME '=' propdata ';'
+		{
+			$$ = build_property_append($2, $4, &@$);
+			free($2);
+		}
 	| DT_LABEL propdef
 		{
 			add_label(&$2->labels, $1);
diff --git a/dtc.h b/dtc.h
index 4c4aaca..3c9ba5a 100644
--- a/dtc.h
+++ b/dtc.h
@@ -205,6 +205,9 @@ struct bus_type {
 
 struct property {
 	bool deleted;
+	/* Flag to indicate if the property value should be appended instead
+	 * of being overwritten */
+	bool append;
 	char *name;
 	struct data val;
 
@@ -263,6 +266,8 @@ void delete_labels(struct label **labels);
 struct property *build_property(const char *name, struct data val,
 				struct srcpos *srcpos);
 struct property *build_property_delete(const char *name);
+struct property *build_property_append(const char *name, struct data val,
+				       struct srcpos *srcpos);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
 
diff --git a/livetree.c b/livetree.c
index 49f7230..d9b4dc9 100644
--- a/livetree.c
+++ b/livetree.c
@@ -62,6 +62,20 @@ struct property *build_property_delete(const char *name)
 	return new;
 }
 
+struct property *build_property_append(const char *name, struct data val,
+				       struct srcpos *srcpos)
+{
+	struct property *new = xmalloc(sizeof(*new));
+
+	memset(new, 0, sizeof(*new));
+	new->name = xstrdup(name);
+	new->append = true;
+	new->val = val;
+	new->srcpos = srcpos_copy(srcpos);
+
+	return new;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);
@@ -151,8 +165,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	for_each_label_withdel(new_node->labels, l)
 		add_label(&old_node->labels, l->label);
 
-	/* Move properties from the new node to the old node.  If there
-	 * is a collision, replace the old value with the new */
+	/* Move properties from the new node to the old node. If there
+	 * is a collision, replace/append the old value with the new */
 	while (new_node->proplist) {
 		/* Pop the property off the list */
 		new_prop = new_node->proplist;
@@ -165,15 +179,20 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 			continue;
 		}
 
-		/* Look for a collision, set new value if there is */
+		/* Look for a collision, set new value/append if there is */
 		for_each_property_withdel(old_node, old_prop) {
 			if (streq(old_prop->name, new_prop->name)) {
 				/* Add new labels to old property */
 				for_each_label_withdel(new_prop->labels, l)
 					add_label(&old_prop->labels, l->label);
 
-				old_prop->val = new_prop->val;
-				old_prop->deleted = 0;
+				if (new_prop->append)
+					old_prop->val = data_merge(old_prop->val, new_prop->val);
+				else
+					old_prop->val = new_prop->val;
+
+				old_prop->deleted = false;
+				old_prop->append = false;
 				free(old_prop->srcpos);
 				old_prop->srcpos = new_prop->srcpos;
 				free(new_prop);

-- 
2.46.0

[PATCH v2 2/2] tests: Add test for append-property

[Ayush Singh]

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

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

diff --git a/tests/append_prop.dts b/tests/append_prop.dts
new file mode 100644
index 0000000..248d4ed
--- /dev/null
+++ b/tests/append_prop.dts
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/ {
+	str-prop = "0";
+	num-prop = <2>;
+
+	subnode{
+		str-prop = "0";
+		num-prop = <2>;
+	};
+};
+
+/ {
+	/append-property/ str-prop = "1";
+	/append-property/ num-prop = <1>;
+
+	subnode{
+		/append-property/ str-prop = "1";
+		/append-property/ num-prop = <1>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 937b128..571980a 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 prop append functionality
+    run_dtc_test -I dts -O dtb -o append_prop.test.dtb "$SRCDIR/append_prop.dts"
+    run_fdtget_test "0 1" append_prop.test.dtb "/" "str-prop"
+    run_fdtget_test "2 1" append_prop.test.dtb "/" "num-prop"
+    run_fdtget_test "0 1" append_prop.test.dtb "/subnode" "str-prop"
+    run_fdtget_test "2 1" append_prop.test.dtb "/subnode" "num-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.46.0

Re: [PATCH v2 2/2] tests: Add test for append-property

[Simon Glass]

Hi Ayush,

On Thu, 29 Aug 2024 at 14:04, Ayush Singh <ayush@beagleboard.org> wrote:
>
> - Test /append-property/ on a string and int array.
> - Also test on subnode property.
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  tests/append_prop.dts | 21 +++++++++++++++++++++
>  tests/run_tests.sh    |  7 +++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/tests/append_prop.dts b/tests/append_prop.dts
> new file mode 100644
> index 0000000..248d4ed
> --- /dev/null
> +++ b/tests/append_prop.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +
> +/ {
> +       str-prop = "0";
> +       num-prop = <2>;
> +
> +       subnode{
> +               str-prop = "0";
> +               num-prop = <2>;
> +       };
> +};
> +
> +/ {
> +       /append-property/ str-prop = "1";
> +       /append-property/ num-prop = <1>;
> +
> +       subnode{
> +               /append-property/ str-prop = "1";
> +               /append-property/ num-prop = <1>;
> +       };
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 937b128..571980a 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 prop append functionality
> +    run_dtc_test -I dts -O dtb -o append_prop.test.dtb "$SRCDIR/append_prop.dts"
> +    run_fdtget_test "0 1" append_prop.test.dtb "/" "str-prop"
> +    run_fdtget_test "2 1" append_prop.test.dtb "/" "num-prop"
> +    run_fdtget_test "0 1" append_prop.test.dtb "/subnode" "str-prop"
> +    run_fdtget_test "2 1" append_prop.test.dtb "/subnode" "num-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"

I wonder what happens when you try to append but there is no existing
property? Or when you append something empty?

Regards,
Simon

Re: [PATCH v2 2/2] tests: Add test for append-property

[CVS]

Also, how does the existing -T / --annotate option behave with this
proposed feature ?
i.e. when a line in the device-tree is modified using /append-property
what (all) would the annotation comment contain ?

regards
CVS

Re: [PATCH v2 2/2] tests: Add test for append-property

[Ayush Singh]

On 8/30/24 06:36, Simon Glass wrote:

> Hi Ayush,
>
> On Thu, 29 Aug 2024 at 14:04, Ayush Singh <ayush@beagleboard.org> wrote:
>> - Test /append-property/ on a string and int array.
>> - Also test on subnode property.
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   tests/append_prop.dts | 21 +++++++++++++++++++++
>>   tests/run_tests.sh    |  7 +++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/tests/append_prop.dts b/tests/append_prop.dts
>> new file mode 100644
>> index 0000000..248d4ed
>> --- /dev/null
>> +++ b/tests/append_prop.dts
>> @@ -0,0 +1,21 @@
>> +/dts-v1/;
>> +
>> +/ {
>> +       str-prop = "0";
>> +       num-prop = <2>;
>> +
>> +       subnode{
>> +               str-prop = "0";
>> +               num-prop = <2>;
>> +       };
>> +};
>> +
>> +/ {
>> +       /append-property/ str-prop = "1";
>> +       /append-property/ num-prop = <1>;
>> +
>> +       subnode{
>> +               /append-property/ str-prop = "1";
>> +               /append-property/ num-prop = <1>;
>> +       };
>> +};
>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
>> index 937b128..571980a 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 prop append functionality
>> +    run_dtc_test -I dts -O dtb -o append_prop.test.dtb "$SRCDIR/append_prop.dts"
>> +    run_fdtget_test "0 1" append_prop.test.dtb "/" "str-prop"
>> +    run_fdtget_test "2 1" append_prop.test.dtb "/" "num-prop"
>> +    run_fdtget_test "0 1" append_prop.test.dtb "/subnode" "str-prop"
>> +    run_fdtget_test "2 1" append_prop.test.dtb "/subnode" "num-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"
> I wonder what happens when you try to append but there is no existing
> property? Or when you append something empty?
>
> Regards,
> Simon


Currently, it will just create a new property. I have mentioned this as 
an Open Item in the cover letter. We can make it fail if that makes more 
sense.


Ayush Singh

Re: [PATCH v2 2/2] tests: Add test for append-property

[Ayush Singh]

On 8/30/24 08:58, CVS wrote:

> Also, how does the existing -T / --annotate option behave with this
> proposed feature ?
> i.e. when a line in the device-tree is modified using /append-property
> what (all) would the annotation comment contain ?
>
> regards
> CVS


I was not aware of this option, so just tried using it and here are the 
results:


Input file:

```

/dts-v1/;

/ {
     str-prop = "0";
     num-prop = <2>;

     subnode{
         str-prop = "0";
         num-prop = <2>;
     };
};

/ {
     /append-property/ str-prop = "1";
     /append-property/ num-prop = <1>;

     subnode{
         /append-property/ str-prop = "1";
         /append-property/ num-prop = <1>;
     };
};

```

Command:

./dtc -T -o temp.dts ../tests/append_prop.dts


Results:

```

/dts-v1/;

/ { /* append_prop.dts:3, append_prop.dts:13 */
     str-prop = "0", "1"; /* append_prop.dts:14 */
     num-prop = <0x02>, <0x01>; /* append_prop.dts:15 */

     subnode { /* append_prop.dts:7, append_prop.dts:17 */
         str-prop = "0", "1"; /* append_prop.dts:18 */
         num-prop = <0x02>, <0x01>; /* append_prop.dts:19 */
     }; /* append_prop.dts:10, append_prop.dts:20 */
}; /* append_prop.dts:11, append_prop.dts:21 */

```


For more detaild information using -T -T:

```

/dts-v1/;

/ { /* ../tests/append_prop.dts:3:3-11:3, 
../tests/append_prop.dts:13:3-21:3 */
     str-prop = "0", "1"; /* ../tests/append_prop.dts:14:2-14:35 */
     num-prop = <0x02>, <0x01>; /* ../tests/append_prop.dts:15:2-15:35 */

     subnode { /* ../tests/append_prop.dts:7:9-10:4, 
../tests/append_prop.dts:17:9-20:4 */
         str-prop = "0", "1"; /* ../tests/append_prop.dts:18:3-18:36 */
         num-prop = <0x02>, <0x01>; /* 
../tests/append_prop.dts:19:3-19:36 */
     }; /* ../tests/append_prop.dts:7:9-10:4, 
../tests/append_prop.dts:17:9-20:4 */
}; /* ../tests/append_prop.dts:3:3-11:3, 
../tests/append_prop.dts:13:3-21:3 */

```


Does this look acceptable?


Ayush Singh

Re: [PATCH v2 0/2] Add capability to append to property

[Geert Uytterhoeven]

Hi Ayush,

On Thu, Aug 29, 2024 at 10:04 PM Ayush Singh <ayush@beagleboard.org> wrote:
> Allow appending values to a property instead of overriding the previous
> values of property.

Thanks for your series!

> Open items

> - Appending to non-existent property: Currently am just adding a new
>   property if not found. Not sure if this is the desired behaviour.

I think this should raise an error.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/2] Add capability to append to property

[Andrew Davis]

On 8/30/24 3:09 AM, Geert Uytterhoeven wrote:
> Hi Ayush,
> 
> On Thu, Aug 29, 2024 at 10:04 PM Ayush Singh <ayush@beagleboard.org> wrote:
>> Allow appending values to a property instead of overriding the previous
>> values of property.
> 
> Thanks for your series!
> 
>> Open items
> 
>> - Appending to non-existent property: Currently am just adding a new
>>    property if not found. Not sure if this is the desired behaviour.
> 
> I think this should raise an error.
> 

I was thinking this at first too, but if we want to be consistent we
may want to instead go with adding the new property without error for
the following reasons.

High level, nodes and properties should behave the similarly. When
we run into an already defined node we take the content and append
to the existing node. But for properties we replace the content,
not append. This is the fundamental asymmetry that causes us to
need /append-property/ but not need /append-node/, nodes append
by default.

When we want to replace a node we do /delete-node/ first, followed
by the defining the new node content. If properties were default
append, we would do the same with /delete-property/ followed by
the new content. If we could do all this over that would have made
the semantics much cleaner and only required /delete-node/ and
/delete-property/, but little late for that now..

So when we label a property with /append-property/, we want it to
have the same semantics as nodes. Which is to append if it exists,
and to add new if not, without error.

Andrew

Re: [PATCH v2 0/2] Add capability to append to property

[David Gibson]

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

On Fri, Aug 30, 2024 at 10:43:11AM -0500, Andrew Davis wrote:
> On 8/30/24 3:09 AM, Geert Uytterhoeven wrote:
> > Hi Ayush,
> > 
> > On Thu, Aug 29, 2024 at 10:04 PM Ayush Singh <ayush@beagleboard.org> wrote:
> > > Allow appending values to a property instead of overriding the previous
> > > values of property.

I can see how this would be useful for certain cases.  I don't love
implementing it just as a specific ad-hoc new keyword thuogh.  I did
long ago have a more comprehensive plan that would allow for this,
amongst other things.  Currently we allow integer expressions in dts
files, but I had been hoping to add string-valued and
bytestring-valued expressions as well.  That would allow appends as
something like:

	prop = /previous-value/, "extra stuff";

',' being the bytestring append operator.  It would also allow
prepend, and - with the addition of other bytestring operations -
various other manipulations.

However, the chances of me ever actually getting around to
implementing this are basically zero at this point.  I guess based on
that design we could allow appends with the syntax:
	prop ,= "additional data";

Which is, I think, theoretically more elegant, but also risks being
pretty unreadable.

> > Thanks for your series!
> > 
> > > Open items
> > 
> > > - Appending to non-existent property: Currently am just adding a new
> > >    property if not found. Not sure if this is the desired behaviour.
> > 
> > I think this should raise an error.
> > 
> 
> I was thinking this at first too, but if we want to be consistent we
> may want to instead go with adding the new property without error for
> the following reasons.
> 
> High level, nodes and properties should behave the similarly.

I'm not sure I buy that premise.  dtb is not like (say) json, where
"node" and "property" are just different types an entry could have.
Properties are structurally distinct from nodes (the subnodes and
properties and a node even occupy different namespaces).

The other important distinction is that dtc necessarily understands
the internal structure of nodes, but it doesn't necessarily know
anything about the internal structure of properties (beyond that
they're bytestrings).

> When
> we run into an already defined node we take the content and append
> to the existing node. But for properties we replace the content,
> not append. This is the fundamental asymmetry that causes us to
> need /append-property/ but not need /append-node/, nodes append
> by default.
> 
> When we want to replace a node we do /delete-node/ first, followed
> by the defining the new node content. If properties were default
> append, we would do the same with /delete-property/ followed by
> the new content. If we could do all this over that would have made
> the semantics much cleaner and only required /delete-node/ and
> /delete-property/, but little late for that now..
> 
> So when we label a property with /append-property/, we want it to
> have the same semantics as nodes. Which is to append if it exists,
> and to add new if not, without error.

Hrm.  I'm pretty dubious about this approach because a property being
absent is different from a property being present, but containing an
empty bytestring as value ("boolean" properties rely on this).
Appending in this matter erases that distinction in the base tree.

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

Re: [PATCH v2 2/2] tests: Add test for append-property

[CVS]

Hmmm... in the above examples, using --annotate on a single source file,
it is not immediately evident whether the existing annotation logic within dtc
handles "append-property" across multiple dts(i) files properly.

As you can see, the -T (annotate) and -T -T (detailed annotate)
options will add inline comments in the final unified device-tree with
the actual source file:line.
This is a critical feature in managing complex hierarchies of dtsi
files being included in a dts.

Until now (without the append-property keyword), IIUC, there can be
ONLY one source file/line responsible for a line in the final unified
device-tree. The latest line specifying a property overwrites all
other specifications of a property before it.

With the introduction of this append-property keyword, now more than
one file:line can be a source of a line in the final unified
device-tree. So, i wonder how the annotations will look like in the
scenario in which more than one source file:line is responsible for
the content of a single line in the final unified device-tree.

If by any chance there is insufficient annotation containing of all
the lines responsible for the final  "appended list", i worry it will
be a nightmare to identify the source of the resulting "appended list"
of values being assigned to a property.

regards
CVS

Re: [PATCH v2 2/2] tests: Add test for append-property

[Ayush Singh]

On 9/12/24 16:36, CVS wrote:

> Hmmm... in the above examples, using --annotate on a single source file,
> it is not immediately evident whether the existing annotation logic within dtc
> handles "append-property" across multiple dts(i) files properly.
>
> As you can see, the -T (annotate) and -T -T (detailed annotate)
> options will add inline comments in the final unified device-tree with
> the actual source file:line.
> This is a critical feature in managing complex hierarchies of dtsi
> files being included in a dts.
>
> Until now (without the append-property keyword), IIUC, there can be
> ONLY one source file/line responsible for a line in the final unified
> device-tree. The latest line specifying a property overwrites all
> other specifications of a property before it.
>
> With the introduction of this append-property keyword, now more than
> one file:line can be a source of a line in the final unified
> device-tree. So, i wonder how the annotations will look like in the
> scenario in which more than one source file:line is responsible for
> the content of a single line in the final unified device-tree.
>
> If by any chance there is insufficient annotation containing of all
> the lines responsible for the final  "appended list", i worry it will
> be a nightmare to identify the source of the resulting "appended list"
> of values being assigned to a property.
>
> regards
> CVS


So I tried it with 3 files:

base.dtsi

```

/dts-v1/;

/ {
     prop = "0";

     propa = <0>;

};

```


app1.dtsi

```

/dts-v1/;

/include/ "base.dtsi"

/ {
     /append-property/ prop = "1";
};

```


app2.dts

```

/dts-v1/;

/include/ "app1.dtsi"

/ {
     /append-property/ prop = "2";
     /append-property/ propa = <2>;
};

```


Output:

❯ ../dtc -T -T -o temp.dts app2.dts
❯ cat temp.dts
/dts-v1/;

/ { /* base.dtsi:3:3-7:3, app1.dtsi:5:3-7:3, app2.dts:5:3-8:3 */
         prop = "0", "1", "2"; /* app2.dts:6:2-6:31 */
         propa = <0x00>, <0x02>; /* app2.dts:7:2-7:32 */
}; /* base.dtsi:3:3-7:3, app1.dtsi:5:3-7:3, app2.dts:5:3-8:3 */


So it seems to only contain info about the last append file. I will fix 
it in the next patch. It just needs to have info regarding all files 
that appended to it right? We already seem to be doing that for nodes so 
I can probably use similar strategy for properties.


Ayush Singh

Re: [PATCH v2 0/2] Add capability to append to property

[Ayush Singh]

So, I would like to push append support forward since it is useful even 
outside of connector overlays. I already have patches that handle the 
annotations properly as pointed out by CVS, so let me try to answer some 
stuff here to revive the discussion. Personally, I do not really have 
much preference in the syntax or the non-existent property thing, so I 
would be okay with whatever the kernel wants.


> On Fri, Aug 30, 2024 at 10:43:11AM -0500, Andrew Davis wrote:
>> On 8/30/24 3:09 AM, Geert Uytterhoeven wrote:
>>> Hi Ayush,
>>>
>>> On Thu, Aug 29, 2024 at 10:04 PM Ayush Singh <ayush@beagleboard.org> wrote:
>>>> Allow appending values to a property instead of overriding the previous
>>>> values of property.
> I can see how this would be useful for certain cases.  I don't love
> implementing it just as a specific ad-hoc new keyword thuogh.  I did
> long ago have a more comprehensive plan that would allow for this,
> amongst other things.  Currently we allow integer expressions in dts
> files, but I had been hoping to add string-valued and
> bytestring-valued expressions as well.  That would allow appends as
> something like:
>
> 	prop = /previous-value/, "extra stuff";
>
> ',' being the bytestring append operator.  It would also allow
> prepend, and - with the addition of other bytestring operations -
> various other manipulations.
>
> However, the chances of me ever actually getting around to
> implementing this are basically zero at this point.  I guess based on
> that design we could allow appends with the syntax:
> 	prop ,= "additional data";
>
> Which is, I think, theoretically more elegant, but also risks being
> pretty unreadable.

Well, both of these kind of introduce a completely new style. Meanwhile 
`/delete-node/` already exists, so I think unless there are more plans 
regarding this new syntax, sticking with something familiar might be better.


>
>>> Thanks for your series!
>>>
>>>> Open items
>>>> - Appending to non-existent property: Currently am just adding a new
>>>>     property if not found. Not sure if this is the desired behaviour.
>>> I think this should raise an error.
>>>
>> I was thinking this at first too, but if we want to be consistent we
>> may want to instead go with adding the new property without error for
>> the following reasons.
>>
>> High level, nodes and properties should behave the similarly.
> I'm not sure I buy that premise.  dtb is not like (say) json, where
> "node" and "property" are just different types an entry could have.
> Properties are structurally distinct from nodes (the subnodes and
> properties and a node even occupy different namespaces).
>
> The other important distinction is that dtc necessarily understands
> the internal structure of nodes, but it doesn't necessarily know
> anything about the internal structure of properties (beyond that
> they're bytestrings).

Isn't this a dtc implementation detail and not a limitation of spec. As 
far as I understand it, this whole notion of bytestrings comes from 
IEEE1275 days.


>
>> When
>> we run into an already defined node we take the content and append
>> to the existing node. But for properties we replace the content,
>> not append. This is the fundamental asymmetry that causes us to
>> need /append-property/ but not need /append-node/, nodes append
>> by default.
>>
>> When we want to replace a node we do /delete-node/ first, followed
>> by the defining the new node content. If properties were default
>> append, we would do the same with /delete-property/ followed by
>> the new content. If we could do all this over that would have made
>> the semantics much cleaner and only required /delete-node/ and
>> /delete-property/, but little late for that now..
>>
>> So when we label a property with /append-property/, we want it to
>> have the same semantics as nodes. Which is to append if it exists,
>> and to add new if not, without error.
> Hrm.  I'm pretty dubious about this approach because a property being
> absent is different from a property being present, but containing an
> empty bytestring as value ("boolean" properties rely on this).
> Appending in this matter erases that distinction in the base tree.
>
Well, I think the dt bindings check is supposed to catch stuff like 
this. Since all property values are bytestrings, it is possible to 
append any data type to well, anything.


I think it might be better to think about cases where one default 
behavior is preferable over other. It is important to note that there is 
no support for conditionals in devicetree. This means that if we go with 
an approach to throw error if a property does not exist, we will not 
have any way to express the following case: "append if property exits, 
else create new one". If there are any situations where that case comes 
into play, we probably should stick with the current approach.


Ayush Singh