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

devicetree should support making properties with defaults required #21181

Closed
pabigot opened this issue Dec 4, 2019 · 4 comments · Fixed by #21271
Closed

devicetree should support making properties with defaults required #21181

pabigot opened this issue Dec 4, 2019 · 4 comments · Fixed by #21271
Assignees
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features

Comments

@pabigot
Copy link
Collaborator

pabigot commented Dec 4, 2019

A relatively common idiom in devicetree bindings is to define a property that is optional in the base binding, and made required in a binding that extends the base. An existing example is the ngpios property in gpio-controller.yaml, which is made required for specific controllers that have been updated to support it.

To simplify the process of updating drivers that need the property without having to update every SOC binding it would be nice to be able to provide a default in the base. For ngpios in particular this is appropriate since almost all bindings will use the value 32.

The current tooling rejects this because it combines the default value from the base definition with the required property from the extended version and complains that required properties cannot have defaults.

Options include:

  • Stop complaining
  • Complain only when the required and default appear in the same YAML file
  • Continue to enforce this and disallow changing an optional property with a default into a required property.

(It's unclear whether this is a bug or an enhancement, but this gap prevents the straightforward path to merging #20115. A decision on how it should be resolved is needed soon so that can make progress.)

@pabigot pabigot added bug The issue is a bug, or the PR is fixing a bug Enhancement Changes/Updates/Additions to existing features area: Devicetree labels Dec 4, 2019
@pabigot pabigot removed the bug The issue is a bug, or the PR is fixing a bug label Dec 4, 2019
@galak
Copy link
Collaborator

galak commented Dec 4, 2019

  • Complain only when the required and default appear in the same YAML file

I'd say something along these lines makes sense to me.

@ulfalizer
Copy link
Collaborator

ulfalizer commented Dec 4, 2019

Think this might've been based on a misunderstanding. See #20115 (comment).

All semantic binding checks run on the final merged binding btw, so it doesn't matter where keys come from as long as the binding file and all the include:d files combine into something valid.

@pabigot
Copy link
Collaborator Author

pabigot commented Dec 4, 2019

No, I did misunderstand, but the accepted workaround will break if the type of the property changes between where the default is specified and where required is added. Unless I misunderstand again.

I think "Complain only when the required and default appear in the same YAML file" is the place to put this validation. I understand that means checking this is not trivial, so "Stop complaining" is the workable alternative (and what my commit does).

If we're agreed that's acceptable I'll pull my commit out and submit it to master as well.

@ulfalizer
Copy link
Collaborator

ulfalizer commented Dec 4, 2019

No, I did misunderstand, but the accepted workaround will break if the type of the property changes between where the default is specified and where required is added. Unless I misunderstand again.

Currently, you'll get an error if you try to change anything from an included file besides changing required: false to required: true, so more things would have to change.

I suspect it won't be needed though. Changing the type feels a bit hackish.

Could look into it if a case comes up where it seems to be the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants