[PATCH] Generic, platform independent matrix keyboard support

[PATCH] Generic, platform independent matrix keyboard support

[Pavel Pisa]

The genmatrix_kbd module provides support for matrix keyboard
where switches interconnects return lines with port driven
scan lines. Actual version code allow to register concrete
hardware described by platform device. Hardware can be described
by list of output and input pins and manipulation can be delegated
to GPIO layer or hardware specific setup(), release(), activate_all()
deactivate_all() and scan_single() functions can be defined.

Code is written as much independent on platform device, so providing
support for keyboard matrix driven by ports on PCI or AMBA bus
should require add only thin piece of code.

Key activation can be noticed by interrupt or if it is not available,
simple poll mode can be used. Additional logic provides noise
filtering and glitches filtering.

Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
---

Our board specific part registering device for generic
matrix keyboard driver can be found there

 http://rtime.felk.cvut.cz/repos/ppisa-linux-devel/kernel-patches/current/pimx1-board-kbd.patch

It has been tested in IRQ and non-IRQ driven mode with i.MX
optimized functions and in generic platform independent GPIO
support. Unfortunately generic GPIO does not provide ability
to control internal pullups for input, so there has been one
line modification necessary in gpiomatrix_setup().

The discussion about this patch would be more interresting
for embedded targets people (for example on ARM list), but
due to not-crosspost rule I am sending it to input list
only for now. I want to know, if something like this driver
is acceptable for mainline in some timeframe.

Best wishes

            Pavel Pisa

 drivers/input/keyboard/Kconfig         |   12 
 drivers/input/keyboard/Makefile        |    2 
 drivers/input/keyboard/genmatrix_kbd.c |  693 +++++++++++++++++++++++++++++++++
 include/linux/genmatrix_kbd.h          |   34 +
 4 files changed, 740 insertions(+), 1 deletion(-)

Index: linux-2.6.23-git/drivers/input/keyboard/Kconfig
===================================================================
--- linux-2.6.23-git.orig/drivers/input/keyboard/Kconfig
+++ linux-2.6.23-git/drivers/input/keyboard/Kconfig
@@ -253,4 +253,16 @@ config KEYBOARD_GPIO
 	  To compile this driver as a module, choose M here: the
 	  module will be called gpio-keys.
 
+config KEYBOARD_GENMATRIX
+	tristate "Generic matrix keyboard"
+	default n
+	help
+	  Say Y here to enable the generic scanned matrix keyboard
+	  on your platform. This is platform driver, which
+	  can be used by any platform device providing calls
+	  for actual key scanning.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called genmatrix_kbd.
+
 endif
Index: linux-2.6.23-git/drivers/input/keyboard/Makefile
===================================================================
--- linux-2.6.23-git.orig/drivers/input/keyboard/Makefile
+++ linux-2.6.23-git/drivers/input/keyboard/Makefile
@@ -21,4 +21,4 @@ obj-$(CONFIG_KEYBOARD_OMAP)		+= omap-key
 obj-$(CONFIG_KEYBOARD_PXA27x)		+= pxa27x_keyboard.o
 obj-$(CONFIG_KEYBOARD_AAED2000)		+= aaed2000_kbd.o
 obj-$(CONFIG_KEYBOARD_GPIO)		+= gpio_keys.o
