From 6c7ab0ce53ec75394eaa84ae303d725300cb1f45 Mon Sep 17 00:00:00 2001 From: Pete Johanson Date: Thu, 28 Jan 2021 11:55:02 -0500 Subject: [PATCH] refactor(kscan): Fix polling of GPIO matrices. * Add easier macros for conditional polling/interrupt code. * Properly continue polling on intervals, without extra enable/disable code for pins that is superfluous when not trying to deal with interupts firing. * Fix to allow multiple GPIO drivers when doing splits w/ IO expanders --- app/drivers/kscan/kscan_gpio_matrix.c | 144 ++++++++++++++------------ 1 file changed, 78 insertions(+), 66 deletions(-) diff --git a/app/drivers/kscan/kscan_gpio_matrix.c b/app/drivers/kscan/kscan_gpio_matrix.c index 6697cf69..9af3171a 100644 --- a/app/drivers/kscan/kscan_gpio_matrix.c +++ b/app/drivers/kscan/kscan_gpio_matrix.c @@ -51,6 +51,11 @@ static int kscan_gpio_config_interrupts(const struct device **devices, } #endif +#define COND_POLLING(code) COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (code), ()) +#define COND_INTERRUPTS(code) COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (), (code)) +#define COND_POLL_OR_INTERRUPTS(pollcode, intcode) \ + COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, pollcode, intcode) + #define INST_MATRIX_ROWS(n) DT_INST_PROP_LEN(n, row_gpios) #define INST_MATRIX_COLS(n) DT_INST_PROP_LEN(n, col_gpios) #define INST_OUTPUT_LEN(n) \ @@ -61,19 +66,21 @@ static int kscan_gpio_config_interrupts(const struct device **devices, (INST_MATRIX_ROWS(n))) #define GPIO_INST_INIT(n) \ - struct kscan_gpio_irq_callback_##n { \ - struct COND_CODE_0(DT_INST_PROP(n, debounce_period), (k_work), (k_delayed_work)) * work; \ - struct gpio_callback callback; \ - const struct device *dev; \ - }; \ - static struct kscan_gpio_irq_callback_##n irq_callbacks_##n[INST_INPUT_LEN(n)]; \ + COND_INTERRUPTS( \ + struct kscan_gpio_irq_callback_##n { \ + struct COND_CODE_0(DT_INST_PROP(n, debounce_period), (k_work), (k_delayed_work)) * \ + work; \ + struct gpio_callback callback; \ + const struct device *dev; \ + }; \ + static struct kscan_gpio_irq_callback_##n irq_callbacks_##n[INST_INPUT_LEN(n)];) \ struct kscan_gpio_config_##n { \ struct kscan_gpio_item_config rows[INST_MATRIX_ROWS(n)]; \ struct kscan_gpio_item_config cols[INST_MATRIX_COLS(n)]; \ }; \ struct kscan_gpio_data_##n { \ kscan_callback_t callback; \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (struct k_timer poll_timer;), ()) \ + COND_POLLING(struct k_timer poll_timer;) \ struct COND_CODE_0(DT_INST_PROP(n, debounce_period), (k_work), (k_delayed_work)) work; \ bool matrix_state[INST_MATRIX_ROWS(n)][INST_MATRIX_COLS(n)]; \ const struct device *rows[INST_MATRIX_ROWS(n)]; \ @@ -102,17 +109,16 @@ static int kscan_gpio_config_interrupts(const struct device **devices, return ( \ COND_CODE_0(DT_ENUM_IDX(DT_DRV_INST(n), diode_direction), (cfg->rows), (cfg->cols))); \ } \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (), \ - ( \ - static int kscan_gpio_enable_interrupts_##n(const struct device *dev) { \ - return kscan_gpio_config_interrupts( \ - kscan_gpio_input_devices_##n(dev), kscan_gpio_input_configs_##n(dev), \ - INST_INPUT_LEN(n), GPIO_INT_LEVEL_ACTIVE); \ - } static int kscan_gpio_disable_interrupts_##n(const struct device *dev) { \ - return kscan_gpio_config_interrupts(kscan_gpio_input_devices_##n(dev), \ - kscan_gpio_input_configs_##n(dev), \ - INST_INPUT_LEN(n), GPIO_INT_DISABLE); \ - })) \ + COND_INTERRUPTS( \ + static int kscan_gpio_enable_interrupts_##n(const struct device *dev) { \ + return kscan_gpio_config_interrupts(kscan_gpio_input_devices_##n(dev), \ + kscan_gpio_input_configs_##n(dev), \ + INST_INPUT_LEN(n), GPIO_INT_LEVEL_ACTIVE); \ + } static int kscan_gpio_disable_interrupts_##n(const struct device *dev) { \ + return kscan_gpio_config_interrupts(kscan_gpio_input_devices_##n(dev), \ + kscan_gpio_input_configs_##n(dev), \ + INST_INPUT_LEN(n), GPIO_INT_DISABLE); \ + }) \ static void kscan_gpio_set_output_state_##n(const struct device *dev, int value) { \ int err; \ for (int i = 0; i < INST_OUTPUT_LEN(n); i++) { \ @@ -132,17 +138,22 @@ static int kscan_gpio_config_interrupts(const struct device **devices, (output_index))] = value; \ } \ static int kscan_gpio_read_##n(const struct device *dev) { \ - bool submit_follow_up_read = false; \ + COND_INTERRUPTS(bool submit_follow_up_read = false;) \ struct kscan_gpio_data_##n *data = dev->data; \ static bool read_state[INST_MATRIX_ROWS(n)][INST_MATRIX_COLS(n)]; \ + int err; \ /* Disable our interrupts temporarily while we scan, to avoid */ \ /* re-entry while we iterate columns and set them active one by one */ \ /* to get pressed state for each matrix cell. */ \ - kscan_gpio_set_output_state_##n(dev, 0); \ + COND_INTERRUPTS(kscan_gpio_set_output_state_##n(dev, 0);) \ for (int o = 0; o < INST_OUTPUT_LEN(n); o++) { \ const struct device *out_dev = kscan_gpio_output_devices_##n(dev)[o]; \ const struct kscan_gpio_item_config *out_cfg = &kscan_gpio_output_configs_##n(dev)[o]; \ - gpio_pin_set(out_dev, out_cfg->pin, 1); \ + err = gpio_pin_set(out_dev, out_cfg->pin, 1); \ + if (err) { \ + LOG_ERR("Failed to set output active (err %d)", err); \ + return err; \ + } \ for (int i = 0; i < INST_INPUT_LEN(n); i++) { \ const struct device *in_dev = kscan_gpio_input_devices_##n(dev)[i]; \ const struct kscan_gpio_item_config *in_cfg = \ @@ -150,16 +161,20 @@ static int kscan_gpio_config_interrupts(const struct device **devices, kscan_gpio_set_matrix_state_##n(read_state, i, o, \ gpio_pin_get(in_dev, in_cfg->pin) > 0); \ } \ - gpio_pin_set(out_dev, out_cfg->pin, 0); \ + err = gpio_pin_set(out_dev, out_cfg->pin, 0); \ + if (err) { \ + LOG_ERR("Failed to set output inactive (err %d)", err); \ + return err; \ + } \ } \ /* Set all our outputs as active again. */ \ - kscan_gpio_set_output_state_##n(dev, 1); \ + COND_INTERRUPTS(kscan_gpio_set_output_state_##n(dev, 1);) \ for (int r = 0; r < INST_MATRIX_ROWS(n); r++) { \ for (int c = 0; c < INST_MATRIX_COLS(n); c++) { \ bool pressed = read_state[r][c]; \ /* Follow up reads needed because further interrupts won't fire on already tripped \ * input GPIO pins */ \ - submit_follow_up_read = (submit_follow_up_read || pressed); \ + COND_INTERRUPTS(submit_follow_up_read = (submit_follow_up_read || pressed);) \ if (pressed != data->matrix_state[r][c]) { \ LOG_DBG("Sending event at %d,%d state %s", r, c, (pressed ? "on" : "off")); \ data->matrix_state[r][c] = pressed; \ @@ -167,33 +182,31 @@ static int kscan_gpio_config_interrupts(const struct device **devices, } \ } \ } \ - if (submit_follow_up_read) { \ - COND_CODE_0(DT_INST_PROP(n, debounce_period), ({ k_work_submit(&data->work); }), ({ \ - k_delayed_work_cancel(&data->work); \ - k_delayed_work_submit(&data->work, K_MSEC(5)); \ - })) \ - } else { \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (), \ - (kscan_gpio_enable_interrupts_##n(dev);)) \ - } \ + COND_INTERRUPTS( \ + if (submit_follow_up_read) { \ + COND_CODE_0(DT_INST_PROP(n, debounce_period), ({ k_work_submit(&data->work); }), \ + ({ \ + k_delayed_work_cancel(&data->work); \ + k_delayed_work_submit(&data->work, K_MSEC(5)); \ + })) \ + } else { kscan_gpio_enable_interrupts_##n(dev); }) \ return 0; \ } \ static void kscan_gpio_work_handler_##n(struct k_work *work) { \ struct kscan_gpio_data_##n *data = CONTAINER_OF(work, struct kscan_gpio_data_##n, work); \ kscan_gpio_read_##n(data->dev); \ } \ - static void kscan_gpio_irq_callback_handler_##n( \ + COND_INTERRUPTS(static void kscan_gpio_irq_callback_handler_##n( \ const struct device *dev, struct gpio_callback *cb, gpio_port_pins_t pin) { \ struct kscan_gpio_irq_callback_##n *data = \ CONTAINER_OF(cb, struct kscan_gpio_irq_callback_##n, callback); \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, (), \ - (kscan_gpio_disable_interrupts_##n(data->dev);)) \ + kscan_gpio_disable_interrupts_##n(data->dev); \ COND_CODE_0(DT_INST_PROP(n, debounce_period), ({ k_work_submit(data->work); }), ({ \ k_delayed_work_cancel(data->work); \ k_delayed_work_submit(data->work, \ K_MSEC(DT_INST_PROP(n, debounce_period))); \ })) \ - } \ + }) \ \ static struct kscan_gpio_data_##n kscan_gpio_data_##n = { \ .rows = {[INST_MATRIX_ROWS(n) - 1] = NULL}, .cols = {[INST_MATRIX_COLS(n) - 1] = NULL}}; \ @@ -207,25 +220,22 @@ static int kscan_gpio_config_interrupts(const struct device **devices, return 0; \ }; \ static int kscan_gpio_enable_##n(const struct device *dev) { \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, \ - (struct kscan_gpio_data_##n *data = dev->data; \ - k_timer_start(&data->poll_timer, K_MSEC(10), K_MSEC(10)); return 0;), \ - (int err = kscan_gpio_enable_interrupts_##n(dev); \ - if (err) { return err; } return kscan_gpio_read_##n(dev);)) \ + COND_POLL_OR_INTERRUPTS((struct kscan_gpio_data_##n *data = dev->data; \ + k_timer_start(&data->poll_timer, K_MSEC(10), K_MSEC(10)); \ + return 0;), \ + (int err = kscan_gpio_enable_interrupts_##n(dev); \ + if (err) { return err; } return kscan_gpio_read_##n(dev);)) \ }; \ static int kscan_gpio_disable_##n(const struct device *dev) { \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, \ - (struct kscan_gpio_data_##n *data = dev->data; \ - k_timer_stop(&data->poll_timer); return 0;), \ - (return kscan_gpio_disable_interrupts_##n(dev);)) \ + COND_POLL_OR_INTERRUPTS((struct kscan_gpio_data_##n *data = dev->data; \ + k_timer_stop(&data->poll_timer); return 0;), \ + (return kscan_gpio_disable_interrupts_##n(dev);)) \ }; \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, \ - (static void kscan_gpio_timer_handler(struct k_timer *timer) { \ - struct kscan_gpio_data_##n *data = \ - CONTAINER_OF(timer, struct kscan_gpio_data_##n, poll_timer); \ - k_work_submit(&data->work.work); \ - }), \ - ()) \ + COND_POLLING(static void kscan_gpio_timer_handler_##n(struct k_timer *timer) { \ + struct kscan_gpio_data_##n *data = \ + CONTAINER_OF(timer, struct kscan_gpio_data_##n, poll_timer); \ + k_work_submit(&data->work.work); \ + }) \ static int kscan_gpio_init_##n(const struct device *dev) { \ struct kscan_gpio_data_##n *data = dev->data; \ int err; \ @@ -244,15 +254,15 @@ static int kscan_gpio_config_interrupts(const struct device **devices, } else { \ LOG_DBG("Configured pin %d on %s for input", in_cfg->pin, in_cfg->label); \ } \ - irq_callbacks_##n[i].work = &data->work; \ - irq_callbacks_##n[i].dev = dev; \ - gpio_init_callback(&irq_callbacks_##n[i].callback, \ - kscan_gpio_irq_callback_handler_##n, BIT(in_cfg->pin)); \ - err = gpio_add_callback(input_devices[i], &irq_callbacks_##n[i].callback); \ - if (err) { \ - LOG_ERR("Error adding the callback to the column device"); \ - return err; \ - } \ + COND_INTERRUPTS( \ + irq_callbacks_##n[i].work = &data->work; irq_callbacks_##n[i].dev = dev; \ + gpio_init_callback(&irq_callbacks_##n[i].callback, \ + kscan_gpio_irq_callback_handler_##n, BIT(in_cfg->pin)); \ + err = gpio_add_callback(input_devices[i], &irq_callbacks_##n[i].callback); \ + if (err) { \ + LOG_ERR("Error adding the callback to the input device"); \ + return err; \ + }) \ } \ const struct device **output_devices = kscan_gpio_output_devices_##n(dev); \ for (int o = 0; o < INST_OUTPUT_LEN(n); o++) { \ @@ -262,8 +272,8 @@ static int kscan_gpio_config_interrupts(const struct device **devices, LOG_ERR("Unable to find output GPIO device"); \ return -EINVAL; \ } \ - err = gpio_pin_configure(output_devices[o], out_cfg->pin, \ - GPIO_OUTPUT_ACTIVE | out_cfg->flags); \ + err = \ + gpio_pin_configure(output_devices[o], out_cfg->pin, GPIO_OUTPUT | out_cfg->flags); \ if (err) { \ LOG_ERR("Unable to configure pin %d on %s for output", out_cfg->pin, \ out_cfg->label); \ @@ -271,10 +281,12 @@ static int kscan_gpio_config_interrupts(const struct device **devices, } \ } \ data->dev = dev; \ - COND_CODE_1(CONFIG_ZMK_KSCAN_MATRIX_POLLING, \ - (k_timer_init(&data->poll_timer, kscan_gpio_timer_handler, NULL);), ()) \ (COND_CODE_0(DT_INST_PROP(n, debounce_period), (k_work_init), (k_delayed_work_init)))( \ &data->work, kscan_gpio_work_handler_##n); \ + COND_POLL_OR_INTERRUPTS( \ + (k_timer_init(&data->poll_timer, kscan_gpio_timer_handler_##n, NULL); \ + kscan_gpio_set_output_state_##n(dev, 0);), \ + (kscan_gpio_set_output_state_##n(dev, 1);)) \ return 0; \ } \ static const struct kscan_driver_api gpio_driver_api_##n = { \