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

[PATCH v3 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
- Appending to non-existent property:

  I think it is better to create a new property if the property does not
  exist instead of giving an error.
  If the default is an error, then the condition, "create a property if
  it does not exist, else append" cannot be expressed. I think this
  behaviour is desirable with more complex overlays required for
  supporting addon boards using devicetree.

[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 v3:
- Add support for annotations. Works similar to nodes (since they
  already append by default).
- Link to v2: https://lore.kernel.org/r/20240830-append-v2-0-ec1e03f110ad@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            | 36 +++++++++++++++++++++++++++++-------
 tests/append_prop.dts | 21 +++++++++++++++++++++
 tests/run_tests.sh    |  7 +++++++
 6 files changed, 75 insertions(+), 7 deletions(-)
---
base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
change-id: 20240827-append-f614fe3261f9

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

[PATCH v3 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.

The annotate feature works similar to how it works for nodes. Here is a
small example:

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"; /* base.dtsi:4:2-4:13, app1.dtsi:6:2-6:31, app2.dts:6:2-6:31 */
	propa = <0x00>, <0x02>; /* base.dtsi:6:3-6:15, app2.dts:7:2-7:32 */
}; /* base.dtsi:3:3-7:3, app1.dtsi:5:3-7:3, app2.dts:5:3-8:3 */
```

[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   | 36 +++++++++++++++++++++++++++++-------
 4 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index de60a70b6bdbcb5ae4336ea4171ad6f645e91b36..5da4ca5bbacbababa43d00199d5bb3bc15c8ff6a 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 4d5eece526243460203157464e3cd75f781e50e7..94ce405ab289ca82680a62bfe71e590f7b2e5e8e 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 4c4aaca1fc417c9d93e904e64b2c40216ee1b093..3c9ba5aa92e90dd50995596b753e246d4fa52b24 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 49f723002f855764452b30b5a979b6096730a33b..04f5014ab227f2458b8a93fcc6c8cb3a1388fd0d 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,17 +179,25 @@ 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;
-				free(old_prop->srcpos);
-				old_prop->srcpos = new_prop->srcpos;
+				if (new_prop->append) {
+					old_prop->val = data_merge(old_prop->val, new_prop->val);
+					old_prop->srcpos = srcpos_extend(old_prop->srcpos, new_prop->srcpos);
+				}
+				else {
+					old_prop->val = new_prop->val;
+					free(old_prop->srcpos);
+					old_prop->srcpos = new_prop->srcpos;
+				}
+
+				old_prop->deleted = false;
+				old_prop->append = false;
 				free(new_prop);
 				new_prop = NULL;
 				break;

-- 
2.47.0

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

[Dhruva Gole]

On Nov 11, 2024 at 15:24:33 +0530, Ayush Singh wrote:
> 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.
> 
> The annotate feature works similar to how it works for nodes. Here is a
> small example:
> 
> 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"; /* base.dtsi:4:2-4:13, app1.dtsi:6:2-6:31, app2.dts:6:2-6:31 */
> 	propa = <0x00>, <0x02>; /* base.dtsi:6:3-6:15, app2.dts:7:2-7:32 */
> }; /* base.dtsi:3:3-7:3, app1.dtsi:5:3-7:3, app2.dts:5:3-8:3 */
> ```
> 
> [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   | 36 +++++++++++++++++++++++++++++-------
>  4 files changed, 47 insertions(+), 7 deletions(-)

In general, I agree with these changes. Would be really good to have
append-property.

Reviewed-by: Dhruva Gole <d-gole@ti.com>

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

[PATCH v3 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 0000000000000000000000000000000000000000..248d4ed5ce5a753c9444d21359249669d88b7784
--- /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 937b128864d03a2aaa8edc171d7ec7bd8fa28569..571980a155ac4510c77686c254ee809a0f4ff108 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.47.0

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

[Ayush Singh]

On 11/11/24 15:24, Ayush Singh wrote:
> 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
> - Appending to non-existent property:
> 
>    I think it is better to create a new property if the property does not
>    exist instead of giving an error.
>    If the default is an error, then the condition, "create a property if
>    it does not exist, else append" cannot be expressed. I think this
>    behaviour is desirable with more complex overlays required for
>    supporting addon boards using devicetree.
> 
> [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 v3:
> - Add support for annotations. Works similar to nodes (since they
>    already append by default).
> - Link to v2: https://lore.kernel.org/r/20240830-append-v2-0-ec1e03f110ad@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            | 36 +++++++++++++++++++++++++++++-------
>   tests/append_prop.dts | 21 +++++++++++++++++++++
>   tests/run_tests.sh    |  7 +++++++
>   6 files changed, 75 insertions(+), 7 deletions(-)
> ---
> base-commit: 99031e3a4a6e479466ae795790b44727434ca27d
> change-id: 20240827-append-f614fe3261f9
> 
> Best regards,

cc David Gibson <david@gibson.dropbear.id.au>

Sorry, forgot to tag you in this patch as well.


Ayush Singh

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

[Andreas Gnau]

On 2024-11-11 10:54, Ayush Singh wrote:
> 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";
> };
> ```

If we add /append-property/, why not add /prepend-property/ as well? 
This is not only "nice for consistency", but it would also enable 
solving a problem where SoC compatible strings from dtsi [1] need to be 
repeated in board dts [2], because one cannot prepend to an existing 
property.

```
dts-v1/;
/ {
	compatible = "soc-vendor,soc1234";
};

/ {
	/prepend-property/ compatible = "board-vendor,board-xyz";
};
```

What do you think?

[1] 
https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/renesas/r8a77951.dtsi#L18
[2] 
https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts#L14

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

[Ayush Singh]

On 10/12/24 21:04, Andreas Gnau wrote:
> On 2024-11-11 10:54, Ayush Singh wrote:
>> 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";
>> };
>> ```
> 
> If we add /append-property/, why not add /prepend-property/ as well? 
> This is not only "nice for consistency", but it would also enable 
> solving a problem where SoC compatible strings from dtsi [1] need to be 
> repeated in board dts [2], because one cannot prepend to an existing 
> property.
> 
> ```
> dts-v1/;
> / {
>      compatible = "soc-vendor,soc1234";
> };
> 
> / {
>      /prepend-property/ compatible = "board-vendor,board-xyz";
> };
> ```
> 
> What do you think?
> 
> [1] https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/ 
> renesas/r8a77951.dtsi#L18
> [2] https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/ 
> renesas/r8a77951-salvator-xs.dts#L14

Well, it does seem useful. I can implement it after append-property 
support is merged.

Ayush Singh

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

[David Gibson]

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

On Tue, Dec 10, 2024 at 04:34:17PM +0100, Andreas Gnau wrote:
> On 2024-11-11 10:54, Ayush Singh wrote:
> > 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";
> > };
> > ```
> 
> If we add /append-property/, why not add /prepend-property/ as well? This is
> not only "nice for consistency", but it would also enable solving a problem
> where SoC compatible strings from dtsi [1] need to be repeated in board dts
> [2], because one cannot prepend to an existing property.
> 
> ```
> dts-v1/;
> / {
> 	compatible = "soc-vendor,soc1234";
> };
> 
> / {
> 	/prepend-property/ compatible = "board-vendor,board-xyz";
> };
> ```
> 
> What do you think?

So, this kind of demonstrates why I don't love /append-property/ as a
proposed syntax.  The way I'd prefer to do this, ideally, is to allow
properties to be described as expressions.  We already have integer
expressions that can be used in < > context, but I had intended to
extent to string and bytestring expressions - but I've never had the
time to implement that.

Under that proposal, I'd expect appending to look something like:
	str-prop = /previous-value/, "1";

Prepending,
	str-prop = "1", /previous-value/;

.. and you could do both at once in the obvious way.

The downside, of course, is that this is a much more complicated
proposal to implement.  Parsing that syntax isn't too hard, but I
think doing it sensibly will need some structural changes in order to
evaluate property values as expressions, rather than simply
constructing the properties directly left to right.  In particular the
interactions between expression syntax and within-property labels (and
other markers) could be fiddly to get right.

-- 
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 v3 0/2] Add capability to append to property

[Ayush Singh]

On 16/12/24 11:39, David Gibson wrote:
> On Tue, Dec 10, 2024 at 04:34:17PM +0100, Andreas Gnau wrote:
>> On 2024-11-11 10:54, Ayush Singh wrote:
>>> 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";
>>> };
>>> ```
>> If we add /append-property/, why not add /prepend-property/ as well? This is
>> not only "nice for consistency", but it would also enable solving a problem
>> where SoC compatible strings from dtsi [1] need to be repeated in board dts
>> [2], because one cannot prepend to an existing property.
>>
>> ```
>> dts-v1/;
>> / {
>> 	compatible = "soc-vendor,soc1234";
>> };
>>
>> / {
>> 	/prepend-property/ compatible = "board-vendor,board-xyz";
>> };
>> ```
>>
>> What do you think?
> So, this kind of demonstrates why I don't love /append-property/ as a
> proposed syntax.  The way I'd prefer to do this, ideally, is to allow
> properties to be described as expressions.  We already have integer
> expressions that can be used in < > context, but I had intended to
> extent to string and bytestring expressions - but I've never had the
> time to implement that.
>
> Under that proposal, I'd expect appending to look something like:
> 	str-prop = /previous-value/, "1";
>
> Prepending,
> 	str-prop = "1", /previous-value/;
>
> .. and you could do both at once in the obvious way.
>
> The downside, of course, is that this is a much more complicated
> proposal to implement.  Parsing that syntax isn't too hard, but I
> think doing it sensibly will need some structural changes in order to
> evaluate property values as expressions, rather than simply
> constructing the properties directly left to right.  In particular the
> interactions between expression syntax and within-property labels (and
> other markers) could be fiddly to get right.
>

Do you wish to allow `/previous-value/` to be repeated in the same 
property? Something like this:

     str-prop = "1", /previous-value/, "2", /previous-value/, "3";


Ayush Singh

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

[David Gibson]

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

On Fri, Dec 20, 2024 at 09:00:14PM +0530, Ayush Singh wrote:
> 
> On 16/12/24 11:39, David Gibson wrote:
> > On Tue, Dec 10, 2024 at 04:34:17PM +0100, Andreas Gnau wrote:
> > > On 2024-11-11 10:54, Ayush Singh wrote:
> > > > 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";
> > > > };
> > > > ```
> > > If we add /append-property/, why not add /prepend-property/ as well? This is
> > > not only "nice for consistency", but it would also enable solving a problem
> > > where SoC compatible strings from dtsi [1] need to be repeated in board dts
> > > [2], because one cannot prepend to an existing property.
> > > 
> > > ```
> > > dts-v1/;
> > > / {
> > > 	compatible = "soc-vendor,soc1234";
> > > };
> > > 
> > > / {
> > > 	/prepend-property/ compatible = "board-vendor,board-xyz";
> > > };
> > > ```
> > > 
> > > What do you think?
> > So, this kind of demonstrates why I don't love /append-property/ as a
> > proposed syntax.  The way I'd prefer to do this, ideally, is to allow
> > properties to be described as expressions.  We already have integer
> > expressions that can be used in < > context, but I had intended to
> > extent to string and bytestring expressions - but I've never had the
> > time to implement that.
> > 
> > Under that proposal, I'd expect appending to look something like:
> > 	str-prop = /previous-value/, "1";
> > 
> > Prepending,
> > 	str-prop = "1", /previous-value/;
> > 
> > .. and you could do both at once in the obvious way.
> > 
> > The downside, of course, is that this is a much more complicated
> > proposal to implement.  Parsing that syntax isn't too hard, but I
> > think doing it sensibly will need some structural changes in order to
> > evaluate property values as expressions, rather than simply
> > constructing the properties directly left to right.  In particular the
> > interactions between expression syntax and within-property labels (and
> > other markers) could be fiddly to get right.
> > 
> 
> Do you wish to allow `/previous-value/` to be repeated in the same property?
> Something like this:
> 
>     str-prop = "1", /previous-value/, "2", /previous-value/, "3";

Yes, I'd expect that to work.  Note that I don't particularly like the
name "/previous-value/" - that was just an example to demonstrate what
I had in mind.  If anyone can think of a better or more succinct name
for this, please suggest it.

-- 
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 v3 0/2] Add capability to append to property

[Geert Uytterhoeven]

On Thu, Dec 26, 2024 at 7:54 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Dec 20, 2024 at 09:00:14PM +0530, Ayush Singh wrote:
> > On 16/12/24 11:39, David Gibson wrote:
> > > On Tue, Dec 10, 2024 at 04:34:17PM +0100, Andreas Gnau wrote:
> > > > If we add /append-property/, why not add /prepend-property/ as well? This is
> > > > not only "nice for consistency", but it would also enable solving a problem
> > > > where SoC compatible strings from dtsi [1] need to be repeated in board dts
> > > > [2], because one cannot prepend to an existing property.
> > > >
> > > > ```
> > > > dts-v1/;
> > > > / {
> > > >   compatible = "soc-vendor,soc1234";
> > > > };
> > > >
> > > > / {
> > > >   /prepend-property/ compatible = "board-vendor,board-xyz";
> > > > };
> > > > ```
> > > >
> > > > What do you think?
> > > So, this kind of demonstrates why I don't love /append-property/ as a
> > > proposed syntax.  The way I'd prefer to do this, ideally, is to allow
> > > properties to be described as expressions.  We already have integer
> > > expressions that can be used in < > context, but I had intended to
> > > extent to string and bytestring expressions - but I've never had the
> > > time to implement that.
> > >
> > > Under that proposal, I'd expect appending to look something like:
> > >     str-prop = /previous-value/, "1";
> > >
> > > Prepending,
> > >     str-prop = "1", /previous-value/;
> > >
> > > .. and you could do both at once in the obvious way.
> > >
> > > The downside, of course, is that this is a much more complicated
> > > proposal to implement.  Parsing that syntax isn't too hard, but I
> > > think doing it sensibly will need some structural changes in order to
> > > evaluate property values as expressions, rather than simply
> > > constructing the properties directly left to right.  In particular the
> > > interactions between expression syntax and within-property labels (and
> > > other markers) could be fiddly to get right.
> >
> > Do you wish to allow `/previous-value/` to be repeated in the same property?
> > Something like this:
> >
> >     str-prop = "1", /previous-value/, "2", /previous-value/, "3";
>
> Yes, I'd expect that to work.  Note that I don't particularly like the
> name "/previous-value/" - that was just an example to demonstrate what
> I had in mind.  If anyone can think of a better or more succinct name
> for this, please suggest it.

Something like "/./"? (dot is also used in assembler for the current address)
So e.g.

    str-prop = "1", /./, "2", /./, "3";

Or would that be too short? "/orig/"?

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 v3 0/2] Add capability to append to property

[David Gibson]

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

On Thu, Dec 26, 2024 at 12:10:04PM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 26, 2024 at 7:54 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Fri, Dec 20, 2024 at 09:00:14PM +0530, Ayush Singh wrote:
> > > On 16/12/24 11:39, David Gibson wrote:
> > > > On Tue, Dec 10, 2024 at 04:34:17PM +0100, Andreas Gnau wrote:
> > > > > If we add /append-property/, why not add /prepend-property/ as well? This is
> > > > > not only "nice for consistency", but it would also enable solving a problem
> > > > > where SoC compatible strings from dtsi [1] need to be repeated in board dts
> > > > > [2], because one cannot prepend to an existing property.
> > > > >
> > > > > ```
> > > > > dts-v1/;
> > > > > / {
> > > > >   compatible = "soc-vendor,soc1234";
> > > > > };
> > > > >
> > > > > / {
> > > > >   /prepend-property/ compatible = "board-vendor,board-xyz";
> > > > > };
> > > > > ```
> > > > >
> > > > > What do you think?
> > > > So, this kind of demonstrates why I don't love /append-property/ as a
> > > > proposed syntax.  The way I'd prefer to do this, ideally, is to allow
> > > > properties to be described as expressions.  We already have integer
> > > > expressions that can be used in < > context, but I had intended to
> > > > extent to string and bytestring expressions - but I've never had the
> > > > time to implement that.
> > > >
> > > > Under that proposal, I'd expect appending to look something like:
> > > >     str-prop = /previous-value/, "1";
> > > >
> > > > Prepending,
> > > >     str-prop = "1", /previous-value/;
> > > >
> > > > .. and you could do both at once in the obvious way.
> > > >
> > > > The downside, of course, is that this is a much more complicated
> > > > proposal to implement.  Parsing that syntax isn't too hard, but I
> > > > think doing it sensibly will need some structural changes in order to
> > > > evaluate property values as expressions, rather than simply
> > > > constructing the properties directly left to right.  In particular the
> > > > interactions between expression syntax and within-property labels (and
> > > > other markers) could be fiddly to get right.
> > >
> > > Do you wish to allow `/previous-value/` to be repeated in the same property?
> > > Something like this:
> > >
> > >     str-prop = "1", /previous-value/, "2", /previous-value/, "3";
> >
> > Yes, I'd expect that to work.  Note that I don't particularly like the
> > name "/previous-value/" - that was just an example to demonstrate what
> > I had in mind.  If anyone can think of a better or more succinct name
> > for this, please suggest it.
> 
> Something like "/./"? (dot is also used in assembler for the current address)
> So e.g.
> 
>     str-prop = "1", /./, "2", /./, "3";

/./ might work.

> Or would that be too short? "/orig/"?

It's not too short but I think "/orig/" is a bad idea. In cases where
the value of a property is modified more than twice, this would refer
to the most recent previous value, not the "original" / first value
assigned.  "/prev/" might work, though.

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