hartmannathan commented on code in PR #8827:
URL: https://github.com/apache/nuttx/pull/8827#discussion_r1140426773


##########
arch/arm/src/nrf52/nrf52_nvmc.c:
##########
@@ -102,78 +102,6 @@ static inline void nrf_mem_barrier(void)
  * Public Functions
  ****************************************************************************/
 
-/****************************************************************************
- * Name: nrf_nvmc_enable_icache
- *
- * Description:
- *   Enable I-Cache for Flash
- *
- * Input Parameter:
- *   flag - Flag to enable or disable.
- *
- * Returned Values:
- *   None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_icache(bool flag)
-{
-  uint32_t value;
-
-  /* Read the current icache configuration */
-
-  value = getreg32(NRF52_NVMC_ICACHECNF);
-
-  if (flag)
-    {
-      value |= NVMC_ICACHECNF_CACHEEN;
-    }
-  else
-    {
-      value &= ~NVMC_ICACHECNF_CACHEEN;
-    }
-
-  /* Setup the new icache configuration */
-
-  putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
-/****************************************************************************
- * Name: nrf_nvmc_enable_profile
- *
- * Description:
- *   Enable profiling I-Cache for flash
- *
- * Input Parameter:
- *   flag - Flag to enable or disable.
- *
- * Returned Values:
- *   None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_profile(bool flag)
-{
-  uint32_t value;
-
-  /* Read the current icache configuration */
-
-  value = getreg32(NRF52_NVMC_ICACHECNF);
-
-  if (flag)
-    {
-      value |= NVMC_ICACHECNF_CACHEPROFEN;
-    }
-  else
-    {
-      value &= ~NVMC_ICACHECNF_CACHEPROFEN;
-    }
-
-  /* Setup the new icache configuration */
-
-  putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-

Review Comment:
   I'm not sure if it's best to handle this in Make.defs or to wrap the 
contents with ifdefs like I suggested.
   
   Some pros and cons:
   
   Suppose we have a mistake (like the reason for this PR) and a BSD component 
is included in the build and Kconfig does not have ALLOW_BSD_COMPONENTS.
   
   Doing the ifdef in Make.defs will break the build with a Linker error. These 
are confusing because the error will say it can't find some symbol, but won't 
explain why.
   
   Doing the ifdef in the source file keeps the BSD license check in the same 
place as the BSD licensed content and we can have:
   
   ```
   #ifndef ALLOW_BSD_COMPONENTS
   #  error "This file requires Kconfig ALLOW_BSD_COMPONENTS"
   #else
   .
   .
   .
   #endif /* ALLOW_BSD_COMPONENTS */
   ```
   
   We can do that for all affected files (but not in this PR). I replied to the 
dev list just now with thoughts about how we should handle non-Apache-2.0 
licenses throughout the codebase: 
https://lists.apache.org/thread/2tbw5jbvsbtgl0wjkbld9ync2vn1sz4b
   



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to