Re: [Qemu-devel] [Qemu-trivial] [PATCH] hw/arm/highbank: Simplify code (memory region in device state)

[Peter Maydell]

On 6 December 2013 19:12, Michael Tokarev <address@hidden> wrote:
> 06.12.2013 22:43, Stefan Weil wrote:
>> The memory region can be included by value instead of by reference in the
>> device state (like it is done in other SoCs).
>>
>> Signed-off-by: Stefan Weil <address@hidden>
>> ---
>>  hw/arm/highbank.c |    7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
>> index fe98ef1..54384b3 100644
>> --- a/hw/arm/highbank.c
>> +++ b/hw/arm/highbank.c
>> @@ -125,7 +125,7 @@ typedef struct {
>>      SysBusDevice parent_obj;
>>      /*< public >*/
>>
>> -    MemoryRegion *iomem;
>> +    MemoryRegion iomem;
>>      uint32_t regs[NUM_REGS];
>>  } HighbankRegsState;
>
> Don't we have active maintainer for arm? (Who is Cc'd on the original patch).

Yeah, and there's highbank related patches currently going through
review so it might be better for me to take this in the target-arm queue
(though for a trivial one like this it doesn't make much difference).

>> @@ -154,10 +154,9 @@ static int highbank_regs_init(SysBusDevice *dev)
>>  {
>>      HighbankRegsState *s = HIGHBANK_REGISTERS(dev);
>>
>> -    s->iomem = g_new(MemoryRegion, 1);
>> -    memory_region_init_io(s->iomem, OBJECT(s), &hb_mem_ops, s->regs,
>> +    memory_region_init_io(&s->iomem, OBJECT(s), &hb_mem_ops, s->regs,
>>                            "highbank_regs", 0x1000);
>
> I know right to nothing about arm, does it have any alignment requiriments,
> which may break here?

Hmm? This is just putting the MemoryRegion struct in the HighBankRegsState
structure rather than allocating it via g_new(). That's all host related
and in any case is the host C compiler's problem to deal with.

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM