| Message ID | 20211105172127.224462-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/qdev: Clarify qdev_connect_gpio_out() documentation | expand |
On 2021/11/6 1:21, Philippe Mathieu-Daudé wrote: > 'a20_out' is an input IRQ, rename it as 'a20_input'. > i8042_setup_a20_line() doesn't take a Device parameter > but an ISADevice one. Rename it as i8042_isa_*() to > make it explicit. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/input/i8042.h | 2 +- > hw/i386/pc.c | 2 +- > hw/input/pckbd.c | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h > index 1d90432daef..3534fcc4b43 100644 > --- a/include/hw/input/i8042.h > +++ b/include/hw/input/i8042.h > @@ -21,6 +21,6 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq, > MemoryRegion *region, ram_addr_t size, > hwaddr mask); > void i8042_isa_mouse_fake_event(ISAKBDState *isa); > -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out); > +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input); > > #endif /* HW_INPUT_I8042_H */ > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 2592a821486..06ef74ca22b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1043,7 +1043,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) > port92 = isa_create_simple(isa_bus, TYPE_PORT92); > > a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); > - i8042_setup_a20_line(i8042, a20_line[0]); > + i8042_isa_setup_a20_line(i8042, a20_line[0]); > qdev_connect_gpio_out_named(DEVICE(port92), > PORT92_A20_LINE, 0, a20_line[1]); > g_free(a20_line); > diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c > index baba62f357a..7b842c8f3b5 100644 > --- a/hw/input/pckbd.c > +++ b/hw/input/pckbd.c > @@ -680,9 +680,9 @@ void i8042_isa_mouse_fake_event(ISAKBDState *isa) > ps2_mouse_fake_event(s->mouse); > } > > -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out) > +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input) > { > - qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out); > + qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_input); > } > > static const VMStateDescription vmstate_kbd_isa = { Looks reasonable: Reviewed-by: Yanan Wang <wangyanan55@huawei.com> Thanks, Yanan
On Fri, 5 Nov 2021 at 17:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > 'a20_out' is an input IRQ, rename it as 'a20_input'. > i8042_setup_a20_line() doesn't take a Device parameter > but an ISADevice one. Rename it as i8042_isa_*() to > make it explicit. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/input/i8042.h | 2 +- > hw/i386/pc.c | 2 +- > hw/input/pckbd.c | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h > index 1d90432daef..3534fcc4b43 100644 > --- a/include/hw/input/i8042.h > +++ b/include/hw/input/i8042.h > @@ -21,6 +21,6 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq, > MemoryRegion *region, ram_addr_t size, > hwaddr mask); > void i8042_isa_mouse_fake_event(ISAKBDState *isa); > -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out); > +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input); > > #endif /* HW_INPUT_I8042_H */ > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 2592a821486..06ef74ca22b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1043,7 +1043,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) > port92 = isa_create_simple(isa_bus, TYPE_PORT92); > > a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); > - i8042_setup_a20_line(i8042, a20_line[0]); > + i8042_isa_setup_a20_line(i8042, a20_line[0]); I think these days we can directly call qdev_connect_gpio_out_named(DEVICE(i8042), I8042_A20_LINE, 0, a20_line[0]); and drop the i8042_setup_a20_line() wrapper entirely, since the named GPIO lines are a "public" interface to the device. We only have this i8042_setup_a20_line() because the original implementation (added in commit 956a3e6bb738) predates gpio lines and did an assignment into the KBDState struct which needed to be private to pckbd.c. > qdev_connect_gpio_out_named(DEVICE(port92), > PORT92_A20_LINE, 0, a20_line[1]); That would then make it consistent with how we're wiring up the other A20 input source here. (Some day we should perhaps make the A20 input to the CPU an actual GPIO input on the CPU device object, so we could wire the I8042_A20_LINE and PORT92_A20_LINE to it via an OR gate, and drop the intermidate qemu_irq array and handle_a20_line_change function. But needing the OR gate makes that a little clunky so I'm not sure it's really worth the effort.) -- PMM
On 11/22/21 12:14, Peter Maydell wrote: > On Fri, 5 Nov 2021 at 17:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> 'a20_out' is an input IRQ, rename it as 'a20_input'. >> i8042_setup_a20_line() doesn't take a Device parameter >> but an ISADevice one. Rename it as i8042_isa_*() to >> make it explicit. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/input/i8042.h | 2 +- >> hw/i386/pc.c | 2 +- >> hw/input/pckbd.c | 4 ++-- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h >> index 1d90432daef..3534fcc4b43 100644 >> --- a/include/hw/input/i8042.h >> +++ b/include/hw/input/i8042.h >> @@ -21,6 +21,6 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq, >> MemoryRegion *region, ram_addr_t size, >> hwaddr mask); >> void i8042_isa_mouse_fake_event(ISAKBDState *isa); >> -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out); >> +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input); >> >> #endif /* HW_INPUT_I8042_H */ >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 2592a821486..06ef74ca22b 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1043,7 +1043,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) >> port92 = isa_create_simple(isa_bus, TYPE_PORT92); >> >> a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); >> - i8042_setup_a20_line(i8042, a20_line[0]); >> + i8042_isa_setup_a20_line(i8042, a20_line[0]); > > I think these days we can directly call > qdev_connect_gpio_out_named(DEVICE(i8042), I8042_A20_LINE, 0, a20_line[0]); > and drop the i8042_setup_a20_line() wrapper entirely, > since the named GPIO lines are a "public" interface to the device. > We only have this i8042_setup_a20_line() because the original > implementation (added in commit 956a3e6bb738) predates gpio lines > and did an assignment into the KBDState struct which needed to > be private to pckbd.c. > >> qdev_connect_gpio_out_named(DEVICE(port92), >> PORT92_A20_LINE, 0, a20_line[1]); > > That would then make it consistent with how we're wiring up the > other A20 input source here. Very good point, thank you. > (Some day we should perhaps make the A20 input to the CPU an actual > GPIO input on the CPU device object, so we could wire the I8042_A20_LINE > and PORT92_A20_LINE to it via an OR gate, and drop the intermidate > qemu_irq array and handle_a20_line_change function. But needing > the OR gate makes that a little clunky so I'm not sure it's > really worth the effort.) I once worked on an "info irqtree" HMP command; if I finish it then would be nice to display.
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h index 1d90432daef..3534fcc4b43 100644 --- a/include/hw/input/i8042.h +++ b/include/hw/input/i8042.h @@ -21,6 +21,6 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq, MemoryRegion *region, ram_addr_t size, hwaddr mask); void i8042_isa_mouse_fake_event(ISAKBDState *isa); -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out); +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input); #endif /* HW_INPUT_I8042_H */ diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 2592a821486..06ef74ca22b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1043,7 +1043,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) port92 = isa_create_simple(isa_bus, TYPE_PORT92); a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2); - i8042_setup_a20_line(i8042, a20_line[0]); + i8042_isa_setup_a20_line(i8042, a20_line[0]); qdev_connect_gpio_out_named(DEVICE(port92), PORT92_A20_LINE, 0, a20_line[1]); g_free(a20_line); diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c index baba62f357a..7b842c8f3b5 100644 --- a/hw/input/pckbd.c +++ b/hw/input/pckbd.c @@ -680,9 +680,9 @@ void i8042_isa_mouse_fake_event(ISAKBDState *isa) ps2_mouse_fake_event(s->mouse); } -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out) +void i8042_isa_setup_a20_line(ISADevice *dev, qemu_irq a20_input) { - qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out); + qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_input); } static const VMStateDescription vmstate_kbd_isa = {
'a20_out' is an input IRQ, rename it as 'a20_input'. i8042_setup_a20_line() doesn't take a Device parameter but an ISADevice one. Rename it as i8042_isa_*() to make it explicit. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/input/i8042.h | 2 +- hw/i386/pc.c | 2 +- hw/input/pckbd.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)