Hi Conor, Thanks for taking the time to look at the patch. On Tue, May 21, 2024 at 08:21:45PM +0100, Conor Dooley wrote: > On Tue, May 21, 2024 at 11:37:59AM -0700, Elliot Berman wrote: > > Device manufcturers frequently ship multiple boards or SKUs under a > > single softwre package. These software packages ship multiple devicetree > > blobs and require some mechanims to pick the correct DTB for the boards > > that use the software package. > > Okay, you've got the problem statement here, nice. > > > This patch introduces a common language > > for adding board identifiers to devicetrees. > > But then a completely useless remainder of the commit message. > I open this patch, see the regexes, say "wtf", look at the commit > message and there is absolutely no explanation of what these properties > are for. That's quite frankly just not good enough - even for an RFC. > Understood, I've been trying to walk the line of getting the idea across to have conversation about the board-ids, while not getting into too much of the weeds. I was hoping the example and the matching code in the first patch would get enough of the idea across, but I totally empathize that might not be enough. I'll reply here shortly with a version of this patch which adds more details. > > > > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx> > > --- > > .../devicetree/bindings/board/board-id.yaml | 24 ++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/board/board-id.yaml b/Documentation/devicetree/bindings/board/board-id.yaml > > new file mode 100644 > > index 000000000000..99514aef9718 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/board/board-id.yaml > > @@ -0,0 +1,24 @@ > > +# SPDX-License-Identifier: BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/board/board-id.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: board identifiers > > +description: Common property for board-id subnode > > s/property/properties/ > > > + > > +maintainers: > > + - Elliot Berman <quic_eberman@xxxxxxxxxxx> > > + > > +properties: > > + $nodename: > > + const: '/' > > + board-id: > > + type: object > > + patternProperties: > > + "^.*(?!_str)$": > > Does this regex even work? Take "foo_str" as an example - doesn't "^.*" > consume all of the string, leaving the negative lookahead with nothing > to object to? I didn't properly test this with an example and the dt > tooling, but I lazily threw it into regex101 and both the python and > emcascript versions agree with me. Did you test this? Right, it should be a lookbehind, not a lookahead. > > And while I am here, no underscores in property names please. And if > "str" means string, I suggest not saving 3 characters. > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > + "^.*_str$": > > + $ref: /schemas/types.yaml#/definitions/string-array > > Why do we even need two methods? Commit message tells me nothing and > there's no description at all... Why do we need regexes here, rather > than explicitly defined properties? Your commit message should explain > the justification for that and the property descriptions (as comments if > needs be for patternProperties) should explain why this is intended to > be used. > > How is anyone supposed to look at this binding and understand how it > should be used? I was thinking that firmware may only provide the data without being able to provide the context whether the value is a number or a string. I think this is posisble if firmware provides the device's board identifier in the format of a DT itself. It seems natural to me in the EBBR flow. There is example of this in example in patches 3 (fdt-select-board) and 9 (the test suite). DTB doesn't inherently provide instruction on how to interpret a property's value, so I created a rule that strings have to be suffixed with "-string". One other note -- I (QCOM) don't currently have a need for board-ids to be strings. I thought it was likely that someone might want that though. Thanks, Elliot
