Hi Peter, On Tue, Sep 8, 2026 at 4:18 PM Peter Maydell <[email protected]> wrote:
> On Sat, 29 Aug 2026 at 13:56, Strahinja Jankovic > <[email protected]> wrote: > > > > Add Allwinner GPIO set of qtests to validate functionality. > > Tests cover input and output port setting, as well as interrupt > > functionality. > > > > Signed-off-by: Strahinja Jankovic <[email protected]> > > --- > > > > +static void test_irq_pin_rising(const void *data) > > +{ > > + PortPinTestParam *param = (PortPinTestParam *)data; > > + QTestState *s = qtest_init("-machine cubieboard"); > > + const int gpio_port = param->port; > > + const int port_pin = param->pin; > > + g_autofree char *in_name = portname_in(gpio_port); > > + int irq_pin = irq_nr(gpio_port, port_pin); > > + unsigned int irq_mask = 1u << irq_pin; > > + > > + qtest_irq_intercept_in(s, "/machine/soc"); > > + > > + /* Configure as input and set pin low */ > > + update_cfg_reg(s, gpio_port, port_pin, AW_GPIO_CFG_IN); > > + assert_cfg_regval(s, gpio_port, port_pin, AW_GPIO_CFG_IN); > > + qtest_set_irq_in( > > + s, "/machine/soc/gpio", in_name, port_pin, AW_GPIO_LEVEL_LOW); > > Breaking the line immediately after the opening "(" looks odd; > generally put the first parameter there and break the line > after that where it makes sense. > I will change this, had a lot of issues with the line width, so I will update all the places where I did a break like this. > > > + > > + /* Enable interrupt */ > > + update_int_cfg_reg(s, irq_pin, AW_GPIO_IRQ_CFG_RISING_EDGE); > > + assert_int_cfg_regval(s, irq_pin, AW_GPIO_IRQ_CFG_RISING_EDGE); > > + qtest_writel(s, GPIO_INT_CTL_ADDR, irq_mask); > > + > > + /* Raise interrupt */ > > + qtest_set_irq_in( > > + s, "/machine/soc/gpio", in_name, port_pin, AW_GPIO_LEVEL_HIGH); > > + /* Check that value is written */ > > + assert_data_reg(s, gpio_port, port_pin, AW_GPIO_LEVEL_HIGH); > > + /* Check interrupt status */ > > + g_assert_cmphex(qtest_readl(s, GPIO_INT_STA_ADDR), ==, irq_mask); > > + g_assert_true(qtest_get_irq(s, AW_A10_IRQ_LINE)); > > + > > + /* Clear interrupt */ > > + qtest_writel(s, GPIO_INT_STA_ADDR, irq_mask); > > + > > + /* Check that IRQ line is low */ > > + g_assert_cmphex(qtest_readl(s, GPIO_INT_STA_ADDR) & irq_mask, ==, > 0); > > + g_assert_false(qtest_get_irq(s, AW_A10_IRQ_LINE)); > > + > > + qtest_quit(s); > > +} > > > > > +int main(int argc, char **argv) > > +{ > > + int r; > > + > > + g_test_init(&argc, &argv, NULL); > > + > > + for (int i = 0; i < G_N_ELEMENTS(in_out_test_parameters); i++) { > > Prefer ARRAY_SIZE() over G_N_ELEMENTS(). > Ok! > > > + g_autofree char *out_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/set_output_pins/%s%d", > > + portname(in_out_test_parameters[i].port), > in_out_test_parameters[i].pin); > > + g_autofree char *in_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/set_input_pins/%s%d", > > + portname(in_out_test_parameters[i].port), > in_out_test_parameters[i].pin); > > This has turned into an unreadable lump especially given the > lack of indentation. I suggest factoring out the creation of the > test: > static char *add_test(const char *type, const PortPinTestParam *p, > void (*testfn)(const void *)) > { > g_autofree char *name = > g_strdup_printf("/allwinner-cubieboard/gpio/%s/%s%d", > type, portname(p->port), p->pin); > > qtest_add_data_func(name, p, testfn); > } > > Then you in your loop you can do: > > PortPinTestParam *p = &in_out_test_parameters[i]; > > add_test("set_output_pins", p, test_set_output_pins); > add_test("set_input_pins", p, test_set_input_pins); > > Now that you point it out, it has. I was adding line after line just to add all tests, but I forgot to take another look to prettify it. Thanks for the suggestion, I will use it! > > > + qtest_add_data_func(out_name, &in_out_test_parameters[i], > > + test_set_output_pins); > > + qtest_add_data_func(in_name, &in_out_test_parameters[i], > > + test_set_input_pins); > > + } > > + qtest_add_func("/allwinner-cubieboard/gpio/reset_values", > > + test_reset_values); > > + for (int i = 0; i < G_N_ELEMENTS(irq_test_parameters); i++) { > > + g_autofree char *rising_edge_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/irq_pin_rising/%s%d", > > + portname(irq_test_parameters[i].port), irq_test_parameters[i].pin); > > + qtest_add_data_func( > > + rising_edge_name, &irq_test_parameters[i], > test_irq_pin_rising); > > + g_autofree char *falling_edge_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/irq_pin_falling/%s%d", > > + portname(irq_test_parameters[i].port), irq_test_parameters[i].pin); > > add_test() should make this loop more readable too. > Ok! Thank you very much for your thorough review and help, I will make the changes and send a new version. Best regards, Strahinja Jankovic > > > + qtest_add_data_func( > > + falling_edge_name, &irq_test_parameters[i], > test_irq_pin_falling); > > + g_autofree char *high_level_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/irq_pin_high_level/%s%d", > > + portname(irq_test_parameters[i].port), irq_test_parameters[i].pin); > > + qtest_add_data_func( > > + high_level_name, &irq_test_parameters[i], > test_irq_pin_high_level); > > + g_autofree char *low_level_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/irq_pin_low_level/%s%d", > > + portname(irq_test_parameters[i].port), irq_test_parameters[i].pin); > > + qtest_add_data_func( > > + low_level_name, &irq_test_parameters[i], > test_irq_pin_low_level); > > + g_autofree char *both_edge_name = g_strdup_printf( > > + "/allwinner-cubieboard/gpio/irq_pin_both_edge/%s%d", > > + portname(irq_test_parameters[i].port), irq_test_parameters[i].pin); > > + qtest_add_data_func( > > + both_edge_name, &irq_test_parameters[i], > test_irq_pin_both_edge); > > + > > + } > > + r = g_test_run(); > > + > > + return r; > > +} > > thanks > -- PMM >
