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@quicinc.com
.../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@quicinc.com
+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-arrayWhy 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