Re: [PATCH v4] kbuild: Add support for DT binding schema checks

[Rob Herring]

On Mon, Jan 28, 2019 at 2:59 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> On Sun, Jan 27, 2019 at 4:00 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> > On Wed, Jan 23, 2019 at 9:33 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Dec 11, 2018 at 9:24 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > > > This adds the build infrastructure for checking DT binding schema
> > > > documents and validating dts files using the binding schema.
> > > >
> > > > Check DT binding schema documents:
> > > > make dt_binding_check
> > > >
> > > > Build dts files and check using DT binding schema:
> > > > make dtbs_check
> > > >
> > > > Optionally, DT_SCHEMA_FILES can be passed in with a schema file(s) to
> > > > use for validation. This makes it easier to find and fix errors
> > > > generated by a specific schema.
> > > >
> > > > Currently, the validation targets are separate from a normal build to
> > > > avoid a hard dependency on the external DT schema project and because
> > > > there are lots of warnings generated.
> > >
> > > Thanks, I'm giving this a try, and get errors like:
> > >
> > >       DTC     arch/arm/boot/dts/emev2-kzm9d.dt.yaml
> > >     FATAL ERROR: No markers present in property 'cpu0' value
> > >
> > > and
> > >
> > >       DTC     arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dt.yaml
> > >     FATAL ERROR: No markers present in property 'audio_clk_a' value
> > >
> > > Do you have a clue?
> >
> > That's really strange because those aren't even properties. Are other
> > dts files okay? This is the in tree dtc?
> >
> > The only time you should be missing markers is if you did a dts -> dts
> > -> dt.yaml.
>
> Found it: make dtbs_check doesn't play well with my local change to
> add symbols for DT overlays:

Now that makes sense.

>     --- a/scripts/Makefile.lib
>     +++ b/scripts/Makefile.lib
>     @@ -285,6 +285,10 @@ cmd_dt_S_dtb=
>              \
>      $(obj)/%.dtb.S: $(obj)/%.dtb FORCE
>             $(call if_changed,dt_S_dtb)
>
>     +ifeq ($(CONFIG_OF_OVERLAY),y)
>     +DTC_FLAGS += -@
>     +endif
>     +
>      quiet_cmd_dtc = DTC     $@
>      cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>             $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o
> $(dtc-tmp) $< ; \
>
> Do you see a way to handle that better?

We need to have the code that generates these properties to also add
markers. Or we could drop the __symbols__ nodes on YAML output. Or
ignore the option when doing YAML output.

> Apart from a few expected issues, I'm seeing one other strange message:
>
>     arch/arm/boot/dts/sh73a0-kzm9g.dt.yaml: interrupts: [[2, 4], [3,
> 4]] is too long
>
> This is the interrupts property in the adi,adxl345 node in
> arch/arm/boot/dts/sh73a0-kzm9g.dts.
> Apparently the check complains if more than one interrupt is listed here.
> Is this a known issue?

There are lots of warnings... I've gone thru and checked some to make
sure they are valid, but certainly not all. There's probably some
cases that are too strict too.

This one is because this device is listed in trivial-devices.yaml and
you can't have 2 interrupts for a trivial device (because you need to
define the interrupt order). Looks like we have a binding doc for it
too, so we should just remove it from trivial-devices.yaml.

There are lots of '... is too (long|short)' messages because of how
the dts file property values are bracketed. This used to not matter,
but is significant in the YAML output. I have a dtc patch to give
warnings on all these (and dtc will give source location).

Rob