tests: Change the result compare method in pin_get_config()

[hunterhu3000]

trafficstars

Fixes #47921 Add a mask to compare the common bits only

Signed-off-by: Hu Zhenyu [email protected]

[hunterhu3000]

This PR is to fix https://github.com/zephyrproject-rtos/zephyr/issues/47921

[FRASTM]

This fixes the pb as reported by issues https://github.com/zephyrproject-rtos/zephyr/issues/47921 and https://github.com/zephyrproject-rtos/zephyr/issues/48007

In any case the test should also PASS when CONFIG_GPIO_GET_CONFIG=n suggestion : flag pin_get_config() with #ifdef CONFIG_GPIO_GET_CONFIG

[MaureenHelm]

This PR is to fix #47921

Please add "Fixes #47921" to the PR description. This will link the Issue and close it automatically when the PR gets merged. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

[FRASTM]

When the function to get the gpio pin config was introduced by the PR #43225, flags was supposed to give the position of the gpio : in/out, GPIO_OPEN_DRAIN, GPIO_PULL_DOWN/GPIO_PULL_UP I guess this information should fit the GPIO input/output configuration flags If the purpose is to know about high or low states only of the gpio, what is the benefit of gpio_stm32_get_config() function when reading the

	otype = LL_GPIO_GetPinOutputType(gpio, pin_ll);
	pupd = LL_GPIO_GetPinPull(gpio, pin_ll);
	mode = LL_GPIO_GetPinMode(gpio, pin_ll);

[mmahadevan108]

cc @semihalf-barnas-michal

[barnas-michal]

If the purpose is to know about high or low states only of the gpio, what is the benefit of gpio_stm32_get_config() function

Exactly, the gpio_get_config should return all (supported) flags set for the gpio, not only input or output. So this PR hides the problem with the stm32 driver instead of fixing it. There is probably some problem in the implementation of stm32 driver, some flags are missing or are reading invalid enums for specific stm boards.

In any case the test should also PASS when CONFIG_GPIO_GET_CONFIG=n suggestion : flag pin_get_config() with #ifdef CONFIG_GPIO_GET_CONFIG

Or just wrap the whole test with the ifdef. It may be better to not have this test run, when it's not implemented, instead of returning the positive.

[hunterhu3000]

This fixes the pb as reported by issues #47921 and #48007

In any case the test should also PASS when CONFIG_GPIO_GET_CONFIG=n suggestion : flag pin_get_config() with #ifdef CONFIG_GPIO_GET_CONFIG OK

[hunterhu3000]

gpio_stm32_get_config

I think the user can use pin_get_config to get more infomation than just the high/low input/output. But some of the infomation is read only and may not be used in gpio_pin_configure. So in the test, it is enough to check the high/low input/output.

[hunterhu3000]

If the purpose is to know about high or low states only of the gpio, what is the benefit of gpio_stm32_get_config() function

Exactly, the gpio_get_config should return all (supported) flags set for the gpio, not only input or output. So this PR hides the problem with the stm32 driver instead of fixing it. There is probably some problem in the implementation of stm32 driver, some flags are missing or are reading invalid enums for specific stm boards.

In any case the test should also PASS when CONFIG_GPIO_GET_CONFIG=n suggestion : flag pin_get_config() with #ifdef CONFIG_GPIO_GET_CONFIG

Or just wrap the whole test with the ifdef. It may be better to not have this test run, when it's not implemented, instead of returning the positive.

I think #48007 is another issue. This PR will not fix it, as the set value of #48007 is 0xa0000 but the get value is 0x20000b74. 0xa0000 & 0x20000b74 is 0, so the assertion in this PR cannot pass. For #48007, we need to check gpio_stm32_get_config(), it seems does not follow the zephyr definition

[barnas-michal]

But some of the infomation is read only and may not be used in gpio_pin_configure

Sure, but if some implementation will be (more...) broken and returns the 0xffffffff, then this test will also pass, since you are checking if expected flag is set, instead of checking also if the invalid flags are not set.

The problem with ITE is that it returns voltage information, which was not set specifically, but read from register. I feel like it would be better to hide the voltage bits, instead of all of them. However, it may be worth to discuss if there is some better more flexible solution.

[barnas-michal]

I think #48007 is another issue

Yes, there seems to be a bug in the driver implementation. Both issues are connected to my PR, but definitely different root cause.

[hunterhu3000]

But some of the infomation is read only and may not be used in gpio_pin_configure

Sure, but if some implementation will be (more...) broken and returns the 0xffffffff, then this test will also pass, since you are checking if expected flag is set, instead of checking also if the invalid flags are not set.

The problem with ITE is that it returns voltage information, which was not set specifically, but read from register. I feel like it would be better to hide the voltage bits, instead of all of them. However, it may be worth to discuss if there is some better more flexible solution.

You are right. I will modify my patch to avoid this case

