qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v39 01/22] target/avr: Add outward facing interfaces and core


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v39 01/22] target/avr: Add outward facing interfaces and core CPU logic
Date: Sat, 21 Dec 2019 12:22:55 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

Hi Aleksandar,

On 12/21/19 11:53 AM, Aleksandar Markovic wrote:


On Wednesday, December 18, 2019, Michael Rolnik <address@hidden <mailto:address@hidden>> wrote:

    This includes:
    - CPU data structures
    - object model classes and functions
    - migration functions
    - GDB hooks

    Co-developed-by: Michael Rolnik <address@hidden
    <mailto:address@hidden>>
    Co-developed-by: Sarah Harris <address@hidden
    <mailto:address@hidden>>
    Signed-off-by: Michael Rolnik <address@hidden
    <mailto:address@hidden>>
    Signed-off-by: Sarah Harris <address@hidden
    <mailto:address@hidden>>
    Signed-off-by: Michael Rolnik <address@hidden
    <mailto:address@hidden>>
    Acked-by: Igor Mammedov <address@hidden
    <mailto:address@hidden>>
    Tested-by: Philippe Mathieu-Daudé <address@hidden
    <mailto:address@hidden>>
    ---
      target/avr/cpu-param.h |  37 +++
      target/avr/cpu-qom.h   |  54 ++++
      target/avr/cpu.h       | 258 ++++++++++++++++
      target/avr/cpu.c       | 654 +++++++++++++++++++++++++++++++++++++++++
      target/avr/gdbstub.c   |  84 ++++++
      target/avr/machine.c   | 121 ++++++++
      gdb-xml/avr-cpu.xml    |  49 +++
      7 files changed, 1257 insertions(+)
      create mode 100644 target/avr/cpu-param.h
      create mode 100644 target/avr/cpu-qom.h
      create mode 100644 target/avr/cpu.h
      create mode 100644 target/avr/cpu.c
      create mode 100644 target/avr/gdbstub.c
      create mode 100644 target/avr/machine.c
      create mode 100644 gdb-xml/avr-cpu.xml

[...]
    +typedef enum AVRFeature {
    +    AVR_FEATURE_SRAM,
    +
    +    AVR_FEATURE_1_BYTE_PC,
    +    AVR_FEATURE_2_BYTE_PC,
    +    AVR_FEATURE_3_BYTE_PC,
    +
    +    AVR_FEATURE_1_BYTE_SP,
    +    AVR_FEATURE_2_BYTE_SP,
    +
    +    AVR_FEATURE_BREAK,
    +    AVR_FEATURE_DES,
    +    AVR_FEATURE_RMW, /* Read Modify Write - XCH LAC LAS LAT */
    +
    +    AVR_FEATURE_EIJMP_EICALL,
    +    AVR_FEATURE_IJMP_ICALL,
    +    AVR_FEATURE_JMP_CALL,
    +
    +    AVR_FEATURE_ADIW_SBIW,
    +
    +    AVR_FEATURE_SPM,
    +    AVR_FEATURE_SPMX,
    +
    +    AVR_FEATURE_ELPMX,
    +    AVR_FEATURE_ELPM,
    +    AVR_FEATURE_LPMX,
    +    AVR_FEATURE_LPM,
    +
    +    AVR_FEATURE_MOVW,
    +    AVR_FEATURE_MUL,
    +    AVR_FEATURE_RAMPD,
    +    AVR_FEATURE_RAMPX,
    +    AVR_FEATURE_RAMPY,
    +    AVR_FEATURE_RAMPZ,
    +} AVRFeature;
    +


Very good! Now, each feature should have some really brief (less than say 45 characters) comment in the same line as the definition, like this one:

         AVR_FEATURE_MOVW,      /* supports MOVW instruction */

IMHO this looks overkill when the name is already obvious. In some cases we can explicit, such 'RMW'.

Maybe we can simply group the features by type with a single comment previous the group, such:

          /* Instructions */
          AVR_FEATURE_MOVW,
          AVR_FEATURE_MUL,
          AVR_FEATURE_RMW, /* Read Modify Write - XCH LAC LAS LAT */
          ...


You already did it for AVR_FEATURE_RMW

Of course, all such comments should be vertically aligned nicely.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]