Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scripts: edtlib: Automatically include base.yaml in all bindings #19846

Closed
wants to merge 3 commits into from

Conversation

ulfalizer
Copy link
Collaborator

@ulfalizer ulfalizer commented Oct 15, 2019

Main commits:

scripts: edtlib: Automatically include base.yaml in all bindings

This removes some boilerplate from bindings and makes things more
consistent. One downside is that all properties declared in base.yaml
can now appear on all nodes, including on nodes where they don't make
sense, but it's probably worth it.

base.yaml is moved to scripts/dts/base.yaml, and is considered part of
edtlib internals.

The automatic base.yaml inclusion is documented in
dts/binding-template.yaml and in the legacy syntax section of
doc/guides/dts/index.rst.

'include: base.yaml' is still accepted, but is ignored, and makes edtlib
print a deprecation warning. The warning mentions the new location of
base.yaml.

Remove a 'clocks' declaration from dts/bindings/clock/fixed-clock.yaml,
which declares it as 'type: array'. It clashes with the declaration for
'clocks' in base.yaml, which has it as 'type: phandle-array'. No 'clocks
= <(number) (number) ...>' assignments exist anywhere, which is the form
of assignment expected for 'type: array'.

Piggyback some binding consistency nits and a small bugfix that allows
empty files to be included (the crux is that PyYAML returns None instead
of an empty dict for them).

The empty file fix is needed because of dts/bindings/mmc/mmc.yaml, which
turns empty besides some comments if the 'include: base.yaml' is
removed. Maybe it should be removed instead.
dts: bindings: Remove base.yaml includes

base.yaml is now included automatically, so it doesn't need to be
included explicitly.

Piggybacked nit:

scripts: edtlib: Fix some *-map-mask/*-map-pass-thru typos in comments

The "map" part got lost.

@ulfalizer ulfalizer added the DNM This PR should not be merged (Do Not Merge) label Oct 15, 2019
@ulfalizer ulfalizer changed the title [wip] dts: bindings: Automatically include base.yaml in all bindings [wip] scripts: edtlib: Automatically include base.yaml in all bindings Oct 15, 2019
@ulfalizer ulfalizer force-pushed the dts-auto-base branch 2 times, most recently from 1619099 to af2b2fb Compare October 16, 2019 00:07
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 16, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@erwango
Copy link
Member

erwango commented Oct 16, 2019

@ulfalizer, I understand the interest but I'm afraid this could confuse people. One advantage of current inclusion of base.yaml is that it makes script logic vs bindings quite transparent to users.
If we're doing this change, would be nice to keep a clear enought comment in all bindings to make base.yaml inclusion less opaque.

@ulfalizer ulfalizer force-pushed the dts-auto-base branch 4 times, most recently from 1703632 to ecf64d9 Compare October 16, 2019 12:55
@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Oct 16, 2019

@ulfalizer, I understand the interest but I'm afraid this could confuse people. One advantage of current inclusion of base.yaml is that it makes script logic vs bindings quite transparent to users.

Yeah, it needs to be well-documented and discoverable at least.

I've now documented it in dts/binding-template.yaml and in the legacy syntax section of doc/guides/dts/index.rst. I also added a warning to edtlib.py that's printed if base.yaml is included:

for fname in fnames:
    if fname == "base.yaml":
        self._warn(
            "'include: base.yaml' in {} is deprecated and redundant. "
            "base.yaml is now automatically included in all bindings, "
            "and can be found at scripts/dts/base.yaml."
            .format(binding_path))
        continue

I wonder if it could be mentioned elsewhere too.

If we're doing this change, would be nice to keep a clear enought comment in all bindings to make base.yaml inclusion less opaque.

I wonder if the added documentation and warning might make it easy enough to figure out. Feels a bit spammy to add 2-3 lines to every binding, especially once you already know it.

@ulfalizer ulfalizer changed the title [wip] scripts: edtlib: Automatically include base.yaml in all bindings scripts: edtlib: Automatically include base.yaml in all bindings Oct 16, 2019
@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Oct 17, 2019

I'm not sure I think this makes sense. I get the value in reducing the need to include base.yaml everywhere, but I think its better to be explicit vs having an implied "inclusion".

One nice thing about it besides shortening bindings is that it forces consistency. If someone tries to add a property to a binding that clashes with something in base.yaml, it will be an error, even if they forget to include base.yaml. That revealed an issue in fixed-clock.yaml in this PR, for example.

Some other systems (e.g. bitbake and dt-schema) also have base configuration files that are included implicitly.

It makes things slightly less transparent, but I think people will pick up on it pretty quickly.

@ulfalizer
Copy link
Collaborator Author

Should also speed up binding parsing a bit btw, because base.yaml is only parsed once now.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

ulfalizer added a commit to ulfalizer/zephyr that referenced this pull request Oct 30, 2019
Explain how 'include:' works in some more detail and mention base.yaml,
along with an example of how it can be used.

This was adapted from
zephyrproject-rtos#19846.

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
galak pushed a commit that referenced this pull request Nov 5, 2019
Explain how 'include:' works in some more detail and mention base.yaml,
along with an example of how it can be used.

This was adapted from
#19846.

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
@galak galak added area: Devicetree Tooling PR modifies or adds a Device Tree tooling area: Devicetree Binding PR modifies or adds a Device Tree binding and removed area: Devicetree labels Jan 30, 2020
@erwango
Copy link
Member

erwango commented May 4, 2020

@galak, should we keep this one ?

@github-actions github-actions bot added area: Devicetree has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree labels Jun 29, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR platform: NXP NXP and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020
@galak galak closed this Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree Tooling PR modifies or adds a Device Tree tooling area: Devicetree area: Documentation has-conflicts Issue/PR has conflicts with another issue/PR platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants