From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1725 invoked by alias); 24 Mar 2011 20:34:26 -0000 Received: (qmail 1300 invoked by uid 22791); 24 Mar 2011 20:34:25 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-iy0-f169.google.com (HELO mail-iy0-f169.google.com) (209.85.210.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 Mar 2011 20:34:19 +0000 Received: by iyf13 with SMTP id 13so429996iyf.0 for ; Thu, 24 Mar 2011 13:34:19 -0700 (PDT) Received: by 10.231.185.105 with SMTP id cn41mr4584977ibb.72.1300998859285; Thu, 24 Mar 2011 13:34:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.231.11.195 with HTTP; Thu, 24 Mar 2011 13:33:59 -0700 (PDT) In-Reply-To: <20110324151403.GM2520@adacore.com> References: <1293750414-14626-1-git-send-email-vapier@gentoo.org> <1300945105-25242-1-git-send-email-vapier@gentoo.org> <20110324151403.GM2520@adacore.com> From: Mike Frysinger Date: Fri, 25 Mar 2011 00:13:00 -0000 Message-ID: Subject: Re: [PATCH v2] sim: cfi: new flash device simulation To: Joel Brobecker Cc: gdb-patches@sourceware.org, toolchain-devel@blackfin.uclinux.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-03/txt/msg01084.txt.bz2 On Thu, Mar 24, 2011 at 11:14 AM, Joel Brobecker wrote: > There are just a few minor comments: > =A0- opening curly brace in struct union declarations should be on > =A0 =A0the next line some habits die hard ;). especially when i looked at sim/common/ and they all use the style i did. > =A0- We're not fond of commented out code. =A0You could replace it by > =A0 =A0a comment if useful. only place i see this is top of cfi_io_read_buffer() where a minor check is #ifdef-ed out. i guess i could change it to something like: /* XXX: Should we require read's to have cfi->width =3D=3D nr_bytes ? */ obviously i prefer the existing #if 0 approach since it has been tested ;) > =A0- I realize that this is a large-ish job, but it would be nice to > =A0 =A0have all types and routines documented. =A0Generally, it's even > =A0 =A0preferable to document the fields in the struct/union types, > =A0 =A0but a small description of the type will be a good start for now. the fields should match 1:1 to the CFI spec, but i can sprinkle some comments about and see what happens. i spent the most time on the comment that people who want to *use* the code need. after all, people should only need to care about the device tree format and not know any of the internals. > =A0- I think that this deserves a NEWS entry in gdb/NEWS i guess > =A0- And I think that we should add some documentation in the GDB > =A0 =A0Manual. =A0The simulator section is almost non-existent in the > =A0 =A0current manual, but if we could start defining a general structure, > =A0 =A0even if they are empty, and document this new feature in that > =A0 =A0structure, at least we won't make things worse. yeah, i think you mentioned this before -mike