From a4afcf8c93589bca5752330ba8904f3fe2ee9d2a Mon Sep 17 00:00:00 2001 From: Bjarki Arge Andreasen Date: Mon, 21 Nov 2022 07:16:38 +0100 Subject: [PATCH] drivers/modem/modem_iface_uart: Update API The UART IFACE API currently exposes the context struct modem_iface_uart_data, expecting the user to fill in some of the fields, with no documentation specifying which fields and what they mean. This API update moves all user configurable values in the context out into a config struct, modem_iface_uart_config, within which members are documented. This prevents the user from interacting directly with the context making use of the library safer and more readable. The config structure helps make code readable by using "named args" in a const struct instead of a long, nameless, parameter list passed to the modem_iface_uart_init function. The new API function modem_iface_uart_rx_wait is added to prevent the user from having to interact with the rx sem in the context directly. The context can now be safely interated with without direct access to any of its members. Pointers to the ring buffer params in the context have been moved to config struct, as these are only useful during initialization of the context. Signed-off-by: Bjarki Arge Andreasen --- drivers/modem/gsm_ppp.c | 16 ++++--- drivers/modem/modem_iface_uart.h | 53 ++++++++++++++++------ drivers/modem/modem_iface_uart_async.c | 16 ++++--- drivers/modem/modem_iface_uart_interrupt.c | 16 ++++--- drivers/modem/quectel-bg9x.c | 14 ++++-- drivers/modem/quectel-bg9x.h | 3 +- drivers/modem/simcom-sim7080.c | 13 ++++-- drivers/modem/simcom-sim7080.h | 3 +- drivers/modem/ublox-sara-r4.c | 16 ++++--- drivers/wifi/esp_at/esp.c | 15 +++--- 10 files changed, 107 insertions(+), 58 deletions(-) diff --git a/drivers/modem/gsm_ppp.c b/drivers/modem/gsm_ppp.c index 76f395b5100..c570857293a 100644 --- a/drivers/modem/gsm_ppp.c +++ b/drivers/modem/gsm_ppp.c @@ -174,7 +174,7 @@ static void gsm_rx(struct gsm_modem *gsm) LOG_DBG("starting"); while (true) { - (void)k_sem_take(&gsm->gsm_data.rx_sem, K_FOREVER); + modem_iface_uart_rx_wait(&gsm->context.iface, K_FOREVER); /* The handler will listen AT channel */ gsm->context.cmd_handler.process(&gsm->context.cmd_handler, @@ -1305,13 +1305,15 @@ static int gsm_init(const struct device *dev) #endif /* CONFIG_MODEM_SHELL */ gsm->context.is_automatic_oper = false; - gsm->gsm_data.rx_rb_buf = &gsm->gsm_rx_rb_buf[0]; - gsm->gsm_data.rx_rb_buf_len = sizeof(gsm->gsm_rx_rb_buf); - gsm->gsm_data.hw_flow_control = DT_PROP(GSM_UART_NODE, - hw_flow_control); - ret = modem_iface_uart_init(&gsm->context.iface, &gsm->gsm_data, - DEVICE_DT_GET(GSM_UART_NODE)); + const struct modem_iface_uart_config uart_config = { + .rx_rb_buf = &gsm->gsm_rx_rb_buf[0], + .rx_rb_buf_len = sizeof(gsm->gsm_rx_rb_buf), + .hw_flow_control = DT_PROP(GSM_UART_NODE, hw_flow_control), + .dev = DEVICE_DT_GET(GSM_UART_NODE), + }; + + ret = modem_iface_uart_init(&gsm->context.iface, &gsm->gsm_data, &uart_config); if (ret < 0) { LOG_DBG("iface uart error %d", ret); return ret; diff --git a/drivers/modem/modem_iface_uart.h b/drivers/modem/modem_iface_uart.h index a60be74d2ef..871c0649748 100644 --- a/drivers/modem/modem_iface_uart.h +++ b/drivers/modem/modem_iface_uart.h @@ -23,10 +23,6 @@ struct modem_iface_uart_data { /* HW flow control */ bool hw_flow_control; - /* ring buffer char buffer */ - char *rx_rb_buf; - size_t rx_rb_buf_len; - /* ring buffer */ struct ring_buf rx_rb; @@ -55,17 +51,48 @@ int modem_iface_uart_init_dev(struct modem_iface *iface, const struct device *dev); /** - * @brief Init modem interface for UART + * @brief Modem uart interface configuration * - * @param *iface: modem interface to initialize. - * @param *data: modem interface data to use - * @param *dev_name: name of the UART device to use - * - * @retval 0 if ok, < 0 if error. + * @param rx_rb_buf Buffer used for internal ring buffer + * @param rx_rb_buf_len Size of buffer used for internal ring buffer + * @param dev UART device used for interface + * @param hw_flow_control Set if hardware flow control is used */ -int modem_iface_uart_init(struct modem_iface *iface, - struct modem_iface_uart_data *data, - const struct device *dev); +struct modem_iface_uart_config { + char *rx_rb_buf; + size_t rx_rb_buf_len; + const struct device *dev; + bool hw_flow_control; +}; + +/** + * @brief Initialize modem interface for UART + * + * @param iface Interface structure to initialize + * @param data UART data structure used by the modem interface + * @param config UART configuration structure used to configure UART data structure + * + * @return -EINVAL if any argument is invalid + * @return 0 if successful + */ +int modem_iface_uart_init(struct modem_iface *iface, struct modem_iface_uart_data *data, + const struct modem_iface_uart_config *config); + +/** + * @brief Wait for rx data ready from uart interface + * + * @param iface Interface to wait on + * + * @return 0 if data is ready + * @return -EBUSY if returned without waiting + * @return -EAGAIN if timeout occurred + */ +static inline int modem_iface_uart_rx_wait(struct modem_iface *iface, k_timeout_t timeout) +{ + struct modem_iface_uart_data *data = (struct modem_iface_uart_data *)iface->iface_data; + + return k_sem_take(&data->rx_sem, timeout); +} #ifdef __cplusplus } diff --git a/drivers/modem/modem_iface_uart_async.c b/drivers/modem/modem_iface_uart_async.c index 35d76a24d80..8f9ed228120 100644 --- a/drivers/modem/modem_iface_uart_async.c +++ b/drivers/modem/modem_iface_uart_async.c @@ -148,13 +148,12 @@ int modem_iface_uart_init_dev(struct modem_iface *iface, return rc; } -int modem_iface_uart_init(struct modem_iface *iface, - struct modem_iface_uart_data *data, - const struct device *dev) +int modem_iface_uart_init(struct modem_iface *iface, struct modem_iface_uart_data *data, + const struct modem_iface_uart_config *config) { int ret; - if (!iface || !data) { + if (iface == NULL || data == NULL || config == NULL) { return -EINVAL; } @@ -162,12 +161,15 @@ int modem_iface_uart_init(struct modem_iface *iface, iface->read = modem_iface_uart_async_read; iface->write = modem_iface_uart_async_write; - ring_buf_init(&data->rx_rb, data->rx_rb_buf_len, data->rx_rb_buf); + ring_buf_init(&data->rx_rb, config->rx_rb_buf_len, config->rx_rb_buf); k_sem_init(&data->rx_sem, 0, 1); k_sem_init(&data->tx_sem, 0, 1); - /* get UART device */ - ret = modem_iface_uart_init_dev(iface, dev); + /* Configure hardware flow control */ + data->hw_flow_control = config->hw_flow_control; + + /* Get UART device */ + ret = modem_iface_uart_init_dev(iface, config->dev); if (ret < 0) { iface->iface_data = NULL; iface->read = NULL; diff --git a/drivers/modem/modem_iface_uart_interrupt.c b/drivers/modem/modem_iface_uart_interrupt.c index c837b214767..220dfe685c8 100644 --- a/drivers/modem/modem_iface_uart_interrupt.c +++ b/drivers/modem/modem_iface_uart_interrupt.c @@ -198,13 +198,12 @@ int modem_iface_uart_init_dev(struct modem_iface *iface, return 0; } -int modem_iface_uart_init(struct modem_iface *iface, - struct modem_iface_uart_data *data, - const struct device *dev) +int modem_iface_uart_init(struct modem_iface *iface, struct modem_iface_uart_data *data, + const struct modem_iface_uart_config *config) { int ret; - if (!iface || !data) { + if (iface == NULL || data == NULL || config == NULL) { return -EINVAL; } @@ -212,11 +211,14 @@ int modem_iface_uart_init(struct modem_iface *iface, iface->read = modem_iface_uart_read; iface->write = modem_iface_uart_write; - ring_buf_init(&data->rx_rb, data->rx_rb_buf_len, data->rx_rb_buf); + ring_buf_init(&data->rx_rb, config->rx_rb_buf_len, config->rx_rb_buf); k_sem_init(&data->rx_sem, 0, 1); - /* get UART device */ - ret = modem_iface_uart_init_dev(iface, dev); + /* Configure hardware flow control */ + data->hw_flow_control = config->hw_flow_control; + + /* Get UART device */ + ret = modem_iface_uart_init_dev(iface, config->dev); if (ret < 0) { iface->iface_data = NULL; iface->read = NULL; diff --git a/drivers/modem/quectel-bg9x.c b/drivers/modem/quectel-bg9x.c index 40baa2c21d6..8020478d859 100644 --- a/drivers/modem/quectel-bg9x.c +++ b/drivers/modem/quectel-bg9x.c @@ -848,7 +848,7 @@ static void modem_rx(void) while (true) { /* Wait for incoming data */ - k_sem_take(&mdata.iface_data.rx_sem, K_FOREVER); + modem_iface_uart_rx_wait(&mctx.iface, K_FOREVER); mctx.cmd_handler.process(&mctx.cmd_handler, &mctx.iface); } @@ -1169,10 +1169,14 @@ static int modem_init(const struct device *dev) } /* modem interface */ - mdata.iface_data.rx_rb_buf = &mdata.iface_rb_buf[0]; - mdata.iface_data.rx_rb_buf_len = sizeof(mdata.iface_rb_buf); - ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, - MDM_UART_DEV); + const struct modem_iface_uart_config uart_config = { + .rx_rb_buf = &mdata.iface_rb_buf[0], + .rx_rb_buf_len = sizeof(mdata.iface_rb_buf), + .dev = MDM_UART_DEV, + .hw_flow_control = DT_PROP(MDM_UART_NODE, hw_flow_control), + }; + + ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, &uart_config); if (ret < 0) { goto error; } diff --git a/drivers/modem/quectel-bg9x.h b/drivers/modem/quectel-bg9x.h index 899f1ece887..be5cd69b5e1 100644 --- a/drivers/modem/quectel-bg9x.h +++ b/drivers/modem/quectel-bg9x.h @@ -24,7 +24,8 @@ #include "modem_cmd_handler.h" #include "modem_iface_uart.h" -#define MDM_UART_DEV DEVICE_DT_GET(DT_INST_BUS(0)) +#define MDM_UART_NODE DT_INST_BUS(0) +#define MDM_UART_DEV DEVICE_DT_GET(MDM_UART_NODE) #define MDM_CMD_TIMEOUT K_SECONDS(10) #define MDM_CMD_CONN_TIMEOUT K_SECONDS(120) #define MDM_REGISTRATION_TIMEOUT K_SECONDS(180) diff --git a/drivers/modem/simcom-sim7080.c b/drivers/modem/simcom-sim7080.c index a9256d9a45d..c6d54e2e97f 100644 --- a/drivers/modem/simcom-sim7080.c +++ b/drivers/modem/simcom-sim7080.c @@ -806,7 +806,7 @@ static void modem_rx(void) { while (true) { /* Wait for incoming data */ - k_sem_take(&mdata.iface_data.rx_sem, K_FOREVER); + modem_iface_uart_rx_wait(&mctx.iface, K_FOREVER); mctx.cmd_handler.process(&mctx.cmd_handler, &mctx.iface); } @@ -2364,9 +2364,14 @@ static int modem_init(const struct device *dev) } /* Uart handler. */ - mdata.iface_data.rx_rb_buf = &mdata.iface_rb_buf[0]; - mdata.iface_data.rx_rb_buf_len = sizeof(mdata.iface_rb_buf); - ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, MDM_UART_DEV); + const struct modem_iface_uart_config uart_config = { + .rx_rb_buf = &mdata.iface_rb_buf[0], + .rx_rb_buf_len = sizeof(mdata.iface_rb_buf), + .dev = MDM_UART_DEV, + .hw_flow_control = DT_PROP(MDM_UART_NODE, hw_flow_control), + }; + + ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, &uart_config); if (ret < 0) { goto error; } diff --git a/drivers/modem/simcom-sim7080.h b/drivers/modem/simcom-sim7080.h index 25a8335dcd6..cb16100cb58 100644 --- a/drivers/modem/simcom-sim7080.h +++ b/drivers/modem/simcom-sim7080.h @@ -27,7 +27,8 @@ #include "modem_iface_uart.h" #include "modem_socket.h" -#define MDM_UART_DEV DEVICE_DT_GET(DT_INST_BUS(0)) +#define MDM_UART_NODE DT_INST_BUS(0) +#define MDM_UART_DEV DEVICE_DT_GET(MDM_UART_NODE) #define MDM_MAX_DATA_LENGTH 1024 #define MDM_RECV_BUF_SIZE 1024 #define MDM_MAX_SOCKETS 5 diff --git a/drivers/modem/ublox-sara-r4.c b/drivers/modem/ublox-sara-r4.c index 29587839757..cbcb1e8f55f 100644 --- a/drivers/modem/ublox-sara-r4.c +++ b/drivers/modem/ublox-sara-r4.c @@ -940,7 +940,7 @@ static void modem_rx(void) { while (true) { /* wait for incoming data */ - k_sem_take(&mdata.iface_data.rx_sem, K_FOREVER); + modem_iface_uart_rx_wait(&mctx.iface, K_FOREVER); mctx.cmd_handler.process(&mctx.cmd_handler, &mctx.iface); @@ -2165,12 +2165,14 @@ static int modem_init(const struct device *dev) } /* modem interface */ - mdata.iface_data.hw_flow_control = DT_PROP(MDM_UART_NODE, - hw_flow_control); - mdata.iface_data.rx_rb_buf = &mdata.iface_rb_buf[0]; - mdata.iface_data.rx_rb_buf_len = sizeof(mdata.iface_rb_buf); - ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, - MDM_UART_DEV); + const struct modem_iface_uart_config uart_config = { + .rx_rb_buf = &mdata.iface_rb_buf[0], + .rx_rb_buf_len = sizeof(mdata.iface_rb_buf), + .dev = MDM_UART_DEV, + .hw_flow_control = DT_PROP(MDM_UART_NODE, hw_flow_control), + }; + + ret = modem_iface_uart_init(&mctx.iface, &mdata.iface_data, &uart_config); if (ret < 0) { goto error; } diff --git a/drivers/wifi/esp_at/esp.c b/drivers/wifi/esp_at/esp.c index b1dcd9d6bef..16493ccf793 100644 --- a/drivers/wifi/esp_at/esp.c +++ b/drivers/wifi/esp_at/esp.c @@ -203,7 +203,7 @@ static void esp_rx(struct esp_data *data) { while (true) { /* wait for incoming data */ - k_sem_take(&data->iface_data.rx_sem, K_FOREVER); + modem_iface_uart_rx_wait(&data->mctx.iface, K_FOREVER); data->mctx.cmd_handler.process(&data->mctx.cmd_handler, &data->mctx.iface); @@ -1286,11 +1286,14 @@ static int esp_init(const struct device *dev) } /* modem interface */ - data->iface_data.hw_flow_control = DT_PROP(ESP_BUS, hw_flow_control); - data->iface_data.rx_rb_buf = &data->iface_rb_buf[0]; - data->iface_data.rx_rb_buf_len = sizeof(data->iface_rb_buf); - ret = modem_iface_uart_init(&data->mctx.iface, &data->iface_data, - DEVICE_DT_GET(DT_INST_BUS(0))); + const struct modem_iface_uart_config uart_config = { + .rx_rb_buf = &data->iface_rb_buf[0], + .rx_rb_buf_len = sizeof(data->iface_rb_buf), + .dev = DEVICE_DT_GET(DT_INST_BUS(0)), + .hw_flow_control = DT_PROP(ESP_BUS, hw_flow_control), + }; + + ret = modem_iface_uart_init(&data->mctx.iface, &data->iface_data, &uart_config); if (ret < 0) { goto error; }