-
+obj-$(CONFIG_KEYBOARD_GENMATRIX)	+= genmatrix_kbd.o
Index: linux-2.6.23-git/drivers/input/keyboard/genmatrix_kbd.c
===================================================================
--- /dev/null
+++ linux-2.6.23-git/drivers/input/keyboard/genmatrix_kbd.c
@@ -0,0 +1,693 @@
+/*
+ *  Generic matrix keyboard driver for embedded platforms
+ *
+ *  Copyright (c) 2007 Pavel Pisa
+ *
+ *  Based on PiKRON's COLAMI keyboard code and corgikbd.c
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/bitops.h>
+
+#include <linux/genmatrix_kbd.h>
+
+/* Some sane defaults */
+#define KEY_PUSH_T      20
+#define KEY_RELEASE_T   10
+#define KEY_PUSHCHECK_T  20
+
+/* Individual key states */
+#define KEY_STATE_IDLE     0
+#define KEY_STATE_PUSH     1
+#define KEY_STATE_RELEASE  2
+#define KEY_STATE_PUSHED   4
+#define KEY_STATE_NOISE    8
+#define KEY_STATE_BUSY     (KEY_STATE_PUSH|KEY_STATE_RELEASE)
+
+/* Bit flags */
+#define GENMATRIX_FLG_WITH_IRQ_b 0
+#define GENMATRIX_FLG_IRQ_EN_b   1
+
+typedef unsigned genmatrix_retmask_t;
+
+struct genmatrix_kbd {
+	struct input_dev   *input;
+	struct device      *hw_dev;
+	char   *phys;
+	unsigned long      flags;
+
+	/* Platform specific information */
+	unsigned           scan_cnt;
+	unsigned           ret_cnt;
+
+	int  (*setup)(struct device *dev);
+	void (*release)(struct device *dev);
+	unsigned (*activate_all)(struct device *dev, int setup_irq);
+	void (*deactivate_all)(struct device *dev, int disable_irq);
+	unsigned (*scan_single)(struct device *dev, unsigned scannr);
+
+	/* State of keyboard matrix and key press reporting */
+	genmatrix_retmask_t *down_arr;	/* array of size scan_cnt */
+	genmatrix_retmask_t *chng_arr;	/* array of size scan_cnt */
+	unsigned char      key_hit;
+	int                key_last_changed;
+
+	/* Internal state for repeat processing */
+	int                key_state;
+	unsigned long      check_time;
+
+	unsigned long      push_time;
+	unsigned long      release_time;
+	unsigned long      pushcheck_time;
+
+	spinlock_t         lock;
+	struct timer_list  timer;
+
+	/* Scancode to key translation */
+	unsigned           keycode_cnt;
+	unsigned char      *keycode;
+
+	int                suspended;
+};
+
+/**
+ * genmatrix_scan - Scan keyboard matrix and report requests for state change
+ *
+ * Scans keyboard matrix connected row by row by calling function
+ * mx1_kbd_onerow(). Number of scanned output lines is defined
+ * by %KBD_SCAN_CNT. Checks read keyboard state against @down_arr
+ * and updates @key_change_arr array. The @down_arr state is
+ * left unchanged. It is changed later by kbd_down() function.
+ * Returns 0, if no keyboard activity is found. Returns 1
+ * if at least one key is pressed. Returns 2 or 3 in case
+ * of detected change.
+ */
+static int genmatrix_scan(struct genmatrix_kbd *kbd)
+{
+	int i, ret=0;
+	genmatrix_retmask_t val, chng;
+	for(i=0; i<kbd->scan_cnt; i++) {
+		val = kbd->scan_single(kbd->hw_dev, i);
+		chng = val ^ kbd->down_arr[i];
+		kbd->chng_arr[i] = chng;
+		if (val)
+			ret |= 1;
+		if (chng)
+			ret |= 2;
+	}
+	return ret;
+}
+
+/**
+ * genmatrix_down - Detects changed key scancode and applies changes to matrix state
+ *
+ * Functions check @chng_arr and process changes.
+ * It updates its internal state @key_state, does
+ * noise cancellation and repeat timing, then updates
+ * @down_arr, stores detected scancode to @key_last_changed
+ * and calls modifiers processing kbd_scan2mod().
+ * Return value is zero if no change is detected.
+ * In other case evaluated scancode is returned.
+ * Variable @key_hit signals by value 1 pressed key, by value
+ * 2 key release.
+ */
+static int genmatrix_down(struct genmatrix_kbd *kbd)
+{
+	int si, ri=0;
+	unsigned char val;
+	unsigned long act_time = jiffies;
+
+        if (!(kbd->key_state & KEY_STATE_BUSY)){
+		for(si=0; si < kbd->scan_cnt; si++) {
+			if (!(val = kbd->chng_arr[si])) continue;
+			ri = fls(val) - 1;
+			kbd->key_last_changed = si * kbd->ret_cnt + ri;
+			if (kbd->down_arr[si] & (1 << ri)) {
+				kbd->check_time = act_time + kbd->push_time;
+				kbd->key_state = KEY_STATE_RELEASE;
+			}else{
+				kbd->check_time = act_time + kbd->release_time;
+				kbd->key_state = KEY_STATE_PUSH;
+			}
+			break;
+		}
+		if (kbd->key_state == KEY_STATE_IDLE)
+			return 0;
+	} else {
+		if (kbd->key_last_changed < 0){
+			kbd->key_state = KEY_STATE_IDLE;
+	  		return 0;
+		}
+		si = (kbd->key_last_changed) / kbd->ret_cnt;
+		ri = (kbd->key_last_changed) % kbd->ret_cnt;
+		if (!(kbd->chng_arr[si] & (1 << ri))){
+			/* Noise detected */
+			if (!(kbd->key_state & KEY_STATE_NOISE)){
+			        kbd->check_time = act_time + kbd->release_time;
+			        kbd->key_state |= KEY_STATE_NOISE;
+			}
+		}
+	}
+
+	if (!time_after(jiffies, kbd->check_time))
+		return 0;
+
+	if (kbd->key_state == KEY_STATE_PUSH) {
+		kbd->down_arr[si] |= 1 << ri;
+		kbd->key_state = KEY_STATE_PUSHED;
+		kbd->check_time = act_time + kbd->pushcheck_time;
+		kbd->key_hit = 1;
+		input_report_key(kbd->input, kbd->keycode[kbd->key_last_changed], 1);
+		return 1;
+	} else if (kbd->key_state == KEY_STATE_PUSHED) {
+		kbd->check_time = act_time + kbd->pushcheck_time;
+		return 0;
+	} else if (kbd->key_state == KEY_STATE_RELEASE) {
+		kbd->down_arr[si] &= ~(1<<ri);
+	        kbd->key_state = KEY_STATE_IDLE;
+		kbd->key_hit = 2;
+		input_report_key(kbd->input, kbd->keycode[kbd->key_last_changed], 0);
+		return 2;
+	}
+	kbd->key_state = KEY_STATE_IDLE;
+	return 0;
+}
+
+void genmatrix_report_irq(struct device *dev)
+{
+	int res;
+	struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
+	res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+	kbd->deactivate_all(kbd->hw_dev, res);
+	mod_timer(&kbd->timer, jiffies + 1);
+}
+
+EXPORT_SYMBOL(genmatrix_report_irq);
+
+static void genmatrix_timer(unsigned long context)
+{
+	struct genmatrix_kbd *kbd = (struct genmatrix_kbd *)context;
+	int res;
+	unsigned long ticks;
+
+	if (test_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags)) {
+		res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+		kbd->deactivate_all(kbd->hw_dev, res);
+	}
+
+	res = genmatrix_scan(kbd);
+	if (res & 2)
+		dev_dbg(kbd->hw_dev, "genmatrix_scan returned %d, state %d, last %d\n",
+			res, kbd->key_state, kbd->key_last_changed);
+
+	if (res || (kbd->key_state != KEY_STATE_IDLE)) {
+		res = genmatrix_down(kbd);
+		if (res)
+			dev_dbg(kbd->hw_dev, "genmatrix_down returned %d, last %d\n",
+				res, kbd->key_last_changed);
+	}
+
+	if (test_bit(GENMATRIX_FLG_WITH_IRQ_b, &kbd->flags) &&
+		(kbd->key_state == KEY_STATE_IDLE)) {
+
+		res = test_and_set_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+		kbd->activate_all(kbd->hw_dev, !res);
+		if (res || !test_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags))
+			mod_timer(&kbd->timer, jiffies + kbd->pushcheck_time);
+	} else {
+		res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+		kbd->deactivate_all(kbd->hw_dev, res);
+
+		ticks = kbd->check_time - jiffies;
+		if((long)ticks <= kbd->pushcheck_time / 4)
+			ticks = (kbd->pushcheck_time + 3) / 4;
+		if(ticks > kbd->pushcheck_time)
+			ticks = kbd->pushcheck_time;
+
+		mod_timer(&kbd->timer, jiffies + ticks);
+	}
+}
+
+#ifdef CONFIG_PM
+static int genmatrix_suspend(struct device *dev, pm_message_t state)
+{
+	int res;
+	struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
+
+	kbd->suspended = 1;
+
+	res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+	kbd->deactivate_all(kbd->hw_dev, res);
+
+	return 0;
+}
+
+static int genmatrix_resume(struct device *dev)
+{
+	struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
+
+	kbd->suspended = 0;
+
+	mod_timer(&kbd->timer, jiffies + kbd->pushcheck_time);
+	return 0;
+}
+
+#endif
+
+const char genmatrix_input_name[] = "genmatrixkbd/input";
+
+static int genmatrix_probe_common(struct device *dev, struct genmatrix_kbd *kbd)
+{
+	struct input_dev   *input_dev;
+	int    err = -ENOMEM;
+	int    i;
+
+	/* Initialize spin-lock and timer */
+	spin_lock_init(&kbd->lock);
+	setup_timer(&kbd->timer, genmatrix_timer, (unsigned long)kbd);
+
+	/* State of keyboard matrix and key press reporting */
+	kbd->key_hit = 0;
+	kbd->key_last_changed = 0;
+
+	kbd->down_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]), GFP_KERNEL);
+	if (!kbd->down_arr)
+		goto fail_arr_alloc;
+	kbd->chng_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]), GFP_KERNEL);
+	if (!kbd->chng_arr)
+		goto fail_arr_alloc;
+
+	/* Internal state for repeat processing */
+	kbd->key_state = KEY_STATE_IDLE;
+	kbd->check_time = jiffies;
+
+	kbd->push_time = msecs_to_jiffies(KEY_PUSH_T);
+	kbd->release_time = msecs_to_jiffies(KEY_RELEASE_T);
+	kbd->pushcheck_time = msecs_to_jiffies(KEY_PUSHCHECK_T);
+
+	input_dev = input_allocate_device();
+	if (!input_dev)
+		goto fail_arr_alloc;
+
+	kbd->input = input_dev;
+	kbd->hw_dev = dev;
+	dev_set_drvdata(dev, kbd);
+
+	/* Setup input device */
+	input_dev->name = "GenMatrix Keyboard";
+	input_dev->phys = kbd->phys;
+	input_dev->dev.parent = dev;
+
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->id.vendor = 0x0001;
+	input_dev->id.product = 0x0001;
+	input_dev->id.version = 0x0100;
+
+	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); /* BIT(EV_PWR) | BIT(EV_SW); */
+	input_dev->keycode = kbd->keycode;
+	input_dev->keycodesize = sizeof(*kbd->keycode);
+	input_dev->keycodemax = kbd->keycode_cnt;
+
+	for (i = 0; i < kbd->keycode_cnt; i++)
+		set_bit(kbd->keycode[i], input_dev->keybit);
+	clear_bit(0, input_dev->keybit);
+	/*
+	set_bit(SW_LID, input_dev->swbit);
+	set_bit(SW_TABLET_MODE, input_dev->swbit);
+	set_bit(SW_HEADPHONE_INSERT, input_dev->swbit);
+	*/
+
+	err = kbd->setup(kbd->hw_dev);
+	if (err) {
+		dev_err(kbd->hw_dev, "low-level hardware setup failed\n");
+		goto fail;
+	}
+
+	err = input_register_device(input_dev);
+	if (err) {
+		dev_err(kbd->hw_dev, "input device registration\n");
+		goto fail_to_register;
+	}
+
+	mod_timer(&kbd->timer, jiffies + 1);
+
+	return 0;
+
+fail_to_register:
+	kbd->release(kbd->hw_dev);
+	del_timer_sync(&kbd->timer);
+fail:
+	input_free_device(input_dev);
+
+fail_arr_alloc:
+	kfree(kbd->phys);
+	kfree(kbd->down_arr);
+	kfree(kbd->chng_arr);
+	kfree(kbd->keycode);
+
+	kbd->phys = NULL;
+	kbd->down_arr = NULL;
+	kbd->chng_arr = NULL;
+	kbd->keycode = NULL;
+
+	return err;
+}
+
+static int genmatrix_remove(struct device *dev)
+{
+	struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
+	int res;
+
+	del_timer_sync(&kbd->timer);
+
+	res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
+	kbd->deactivate_all(kbd->hw_dev, res);
+
+	del_timer_sync(&kbd->timer);
+
+	kbd->release(kbd->hw_dev);
+
+	input_unregister_device(kbd->input);
+
+	dev_set_drvdata(dev, NULL);
+
+	kfree(kbd->phys);
+	kfree(kbd->down_arr);
+	kfree(kbd->chng_arr);
+	kfree(kbd->keycode);
+
+	kfree(kbd);
+
+	return 0;
+}
+
+
+/*============================================================================*/
+/* Part of the driver specific to GPIO and platform devices */
+/*
+ *  If it is possible without nasty exports, imports and exposing internals,
+ *  this should be moved to separate file
+ */
+
+#ifdef CONFIG_GENERIC_GPIO
+
+#include <linux/interrupt.h>
+#include <asm/arch/gpio.h>
+
+static inline struct genmatrix_platform_data *
+gpiomatrix_devdata(struct device *dev)
+{
+	return dev->platform_data;
+}
+
+static int gpiomatrix_irq(int irq, void *id)
+{
+	struct device *dev = (struct device *)id;
+	genmatrix_report_irq(dev);
+	return IRQ_HANDLED;
+}
+
+static int gpiomatrix_setup(struct device *dev)
+{
+	struct genmatrix_platform_data *data = gpiomatrix_devdata(dev);
+	int si, ri;
+	int ret;
+	unsigned pin;
+
+	for(si = 0; si < data->scan_cnt; si++) {
+		pin = data->scan_pin[si];
+		ret = gpio_request(pin, "gpio-kbd-scan-out");
+		if (ret) {
+			dev_err(dev, "scan pin allocation failed %d\n",
+				pin);
+			goto scan_rq_fail;
+		}
+		if(data->emulate_oc)
+			gpio_direction_input(pin);
+		else
+			gpio_direction_output(pin, 1);
+	}
+
+	for(ri = 0; ri < data->ret_cnt; ri++) {
+		pin = data->ret_pin[ri];
+		ret = gpio_request(pin, "gpio-kbd-ret-in");
+		if (ret) {
+			dev_err(dev, "ret pin allocation failed %d\n",
+				pin);
+			goto ret_rq_fail;
+		}
+		gpio_direction_input(pin);
+	}
+
+	if (!data->with_irq)
+		return 0;
+
+	for(ri = 0; ri < data->ret_cnt; ri++) {
+		pin = data->ret_pin[ri];
+		if (request_irq(gpio_to_irq(pin), gpiomatrix_irq,
+		    IRQF_TRIGGER_FALLING, "gpio-kbd", dev)) {
+			dev_err(dev, "cannot request irq %d\n",
+				pin);
+			ret = -ENXIO;
+			goto irq_rq_fail;
+		}
+		disable_irq(gpio_to_irq(pin));
+	}
+
+	return 0;
+
+irq_rq_fail:
+	while(ri--)
+		free_irq(gpio_to_irq(data->ret_pin[ri]), dev);
+
+	ri = data->ret_cnt;
+
+ret_rq_fail:
+	while(ri--)
+		gpio_free(data->ret_pin[ri]);
+scan_rq_fail:
+	while(si--) {
+		gpio_free(data->scan_pin[si]);
+	}
+
+	return ret;
+}
+
+static void gpiomatrix_release(struct device *dev)
+{
+	/*struct platform_device *pdev = to_platform_device(dev);*/
+	struct genmatrix_platform_data *data = gpiomatrix_devdata(dev);
+	int si, ri;
+
+	if (data->with_irq) {
+		for(ri = data->ret_cnt; ri-- ; )
+			free_irq(gpio_to_irq(data->ret_pin[ri]), dev);
+	}
+
+	for(ri = data->ret_cnt; ri-- ; )
+		gpio_free(data->ret_pin[ri]);
+
+	for(si = data->scan_cnt; si-- ; )
+		gpio_free(data->scan_pin[si]);
+}
+
+static unsigned gpiomatrix_activate_all(struct device *dev, int setup_irq)
+{
+	struct genmatrix_platform_data *data = gpiomatrix_devdata(dev);
+	int si, ri;
+	unsigned ret;
+
+	if (setup_irq) {
+		for(ri = 0; ri < data->ret_cnt; ri++)
+			enable_irq(gpio_to_irq(data->ret_pin[ri]));
+	}
+
+	for(si = 0; si < data->scan_cnt; si++)
+		if (data->emulate_oc)
+			gpio_direction_output(data->scan_pin[si], 0);
+		else
+			gpio_set_value(data->scan_pin[si], 0);
+
+	for(ri = data->ret_cnt, ret = 0; ri-- ; ) {
+		ret <<= 1;
+		ret |= gpio_get_value(data->ret_pin[ri]) ? 0 : 1;
+	}
+
+	return ret;
+}
+
+static void gpiomatrix_deactivate_all(struct device *dev, int disable_irq)
+{
+	struct genmatrix_platform_data *data = gpiomatrix_devdata(dev);
+	int si, ri;
+
+	if (disable_irq) {
+		for(ri = 0; ri < data->ret_cnt; ri++)
+			disable_irq_nosync(gpio_to_irq(data->ret_pin[ri]));
+	}
+
+	for(si = 0; si < data->scan_cnt; si++) {
+		if (data->emulate_oc)
+			gpio_direction_input(data->scan_pin[si]);
+		else
+			gpio_set_value(data->scan_pin[si], 1);
+	}
+}
+
+static unsigned gpiomatrix_scan_single(struct device *dev, unsigned scannr)
+{
+	struct genmatrix_platform_data *data = gpiomatrix_devdata(dev);
+	int ri;
+	unsigned ret;
+
+	if (data->emulate_oc)
+		gpio_direction_output(data->scan_pin[scannr], 0);
+	else
+		gpio_set_value(data->scan_pin[scannr], 0);
+
+	udelay(5);
+
+	for(ri = data->ret_cnt, ret = 0; ri-- ; ) {
+		ret <<= 1;
+		ret |= gpio_get_value(data->ret_pin[ri]) ? 0 : 1;
+	}
+
+	if (data->emulate_oc)
+		gpio_direction_input(data->scan_pin[scannr]);
+	else
+		gpio_set_value(data->scan_pin[scannr], 1);
+
+	return ret;
+}
+
+#endif /* CONFIG_GENERIC_GPIO */
+
+
+#ifdef CONFIG_PM
+
+static int genmatrix_plat_suspend(struct platform_device *dev, pm_message_t state)
+{
+	return genmatrix_suspend(&dev->dev, state);
+}
+
+static int genmatrix_plat_resume(struct platform_device *dev)
+{
+	return genmatrix_resume(&dev->dev);
+}
+
+#else
+#define genmatrix_plat_suspend	NULL
+#define genmatrix_plat_resume	NULL
+#endif
+
+static int genmatrix_plat_probe(struct platform_device *dev)
+{
+	struct genmatrix_platform_data *pdata;
+	struct genmatrix_kbd *kbd;
+	unsigned keycode_tab_size;
+	int    err = -ENOMEM;
+	int    res;
+
+	kbd = kzalloc(sizeof(*kbd), GFP_KERNEL);
+	if (!kbd)
+		goto fail_kbd_alloc;
+
+	/* Platform specific information */
+	pdata = dev->dev.platform_data;
+	kbd->phys = kzalloc(sizeof(genmatrix_input_name)+4, GFP_KERNEL);
+	if (!kbd->phys)
+		goto fail_other;
+	strcpy(kbd->phys, genmatrix_input_name);
+	kbd->phys[strlen(kbd->phys)] = '0';
+
+	kbd->scan_cnt = pdata->scan_cnt;
+	kbd->ret_cnt = pdata->ret_cnt;
+
+	kbd->setup = pdata->setup;
+	kbd->release = pdata->release;
+	kbd->activate_all = pdata->activate_all;
+	kbd->deactivate_all = pdata->deactivate_all;
+	kbd->scan_single = pdata->scan_single;
+
+
+#ifdef CONFIG_GENERIC_GPIO
+	if(kbd->setup == NULL)
+		kbd->setup = gpiomatrix_setup;
+	if(kbd->release == NULL)
+		kbd->release = gpiomatrix_release;
+	if(kbd->activate_all == NULL)
+		kbd->activate_all = gpiomatrix_activate_all;
+	if(kbd->deactivate_all == NULL)
+		kbd->deactivate_all = gpiomatrix_deactivate_all;
+	if(kbd->scan_single == NULL)
+		kbd->scan_single = gpiomatrix_scan_single;
+#endif /* CONFIG_GENERIC_GPIO */
+
+	/* Scancode to key translation */
+	kbd->keycode_cnt = pdata->keycode_cnt;
+	keycode_tab_size = sizeof(*kbd->keycode) * kbd->keycode_cnt;
+	kbd->keycode = kzalloc(keycode_tab_size, GFP_KERNEL);;
+	if (!kbd->keycode)
+		goto fail_other;
+	memcpy(kbd->keycode, pdata->keycode, keycode_tab_size);
+	if(pdata->with_irq)
+		set_bit(GENMATRIX_FLG_WITH_IRQ_b, &kbd->flags);
+
+	res = genmatrix_probe_common(&dev->dev, kbd);
+
+	if (!res)
+		return 0;
+
+fail_other:
+	kfree(kbd->keycode);
+	kfree(kbd->phys);
+
+	kfree(kbd);
+fail_kbd_alloc:
+	dev_err(&dev->dev, "keyboard inicialization failed (ret = %d)\n", err);
+	return err;
+}
+
+static int genmatrix_plat_remove(struct platform_device *dev)
+{
+	return genmatrix_remove(&dev->dev);
+}
+
+static struct platform_driver genmatrix_driver = {
+	.probe		= genmatrix_plat_probe,
+	.remove		= genmatrix_plat_remove,
+	.suspend	= genmatrix_plat_suspend,
+	.resume		= genmatrix_plat_resume,
+	.driver		= {
+		.name	= "genmatrix-keyboard",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init genmatrix_init(void)
+{
+	return platform_driver_register(&genmatrix_driver);
+}
+
+static void __exit genmatrix_exit(void)
+{
+	platform_driver_unregister(&genmatrix_driver);
+}
+
+module_init(genmatrix_init);
+module_exit(genmatrix_exit);
+
+MODULE_DESCRIPTION("Generic Matrix Keyboard Device");
+MODULE_AUTHOR("Pavel Pisa, PiKRON");
+MODULE_LICENSE("GPL");
Index: linux-2.6.23-git/include/linux/genmatrix_kbd.h
===================================================================
--- /dev/null
+++ linux-2.6.23-git/include/linux/genmatrix_kbd.h
@@ -0,0 +1,34 @@
+/*
+ *  Generic matrix keyboard driver for embedded platforms
+ *
+ *  Copyright (c) 2007 Pavel Pisa
+ *
+ *  Based on PiKRON's COLAMI keyboard code and corgikbd.c
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ */
+
+struct device ;
+
+void genmatrix_report_irq(struct device *dev);
+
+struct genmatrix_platform_data {
+	unsigned           scan_cnt;
+	unsigned           ret_cnt;
+	unsigned           *scan_pin;
+	unsigned           *ret_pin;
+
+	int  (*setup)(struct device *dev);
+	void (*release)(struct device *dev);
+	unsigned (*activate_all)(struct device *dev, int setup_irq);
+	void (*deactivate_all)(struct device *dev, int disable_irq);
+	unsigned (*scan_single)(struct device *dev, unsigned scannr);
+
+	unsigned           keycode_cnt;
+	unsigned char      *keycode;
+	char               with_irq;
+	char               emulate_oc;
+};

Re: [PATCH] Generic, platform independent matrix keyboard support

[Dmitry Torokhov]

Hi Pavel,

On 10/9/07, Pavel Pisa <ppisa4lists@pikron.com> wrote:
> Subject: Generic, platform independent matrix keyboard support
> From: Pavel Pisa <pisa@cmp.felk.cvut.cz>
>
> The genmatrix_kbd module provides support for matrix keyboard
> where switches interconnects return lines with port driven
> scan lines. Actual version code allow to register concrete
> hardware described by platform device. Hardware can be described
> by list of output and input pins and manipulation can be delegated
> to GPIO layer or hardware specific setup(), release(), activate_all()
> deactivate_all() and scan_single() functions can be defined.
>

Only commenting on the generic part, not GPIO for now...

> +
> +#include <linux/genmatrix_kbd.h>
> +
> +/* Some sane defaults */
> +#define KEY_PUSH_T      20
> +#define KEY_RELEASE_T   10
> +#define KEY_PUSHCHECK_T  20
> +
> +/* Individual key states */
> +#define KEY_STATE_IDLE     0
> +#define KEY_STATE_PUSH     1
> +#define KEY_STATE_RELEASE  2
> +#define KEY_STATE_PUSHED   4
> +#define KEY_STATE_NOISE    8
> +#define KEY_STATE_BUSY     (KEY_STATE_PUSH|KEY_STATE_RELEASE)
> +
> +/* Bit flags */
> +#define GENMATRIX_FLG_WITH_IRQ_b 0
> +#define GENMATRIX_FLG_IRQ_EN_b   1
> +
> +typedef unsigned genmatrix_retmask_t;
> +
> +struct genmatrix_kbd {
> +       struct input_dev   *input;
> +       struct device      *hw_dev;
> +       char   *phys;
> +       unsigned long      flags;
> +
> +       /* Platform specific information */
> +       unsigned           scan_cnt;
> +       unsigned           ret_cnt;
> +
> +       int  (*setup)(struct device *dev);
> +       void (*release)(struct device *dev);
> +       unsigned (*activate_all)(struct device *dev, int setup_irq);
> +       void (*deactivate_all)(struct device *dev, int disable_irq);
> +       unsigned (*scan_single)(struct device *dev, unsigned scannr);
> +
> +       /* State of keyboard matrix and key press reporting */
> +       genmatrix_retmask_t *down_arr;  /* array of size scan_cnt */
> +       genmatrix_retmask_t *chng_arr;  /* array of size scan_cnt */
> +       unsigned char      key_hit;

I see you are setting it but never use...

> +       int                key_last_changed;
> +
> +       /* Internal state for repeat processing */
> +       int                key_state;
> +       unsigned long      check_time;

Input core alredy has repeat processing. Is it not sufficient?

> +
> +       unsigned long      push_time;
> +       unsigned long      release_time;
> +       unsigned long      pushcheck_time;
> +
> +       spinlock_t         lock;

OK, you have a lock, but I don't see it being used anywhere.

> +       struct timer_list  timer;
> +
> +       /* Scancode to key translation */
> +       unsigned           keycode_cnt;
> +       unsigned char      *keycode;
> +
> +       int                suspended;

What is 'suspended' member needed for?

> +};
> +
> +/**
> + * genmatrix_scan - Scan keyboard matrix and report requests for state change
> + * @kbd:       keyboard instance data and state structure
> + *
> + * Scans keyboard matrix connected row by row by calling function
> + * mx1_kbd_onerow(). Number of scanned output lines is defined
> + * by %KBD_SCAN_CNT. Checks read keyboard state against @down_arr
> + * and updates @key_change_arr array. The @down_arr state is
> + * left unchanged. It is changed later by kbd_down() function.
> + * Returns 0, if no keyboard activity is found. Returns 1
> + * if at least one key is pressed. Returns 2 or 3 in case
> + * of detected change.
> + */

The comment refers to non-existing functions. This makes harder to
fugure out what is going on.

> +static int genmatrix_scan(struct genmatrix_kbd *kbd)
> +{
> +       int i, ret = 0;
> +       genmatrix_retmask_t val, chng;
> +       for (i = 0; i < kbd->scan_cnt; i++) {
> +               val = kbd->scan_single(kbd->hw_dev, i);
> +               chng = val ^ kbd->down_arr[i];
> +               kbd->chng_arr[i] = chng;

Ok, so there can be only 32 keys per row... Kind of unfortunate
limitation for generic layer. Although if you forget about matrix
stuff and convert down_arr and chg_arr to be just a bitmaps and pass
it to scan_single along with irq (if any) that could be generic
enough.

> +               if (val)
> +                       ret |= 1;
> +               if (chng)
> +                       ret |= 2;
> +       }
> +       return ret;
> +}
> +
> +/**
> + * genmatrix_down - Detects changed key scancode and applies changes to matrix state
> + * @kbd:       keyboard instance data and state structure
> + *
> + * Functions check @chng_arr and process changes.
> + * It updates its internal state @key_state, does
> + * noise cancellation and repeat timing, then updates
> + * @down_arr, stores detected scancode to @key_last_changed
> + * and calls modifiers processing kbd_scan2mod().
> + * Return value is zero if no change is detected.
> + * In other case evaluated scancode is returned.
> + * Variable @key_hit signals by value 1 pressed key, by value
> + * 2 key release.
> + */
> +static int genmatrix_down(struct genmatrix_kbd *kbd)
> +{
> +       int si, ri = 0;
> +       unsigned char val;
> +       unsigned long act_time = jiffies;
> +
> +       if (!(kbd->key_state & KEY_STATE_BUSY)) {
> +               for (si = 0; si < kbd->scan_cnt; si++) {
> +                       val = kbd->chng_arr[si];
> +                       if (!val) continue;
> +                       ri = fls(val) - 1;
> +                       kbd->key_last_changed = si * kbd->ret_cnt + ri;
> +                       if (kbd->down_arr[si] & (1 << ri)) {
> +                               kbd->check_time = act_time + kbd->push_time;
> +                               kbd->key_state = KEY_STATE_RELEASE;
> +                       } else {
> +                               kbd->check_time = act_time + kbd->release_time;
> +                               kbd->key_state = KEY_STATE_PUSH;
> +                       }
> +                       break;
> +               }
> +               if (kbd->key_state == KEY_STATE_IDLE)
> +                       return 0;
> +       } else {
> +               if (kbd->key_last_changed < 0) {
> +                       kbd->key_state = KEY_STATE_IDLE;
> +                       return 0;
> +               }
> +               si = (kbd->key_last_changed) / kbd->ret_cnt;
> +               ri = (kbd->key_last_changed) % kbd->ret_cnt;
> +               if (!(kbd->chng_arr[si] & (1 << ri))) {
> +                       /* Noise detected */
> +                       if (!(kbd->key_state & KEY_STATE_NOISE)) {
> +                               kbd->check_time = act_time + kbd->release_time;
> +                               kbd->key_state |= KEY_STATE_NOISE;
> +                       }
> +               }
> +       }
> +
> +       if (!time_after(jiffies, kbd->check_time))
> +               return 0;
> +
> +       if (kbd->key_state == KEY_STATE_PUSH) {
> +               kbd->down_arr[si] |= 1 << ri;
> +               kbd->key_state = KEY_STATE_PUSHED;
> +               kbd->check_time = act_time + kbd->pushcheck_time;
> +               kbd->key_hit = 1;
> +               input_report_key(kbd->input, kbd->keycode[kbd->key_last_changed], 1);
> +               return 1;

What happens if we have several keys pressed at once? It looks like
we'll lose keypresses.

> +       } else if (kbd->key_state == KEY_STATE_PUSHED) {
> +               kbd->check_time = act_time + kbd->pushcheck_time;
> +               return 0;

Input core already does filtering of duplicate events, there is no
need to reimplement it.

> +       } else if (kbd->key_state == KEY_STATE_RELEASE) {
> +               kbd->down_arr[si] &= ~(1<<ri);
> +               kbd->key_state = KEY_STATE_IDLE;
> +               kbd->key_hit = 2;
> +               input_report_key(kbd->input, kbd->keycode[kbd->key_last_changed], 0);
> +               return 2;

And we risk losing releases as well.

> +       }
> +       kbd->key_state = KEY_STATE_IDLE;
> +       return 0;
> +}
> +
> +/**
> + * genmatrix_report_irq - The call-back notifying about IRQ arrival
> + * @dev:       device structure of given genmatrix instance
> + *
> + * This function is called by concrete keyboard hardware adaptation
> + * layer to notify driver core about activation of some return line.
> + * The function deactivates lines and queues event for delayed
> + * processing by the driver core
> + */
> +void genmatrix_report_irq(struct device *dev)
> +{
> +       int res;
> +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> +       res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> +       kbd->deactivate_all(kbd->hw_dev, res);
> +       mod_timer(&kbd->timer, jiffies + 1);

Why do we need to relay the work to timer? The execution context is
the same, why not do it right here?

> +}
> +EXPORT_SYMBOL(genmatrix_report_irq);
> +
> +/**
> + * genmatrix_timer - Time and event driven processing of keyboard state
> + * @context:   registered pointer to keyboard instance data and state structure
> + *
> + * The function disables IRQs if they have been enabled, the it calls
> + * genmatrix_scan() which reports found changes by setting corresponding
> + * bits of @chng_arr. If there is change or there is unfinished
> + * previous state processing, genmatrix_down() is called.
> + * If the final state is %KEY_STATE_IDLE, the wait for state
> + * change is armed else periodic matrix monitoring is scheduled.
> + */
> +static void genmatrix_timer(unsigned long context)
> +{
> +       struct genmatrix_kbd *kbd = (struct genmatrix_kbd *)context;
> +       int res;
> +       unsigned long ticks;
> +
> +       if (test_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags)) {
> +               res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);

What if interrupt arrives here?

> +               kbd->deactivate_all(kbd->hw_dev, res);
> +       }
> +
> +       res = genmatrix_scan(kbd);
> +       if (res & 2)
> +               dev_dbg(kbd->hw_dev, "genmatrix_scan returned %d, state %d, last %d\n",
> +                       res, kbd->key_state, kbd->key_last_changed);
> +
> +       if (res || (kbd->key_state != KEY_STATE_IDLE)) {
> +               res = genmatrix_down(kbd);
> +               if (res)
> +                       dev_dbg(kbd->hw_dev, "genmatrix_down returned %d, last %d\n",
> +                               res, kbd->key_last_changed);
> +       }
> +
> +       if (test_bit(GENMATRIX_FLG_WITH_IRQ_b, &kbd->flags) &&
> +               (kbd->key_state == KEY_STATE_IDLE)) {
> +
> +               res = test_and_set_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> +               kbd->activate_all(kbd->hw_dev, !res);

Do we really need this test? How can IRQ be already enabled here?

> +               if (res || !test_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags))
> +                       mod_timer(&kbd->timer, jiffies + kbd->pushcheck_time);
> +       } else {
> +               res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> +               kbd->deactivate_all(kbd->hw_dev, res);

Do you expect IRQ mode to change after device registration?

> +               ticks = kbd->check_time - jiffies;
> +               if ((long)ticks <= kbd->pushcheck_time / 4)
> +                       ticks = (kbd->pushcheck_time + 3) / 4;
> +               if (ticks > kbd->pushcheck_time)
> +                       ticks = kbd->pushcheck_time;
> +

Why is this needed? In polled mode I expect the timer fie exactly
every pushcheck_time jiffies.

> +               mod_timer(&kbd->timer, jiffies + ticks);
> +       }
> +}
> +
> +#ifdef CONFIG_PM
> +static int genmatrix_suspend(struct device *dev, pm_message_t state)
> +{
> +       int res;
> +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> +
> +       kbd->suspended = 1;
> +
> +       res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> +       kbd->deactivate_all(kbd->hw_dev, res);
> +
> +       return 0;
> +}
> +
> +static int genmatrix_resume(struct device *dev)
> +{
> +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> +
> +       kbd->suspended = 0;
> +
> +       mod_timer(&kbd->timer, jiffies + kbd->pushcheck_time);

Only restart timer if device is open.

> +       return 0;
> +}
> +
> +#endif
> +
> +const char genmatrix_input_name[] = "genmatrixkbd/input";

