[Issue 1001187] New port - HAL: Freescale Kinetis variant, TWR-K60N512, TWR-K40X256 plf; Devs: Freescale UART and ENET (Ethernet)

[bugzilla-daemon@ecoscentric.com]

Please do not reply to this email. Use the web interface provided at:
https://bugzilla.ecoscentric.com/show_bug.cgi?id=1001187

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jifl@ecoscentric.com

--- Comment #20 from Jonathan Larmour <jifl@ecoscentric.com> 2011-10-17 04:22:32 BST ---
As per direct mail, here's a few comments, although I haven't done what I'd
call a full review:


- I'll just mention that if extra TTYs are justified, then you should just
modify CYGPKG_IO_SERIAL.

- "familly" should be spelt "family", and "acces" -> "access"

- CDL display names shouldn't really end in full-stops ("."). The best way to
decide what to use for display names is to think of what would be best in the
graphical configuration tool - even if you, like many maintainers, don't use
it, lots of eCos users do.

- Various components which are "flavor none" should also have "no_define".

- The CYGPKG_HAL_CORTEXM_KINETIS .cdl file is quite big - perhaps the
components related to clocking could be split off into a separate file, and
included. 

- This bit in hal_diag.c:
+#if defined(CYGOPT_HAL_KINETIS_DIAG_IN_MISC_FLASH_SECTION) && \
+        CYGOPT_HAL_KINETIS_DIAG_IN_MISC_FLASH_SECTION
+#  define CYGOPT_HAL_KINETIS_DIAG_FLASH_SECTION_ATTR         \
+        CYGOPT_HAL_KINETIS_MISC_FLASH_SECTION_ATTR
+#else
+#  define CYGOPT_HAL_KINETIS_DIAG_FLASH_SECTION_ATTR
+#endif

I think you originally meant to begin that with:
+#if defined(CYGOPT_HAL_KINETIS_DIAG_IN_MISC_FLASH_SECTION) && \
+        CYGOPT_HAL_KINETIS_MISC_FLASH_SECTION_ATTR

But given the CDL you can probably just use:
#ifdef CYGOPT_HAL_KINETIS_DIAG_IN_MISC_FLASH_SECTION


Nothing else leapt out at me. It seems a very good quality patch from what I
can tell.

Jifl

-- 
Configure issuemail: https://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the issue.