drivers: gpio: gd32: add dependency on the EXTI #50879

soburi · 2022-10-02T11:19:59Z

The GPIO pin interruption depends on the EXTI.
Add "select" to clarify the dependency.

Signed-off-by: TOKITA Hiroshi tokita.hiroshi@gmail.com

cfriedt · 2022-10-02T21:52:25Z

drivers/gpio/gpio_gd32.c

It looks as though clkid will be padded out to 32-bits if CONFIG_GD32_EXTI=n, so the #ifdef can be removed here. Then you can switch to using if (IS_ENABLED(CONFIG_GD32_EXTI) elsewhere and remove the #ifdef around static void gpio_gd32_isr(uint8_t line, void *arg)

It looks as though clkid will be padded out to 32-bits if CONFIG_GD32_EXTI=n, so the #ifdef can be removed here.

Insightful!
Removed the #ifdef.

Then you can switch to using if (IS_ENABLED(CONFIG_GD32_EXTI) elsewhere and remove the #ifdef around static void gpio_gd32_isr(uint8_t line, void *arg)

Commenting in follows topics.

cfriedt · 2022-10-02T21:52:45Z

drivers/gpio/gpio_gd32.c

if (IS_ENABLED(CONFIG_GD32_EXTI))

cfriedt · 2022-10-02T21:53:05Z

drivers/gpio/gpio_gd32.c

if (IS_ENABLED(CONFIG_GD32_EXTI))

cfriedt · 2022-10-02T21:53:38Z

drivers/gpio/gpio_gd32.c

#ifdef can be removed

This #ifdef suppressing unused-function warning.
It should be better not to remove it.

cfriedt · 2022-10-02T21:58:24Z

drivers/gpio/Kconfig.gd32

Should this be enabled by default?

Yes, it is.
Interrupts handling needs the EXTI.
The callback function of the GPIO API will not work If this option is disabled.

This PR is intended mainly to clarify dependencies.
I would like to make as few changes as possible from the current behavior.

If it's required, should be depends on. imply means that it is optional but should be the default. Alternatively, selects could be used..

I added more change gpio_pin_interrupt_configure API returns -ENOTSUP if GD32_EXTI disabled.

It make allow to disable GD32_EXTI without API contradiction.

https://github.com/zephyrproject-rtos/zephyr/pull/50879/files#diff-606da3fe7a304f5544526be564097f77dc7db8e627c004fc62b8eed50e1b45dbR329-R331

henrikbrixandersen

LGTM

nandojve · 2022-10-04T15:31:22Z

drivers/gpio/gpio_gd32.c

I'm not sure but should it be as below?

-#if (IS_ENABLED(CONFIG_GD32_EXTI)) +if (IS_ENABLED(CONFIG_GD32_EXTI)) { ... -#endif +}

It's a beautiful solution!

The if-else condition must determine at compile time.
I worry that the intention maybe not have been conveyed to the code reader, adding some comments.

gmarull

So before reviewing, why do we need this change? Why would a new GD platform need that?

gmarull

-1 if this is needed for #49886

soburi · 2022-10-06T13:44:42Z

@gmarull ,

-1 if this is needed for #49886

I found this problem in work about #49886, but This PR is not related to #49886 and is also not required by.

It is a problem that no dependency defines in Kconfig even though gd32_gpio.c depends on gd32_exti.c.

The simple way to solve this is to use select GD32_EXTI as @cfriedt says.
But making it able to compile without GD32_EXTI is the preferable way, I think, for the following reasons.

It does not violate API spec. (gpio_pin_interrupt_configure can return -ENOTSUP.)
It makes providing more options to users. (User can disable EXTI if no use GPIO callbacks.)
The default behavior has not changed. (imply make EXTI enable default.)

I agree there is no solid reason to merge this patch.
Because it is useful only in rare cases.
But I think it is good to fix it from the viewpoint of the integrity of dependency.

nandojve · 2022-10-24T16:15:19Z

Hi @gmarull ,

Do you still with same position or @soburi justification is OK?
We need decide if we will keep it or not. I'm fine with justification since it is not about #49886.

gmarull · 2022-10-25T07:53:06Z

drivers/gpio/gpio_gd32.c

IMO, what should be done is the following:

Add a Kconfig option to build the GD32 driver w/o interrupt support, e.g. CONFIG_GPIO_GD32_INT_ENABLE

Such option should be enabled by default, and select GD32_EXTI (intc drivers do not have a class, so we need to select on a driver basis)

Just ifdefout code if CONFIG_GPIO_GD32_INT_ENABLE=n. I've personally never liked IS_ENABLED because it can't be universally used without getting compile errors in some cases.

But still, I feel it is wrong to add such option at a driver level. It should likely be a subsystem setting to build GPIO drivers with/without interrupt support.

It is as you say. I look at the implementation of other GPIO drivers, and none of them enable or disable .pin_interrupt_configure in the settings. So I would like to withdraw this idea.
However, I think that it is better to clarify the dependency on EXTI,
I have changed the fix (and PR title).
Could you please confirm this?

The GPIO pin interruption depends on the EXTI. Add "select" to clarify the dependency. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

cameled · 2022-11-02T03:07:54Z

drivers/gpio/Kconfig.gd32

 	bool "GD32 GPIO driver"
 	default y
 	depends on DT_HAS_GD_GD32_GPIO_ENABLED
+	select GD32_EXTI


Sorry, I confuse the NVIX and EXTI definition. NVIC provide SysTick, PendSV and other fault exception. EXTI provide periphral interrupt.

That's ok for me now.

cameled · 2022-11-07T13:09:00Z

Hi @nandojve, @gmarull, please help to confirm the new changes.

Stale

soburi requested a review from mnkp as a code owner October 2, 2022 11:20

zephyrbot added platform: GD32 GigaDevice area: GPIO labels Oct 2, 2022

zephyrbot requested review from cameled, cfriedt, gmarull, henrikbrixandersen and nandojve October 2, 2022 11:20

zephyrbot assigned nandojve Oct 2, 2022

cfriedt reviewed Oct 2, 2022

View reviewed changes

soburi force-pushed the gd32_gpio_wo_exti branch from 7d7ad35 to 37a9f70 Compare October 2, 2022 23:06

soburi requested a review from cfriedt October 2, 2022 23:35

soburi force-pushed the gd32_gpio_wo_exti branch 2 times, most recently from fa22798 to 581b7cd Compare October 4, 2022 05:56

henrikbrixandersen previously approved these changes Oct 4, 2022

View reviewed changes

nandojve previously requested changes Oct 4, 2022

View reviewed changes

gmarull reviewed Oct 4, 2022

View reviewed changes

gmarull requested changes Oct 4, 2022

View reviewed changes

soburi dismissed henrikbrixandersen’s stale review via 6e4af77 October 6, 2022 15:10

soburi force-pushed the gd32_gpio_wo_exti branch 2 times, most recently from 6e4af77 to 64c8ab8 Compare October 6, 2022 21:11

nandojve requested a review from gmarull October 24, 2022 16:15

gmarull reviewed Oct 25, 2022

View reviewed changes

soburi force-pushed the gd32_gpio_wo_exti branch from 64c8ab8 to 0fcd49a Compare November 1, 2022 03:14

soburi changed the title ~~drivers: gpio: gd32: Make possible to compile without EXTI~~ drivers: gpio: gd32: add dependency on the EXTI Nov 1, 2022

soburi requested review from gmarull and nandojve November 1, 2022 03:17

drivers: gpio: gd32: add dependency on the EXTI

f5da65a

The GPIO pin interruption depends on the EXTI. Add "select" to clarify the dependency. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>

soburi force-pushed the gd32_gpio_wo_exti branch from 0fcd49a to f5da65a Compare November 1, 2022 04:59

cameled reviewed Nov 2, 2022

View reviewed changes

cameled approved these changes Nov 7, 2022

View reviewed changes

gmarull approved these changes Nov 7, 2022

View reviewed changes

henrikbrixandersen approved these changes Nov 8, 2022

View reviewed changes

cfriedt merged commit c5c0a1a into zephyrproject-rtos:main Nov 9, 2022

soburi deleted the gd32_gpio_wo_exti branch November 17, 2022 21:44

soburi mentioned this pull request Feb 10, 2023

doc: release: 3.3: add GigaDevice GD32 changes #54644

Merged

drivers: gpio: gd32: add dependency on the EXTI #50879

drivers: gpio: gd32: add dependency on the EXTI #50879

Uh oh!

Conversation

soburi commented Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfriedt Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soburi Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soburi Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

soburi commented Oct 6, 2022

Uh oh!

nandojve commented Oct 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameled Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameled commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

soburi commented Oct 2, 2022 •

edited

Loading

cfriedt Oct 2, 2022 •

edited

Loading

soburi Oct 2, 2022 •

edited

Loading

cfriedt Oct 3, 2022 •

edited

Loading

soburi Oct 3, 2022 •

edited

Loading

cameled Nov 2, 2022 •

edited

Loading