Skip to content

Conversation

bentcooke
Copy link
Contributor

@bentcooke bentcooke commented Mar 20, 2019

Description

This PR adds config options for disabling the SFDP functionality of the SPIF driver and allows a user to specify the parameters needed through config to work with non-SFDP flash devices. It has been tested on the DRAGONFLY_411RE platform which contains a M25P16 serial flash that does not include SFDP. The default config is unchanged with SFDP enabled.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ciarmcom ciarmcom requested review from a team March 20, 2019 20:00
@ciarmcom
Copy link
Member

@bentcooke, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@offirko
Copy link
Contributor

offirko commented Mar 24, 2019

@bentcooke - SPIFlBlockDevice is an SFDP standard SPI flash block device, for targets with SFDP flash.
IMO, adapting the code to targets that do not support SFDP requires many "ifdefs" , makes the code hard to follow, and misses the point of the module. This will be much worse when other targets will each add their unique private logic..

My suggestion is to add a separate BD to each target that does not support one of the existing mbed-os standards BDs

@dannybenor
Copy link

It is better to add a new folder in components (https://github.com/ARMmbed/mbed-os/tree/master/components/storage/blockdevice) and enable the new blockdevice on targets.json on the relevant targets. The new name can be NSFDP_SPIF

@0xc0170 0xc0170 changed the title [SPIF] Add config for non-SFDP flash devices SPIF: Add config for non-SFDP flash devices Apr 1, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 1, 2019

Any update @bentcooke ?

@bentcooke
Copy link
Contributor Author

@offirko and @dannybenor , thanks for the feedback and if your preference is for a separate SPIF driver, I will rework the contribution as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants