This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] sim: cfi: new flash device simulation
- From: Mike Frysinger <vapier at gentoo dot org>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: gdb-patches at sourceware dot org, toolchain-devel at blackfin dot uclinux dot org
- Date: Thu, 24 Mar 2011 16:33:59 -0400
- Subject: Re: [PATCH v2] sim: cfi: new flash device simulation
- References: <1293750414-14626-1-git-send-email-vapier@gentoo.org> <1300945105-25242-1-git-send-email-vapier@gentoo.org> <20110324151403.GM2520@adacore.com>
On Thu, Mar 24, 2011 at 11:14 AM, Joel Brobecker wrote:
> There are just a few minor comments:
> ?- opening curly brace in struct union declarations should be on
> ? ?the next line
some habits die hard ;). especially when i looked at sim/common/ and
they all use the style i did.
> ?- We're not fond of commented out code. ?You could replace it by
> ? ?a 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 == nr_bytes ? */
obviously i prefer the existing #if 0 approach since it has been tested ;)
> ?- I realize that this is a large-ish job, but it would be nice to
> ? ?have all types and routines documented. ?Generally, it's even
> ? ?preferable to document the fields in the struct/union types,
> ? ?but 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.
> ?- I think that this deserves a NEWS entry
in gdb/NEWS i guess
> ?- And I think that we should add some documentation in the GDB
> ? ?Manual. ?The simulator section is almost non-existent in the
> ? ?current manual, but if we could start defining a general structure,
> ? ?even if they are empty, and document this new feature in that
> ? ?structure, at least we won't make things worse.
yeah, i think you mentioned this before
-mike