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 <[email protected]> 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.
