diff mbox

[01/26] ohci: use realize for ohci

Message ID d445aecddbb8eb8d2c6107c1c56e2e5421c12437.1371804804.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao June 22, 2013, 8:50 a.m. UTC 
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/usb/hcd-ohci.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Comments

Peter Crosthwaite June 24, 2013, 5:54 a.m. UTC | #1 
Hi Hu,

On Sat, Jun 22, 2013 at 6:50 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/usb/hcd-ohci.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 51241cd..79ef41b 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1876,17 +1876,16 @@ typedef struct {
>      dma_addr_t dma_offset;
>  } OHCISysBusState;
>
> -static int ohci_init_pxa(SysBusDevice *dev)
> +static void ohci_realize_pxa(DeviceState *dev, Error **errp)
>  {
> -    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> +    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);

I don't think this is an improvement. Until a QOM cast macro is
available, FROM_SYSBUS is preferable to a DO_UPCAST I think?

> +    SysBusDevice *b = SYS_BUS_DEVICE(dev);
>
>      /* Cannot fail as we pass NULL for masterbus */
> -    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
> +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
>                    &dma_context_memory);

Rebase required due to Paolos IOMMU patches going in removing
dma_context_memory.

Regards,
Peter

> -    sysbus_init_irq(dev, &s->ohci.irq);
> -    sysbus_init_mmio(dev, &s->ohci.mem);
> -
> -    return 0;
> +    sysbus_init_irq(b, &s->ohci.irq);
> +    sysbus_init_mmio(b, &s->ohci.mem);
>  }
>
>  static Property ohci_pci_properties[] = {
> @@ -1926,9 +1925,8 @@ static Property ohci_sysbus_properties[] = {
>  static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>
> -    sbc->init = ohci_init_pxa;
> +    dc->realize = ohci_realize_pxa;
>      dc->desc = "OHCI USB Controller";
>      dc->props = ohci_sysbus_properties;
>  }
> --
> 1.8.3.1
>
>
Hu Tao June 24, 2013, 6:11 a.m. UTC | #2 
On Mon, Jun 24, 2013 at 03:54:31PM +1000, Peter Crosthwaite wrote:
> Hi Hu,
> 
> On Sat, Jun 22, 2013 at 6:50 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/usb/hcd-ohci.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> > index 51241cd..79ef41b 100644
> > --- a/hw/usb/hcd-ohci.c
> > +++ b/hw/usb/hcd-ohci.c
> > @@ -1876,17 +1876,16 @@ typedef struct {
> >      dma_addr_t dma_offset;
> >  } OHCISysBusState;
> >
> > -static int ohci_init_pxa(SysBusDevice *dev)
> > +static void ohci_realize_pxa(DeviceState *dev, Error **errp)
> >  {
> > -    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> > +    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);
> 
> I don't think this is an improvement. Until a QOM cast macro is
> available, FROM_SYSBUS is preferable to a DO_UPCAST I think?

patch 2 introduces QOM macro and replaces DO_UPCAST. Instead, we can
also first do QOM then realize. Which one do you prefer?

> 
> > +    SysBusDevice *b = SYS_BUS_DEVICE(dev);
> >
> >      /* Cannot fail as we pass NULL for masterbus */
> > -    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
> > +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
> >                    &dma_context_memory);
> 
> Rebase required due to Paolos IOMMU patches going in removing
> dma_context_memory.

Thanks for reminding. I'll do a rebase anyway, patches involving i440fx
and q35 may conflict with your `pci cleanup' series, and ehci patches
duplicates Andreas's work.
Peter Crosthwaite June 24, 2013, 6:17 a.m. UTC | #3 
Hi Hu,

On Mon, Jun 24, 2013 at 4:11 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> On Mon, Jun 24, 2013 at 03:54:31PM +1000, Peter Crosthwaite wrote:
>> Hi Hu,
>>
>> On Sat, Jun 22, 2013 at 6:50 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
>> > Cc: Gerd Hoffmann <kraxel@redhat.com>
>> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> > ---
>> >  hw/usb/hcd-ohci.c | 16 +++++++---------
>> >  1 file changed, 7 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
>> > index 51241cd..79ef41b 100644
>> > --- a/hw/usb/hcd-ohci.c
>> > +++ b/hw/usb/hcd-ohci.c
>> > @@ -1876,17 +1876,16 @@ typedef struct {
>> >      dma_addr_t dma_offset;
>> >  } OHCISysBusState;
>> >
>> > -static int ohci_init_pxa(SysBusDevice *dev)
>> > +static void ohci_realize_pxa(DeviceState *dev, Error **errp)
>> >  {
>> > -    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
>> > +    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);
>>
>> I don't think this is an improvement. Until a QOM cast macro is
>> available, FROM_SYSBUS is preferable to a DO_UPCAST I think?
>
> patch 2 introduces QOM macro and replaces DO_UPCAST. Instead, we can
> also first do QOM then realize. Which one do you prefer?
>

Other way round I think make more sense, as no need to have this ugly
hunk for transition sake.

Squashing is another low effort option, one patch that just does it
all makes sense to me (and ive done this a few times already with
various devices).

Regards,
Peter

>>
>> > +    SysBusDevice *b = SYS_BUS_DEVICE(dev);
>> >
>> >      /* Cannot fail as we pass NULL for masterbus */
>> > -    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
>> > +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
>> >                    &dma_context_memory);
>>
>> Rebase required due to Paolos IOMMU patches going in removing
>> dma_context_memory.
>
> Thanks for reminding. I'll do a rebase anyway, patches involving i440fx
> and q35 may conflict with your `pci cleanup' series, and ehci patches
> duplicates Andreas's work.
>
>
Eduardo Habkost June 24, 2013, 2:01 p.m. UTC | #4 
On Sat, Jun 22, 2013 at 04:50:13PM +0800, Hu Tao wrote:
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/usb/hcd-ohci.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 51241cd..79ef41b 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1876,17 +1876,16 @@ typedef struct {
>      dma_addr_t dma_offset;
>  } OHCISysBusState;
>  
> -static int ohci_init_pxa(SysBusDevice *dev)
> +static void ohci_realize_pxa(DeviceState *dev, Error **errp)
>  {
> -    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> +    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);
> +    SysBusDevice *b = SYS_BUS_DEVICE(dev);
>  
>      /* Cannot fail as we pass NULL for masterbus */
> -    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
> +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
>                    &dma_context_memory);

