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

support of QSPI transport to operate in 1BIT I/O mode #109

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lyusupov
Copy link
Contributor

@lyusupov lyusupov commented Dec 7, 2021

Fix for #104

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

changes looks great, I kind of guess that you will enable 1bit mode by modifying the flash device table to supports_qspi=false right ? If that is the case, I think we should take another approach by updating the transport layer instead. I will explain in more detail after getting your confirmation.

@@ -75,6 +75,8 @@ class Adafruit_SPIFlashBase {
int _ind_pin;
bool _ind_active;

void write_status_register(uint8_t *);
Copy link
Member

Choose a reason for hiding this comment

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

the prototype should be

void write_status_register(uint8_t const full_status[2]);

Copy link
Contributor Author

@lyusupov lyusupov Dec 8, 2021

Choose a reason for hiding this comment

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

Done in c38a7ca

This PR is intentionally designed to create of no or least harm possible:

  • it uses current API only
    • setReadCommand() on an intermediate level and
    • supports_qspi=false on a 'user level' (in flash device descriptor)
  • default behavior remains the same as before - QSPI will operate at quad data rate unless user have not supplied supports_qspi=false argument intentionally.

It is out of the scopes for this PR to alter current API of the library.
I suppose that any changes in the API are only necessary when there are high demands of users to use extended features.
I doubt that this 1-bit access feature is currently so important that we need to change internal and external API of the library.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for confirmation, you don't have to alter the current API. IMHO, the flash device table is flash description, if the flash itself support quad mode, we should just leave it as it is. Instead We could update the actual transport layer. E.g 1bit mode or 2bit mode can be activated when declaring an object with IO1-IO3 = -1

https://github.com/adafruit/Adafruit_SPIFlash/blob/master/src/qspi/Adafruit_FlashTransport_QSPI_NRF.cpp#L35

the rest of the code can still apply, and it is transparent to existing user. That also allow using existing flash dev database and the n-bit mode is dictated by the user sketch only. Let me know if this makes sense to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be happy to see (and use) your own PR that will supersede this one

Copy link
Member

@hathach hathach Dec 9, 2021

Choose a reason for hiding this comment

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

I would like to, but I don't have motivation (and time) unless adafruit make board that need this. Meanwhile, we can leave this PR pending as reference.

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.

2 participants