Does not seem to be used.

> +
> +static int genmatrix_probe_common(struct device *dev, struct genmatrix_kbd *kbd)
> +{
> +       struct input_dev   *input_dev;
> +       int    err = -ENOMEM;
> +       int    i;
> +
> +       /* Initialize spin-lock and timer */
> +       spin_lock_init(&kbd->lock);
> +       setup_timer(&kbd->timer, genmatrix_timer, (unsigned long)kbd);
> +
> +       /* State of keyboard matrix and key press reporting */
> +       kbd->key_hit = 0;
> +       kbd->key_last_changed = 0;
> +
> +       kbd->down_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]), GFP_KERNEL);
> +       if (!kbd->down_arr)
> +               goto fail_arr_alloc;
> +       kbd->chng_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]), GFP_KERNEL);
> +       if (!kbd->chng_arr)
> +               goto fail_arr_alloc;
> +
> +       /* Internal state for repeat processing */
> +       kbd->key_state = KEY_STATE_IDLE;
> +       kbd->check_time = jiffies;
> +
> +       kbd->push_time = msecs_to_jiffies(KEY_PUSH_T);
> +       kbd->release_time = msecs_to_jiffies(KEY_RELEASE_T);
> +       kbd->pushcheck_time = msecs_to_jiffies(KEY_PUSHCHECK_T);

