Re: [PATCH v2 3/4] dtc: Add /./

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Mar 11, 2025 at 09:05:39PM +0530, Ayush Singh wrote:
> Allow constructing new values for a property using old property values.
> Can be used to append, pre-append, duplicate, etc.
> 
> In practice, it looks as follows:
> 
> dts-v1/;
> 
> / {
> 	str-prop = "0";
> 	int-prop = <0>;
> };
> 
> / {
> 	str-prop = /./, "1", /./;
> 	int-prop = /./, <1>, /./;
> };
> 
> dts to source output with -T -T also works as expected:
> 
> /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 */
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  dtc-lexer.l  |  5 +++++
>  dtc-parser.y |  5 +++++
>  dtc.h        |  1 +
>  livetree.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 59 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

I think DT_PREV_VALUE would be a better name.  "prev prop" suggests
the property defined immediately above, rather than the previous value
of this property.

>  %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..22d45ca90bec8971d02231787217deb7caf53310 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -139,10 +139,40 @@ struct node *reference_node(struct node *node)
>  	return node;
>  }
>  
> +static struct data data_insert_old_value(struct data d, struct marker *m,
> +					 struct data old)

Nothing in this function requires 'old' to be the previous value of
the property.  So 'data_insert_data()' would be a better name.  It's
also strictly about manipulating a struct data so it should go in
data.c, not livetree.c.

> +{
> +	unsigned int offset = m->offset;
> +	struct marker *next = m->next;
> +	struct marker *marker;
> +	struct data new_data;
> +	char *ref;
> +
> +	new_data = data_insert_at_marker(d, m, old.val, old.len);
> +
> +	/* Copy all markers from old value */
> +	marker = old.markers;
> +	for_each_marker(marker) {
> +		ref = NULL;
> +
> +		if (marker->ref)
> +			ref = xstrdup(marker->ref);
> +
> +		m->next = alloc_marker(marker->offset + offset, marker->type,
> +				       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;

This is logically per-property, not per-node.  So it needs to be
initialised to false for each property, not just once per node.  So it
might as well be local to the first while loop as well.

> +	struct marker *marker;
>  	struct label *l;
>  
>  	old_node->deleted = 0;
> @@ -172,10 +202,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 {
> +					srcpos_free(old_prop->srcpos);
> +					old_prop->srcpos = new_prop->srcpos;
> +				}
> +
>  				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 --]