SFDP: consolidation of SFDP parsing [3/5] #12450
Conversation
|
@VeijoPesonen, thank you for your changes. |
VeijoPesonen
requested review from
kyle-cypress,
SeppoTakalo and
jeromecoutant
|
Rebased. |
Would it be worth describing what consolidation is in this commit in the commit message? There are couple of things changed based on the files diff. |
There was a problem hiding this comment.
Changes look good functionally. Made some suggestions to improve the documentation of the new shared sfdp functions. Could be deferred if further cleanup to those functions is already planned once the consolidation is finished.
Validated on CY8CKIT-062-WIFI-BT and CY8CPROTO-064-SB.
drivers/internal/SFDP.h
Outdated
| @@ -105,12 +106,36 @@ size_t sfdp_detect_page_size(uint8_t *bptbl_ptr, size_t bptbl_size); | |||
| /** Detect all supported erase types | |||
| * | |||
| * @param bptbl_ptr Pointer to memory holding a Basic Parameter Table structure | |||
| * @param smtbl All information parsed from the table gets passed back on this structure | |||
| * @param smtbl Sector Map Table information structure | |||
There was a problem hiding this comment.
The name and description of this argument doesn't match the function declaration. Same on line 120.
| * | ||
| * @param offset Offset value | ||
| * @param smtbl Sector Map Table information structure | ||
| */ |
drivers/internal/SFDP.h
Outdated
| * @param region | ||
| * @param smtbl Sector Map Table information structure | ||
| * | ||
| * @return |
There was a problem hiding this comment.
Most of the parameters, and the return type, are not described
drivers/internal/SFDP.h
Outdated
| @@ -87,7 +88,7 @@ int sfdp_parse_headers(Callback<int(bd_addr_t, void *, bd_size_t)> sfdp_reader, | |||
| * Retrieves the table from a device and parses the information contained by the table | |||
| * | |||
| * @param sfdp_reader Callback function used to read headers from a device | |||
| * @param smtbl All information parsed from the table gets passed back on this structure | |||
| * @param smtbl Sector Map Table information structure | |||
There was a problem hiding this comment.
I like the more descriptive comments, but can the descriptions still make it clear when these are "output" parameters that the function modifies?
Thanks Kyle, I think it starts to be time to make those modifications you suggested - otherwise those get easily forgotten.
Much appreciated. |
Merges erase region search found from SPIFBlockDevice and QSPIFBlockDevice. Moves the implementation within the SFDP component
Yeah, I agree and did the rewording. |
drivers/internal/SFDP.h
Outdated
| * @param sfdp_reader Callback function used to read headers from within a device | ||
| * @param[out] sfdp_info Contains the results of parsing the SFDP Database JEDEC headers | ||
| * | ||
| * @return MBED_SUCCESS, negative error code on failure |
There was a problem hiding this comment.
This is probably obvious from the macro name, but should it still be:
* @return MBED_SUCCESS on success, negative error code on failure
mergify
bot
dismissed
SeppoTakalo’s stale review
Pull request has been modified.
Test run: FAILEDSummary: 4 of 11 test jobs failed Failed test jobs:
|
Test run: SUCCESSSummary: 8 of 8 test jobs passed |
VeijoPesonen
changed the title
Summary of changes
Depends on PR #12426.Scratch that, the PR got merged to master. 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
None
Pull request type
Test results
Reviewers