|
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.
[Prev in Thread] | Current Thread | [Next in Thread] |