From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Urx7C-0003Vg-6R for qemu-devel@nongnu.org; Wed, 26 Jun 2013 17:18:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Urx7B-0004hA-4O for qemu-devel@nongnu.org; Wed, 26 Jun 2013 17:18:58 -0400 Received: from mail-ee0-x234.google.com ([2a00:1450:4013:c00::234]:55129) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Urx7A-0004ge-UN for qemu-devel@nongnu.org; Wed, 26 Jun 2013 17:18:57 -0400 Received: by mail-ee0-f52.google.com with SMTP id c50so7827516eek.11 for ; Wed, 26 Jun 2013 14:18:55 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51CB5AB8.9030409@redhat.com> Date: Wed, 26 Jun 2013 23:18:48 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1372184516-32397-1-git-send-email-peter.maydell@linaro.org> <1372184516-32397-3-git-send-email-peter.maydell@linaro.org> <51C9E490.7090304@redhat.com> <51CA9086.80802@redhat.com> <51CB39C3.30707@tribudubois.net> <51CB5995.6020702@tribudubois.net> In-Reply-To: <51CB5995.6020702@tribudubois.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: Peter Maydell , Peter Crosthwaite , Anthony Liguori , qemu-devel@nongnu.org, Paul Brook , =?ISO-8859-1?Q?Andreas_F=E4rber?= 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 >>> wrote: >>>> Il 25/06/2013 22:53, Peter Maydell ha scritto: >>>>> On 25 June 2013 19:42, Paolo Bonzini 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