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@xxxxxxxxxxxxxxx> 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
