Re: [PATCH 11/13] dt-bindings: serial: restrict possible child node names

From: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: - <devicetree@vger.kernel.org>,
	linux-riscv@lists.infradead.org,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Tomer Maimon <tmaimon77@gmail.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Peter Korsgaard <jacmet@sunsite.dk>, Chester Lin <clin@suse.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	linux-stm32@st-md-mailman.stormreply.com,
	Kevin Hilman <khilman@baylibre.com>,
	linux-kernel@vger.kernel.org, Fugang Duan <fugang.duan@nxp.com>,
	Shawn Guo <shawnguo@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-serial@vger.kernel.org,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Fabio Estevam <festevam@gmail.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Le Ray <erwan.leray@foss.st.com>,
	Michal Simek <michal.simek@xilinx.com>,
	Lubomir Rintel <lkundrak@v3.sk>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Magnus Damm <magnus.damm@gmail.com>,
	Pragnesh Patel <pragnesh.patel@sifive.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-amlogic@lists.infradead.org,
	NXP Linux Team <linux-imx@nxp.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 11/13] dt-bindings: serial: restrict possible child node names
Date: Wed, 25 Jan 2023 11:11:21 -0600	[thread overview]
Message-ID: <CAL_JsqK7ZYe+PMXZ34WG746JzgGvaLanRpwF2x3sSGdsA7PCSA@mail.gmail.com> (raw)
In-Reply-To: <71fd9674-63a7-9887-d602-1f57ae3982aa@linaro.org>

On Tue, Jan 24, 2023 at 3:05 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/01/2023 00:05, Rob Herring wrote:
> >
> > On Mon, 23 Jan 2023 16:13:00 +0100, Krzysztof Kozlowski wrote:
> >> The re-usable serial.yaml schema matches every property with ".*"
> >> pattern, thus any other schema referencing it will not report unknown
> >> (unevaluated) properties.  This hides several wrong properties.  It is
> >> a limitation of dtschema, thus provide a simple workaround: expect
> >> children to be only of few names matching upstream usage (Bluetooth,
> >> GNSS, GPS and MCU).
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/serial/serial.yaml | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/bus/intel,ixp4xx-expansion-bus-controller.example.dtb: serial@1,0: Unevaluated properties are not allowed ('intel,ixp4xx-eb-byte-access', 'intel,ixp4xx-eb-cycle-type', 'intel,ixp4xx-eb-t3', 'intel,ixp4xx-eb-write-enable' were unexpected)
> >       From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/8250.yaml
>
> Eh, this is not trivial to solve. The
> "intel,ixp4xx-expansion-bus-controller.yaml" bindings add properties to
> children nodes, just like spi-peripheral-props:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/bus/intel,ixp4xx-expansion-bus-controller.yaml?h=v6.2-rc5#n147

Yep, those properties probably need to go into mc-peripheral-props.yaml.

> Any node can be there:
> 1. serial, which was broken since beginning but errors not visible,

8250 only in all likelyhood.

> 2. flash, which work fine just because mtd-physmap.yaml allows it,
> 3. more?

I guess it is *only* external parallel bus devices...

>
> Existing DTS of this ixp4xx controller have different nodes, for example:
> flash, shared-dma-pool, ns8250, arcom,vulcan-gpio, maxim,max6369:

'shared-dma-pool' is odd.

So is max6369. Looks like that h/w was creatively attached.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/intel-ixp42x-arcom-vulcan.dts?h=v6.2-rc5#n40
>
> Probably we need expansion-bus-controller-peripheral-props.yaml and
> include it in every possible child. Does it sound reasonable?

What's the difference to mc-peripheral-props.yaml?

Honestly, this stuff is somewhat rare that I'd rather worry about it
later. Maybe ixp4xx will be removed by then... We could just comment
out the problematic parts of the example for now.

Rob