Received-SPF: pass (google.com: domain of linux-kernel+bounces-185483-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240521-board-ids-v3-0-e6c71d05f4d2@quicinc.com>
 <20240521-board-ids-v3-2-e6c71d05f4d2@quicinc.com> <20240521-bonfire-backboned-9ef33c10d447@spud>
 <20240521-unskilled-spindle-c4161092ae83@spud>
In-Reply-To: <20240521-unskilled-spindle-c4161092ae83@spud>
From: Rob Herring <robh+dt@kernel.org>
Date: Tue, 21 May 2024 16:32:16 -0500
Message-ID: <CAL_JsqJjvNfr_Vo7JNsBn6Fcv5r--ZqQTiforSLwrZ-z4_mURQ@mail.gmail.com>
Subject: Re: [PATCH RFC v3 2/9] dt-bindings: board: Introduce board-id
To: Conor Dooley <conor@kernel.org>
Cc: Elliot Berman <quic_eberman@quicinc.com>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 21, 2024 at 2:25=E2=80=AFPM Conor Dooley <conor@kernel.org> wro=
te:
>
> 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 devicet=
ree
> > > blobs and require some mechanims to pick the correct DTB for the boar=
ds
> > > 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 i=
f
> > 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?
>
> Also, please do not CC private mailing lists on your postings, I do not
> want to get spammed by linaro's mailman :(

boot-architecture is not private[0]. It is where EBBR gets discussed
amongst other things. This came up in a thread there[1].

Rob

[0] https://lists.linaro.org/mailman3/lists/boot-architecture.lists.linaro.=
org/
[1] https://lists.linaro.org/archives/list/boot-architecture@lists.linaro.o=
rg/thread/DZCZSOCRH5BN7YOXEL2OQKSDIY7DCW2M/