From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28678 invoked by alias); 17 Oct 2011 21:30:53 -0000 Received: (qmail 28669 invoked by uid 22791); 17 Oct 2011 21:30:52 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 17 Oct 2011 21:30:38 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id B86BB2F78012 for ; Mon, 17 Oct 2011 22:30:37 +0100 (BST) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NRnVHyEJgJIO; Mon, 17 Oct 2011 22:30:35 +0100 (BST) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001187] New port - HAL: Freescale Kinetis variant, TWR-K60N512, TWR-K40X256 plf; Devs: Freescale UART and ENET (Ethernet) X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: eCos X-Bugzilla-Component: Patches and contributions X-Bugzilla-Keywords: X-Bugzilla-Severity: enhancement X-Bugzilla-Who: ilijak@siva.com.mk X-Bugzilla-Status: NEEDINFO X-Bugzilla-Priority: low X-Bugzilla-Assigned-To: unassigned@bugs.ecos.sourceware.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Changed-Fields: In-Reply-To: References: X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Date: Mon, 17 Oct 2011 21:30:00 -0000 Message-Id: <20111017213035.DDF192F78004@mail.ecoscentric.com> Mailing-List: contact ecos-patches-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-patches-owner@ecos.sourceware.org X-SW-Source: 2011-10/txt/msg00036.txt.bz2 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 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.