Require recommended word separator (`-`) in devicetree property names

mbolivar-nordic commented 1 year ago

Introduction

Devicetree properties, by convention, use dash (-) as a word separator. This RFC proposes making this a requirement for upstream bindings.

Problem description

There are currently no automated checks in CI that ensure that devicetree properties use dashes as separators. As a result, many properties use underscore (_) as a separator. This is inconsistent with the style adopted by the devicetree specification for all standard properties (with the exception of device_type, but this property is deprecated and has limited usage).

It would be better style to use dashes instead, and would make things consistent for users typing properties. It also would avoid situations like having a foo-bar and foo_bar property defined on the same node due to typos.

Proposed change

Make all in-tree bindings and DTS files use dashes
Add CI checks to make sure we don't regress this

Detailed RFC

This is a straightforward treewide change.

Proposed change (Detailed)

Use grep to fix in-tree occurrences
Modify edtlib to expose first-class methods that list all binding objects
Add a new compliance check that uses these methods to complain on regressions, allowing administrator override if cases arise where we are inheriting an underscore in a property name from Linux bindings or other authoritative sources

Dependencies

Out of tree users of existing bindings will need to migrate to the new property names.

Concerns and Unresolved Questions

None.

Alternatives

An approach which allowed both styles for backwards incompatibility was considered, but this is too ripe for introducing simultaneous use of foo-bar vs foo_bar in a single devicetree.

henrikbrixandersen commented 1 year ago

I welcome this policy, however we need to consider existing "upstream" (Linux kernel) bindings using _ separators for properties as a valid exemption for this.

mbolivar-nordic commented 1 year ago

we need to consider existing "upstream" (Linux kernel) bindings using _ separators for properties as a valid exemption for this.

Indeed, this would make for a good exception -- could you give me some pointers? I did a search in our existing bindings for anything using underscores that had a corresponding binding doing the same in Linux, and I could not find any.

mbolivar-nordic commented 1 year ago

@henrikbrixandersen I added some language about this potential exception. I think the CI results should still be a failure which require admin override, the same way we handle additions of new typedefs.

henrikbrixandersen commented 1 year ago

Indeed, this would make for a good exception -- could you give me some pointers? I did a search in our existing bindings for anything using underscores that had a corresponding binding doing the same in Linux, and I could not find any.

I am not aware of any current "violations" - I just want to make sure that we do not create a policy like this without an exception for upstream/well-established bindings.

henrikbrixandersen commented 1 year ago

@henrikbrixandersen I added some language about this potential exception. I think the CI results should still be a failure which require admin override, the same way we handle additions of new typedefs.

Looks good, thank you.

cfriedt commented 1 year ago

Makes sense to me

carlescufi commented 1 year ago

Architecture WG:

@henrikbrixandersen is in favor but asks about how the CI check would work. @mbolivar-nordic mentions that the tooling would issue a warning and CI would upgrade the warning to an error
@galak mentions that out of tree users would benefit from a script that helps with the transition from _ to -, otherwise he's in favor. The script might not be trivial and is unclear whether it is worthwhile. @mbolivar-nordic to investigate further
The number of bindings to change is between one and two dozens, which is fairly small
Draft PR: https://github.com/henrikbrixandersen/zephyr/tree/gpio_hogs

No objections to the treewide change

nashif commented 1 year ago

can this be closed? AFAIK it was done already.

cfriedt commented 1 year ago

can this be closed? AFAIK it was done already.

https://github.com/zephyrproject-rtos/zephyr/pull/53502 is still open

zephyrproject-rtos / zephyr