On Mon, Dec 10, 2018 at 10:39 PM Masahiro Yamada
[off-list ref] wrote:

Hi Rob,

On Tue, Dec 11, 2018 at 5:50 AM Rob Herring [off-list ref] wrote:

quoted

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" ?

Yes.

[...]

quoted

+quiet_cmd_chk_binding = CHKDT   $<

In convention, the short log displays the target name
instead of the prerequisite.

Yes, but here there is no target. We're just running a check on the source.

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.

I've changed it to this:

quiet_cmd_chk_binding = CHKDT   $(patsubst $(srctree)/%,%,$<)

[...]

quoted

+$(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?

Nothing really. Just named it something so it gets cleaned and ignored by git.

quoted

+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?

It is for error case when the output file is not generated. I can
handle this within dt-mk-schema instead.

quoted

+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.

quoted

+$(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.

It is enforcing the ordering. Without it, the binding checks and
building .schema.yaml.tmp happen in parallel because both only have
the source files as dependencies. The '|' keeps the dependencies out
of the dependency list($^).

[...]

quoted

+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?

Looks good.

 ifeq ($(wildcard /usr/include/yaml.h),)
+ifeq ($(CHECK_DTBS),1)

I'm going to change this to "ifneq ($CHECK_DTBS),)" in case we start
supporting more than 1 value such as warning levels.

+$(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.

Sadly, that's probably true.

Thanks for your thorough review.

Rob