Re: [PATCH] dt-bindings: clk: qcom: Fix self-validation, split, and clean cruft

From: Doug Anderson <dianders@chromium.org>
To: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>, Taniya Das <tdas@codeaurora.org>,
	Jeffrey Hugo <jhugo@codeaurora.org>,
	Abhishek Sahu <absahu@codeaurora.org>,
	sivaprak@codeaurora.org, anusharao@codeaurora.org,
	Sricharan <sricharan@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Andy Gross <agross@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-clk <linux-clk@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: clk: qcom: Fix self-validation, split, and clean cruft
Date: Wed, 29 Jan 2020 16:24:10 -0800	[thread overview]
Message-ID: <CAD=FV=Uv-td=PhCSZNsjBB-cQB=vJKLbw_BLbM3B1ORRzuTB5A@mail.gmail.com> (raw)
In-Reply-To: <CAL_JsqLVaJMidm2QcpmxXeT+Q+uU8esm1shdRs3BVoeRYqhJng@mail.gmail.com>

Hi,

On Wed, Jan 29, 2020 at 3:50 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Jan 29, 2020 at 5:26 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 29, 2020 at 2:01 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Wed, Jan 29, 2020 at 3:23 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > >
> > > > The 'qcom,gcc.yaml' file failed self-validation (dt_binding_check)
> > > > because it required a property to be either (3 entries big),
> > > > (3 entries big), or (7 entries big), but not more than one of those
> > > > things.  That didn't make a ton of sense.
> > > >
> > > > This patch splits all of the exceptional device trees (AKA those that
> > > > would have needed if/then/else rules) from qcom,gcc.yaml.  It also
> > > > cleans up some cruft found while doing that.
> > > >
> > > > After this lands, this worked for me atop clk-next:
> > > >   for f in \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-apq8064.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-ipq8074.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-msm8996.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-msm8998.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-qcs404.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-sc7180.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml \
> > > >     Documentation/devicetree/bindings/clock/qcom,gcc.yaml; do \
> > > >       ARCH=arm64 make dt_binding_check DT_SCHEMA_FILES=$f; \
> > > >       ARCH=arm64 make dtbs_check DT_SCHEMA_FILES=$f; \
> > > >   done
> > >
> > > Note that using DT_SCHEMA_FILES may hide some errors in examples as
> > > all other schemas (including the core ones) are not used for
> > > validation. So just 'make dt_binding_check' needs to pass (ignoring
> > > any other unrelated errors as it breaks frequently). Supposedly a
> > > patch is coming explaining this in the documentation.
> >
> > That seems like it's going to be a huge pain going forward, but OK.
>
> Use of DT_SCHEMA_FILES hiding problems or having to run 'make
> dt_binding_check' on everything?

Having to run 'make dt_binding_check' on everything.  I guess maybe if
the tree stays clean it won't be too bad and it wasn't too bad against
the current linux-next, but I can imagine that every time I want to
run this I'll run into a pile of warnings / errors in other people's
files.  Then I need to figure out what to ignore / workaround.  If
something fails badly (like intel-gw-pcie.yaml) I'll have to realize
that I should just delete that file to get the rest of the run to
report errors that are relevant to me.

This'll probably be worse because most maintainer trees are based on
"-rc1" and at least in the past I've found that "-rc1" tends to have
lots of problems.  Each maintainer then fixes the problems relative to
their own subsystem, but it's not a wonderful thing to rely on.

> I could probably rework things such that you can check a single
> binding example against all schema, but dtbs still get validated by
> just a single schema.

This would be helpful.  ...or some way to easily make really bad
failures non-fatal.  Then I can maybe just diff the results before my
patch and after and that'll give me a hint of what I've fixed / made
worse.

> Probably is. There are cases where a new schema breaks another file's
> example. If someone has a gcc node in another example for example.

At this point I'm going to say that we're better off than we were, but
I'll try to keep this in mind for future patches.

-Doug