-
Notifications
You must be signed in to change notification settings - Fork 7.6k
drivers: gpio: add interrupt support for the CH32V family #89139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,10 @@ | |
clocks = <&pll>; | ||
}; | ||
|
||
&exti { | ||
status = "okay"; | ||
}; | ||
|
||
&gpioc { | ||
status = "okay"; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,10 @@ | |
clocks = <&pll>; | ||
}; | ||
|
||
&exti { | ||
status = "okay"; | ||
}; | ||
|
||
&gpioc { | ||
status = "okay"; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,10 @@ | |
clocks = <&pll>; | ||
}; | ||
|
||
&exti { | ||
status = "okay"; | ||
}; | ||
|
||
&gpioa { | ||
status = "okay"; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
#include <zephyr/drivers/clock_control.h> | ||
#include <zephyr/drivers/gpio.h> | ||
#include <zephyr/drivers/gpio/gpio_utils.h> | ||
#include <zephyr/drivers/interrupt_controller/wch_exti.h> | ||
#include <zephyr/dt-bindings/gpio/gpio.h> | ||
#include <zephyr/irq.h> | ||
|
||
|
@@ -23,6 +24,7 @@ struct gpio_ch32v00x_config { | |
|
||
struct gpio_ch32v00x_data { | ||
struct gpio_driver_data common; | ||
sys_slist_t callbacks; | ||
}; | ||
|
||
static int gpio_ch32v00x_configure(const struct device *dev, gpio_pin_t pin, gpio_flags_t flags) | ||
|
@@ -111,13 +113,142 @@ static int gpio_ch32v00x_port_toggle_bits(const struct device *dev, uint32_t pin | |
return 0; | ||
} | ||
|
||
#if defined(CONFIG_GPIO_WCH_GPIO_INTERRUPTS) | ||
|
||
static void gpio_ch32v00x_isr(uint8_t line, void *user) | ||
{ | ||
const struct device *dev = user; | ||
struct gpio_ch32v00x_data *data = dev->data; | ||
|
||
gpio_fire_callbacks(&data->callbacks, dev, BIT(line)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why assigning the data here to local variable (on stack) instead of direct using in gpio_fire_callbacks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistent style. Zephyr drivers typically have an initial variable block where |
||
} | ||
|
||
static int gpio_ch32v00x_configure_exti(const struct device *dev, gpio_pin_t pin) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could it be a naming without v00x, because also used in 20x and 30x? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is existing code, so any rename should be done in a separate PR. I don't disagree though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this lines are new added, so could it be done? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, as that would make the file inconsistent, and one of the first rules of maintainable code is match the current style, no matter how bad :) |
||
{ | ||
const struct gpio_ch32v00x_config *config = dev->config; | ||
AFIO_TypeDef *afio = (AFIO_TypeDef *)DT_REG_ADDR(DT_NODELABEL(pinctrl)); | ||
uint8_t port_id; | ||
uint8_t cr_id; | ||
uint8_t bit0; | ||
|
||
/* Convert the device into a port ID by checking the address */ | ||
switch ((uintptr_t)config->regs) { | ||
case DT_REG_ADDR(DT_NODELABEL(gpioa)): | ||
port_id = 0; | ||
break; | ||
#if DT_NODE_EXISTS(DT_NODELABEL(gpiob)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this also be done on the other banks? I would never think about, that parts of device tree comes so in driver. It could help in future and the defines doesn't take memory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this switch is based on the hardware capabilities, not what is enabled (i.e. status = "okay") in Devicetree. All CH32V devices have GPIOA, but at least the CH32V003 does not have GPIOB. |
||
case DT_REG_ADDR(DT_NODELABEL(gpiob)): | ||
port_id = 1; | ||
break; | ||
#endif | ||
case DT_REG_ADDR(DT_NODELABEL(gpioc)): | ||
port_id = 2; | ||
break; | ||
case DT_REG_ADDR(DT_NODELABEL(gpiod)): | ||
port_id = 3; | ||
break; | ||
#if DT_NODE_EXISTS(DT_NODELABEL(gpioe)) | ||
case DT_REG_ADDR(DT_NODELABEL(gpioe)): | ||
port_id = 4; | ||
break; | ||
#endif | ||
default: | ||
nzmichaelh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return -EINVAL; | ||
} | ||
|
||
#if defined(AFIO_EXTICR_EXTI0) | ||
/* CH32V003 style with one register with 2 bits per map. */ | ||
BUILD_ASSERT(AFIO_EXTICR_EXTI0 == 0x03); | ||
|
||
(void)cr_id; | ||
bit0 = pin << 1; | ||
afio->EXTICR = (afio->EXTICR & ~(AFIO_EXTICR_EXTI0 << bit0)) | (port_id << bit0); | ||
#elif defined(AFIO_EXTICR1_EXTI0) | ||
/* | ||
* CH32V20x style with multiple registers with 4 pins per register and 4 bits per | ||
* map. | ||
*/ | ||
BUILD_ASSERT(AFIO_EXTICR1_EXTI0 == 0x0F); | ||
BUILD_ASSERT(ARRAY_SIZE(afio->EXTICR) == 4); | ||
|
||
cr_id = pin / 4; | ||
bit0 = (pin % 4) * 4; | ||
afio->EXTICR[cr_id] = | ||
(afio->EXTICR[cr_id] & ~(AFIO_EXTICR1_EXTI0 << bit0)) | (port_id << bit0); | ||
#else | ||
#error Unrecognised EXTICR format | ||
#endif | ||
|
||
return 0; | ||
} | ||
|
||
static int gpio_ch32v00x_pin_interrupt_configure(const struct device *dev, gpio_pin_t pin, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s.h: could it be a naming without v00x, because also used in 20x and 30x? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reply as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reply as above. |
||
enum gpio_int_mode mode, | ||
enum gpio_int_trig trigger) | ||
{ | ||
int err; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you assign here a 0? I like to have always clear declaration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the intent, but that's not Zephyr style and, with a modern compiler, lets through more bugs. For example, if you initialise err to zero, and you intended to write:
but instead you wrote:
then the compiler will let this logic bug through. If you have the use of uninitialized variable warning on, then the compiler would warn. |
||
|
||
switch (mode) { | ||
case GPIO_INT_MODE_DISABLED: | ||
wch_exti_disable(pin); | ||
err = wch_exti_configure(pin, NULL, NULL); | ||
break; | ||
case GPIO_INT_MODE_EDGE: | ||
err = wch_exti_configure(pin, gpio_ch32v00x_isr, (void *)dev); | ||
if (err != 0) { | ||
break; | ||
} | ||
|
||
err = gpio_ch32v00x_configure_exti(dev, pin); | ||
if (err != 0) { | ||
break; | ||
} | ||
|
||
switch (trigger) { | ||
case GPIO_INT_TRIG_LOW: | ||
wch_exti_set_trigger(pin, WCH_EXTI_TRIGGER_FALLING_EDGE); | ||
break; | ||
case GPIO_INT_TRIG_HIGH: | ||
wch_exti_set_trigger(pin, WCH_EXTI_TRIGGER_RISING_EDGE); | ||
break; | ||
case GPIO_INT_TRIG_BOTH: | ||
wch_exti_set_trigger(pin, WCH_EXTI_TRIGGER_FALLING_EDGE | | ||
WCH_EXTI_TRIGGER_RISING_EDGE); | ||
break; | ||
default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
return -ENOTSUP; | ||
} | ||
|
||
wch_exti_enable(pin); | ||
break; | ||
default: | ||
return -ENOTSUP; | ||
} | ||
|
||
return err; | ||
} | ||
|
||
static int gpio_ch32v00x_manage_callback(const struct device *dev, struct gpio_callback *callback, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s.h: could it be a naming without v00x, because also used in 20x and 30x? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
bool set) | ||
{ | ||
struct gpio_ch32v00x_data *data = dev->data; | ||
|
||
return gpio_manage_callback(&data->callbacks, callback, set); | ||
} | ||
|
||
#endif /* CONFIG_GPIO_WCH_GPIO_INTERRUPTS */ | ||
|
||
static DEVICE_API(gpio, gpio_ch32v00x_driver_api) = { | ||
.pin_configure = gpio_ch32v00x_configure, | ||
.port_get_raw = gpio_ch32v00x_port_get_raw, | ||
.port_set_masked_raw = gpio_ch32v00x_port_set_masked_raw, | ||
.port_set_bits_raw = gpio_ch32v00x_port_set_bits_raw, | ||
.port_clear_bits_raw = gpio_ch32v00x_port_clear_bits_raw, | ||
.port_toggle_bits = gpio_ch32v00x_port_toggle_bits, | ||
#if defined(CONFIG_GPIO_WCH_GPIO_INTERRUPTS) | ||
.pin_interrupt_configure = gpio_ch32v00x_pin_interrupt_configure, | ||
.manage_callback = gpio_ch32v00x_manage_callback, | ||
#endif | ||
}; | ||
|
||
static int gpio_ch32v00x_init(const struct device *dev) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Copyright (c) 2025 Michael Hope <michaelh@juju.nz> | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
config WCH_EXTI | ||
bool "WCH CH32V00x/20x/30x External Interrupt and Event Controller (EXTI)" | ||
default y | ||
depends on DT_HAS_WCH_EXTI_ENABLED | ||
help | ||
Enable the WCH CH32V00x/20x/30x External Interrupt and Event Controller (EXTI) | ||
driver. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,135 @@ | ||||||||||||||||||||||
/* | ||||||||||||||||||||||
* Copyright (c) 2025 Michael Hope <michaelh@juju.nz> | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
|
||||||||||||||||||||||
#define DT_DRV_COMPAT wch_exti | ||||||||||||||||||||||
|
||||||||||||||||||||||
#include <errno.h> | ||||||||||||||||||||||
|
||||||||||||||||||||||
#include <zephyr/device.h> | ||||||||||||||||||||||
#include <zephyr/irq.h> | ||||||||||||||||||||||
#include <zephyr/sys/util_macro.h> | ||||||||||||||||||||||
#include <zephyr/drivers/interrupt_controller/wch_exti.h> | ||||||||||||||||||||||
|
||||||||||||||||||||||
#include <hal_ch32fun.h> | ||||||||||||||||||||||
|
||||||||||||||||||||||
#define WCH_EXTI_NUM_LINES DT_PROP(DT_NODELABEL(exti), num_lines) | ||||||||||||||||||||||
|
||||||||||||||||||||||
/* Per EXTI callback registration */ | ||||||||||||||||||||||
struct wch_exti_registration { | ||||||||||||||||||||||
wch_exti_callback_handler_t callback; | ||||||||||||||||||||||
void *user; | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
struct wch_exti_data { | ||||||||||||||||||||||
struct wch_exti_registration callbacks[WCH_EXTI_NUM_LINES]; | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
#define WCH_EXTI_INIT_RANGE(node_id, interrupts, idx) \ | ||||||||||||||||||||||
DT_PROP_BY_IDX(node_id, line_ranges, UTIL_X2(idx)), | ||||||||||||||||||||||
|
||||||||||||||||||||||
/* | ||||||||||||||||||||||
* List of [start, end) line ranges for each line group, where the range for group n is | ||||||||||||||||||||||
* `[wch_exti_ranges[n-1]...wch_exti_ranges[n])`. This uses the fact that the ranges are contiguous, | ||||||||||||||||||||||
* so the end of group n is the same as the start of group n+1. | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
static const uint8_t wch_exti_ranges[] = { | ||||||||||||||||||||||
DT_FOREACH_PROP_ELEM(DT_NODELABEL(exti), interrupt_names, WCH_EXTI_INIT_RANGE) | ||||||||||||||||||||||
WCH_EXTI_NUM_LINES, | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
#define WCH_EXTI_INIT_INTERRUPT(node_id, interrupts, idx) DT_IRQ_BY_IDX(node_id, idx, irq), | ||||||||||||||||||||||
|
||||||||||||||||||||||
/* Interrupt number for each line group. Used when enabling the interrupt. */ | ||||||||||||||||||||||
static const uint8_t wch_exti_interrupts[] = { | ||||||||||||||||||||||
DT_FOREACH_PROP_ELEM(DT_NODELABEL(exti), interrupt_names, WCH_EXTI_INIT_INTERRUPT)}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
BUILD_ASSERT(ARRAY_SIZE(wch_exti_interrupts) + 1 == ARRAY_SIZE(wch_exti_ranges)); | ||||||||||||||||||||||
|
||||||||||||||||||||||
static void wch_exti_isr(const void *user) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
const struct device *const dev = DEVICE_DT_INST_GET(0); | ||||||||||||||||||||||
struct wch_exti_data *data = dev->data; | ||||||||||||||||||||||
EXTI_TypeDef *regs = (EXTI_TypeDef *)DT_INST_REG_ADDR(0); | ||||||||||||||||||||||
const uint8_t *range = user; | ||||||||||||||||||||||
uint32_t intfr = regs->INTFR; | ||||||||||||||||||||||
|
||||||||||||||||||||||
for (uint8_t line = range[0]; line < range[1]; line++) { | ||||||||||||||||||||||
if ((intfr & BIT(line)) != 0) { | ||||||||||||||||||||||
const struct wch_exti_registration *callback = &data->callbacks[line]; | ||||||||||||||||||||||
/* Clear the interrupt */ | ||||||||||||||||||||||
regs->INTFR = BIT(line); | ||||||||||||||||||||||
if (callback->callback != NULL) { | ||||||||||||||||||||||
callback->callback(line, callback->user); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
void wch_exti_enable(uint8_t line) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
EXTI_TypeDef *regs = (EXTI_TypeDef *)DT_INST_REG_ADDR(0); | ||||||||||||||||||||||
|
||||||||||||||||||||||
regs->INTENR |= BIT(line); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where I can find error checking of parameter line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no real Zephyr policy on validating arguments. |
||||||||||||||||||||||
/* Find the corresponding interrupt and enable it */ | ||||||||||||||||||||||
for (uint8_t i = 1; i < ARRAY_SIZE(wch_exti_ranges); i++) { | ||||||||||||||||||||||
if (line < wch_exti_ranges[i]) { | ||||||||||||||||||||||
irq_enable(wch_exti_interrupts[i - 1]); | ||||||||||||||||||||||
break; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
void wch_exti_disable(uint8_t line) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
EXTI_TypeDef *regs = (EXTI_TypeDef *)DT_INST_REG_ADDR(0); | ||||||||||||||||||||||
|
||||||||||||||||||||||
regs->INTENR &= ~BIT(line); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
int wch_exti_configure(uint8_t line, wch_exti_callback_handler_t callback, void *user) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
const struct device *const dev = DEVICE_DT_INST_GET(0); | ||||||||||||||||||||||
struct wch_exti_data *data = dev->data; | ||||||||||||||||||||||
struct wch_exti_registration *registration = &data->callbacks[line]; | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (registration->callback == callback && registration->user == user) { | ||||||||||||||||||||||
return 0; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (callback != NULL && registration->callback != NULL) { | ||||||||||||||||||||||
return -EALREADY; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
+102
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs a check similar to the STM32 EXTI one, to just return 0 if a caller attempts to configure with the exact same callback and user:
Suggested change
Without this, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||||||||||||||
|
||||||||||||||||||||||
registration->callback = callback; | ||||||||||||||||||||||
registration->user = user; | ||||||||||||||||||||||
|
||||||||||||||||||||||
return 0; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
void wch_exti_set_trigger(uint8_t line, enum wch_exti_trigger trigger) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
EXTI_TypeDef *regs = (EXTI_TypeDef *)DT_INST_REG_ADDR(0); | ||||||||||||||||||||||
|
||||||||||||||||||||||
WRITE_BIT(regs->RTENR, line, (trigger & WCH_EXTI_TRIGGER_RISING_EDGE) != 0); | ||||||||||||||||||||||
WRITE_BIT(regs->FTENR, line, (trigger & WCH_EXTI_TRIGGER_FALLING_EDGE) != 0); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above. This already supports both edges. |
||||||||||||||||||||||
|
||||||||||||||||||||||
#define WCH_EXTI_CONNECT_IRQ(node_id, interrupts, idx) \ | ||||||||||||||||||||||
IRQ_CONNECT(DT_IRQ_BY_IDX(node_id, idx, irq), DT_IRQ_BY_IDX(node_id, idx, priority), \ | ||||||||||||||||||||||
wch_exti_isr, &wch_exti_ranges[idx], 0); | ||||||||||||||||||||||
|
||||||||||||||||||||||
static int wch_exti_init(const struct device *dev) | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
/* Generate the registrations for each interrupt */ | ||||||||||||||||||||||
DT_FOREACH_PROP_ELEM(DT_NODELABEL(exti), interrupt_names, WCH_EXTI_CONNECT_IRQ); | ||||||||||||||||||||||
|
||||||||||||||||||||||
return 0; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
static struct wch_exti_data wch_exti_data_0; | ||||||||||||||||||||||
|
||||||||||||||||||||||
DEVICE_DT_INST_DEFINE(0, wch_exti_init, NULL, &wch_exti_data_0, NULL, PRE_KERNEL_2, | ||||||||||||||||||||||
CONFIG_INTC_INIT_PRIORITY, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could / should this include also guided by a config to reduce memory consumption when exti not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yip, it already does that. There's two levels: the exti driver is only compiled in if the corresponding Devicetree entry is enabled, and then the GPIO driver only uses it if the corresponding Kconfig option is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the code in c-files are only compiled by device-tree entry. but in header are some variables which takes some memory. pls check it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't see any variables in zephyr/drivers/interrupt_controller/wch_exti.h. There is a typedef, enum, and prototypes. Tell me more?