SFDP: consolidation of SFDP parsing [2/5] #12426
Conversation
VeijoPesonen
requested review from
SeppoTakalo,
michalpasztamobica,
kyle-cypress and
jeromecoutant
|
@ARMmbed/team-nxp for your information. |
|
@VeijoPesonen, thank you for your changes. |
There was a problem hiding this comment.
This all looks good and is a valuable code clean-up. The remarks I put in are minor and can also be included in the next stage of work.
I wonder about two things:
- Do the available greentea tests provide enough coverage to give us confidence that when they pass we haven't broken the feature? :) If not - would it be the right moment to add some unittests before the refactoring, to ensure the functionality stays the same (for error handling and corner cases)? Maybe they would also make the debugging easier in case some bug hides somewhere, as was the case in the previous PR. They are also more glitch-proof than tests running on real HW ;-)
- How much is the
nfrom the PR's title? I appreciate splitting the work into smaller chunks as that also makes the review easier, I am just wondering what does the split look like.
drivers/source/SFDP.cpp
Outdated
| } else { | ||
| tr_debug("SFDP erase types are not available - falling back to legacy 4k erase instruction"); | ||
|
|
||
| // 0xFF indicates that the legacy 4k erase instruction is not supported |
There was a problem hiding this comment.
[minor] Perhaps we could add an SFDP_ERASE_BITMASK_TYPE_4K_ERASE_UNSUPPORTED to replace the 0xff below and get rid of this comment?
I had a discussion about this some time ago with @SeppoTakalo and the end result was that HW tests are good enough. I trust that the HW vendors get the SFDP standard more correct than I would by mimicking the structures in unit tests.
I really don't know yet. Now I just try to rip out all the common parts from SPIF- and QSPIF-modules and try to fit those somehow under SFDP module. I'm more eager to do refactoring the code under SFDP module once I have all the parts there. |
|
Ci started |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
|
From the logs |
|
Just noticed this in another PR, creating a new blocker as its in nightly and PRs as well cc @ARMmbed/mbed-os-test |
|
I restarted the job |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
|
CI restarted (the error was fixed) |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
mergify
bot
dismissed stale reviews from mark-edgeworth and SeppoTakalo
Pull request has been modified.
|
Rebased, @0xc0170 would you please restart the tests if necessary. |
|
CI restarted |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
VeijoPesonen
changed the title
Summary of changes
Depends on PR #12318(already merged) .
That one should go in before you start reviewing this one.Purpose of this PR is to consolidate SFDP header and table parsing and make the QSPIF- and SPIFBlockDevices to use the same shared implementation. Before this PR almost the same code was found from both of the components.Impact of changes
Migration actions required
Documentation
Pull request type
Test results
SPIF compilation
QSPIF compilation
Reviewers
@SeppoTakalo
@michalpasztamobica