Re: [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 9/1/23 13:46, BALATON Zoltan wrote:
> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
>> IEC binary prefixes ease code review: the unit is explicit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sh4/r2d.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>> index 39fc4f19d9..b3667e9b12 100644
>> --- a/hw/sh4/r2d.c
>> +++ b/hw/sh4/r2d.c
>> @@ -47,10 +47,10 @@
>> #define FLASH_BASE 0x00000000
>> #define FLASH_SIZE (16 * MiB)
>>
>> -#define SDRAM_BASE 0x0c000000 /* Physical location of SDRAM: Area 3 */
>> -#define SDRAM_SIZE 0x04000000
>> +#define SDRAM_BASE          (192 * MiB) /* Physical location of 
>> SDRAM: Area 3 */
>> +#define SDRAM_SIZE          (64 * MiB)
> 
> I don't think changing these help as the docs probably have memory map 
> with the hex numbers rather than sizes so it's easier to match as it is 
> now.
> 
>> -#define SM501_VRAM_SIZE 0x800000
>> +#define SM501_VRAM_SIZE     (8 * MiB)
> 
> This one is OK but since it's only used once in
> 
> qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
> 
> you might as well just inline it there and remove the define which is 
> then pretty clear and easier to see without needing to look up the 
> define far away from its usage.

I did this change after Peter's feedbacks on:
https://lore.kernel.org/qemu-devel/CAFEAcA8MSO4YMEq2FqvpJKUVYE_1CqaB2KLD1YN-YebOhJVgEg@mail.gmail.com/

But maybe I misunderstood him. Peter, looking at it again, maybe you
asked for a definition because when using pflash_cfi01_register() it
isn't explicit what means each argument? So in this case, since the
properties gives a hint on what is the value ("vram-size") it would
be OK to inline?

Thanks,

Phil.