Skip to content

Conversation

@Mierunski
Copy link

@Mierunski Mierunski commented Oct 4, 2018

All dts defines for nRF series are now extracted through aliases instead of dts.fixup files.
I left NAME defines for compatibility with examples and other drivers.
I moved WNCM14A2A dts.fixups to board level, but they should also be replaced with aliases and overlay files at example level.

Fixes #10658

@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #10377 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10377   +/-   ##
=======================================
  Coverage   48.41%   48.41%           
=======================================
  Files         270      270           
  Lines       42121    42121           
  Branches    10139    10139           
=======================================
  Hits        20394    20394           
  Misses      17645    17645           
  Partials     4082     4082

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a6858...c5bb5f7. Read the comment docs.

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from b972865 to 6596261 Compare October 4, 2018 14:27
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

So the use of non-CONFIG_ defines is something we've been trying to avoid. I guess if we want to go in this direction, we should start create DT_ prefix defines

@anangl
Copy link
Member

anangl commented Oct 5, 2018

@galak I think we should go in this direction. CONFIG_ prefixes in macros generated from DT entries are confusing since they suggest the values are configured via Kconfig.
DT_ prefix seems reasonable to me.

@Mierunski
Copy link
Author

Should I use for example DT_NRF_I2C or DT_I2C ?

@anangl
Copy link
Member

anangl commented Oct 5, 2018

@Mierunski DT_ is sufficient IMO. We'll have then:

IRQ_CONNECT(DT_I2C_0_IRQ, DT_I2C_0_IRQ_PRIORITY, ...

gpio0 = get_device_binding(DT_GPIO_0_LABEL);

Should work.

@galak
Copy link
Contributor

galak commented Oct 5, 2018

@Mierunski DT_ is sufficient IMO. We'll have then:

IRQ_CONNECT(DT_I2C_0_IRQ, DT_I2C_0_IRQ_PRIORITY, ...

gpio0 = get_device_binding(DT_GPIO_0_LABEL);

Should work.

As I think about this more, I worry about this since we are using a convention based on aliases, I guess I'd rather us focus our efforts on using code generation to remove dts.fixup and not introduce a new convention related to aliases (I know we do this some already for GPIOs w/LEDs & BUTTONs).

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 6596261 to 2537b99 Compare October 5, 2018 12:14
@Mierunski
Copy link
Author

Using code generation will be next step

@Mierunski
Copy link
Author

Mierunski commented Oct 8, 2018

@galak @anangl I changed prefix to DT_

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 2537b99 to dcc09a2 Compare October 9, 2018 06:52
@anangl
Copy link
Member

anangl commented Oct 9, 2018

@Mierunski boards/arm/nrf52840_pca10056/dts.fixup should be also renamed to dts_fixup.h

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from dcc09a2 to e2ea496 Compare October 9, 2018 09:34
@Mierunski
Copy link
Author

@anangl Fixed

@carlescufi
Copy link
Member

@galak can you revisit please?

@galak
Copy link
Contributor

galak commented Oct 9, 2018

@galak @anangl I changed prefix to DT_

As I mentioned before, I'd rather us stick with the dts fixup file for now and not introduce this style of DT_ usage. Lets focus on the code generation solution.

galak
galak previously requested changes Nov 16, 2018
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Some minor cleanups and comments

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? We moved wcm14a2a out of the board.dts:

commit edefd7d
Author: Kumar Gala [email protected]
Date: Thu Nov 15 09:58:49 2018 -0600

boards: nrf52840_pca10056: move modem configurtion to overlay

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to rebase this once the I2C PRI fix (#11427) goes in.

Copy link
Contributor

Choose a reason for hiding this comment

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

technically I wouldn't change the name right now since sensor drivers still use CONFIG_I2C_x_NAME. But I'm ok if you want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this should move to samples/net/lwm2m_client/dts_fixup.h

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from d84a8ef to 432c29f Compare November 19, 2018 10:21
Changed driver to use alias defines instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 432c29f to 42af0bb Compare November 19, 2018 10:40
@anangl
Copy link
Member

anangl commented Nov 19, 2018

You can also add "Fixes #10658" in the PR description.

@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 42af0bb to 94e2ee9 Compare November 19, 2018 12:11
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 94e2ee9 to 6a77704 Compare November 19, 2018 14:35
Changed driver to use defines from aliases instead of fixup.

Signed-off-by: Mieszko Mierunski <[email protected]>
@Mierunski Mierunski force-pushed the dts_fixup_cleanup_nrf branch from 6a77704 to c5bb5f7 Compare November 20, 2018 11:27
@Mierunski
Copy link
Author

@galak @anangl All changes should be fixed now

#define DT_SPI_3_NAME DT_NORDIC_NRF_SPI_SPI_3_LABEL

#if defined(DT_NORDIC_NRF_CC310_5002A000_BASE_ADDRESS)
#define DT_CC310_CTL_BASE_ADDR DT_NORDIC_NRF_CC310_5002A000_BASE_ADDRESS
Copy link
Member

Choose a reason for hiding this comment

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

Seems there are no "_NAME" defines for crypto. Is this intentional?

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

This looks fine now.

@galak galak dismissed their stale review November 20, 2018 19:01

issues addressed

@carlescufi carlescufi merged commit c9906dd into zephyrproject-rtos:master Nov 20, 2018
@Mierunski Mierunski deleted the dts_fixup_cleanup_nrf branch November 21, 2018 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot use SPI_0 on nRF52810

6 participants