I think these should be pre-initialized before getting into
generic_probe. We may try to apply sane defaults if they are 0 but
callers should be able to specify their own scan parameters. BTW
genmatrix_register() is probably a better name for this function.

> +
> +       input_dev = input_allocate_device();
> +       if (!input_dev)
> +               goto fail_arr_alloc;
> +
> +       kbd->input = input_dev;
> +       kbd->hw_dev = dev;
> +       dev_set_drvdata(dev, kbd);

Does not belong here.

> +
> +       /* Setup input device */
> +       input_dev->name = "GenMatrix Keyboard";
> +       input_dev->phys = kbd->phys;
> +       input_dev->dev.parent = dev;

Make this input_dev->dev.parent = kbd->dev; and get rid of dev argument.

> +
> +       input_dev->id.bustype = BUS_HOST;
> +       input_dev->id.vendor = 0x0001;
> +       input_dev->id.product = 0x0001;
> +       input_dev->id.version = 0x0100;
> +
> +       input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); /* BIT(EV_PWR) | BIT(EV_SW); */
> +       input_dev->keycode = kbd->keycode;
> +       input_dev->keycodesize = sizeof(*kbd->keycode);
> +       input_dev->keycodemax = kbd->keycode_cnt;
> +
> +       for (i = 0; i < kbd->keycode_cnt; i++)
> +               set_bit(kbd->keycode[i], input_dev->keybit);
> +       clear_bit(0, input_dev->keybit);
> +       /*
> +       set_bit(SW_LID, input_dev->swbit);
> +       set_bit(SW_TABLET_MODE, input_dev->swbit);
> +       set_bit(SW_HEADPHONE_INSERT, input_dev->swbit);
> +       */
> +
> +       err = kbd->setup(kbd->hw_dev);
> +       if (err) {
> +               dev_err(kbd->hw_dev, "low-level hardware setup failed\n");
> +               goto fail;
> +       }
> +
> +       err = input_register_device(input_dev);
> +       if (err) {
> +               dev_err(kbd->hw_dev, "input device registration\n");
> +               goto fail_to_register;
> +       }
> +
> +       mod_timer(&kbd->timer, jiffies + 1);

