Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Paolo Bonzini <pbonzini@redhat.com>]

Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto:
> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote:
>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
>>> Hi
>>>
>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com>
>>> wrote:
>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>>>
>>>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>>>>       memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>>>> -                          s, "imxg-timer",
>>>>>>> +                          s, TYPE_IMX_GPT,
>>>>>>>                             0x00001000);
>>>>>>>       sysbus_init_mmio(dev, &s->iomem);
>>>>>>>
>>>>>> There was some agreement that this is not a good change.
>>>>> I agree (and more so regarding the use of the macro in the
>>>>> vmstate name), but nobody actually posted any comment to
>>>>> that effect against any of the versions of this patch that
>>>>> got sent out for review...
>>>> Yeah, the timing was bad... Can you post a revert, though?
>>>>
>>> The original string being replaced was a poor choice as well. IIUC the
>>> consensus was string field of the memory regions is supposed to
>>> indicate the purpose of the memory region for the device. Good
>>> examples would be "Control regs" or "MMIO". Naming the memory region
>>> after the device type is a redundancy as that info will come via
>>> memory region owners.
>>>
>>> So rather than revert could you just choose something more descriptive?
>> Peter (Maydell),
>>
>> How do you want to work this out?
>>
>> Do you revert it and we start over?
>>
>> Or should I provide a patch on top of the actual file to change the
>> "wrong name"?
>>
>> Please advise.
> 
> I browsed through the various timer implementations in the hw/timer
> directory and I was not able to spot one that would follow the
> convention you indicated.
> 
> Could you point me to a "good citizen" example?

Here is one example (hw/pci-host/piix.c):

    memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
                          "pci-conf-idx", 4);
    sysbus_add_io(dev, 0xcf8, &s->conf_mem);
    sysbus_init_ioports(&s->busdev, 0xcf8, 4);

    memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s,
                          "pci-conf-data", 4);
    sysbus_add_io(dev, 0xcfc, &s->data_mem);
    sysbus_init_ioports(&s->busdev, 0xcfc, 4);

Just reverting the changes to memory regions and vmstate names is enough
for now.

Paolo