Re: [PATCH] gpiolib: reset lookup flags for each hogged line

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] gpiolib: kunit: avoid duplicate hog test suite in gpiolib-kunit.c
- [Low] gpiolib: drop build/test log and tool commentary from commit message
--

commit 089aff495b510f5bab7031abfc65e971b196d1ef
Author: Jinseob Kim <kimjinseob88@gmail.com>

gpiolib: reset lookup flags for each hogged line

This commit resets the lookup flags variable inside the per-line loop in
gpiochip_add_hog() so that active-low flags do not carry over to subsequent
active-high lines in non-OF multi-line GPIO hogs, and adds a KUnit test case
for mixed-polarity software-node hogs.

>     gpiolib: reset lookup flags for each hogged line
>     
>     gpiochip_add_hog() initializes lflags once before iterating over the GPIO
>     specifiers. In the non-OF path, an active-low line sets GPIO_ACTIVE_LOW,
>     but a later active-high line does not clear it. A single firmware hog
>     with mixed polarities therefore configures later active-high lines as
>     active-low, including inverting their initial output value.
>     
>     Initialize the lookup flags for each line before translating its flags.
>     The OF helper already overwrites the flags for each specifier; the
>     non-OF path must also start with a fresh value.
>     
>     Add a KUnit case using one software-node hog with four alternating line
>     polarities. Check both the descriptor polarity and raw output level for
>     a logical high using the existing software GPIO provider. On UML, the
>     eight existing cases pass before the fix while the new case fails;
>     all nine cases pass after the fix with the same configuration.
>     
>     Clang W=1 and private Sparse C=2 builds of both changed files complete
>     without diagnostics. No physical GPIO hardware was tested.
>     
>     Codex assisted with the source analysis, fix, regression test and commit
>     message.
>     
>     Fixes: 802c51a83e9a ("gpiolib: fix hogs with multiple lines")
>     Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>

[Severity: Low]
This isn't a bug, but the commit message body includes before/after UML test
case counts, Clang W=1 and Sparse C=2 build check notes, a hardware testing
disclaimer, and a paragraph describing Codex assistance.

Since this meta-information is already provided below the tear line in the
patch notes, could these details be dropped from the permanent changelog so
the commit message stays focused on the bug, the fix, and the added test?

> diff --git a/drivers/gpio/gpiolib-kunit.c b/drivers/gpio/gpiolib-kunit.c
> index f005854e7ff7c..01205acf62651 100644
> --- a/drivers/gpio/gpiolib-kunit.c
> +++ b/drivers/gpio/gpiolib-kunit.c
> @@ -862,11 +862,99 @@ static struct kunit_suite gpio_swnode_hog_test_suite = {
>  	.init = gpio_swnode_register_drivers,
>  };
>  
> +/* A single non-OF hog may describe lines with different polarities. */
> +static const struct software_node gpio_mixed_hog_provider_swnode = {
> +	.name = "gpio-mixed-hog-provider",
> +};

[ ... ]

> +static int gpio_mixed_hog_suite_init(struct kunit_suite *suite)
> +{
> +	return software_node_register_node_group(gpio_mixed_hog_swnodes);
> +}
> +
> +static void gpio_mixed_hog_suite_exit(struct kunit_suite *suite)
> +{
> +	software_node_unregister_node_group(gpio_mixed_hog_swnodes);
> +}
> +
> +static struct kunit_case gpio_mixed_hog_tests[] = {
> +	KUNIT_CASE(gpio_hog_mixed_polarities),
> +	{ }
> +};
> +
> +static struct kunit_suite gpio_mixed_hog_test_suite = {
> +	.name = "gpio-mixed-hog-polarity",
> +	.test_cases = gpio_mixed_hog_tests,
> +	.suite_init = gpio_mixed_hog_suite_init,
> +	.suite_exit = gpio_mixed_hog_suite_exit,
> +	.init = gpio_swnode_register_drivers,
> +};

[Severity: Low]
This isn't a bug, but drivers/gpio/gpiolib-kunit.c already defines
gpio_swnode_hog_test_suite to group software-node GPIO hog test cases:

static int gpio_hog_suite_init(struct kunit_suite *suite)
{
	return software_node_register_node_group(gpio_hog_swnodes);
}

static void gpio_hog_suite_exit(struct kunit_suite *suite)
{
	software_node_unregister_node_group(gpio_hog_swnodes);
}

static struct kunit_case gpio_swnode_hog_tests[] = {
	KUNIT_CASE(gpio_hog_output_high),
	KUNIT_CASE(gpio_hog_output_low),
	KUNIT_CASE(gpio_hog_input),
	{ }
};

static struct kunit_suite gpio_swnode_hog_test_suite = {
	.name = "gpio-swnode-hog",
	.test_cases = gpio_swnode_hog_tests,
	.suite_init = gpio_hog_suite_init,
	.suite_exit = gpio_hog_suite_exit,
	.init = gpio_swnode_register_drivers,
};

Could gpio_mixed_hog_provider_swnode and gpio_mixed_hog_swnode be added to
gpio_hog_swnodes (or registered inside gpio_hog_mixed_polarities()), with
KUNIT_CASE(gpio_hog_mixed_polarities) added directly to
gpio_swnode_hog_tests[], instead of duplicating the suite definition and the
gpio_mixed_hog_suite_init() and gpio_mixed_hog_suite_exit() wrappers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103224339.1180-1-kimjinseob88@gmail.com?part=1