Do not start timer/IRQs until there are users. Implement
inptu_dev->open() abd ->close() and do it from there.

> +
> +       return 0;
> +
> +fail_to_register:
> +       kbd->release(kbd->hw_dev);
> +       del_timer_sync(&kbd->timer);
> +fail:
> +       input_free_device(input_dev);
> +
> +fail_arr_alloc:
> +       kfree(kbd->phys);
> +       kfree(kbd->down_arr);
> +       kfree(kbd->chng_arr);
> +       kfree(kbd->keycode);
> +
> +       kbd->phys = NULL;
> +       kbd->down_arr = NULL;
> +       kbd->chng_arr = NULL;
> +       kbd->keycode = NULL;
> +
> +       return err;
> +}
> +
> +static int genmatrix_remove(struct device *dev)
> +{
> +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> +       int res;
> +
> +       del_timer_sync(&kbd->timer);
> +
> +       res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> +       kbd->deactivate_all(kbd->hw_dev, res);
> +
> +       del_timer_sync(&kbd->timer);
> +
> +       kbd->release(kbd->hw_dev);
> +
> +       input_unregister_device(kbd->input);
> +
> +       dev_set_drvdata(dev, NULL);
> +
> +       kfree(kbd->phys);
> +       kfree(kbd->down_arr);
> +       kfree(kbd->chng_arr);
> +       kfree(kbd->keycode);

I don't see you allocate kbd->keycode, why are you freeing it?

> +
> +       kfree(kbd);

Actually, why are you freeing kbd? If it was not allocated in probe(),
it should not be freed in remove().

The main advantage is that the code deals with noisy devices... How
widespread are they? I think we should take another look at this once
current implementation cleaned up.

Thanks.

-- 
Dmitry

Re: [PATCH] Generic, platform independent matrix keyboard support

[Pavel Pisa]

Hello Dimitry,

thanks for review and excuse for my mistakes and not well
described things.

On Tuesday 09 October 2007 18:22, Dmitry Torokhov wrote:
> On 10/9/07, Pavel Pisa <ppisa4lists@pikron.com> wrote:
> > Subject: Generic, platform independent matrix keyboard support
>
> Only commenting on the generic part, not GPIO for now...

OK

> > +       /* State of keyboard matrix and key press reporting */
> > +       genmatrix_retmask_t *down_arr;  /* array of size scan_cnt */
> > +       genmatrix_retmask_t *chng_arr;  /* array of size scan_cnt */
> > +       unsigned char      key_hit;
>
> I see you are setting it but never use...

It slipped there from original code, which solves even key repeat
and shift/modifiers processing in same FSM as noise suppression.
The actual dual processing in the driver and input layer leads
to some waste of timer, but is better layered. I agree, that
Linux input subsystem core has evolved into really advanced
piece of code. The option would be to integrate optional noise
suppression directly into it. But I am not sure, if it would not
lead more to mess then good design.

So I am removing abundant key_hit.

> > +       int                key_last_changed;
> > +
> > +       /* Internal state for repeat processing */
> > +       int                key_state;
> > +       unsigned long      check_time;
>
> Input core alredy has repeat processing. Is it not sufficient?

OK, comment has not been updated, state is only
for noise suppression processing in the Linux case.
Comment corrected.

> > +
> > +       unsigned long      push_time;
> > +       unsigned long      release_time;
> > +       unsigned long      pushcheck_time;
> > +
> > +       spinlock_t         lock;
>
> OK, you have a lock, but I don't see it being used anywhere.

Good catch, I have thought that I would need it to serialize
interrupts, timer processing and IRQ enabling. I have solved
all these without need of any locking. If it is preferred,
I remove it. On the other hand this is possible, that it could
be required in next driver iteration.
...
OK, I remove it to not harm your eyes.

> > +       struct timer_list  timer;
> > +
> > +       /* Scancode to key translation */
> > +       unsigned           keycode_cnt;
> > +       unsigned char      *keycode;
> > +
> > +       int                suspended;
>
> What is 'suspended' member needed for?

It holds that information. It is found in other drivers.
I expected to use it to block IRQ and timer processing
in the suspended state. I have added check to the
genmatrix_timer() for it.

> > +};
> > +
> > +/**
> > + * genmatrix_scan - Scan keyboard matrix and report requests for state
> > change + * @kbd:       keyboard instance data and state structure
> > + *
> > + * Scans keyboard matrix connected row by row by calling function
> > + * mx1_kbd_onerow(). Number of scanned output lines is defined
> > + * by %KBD_SCAN_CNT. Checks read keyboard state against @down_arr
> > + * and updates @key_change_arr array. The @down_arr state is
> > + * left unchanged. It is changed later by kbd_down() function.
> > + * Returns 0, if no keyboard activity is found. Returns 1
> > + * if at least one key is pressed. Returns 2 or 3 in case
> > + * of detected change.
> > + */
>
> The comment refers to non-existing functions. This makes harder to
> fugure out what is going on.

