Re: [PATCH v3] kbuild: Add support for DT binding schema checks
From: Masahiro Yamada
Date: Mon Dec 10 2018 - 23:39:27 EST
Hi Rob,
On Tue, Dec 11, 2018 at 5:50 AM 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 passed in with a schema file(s) to use
Perhaps, "can be passed" ?
> 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.
>
> Cc: Jonathan Corbet <corbet@xxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> Cc: Michal Marek <michal.lkml@xxxxxxxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: linux-kbuild@xxxxxxxxxxxxxxx
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> v3:
> - Fix error causing only 1st schema file to get used.
> - Add a more useful error message when dtc is missing YAML support
> telling the user they need to install libyaml devel package.
>
>
> .gitignore | 1 +
> Documentation/Makefile | 2 +-
> Documentation/devicetree/bindings/.gitignore | 1 +
> Documentation/devicetree/bindings/Makefile | 33 +++++++++++++++++
> Makefile | 11 ++++--
> scripts/Makefile.lib | 37 ++++++++++++++++++--
> 6 files changed, 80 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/.gitignore
> create mode 100644 Documentation/devicetree/bindings/Makefile
>
> diff --git a/.gitignore b/.gitignore
> index 97ba6b79834c..a20ac26aa2f5 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -15,6 +15,7 @@
> *.bin
> *.bz2
> *.c.[012]*.*
> +*.dt.yaml
> *.dtb
> *.dtb.S
> *.dwo
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 2ca77ad0f238..9786957c6a35 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for Sphinx documentation
> #
>
> -subdir-y :=
> +subdir-y := devicetree/bindings/
>
> # You can set these variables from the command line.
> SPHINXBUILD = sphinx-build
> diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
> new file mode 100644
> index 000000000000..d9194c02dd08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/.gitignore
> @@ -0,0 +1 @@
> +*.example.dts
> diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
> new file mode 100644
> index 000000000000..43f8657ab070
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: GPL-2.0
> +DT_DOC_CHECKER ?= dt-doc-validate
> +DT_EXTRACT_EX ?= dt-extract-example
> +DT_MK_SCHEMA ?= dt-mk-schema
> +DT_MK_SCHEMA_FLAGS := $(if $(DT_SCHEMA_FILES), -u)
> +
> +quiet_cmd_chk_binding = CHKDT $<
In convention, the short log displays the target name
instead of the prerequisite.
If O=foo/bar is given, $< shows the full path,
then log will look like follows:
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/calxeda.yaml
DTC Documentation/devicetree/bindings/arm/calxeda.example.dtb
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/qcom.yaml
DTC Documentation/devicetree/bindings/arm/qcom.example.dtb
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/xilinx.yaml
DTC Documentation/devicetree/bindings/arm/xilinx.example.dtb
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/nspire.yaml
DTC Documentation/devicetree/bindings/arm/ti/nspire.example.dtb
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/ti,davinci.yaml
DTC Documentation/devicetree/bindings/arm/ti/ti,davinci.example.dtb
CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/altera.yaml
You might think it is ugly.
> + cmd_chk_binding = (set -e; \
> + $(DT_DOC_CHECKER) $< ; \
> + mkdir -p $(dir $@) ; \
> + $(DT_EXTRACT_EX) $< > $@ )
- 'set -e' is redundant because if_changed already has it.
- 'mkdir mkdir -p $(dir $@)' is also redundant because
scripts/Makefile.build automatically creates it.
- You do not need to execute this in a sub-shell
You can simplify like this:
cmd_chk_binding = $(DT_DOC_CHECKER) $< ; \
$(DT_EXTRACT_EX) $< > $@
> +$(obj)/%.example.dts: $(src)/%.yaml FORCE
> + $(call if_changed,chk_binding)
> +
> +DT_TMP_SCHEMA := .schema.yaml.tmp
BTW, why does this file start with a period?
What is the meaning of '.tmp' extension?
> +extra-y += $(DT_TMP_SCHEMA)
> +
> +quiet_cmd_mk_schema = SCHEMA $@
> + cmd_mk_schema = mkdir -p $(obj); \
> + rm -f $@; \
> + $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(filter-out FORCE, $^)
"mkdir -p $(obj)" is redundant.
Why is 'rm -f $@' necessary ?
Can't dt-mk-schema overwrite the output file?
> +DT_DOCS = $(shell cd $(srctree)/$(src) && find * -name '*.yaml')
> +DT_SCHEMA_FILES ?= $(addprefix $(src)/,$(DT_DOCS))
> +
> +extra-y += $(patsubst $(src)/%.yaml,%.example.dts, $(DT_SCHEMA_FILES))
> +extra-y += $(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES))
I assume you intentionally did not do like this:
extra-y += $(patsubst %.yaml,%.example.dtb, $(DT_DOCS))
>From the commit description, DT_SCHEMA_FILES might be overridden by a user.
So, I think this is OK.
> +$(obj)/$(DT_TMP_SCHEMA): | $(addprefix $(obj)/,$(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES)))
I do not understand this line.
Why is it necessary?
*.example.dtb files are generated anyway
since they are listed in extra-y.
> +$(obj)/$(DT_TMP_SCHEMA): $(addprefix $(srctree)/, $(DT_SCHEMA_FILES)) FORCE
> + $(call if_changed,mk_schema)
You do not need to prefix $(srctree)/ because Kbuild uses VPATH.
$(obj)/$(DT_TMP_SCHEMA): $(DT_SCHEMA_FILES) FORCE
$(call if_changed,mk_schema)
is fine.
> +cmd_dtc_yaml_chk = \
> + if ! echo "/dts-v1/; /{};" | $(DTC) -I dts -O yaml > /dev/null ; then \
> + echo "*"; \
> + echo "* dtc needs libyaml for DT schema validation support."; \
> + echo "* Install the necessary libyaml development package from your distro."; \
> + echo "*"; \
> + false; \
> + fi; \
> + touch $@
> +
> +$(objtree)/scripts/dtc/.dtc-yaml-chk.tmp: $(DTC) FORCE
> + $(call if_changed,dtc_yaml_chk)
Hmm, this is kind of ugly.
I'd rather check this in scripts/dtc/Makefile
How about something like below?
diff --git a/Makefile b/Makefile
index ff59adf..a3e2db2 100644
--- a/Makefile
+++ b/Makefile
@@ -1233,11 +1233,11 @@ ifneq ($(dtstree),)
$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
PHONY += dtbs dtbs_install dt_binding_check
-dtbs: prepare3 scripts_dtc
+dtbs dtbs_check: prepare3 scripts_dtc
$(Q)$(MAKE) $(build)=$(dtstree)
-dtbs_check: prepare3 dt_binding_check
- $(Q)$(MAKE) $(build)=$(dtstree) CHECK_DTBS=1
+dtbs_check: export CHECK_DTBS=1
+dtbs_check: dt_binding_check
dtbs_install:
$(Q)$(MAKE) $(dtbinst)=$(dtstree)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 8a7d622..5017175 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -309,20 +309,7 @@ define rule_dtc_dt_yaml
$(call echo-cmd,dtb_check) $(cmd_dtb_check)
endef
-cmd_dtc_yaml_chk = \
- if ! echo "/dts-v1/; /{};" | $(DTC) -I dts -O yaml > /dev/null ; then \
- echo "*"; \
- echo "* dtc needs libyaml for DT schema validation support."; \
- echo "* Install the necessary libyaml development
package from your distro."; \
- echo "*"; \
- false; \
- fi; \
- touch $@
-
-$(objtree)/scripts/dtc/.dtc-yaml-chk.tmp: $(DTC) FORCE
- $(call if_changed,dtc_yaml_chk)
-
-$(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE |
$(objtree)/scripts/dtc/.dtc-yaml-chk.tmp
+$(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
$(call if_changed_rule,dtc_dt_yaml)
dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)
diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index 056d5da..3e497f1 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -12,6 +12,10 @@ dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o
HOST_EXTRACFLAGS := -I$(src)/libfdt
ifeq ($(wildcard /usr/include/yaml.h),)
+ifeq ($(CHECK_DTBS),1)
+$(error dtc needs libyaml for DT schema validation support. \
+ Install the necessary libyaml development package.)
+endif
HOST_EXTRACFLAGS += -DNO_YAML
else
dtc-objs += yamltree.o
One drawback of this approach is,
this is not checked if you are using out-of-tree DTC.
Probably, Rob is the only person that overrides DTC.
--
Best Regards
Masahiro Yamada