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

[Mike Frysinger <vapier@gentoo.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