-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
drivers: serial: define default values for basic options #83324
base: main
Are you sure you want to change the base?
Conversation
@@ -10,12 +10,14 @@ properties: | |||
description: Clock frequency information for UART operation | |||
current-speed: | |||
type: int | |||
default: 115200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lacking justification, see https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that's true for all of the properties or just baudrate?
Also, Shouldn't it be a required property at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of them. If you add a default then they are required as the default will always be present unless something overrides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was trying to propose in #83312 (comment) was to do this in the per-driver binding rather than the base binding.
As pointed out by @yishai1999 in #83312 (comment) there seems to be several drivers using DT_INST_*_OR()
to have defaults in the driver code itself for these properties, e.g. here:
zephyr/drivers/serial/uart_esp32.c
Lines 1058 to 1062 in 6dce233
.parity = DT_INST_ENUM_IDX_OR(idx, parity, UART_CFG_PARITY_NONE), \ | |
.stop_bits = DT_INST_ENUM_IDX_OR(idx, stop_bits, \ | |
UART_CFG_STOP_BITS_1), \ | |
.data_bits = DT_INST_ENUM_IDX_OR(idx, data_bits, \ | |
UART_CFG_DATA_BITS_8), \ |
If there really has to be a default in these cases (which I'm not convinced of) having per-driver DT binding default values seems like the lesser of two evils, since then you can at least see the actual value that was used by looking at build/zephyr/zephyr.dts
. However, either way, it'd be good to establish some consistent approach for this across all UART drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm What do you think about it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the required description as per the above link stating where the default value comes from? E.g. https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/sensor/vishay%2Cvcnl36825t.yaml#L47
Defined default values for baudrate, parity, stop bits, and data bits. This removes complexity and obfuscation from the code. Signed-off-by: Yishai Jaffe <yishai1999@gmail.com>
837afbc
to
3e289c8
Compare
Defined default values for baudrate, parity, stop bits, and data bits. This removes complexity and obfuscation from the code.
I made the changes only to a few drivers to gauge interest - If this is accepted I will do the rest too.