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

drivers: serial: define default values for basic options #83324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yishai1999
Copy link
Contributor

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.

@@ -10,12 +10,14 @@ properties:
description: Clock frequency information for UART operation
current-speed:
type: int
default: 115200
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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

Copy link
Member

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:

.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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree area: UART Universal Asynchronous Receiver-Transmitter platform: STM32 ST Micro STM32 platform: TI SimpleLink Texas Instruments SimpleLink MCU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants