| Message ID | 20220212113519.69527-6-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] malta: Move PCI interrupt handling from gt64xxx to piix4 | expand |
On Sat, 12 Feb 2022, Bernhard Beschow wrote: > This is a follow-up on patch "malta: Move PCI interrupt handling from > gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State > to make the code movement more obvious. However, i8259[] seems redundant > to *isa, so remove it. Should this be squashed in patch 1 or at least come after it? Or are there some other changes needed before this patch? Regards, BALATON Zoltan > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/piix4.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > index a9322851db..692fa8d1f9 100644 > --- a/hw/isa/piix4.c > +++ b/hw/isa/piix4.c > @@ -43,7 +43,6 @@ struct PIIX4State { > PCIDevice dev; > qemu_irq cpu_intr; > qemu_irq *isa; > - qemu_irq i8259[ISA_NUM_IRQS]; > > int32_t pci_irq_levels[PIIX_NUM_PIRQS]; > > @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level) > pic_level |= s->pci_irq_levels[i]; > } > } > - qemu_set_irq(s->i8259[pic_irq], pic_level); > + qemu_set_irq(s->isa[pic_irq], pic_level); > } > } > > @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) > > pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); > > - for (int i = 0; i < ISA_NUM_IRQS; i++) { > - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); > - } > - > return dev; > } >
Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 12 Feb 2022, Bernhard Beschow wrote: >> This is a follow-up on patch "malta: Move PCI interrupt handling from >> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State >> to make the code movement more obvious. However, i8259[] seems redundant >> to *isa, so remove it. > >Should this be squashed in patch 1 or at least come after it? Or are there >some other changes needed before this patch? I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3. Regards, Bernhard > >Regards, >BALATON Zoltan > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/piix4.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >> index a9322851db..692fa8d1f9 100644 >> --- a/hw/isa/piix4.c >> +++ b/hw/isa/piix4.c >> @@ -43,7 +43,6 @@ struct PIIX4State { >> PCIDevice dev; >> qemu_irq cpu_intr; >> qemu_irq *isa; >> - qemu_irq i8259[ISA_NUM_IRQS]; >> >> int32_t pci_irq_levels[PIIX_NUM_PIRQS]; >> >> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level) >> pic_level |= s->pci_irq_levels[i]; >> } >> } >> - qemu_set_irq(s->i8259[pic_irq], pic_level); >> + qemu_set_irq(s->isa[pic_irq], pic_level); >> } >> } >> >> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) >> >> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); >> >> - for (int i = 0; i < ISA_NUM_IRQS; i++) { >> - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); >> - } >> - >> return dev; >> } >>
Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 12 Feb 2022, Bernhard Beschow wrote: >> This is a follow-up on patch "malta: Move PCI interrupt handling from >> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State >> to make the code movement more obvious. However, i8259[] seems redundant >> to *isa, so remove it. > >Should this be squashed in patch 1 or at least come after it? Or are there >some other changes needed before this patch? I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3. Regards, Bernhard > >Regards, >BALATON Zoltan > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/piix4.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >> index a9322851db..692fa8d1f9 100644 >> --- a/hw/isa/piix4.c >> +++ b/hw/isa/piix4.c >> @@ -43,7 +43,6 @@ struct PIIX4State { >> PCIDevice dev; >> qemu_irq cpu_intr; >> qemu_irq *isa; >> - qemu_irq i8259[ISA_NUM_IRQS]; >> >> int32_t pci_irq_levels[PIIX_NUM_PIRQS]; >> >> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level) >> pic_level |= s->pci_irq_levels[i]; >> } >> } >> - qemu_set_irq(s->i8259[pic_irq], pic_level); >> + qemu_set_irq(s->isa[pic_irq], pic_level); >> } >> } >> >> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) >> >> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); >> >> - for (int i = 0; i < ISA_NUM_IRQS; i++) { >> - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); >> - } >> - >> return dev; >> } >>
On Sat, 12 Feb 2022, Bernhard Beschow wrote: > Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 12 Feb 2022, Bernhard Beschow wrote: >>> This is a follow-up on patch "malta: Move PCI interrupt handling from >>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State >>> to make the code movement more obvious. However, i8259[] seems redundant >>> to *isa, so remove it. >> >> Should this be squashed in patch 1 or at least come after it? Or are there >> some other changes needed before this patch? > > I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3. I had another comment in the bottom of this message, not sure you've found that too, I forgot to put a warning that there are more comments below here. Changing the patch order or patches according to review is OK and adding a new patch between already reviewed ones is OK too then you can keep Reviewed-by on patches that did not change. Regards, BALATON Zoltan > Regards, > Bernhard > >> >> Regards, >> BALATON Zoltan >> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/piix4.c | 7 +------ >>> 1 file changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >>> index a9322851db..692fa8d1f9 100644 >>> --- a/hw/isa/piix4.c >>> +++ b/hw/isa/piix4.c >>> @@ -43,7 +43,6 @@ struct PIIX4State { >>> PCIDevice dev; >>> qemu_irq cpu_intr; >>> qemu_irq *isa; >>> - qemu_irq i8259[ISA_NUM_IRQS]; >>> >>> int32_t pci_irq_levels[PIIX_NUM_PIRQS]; >>> >>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level) >>> pic_level |= s->pci_irq_levels[i]; >>> } >>> } >>> - qemu_set_irq(s->i8259[pic_irq], pic_level); >>> + qemu_set_irq(s->isa[pic_irq], pic_level); >>> } >>> } >>> >>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) >>> >>> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); >>> >>> - for (int i = 0; i < ISA_NUM_IRQS; i++) { >>> - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); >>> - } >>> - >>> return dev; >>> } >>> > >
On Sat, 12 Feb 2022, BALATON Zoltan wrote: > On Sat, 12 Feb 2022, Bernhard Beschow wrote: >> Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan >> <balaton@eik.bme.hu>: >>> On Sat, 12 Feb 2022, Bernhard Beschow wrote: >>>> This is a follow-up on patch "malta: Move PCI interrupt handling from >>>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State >>>> to make the code movement more obvious. However, i8259[] seems redundant >>>> to *isa, so remove it. >>> >>> Should this be squashed in patch 1 or at least come after it? Or are there >>> some other changes needed before this patch? >> >> I didn't want to change the patch order since they've been reviewed >> already. But yeah, you're right: It makes sense to get this out of the way >> early in the patch series. I will do a v3. > > I had another comment in the bottom of this message, not sure you've found > that too, I forgot to put a warning that there are more comments below here. I mean at the end of patch 1 not this one, sorry was too quick to send it. > Changing the patch order or patches according to review is OK and adding a > new patch between already reviewed ones is OK too then you can keep > Reviewed-by on patches that did not change. > > Regards, > BALATON Zoltan > >> Regards, >> Bernhard >> >>> >>> Regards, >>> BALATON Zoltan >>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/isa/piix4.c | 7 +------ >>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>> >>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >>>> index a9322851db..692fa8d1f9 100644 >>>> --- a/hw/isa/piix4.c >>>> +++ b/hw/isa/piix4.c >>>> @@ -43,7 +43,6 @@ struct PIIX4State { >>>> PCIDevice dev; >>>> qemu_irq cpu_intr; >>>> qemu_irq *isa; >>>> - qemu_irq i8259[ISA_NUM_IRQS]; >>>> >>>> int32_t pci_irq_levels[PIIX_NUM_PIRQS]; >>>> >>>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, >>>> int level) >>>> pic_level |= s->pci_irq_levels[i]; >>>> } >>>> } >>>> - qemu_set_irq(s->i8259[pic_irq], pic_level); >>>> + qemu_set_irq(s->isa[pic_irq], pic_level); >>>> } >>>> } >>>> >>>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus >>>> **isa_bus, I2CBus **smbus) >>>> >>>> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, >>>> PIIX_NUM_PIRQS); >>>> >>>> - for (int i = 0; i < ISA_NUM_IRQS; i++) { >>>> - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); >>>> - } >>>> - >>>> return dev; >>>> } >>>> >> >> >
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c index a9322851db..692fa8d1f9 100644 --- a/hw/isa/piix4.c +++ b/hw/isa/piix4.c @@ -43,7 +43,6 @@ struct PIIX4State { PCIDevice dev; qemu_irq cpu_intr; qemu_irq *isa; - qemu_irq i8259[ISA_NUM_IRQS]; int32_t pci_irq_levels[PIIX_NUM_PIRQS]; @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level) pic_level |= s->pci_irq_levels[i]; } } - qemu_set_irq(s->i8259[pic_irq], pic_level); + qemu_set_irq(s->isa[pic_irq], pic_level); } } @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); - for (int i = 0; i < ISA_NUM_IRQS; i++) { - s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); - } - return dev; }
This is a follow-up on patch "malta: Move PCI interrupt handling from gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State to make the code movement more obvious. However, i8259[] seems redundant to *isa, so remove it. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/piix4.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)