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

[Simon Glass <sjg@chromium.org>]

Hi Ayush,

On Tue, 27 Aug 2024 at 11:55, Ayush Singh <ayush@beagleboard.org> 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.
>
> [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>
> ---
>  dtc-lexer.l  |  7 +++++++
>  dtc-parser.y |  6 ++++++
>  dtc.h        |  3 +++
>  livetree.c   | 30 +++++++++++++++++++++++++-----
>  4 files changed, 41 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

nits below

>
> 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..06771c5 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -205,6 +205,7 @@ struct bus_type {
>
>  struct property {
>         bool deleted;
> +       bool append;

Can you add a comment as to what this means?

>         char *name;
>         struct data val;
>
> @@ -263,6 +264,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..74eca1b 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,21 @@ 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;
> +                               }

Braces are not needed here.

> +
> +                               old_prop->deleted = false;
> +                               old_prop->append = false;
>                                 free(old_prop->srcpos);
>                                 old_prop->srcpos = new_prop->srcpos;
>                                 free(new_prop);
>
> --
> 2.46.0
>
>

Regards,
Simon