[PATCH v1 05/12] input: matrix-keypad: update comments, diagnostics
Gerhard Sittig
gsi at denx.de
Sat Jun 22 04:09:51 EST 2013
- add comments about individual routines' purpose and their interaction,
pre-conditions and consequences
- mark a few spots which may need some more attention or clarification
- rephrase some diagnostics messages
Signed-off-by: Gerhard Sittig <gsi at denx.de>
---
drivers/input/keyboard/matrix_keypad.c | 81 +++++++++++++++++++++++++++++---
drivers/input/matrix-keymap.c | 4 +-
include/linux/input/matrix_keypad.h | 17 ++++---
3 files changed, 89 insertions(+), 13 deletions(-)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 4f22149..85e16a2 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -43,6 +43,10 @@ struct matrix_keypad {
};
/*
+ * this routine controls a physical column pin which the keypad matrix
+ * is connected to, and takes care of the pin's polarity as well as its
+ * mode of operation (fully driven push/pull, or emulated open drain)
+ *
* former comment before introduction of optional push/pull behaviour:
* <cite>
* NOTE: normally the GPIO has to be put into HiZ when de-activated to cause
@@ -77,6 +81,14 @@ static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata,
}
}
+/*
+ * this routine addresses logical columns of the keypad matrix, and
+ * makes sure that the "scan delay" is applied upon activation of the
+ * column (the delay between activating the column and reading rows)
+ *
+ * the caller ensures that this routine need not de-activate other
+ * columns when dealing with the column specified for the invocation
+ */
static void activate_col(const struct matrix_keypad_platform_data *pdata,
int col, bool on)
{
@@ -86,6 +98,12 @@ static void activate_col(const struct matrix_keypad_platform_data *pdata,
udelay(pdata->col_scan_delay_us);
}
+/*
+ * this routine either de-activates all columns before scanning the
+ * matrix, or re-activates all columns at the same time after the scan
+ * is complete, to make changes in the key press state trigger the
+ * condition to re-scan the matrix
+ */
static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
bool on)
{
@@ -95,6 +113,10 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
__activate_col_pin(pdata, col, on);
}
+/*
+ * this routine checks a single row while a specific column is active,
+ * it takes care of the pin's polarity, the pin level had time to settle
+ */
static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
int row)
{
@@ -103,6 +125,12 @@ static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
pdata->row_gpios_active_low;
}
+/*
+ * this routine enables IRQs after a keypad matrix scan has completed,
+ * to have any subsequent change in the key press status trigger the ISR
+ *
+ * a single IRQ line can be used if all involved GPIOs share that IRQ
+ */
static void enable_row_irqs(struct matrix_keypad *keypad)
{
const struct matrix_keypad_platform_data *pdata = keypad->pdata;
@@ -116,6 +144,13 @@ static void enable_row_irqs(struct matrix_keypad *keypad)
}
}
+/*
+ * this routine disables IRQs for changes in the key press status, which
+ * applies to shutdown or suspend modes, to periods where the keypad
+ * matrix is not used (not opened by any application), as well as to the
+ * interval between the first detected change and scanning the complete
+ * matrix (the debounce interval)
+ */
static void disable_row_irqs(struct matrix_keypad *keypad)
{
const struct matrix_keypad_platform_data *pdata = keypad->pdata;
@@ -130,7 +165,9 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
}
/*
- * This gets the keys from keyboard and reports it to input subsystem
+ * this routine scans the keypad matrix and detects changes in the keys'
+ * status against a previously sampled status, from which events for the
+ * input subsystem get derived
*/
static void matrix_keypad_scan(struct work_struct *work)
{
@@ -142,12 +179,12 @@ static void matrix_keypad_scan(struct work_struct *work)
uint32_t new_state[MATRIX_MAX_COLS];
int row, col, code;
- /* de-activate all columns for scanning */
+ /* de-activate all columns before scanning the matrix */
activate_all_cols(pdata, false);
memset(new_state, 0, sizeof(new_state));
- /* assert each column and read the row status out */
+ /* assert each column in turn and read back the row status */
for (col = 0; col < pdata->num_col_gpios; col++) {
activate_col(pdata, col, true);
@@ -159,6 +196,7 @@ static void matrix_keypad_scan(struct work_struct *work)
activate_col(pdata, col, false);
}
+ /* detect changes and derive input events, update the snapshot */
for (col = 0; col < pdata->num_col_gpios; col++) {
uint32_t bits_changed;
@@ -171,6 +209,15 @@ static void matrix_keypad_scan(struct work_struct *work)
continue;
code = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+ /*
+ * TODO: the key code matrix may be sparse,
+ * ignore items in gaps or report changes in all
+ * positions? this consideration may especially
+ * apply where the key code matrix gets setup or
+ * manipulated from user space, or where the key
+ * code matrix gets switched (shift or function
+ * keys, alternate keyboard modes)
+ */
input_event(input_dev, EV_MSC, MSC_SCAN, code);
input_report_key(input_dev,
keycodes[code],
@@ -178,9 +225,13 @@ static void matrix_keypad_scan(struct work_struct *work)
}
}
input_sync(input_dev);
-
memcpy(keypad->last_key_state, new_state, sizeof(new_state));
+ /*
+ * re-enable all columns after the scan has completed, to have
+ * changes in their key press status issue interrupts and
+ * trigger another complete scan of the matrix
+ */
activate_all_cols(pdata, true);
/* Enable IRQs again */
@@ -190,6 +241,14 @@ static void matrix_keypad_scan(struct work_struct *work)
spin_unlock_irq(&keypad->lock);
}
+/*
+ * interrupt service routine, invoked upon the first detected change in
+ * the key press status, initiating a debounce delay, and suppressing
+ * subsequent interrupts until scanning all of the matrix has completed
+ *
+ * copes with interrupts which arrive during the debounce interval or
+ * the actual matrix scan or a shutdown or suspend sequence
+ */
static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
{
struct matrix_keypad *keypad = id;
@@ -310,6 +369,10 @@ static int matrix_keypad_resume(struct device *dev)
if (device_may_wakeup(&pdev->dev))
matrix_keypad_disable_wakeup(keypad);
+ /*
+ * TODO: consider whether to only (re-)start the keypad matrix
+ * driver when it was opened by applications?
+ */
matrix_keypad_start(keypad->input_dev);
return 0;
@@ -425,7 +488,7 @@ matrix_keypad_parse_dt(struct device *dev)
int i, nrow, ncol;
if (!np) {
- dev_err(dev, "device lacks DT data\n");
+ dev_err(dev, "device lacks DT data for the keypad config\n");
return ERR_PTR(-ENODEV);
}
@@ -435,6 +498,7 @@ matrix_keypad_parse_dt(struct device *dev)
return ERR_PTR(-ENOMEM);
}
+ /* get the pin count for rows and columns */
pdata->num_row_gpios = nrow = of_gpio_named_count(np, "row-gpios");
pdata->num_col_gpios = ncol = of_gpio_named_count(np, "col-gpios");
if (nrow <= 0 || ncol <= 0) {
@@ -442,6 +506,7 @@ matrix_keypad_parse_dt(struct device *dev)
return ERR_PTR(-EINVAL);
}
+ /* get input, power, and GPIO pin properties */
if (of_get_property(np, "linux,no-autorepeat", NULL))
pdata->no_autorepeat = true;
if (of_get_property(np, "linux,wakeup", NULL))
@@ -457,10 +522,12 @@ matrix_keypad_parse_dt(struct device *dev)
if (of_get_property(np, "col-gpios-pushpull", NULL))
pdata->col_gpios_push_pull = true;
+ /* get delay and interval timing specs */
of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);
of_property_read_u32(np, "col-scan-delay-us",
&pdata->col_scan_delay_us);
+ /* get the individual row and column GPIO pins */
gpios = devm_kzalloc(dev,
sizeof(unsigned int) *
(pdata->num_row_gpios + pdata->num_col_gpios),
@@ -486,7 +553,7 @@ matrix_keypad_parse_dt(struct device *dev)
static inline struct matrix_keypad_platform_data *
matrix_keypad_parse_dt(struct device *dev)
{
- dev_err(dev, "no platform data defined\n");
+ dev_err(dev, "device lacks DT support for the keypad config\n");
return ERR_PTR(-EINVAL);
}
@@ -521,6 +588,8 @@ static int matrix_keypad_probe(struct platform_device *pdev)
keypad->input_dev = input_dev;
keypad->pdata = pdata;
keypad->row_shift = get_count_order(pdata->num_col_gpios);
+
+ /* start in stopped state, open(2) will activate the scan */
keypad->stopped = true;
INIT_DELAYED_WORK(&keypad->work_scan_matrix, matrix_keypad_scan);
spin_lock_init(&keypad->lock);
diff --git a/drivers/input/matrix-keymap.c b/drivers/input/matrix-keymap.c
index b7091f2..c427bf9 100644
--- a/drivers/input/matrix-keymap.c
+++ b/drivers/input/matrix-keymap.c
@@ -103,7 +103,9 @@ static int matrix_keypad_parse_of_keymap(const char *propname,
size = proplen / sizeof(u32);
if (size > max_keys) {
- dev_err(dev, "OF: %s size overflow\n", propname);
+ dev_err(dev,
+ "OF: %s size overflow, keymap size %u, max keys %u\n",
+ propname, size, max_keys);
return -EINVAL;
}
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 5496a00..4bbe6b3 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -39,9 +39,11 @@ struct matrix_keymap_data {
* @col_gpios: pointer to array of gpio numbers reporesenting colums
* @num_row_gpios: actual number of row gpios used by device
* @num_col_gpios: actual number of col gpios used by device
- * @col_scan_delay_us: delay, measured in microseconds, that is
- * needed before we can keypad after activating column gpio
- * @debounce_ms: debounce interval in milliseconds
+ * @col_scan_delay_us: delay in microseconds, the interval between
+ * activating a column and reading back row information
+ * @debounce_ms: debounce interval in milliseconds, the interval between
+ * detecting a change in the key press status and determining the new
+ * overall keypad matrix status
* @clustered_irq: may be specified if interrupts of all row/column GPIOs
* are bundled to one single irq
* @clustered_irq_flags: flags that are needed for the clustered irq
@@ -53,26 +55,29 @@ struct matrix_keymap_data {
* source
* @no_autorepeat: disable key autorepeat
*
- * This structure represents platform-specific data that use used by
+ * This structure represents platform-specific data that is used by
* matrix_keypad driver to perform proper initialization.
*/
struct matrix_keypad_platform_data {
+ /* map keys to codes */
const struct matrix_keymap_data *keymap_data;
+ /* the physical GPIO pin connections */
const unsigned int *row_gpios;
const unsigned int *col_gpios;
-
unsigned int num_row_gpios;
unsigned int num_col_gpios;
+ /* delays and intervals specs */
unsigned int col_scan_delay_us;
- /* key debounce interval in milli-second */
unsigned int debounce_ms;
+ /* optionally reduce interrupt mgmt overhead */
unsigned int clustered_irq;
unsigned int clustered_irq_flags;
+ /* pin and input system properties */
bool row_gpios_active_low;
bool col_gpios_active_low;
bool col_gpios_push_pull;
--
1.7.10.4
More information about the devicetree-discuss
mailing list