[2/8] hw/ide: Get rid of piix4_init function

This removes pci_piix4_ide_init() function similar to clean up done to
other ide devices.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/piix.c    | 12 +-----------
 hw/isa/piix4.c   |  5 ++++-
 include/hw/ide.h |  1 -
 3 files changed, 5 insertions(+), 13 deletions(-)

On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> This removes pci_piix4_ide_init() function similar to clean up done to
> other ide devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ide/piix.c    | 12 +-----------
>   hw/isa/piix4.c   |  5 ++++-
>   include/hw/ide.h |  1 -
>   3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 8bcd6b72c2..3b2de4c312 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>       }
>   }
>   
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>   {
> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>       .class_init    = piix3_ide_class_init,
>   };
>   
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>   static void piix4_ide_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 7edec5e149..0ab4787658 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -35,6 +35,7 @@
>   #include "hw/timer/i8254.h"
>   #include "hw/rtc/mc146818rtc.h"
>   #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>   #include "migration/vmstate.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/runstate.h"
> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>       }
>   
> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");

Why are you re-assigning 'pci'?

>       hd = g_new(DriveInfo *, ide_drives);
>       ide_drive_get(hd, ide_drives);
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_ide_create_devs(pci, hd);
>       g_free(hd);
> +
>       pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>       if (smbus) {
>           *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 883bbaeb9b..21bd8f23f1 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                           DriveInfo *hd0, DriveInfo *hd1);
>   
>   /* ide-pci.c */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>   
>   /* ide-mmio.c */
>

On Sat, 14 Mar 2020, Philippe Mathieu-Daudé wrote:
> On 3/13/20 10:14 PM, BALATON Zoltan wrote:
>> This removes pci_piix4_ide_init() function similar to clean up done to
>> other ide devices.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ide/piix.c    | 12 +-----------
>>   hw/isa/piix4.c   |  5 ++++-
>>   include/hw/ide.h |  1 -
>>   3 files changed, 5 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 8bcd6b72c2..3b2de4c312 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>       }
>>   }
>>   -/* hd_table must contain 4 block drivers */
>> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
>> devfn)
>> -{
>> -    PCIDevice *dev;
>> -
>> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
>> -    pci_ide_create_devs(dev, hd_table);
>> -    return dev;
>> -}
>> -
>>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>   {
>> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>>       .class_init    = piix3_ide_class_init,
>>   };
>>   +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>>   static void piix4_ide_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 7edec5e149..0ab4787658 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -35,6 +35,7 @@
>>   #include "hw/timer/i8254.h"
>>   #include "hw/rtc/mc146818rtc.h"
>>   #include "hw/ide.h"
>> +#include "hw/ide/pci.h"
>>   #include "migration/vmstate.h"
>>   #include "sysemu/reset.h"
>>   #include "sysemu/runstate.h"
>> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
>> **isa_bus,
>>           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>       }
>>   +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>
> Why are you re-assigning 'pci'?

