From 71bdb351913e58beef26d4a9151ef782ae99c617 Mon Sep 17 00:00:00 2001 From: Marcin Niestroj Date: Thu, 2 Jul 2020 19:25:01 +0200 Subject: [PATCH] drivers: lora: sx1276: rework gpio configuration Introduce sx1276_configure_pin() helper macro, which hides lots of boilerplate preprocessor and C code. Doing so allows to uncover inconsistent gpio initialization flow and prevent such in future. Also add error log whenever gpio_pin_configure() fails. It seems like output of gpio_pin_configure() for several gpios (antenna_enable, rfi_enable, rfo_enable) was assigned to variable, but its values was never checked. Return value from gpio_pin_configure() of tcxo_power gpio was even not catched. The only output gpio configuration that was handled properly was actually reset gpio. Fix that inconsistent behavior by always checking return value of sx1276_configure_pin(). Signed-off-by: Marcin Niestroj --- drivers/lora/sx1276.c | 136 ++++++++++++++++-------------------------- 1 file changed, 52 insertions(+), 84 deletions(-) diff --git a/drivers/lora/sx1276.c b/drivers/lora/sx1276.c index e4a88bef3f0..23b7ba46727 100644 --- a/drivers/lora/sx1276.c +++ b/drivers/lora/sx1276.c @@ -27,26 +27,14 @@ LOG_MODULE_REGISTER(sx1276); #define GPIO_ANTENNA_ENABLE_PIN \ DT_INST_GPIO_PIN(0, antenna_enable_gpios) -#define GPIO_ANTENNA_ENABLE_FLAGS \ - DT_INST_GPIO_FLAGS(0, antenna_enable_gpios) - #define GPIO_RFI_ENABLE_PIN \ DT_INST_GPIO_PIN(0, rfi_enable_gpios) -#define GPIO_RFI_ENABLE_FLAGS \ - DT_INST_GPIO_FLAGS(0, rfi_enable_gpios) - #define GPIO_RFO_ENABLE_PIN \ DT_INST_GPIO_PIN(0, rfo_enable_gpios) -#define GPIO_RFO_ENABLE_FLAGS \ - DT_INST_GPIO_FLAGS(0, rfo_enable_gpios) - #define GPIO_PA_BOOST_ENABLE_PIN \ DT_INST_GPIO_PIN(0, pa_boost_enable_gpios) -#define GPIO_PA_BOOST_ENABLE_FLAGS \ - DT_INST_GPIO_FLAGS(0, pa_boost_enable_gpios) #define GPIO_TCXO_POWER_PIN DT_INST_GPIO_PIN(0, tcxo_power_gpios) -#define GPIO_TCXO_POWER_FLAGS DT_INST_GPIO_FLAGS(0, tcxo_power_gpios) #if DT_INST_NODE_HAS_PROP(0, tcxo_power_startup_delay_ms) #define TCXO_POWER_STARTUP_DELAY_MS \ @@ -459,67 +447,62 @@ const struct Radio_s Radio = { .SetTxContinuousWave = SX1276SetTxContinuousWave, }; +static inline int __sx1276_configure_pin(struct device **dev, + const char *controller, + gpio_pin_t pin, gpio_flags_t flags) +{ + int err; + + *dev = device_get_binding(controller); + if (!(*dev)) { + LOG_ERR("Cannot get pointer to %s device", controller); + return -EIO; + } + + err = gpio_pin_configure(*dev, pin, flags); + if (err) { + LOG_ERR("Cannot configure gpio %s %d: %d", controller, pin, + err); + return err; + } + + return 0; +} + +#define sx1276_configure_pin(_name, _flags) \ + COND_CODE_1(DT_INST_NODE_HAS_PROP(0, _name##_gpios), \ + (__sx1276_configure_pin(&dev_data._name, \ + DT_INST_GPIO_LABEL(0, _name##_gpios), \ + DT_INST_GPIO_PIN(0, _name##_gpios), \ + DT_INST_GPIO_FLAGS(0, _name##_gpios) | \ + _flags)), \ + (0)) + static int sx1276_antenna_configure(void) { - int ret = 0; + int ret; -#if DT_INST_NODE_HAS_PROP(0, antenna_enable_gpios) - dev_data.antenna_enable = device_get_binding( - DT_INST_GPIO_LABEL(0, antenna_enable_gpios)); - if (!dev_data.antenna_enable) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, antenna_enable_gpios)); - return -EIO; + ret = sx1276_configure_pin(antenna_enable, GPIO_OUTPUT_INACTIVE); + if (ret) { + return ret; } - ret = gpio_pin_configure(dev_data.antenna_enable, - GPIO_ANTENNA_ENABLE_PIN, - GPIO_OUTPUT_INACTIVE | - GPIO_ANTENNA_ENABLE_FLAGS); -#endif - -#if DT_INST_NODE_HAS_PROP(0, rfi_enable_gpios) - dev_data.rfi_enable = device_get_binding( - DT_INST_GPIO_LABEL(0, rfi_enable_gpios)); - if (!dev_data.rfi_enable) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, rfi_enable_gpios)); - return -EIO; + ret = sx1276_configure_pin(rfi_enable, GPIO_OUTPUT_INACTIVE); + if (ret) { + return ret; } - ret = gpio_pin_configure(dev_data.rfi_enable, GPIO_RFI_ENABLE_PIN, - GPIO_OUTPUT_INACTIVE | GPIO_RFI_ENABLE_FLAGS); -#endif - -#if DT_INST_NODE_HAS_PROP(0, rfo_enable_gpios) - dev_data.rfo_enable = device_get_binding( - DT_INST_GPIO_LABEL(0, rfo_enable_gpios)); - if (!dev_data.rfo_enable) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, rfo_enable_gpios)); - return -EIO; + ret = sx1276_configure_pin(rfo_enable, GPIO_OUTPUT_INACTIVE); + if (ret) { + return ret; } - ret = gpio_pin_configure(dev_data.rfo_enable, GPIO_RFO_ENABLE_PIN, - GPIO_OUTPUT_INACTIVE | GPIO_RFO_ENABLE_FLAGS); -#endif - -#if DT_INST_NODE_HAS_PROP(0, pa_boost_enable_gpios) - dev_data.pa_boost_enable = device_get_binding( - DT_INST_GPIO_LABEL(0, pa_boost_enable_gpios)); - if (!dev_data.pa_boost_enable) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, pa_boost_enable_gpios)); - return -EIO; + ret = sx1276_configure_pin(pa_boost_enable, GPIO_OUTPUT_INACTIVE); + if (ret) { + return ret; } - ret = gpio_pin_configure(dev_data.pa_boost_enable, - GPIO_PA_BOOST_ENABLE_PIN, - GPIO_OUTPUT_INACTIVE | - GPIO_PA_BOOST_ENABLE_FLAGS); -#endif - - return ret; + return 0; } static int sx1276_lora_init(struct device *dev) @@ -557,32 +540,17 @@ static int sx1276_lora_init(struct device *dev) dev_data.spi_cfg.cs = &spi_cs; #endif -#if DT_INST_NODE_HAS_PROP(0, tcxo_power_gpios) - dev_data.tcxo_power = device_get_binding( - DT_INST_GPIO_LABEL(0, tcxo_power_gpios)); - if (!dev_data.tcxo_power) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, tcxo_power_gpios)); - return -EIO; + ret = sx1276_configure_pin(tcxo_power, GPIO_OUTPUT_INACTIVE); + if (ret) { + return ret; } - gpio_pin_configure(dev_data.tcxo_power, GPIO_TCXO_POWER_PIN, - GPIO_OUTPUT_INACTIVE | GPIO_TCXO_POWER_FLAGS); -#endif - - /* Setup Reset gpio */ - dev_data.reset = device_get_binding( - DT_INST_GPIO_LABEL(0, reset_gpios)); - if (!dev_data.reset) { - LOG_ERR("Cannot get pointer to %s device", - DT_INST_GPIO_LABEL(0, reset_gpios)); - return -EIO; + /* Setup Reset gpio and perform soft reset */ + ret = sx1276_configure_pin(reset, GPIO_OUTPUT_ACTIVE); + if (ret) { + return ret; } - /* Perform soft reset */ - ret = gpio_pin_configure(dev_data.reset, GPIO_RESET_PIN, - GPIO_OUTPUT_ACTIVE | GPIO_RESET_FLAGS); - k_sleep(K_MSEC(100)); gpio_pin_set(dev_data.reset, GPIO_RESET_PIN, 0); k_sleep(K_MSEC(100));