| Message ID | 20230422150728.176512-8-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up PCI IDE device models | expand |
On 22/4/23 17:07, Bernhard Beschow wrote: > There is redundant code in cmd646 and via which can be extracted into the base > class. In case of piix and sii3112 this is currently unneccessary but shouldn't > interfere since the memory regions aren't mapped by those devices. In few > commits later this will be changed, i.e. those device models will also make use > of these memory regions. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/ide/cmd646.c | 11 ----------- > hw/ide/pci.c | 10 ++++++++++ > hw/ide/via.c | 11 ----------- > 3 files changed, 10 insertions(+), 22 deletions(-) > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 65ed6f7f72..a9194313bd 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -543,6 +543,16 @@ static void pci_ide_init(Object *obj) > { > PCIIDEState *d = PCI_IDE(obj); > > + memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > + &d->bus[0], "pci-ide0-data-ops", 8); > + memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > + &d->bus[0], "pci-ide0-cmd-ops", 4); > + > + memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > + &d->bus[1], "pci-ide1-data-ops", 8); > + memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > + &d->bus[1], "pci-ide1-cmd-ops", 4); The trailing "-ops" just adds noise IMO. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 23. April 2023 17:46:18 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 22/4/23 17:07, Bernhard Beschow wrote: >> There is redundant code in cmd646 and via which can be extracted into the base >> class. In case of piix and sii3112 this is currently unneccessary but shouldn't >> interfere since the memory regions aren't mapped by those devices. In few >> commits later this will be changed, i.e. those device models will also make use >> of these memory regions. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/ide/cmd646.c | 11 ----------- >> hw/ide/pci.c | 10 ++++++++++ >> hw/ide/via.c | 11 ----------- >> 3 files changed, 10 insertions(+), 22 deletions(-) > > >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index 65ed6f7f72..a9194313bd 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -543,6 +543,16 @@ static void pci_ide_init(Object *obj) >> { >> PCIIDEState *d = PCI_IDE(obj); >> + memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, >> + &d->bus[0], "pci-ide0-data-ops", 8); >> + memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, >> + &d->bus[0], "pci-ide0-cmd-ops", 4); >> + >> + memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, >> + &d->bus[1], "pci-ide1-data-ops", 8); >> + memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, >> + &d->bus[1], "pci-ide1-cmd-ops", 4); > >The trailing "-ops" just adds noise IMO. I'll remove them in the next iteration. Best regards, Bernhard > >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On 22/04/2023 16:07, Bernhard Beschow wrote: > There is redundant code in cmd646 and via which can be extracted into the base > class. In case of piix and sii3112 this is currently unneccessary but shouldn't > interfere since the memory regions aren't mapped by those devices. In few > commits later this will be changed, i.e. those device models will also make use > of these memory regions. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/ide/cmd646.c | 11 ----------- > hw/ide/pci.c | 10 ++++++++++ > hw/ide/via.c | 11 ----------- > 3 files changed, 10 insertions(+), 22 deletions(-) > > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 6fd09fe74e..85716aaf17 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -251,20 +251,9 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[MRDMODE] = 0x0; > dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; > > - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > - &d->bus[0], "cmd646-data0", 8); > pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - > - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > - &d->bus[0], "cmd646-cmd0", 4); > pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - > - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > - &d->bus[1], "cmd646-data1", 8); > pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - > - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > - &d->bus[1], "cmd646-cmd1", 4); > pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > > bmdma_init_ops(d, &cmd646_bmdma_ops); > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 65ed6f7f72..a9194313bd 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -543,6 +543,16 @@ static void pci_ide_init(Object *obj) > { > PCIIDEState *d = PCI_IDE(obj); > > + memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > + &d->bus[0], "pci-ide0-data-ops", 8); > + memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > + &d->bus[0], "pci-ide0-cmd-ops", 4); > + > + memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > + &d->bus[1], "pci-ide1-data-ops", 8); > + memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > + &d->bus[1], "pci-ide1-cmd-ops", 4); > + > qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); > } > > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 40704e2857..704a8024cb 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -154,20 +154,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[PCI_INTERRUPT_LINE] = 0; > dev->wmask[PCI_CLASS_PROG] = 5; > > - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > - &d->bus[0], "via-ide0-data", 8); > pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - > - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > - &d->bus[0], "via-ide0-cmd", 4); > pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - > - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > - &d->bus[1], "via-ide1-data", 8); > pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - > - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > - &d->bus[1], "via-ide1-cmd", 4); > pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > > bmdma_init_ops(d, &via_bmdma_ops); I'd also be inclined to agree with Phil/Zoltan re: dropping the trailing "-ops" in the name, otherwise: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 6fd09fe74e..85716aaf17 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -251,20 +251,9 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) dev->wmask[MRDMODE] = 0x0; dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, - &d->bus[0], "cmd646-data0", 8); pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); - - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, - &d->bus[0], "cmd646-cmd0", 4); pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); - - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, - &d->bus[1], "cmd646-data1", 8); pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); - - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, - &d->bus[1], "cmd646-cmd1", 4); pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); bmdma_init_ops(d, &cmd646_bmdma_ops); diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 65ed6f7f72..a9194313bd 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -543,6 +543,16 @@ static void pci_ide_init(Object *obj) { PCIIDEState *d = PCI_IDE(obj); + memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, + &d->bus[0], "pci-ide0-data-ops", 8); + memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, + &d->bus[0], "pci-ide0-cmd-ops", 4); + + memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, + &d->bus[1], "pci-ide1-data-ops", 8); + memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, + &d->bus[1], "pci-ide1-cmd-ops", 4); + qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); } diff --git a/hw/ide/via.c b/hw/ide/via.c index 40704e2857..704a8024cb 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -154,20 +154,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) dev->wmask[PCI_INTERRUPT_LINE] = 0; dev->wmask[PCI_CLASS_PROG] = 5; - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, - &d->bus[0], "via-ide0-data", 8); pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); - - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, - &d->bus[0], "via-ide0-cmd", 4); pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); - - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, - &d->bus[1], "via-ide1-data", 8); pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); - - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, - &d->bus[1], "via-ide1-cmd", 4); pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); bmdma_init_ops(d, &via_bmdma_ops);
There is redundant code in cmd646 and via which can be extracted into the base class. In case of piix and sii3112 this is currently unneccessary but shouldn't interfere since the memory regions aren't mapped by those devices. In few commits later this will be changed, i.e. those device models will also make use of these memory regions. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/ide/cmd646.c | 11 ----------- hw/ide/pci.c | 10 ++++++++++ hw/ide/via.c | 11 ----------- 3 files changed, 10 insertions(+), 22 deletions(-)