Re: [PATCH RFC v3 2/9] dt-bindings: board: Introduce board-id

From: Conor Dooley <conor@kernel.org>
To: Elliot Berman <quic_eberman@quicinc.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Amrit Anand <quic_amrianan@quicinc.com>,
	Peter Griffin <peter.griffin@linaro.org>,
	Caleb Connolly <caleb.connolly@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Doug Anderson <dianders@chromium.org>,
	Simon Glass <sjg@chromium.org>, Chen-Yu Tsai <wenst@chromium.org>,
	Julius Werner <jwerner@chromium.org>,
	"Humphreys, Jonathan" <j-humphreys@ti.com>,
	Sumit Garg <sumit.garg@linaro.org>,
	Jon Hunter <jonathanh@nvidia.org>,
	Michal Simek <michal.simek@amd.com>,
	boot-architecture@lists.linaro.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH RFC v3 2/9] dt-bindings: board: Introduce board-id
Date: Tue, 21 May 2024 20:21:45 +0100	[thread overview]
Message-ID: <20240521-bonfire-backboned-9ef33c10d447@spud> (raw)
In-Reply-To: <20240521-board-ids-v3-2-e6c71d05f4d2@quicinc.com>

[-- Attachment #1.1: Type: text/plain, Size: 2852 bytes --]

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.

> 
> 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?

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?

Utterly lost,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel