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: Macro to initialize driver for every device instance #23416

Merged

Conversation

io-pa
Copy link
Contributor

@io-pa io-pa commented Mar 12, 2020

This pull request introduces the macro: DT_INST_FOREACH(inst_expr).

This makes it possible to do this:
DT_INST_FOREACH(I2C_NRFX_TWIM_DEVICE);

Instead of:

#ifdef DT_INST_N_0_nordic_nrf_twim
I2C_NRFX_TWIM_DEVICE(0);
#endif

#ifdef DT_INST_N_1_nordic_nrf_twim
I2C_NRFX_TWIM_DEVICE(1);
#endif

#ifdef DT_INST_N_2_nordic_nrf_twim
I2C_NRFX_TWIM_DEVICE(2);
#endif

#ifdef DT_INST_N_3_nordic_nrf_twim
I2C_NRFX_TWIM_DEVICE(3);
#endif

The macro calls the passed inst_expr for every compatible (and enabled) device in the device tree.
By using this approach the drivers don't have to make assumption about the maximum count of device instances anymore.
The expr argument can be either a macro or a function that take the instance number as argument.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Mar 12, 2020

All checks are passing now.

checkpatch (informational only, not a failure)

-:62: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#62: FILE: include/devicetree.h:1451:
+#define DT_CALL_WITH_ARG(arg, expr) expr(arg);

- total: 0 errors, 1 warnings, 74 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this. This will be very useful for initialising multiple device instances.

@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch 6 times, most recently from 3de925e to 8c9ea2e Compare March 18, 2020 13:21
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

I think ideally this would be rebased on #23245 and added to devicetree.h instead. @galak?

@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch from 8c9ea2e to 85e2039 Compare March 25, 2020 13:17
@galak galak requested a review from nordic-krch March 25, 2020 13:31
@galak
Copy link
Collaborator

galak commented Mar 25, 2020

Generally looks go to me. Will let @mbolivar-nordic and @nordic-krch comment on the UTIL_PASS_ARG_TO_MACRO bits.

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

This macro will be really helpful, thanks for this constribution. @io-pa it would be helpful to add some documentation/example.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Looks good to me

I think you could define UTIL_PASS_ARG_TO_MACRO as DT_PASS_ARG_TO_MACRO_INTERNAL at the end of devicetree.h to avoid having to extend the util.h API for this.

Can you please also:

  • update doc/guides/dts/howtos.rst to use this in the code example in the "Create struct devices in a driver" section? IMO this should be the standard way to define devices now, whenever possible.
  • add a test case for DT_INST_FOREACH to tests/lib/devicetree/src/main.c? Every macro in devicetree.h has at least one test case in that file. One idea would be to pass TEST_GPIO_INIT to DT_INST_FOREACH instead of the way it's done now.

@mbolivar-nordic
Copy link
Contributor

Might want to change the commit log's "device.h" to "devicetree.h" if you get a chance also. Not a blocker.

With this macro device drivers can call macros and functions
on every device instance compatible to that driver.
This makes it possible to make drivers agnostic to the
potential counts of instances.
Sidenote: Introduces helper macro DT_CALL_WITH_ARG.

Signed-off-by: Ioannis Papamanoglou <iopapamanoglou@gmail.com>
@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch from 85e2039 to af45164 Compare March 26, 2020 09:22
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Mar 26, 2020
@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch from af45164 to 36d8949 Compare March 26, 2020 09:27
@io-pa
Copy link
Contributor Author

io-pa commented Mar 26, 2020

I changed the helper macro to add ; at the end, since it's now specific to DT_INST_FOREACH.
That way it supports also functions next to macros and macros that need to be ;-terminated.

Updated howto "create struct devices in a driver" section to use
DT_INST_FOREACH instead of manual per-instance macros.

Signed-off-by: Ioannis Papamanoglou <iopapamanoglou@gmail.com>
@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch from 36d8949 to 45e1723 Compare March 26, 2020 09:35
Replace manual per-instance macros with DT_INST_FOREACH
for TEST_GPIO_INIT test.

Signed-off-by: Ioannis Papamanoglou <iopapamanoglou@gmail.com>
@io-pa io-pa force-pushed the dts/dynamic_instance_init_macro branch from 45e1723 to f68822e Compare March 26, 2020 12:34
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Thank you!

* Has to accept instance_number as only parameter.
*/
#define DT_INST_FOREACH(inst_expr) \
UTIL_LISTIFY(DT_NUM_INST(DT_DRV_COMPAT), DT_CALL_WITH_ARG, inst_expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking -- this does the right thing if DT_NUM_INST(DT_DRV_COMPAT) is 0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uses LISTIFY internally, and that uses REPEAT and REPEAT checks wether the variable is 0 in the beginning so should be fine. I just tested it though to be sure and it looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming.

@mbolivar-nordic
Copy link
Contributor

Shippable failure looks ignorable.

@galak
Copy link
Collaborator

galak commented Mar 26, 2020

ignoring the shippable failure as its not related.

@galak galak merged commit 110d3d9 into zephyrproject-rtos:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Devicetree area: Documentation area: Drivers area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants