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: flash: spi: Move to using select in Kconfig for SPI bus #51598

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

pdgendt
Copy link
Collaborator

@pdgendt pdgendt commented Oct 25, 2022

  • Updated to the latest policy to select the SPI bus for flash controllers
  • Updated SPI flash samples to remove default selected symbols

MaureenHelm
MaureenHelm previously approved these changes Oct 25, 2022
galak
galak previously approved these changes Oct 25, 2022
@galak galak added the DNM This PR should not be merged (Do Not Merge) label Oct 25, 2022
@galak
Copy link
Collaborator

galak commented Oct 25, 2022

Marking DNM to allow some board maintainer feedback based on @de-nordic's comment.

@pdgendt
Copy link
Collaborator Author

pdgendt commented Nov 7, 2022

Marking DNM to allow some board maintainer feedback based on @de-nordic's comment.

@galak Anyone in particular we should ping?

@github-actions
Copy link

github-actions bot commented Jan 7, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 7, 2023
@pdgendt pdgendt removed the Stale label Jan 9, 2023
@pdgendt
Copy link
Collaborator Author

pdgendt commented Jan 9, 2023

@galak is the DNM label still applicable?

@galak
Copy link
Collaborator

galak commented Jan 13, 2023

@henrikbrixandersen, @dleach02 , @danieldegrasse can you guys take a look

@galak
Copy link
Collaborator

galak commented Jan 13, 2023

@pdgendt looks like this needs a rebase to resolve a merge conflict.

drivers/flash/Kconfig.nor Show resolved Hide resolved
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

currently blocking.

Although a discussion seems to have happened in #48707 then I was not consulted on such change and I need to take a closer look at this discussion because there are risks / pitfalls related to select when comparing to depends on.

I need to take a closer look at first #48707.

@carlescufi
Copy link
Member

carlescufi commented Jan 17, 2023

Architecture WG:

@nordicjm nordicjm dismissed their stale review January 17, 2023 16:59

Dismissing as per architecture group review

tejlmand
tejlmand previously approved these changes Feb 27, 2023
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

A bit concerned on selecting higher level SPI from drivers.

One concern is the stuck-symbol which may occur when re-configuring.
Second concern is the select always kicking in, meaning users cannot get rid of a select as well as select overruling any depends on (a behavior which often surprises users)
There are some general design discussions / concerns as well, but would go to far to list everything here.

I acknowledge the limitation of current Kconfig / dts implementation wrt. scalability as described by df81fef

To accommodate concerns stated above we should see if a corresponding hidden (promptless) symbol should be introduced in addition to, e.g menuconfig SPI.
But that is considered outside the scope of this PR.

Approving with the concerns given above.

@carlescufi carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Feb 27, 2023
carlescufi
carlescufi previously approved these changes Feb 27, 2023
Move to using 'select SPI' instead of 'depends on SPI'
(see commit df81fef for more details)

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
The Kconfig symbol for CONFIG_SPI_NOR is selected by default if a
compatible device is enabled.
Remove obsolete board config files.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
The Kconfig symbol CONFIG_SPI_FLASH_AT45 is selected by default if a
compatible device is enabled.
Remove obsolete config entry.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
@carlescufi
Copy link
Member

@pdgendt I've rebased the branch

@aescolar aescolar dismissed stale reviews from carlescufi and tejlmand via 07f7cfa February 27, 2023 12:20
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

@carlescufi carlescufi merged commit b1d74e4 into zephyrproject-rtos:main Feb 27, 2023
@pdgendt pdgendt deleted the spi-flash-at45-select branch June 28, 2023 12:41
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.

10 participants