Skip to content

support default property values in devicetree bindings #17829

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

Closed
pabigot opened this issue Jul 28, 2019 · 5 comments · Fixed by #18788
Closed

support default property values in devicetree bindings #17829

pabigot opened this issue Jul 28, 2019 · 5 comments · Fixed by #18788
Assignees
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features

Comments

@pabigot
Copy link
Collaborator

pabigot commented Jul 28, 2019

I have a device for which the binding should include a property like this:

startup-delay-us:
    type: int
    category: optional
    description: >
        Startup time, in microseconds.

        The driver will busy-wait for this period before returning
        from a call that enables the regulator.

where absence of the property means that no delay is required.

There may be multiple of these devices, so I'd like the driver implementation file to use a trick like this[*]:

#ifdef DT_REGULATOR_GPIO_0
REGULATOR_GPIO_DEVICE(0);
#endif
#ifdef DT_REGULATOR_GPIO_1
REGULATOR_GPIO_DEVICE(1);
#endif
/* ... */

The killer is that REGULATOR_GPIO_DEVICE(id) needs to populate a config structure field like this:

    .startup_us = CPP_MAGIC_WRAPPER(DT_INST_##id##_REGULATOR_GPIO_STARTUP_DELAY_US, 0),

DT_INST_0_REGULATOR_GPIO_STARTUP_DELAY_US will either be undefined, or will be defined to a value that is unlikely to be 1, so preprocessor tricks like COND_CODE() won't work for CPP_MAGIC_WRAPPER.

If I could add this to the yaml:

    default: 0

and have the Zephyr scripting provide the default if there isn't one in the node that would make life easier. (Because there are actually multiple properties of this type, and having to do:

#ifdef REGULATOR_GPIO_0
#ifndef DT_INST_0_REGULATOR_GPIO_STARTUP_DELAY_US
#define DT_INST_0_REGULATOR_GPIO_STARTUP_DELAY_US 0
#endif
...
REGULATOR_GPIO_DEVICE(0)

rapidly becomes tedious.)

Is there another way to solve this?

[*] well, no, I'd like to have something that does that without having to unroll all the instances up to an arbitrary maximum, but I don't know how to make that happen, either.

@pabigot pabigot added Enhancement Changes/Updates/Additions to existing features area: Devicetree labels Jul 28, 2019
@ulfalizer
Copy link
Collaborator

Only skimmed, but would making startup-delay-us obligatory work? The description string could mention that 0 works and means no delay.

@pabigot
Copy link
Collaborator Author

pabigot commented Jul 29, 2019

That's what I'm going to do, but it's not ideal since the common case is there is no delay and so no need for the property to be specified. It just clutters the devicetree source.

This is at least the third time I've had a situation where I want a property to be optional in the device tree because I can infer a reasonable default in the driver from other information. This is the first time I've had to handle multiple instances of the device, and C simply doesn't provide the preprocessor capability to abstract the conditional into the generator macro.

@ulfalizer
Copy link
Collaborator

ulfalizer commented Jul 29, 2019

Yeah... just spent a while trying to make an IS_ENABLED()-like macro for it, before I realized why it's tricky (can't tell whether something is the result of a macro replacement or not). :P

Seems like a reasonable feature now, even if some #ifdefs might not be horrible in that case.

I'll add it to my todo list.

@ulfalizer ulfalizer self-assigned this Jul 29, 2019
ulfalizer added a commit to ulfalizer/zephyr that referenced this issue Aug 1, 2019
For optional properties, it can be handy to generate a default value
instead of no value if the property is missing, to cut down on #ifdefs.

Allow a default value to be specified in the binding, via a new
'default: <default value>' key on properties in 'properties:'.

Suggested by Peter A. Bigot in
zephyrproject-rtos#17829.

The documentation changes in binding-template.yaml explain the syntax.

Fixes: zephyrproject-rtos#17829

Signed-off-by: Ulf Magnusson <[email protected]>
@galak
Copy link
Collaborator

galak commented Aug 1, 2019

I'm not a fan of this approach, if a default is desired, just have the value set in the .dts file.

@pabigot
Copy link
Collaborator Author

pabigot commented Aug 1, 2019

@galak is that a "reject" or merely "I don't like it but won't block it"? I was about to start using this via #17928, but if it's not acceptable I won't waste my time.

ulfalizer added a commit to ulfalizer/zephyr that referenced this issue Sep 9, 2019
For missing optional properties, it can be handy to generate a default
value instead of no value, to cut down on #ifdefs.

Allow a default value to be specified in the binding, via a new
'default: <default value>' setting for properties in bindings.
Defaults are supported for both scalar and array types. YAML arrays are
used to specify the value for array types.

'default:' also appears in json-schema, with the same meaning.

Include misc. sanity checks, like the 'default' value matching 'type'.

The documentation changes in binding-template.yaml explain the syntax.

Suggested by Peter A. Bigot in
zephyrproject-rtos#17829.

Fixes: zephyrproject-rtos#17829
Signed-off-by: Ulf Magnusson <[email protected]>
galak pushed a commit that referenced this issue Sep 9, 2019
For missing optional properties, it can be handy to generate a default
value instead of no value, to cut down on #ifdefs.

Allow a default value to be specified in the binding, via a new
'default: <default value>' setting for properties in bindings.
Defaults are supported for both scalar and array types. YAML arrays are
used to specify the value for array types.

'default:' also appears in json-schema, with the same meaning.

Include misc. sanity checks, like the 'default' value matching 'type'.

The documentation changes in binding-template.yaml explain the syntax.

Suggested by Peter A. Bigot in
#17829.

Fixes: #17829
Signed-off-by: Ulf Magnusson <[email protected]>
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