|
From: | Tristan Gingold |
Subject: | Re: [avr-gcc-list] Patch avrtest: add support for avr6 |
Date: | Wed, 18 Jun 2008 09:59:12 -0400 |
On Jun 18, 2008, at 8:14 AM, Paulo Marques wrote: Hi Paulo, thank you for the quick review.
I don't know if it makes sense to use bit fields here, because there might be a performance penalty in accessing them and "pc_3bytes" is accessed a few times while executing avr code.
Changed to unsigned char.
+ // List of supported archs with their features. + const struct arch_desc arch_descs[] = + { + { "avr51", 0, 0}, + { "avr6", 1, 1}, + { NULL, 0, 0} + };It would probably bew easier to read these structures if they were written like:const struct arch_desc arch_descs[] = { { .name = "avr51", .pc_3_bytes = 0, .has_eind = 0, }, { .name = "avr6", .pc_3_bytes = 1, .has_eind = 1, }, { NULL, 0, 0} };I know its more verbose, but if we keep adding flags it will become impossible to understand what { "avr51", 0, 0, 1, 0, 1, 1, 0 } means ;)
I agree.
You forgot to update usage() to reflect the new options, no?
Oops, you're right.
I don't think we need to restrict the program name to be the last argument.
Ok.
} } ! // setup default values! flash_addr_mask = (arch->pc_3bytes ? MAX_FLASH_SIZE : (1 << 16)) - 1;flash_addr_mask is in bytes, so to keep the previous default 128kB this should be "(1 << 17) - 1", no?
Oops, you're right. Thank you for catching this!
Anyway, overall this seems like a nice simple structure to add more architectures and features in the future.
Attached is the updated patch. Thanks, Tristan.
avr6.diff
Description: Binary data
[Prev in Thread] | Current Thread | [Next in Thread] |