[PATCH 0/3] Add support for TCA6416 based Keypad driver.

[PATCH 0/3] Add support for TCA6416 based Keypad driver.

[Sriramakrishnan]

AM3517 EVM with APPS board includes keys interfaced to TCA6416 IO expander.
User keys are connected as GPIO lines to TCA6416 IO expander. Unlike the
case with generic gpio-keypad driver individual keys do not generate an
interrupt event. Hence we implement a simple keypad driver(based on gpio-keys)
that registers as direct I2C client.

The implementation has been tested on AM3517 EVM with the driver tested
in polling mode.

Sriramakrishnan (3):
  TCA6416 keypad : Implement keypad driver for keys interfaced to
    TCA6416
  AM3517 EVM : Enable TCA6416 keypad
  AM3517: Board hookup for TCA6416 keypad driver.

 arch/arm/configs/am3517_evm_defconfig   |   15 ++-
 arch/arm/mach-omap2/board-am3517evm.c   |   48 ++++-
 drivers/input/keyboard/Kconfig          |   17 ++
 drivers/input/keyboard/Makefile         |    1 +
 drivers/input/keyboard/tca6416-keypad.c |  355 +++++++++++++++++++++++++++++++
 include/linux/tca6416_keypad.h          |   30 +++
 6 files changed, 460 insertions(+), 6 deletions(-)
 create mode 100755 drivers/input/keyboard/tca6416-keypad.c
 create mode 100755 include/linux/tca6416_keypad.h

[PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Sriramakrishnan]

This patch implements a simple Keypad driver that functions
as an I2C client. It handles key press events for keys
connected to TCA6416 I2C based IO expander.

Signed-off-by: Sriramakrishnan <srk@ti.com>
---
 drivers/input/keyboard/Kconfig          |   17 ++
 drivers/input/keyboard/Makefile         |    1 +
 drivers/input/keyboard/tca6416-keypad.c |  355 +++++++++++++++++++++++++++++++
 include/linux/tca6416_keypad.h          |   30 +++
 4 files changed, 403 insertions(+), 0 deletions(-)
 create mode 100755 drivers/input/keyboard/tca6416-keypad.c
 create mode 100755 include/linux/tca6416_keypad.h

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 02c836e..c2cd31b 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -190,6 +190,23 @@ config KEYBOARD_GPIO
 	  To compile this driver as a module, choose M here: the
 	  module will be called gpio_keys.
 
+config KEYBOARD_TCA6416
+	tristate "TCA6416 Keypad Support"
+	depends on I2C
+	help
+	  This driver implements basic keypad functionality
+	  for keys connected through TCA6416 IO expander
+
+	  Say Y here if your device has keys connected to
+	  TCA6416 IO expander. Your board-specific setup logic
+	  must also provide pin-mask details(of which TCA6416 pins
+	  are used for keypad).
+
+	  If enabled the complete TCA6416 device will be managed through
+	  this driver(precludes gpio interface for remaining pins on
+	  TCA6416)
+
+
 config KEYBOARD_MATRIX
 	tristate "GPIO driven matrix keypad support"
 	depends on GENERIC_GPIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 78654ef..0f1ad54 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_KEYBOARD_CORGI)		+= corgikbd.o
 obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keyscan.o
 obj-$(CONFIG_KEYBOARD_EP93XX)		+= ep93xx_keypad.o
 obj-$(CONFIG_KEYBOARD_GPIO)		+= gpio_keys.o
+obj-$(CONFIG_KEYBOARD_TCA6416)		+= tca6416-keypad.o
 obj-$(CONFIG_KEYBOARD_HIL)		+= hil_kbd.o
 obj-$(CONFIG_KEYBOARD_HIL_OLD)		+= hilkbd.o
 obj-$(CONFIG_KEYBOARD_HP6XX)		+= jornada680_kbd.o
