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

[Philippe Mathieu-Daudé]

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.