OK, comments updated

> > +static int genmatrix_scan(struct genmatrix_kbd *kbd)
> > +{
> > +       int i, ret = 0;
> > +       genmatrix_retmask_t val, chng;
> > +       for (i = 0; i < kbd->scan_cnt; i++) {
> > +               val = kbd->scan_single(kbd->hw_dev, i);
> > +               chng = val ^ kbd->down_arr[i];
> > +               kbd->chng_arr[i] = chng;
>
> Ok, so there can be only 32 keys per row... Kind of unfortunate
> limitation for generic layer. Although if you forget about matrix
> stuff and convert down_arr and chg_arr to be just a bitmaps and pass
> it to scan_single along with irq (if any) that could be generic
> enough.

OK, you are right that I have not thought about this limitation.
On the other hand I cannot imagine electronic designed wanting
so many wires. Reduction of the wires counts is why matrix keyboards
are used. The 32 bits limitation then means that maximal supported
keyboard can be 32x32 key => 1024 keys, that is quite imposing.
I cannot imagine anybody selecting something like 2x33 keys,
but you are right that there is limitation. 32 lines are natural
limit of pin number quickly accessible by single read on 32-bit
system if optimized low level stuff is used. The same comes to mind
as limit for simple passed function return values. I have thought
and used this knowledge of matrix organization for simpler probe
in down processing, but this optimization in not used there yet.
I would be happy, if I would not need to expose down_arr and
chg_arr details to hardware specific functions as well.
If bits are simple array, then fast processing of return pins read
for one scan line gets to be complicated. 

So I agree, that there is limit. I try to thing about it a little
more.

> > +       if (kbd->key_state == KEY_STATE_PUSH) {
> > +               kbd->down_arr[si] |= 1 << ri;
> > +               kbd->key_state = KEY_STATE_PUSHED;
> > +               kbd->check_time = act_time + kbd->pushcheck_time;
> > +               kbd->key_hit = 1;
> > +               input_report_key(kbd->input,
> > kbd->keycode[kbd->key_last_changed], 1); +               return 1;
>
> What happens if we have several keys pressed at once? It looks like
> we'll lose keypresses.

No, the processing is done such way, that first detected change is
noted and checked until key gets stable. Then change is propagated
into down_arr and other key change is looked for.
This is safe. You can argue, that the double key simultaneous press
is not propagated with same time delay equivalent to noise check
for both keys. That is correct, but user is not typically able
to time keys so precisely or rely on so precise key press events
timing. It could be problem for organ keys, but I think, that
it is different case, requires press force sensitivity in most
cases and different kind of HW solution.

I see as only other, really clean solution to have state automata
and timer for each key, but it would lead to much much higher
resources demand.

> > +       } else if (kbd->key_state == KEY_STATE_PUSHED) {
> > +               kbd->check_time = act_time + kbd->pushcheck_time;
> > +               return 0;
>
> Input core already does filtering of duplicate events, there is no
> need to reimplement it.

You are right there, but state processing is necessary for noise
filterring anyway so I do not see any reason for the overhead
of calling of the input layer there.

> > +       } else if (kbd->key_state == KEY_STATE_RELEASE) {
> > +               kbd->down_arr[si] &= ~(1<<ri);
> > +               kbd->key_state = KEY_STATE_IDLE;
> > +               kbd->key_hit = 2;
> > +               input_report_key(kbd->input,
> > kbd->keycode[kbd->key_last_changed], 0); +               return 2;
>
> And we risk losing releases as well.

No, same serialization there as above.

> > +void genmatrix_report_irq(struct device *dev)
> > +{
> > +       int res;
> > +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> > +       res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags);
> > +       kbd->deactivate_all(kbd->hw_dev, res);
> > +       mod_timer(&kbd->timer, jiffies + 1);
>
> Why do we need to relay the work to timer? The execution context is
> the same, why not do it right here?

Because else there are two not synchronized context accessing same
state structure => serialization is required and Linux kernel
driver model would require ugly spinlock_irq there over relatively
long periods. This is bad for other interrupts and timers latencies
if RT patch is not used.

Other options is to be absolutely sure, that IRQ and timer
cannot be activated in parallel, but again, for SMP this
means some busy loops in irq_disable leading to IPI ...
nothing nice. In this case it is left on kernel to synchronize
timer removal and activation and ensuring that timer runs
only once. Again it could lead to complex cases on SMP,
but it is hidden in the kernel core and should be optimal
for given kernel build. The minimal delay is even advantage
for fast stopping of processing spike caused by noise.

But may it be, that there is some better solution there.

> > +static void genmatrix_timer(unsigned long context)
> > +{
> > +       struct genmatrix_kbd *kbd = (struct genmatrix_kbd *)context;
> > +       int res;
> > +       unsigned long ticks;
> > +
> > +       if (test_bit(GENMATRIX_FLG_IRQ_EN_b, &kbd->flags)) {
> > +               res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b,
> > &kbd->flags);
>
> What if interrupt arrives here?

If the interrupts are edge trigerred (reasonable requirement for keyboard),
than there is no problem. But for level triggered case this could lead
to the problem. But how could that be done better? I do not like to expose
interrupts numbers to the driver core. I did not like to move state into
deactivate_all/activate_all functions. This does not to seems simple for
me. Or OK, I can add spinlock and block IRQs over whole test_and_clear_bit() 
and deactivate_all(). Probably no so big lose for non RT kernels which
do not guarantee anything and converted to RT-mutex for RT ones.
But anyway, there may be some race problem in theory on SMP.

Returning lock to rethink this again.

> > +               kbd->deactivate_all(kbd->hw_dev, res);
> > +       }
> > +
> > +       res = genmatrix_scan(kbd);
> > +       if (test_bit(GENMATRIX_FLG_WITH_IRQ_b, &kbd->flags) &&
> > +               (kbd->key_state == KEY_STATE_IDLE)) {
> > +
> > +               res = test_and_set_bit(GENMATRIX_FLG_IRQ_EN_b,
> > &kbd->flags); +               kbd->activate_all(kbd->hw_dev, !res);
>
> Do we really need this test? How can IRQ be already enabled here?

It could be simplified probably.

> > +               if (res || !test_bit(GENMATRIX_FLG_IRQ_EN_b,
> > &kbd->flags)) +                       mod_timer(&kbd->timer, jiffies +
> > kbd->pushcheck_time); +       } else {
> > +               res = test_and_clear_bit(GENMATRIX_FLG_IRQ_EN_b,
> > &kbd->flags); +               kbd->deactivate_all(kbd->hw_dev, res);
>
> Do you expect IRQ mode to change after device registration?

Not at this moment.

> > +               ticks = kbd->check_time - jiffies;
> > +               if ((long)ticks <= kbd->pushcheck_time / 4)
> > +                       ticks = (kbd->pushcheck_time + 3) / 4;
> > +               if (ticks > kbd->pushcheck_time)
> > +                       ticks = kbd->pushcheck_time;
> > +
>
> Why is this needed? In polled mode I expect the timer fie exactly
> every pushcheck_time jiffies.

The first comparison ensures, that in kernel overload state the keyboard
would not make things even worse. The second one, that the extensive long
time is not used. The timing can be adjusted for different states, but may
it be, that it could be simplified for use within Linux kernel.

> > +static int genmatrix_resume(struct device *dev)
> > +{
> > +       struct genmatrix_kbd *kbd = dev_get_drvdata(dev);
> > +
> > +       kbd->suspended = 0;
> > +
> > +       mod_timer(&kbd->timer, jiffies + kbd->pushcheck_time);
>
> Only restart timer if device is open.

OK.

> > +       return 0;
> > +}
> > +
> > +#endif
> > +
> > +const char genmatrix_input_name[] = "genmatrixkbd/input";
>
> Does not seem to be used.

This is required for kbd->phys, which is needed for input_dev->phys.
I should move this near to genmatrix_plat_probe() instead of one
global name. But then name should reflect that. I should find
how to add some pseudo/unique number to the input to make
it better addressable.

> > +
> > +static int genmatrix_probe_common(struct device *dev, struct
> > genmatrix_kbd *kbd) +{
> > +       struct input_dev   *input_dev;
> > +       int    err = -ENOMEM;
> > +       int    i;
> > +
> > +       /* Initialize spin-lock and timer */
> > +       spin_lock_init(&kbd->lock);
> > +       setup_timer(&kbd->timer, genmatrix_timer, (unsigned long)kbd);
> > +
> > +       /* State of keyboard matrix and key press reporting */
> > +       kbd->key_hit = 0;
> > +       kbd->key_last_changed = 0;
> > +
> > +       kbd->down_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]),
> > GFP_KERNEL); +       if (!kbd->down_arr)
> > +               goto fail_arr_alloc;
> > +       kbd->chng_arr = kzalloc(kbd->scan_cnt * sizeof(kbd->down_arr[0]),
> > GFP_KERNEL); +       if (!kbd->chng_arr)
> > +               goto fail_arr_alloc;
> > +
> > +       /* Internal state for repeat processing */
> > +       kbd->key_state = KEY_STATE_IDLE;
> > +       kbd->check_time = jiffies;
> > +
> > +       kbd->push_time = msecs_to_jiffies(KEY_PUSH_T);
> > +       kbd->release_time = msecs_to_jiffies(KEY_RELEASE_T);
> > +       kbd->pushcheck_time = msecs_to_jiffies(KEY_PUSHCHECK_T);
>
> I think these should be pre-initialized before getting into
> generic_probe. We may try to apply sane defaults if they are 0 but
> callers should be able to specify their own scan parameters. BTW
> genmatrix_register() is probably a better name for this function.

Changed to allow caller specify different values.

Name of corresponding platform and PCI methods is probe.
But name can be changed if new one is seen as more logical.

> > +
> > +       input_dev = input_allocate_device();
> > +       if (!input_dev)
> > +               goto fail_arr_alloc;
> > +
> > +       kbd->input = input_dev;
> > +       kbd->hw_dev = dev;
> > +       dev_set_drvdata(dev, kbd);
>
> Does not belong here.

Where I can store pointer to specialized part.

> > +
> > +       /* Setup input device */
> > +       input_dev->name = "GenMatrix Keyboard";
> > +       input_dev->phys = kbd->phys;
> > +       input_dev->dev.parent = dev;
>
> Make this input_dev->dev.parent = kbd->dev; and get rid of dev argument.

I am not fully sure what it would cause in device model hierarchy,
but I try to learn and check that.

> > +
> > +       input_dev->id.bustype = BUS_HOST;
> > +       input_dev->id.vendor = 0x0001;
> > +       input_dev->id.product = 0x0001;
> > +       input_dev->id.version = 0x0100;
> > +
> > +       input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); /* BIT(EV_PWR) |
> > BIT(EV_SW); */ +       input_dev->keycode = kbd->keycode;
> > +       input_dev->keycodesize = sizeof(*kbd->keycode);
> > +       input_dev->keycodemax = kbd->keycode_cnt;
> > +
> > +       for (i = 0; i < kbd->keycode_cnt; i++)
> > +               set_bit(kbd->keycode[i], input_dev->keybit);
> > +       clear_bit(0, input_dev->keybit);
> > +       /*
> > +       set_bit(SW_LID, input_dev->swbit);
> > +       set_bit(SW_TABLET_MODE, input_dev->swbit);
> > +       set_bit(SW_HEADPHONE_INSERT, input_dev->swbit);
> > +       */
> > +
> > +       err = kbd->setup(kbd->hw_dev);
> > +       if (err) {
> > +               dev_err(kbd->hw_dev, "low-level hardware setup
> > failed\n"); +               goto fail;
> > +       }
> > +
> > +       err = input_register_device(input_dev);
> > +       if (err) {
> > +               dev_err(kbd->hw_dev, "input device registration\n");
> > +               goto fail_to_register;
> > +       }
> > +
> > +       mod_timer(&kbd->timer, jiffies + 1);
>
> Do not start timer/IRQs until there are users. Implement
> inptu_dev->open() abd ->close() and do it from there.

OK, that is right solution. I look at that.

> > +static int genmatrix_remove(struct device *dev)
> > +{
> > ..........
> > +       dev_set_drvdata(dev, NULL);
> > +
> > +       kfree(kbd->phys);
> > +       kfree(kbd->down_arr);
> > +       kfree(kbd->chng_arr);
> > +       kfree(kbd->keycode);
>
> I don't see you allocate kbd->keycode, why are you freeing it?

Somebody has to remove it. May it be, that free calls could/should
be distributed different way between genmatrix_plat_remove()
and genmatrix_remove().

> > +
> > +       kfree(kbd);
>
> Actually, why are you freeing kbd? If it was not allocated in probe(),
> it should not be freed in remove().

Somebody has to free it.  The genmatrix_remove() may be more
appropriate, but I would be happy to not teach that how pointer to
kbd is stored in dev hierarchy. But move could be right thing to do.

> The main advantage is that the code deals with noisy devices... How
> widespread are they? I think we should take another look at this once
> current implementation cleaned up.

My own experience is, that there does not exists any mechanical
switch/contact without danger of unexpected brushes and noises.
The question is only time duration. Even microswitches has
brushes. But they are short (at least for new devices).
If they are directly connected to the TTL speed rate logic
than single edge cannot be guaranteed anyway. Only really
safe is to use both contact (DIS and CON) and connect them
to R/S flip-flop. So there does not exists any mechanical
matrix keyboard without noise and when you do new design,
it is misleading, that it works for new devices better
and if you are not prepared for rapid degradation you
end up with multiple software updates. For example TI-57..59
calculators are unusable today due to degraded keyboards
to the level, that it is big mastery to make single press.

But on the other hand, if scan frequency is slow (100Hz or 10Hz)
situation gets better. Anyway I would not believe device
without reasonable noise suppression for any little more
critical application. And as I have said, mechanical contact
is vengeful beast, it seems to work in lab and breaks in
field. And if you do not want to invest into really
expensive construction brushes duration is relatively long.

Because I have not resolved all notices yet, I am  sending
only pointer to the slightly updated patch.

http://rtime.felk.cvut.cz/repos/ppisa-linux-devel/kernel-patches/current/genmatrix-kbd.patch

Best wishes

             Pavel

Re: [PATCH] Generic, platform independent matrix keyboard support

[David Brownell]

On Tuesday 09 October 2007, Pavel Pisa wrote:
> On the other hand I cannot imagine electronic designed wanting
> so many wires. Reduction of the wires counts is why matrix keyboards
> are used. The 32 bits limitation then means that maximal supported
> keyboard can be 32x32 key => 1024 keys, that is quite imposing.

Yes:  http://weblog.sinteur.com/?p=20412  :)

Re: [PATCH] Generic, platform independent matrix keyboard support

[Dmitry Torokhov]

On 10/9/07, David Brownell <david-b@pacbell.net> wrote:
> On Tuesday 09 October 2007, Pavel Pisa wrote:
> > On the other hand I cannot imagine electronic designed wanting
> > so many wires. Reduction of the wires counts is why matrix keyboards
> > are used. The 32 bits limitation then means that maximal supported
> > keyboard can be 32x32 key => 1024 keys, that is quite imposing.
>
> Yes:  http://weblog.sinteur.com/?p=20412  :)
>

Thank you David ;)

-- 
Dmitry

Re: [PATCH] Generic, platform independent matrix keyboard support

[Dmitry Torokhov]

On 10/9/07, Pavel Pisa <ppisa4lists@pikron.com> wrote:
>
> Name of corresponding platform and PCI methods is probe.
> But name can be changed if new one is seen as more logical.
>

There you indeed probing devices for supported functionality. But here
you just register a new object with the system (note that all such
methods have 'register' in them - device_register(),
input_register_device() , etc).

> > > +
> > > +       input_dev = input_allocate_device();
> > > +       if (!input_dev)
> > > +               goto fail_arr_alloc;
> > > +
> > > +       kbd->input = input_dev;
> > > +       kbd->hw_dev = dev;
> > > +       dev_set_drvdata(dev, kbd);
> >
> > Does not belong here.
>
> Where I can store pointer to specialized part.
>

I mean setting up kbd does not belong here. You set it up (by setting
hw_dev and  calling dev_set_drvdadat) before calling
genmatrix_register().

> > > +
> > > +       /* Setup input device */
> > > +       input_dev->name = "GenMatrix Keyboard";
> > > +       input_dev->phys = kbd->phys;
> > > +       input_dev->dev.parent = dev;
> >
> > Make this input_dev->dev.parent = kbd->dev; and get rid of dev argument.
>
> I am not fully sure what it would cause in device model hierarchy,
> but I try to learn and check that.
>

I meant input_dev->dev.parent = kbd->hw_dev;

> > > +
> > > +       input_dev->id.bustype = BUS_HOST;
> > > +       input_dev->id.vendor = 0x0001;
> > > +       input_dev->id.product = 0x0001;
> > > +       input_dev->id.version = 0x0100;
> > > +
> > > +       input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); /* BIT(EV_PWR) |
> > > BIT(EV_SW); */ +       input_dev->keycode = kbd->keycode;
> > > +       input_dev->keycodesize = sizeof(*kbd->keycode);
> > > +       input_dev->keycodemax = kbd->keycode_cnt;
> > > +
> > > +       for (i = 0; i < kbd->keycode_cnt; i++)
> > > +               set_bit(kbd->keycode[i], input_dev->keybit);
> > > +       clear_bit(0, input_dev->keybit);
> > > +       /*
> > > +       set_bit(SW_LID, input_dev->swbit);
> > > +       set_bit(SW_TABLET_MODE, input_dev->swbit);
> > > +       set_bit(SW_HEADPHONE_INSERT, input_dev->swbit);
> > > +       */
> > > +
> > > +       err = kbd->setup(kbd->hw_dev);
> > > +       if (err) {
> > > +               dev_err(kbd->hw_dev, "low-level hardware setup
> > > failed\n"); +               goto fail;
> > > +       }
> > > +
> > > +       err = input_register_device(input_dev);
> > > +       if (err) {
> > > +               dev_err(kbd->hw_dev, "input device registration\n");
> > > +               goto fail_to_register;
> > > +       }
> > > +
> > > +       mod_timer(&kbd->timer, jiffies + 1);
> >
> > Do not start timer/IRQs until there are users. Implement
> > inptu_dev->open() abd ->close() and do it from there.
>
> OK, that is right solution. I look at that.
>
> > > +static int genmatrix_remove(struct device *dev)
> > > +{
> > > ..........
> > > +       dev_set_drvdata(dev, NULL);
> > > +
> > > +       kfree(kbd->phys);
> > > +       kfree(kbd->down_arr);
> > > +       kfree(kbd->chng_arr);
> > > +       kfree(kbd->keycode);
> >
> > I don't see you allocate kbd->keycode, why are you freeing it?
>
> Somebody has to remove it. May it be, that free calls could/should
> be distributed different way between genmatrix_plat_remove()
> and genmatrix_remove().
>
> > > +
> > > +       kfree(kbd);
> >
> > Actually, why are you freeing kbd? If it was not allocated in probe(),
> > it should not be freed in remove().
>
> Somebody has to free it.  The genmatrix_remove() may be more
> appropriate, but I would be happy to not teach that how pointer to
> kbd is stored in dev hierarchy. But move could be right thing to do.
>

But kbd may be a part of a bigger structure and not allocated
separately. I prefer the following rule - if a layer did not allocate
a resource it should not try to free it.

-- 
Dmitry

Re: [PATCH] Generic, platform independent matrix keyboard support

[Pavel Pisa]

On Wednesday 10 October 2007 15:10, Dmitry Torokhov wrote:
> On 10/9/07, David Brownell <david-b@pacbell.net> wrote:
> > On Tuesday 09 October 2007, Pavel Pisa wrote:
> > > On the other hand I cannot imagine electronic designed wanting
> > > so many wires. Reduction of the wires counts is why matrix keyboards
> > > are used. The 32 bits limitation then means that maximal supported
> > > keyboard can be 32x32 key => 1024 keys, that is quite imposing.
> >
> > Yes:  http://weblog.sinteur.com/?p=20412  :)
>
> Thank you David ;)

OK, I am defeated for now.

I do not have time at this moment to rewrite code with unlimited
return lines number support. I even think, that finding decent interface
could be problematic. I have some ideas, but each has its own ugliness
in it. I am putting your other suggestion on my instantly growing
unfinished tasks queue. I hope, that I find some time before
2.6.25 merge window to sort that out. I believe, that the
code could be usesfull for other as well. 
I need to work on other more urgent things and actual
code version seems to be sufficient for me at this moment.

The IRQ cannot cause problem in my case, because I have edge
triggered sources, UP system and keyboard code generally work
and is designed to work such way, that only one of timer
and IRQ event is armed at one time instant so there should not
be a problem. But I am aware of and fully agree that this has
to be cleaned up before mainline inclusion. It could be solved
by spinlock_irq quickly too. I have that in my mind first but
then it looked as possible to implement it without this
requirement.

As for 32 lines limit, I think, that it is nonsense, but I agree,
that there could be simple-minded people for which could come to mind
to weave 100 x 100 matrix keyboard in single net. I have never seen
single PCI card for so many direct IOs, but I have seen assembly of many
boxes full of mechanical relays connected to multiport PLC to do
centralized control of big industrial systems spread on large area.
I would never do that. Even in keyboard case it would be much better
to use more building blocks with local MCU or FPGA and interconnect
them by I2C, USB, CAN or SPI. But because there exists people to whom such
weaving comes to mind, I try to think if I could find way to support
their strange preferences without big code usability and performance
degradation.

I would be much more happy, if such pressure for generality would
be pushed into directions where such universality makes more sense.
GPIO is an example. It would be great if there would be support to test
if pin can become input, output, has pull-up and or OC support
and if these options could be controlled portable way. These things
are common on most of todays chips (even on 8-bit MCUs) and
pull-ups are required to prevent extraordinary power consumption
caused by floating input pins. The restricted GPIO API is bigger
obstacle to build really portable matrix keyboard support
than 32 return lines limit from my point of view.
The other area is runtime registration and mapping of GPIO
ports, but I agree, that it is problematic to design well.

Best wishes

              Pavel Pisa

PS: I do not expect, that MS is so stupid to build single
    matrix but who knows. There could be even signal crossover
    noise problems caused by scanning soccer field size keyboards.

    

Re: [PATCH] Generic, platform independent matrix keyboard support

[David Brownell]

On Sunday 14 October 2007, Pavel Pisa wrote:
> I would be much more happy, if such pressure for generality would
> be pushed into directions where such universality makes more sense.
> GPIO is an example. It would be great if there would be support to test
> if pin can become input, output, has pull-up and or OC support
> and if these options could be controlled portable way.

Test if it can be an input or output by calling the function to
set the direction, and seeing if it fails.  Simple, portable.

Pullups/pulldowns are platform-specific pin mux issues, along
with configuring pins as GPIOs.  That is, it needs a separate
programming interface.  See the note in Documentation/gpio.txt
talking about the range of platform variation there ... that
isn't very amenable to a cross-platform interface.

I don't know what you mean by "OC support".  OverCurrent?
"Open Collector" would be TTL-specific, and TTL isn't used
much with VLSI.  "Obstetric Cholestasis" is what Google
suggested, but I rather doubt that's what you mean.  ;)


> These things 
> are common on most of todays chips (even on 8-bit MCUs) and
> pull-ups are required to prevent extraordinary power consumption
> caused by floating input pins.

True but irrelevant.  That is a *PIN* specific interface, and
the fact that some platforms can route a given GPIO to multiple
pins illustrates exactly why it needs a separate interface (not
a GPIO interface).  Plus, whether or not an on-chip pullup should
be used (vs external ones) is a *BOARD* specific decision.

- Dave