diff --git a/drivers/input/keyboard/tca6416-keypad.c b/drivers/input/keyboard/tca6416-keypad.c
new file mode 100755
index 0000000..4c1ba1e
--- /dev/null
+++ b/drivers/input/keyboard/tca6416-keypad.c
@@ -0,0 +1,355 @@
+/*
+ * Driver for keys on TCA6416 I2C IO expander
+ *
+ * Implementation based on drivers/input/keyboard/gpio_keys.c
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Author : Sriramakrishnan.A.G. <srk@ti.com>
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/tca6416_keypad.h>
+#include <linux/workqueue.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+
+#define TCA6416_INPUT          0
+#define TCA6416_OUTPUT         1
+#define TCA6416_INVERT         2
+#define TCA6416_DIRECTION      3
+
+static const struct i2c_device_id tca6416_id[] = {
+	{ "tca6416-keys", 16, },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tca6416_id);
+
+struct tca6416_button_data {
+	struct gpio_keys_button *button;
+	struct input_dev *input;
+};
+
+struct tca6416_drv_data {
+	struct input_dev *input;
+	struct tca6416_button_data data[0];
+};
+
+struct tca6416_keypad_chip {
+	uint16_t reg_output;
+	uint16_t reg_direction;
+	uint16_t reg_input;
+
+	struct i2c_client *client;
+	struct tca6416_drv_data  *drv_data;
+	struct delayed_work dwork;
+	uint16_t pinmask;
+	int irqnum;
+	int use_polling;
+};
+
+static int tca6416_write_reg(struct tca6416_keypad_chip *chip, int reg,
+				uint16_t val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_word_data(chip->client, reg << 1, val);
+
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "failed writing register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tca6416_read_reg(struct tca6416_keypad_chip *chip, int reg,
+				uint16_t *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_data(chip->client, reg << 1);
+
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "failed reading register\n");
+		return ret;
+	}
+
+	*val = (uint16_t)ret;
+	return 0;
+}
+
+static irqreturn_t tca6416_keys_isr(int irq, void *dev_id)
+{
+	struct tca6416_keypad_chip *chip =
+		(struct tca6416_keypad_chip *) dev_id;
+
+	disable_irq(irq);
+	schedule_delayed_work(&chip->dwork, 0);
+	return IRQ_HANDLED;
+
+}
+
+static void tca6416_keys_work_func(struct work_struct *workstruct)
+{
+	struct delayed_work *delay_work =
+		container_of(workstruct, struct delayed_work, work);
+	struct tca6416_keypad_chip *chip =
+		container_of(delay_work, struct tca6416_keypad_chip, dwork);
+	struct tca6416_drv_data *ddata = chip->drv_data;
+	uint16_t reg_val, val;
+	int ret, i, pin_index;
+
+	ret = tca6416_read_reg(chip, TCA6416_INPUT, &reg_val);
+	if (ret)
+		return;
+
+	reg_val &= chip->pinmask;
+
+	/* Figure out which lines have changed */
+	val = reg_val ^ (chip->reg_input);
+	chip->reg_input = reg_val;
+
+	for (i = 0, pin_index = 0; i < 16; i++) {
+		if (val & (1 << i)) {
+			struct tca6416_button_data *tca_button =
+				&ddata->data[pin_index];
+			struct gpio_keys_button *button = tca_button->button;
+			struct input_dev *input = tca_button->input;
+			unsigned int type = button->type ?: EV_KEY;
+			int state = ((reg_val & (1 << i)) ? 1 : 0)
+						^ button->active_low;
+
+			input_event(input, type, button->code, !!state);
+			input_sync(input);
+		}
+
+		if (chip->pinmask & (1 << i))
+			pin_index++;
+	}
+
+	if (chip->use_polling)
+		schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
+	else
+		enable_irq(chip->irqnum);
+
+}
+
+
+static int __devinit tca6416_keypad_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
+{
+	struct tca6416_keys_platform_data *pdata;
+	struct tca6416_keypad_chip *chip;
+	struct tca6416_drv_data *ddata;
+	struct input_dev *input;
+	int i, ret, pin_index;
+	uint16_t reg_val;
+
+	chip = kzalloc(sizeof(struct tca6416_keypad_chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+
+	pdata = client->dev.platform_data;
+	if (pdata == NULL) {
+		dev_dbg(&client->dev, "no platform data\n");
+		ret = -EINVAL;
+		goto fail1;
+	}
+
+	chip->client = client;
+	chip->pinmask = pdata->pinmask;
+
+	/* initialize cached registers from their original values.
+	 * we can't share this chip with another i2c master.
+	 */
+	ret = tca6416_read_reg(chip, TCA6416_OUTPUT, &chip->reg_output);
+	if (ret)
+		goto fail1;
+
+	ret = tca6416_read_reg(chip, TCA6416_DIRECTION, &chip->reg_direction);
+	if (ret)
+		goto fail1;
+
+	/* ensure that keypad pins are set to input */
+	reg_val = chip->reg_direction | chip->pinmask;
+	ret = tca6416_write_reg(chip, TCA6416_DIRECTION, reg_val);
+	if (ret)
+		goto fail1;
+
+	ret = tca6416_read_reg(chip, TCA6416_DIRECTION, &chip->reg_direction);
+	if (ret)
+		goto fail1;
+
+	ret = tca6416_read_reg(chip, TCA6416_INPUT, &chip->reg_input);
+	if (ret)
+		goto fail1;
+
+	i2c_set_clientdata(client, chip);
+
+
+	ddata = kzalloc(sizeof(struct tca6416_drv_data) +
+			pdata->nbuttons * sizeof(struct tca6416_button_data),
+			GFP_KERNEL);
+	if (!ddata) {
+		ret = -ENOMEM;
+		goto fail1;
+	}
+
+	input = input_allocate_device();
+	if (!input) {
+		dev_dbg(&client->dev, "failed to allocate state\n");
+		ret = -ENOMEM;
+		kfree(ddata);
+		goto fail2;
+	}
+
+	input->phys = "tca6416-keys/input0";
+	input->dev.parent = &client->dev;
+
+	input->id.bustype = BUS_HOST;
+	input->id.vendor = 0x0001;
+	input->id.product = 0x0001;
+	input->id.version = 0x0100;
+
+	/* Enable auto repeat feature of Linux input subsystem */
+	if (pdata->rep)
+		__set_bit(EV_REP, input->evbit);
+
+	ddata->input = input;
+
+	for (i = 0; i < pdata->nbuttons; i++) {
+		struct gpio_keys_button *button = &pdata->buttons[i];
+		struct tca6416_button_data *bdata = &ddata->data[i];
+		unsigned int type = button->type ?: EV_KEY;
+
+		bdata->input = input;
+		bdata->button = button;
+
+		input_set_capability(input, type, button->code);
+	}
+
+	chip->drv_data = ddata;
+	chip->use_polling = pdata->use_polling;
+
+	INIT_DELAYED_WORK(&chip->dwork, tca6416_keys_work_func);
+
+	if (!chip->use_polling) {
+		if (pdata->irq_is_gpio)
+			chip->irqnum = gpio_to_irq(pdata->irqnum);
+		else
+			chip->irqnum = pdata->irqnum;
+
+		ret = request_irq(chip->irqnum, tca6416_keys_isr,
+				IRQF_SHARED | IRQF_TRIGGER_FALLING ,
+				"tca6416-keypad", chip);
+		if (ret) {
+			dev_dbg(&client->dev,
+				"Unable to claim irq %d; error %d\n",
+				chip->irqnum, ret);
+			goto fail3;
+		}
+		disable_irq(chip->irqnum);
+	}
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_dbg(&client->dev, "Unable to register input device, "
+			"error: %d\n", ret);
+		goto fail3;
+	}
+
+	/* get current state of buttons */
+
+	ret = tca6416_read_reg(chip, TCA6416_INPUT, &reg_val);
+	if (ret)
+		goto fail4;
+
+	chip->reg_input = reg_val & chip->pinmask;
+
+	for (i = 0, pin_index = 0; i < 16; i++) {
+		if (chip->pinmask & (1 << i)) {
+			struct tca6416_button_data *tca_button =
+					&ddata->data[pin_index];
+			struct gpio_keys_button *button = tca_button->button;
+			struct input_dev *input = tca_button->input;
+			unsigned int type = button->type ?: EV_KEY;
+			int state = ((reg_val & (1 << i)) ? 1 : 0)
+						^ button->active_low;
+
+			input_event(input, type, button->code, !!state);
+			input_sync(input);
+			pin_index++;
+		}
+	}
+	input_sync(input);
+
+	if (chip->use_polling)
+		schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
+	else
+		enable_irq(chip->irqnum);
+
+	return 0;
+
+fail4:
+	input_unregister_device(input);
+fail3:
+	input_free_device(input);
+fail2:
+	kfree(ddata);
+fail1:
+	kfree(chip);
+	return ret;
+}
+
+static int tca6416_keypad_remove(struct i2c_client *client)
+{
+	struct tca6416_keypad_chip *chip = i2c_get_clientdata(client);
+	struct tca6416_drv_data *ddata = chip->drv_data;
+	struct input_dev *input = ddata->input;
+
+	if (!chip->use_polling)
+		free_irq(chip->irqnum, chip);
+	cancel_delayed_work_sync(&chip->dwork);
+	input_unregister_device(input);
+	input_free_device(input);
+	kfree(ddata);
+	kfree(chip);
+	return 0;
+}
+
+
+static struct i2c_driver tca6416_keypad_driver = {
+	.driver = {
+		.name	= "tca6416-keypad",
+	},
+	.probe		= tca6416_keypad_probe,
+	.remove		= tca6416_keypad_remove,
+	.id_table	= tca6416_id,
+};
+
+static int __init tca6416_keypad_init(void)
+{
+	return i2c_add_driver(&tca6416_keypad_driver);
+}
+
+subsys_initcall(tca6416_keypad_init);
+
+static void __exit tca6416_keypad_exit(void)
+{
+	i2c_del_driver(&tca6416_keypad_driver);
+}
+module_exit(tca6416_keypad_exit);
+
+MODULE_AUTHOR("Sriramakrishnan <srk@ti.com>");
+MODULE_DESCRIPTION("Keypad driver over tca6146 IO expander");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/tca6416_keypad.h b/include/linux/tca6416_keypad.h
new file mode 100755
index 0000000..e996176
--- /dev/null
+++ b/include/linux/tca6416_keypad.h
@@ -0,0 +1,30 @@
+/*
+ * tca6416 keypad platform support
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Author: Sriramakrishnan <srk@ti.com>
+ *
+ * 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.
+ */
+
+#ifndef _TCA6416_KEYS_H
+#define _TCA6416_KEYS_H
+
+#include <linux/gpio_keys.h>
+#include <linux/types.h>
+
+struct tca6416_keys_platform_data {
+	struct gpio_keys_button *buttons;
+	int nbuttons;
+	unsigned int rep:1;	/* enable input subsystem auto repeat */
+	uint16_t pinmask;
+	uint16_t invert;
+	int irqnum;
+	int irq_is_gpio;
+	int use_polling;	/* use polling if Interrupt is not connected*/
+};
+
+#endif
-- 
1.6.2.4

[PATCH 2/3] AM3517 EVM : Enable TCA6416 keypad

[Sriramakrishnan]

Update kernel configuration for AM3517EVM to include
support for TCA6416 keypad.

Signed-off-by: Sriramakrishnan <srk@ti.com>
---
 arch/arm/configs/am3517_evm_defconfig |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/arm/configs/am3517_evm_defconfig b/arch/arm/configs/am3517_evm_defconfig
index 04a804b..439ba7f 100644
--- a/arch/arm/configs/am3517_evm_defconfig
+++ b/arch/arm/configs/am3517_evm_defconfig
@@ -539,7 +539,20 @@ CONFIG_INPUT_EVDEV=y
 #
 # Input Device Drivers
 #
-# CONFIG_INPUT_KEYBOARD is not set
+CONFIG_INPUT_KEYBOARD=y
+# CONFIG_KEYBOARD_ADP5588 is not set
+CONFIG_KEYBOARD_ATKBD=y
+# CONFIG_QT2160 is not set
+# CONFIG_KEYBOARD_LKKBD is not set
+# CONFIG_KEYBOARD_GPIO is not set
+CONFIG_KEYBOARD_TCA6416=y
+# CONFIG_KEYBOARD_MATRIX is not set
+# CONFIG_KEYBOARD_MAX7359 is not set
+# CONFIG_KEYBOARD_NEWTON is not set
+# CONFIG_KEYBOARD_OPENCORES is not set
+# CONFIG_KEYBOARD_STOWAWAY is not set
+# CONFIG_KEYBOARD_SUNKBD is not set
+# CONFIG_KEYBOARD_XTKBD is not set
 # CONFIG_INPUT_MOUSE is not set
 # CONFIG_INPUT_JOYSTICK is not set
 # CONFIG_INPUT_TABLET is not set
-- 
1.6.2.4

[PATCH 3/3] AM3517: Board hookup for TCA6416 keypad driver.

[Sriramakrishnan]

Add board specific hookup for TCA6416 keypad driver.

Signed-off-by: Sriramakrishnan <srk@ti.com>
---
This patch depends on "AM3517EVM : correct typo - tca6416 mispelt as tca6516"
patch submitted earlier.
[1] http://marc.info/?l=linux-omap&m=126709475619102&w=2


 arch/arm/mach-omap2/board-am3517evm.c |   48 +++++++++++++++++++++++++++++---
 1 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/board-am3517evm.c b/arch/arm/mach-omap2/board-am3517evm.c
index b336adc..a8228b7 100644
--- a/arch/arm/mach-omap2/board-am3517evm.c
+++ b/arch/arm/mach-omap2/board-am3517evm.c
@@ -20,7 +20,10 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
+#include <linux/i2c.h>
 #include <linux/i2c/pca953x.h>
+#include <linux/input.h>
+#include <linux/tca6416_keypad.h>
 
 #include <mach/hardware.h>
 #include <mach/am35xx.h>
@@ -88,16 +91,51 @@ static struct i2c_board_info __initdata am3517evm_tca6416_info_0[] = {
 };
 
 /* Mounted on UI Card */
-static struct pca953x_platform_data am3517evm_ui_gpio_expander_info_1 = {
+/* IO expander at address 0x20 on UI card will be managed by Keypad driver */
+
+static struct pca953x_platform_data am3517evm_ui_gpio_expander_info_2 = {
 	.gpio_base	= OMAP_MAX_GPIO_LINES + 16,
 };
-static struct pca953x_platform_data am3517evm_ui_gpio_expander_info_2 = {
-	.gpio_base	= OMAP_MAX_GPIO_LINES + 32,
+
+/*Keypad Initialization */
+#define KEYPAD_PIN_MASK                0xFFC0
+
+#define KEYPAD_BUTTON(ev_type, ev_code, act_low, descr) \
+{                                                               \
+	.type           = ev_type,                              \
+	.code           = ev_code,                              \
+	.active_low     = act_low,                              \
+	.desc           = "btn " descr,                         \
+}
+
+#define KEYPAD_BUTTON_LOW(event_code, description)      \
+	KEYPAD_BUTTON(EV_KEY, event_code, 1, description)
+
+static struct gpio_keys_button am3517_gpio_keys[] = {
+	KEYPAD_BUTTON_LOW(KEY_DOWN, "down"),
+	KEYPAD_BUTTON_LOW(KEY_UP, "up"),
+	KEYPAD_BUTTON_LOW(KEY_MENU, "menu"),
+	KEYPAD_BUTTON_LOW(KEY_MODE, "mode"),
+	KEYPAD_BUTTON_LOW(KEY_LEFTSHIFT, "shift"),
+	KEYPAD_BUTTON_LOW(KEY_REWIND, "rewind"),
+	KEYPAD_BUTTON_LOW(KEY_FORWARD, "forward"),
+	KEYPAD_BUTTON_LOW(KEY_STOP, "stop"),
+	KEYPAD_BUTTON_LOW(KEY_PLAY, "play"),
+	KEYPAD_BUTTON_LOW(KEY_RECORD, "rec"),
 };
+
+static struct tca6416_keys_platform_data am3517evm_tca6416_keys_info = {
+	.buttons        = am3517_gpio_keys,
+	.nbuttons       = ARRAY_SIZE(am3517_gpio_keys),
+	.rep            = 0,
+	.use_polling    = 1,
+	.pinmask        = KEYPAD_PIN_MASK,
+};
+
 static struct i2c_board_info __initdata am3517evm_ui_tca6416_info[] = {
 	{
-		I2C_BOARD_INFO("tca6416", 0x20),
-		.platform_data = &am3517evm_ui_gpio_expander_info_1,
+		I2C_BOARD_INFO("tca6416-keys", 0x20),
+		.platform_data = &am3517evm_tca6416_keys_info,
 	},
 	{
 		I2C_BOARD_INFO("tca6416", 0x21),
-- 
1.6.2.4

Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Felipe Balbi]

Hi,

On Thu, Feb 25, 2010 at 06:44:59PM +0530, Sriramakrishnan wrote:
> This patch implements a simple Keypad driver that functions
> as an I2C client. It handles key press events for keys
> connected to TCA6416 I2C based IO expander.

what's wrong with gpio-keys ??

> + * Implementation based on drivers/input/keyboard/gpio_keys.c

I see,

shouldn't you instead provide a gpiolib driver for tca6416 and use the
generic gpio_keys driver ??

> +	if (!chip->use_polling) {

IMO, you should only use polling if the irq line isn't connected.

> +		if (pdata->irq_is_gpio)
> +			chip->irqnum = gpio_to_irq(pdata->irqnum);

you can pass the irq number via i2c_board_info. Use it.

> +		else
> +			chip->irqnum = pdata->irqnum;
> +
> +		ret = request_irq(chip->irqnum, tca6416_keys_isr,

it's an i2c driver!!! this should be request_threaded_irq()

-- 
balbi

Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Dmitry Torokhov]

On Thu, Feb 25, 2010 at 08:47:03PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Feb 25, 2010 at 06:44:59PM +0530, Sriramakrishnan wrote:
> > This patch implements a simple Keypad driver that functions
> > as an I2C client. It handles key press events for keys
> > connected to TCA6416 I2C based IO expander.
> 
> what's wrong with gpio-keys ??
> 
> > + * Implementation based on drivers/input/keyboard/gpio_keys.c
> 
> I see,
> 
> shouldn't you instead provide a gpiolib driver for tca6416 and use the
> generic gpio_keys driver ??
> 

Right. The fact that the driver precludes all otehr gpios from being
used is a major drawback.


> > +	if (!chip->use_polling) {
> 
> IMO, you should only use polling if the irq line isn't connected.
> 
> > +		if (pdata->irq_is_gpio)
> > +			chip->irqnum = gpio_to_irq(pdata->irqnum);
> 
> you can pass the irq number via i2c_board_info. Use it.
> 
> > +		else
> > +			chip->irqnum = pdata->irqnum;
> > +
> > +		ret = request_irq(chip->irqnum, tca6416_keys_isr,
> 
> it's an i2c driver!!! this should be request_threaded_irq()
> 

Threaded IRQ probably does not fit well when you want to support both
interrupt and polling in the same driver...

-- 
Dmitry

RE: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Govindarajan, Sriramakrishnan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Friday, February 26, 2010 1:58 PM
> To: Felipe Balbi
> Cc: Govindarajan, Sriramakrishnan; linux-omap@vger.kernel.org; linux-
> input@vger.kernel.org
> Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys
> interfaced to TCA6416
> 
> On Thu, Feb 25, 2010 at 08:47:03PM +0200, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Feb 25, 2010 at 06:44:59PM +0530, Sriramakrishnan wrote:
> > > This patch implements a simple Keypad driver that functions
> > > as an I2C client. It handles key press events for keys
> > > connected to TCA6416 I2C based IO expander.
> >
> > what's wrong with gpio-keys ??
> >
> > > + * Implementation based on drivers/input/keyboard/gpio_keys.c
> >
> > I see,
> >
> > shouldn't you instead provide a gpiolib driver for tca6416 and use the
> > generic gpio_keys driver ??
> >
> 
> Right. The fact that the driver precludes all otehr gpios from being
> used is a major drawback.
[Sriram] gpio_keys implementation assumes that every key can generate an interrupt to the processor and also the state machine to process events will handle each line individually. Imagine if 16 keys are connected to the TCA controller and you have to query(assume it is polling, interrupt mode not possible - as you have single interrupt line to the processor for event on any of the 16 input lines) 16 lines individually over i2c bus - the associated overhead is too high. Instead this implementation handles the necessary book-keeping in one simple function (get status of all 16 lines in one read transaction). Building on a GPIO implementation and using gpio-keys above will be both clumsy and incur runtime overhead as against a direct keypad driver.

More often(as on AM3517EVM) the keypad keys are not mixed with other GPIO keys. The initial implementation includes key_mask to identify the keys to be used for keypad and the others are logically not modified. Hence the implementation can be extended if need arises to overcome this restriction.
> 
> 
> > > +	if (!chip->use_polling) {
> >
> > IMO, you should only use polling if the irq line isn't connected.
> >
> > > +		if (pdata->irq_is_gpio)
> > > +			chip->irqnum = gpio_to_irq(pdata->irqnum);
> >
> > you can pass the irq number via i2c_board_info. Use it.
> >
> > > +		else
> > > +			chip->irqnum = pdata->irqnum;
> > > +
> > > +		ret = request_irq(chip->irqnum, tca6416_keys_isr,
> >
> > it's an i2c driver!!! this should be request_threaded_irq()
> >
> 
> Threaded IRQ probably does not fit well when you want to support both
> interrupt and polling in the same driver...
[Sriram] All the core logic (including I2C transactions) is implemented through a work queue implementation. The ISR is slim and only schedules the work queue. This also enables the implementation to be easily adaptable for both interrupt/polled mode.

Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Dmitry Torokhov]

On Fri, Feb 26, 2010 at 08:13:28PM +0530, Govindarajan, Sriramakrishnan wrote:
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Friday, February 26, 2010 1:58 PM
> > To: Felipe Balbi
> > Cc: Govindarajan, Sriramakrishnan; linux-omap@vger.kernel.org; linux-
> > input@vger.kernel.org
> > Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys
> > interfaced to TCA6416
> > 
> > On Thu, Feb 25, 2010 at 08:47:03PM +0200, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Thu, Feb 25, 2010 at 06:44:59PM +0530, Sriramakrishnan wrote:
> > > > This patch implements a simple Keypad driver that functions
> > > > as an I2C client. It handles key press events for keys
> > > > connected to TCA6416 I2C based IO expander.
> > >
> > > what's wrong with gpio-keys ??
> > >
> > > > + * Implementation based on drivers/input/keyboard/gpio_keys.c
> > >
> > > I see,
> > >
> > > shouldn't you instead provide a gpiolib driver for tca6416 and use the
> > > generic gpio_keys driver ??
> > >
> > 
> > Right. The fact that the driver precludes all otehr gpios from being
> > used is a major drawback.

> [Sriram] gpio_keys implementation assumes that every key can generate
> an interrupt to the processor and also the state machine to process
> events will handle each line individually. Imagine if 16 keys are
> connected to the TCA controller and you have to query(assume it is
> polling, interrupt mode not possible - as you have single interrupt
> line to the processor for event on any of the 16 input lines) 16 lines
> individually over i2c bus - the associated overhead is too high.
> Instead this implementation handles the necessary book-keeping in one
> simple function (get status of all 16 lines in one read transaction).
> Building on a GPIO implementation and using gpio-keys above will be
> both clumsy and incur runtime overhead as against a direct keypad
> driver.
> 
> More often(as on AM3517EVM) the keypad keys are not mixed with other
> GPIO keys. The initial implementation includes key_mask to identify
> the keys to be used for keypad and the others are logically not
> modified. Hence the implementation can be extended if need arises to
> overcome this restriction.

In this case you should not build on gpio_keys at all but select some
other keyboard as an example that smply uses keytable with set of
keycodes, single interrupt, etc. The overhead of all gpio_keys data
structures is not needed here.

> > 
> > 
> > > > +	if (!chip->use_polling) {
> > >
> > > IMO, you should only use polling if the irq line isn't connected.
> > >
> > > > +		if (pdata->irq_is_gpio)
> > > > +			chip->irqnum = gpio_to_irq(pdata->irqnum);
> > >
> > > you can pass the irq number via i2c_board_info. Use it.
> > >
> > > > +		else
> > > > +			chip->irqnum = pdata->irqnum;
> > > > +
> > > > +		ret = request_irq(chip->irqnum, tca6416_keys_isr,
> > >
> > > it's an i2c driver!!! this should be request_threaded_irq()
> > >
> > 
> > Threaded IRQ probably does not fit well when you want to support both
> > interrupt and polling in the same driver...

> [Sriram] All the core logic (including I2C transactions) is
> implemented through a work queue implementation. The ISR is slim and
> only schedules the work queue. This also enables the implementation to
> be easily adaptable for both interrupt/polled mode.
> 

Fair enough but you need to ensure that you do not leave irq unbalances
(in regards to enable/disable) when you using workqueue and disabling
interrupt in the ISR.

-- 
Dmitry

Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Anirudh Ghayal]

Hi,

> +
> +       chip = kzalloc(sizeof(struct tca6416_keypad_chip), GFP_KERNEL);
> +       if (chip == NULL)
> +               return -ENOMEM;

Check for i2c functionality support ?

> +               ret = request_irq(chip->irqnum, tca6416_keys_isr,
> +                               IRQF_SHARED | IRQF_TRIGGER_FALLING ,
> +                               "tca6416-keypad", chip);
> +               if (ret) {
> +                       dev_dbg(&client->dev,
> +                               "Unable to claim irq %d; error %d\n",
> +                               chip->irqnum, ret);
> +                       goto fail3;
> +               }
> +               disable_irq(chip->irqnum);

The error path needs to free_irq() in case of any failure after this,
for an interrupt based driver.

Thanks,
Anirudh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

[Govindarajan, Sriramakrishnan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Saturday, February 27, 2010 2:30 PM
> To: Govindarajan, Sriramakrishnan
> Cc: Felipe Balbi; linux-omap@vger.kernel.org; linux-input@vger.kernel.org
> Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys
> interfaced to TCA6416

---snip---
> > > > I see,
> > > >
> > > > shouldn't you instead provide a gpiolib driver for tca6416 and use
> the
> > > > generic gpio_keys driver ??
> > > >
> > >
> > > Right. The fact that the driver precludes all otehr gpios from being
> > > used is a major drawback.
> 
> > [Sriram] gpio_keys implementation assumes that every key can generate
> > an interrupt to the processor and also the state machine to process
> > events will handle each line individually. Imagine if 16 keys are
> > connected to the TCA controller and you have to query(assume it is
> > polling, interrupt mode not possible - as you have single interrupt
> > line to the processor for event on any of the 16 input lines) 16 lines
> > individually over i2c bus - the associated overhead is too high.
> > Instead this implementation handles the necessary book-keeping in one
> > simple function (get status of all 16 lines in one read transaction).
> > Building on a GPIO implementation and using gpio-keys above will be
> > both clumsy and incur runtime overhead as against a direct keypad
> > driver.
> >
> > More often(as on AM3517EVM) the keypad keys are not mixed with other
> > GPIO keys. The initial implementation includes key_mask to identify
> > the keys to be used for keypad and the others are logically not
> > modified. Hence the implementation can be extended if need arises to
> > overcome this restriction.
> 
> In this case you should not build on gpio_keys at all but select some
> other keyboard as an example that smply uses keytable with set of
> keycodes, single interrupt, etc. The overhead of all gpio_keys data
> structures is not needed here.
> 
[Sriram] I am working on cleaning up the driver to use a simpler book keeping structure(move away from gpio_keys). Will post the reworked patch shortly.

---snip ---
> 
> > [Sriram] All the core logic (including I2C transactions) is
> > implemented through a work queue implementation. The ISR is slim and
> > only schedules the work queue. This also enables the implementation to
> > be easily adaptable for both interrupt/polled mode.
> >
> 
> Fair enough but you need to ensure that you do not leave irq unbalances
> (in regards to enable/disable) when you using workqueue and disabling
> interrupt in the ISR.

[Sriram] Will ensure that this is addressed appropriately (including cleanup on error conditions).

---
Sriram