Message ID | 20230726030450.757462-4-chris@laplante.io |
---|---|
State | New |
Headers | show |
Series | Add nRF51 DETECT signal with test | expand |
On Wed, 26 Jul 2023 at 04:32, Chris Laplante <chris@laplante.io> wrote: > > Adds qtest_irq_intercept_out_named method, which utilizes a new optional > name parameter to the irq_intercept_out qtest command. > > Signed-off-by: Chris Laplante <chris@laplante.io> > --- > softmmu/qtest.c | 24 ++++++++++++++++-------- > tests/qtest/libqtest.c | 6 ++++++ > tests/qtest/libqtest.h | 11 +++++++++++ > 3 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/softmmu/qtest.c b/softmmu/qtest.c > index 1c92e5a6a3..7fd8546ed2 100644 > --- a/softmmu/qtest.c > +++ b/softmmu/qtest.c > @@ -397,8 +397,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > || strcmp(words[0], "irq_intercept_in") == 0) { > DeviceState *dev; > NamedGPIOList *ngl; > + bool is_named; > + bool is_outbound; > > g_assert(words[1]); > + is_named = words[2] != NULL; > + is_outbound = words[0][14] == 'o'; > dev = DEVICE(object_resolve_path(words[1], NULL)); > if (!dev) { > qtest_send_prefix(chr); > @@ -417,14 +421,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words) > } > > QLIST_FOREACH(ngl, &dev->gpios, node) { > - /* We don't support intercept of named GPIOs yet */ > - if (ngl->name) { > - continue; > - } > - if (words[0][14] == 'o') { > - int i; > - for (i = 0; i < ngl->num_out; ++i) { > - qtest_install_gpio_out_intercept(dev, ngl->name, i); > + /* We don't support inbound interception of named GPIOs yet */ > + if (is_outbound) { > + if (is_named) { > + if (ngl->name && strcmp(ngl->name, words[2]) == 0) { > + qtest_install_gpio_out_intercept(dev, ngl->name, 0); > + break; > + } Named gpio-outs can have more than one line, the same as unnamed ones (you create them with qdev_init_gpio_out_named(dev, pins, name, n) -- there's an argument for how many to create), so I think this is_named branch also needs to install an intercept for each one, not just for 0. > + } else if (!ngl->name) { > + int i; > + for (i = 0; i < ngl->num_out; ++i) { > + qtest_install_gpio_out_intercept(dev, ngl->name, i); > + } ...at which point the code looks pretty similar in both branches of the if(), so I think you end up with something like if (is_outbound) { /* NULL is valid and matchable, for "unnamed GPIO" */ if (g_strcmp0(ngl->name, words[2]) == 0) { int i; ; for (i = 0; i < ngl->num_out; ++i) { qtest_install_gpio_out_intercept(dev, ngl->name, i); } } } ... (g_strcmp0() can handle the NULL case without having to special case it -- this is how qdev_get_named_gpio_list() finds entries in the ngl list.) Apologies for not noticing that on the first round of review. thanks -- PMM
> (g_strcmp0() can handle the NULL case without having > to special case it -- this is how qdev_get_named_gpio_list() > finds entries in the ngl list.) > > Apologies for not noticing that on the first round of review. No worries - it makes the code much simpler anyway. Should we bother factoring out qtest_install_gpio_out_intercept still? It is only used in one place now, as before. Thanks, Chris
On Thu, 27 Jul 2023 at 22:32, Chris Laplante <chris@laplante.io> wrote: > > > > (g_strcmp0() can handle the NULL case without having > > to special case it -- this is how qdev_get_named_gpio_list() > > finds entries in the ngl list.) > > > > Apologies for not noticing that on the first round of review. > > No worries - it makes the code much simpler anyway. Should we bother factoring out qtest_install_gpio_out_intercept still? It is only used in one place now, as before. I'd keep the function, I think, since you've already written it. It's kind of usefully documenting of the intent. -- PMM
diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 1c92e5a6a3..7fd8546ed2 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -397,8 +397,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words) || strcmp(words[0], "irq_intercept_in") == 0) { DeviceState *dev; NamedGPIOList *ngl; + bool is_named; + bool is_outbound; g_assert(words[1]); + is_named = words[2] != NULL; + is_outbound = words[0][14] == 'o'; dev = DEVICE(object_resolve_path(words[1], NULL)); if (!dev) { qtest_send_prefix(chr); @@ -417,14 +421,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words) } QLIST_FOREACH(ngl, &dev->gpios, node) { - /* We don't support intercept of named GPIOs yet */ - if (ngl->name) { - continue; - } - if (words[0][14] == 'o') { - int i; - for (i = 0; i < ngl->num_out; ++i) { - qtest_install_gpio_out_intercept(dev, ngl->name, i); + /* We don't support inbound interception of named GPIOs yet */ + if (is_outbound) { + if (is_named) { + if (ngl->name && strcmp(ngl->name, words[2]) == 0) { + qtest_install_gpio_out_intercept(dev, ngl->name, 0); + break; + } + } else if (!ngl->name) { + int i; + for (i = 0; i < ngl->num_out; ++i) { + qtest_install_gpio_out_intercept(dev, ngl->name, i); + } } } else { qemu_irq_intercept_in(ngl->in, qtest_irq_handler, diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index c22dfc30d3..471529e6cc 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -993,6 +993,12 @@ void qtest_irq_intercept_out(QTestState *s, const char *qom_path) qtest_rsp(s); } +void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name) +{ + qtest_sendf(s, "irq_intercept_out %s %s\n", qom_path, name); + qtest_rsp(s); +} + void qtest_irq_intercept_in(QTestState *s, const char *qom_path) { qtest_sendf(s, "irq_intercept_in %s\n", qom_path); diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index 3a71bc45fc..e53e350e3a 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -371,6 +371,17 @@ void qtest_irq_intercept_in(QTestState *s, const char *string); */ void qtest_irq_intercept_out(QTestState *s, const char *string); +/** + * qtest_irq_intercept_out_named: + * @s: #QTestState instance to operate on. + * @qom_path: QOM path of a device. + * @name: Name of the GPIO out pin + * + * Associate a qtest irq with the named GPIO-out pin of the device + * whose path is specified by @string and whose name is @name. + */ +void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const char *name); + /** * qtest_set_irq_in: * @s: QTestState instance to operate on.
Adds qtest_irq_intercept_out_named method, which utilizes a new optional name parameter to the irq_intercept_out qtest command. Signed-off-by: Chris Laplante <chris@laplante.io> --- softmmu/qtest.c | 24 ++++++++++++++++-------- tests/qtest/libqtest.c | 6 ++++++ tests/qtest/libqtest.h | 11 +++++++++++ 3 files changed, 33 insertions(+), 8 deletions(-)