You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/09/11 11:25:51 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #1754: esp32-core: Add support for the external SPIFLASH
Ouss4 opened a new pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754
## Summary
- Fix some trivial, mostly cosmetic, issues in the driver.
- Add support for the board.
## Impact
Shouldn't affect any other existing code.
## Testing
The provided defconfig was used to test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d merged pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
v01d merged pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487013537
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691116276
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487022744
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691108224
@Ouss4 PR https://github.com/apache/incubator-nuttx/pull/1755 should fix the break, please rebase your change again. Since PR https://github.com/apache/incubator-nuttx/pull/1611 send out three weeks ago, It's hard to catch the new project config file added recently. Sorry for inconvenience.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487522557
##########
File path: arch/xtensa/src/esp32/esp32_spiflash.c
##########
@@ -131,7 +155,7 @@ static struct esp32_spiflash_s s_esp32_spiflash1 =
.bwrite = esp32_bwrite,
.read = esp32_read,
.ioctl = esp32_ioctl,
-#if defined(CONFIG_MTD_BYTE_WRITE)
+#ifdef CONFIG_MTD_BYTE_WRITE
Review comment:
@davids5 That was originally `#if defined(CONFIG_MTD_BYTE_WRITE) && defined(ANOTHER_CFG)` that other config was just a copy/paste typo when porting, that doesn't apply to this file.
I find "#ifdef" a little explicit compared to `#defined(ONLY_ONE_CFG)` but the two are equivalent in this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487022744
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487022744
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
We could add any other file system, I just picked some of what we have in the repo.
The board logic that was added with this PR prepares the driver to be mounted on a file system, but the SPIFLASH driver itself is completely independent from that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691129600
The mac fail is:
<pre>
====================================================================================
Configuration/Tool: esp32-core/random
------------------------------------------------------------------------------------
Cleaning...
HEAD detached at pull/1754/merge
Changes not staged for commit:
modified: boards/xtensa/esp32/esp32-core/configs/mmcsdspi/defconfig
no changes added to commit
Configuring...
Building NuttX...
Normalize esp32-core/random
HEAD detached at pull/1754/merge
Changes not staged for commit:
modified: boards/xtensa/esp32/esp32-core/configs/mmcsdspi/defconfig
</pre>
I think this needs a configuration refresh.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487013537
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691116276
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487498848
##########
File path: arch/xtensa/src/esp32/esp32_spiflash.c
##########
@@ -131,7 +155,7 @@ static struct esp32_spiflash_s s_esp32_spiflash1 =
.bwrite = esp32_bwrite,
.read = esp32_read,
.ioctl = esp32_ioctl,
-#if defined(CONFIG_MTD_BYTE_WRITE)
+#ifdef CONFIG_MTD_BYTE_WRITE
Review comment:
@Ouss4 - Curiosity question: Why would the the `#ifdef` be preferred over the `#if defined` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d merged pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
v01d merged pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691116276
@xiaoxiang781216 It looks like it passed. The only one failing is a macOS build which is not related to that PR. I did a similar change as #1755 when I saw your PR, I think if I didn't the build would've failed when #1611 was merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691108224
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691108224
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691161196
> I think this needs a configuration refresh.
That's what I did earlier. I wonder why only the macOS one caught that, the rest didn't complain.
Anyway, I rebased, let's see again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#issuecomment-691129600
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d merged pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
v01d merged pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #1754: esp32-core: Add support for the external SPIFLASH
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #1754:
URL: https://github.com/apache/incubator-nuttx/pull/1754#discussion_r487013537
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
##########
File path: boards/xtensa/esp32/esp32-core/Kconfig
##########
@@ -44,4 +44,23 @@ config ESP32CORE_FLASH_IMAGE
---help---
Create flash_image.bin mainly used for QEMU.
+choice
Review comment:
The SPIFLASH should work independently from the File System the user chose, maybe it is a limitation of current implementation, correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org