Re: [PATCH v2] sim: cfi: new flash device simulation

[Mike Frysinger <vapier at gentoo dot org>]

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