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

[bugzilla-daemon@bugs.ecos.sourceware.org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001187

--- Comment #22 from Ilija Kocho <ilijak@siva.com.mk> 2011-10-17 22:30:28 BST ---
(In reply to comment #20)
> 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.


This has some history. The idea was a provision for adding (as much as needed)
additional tty/termios drivers into target without the need for changing IO.
Luckily after a long session with Sergei the patch was surprisingly short.
Please see: http://ecos.sourceware.org/ml/ecos-devel/2011-03/msg00023.html
Also:      Bug #1001180


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

Thank you. Luckily there isn't wellcome (pardon welcome) or luckilly :) in my
text.

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

I'm trying to make the displays as appropriate as I can, but sometimes they
resemble sentences - hence full-stops. I'll clean them.

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

I don't know why, but I had impression that flavor none are no_define by
default... but now I know they aren't.

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


Yeah. This clock is more complex than some 8-bit micro-controllers. Good idea.
This brings me an idea of extracting clocking code from kinetis_misc.c?

kinetis_clocking.cdl
kinetis_clocking.c

Any comment?


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


It probably used to be flavor data or booldata defining section name. Than I
decided to simplify it (literal section name), but preprocessor code remained.
I'll look for eventual other cases.


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

Thank you for good advices.
Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.