This patch doesn't apply cleanly anymore, since "[PULL 00/25]
Memory/IOMMU patches, part 3: IOMMU implementation" from Paolo was
merged (commit 576156f)


> -    sysbus_init_irq(dev, &s->ohci.irq);
> -    sysbus_init_mmio(dev, &s->ohci.mem);
> -
> -    return 0;
> +    sysbus_init_irq(b, &s->ohci.irq);
> +    sysbus_init_mmio(b, &s->ohci.mem);
>  }
>  
>  static Property ohci_pci_properties[] = {
> @@ -1926,9 +1925,8 @@ static Property ohci_sysbus_properties[] = {
>  static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>  
> -    sbc->init = ohci_init_pxa;
> +    dc->realize = ohci_realize_pxa;
>      dc->desc = "OHCI USB Controller";
>      dc->props = ohci_sysbus_properties;
>  }
> -- 
> 1.8.3.1
> 
>
Hu Tao June 25, 2013, 1:54 a.m. UTC | #5 
On Mon, Jun 24, 2013 at 04:17:28PM +1000, Peter Crosthwaite wrote:
> Hi Hu,
> 
> On Mon, Jun 24, 2013 at 4:11 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> > On Mon, Jun 24, 2013 at 03:54:31PM +1000, Peter Crosthwaite wrote:
> >> Hi Hu,
> >>
> >> On Sat, Jun 22, 2013 at 6:50 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> >> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >> > ---
> >> >  hw/usb/hcd-ohci.c | 16 +++++++---------
> >> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> >> > index 51241cd..79ef41b 100644
> >> > --- a/hw/usb/hcd-ohci.c
> >> > +++ b/hw/usb/hcd-ohci.c
> >> > @@ -1876,17 +1876,16 @@ typedef struct {
> >> >      dma_addr_t dma_offset;
> >> >  } OHCISysBusState;
> >> >
> >> > -static int ohci_init_pxa(SysBusDevice *dev)
> >> > +static void ohci_realize_pxa(DeviceState *dev, Error **errp)
> >> >  {
> >> > -    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> >> > +    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);
> >>
> >> I don't think this is an improvement. Until a QOM cast macro is
> >> available, FROM_SYSBUS is preferable to a DO_UPCAST I think?
> >
> > patch 2 introduces QOM macro and replaces DO_UPCAST. Instead, we can
> > also first do QOM then realize. Which one do you prefer?
> >
> 
> Other way round I think make more sense, as no need to have this ugly
> hunk for transition sake.

Agreed.

> 
> Squashing is another low effort option, one patch that just does it
> all makes sense to me (and ive done this a few times already with
> various devices).

But I think it's easier to review to keep it in two steps.

> 
> Regards,
> Peter
> 
> >>
> >> > +    SysBusDevice *b = SYS_BUS_DEVICE(dev);
> >> >
> >> >      /* Cannot fail as we pass NULL for masterbus */
> >> > -    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
> >> > +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
> >> >                    &dma_context_memory);
> >>
> >> Rebase required due to Paolos IOMMU patches going in removing
> >> dma_context_memory.
> >
> > Thanks for reminding. I'll do a rebase anyway, patches involving i440fx
> > and q35 may conflict with your `pci cleanup' series, and ehci patches
> > duplicates Andreas's work.
> >
> >
diff mbox

Patch

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 51241cd..79ef41b 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1876,17 +1876,16 @@  typedef struct {
     dma_addr_t dma_offset;
 } OHCISysBusState;
 
-static int ohci_init_pxa(SysBusDevice *dev)
+static void ohci_realize_pxa(DeviceState *dev, Error **errp)
 {
-    OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
+    OHCISysBusState *s = DO_UPCAST(OHCISysBusState, busdev.qdev, dev);
+    SysBusDevice *b = SYS_BUS_DEVICE(dev);
 
     /* Cannot fail as we pass NULL for masterbus */
-    usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
+    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0,
                   &dma_context_memory);
-    sysbus_init_irq(dev, &s->ohci.irq);
-    sysbus_init_mmio(dev, &s->ohci.mem);
-
-    return 0;
+    sysbus_init_irq(b, &s->ohci.irq);
+    sysbus_init_mmio(b, &s->ohci.mem);
 }
 
 static Property ohci_pci_properties[] = {
@@ -1926,9 +1925,8 @@  static Property ohci_sysbus_properties[] = {
 static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
 
-    sbc->init = ohci_init_pxa;
+    dc->realize = ohci_realize_pxa;
     dc->desc = "OHCI USB Controller";
     dc->props = ohci_sysbus_properties;
 }