Need a place to store it to pass to pci_ide_create_devs below and pci is 
unused at this point so it can be reused for this.  (The variable pci 
pointing to a PCIDevice was only used at the beginning of the function to 
cast to dev then it's not needed any more.) Since this is very short func 
and the reassign is right after its previous usage this should not be too 
confusing and avoids needing to define another only once used variable fot 
this. See also patch 6 (http://patchwork.ozlabs.org/patch/1254687/) that 
simplifies it further.

We could also do without this variable and write:

dev = DEVICE(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
              true, TYPE_PIIX4_PCI_DEVICE));

or after patch 6 even

pci_ide_create_devs(pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide"));

but I think those are less readable than reusing variable pci here.

Regards,
BALATON Zoltan

>>       hd = g_new(DriveInfo *, ide_drives);
>>       ide_drive_get(hd, ide_drives);
>> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
>> +    pci_ide_create_devs(pci, hd);
>>       g_free(hd);
>> +
>>       pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>>       if (smbus) {
>>           *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 883bbaeb9b..21bd8f23f1 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int 
>> iobase2, int isairq,
>>                           DriveInfo *hd0, DriveInfo *hd1);
>>     /* ide-pci.c */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
>> devfn);
>>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>>     /* ide-mmio.c */
>> 
>
>

On Fri, Mar 13, 2020 at 10:14:34PM +0100, BALATON Zoltan wrote:
> This removes pci_piix4_ide_init() function similar to clean up done to
> other ide devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/piix.c    | 12 +-----------
>  hw/isa/piix4.c   |  5 ++++-
>  include/hw/ide.h |  1 -
>  3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 8bcd6b72c2..3b2de4c312 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>  {
> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>      .class_init    = piix3_ide_class_init,
>  };
>  
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>  static void piix4_ide_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 7edec5e149..0ab4787658 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -35,6 +35,7 @@
>  #include "hw/timer/i8254.h"
>  #include "hw/rtc/mc146818rtc.h"
>  #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/runstate.h"
> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>          *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>      }
>  
> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>      hd = g_new(DriveInfo *, ide_drives);
>      ide_drive_get(hd, ide_drives);
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_ide_create_devs(pci, hd);
>      g_free(hd);
> +

Why not move pci_create_simple down, and declare a new variable?
-    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
+    pci_dev = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
+    pci_ide_create_devs(pci_dev, hd);

makes it clearer what's going on imho.

>      pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>      if (smbus) {
>          *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 883bbaeb9b..21bd8f23f1 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                          DriveInfo *hd0, DriveInfo *hd1);
>  
>  /* ide-pci.c */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>  
>  /* ide-mmio.c */
> -- 
> 2.21.1

On Sun, 15 Mar 2020, Michael S. Tsirkin wrote:
> On Fri, Mar 13, 2020 at 10:14:34PM +0100, BALATON Zoltan wrote:
>> This removes pci_piix4_ide_init() function similar to clean up done to
>> other ide devices.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/piix.c    | 12 +-----------
>>  hw/isa/piix4.c   |  5 ++++-
>>  include/hw/ide.h |  1 -
>>  3 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 8bcd6b72c2..3b2de4c312 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>      }
>>  }
>>
>> -/* hd_table must contain 4 block drivers */
>> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
>> -{
>> -    PCIDevice *dev;
>> -
>> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
>> -    pci_ide_create_devs(dev, hd_table);
>> -    return dev;
>> -}
>> -
>>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>  {
>> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>>      .class_init    = piix3_ide_class_init,
>>  };
>>
>> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>>  static void piix4_ide_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 7edec5e149..0ab4787658 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -35,6 +35,7 @@
>>  #include "hw/timer/i8254.h"
>>  #include "hw/rtc/mc146818rtc.h"
>>  #include "hw/ide.h"
>> +#include "hw/ide/pci.h"
>>  #include "migration/vmstate.h"
>>  #include "sysemu/reset.h"
>>  #include "sysemu/runstate.h"
>> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>>          *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>      }
>>
>> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>>      hd = g_new(DriveInfo *, ide_drives);
>>      ide_drive_get(hd, ide_drives);
>> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
>> +    pci_ide_create_devs(pci, hd);
>>      g_free(hd);
>> +
>
> Why not move pci_create_simple down, and declare a new variable?
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_dev = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
> +    pci_ide_create_devs(pci_dev, hd);
>
> makes it clearer what's going on imho.

Ends up there after patch 6. Do you still think a new variable would be 
needed for this after that patch? It's pretty simple and clear without all 
the hd array stuff even reusing pci variable. (Or I could rename pci to 
pci_dev but really don't think it worth having two once used variable in 
such a simple function. Normally such variables are called dev but in this 
function that name is taken for a DeviceState *variable.)

Regards,
BALATON Zoltan

>
>>      pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>>      if (smbus) {
>>          *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 883bbaeb9b..21bd8f23f1 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>>                          DriveInfo *hd0, DriveInfo *hd1);
>>
>>  /* ide-pci.c */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>>
>>  /* ide-mmio.c */
>> --
>> 2.21.1
>
>