Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Nov 2, 2019

This is a step towards #19904. The motivation is removing redundant bindings from dts/ that aren't associated with their own driver implementation, and eliminating the annoying checkpatch warnings when an unspecified compatible is added in a commit.

Knowledge of the variants may also assist in addressing #20287.

@pabigot pabigot requested review from galak and ulfalizer November 2, 2019 12:31
@pabigot pabigot force-pushed the dts/20191102b branch 2 times, most recently from adf43ef to 0340856 Compare November 2, 2019 12:33
@zephyrbot
Copy link

zephyrbot commented Nov 2, 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.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Should check the format of variant-compatible too. Search for all(isinstance in edtlib.py for examples of how it's done for other keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we go with this, this ought to explain that variant-compatible isn't used by the code, and is just a "syntaxified comment" in a sense. Otherwise, I'd wonder how it's used.

Think I'm fine with it as long as that's clear. Might help bring some structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Could update the comment later if/when it gets used for other stuff. As long as it's unused, it's nice to document it, to avoid confusion.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could reflow all of ok_top, or put on separate lines if you're worried about history (though I wonder if the vertical spam is worthwhile).

@pabigot pabigot force-pushed the dts/20191102b branch 2 times, most recently from 5856bb5 to 4c5baad Compare November 2, 2019 21:13
@pabigot pabigot marked this pull request as ready for review November 2, 2019 22:31
Copy link
Contributor

Choose a reason for hiding this comment

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

Should verify that it's a list as well, or it'll pass for variant-compatible: "foo" by iterating over "f", "o", "o".

Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe put it outside the loop. Think some other stuff was:

if "variant-compatible" in binding:
    var_comp = binding["variant_compatible"]
    if not (isinstance(var_comp, list) and
            all(isinstance(elm, str) for elm in var_comp)):
        _err("... should be a list of strings")

Copy link
Contributor

Choose a reason for hiding this comment

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

Could put it next to the

if "child-binding" in binding:
    ....

if "sub-node" in binding:
    ...

if "#cells" in binding:
    ...

stuff below.

@pabigot pabigot changed the title [RFC] add binding support for variant compatibles devicetree: add binding support for variant compatibles Nov 3, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to invert the devices in the compatible list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because standard practice is to order them from most-specific to least-specific, selecting the first one that is supported. There is no lsm303agr-magn driver right now, but there is a lis2mdl that works with that device.

Historically Zephyr only paid attention to the first entry, but that was fixed some months ago so we can now be consistent with other devicetree usage.

binding-template specifies the order, but does not explain why it's that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avisconti please revisit this; note @galak's non-verbal support for this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

and here

Copy link
Contributor

Choose a reason for hiding this comment

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

also here

@galak galak requested a review from avisconti November 4, 2019 13:56
Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Accidental two spaces between "expected" and "list".

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Fine with me.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Fine with me.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Fine with me.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Fine with me.

@ulfalizer
Copy link
Contributor

Heh... GitHub bugged out for a moment there.

2 similar comments
@ulfalizer
Copy link
Contributor

Heh... GitHub bugged out for a moment there.

@ulfalizer
Copy link
Contributor

Heh... GitHub bugged out for a moment there.

@galak galak added area: Devicetree Tooling PR modifies or adds a Device Tree tooling and removed area: Devicetree labels Jan 30, 2020
@github-actions github-actions bot added area: Devicetree has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jun 29, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR area: Shields Shields (add-on boards) platform: STM32 ST Micro STM32 and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020
@pabigot
Copy link
Contributor Author

pabigot commented Jul 20, 2020

This PR is too out of data and there seems to be little interest in the cleanup it provides.

@pabigot pabigot closed this Jul 20, 2020
@pabigot pabigot deleted the dts/20191102b branch July 20, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards 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: Shields Shields (add-on boards) DNM This PR should not be merged (Do Not Merge) has-conflicts Issue/PR has conflicts with another issue/PR platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants