On Mon, 15 Jul 2013 10:20:29 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 10.07.2013 21:17 schrieb Stefan Tauner: > > unsigned long is not the right type for manipulating pointer values. > > Since C99 there are suitable unsigned and signed types available, namely > > uintptr_t and intptr_t respectively. > > > > Use them in physmap.c and its callers where applicable. > > > > This patch also changes the display width of all address values in > > physmap.c to 16/8 hex characters depending on the actual size. > > > > Signed-off-by: Stefan Tauner <[email protected]> > > Why don't you use PRIxPTR_WIDTH everywhere including dummyflasher.c? That's a bug AFAICS, thanks. > Put > another way, is there a reason to use PRIxPTR_WIDTH at all? > Do we care that C99 doesn't guarantee a 0x prefix for printed addresses > with PRIxPTR? Will that cause confusion? The main point of it is to get nicely aligned/readable addresses that are padded to the right(tm) size, and yes I guess the guaranteed 0x prefix too (I can't remember TBH :). > I think PRIxPTR is a good idea, but I'm not so sure about the side effects. > > There's also the problem of programmer_map_flash_region. It should not > take an uintptr_t. The address parameter is a horrible mess: A physical > address from the CPU point of view for the internal programmer, a > top-of-4-GB aligned fake address for some other programmers, and a > bottom-aligned address for the rest of our programmers. If we want to do > this the right way, we need to have a wrapper for physmap in the > "internal" programmer case instead of setting map_flash_region = > physmap. And then I need to update my patch which abstracts the flash > bus address vs programmer address conflict into something that won't > cause madness. Hm, I need to look into this again. Using a union for the type and a wrapper does not sound so bad to me (without looking at the code :) This could at least be made platform-independent without jumping through hoops. > Overall, while I'd like to have this patch in, it conceptually breaks as > much stuff as it fixes. I don't see this getting fixed within the > release timeframe, so I'd like to postpone. Fine with me, but this means no win64 support in this stable. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
