[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE
patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add
CONFIG_BOARD_LATE_INITIALIZE
URL: https://github.com/apache/incubator-nuttx/pull/279#discussion_r381330023
##
File path: boards/arm/tiva/lm3s6965-ek/src/lm_appinit.c
##
@@ -152,6 +153,9 @@ int board_app_initialize(uintptr_t arg)
mcinfo("Successfully bound SPI port %d to MMC/SD slot %d\n",
CONFIG_NSH_MMCSDSPIPORTNO, CONFIG_NSH_MMCSDSLOTNO);
+#endif
Review comment:
Yes. Is there some reason why the initialization should be different when
it is performed via the boardctl->board_app_initialize() path vs the
board_late_initialize() path. I don't think so. I think that the
initialiaztion they should be identical use either path. Many applications
will not not call boardctl() to initialize. In fact NSH does call boardctl()
but most do not. In the latter case, we just enable
CONFIG_BOARD_LATE_INITIALIZE and we get the same initialization without
board_app_initialize() being called.
Two separate initialization paths. Two different use cases. But the
resulting initialization should be the same as a general rule.
I can imagine some special-case initialization that uses both, but I don't
know when that would be used. All of the configurations in the NuttX
repository expect the initialization to be identical in either path.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE
patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add
CONFIG_BOARD_LATE_INITIALIZE
URL: https://github.com/apache/incubator-nuttx/pull/279#discussion_r379527673
##
File path: boards/arm/tiva/lm3s6965-ek/src/lm_appinit.c
##
@@ -152,6 +153,9 @@ int board_app_initialize(uintptr_t arg)
mcinfo("Successfully bound SPI port %d to MMC/SD slot %d\n",
CONFIG_NSH_MMCSDSPIPORTNO, CONFIG_NSH_MMCSDSLOTNO);
+#endif
Review comment:
Look for an example at:
https://github.com/apache/incubator-nuttx/blob/master/boards/arm/stm32/stm32f4discovery/src/stm32_boot.c#L124
https://github.com/apache/incubator-nuttx/blob/master/boards/arm/stm32/stm32f4discovery/src/stm32_appinit.c#L58
https://github.com/apache/incubator-nuttx/blob/master/boards/arm/stm32/stm32f4discovery/src/stm32_bringup.c#L140
So it doesn't matter which path you take to do that initialization, the
board initialization is always performed in the same way. Otherwise, the board
will be initialized on one way if it is initialized from the application but in
a different with if initialized by board_late_initialize().
I suppose that I can imagine some scenario where you might want both
initializations and have them doing something different, in general board
support logic, the two initialization paths should should have identical
results.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE
patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add
CONFIG_BOARD_LATE_INITIALIZE
URL: https://github.com/apache/incubator-nuttx/pull/279#discussion_r379521040
##
File path: boards/arm/tiva/lm3s6965-ek/src/lm_appinit.c
##
@@ -152,6 +153,9 @@ int board_app_initialize(uintptr_t arg)
mcinfo("Successfully bound SPI port %d to MMC/SD slot %d\n",
CONFIG_NSH_MMCSDSPIPORTNO, CONFIG_NSH_MMCSDSLOTNO);
+#endif
Review comment:
I am not going to hold this up any longer. But this means that the logic
about the #ifndef CONFIG_BOARD_LATE_INITIALIZE will not be execute when
initialization is done via board_late_initialize. I don't think you really
want that asymmetry.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE
patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE URL: https://github.com/apache/incubator-nuttx/pull/279#discussion_r379516750 ## File path: boards/arm/tiva/lm3s6965-ek/src/lm_bringup.c ## @@ -0,0 +1,96 @@ +/ + * boards/arm/tiva/lm3s6965-ek/src/lm_bringup.c + * + * Copyright (C) 2015, 2018 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + *notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + *notice, this list of conditions and the following disclaimer in + *the documentation and/or other materials provided with the + *distribution. + * 3. Neither the name NuttX nor the names of its contributors may be + *used to endorse or promote products derived from this software + *without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + / Review comment: Yes, you have my permission to change the license header on this file to Apache 2.0. Please copy paste from a file like sched/sched/sched_getcpu.c to assure that the format is identical. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE
patacongo commented on a change in pull request #279: tiva/lm3s6965-ek: Add CONFIG_BOARD_LATE_INITIALIZE URL: https://github.com/apache/incubator-nuttx/pull/279#discussion_r379467853 ## File path: boards/arm/tiva/lm3s6965-ek/src/Makefile ## @@ -46,4 +46,8 @@ ifeq ($(CONFIG_NX_LCDDRIVER),y) CSRCS += lm_oled.c endif +ifeq ($(CONFIG_BOARD_LATE_INITIALIZE),y) + CSRCS += lm_bringup.c +endif + Review comment: Normally in most existing configuration, board_app_initialize() and board_late_initialize() both call the bringup() functions. These are two alternative ways to do the same thing: Either the initialization is done on a separate initialization thread, or on the main thread of the application. But, in either case, the initialization should be the same. 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: us...@infra.apache.org With regards, Apache Git Services
