[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2464: Added: MT29F2G Nand Flash block driver for sam4s-xplained-pro.

[GitBox <gi...@apache.org>]

btashton commented on a change in pull request #2464:
URL: https://github.com/apache/incubator-nuttx/pull/2464#discussion_r534687712



##########
File path: boards/arm/sam34/sam4s-xplained-pro/configs/nsh/defconfig
##########
@@ -56,7 +55,6 @@ CONFIG_NSH_STRERROR=y
 CONFIG_PIPES=y
 CONFIG_PREALLOC_MQ_MSGS=8
 CONFIG_PREALLOC_TIMERS=4
-CONFIG_PTHREAD_STACK_DEFAULT=4096

Review comment:
       Was this reduced for some reason?  I would expect the default to be fine for this configuration so I am actually fine with the change long as you have verified it.  Otherwise I would just leave this configuration alone.

##########
File path: boards/arm/sam34/sam4s-xplained-pro/src/sam_spi.c
##########
@@ -0,0 +1,210 @@
+/****************************************************************************
+ * boards/arm/sam34/sam4s-xplained-pro/src/sam_spi.c

Review comment:
       The original version of this file is from Greg which was granted to Apache, so we can update it to have the Apache header like you have in the other new files.

##########
File path: drivers/mmcsd/mmcsd_sdio.c
##########
@@ -2645,7 +2645,7 @@ static int mmcsd_widebus(FAR struct mmcsd_state_s *priv)
       priv->widebus = true;
 
       SDIO_CLOCK(priv->dev, CLOCK_SD_TRANSFER_4BIT);
-      usleep(MMCSD_CLK_DELAY);
+      up_udelay(MMCSD_CLK_DELAY);

Review comment:
       We should not be using up_delay here this will cause a busy loop that is bad for performance.  Instead we should be using `nxsig_usleep`  (the original `usleep` is also wrong).  If this breaks the functionality we will need to look into that more to see why this is not sleeping the correct amount.

##########
File path: boards/arm/sam34/sam4s-xplained-pro/src/sam4s-xplained-pro.h
##########
@@ -93,6 +99,62 @@
 #  undef HAVE_HSMCI
 #endif
 
+/* MMC/SD minor numbers */
+
+#ifndef CONFIG_NSH_MMCSDMINOR
+#  define CONFIG_NSH_MMCSDMINOR 0
+#endif
+
+#ifndef CONFIG_NSH_MMCSDSLOTNO
+#  define CONFIG_NSH_MMCSDSLOTNO 0
+#endif
+
+#if CONFIG_NSH_MMCSDSLOTNO != 0
+#  error SAM34 has only one MMC/SD slot (CONFIG_NSH_MMCSDSLOTNO)
+#  undef CONFIG_NSH_MMCSDSLOTNO
+#  define CONFIG_NSH_MMCSDSLOTNO 0
+#endif
+
+#define HSMCI_SLOTNO CONFIG_NSH_MMCSDSLOTNO
+#define HSMCI_MINOR  CONFIG_NSH_MMCSDMINOR
+
+/* Automounter.  Currently only works with HSMCI. */
+
+#if !defined(CONFIG_FS_AUTOMOUNTER) || !defined(HAVE_HSMCI)
+#  undef HAVE_AUTOMOUNTER
+#  undef CONFIG_SAM34_HSMCI_AUTOMOUNT
+#endif
+
+#ifndef CONFIG_SAM34_HSMCI_AUTOMOUNT
+#  undef HAVE_AUTOMOUNTER
+#endif
+
+#ifdef HAVE_AUTOMOUNTER
+#  ifdef CONFIG_SAM34_HSMCI_AUTOMOUNT

Review comment:
       These should all be defined in the board Kconfig like this `boards/arm/sama5/sama5d4-ek/Kconfig`
   Then all of the #define CONFIG_ can go away in this section.  This is an old style from before we had Kconfig

##########
File path: drivers/mmcsd/mmcsd_sdio.c
##########
@@ -2777,7 +2777,7 @@ static int mmcsd_mmcinitialize(FAR struct mmcsd_state_s *priv)
   /* Select high speed MMC clocking (which may depend on the DSR setting) */
 
   SDIO_CLOCK(priv->dev, CLOCK_MMC_TRANSFER);
-  usleep(MMCSD_CLK_DELAY);
+  up_udelay(MMCSD_CLK_DELAY);

Review comment:
       Same issue.  We should not use up_udelay unless it is really needed and instead use nxsig_usleep.  There are cases for very small sleeps where this can be the only way, but 5ms for MMCSD_CLK_DELAY should not be a problem for using nxsig_usleep

##########
File path: .gitignore
##########
@@ -23,6 +23,7 @@
 *.sym
 *.wsp
 *~
+.vscode

Review comment:
       should be `/.vscode`  Normally I dont like to add IDE specific entries, but I see we already have one for eclipse (I also use vscode so I personally wont mind)




----------------------------------------------------------------
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