[barnas-michal]

I will modify my patch to avoid this case

Thanks

[barnas-michal]

That looks great

[mmahadevan108]

This PR is to fix #47921

Please add "Fixes #47921" to the PR description. This will link the Issue and close it automatically when the PR gets merged. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@hunterhu3000 as suggested earlier please add Fixes #47921 to the description. This will then automatically close the issue when this PR gets merged.

[hunterhu3000]

@henrikbrixandersen Could you please review my latest patch? If if is OK, thanks to +1.

[FRASTM]

BTW if you could please include the change in the tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c to handle the correct error code from the gpio_pin_get_config return

rc = gpio_pin_get_config(dev, PIN_OUT, &flags_get);
	if (rc == -ENOSYS) {
		return TC_PASS;
	}

[hunterhu3000]

BTW if you could please include the change in the tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c to handle the correct error code from the gpio_pin_get_config return

rc = gpio_pin_get_config(dev, PIN_OUT, &flags_get);
	if (rc == -ENOSYS) {
		return TC_PASS;
	}

I have another question, why ENOSYS is regarded as PASS？

[FRASTM]

BTW if you could please include the change in the tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c to handle the correct error code from the gpio_pin_get_config return

rc = gpio_pin_get_config(dev, PIN_OUT, &flags_get);
	if (rc == -ENOSYS) {
		return TC_PASS;
	}

I have another question, why ENOSYS is regarded as PASS？

The reason for this is in the gpio_pin_get_config The gpio_pin_get_config() zephyr/include/zephyr/drivers/gpio.h return code is not ENOTSUP

	if (api->pin_get_config == NULL)
		return -ENOSYS;

[hunterhu3000]

If getting current pin configuration is not implemented by the driver, then we should not run this test. Returning fail will remind us there might be a problem or not running the test next time if it is as expected, while returning pass will cover the problem. @FRASTM

I think we can just remove the following lines

	if (rc == -ENOTSUP) {
		return TC_PASS;
	}

[hunterhu3000]

Rebased to the latest. Thanks to review

[hunterhu3000]

As there is a long history for this PR, I'd like to summarize it below:

1. henrikbrixandersen/MaureenHelm/mmahadevan108 asked to change the commit log -> done
2. FRASTM think this PR can fix #48007, but they are two different problems. #48077 is fixed by #48159 later.
3. semihalf-barnas-michal pointed out that the patch cannot cover the corner case such as 0xFFFFFFFF -> fixed
4. semihalf-barnas-michal to add "#ifdef CONFIG_GPIO_GET_CONFIG" -> done
5. henrikbrixandersen asked to use GENMASK instead of OR -> done
6. cfriedt asked to use "if (IS_ENABLED(CONFIG_..))" instead of "#ifdef CONFIG_GPIO_GET_CONFIG" -> done
7. FRASTM asked to chaged the ENOTSUP to ENOSYS -> cfriedt disagree this. So nothing is done on this PR. The modification was done in #48159

[hunterhu3000]

@mnkp @FRASTM @MaureenHelm @henrikbrixandersen @cfriedt @semihalf-barnas-michal Could you please help to review and +1? Thanks.

[hunterhu3000]

There are two ways to define the bits to make the patch work 1)

	/* GPIO_INT_MASK		bit 26-21
	 * GPIO_OUTPUT_INIT_HIGH, LOW	bit 19, 18
	 * GPIO_OUTPUT, GPIO_INPUT	bit 17, 16
	 * GPIO_PULL_DOWN, GPIO_PULL_UP	bit 5, 4
	 * GPIO_LINE_OPEN_DRAIN/SOURCE	bit 2
	 * GPIO_SINGLE_ENDED/PUSH_PULL	bit 1
	 * GPIO_ACTIVE_LOW/HIGH		bit 0
	 */
	gpio_flags_t flags_mask = GENMASK(26, 16) | GENMASK(5, 4) | GENMASK(2, 0);

	gpio_flags_t flags_mask = GPIO_INT_MASK | GPIO_OUTPUT_INIT_LOGICAL | 
				  GPIO_OUTPUT_INIT_HIGH | GPIO_OUTPUT_INIT_LOW |
				  GPIO_OUTPUT | GPIO_INPUT |
				  GPIO_PULL_UP | GPIO_PULL_DOWN |
				  GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN | GPIO_ACTIVE_LOW;

For 1, we can optimize the magic numbers to defines, for example GENMASK(26, 16) can be written as GENMASK(GPIO_INT_HIGH_1_SHIFT, GPIO_INPUT_SHIFT). To my opinion, it is still not friend to the reader. Although there is no magic number in the code, the reader still do not know which bits are masked. And he also need to check the defines in the header files. So I prefer method 2, although it is a little concise

[hunterhu3000]

@henrikbrixandersen Could you please review and +1 so that the PR can be merged to the mainline earlier, thanks.