2010-11-16 19:47:59

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v2 0/4] da850-evm: add gpio-{keys,leds} for UI and BB expanders

The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
They are bootstrapped to different I2C addresses so they can be used
concurrently.

The expander on the UI board is currently used to enable/disable the
peripherals that are available on the UI board. In addition to this
functionality the expander is also connected to 8 pushbuttons. The expander
on the baseboard is not currently used; it is connected to deep sleep enable,
sw reset, a push button, some switches and LEDs.

This proposed patch series enables the push buttons and switches on the UI and
BB expanders using the gpio-keys polling mode patch by Alexander Clouter. Some
work was performed to test irq-based gpio-keys support on the expanders (a WIP
patch can be posted on request) but I believe that it is not possible to use
irq-based gpio-keys on IO expanders for arm systems at this time.

The attempt started when I noticed the patch of Alek Du and Alan Cox [1] which
was recently committed [2]; a stab at integrating irq-based gpio-keys support
based on that patch was attempted. I found that I either got a warning that the
irq could not be mapped for the given gpio ; or, when N_IRQ was increased, a
system freeze.

>From what I have read (particularly the message by Grant Likely [3]) IRQs on
IO expanders are not ready in ARM yet. I _think_ that the sparse IRQ rework by
Thomas Gleixner [4] will resolve the blocker to irq-based gpio-keys support.

In the meantime we have buttons and switches that we would like to excersise
in our prototyping development. The patch to convert this series to irq-based
gpio-keys will be straighforward once the support in arch/arm is there.

There is an existing tca6416-keypad driver with polling support which I did not
employ because it isn't possible to keep the gpio's used for peripheral
enable/disable on the UI board or the LEDs on the baseboard registered while
simultaneously registering the pushbuttons or switches as a tca6416-keypad
instance.

I tested this patch series using evtest on the resulting /dev/input/eventN
devices and also on the event node of a non-polling gpio-keys instance to
ensure that irq-based input handling is not broken by the introduction of the
polling-mode gpio-keys patch. The non-polling instance creation and
registration is not included in this series since it uses one of the boot-mode
DIP switches and woult not (I think) be suitable for mainline.

Disclaimer:
I'm not an expert in irq's or gpio-keys; this is, in fact, my first proposed
feature. Please feel free to correct me -- I welcome the chance to learn from
your expertise.

[1] http://article.gmane.org/gmane.linux.kernel/1052551
[2] http://article.gmane.org/gmane.linux.kernel.commits.head/260919
[3] http://www.mail-archive.com/[email protected]/msg01974.html
[4] http://article.gmane.org/gmane.linux.kernel.cross-arch/7786

Alexander Clouter (1):
input: gpio_keys: polling mode support

Ben Gardiner (3):
da850-evm: add UI Expander pushbuttons
da850-evm: extract defines for SEL{A,B,C} pins in UI expander
da850-evm: add baseboard UI expander buttons, switches and LEDs

arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 312 +++++++++++++++++++++++++++++--
drivers/input/keyboard/gpio_keys.c | 120 ++++++++++--
include/linux/gpio_keys.h | 1 +
4 files changed, 406 insertions(+), 30 deletions(-)

---

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used


2010-11-16 19:39:44

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v2 1/4] input: gpio_keys: polling mode support

From: Alexander Clouter <[email protected]>

This implements an optional polling mode for the gpio_keys driver,
necessary for GPIOs that are not able to generate IRQs.

gpio_keys_platform_data has been extended with poll_interval
which specifies the polling interval in ms, this is passed onto
input-polldev.

This work is a rebase of the patch by Alex Clouter [1] which was
based on the patch [2] originally conceived by Paul Mundt.

Signed-off-by: Paul Mundt <[email protected]>
Signed-off-by: Alexander Clouter <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Paul Mundt <[email protected]>

[1] http://article.gmane.org/gmane.linux.kernel.input/13919
[2] http://article.gmane.org/gmane.linux.kernel.input/5814

---

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin

Changes since [1]:
* rebased to 0b1c3ef1072f2b97c86351d3736d2b2d00293a11 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* use _cansleep variant of gpio_get_value in the polling task
to avoid WARN_ON when using I2C GPIO expanders
* prevent unitialized access to 'input' in gpio_keys_close()
Changes since [2]:
* absolute dependency on INPUT_POLLDEV removed

Tested with CONFIG_INPUT_POLLDEV={n,m,y} (gpio_keys as module for 'm').
---
drivers/input/keyboard/gpio_keys.c | 120 ++++++++++++++++++++++++++++++------
include/linux/gpio_keys.h | 1 +
2 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6069abe..d2f23d9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -1,7 +1,9 @@
/*
- * Driver for keys on GPIO lines capable of generating interrupts.
+ * Driver for keys on GPIO lines, either IRQ-driven or polled.
*
* Copyright 2005 Phil Blundell
+ * Copyright 2008 Paul Mundt
+ * Copyright 2010 Alexander Clouter <[email protected]>
*
* 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
@@ -25,6 +27,7 @@
#include <linux/gpio_keys.h>
#include <linux/workqueue.h>
#include <linux/gpio.h>
+#include <linux/input-polldev.h>

struct gpio_button_data {
struct gpio_keys_button *button;
@@ -37,6 +40,7 @@ struct gpio_button_data {

struct gpio_keys_drvdata {
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
struct mutex disable_lock;
unsigned int n_buttons;
int (*enable)(struct device *dev);
@@ -44,6 +48,30 @@ struct gpio_keys_drvdata {
struct gpio_button_data data[0];
};

+#if (!defined(CONFIG_INPUT_POLLDEV) && !defined(CONFIG_INPUT_POLLDEV_MODULE)) \
+ || (defined(CONFIG_INPUT_POLLDEV_MODULE) \
+ && !defined(CONFIG_KEYBOARD_GPIO_MODULE))
+
+static inline struct input_polled_dev *allocate_polled_device(
+ const struct device *dev)
+{
+ dev_err(dev, "device needs polling (enable INPUT_POLLDEV)\n");
+ return NULL;
+}
+
+#define free_polled_device(x) do { } while (0)
+#define register_polled_device(x) (-ENXIO)
+#define unregister_polled_device(x) do { } while (0)
+
+#else
+
+#define allocate_polled_device(x) input_allocate_polled_device()
+#define free_polled_device(x) input_free_polled_device(x)
+#define register_polled_device(x) input_register_polled_device(x)
+#define unregister_polled_device(x) input_unregister_polled_device(x)
+
+#endif
+
/*
* SYSFS interface for enabling/disabling keys and switches:
*
@@ -322,7 +350,8 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
struct gpio_keys_button *button = bdata->button;
struct input_dev *input = bdata->input;
unsigned int type = button->type ?: EV_KEY;
- int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
+ int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0)
+ ^ button->active_low;

input_event(input, type, button->code, !!state);
input_sync(input);
@@ -343,6 +372,16 @@ static void gpio_keys_timer(unsigned long _data)
schedule_work(&data->work);
}

+static void gpio_handle_button_event(struct gpio_keys_button *button,
+ struct gpio_button_data *bdata)
+{
+ if (bdata->timer_debounce)
+ mod_timer(&bdata->timer,
+ jiffies + msecs_to_jiffies(bdata->timer_debounce));
+ else
+ gpio_keys_report_event(bdata);
+}
+
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
@@ -350,15 +389,24 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)

BUG_ON(irq != gpio_to_irq(button->gpio));

- if (bdata->timer_debounce)
- mod_timer(&bdata->timer,
- jiffies + msecs_to_jiffies(bdata->timer_debounce));
- else
- schedule_work(&bdata->work);
+ gpio_handle_button_event(button, bdata);

return IRQ_HANDLED;
}

+static void gpio_keys_poll(struct input_polled_dev *dev)
+{
+ struct gpio_keys_drvdata *ddata = dev->private;
+ int i;
+
+ for (i = 0; i < ddata->n_buttons; i++) {
+ struct gpio_button_data *bdata = &ddata->data[i];
+ struct gpio_keys_button *button = bdata->button;
+
+ gpio_handle_button_event(button, bdata);
+ }
+}
+
static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
struct gpio_button_data *bdata,
struct gpio_keys_button *button)
@@ -446,20 +494,28 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
struct gpio_keys_drvdata *ddata;
struct device *dev = &pdev->dev;
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i, error;
int wakeup = 0;

ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
pdata->nbuttons * sizeof(struct gpio_button_data),
GFP_KERNEL);
- input = input_allocate_device();
+ if (pdata->poll_interval) {
+ poll_dev = allocate_polled_device(dev);
+ input = poll_dev ? poll_dev->input : 0;
+ } else
+ input = input_allocate_device();
if (!ddata || !input) {
dev_err(dev, "failed to allocate state\n");
error = -ENOMEM;
goto fail1;
}

- ddata->input = input;
+ if (pdata->poll_interval)
+ ddata->poll_dev = poll_dev;
+ else
+ ddata->input = input;
ddata->n_buttons = pdata->nbuttons;
ddata->enable = pdata->enable;
ddata->disable = pdata->disable;
@@ -468,6 +524,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ddata);
input_set_drvdata(input, ddata);

+ if (pdata->poll_interval) {
+ poll_dev->private = ddata;
+ poll_dev->poll = gpio_keys_poll;
+ poll_dev->poll_interval = pdata->poll_interval;
+ }
+
input->name = pdev->name;
input->phys = "gpio-keys/input0";
input->dev.parent = &pdev->dev;
@@ -491,14 +553,17 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
bdata->input = input;
bdata->button = button;

+ input_set_capability(input, type, button->code);
+
+ if (pdata->poll_interval)
+ continue;
+
error = gpio_keys_setup_key(pdev, bdata, button);
if (error)
goto fail2;

if (button->wakeup)
wakeup = 1;
-
- input_set_capability(input, type, button->code);
}

error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -508,7 +573,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
goto fail2;
}

- error = input_register_device(input);
+ if (pdata->poll_interval)
+ error = register_polled_device(poll_dev);
+ else
+ error = input_register_device(input);
if (error) {
dev_err(dev, "Unable to register input device, error: %d\n",
error);
@@ -528,7 +596,9 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
fail2:
while (--i >= 0) {
- free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
+ if (!pdata->poll_interval)
+ free_irq(gpio_to_irq(pdata->buttons[i].gpio),
+ &ddata->data[i]);
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
@@ -537,7 +607,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, NULL);
fail1:
- input_free_device(input);
+ if (pdata->poll_interval)
+ free_polled_device(poll_dev);
+ else
+ input_free_device(input);
kfree(ddata);

return error;
@@ -547,7 +620,8 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
{
struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
- struct input_dev *input = ddata->input;
+ struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i;

sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -555,15 +629,25 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, 0);

for (i = 0; i < pdata->nbuttons; i++) {
- int irq = gpio_to_irq(pdata->buttons[i].gpio);
- free_irq(irq, &ddata->data[i]);
+ if (!pdata->poll_interval) {
+ int irq = gpio_to_irq(pdata->buttons[i].gpio);
+ free_irq(irq, &ddata->data[i]);
+ }
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
gpio_free(pdata->buttons[i].gpio);
}

- input_unregister_device(input);
+ if (pdata->poll_interval) {
+ poll_dev = ddata->poll_dev;
+ unregister_polled_device(poll_dev);
+ free_polled_device(poll_dev);
+ } else {
+ input = ddata->input;
+ input_unregister_device(input);
+ input_free_device(input);
+ }

return 0;
}
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ce73a30..5fdd495 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -19,6 +19,7 @@ struct gpio_keys_platform_data {
unsigned int rep:1; /* enable input subsystem auto repeat */
int (*enable)(struct device *dev);
void (*disable)(struct device *dev);
+ unsigned int poll_interval; /* polling interval in ms */
};

#endif
--
1.7.0.4

2010-11-16 19:39:46

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches and
LEDs.

Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.

The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>

---

Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 217 ++++++++++++++++++++++++++++++-
1 files changed, 213 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index dcf21e5..79f2c95 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -410,6 +410,208 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
return 0;
}

+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(ui_expander_names)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
+
+static const char const *baseboard_expander_names[] = {
+ "deep_sleep_en", "sw_rst", "tp_23", "tp_22", "tp_21", "user_pb1",
+ "user_led2", "user_led1", "user_sw_1", "user_sw_2", "user_sw_3",
+ "user_sw_4", "user_sw_5", "user_sw_6", "user_sw_7", "user_sw_8"
+};
+
+#define DA850_DEEP_SLEEP_EN_OFFSET 0
+#define DA850_SW_RST_OFFSET 1
+#define DA850_PB1_OFFSET 5
+#define DA850_USER_LED2_OFFSET 6
+#define DA850_USER_SW_1_OFFSET 8
+
+#define DA850_N_USER_SW 8
+#define DA850_N_USER_LED 2
+
+static struct gpio_keys_button user_pb_gpio_key = {
+ .code = KEY_PROG1,
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_PB_DEBOUNCE_MS,
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+};
+
+static struct gpio_keys_platform_data user_pb_gpio_key_platform_data = {
+ .buttons = &user_pb_gpio_key,
+ .nbuttons = 1,
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_PB_POLL_MS,
+};
+
+static struct platform_device user_pb_gpio_key_device = {
+ .name = "gpio-keys",
+ .id = 1,
+ .dev = {
+ .platform_data = &user_pb_gpio_key_platform_data
+ }
+};
+
+static struct gpio_keys_button user_sw_gpio_keys[DA850_N_USER_SW];
+
+static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
+ .buttons = user_sw_gpio_keys,
+ .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_SW_POLL_MS,
+};
+
+static struct platform_device user_sw_gpio_key_device = {
+ .name = "gpio-keys",
+ .id = 2,
+ .dev = {
+ .platform_data = &user_sw_gpio_key_platform_data
+ }
+};
+
+static void da850_user_switches_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_USER_SW; i++) {
+ button = &user_sw_gpio_keys[i];
+
+ button->code = SW_LID + i;
+ button->type = EV_SW;
+ button->active_low = 1;
+ button->wakeup = 0;
+ button->debounce_interval = DA850_PB_DEBOUNCE_MS;
+ button->desc = (char *)
+ baseboard_expander_names[DA850_USER_SW_1_OFFSET + i];
+
+ button->gpio = gpio + DA850_USER_SW_1_OFFSET + i;
+ }
+}
+
+static struct gpio_led user_leds[DA850_N_USER_LED];
+
+static struct gpio_led_platform_data user_led_gpio_data = {
+ .leds = user_leds,
+ .num_leds = ARRAY_SIZE(user_leds),
+};
+
+static struct platform_device user_leds_gpio_device = {
+ .name = "leds-gpio",
+ .id = -1,
+ .dev = {
+ .platform_data = &user_led_gpio_data
+ }
+};
+
+static void da850_user_leds_init(unsigned gpio)
+{
+ int i;
+ struct gpio_led *led;
+
+ for (i = 0; i < DA850_N_USER_LED; i++) {
+ led = &user_leds[i];
+
+ led->active_low = 1;
+ led->gpio = gpio + DA850_USER_LED2_OFFSET + i;
+ led->name =
+ baseboard_expander_names[DA850_USER_LED2_OFFSET + i];
+ }
+}
+
+static int da850_evm_baseboard_expander_setup(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *c)
+{
+ int ret;
+ int deep_sleep_en, sw_rst;
+
+ deep_sleep_en = gpio + DA850_DEEP_SLEEP_EN_OFFSET;
+ sw_rst = gpio + DA850_SW_RST_OFFSET;
+
+ /* Do not allow sysfs control of deep_sleep_en */
+ ret = gpio_request(deep_sleep_en,
+ baseboard_expander_names[DA850_DEEP_SLEEP_EN_OFFSET]);
+ if (ret) {
+ pr_warning("Cannot open IO expander pin %d\n", deep_sleep_en);
+ goto io_exp_setup_deep_sleep_en_fail;
+ }
+ /* do not drive a value on deep_sleep_en */
+ gpio_direction_input(deep_sleep_en);
+
+ /* Do not allow sysfs control of sw_rst */
+ ret = gpio_request(sw_rst,
+ baseboard_expander_names[DA850_SW_RST_OFFSET]);
+ if (ret) {
+ pr_warning("Cannot open IO expander pin %d\n", sw_rst);
+ goto io_exp_setup_sw_rst_fail;
+ }
+ /* do not drive a value on sw_rst */
+ gpio_direction_input(sw_rst);
+
+ /* Register user_pb1 as a gpio-keys button */
+ user_pb_gpio_key.desc = (char *)
+ baseboard_expander_names[DA850_PB1_OFFSET];
+ user_pb_gpio_key.gpio = gpio + DA850_PB1_OFFSET;
+ ret = platform_device_register(&user_pb_gpio_key_device);
+ if (ret) {
+ pr_warning("Cannot register user pb: %d\n", ret);
+ goto io_exp_setup_pb_fail;
+ }
+
+ /*
+ * Register the switches as a separate gpio-keys device so the fast
+ * polling interval for the pushbuttons is not wasted on switches
+ */
+ da850_user_switches_init(gpio);
+ ret = platform_device_register(&user_sw_gpio_key_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander switches"
+ " device\n");
+ goto io_exp_setup_sw_fail;
+ }
+
+ da850_user_leds_init(gpio);
+ ret = platform_device_register(&user_leds_gpio_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander LEDS "
+ "device\n");
+ goto io_exp_setup_leds_fail;
+ }
+
+ return 0;
+
+io_exp_setup_leds_fail:
+ platform_device_unregister(&user_sw_gpio_key_device);
+io_exp_setup_sw_fail:
+ platform_device_unregister(&user_pb_gpio_key_device);
+io_exp_setup_pb_fail:
+ gpio_free(sw_rst);
+io_exp_setup_sw_rst_fail:
+ gpio_free(deep_sleep_en);
+io_exp_setup_deep_sleep_en_fail:
+ return ret;
+}
+
+static int da850_evm_baseboard_expander_teardown(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio, void *c)
+{
+ int deep_sleep_en, sw_rst;
+
+ deep_sleep_en = gpio + DA850_DEEP_SLEEP_EN_OFFSET;
+ sw_rst = gpio + DA850_SW_RST_OFFSET;
+
+ platform_device_unregister(&user_leds_gpio_device);
+ platform_device_unregister(&user_sw_gpio_key_device);
+ platform_device_unregister(&user_pb_gpio_key_device);
+ gpio_free(sw_rst);
+ gpio_free(deep_sleep_en);
+
+ return 0;
+}
+
static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
@@ -417,6 +619,13 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.names = ui_expander_names,
};

+static struct pca953x_platform_data da850_evm_baseboard_expander_info = {
+ .gpio_base = DA850_BB_EXPANDER_GPIO_BASE,
+ .setup = da850_evm_baseboard_expander_setup,
+ .teardown = da850_evm_baseboard_expander_teardown,
+ .names = baseboard_expander_names,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -424,10 +633,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
- /*
- * TODO : populate at runtime using
- * .irq = gpio_to_irq(GPIO_TO_PIN(2,7)),
- */
+ },
+ {
+ I2C_BOARD_INFO("tca6416", 0x21),
+ .platform_data = &da850_evm_baseboard_expander_info,
},
};

--
1.7.0.4

2010-11-16 19:40:09

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
gpio-key device.

The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
disable the peripherals found on the UI board in addition to the 8 pushbuttons
mentioned above. The reason the existing tca6416-keypad driver is not employed
is because there was no aparent way to keep the gpio lines used as
SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
tca6416-keypad instance.

Some experimentation with the polling interval was performed; we were searching
for the largest polling interval that did not affect the feel of the
responsiveness of the buttons. It is very subjective but 200ms seems to be a
good value that accepts firm pushes but rejects very light ones. The key values
assigned to the buttons were arbitrarily chosen to be F1-F8.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>

---

Changes since v1:
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 76 +++++++++++++++++++++++++++++++
2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index b77b860..5163a1c 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -178,6 +178,9 @@ config DA850_UI_RMII

endchoice

+config INPUT_POLLDEV
+ default MACH_DAVINCI_DA850_EVM
+
config MACH_TNETV107X
bool "TI TNETV107X Reference Platform"
default ARCH_DAVINCI_TNETV107X
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index c6e11c6..ff71ffd 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -17,8 +17,10 @@
#include <linux/i2c.h>
#include <linux/i2c/at24.h>
#include <linux/i2c/pca953x.h>
+#include <linux/input.h>
#include <linux/mfd/tps6507x.h>
#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
#include <linux/platform_device.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -272,6 +274,63 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel)
static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
#endif

+
+#define DA850_PB_DEBOUNCE_MS 10
+/*
+ * At 200ms polling interval it is possible to miss an
+ * event by tapping very lightly on the push button but most
+ * pushes do result in an event; longer intervals require the
+ * user to hold the button whereas shorter intervals require
+ * more CPU time for polling.
+ */
+#define DA850_PB_POLL_MS 200
+/* No need to poll switches anywhere near as fast */
+#define DA850_SW_POLL_MS 700
+
+static const char const *ui_expander_names[] = {
+ "dgnd", "dgnd", "dgnd", "dgnd", "nc", "sel_c", "sel_b", "sel_a", "pb8",
+ "pb7", "pb6", "pb5", "pb4", "pb3", "pb2", "pb1"
+};
+
+#define DA850_UI_PB8_OFFSET 8
+#define DA850_N_UI_PB 8
+
+static struct gpio_keys_button user_ui_pbs_gpio_keys[DA850_N_UI_PB];
+
+static struct gpio_keys_platform_data user_ui_pbs_gpio_key_platform_data = {
+ .buttons = user_ui_pbs_gpio_keys,
+ .nbuttons = ARRAY_SIZE(user_ui_pbs_gpio_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_PB_POLL_MS,
+};
+
+static struct platform_device user_ui_pb_gpio_key_device = {
+ .name = "gpio-keys",
+ .id = 0,
+ .dev = {
+ .platform_data = &user_ui_pbs_gpio_key_platform_data
+ }
+};
+
+static void da850_ui_pushbuttons_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_UI_PB; i++) {
+ button = &user_ui_pbs_gpio_keys[i];
+
+ button->code = KEY_F8 - i;
+ button->type = EV_KEY;
+ button->active_low = 1;
+ button->wakeup = 0;
+ button->debounce_interval = DA850_PB_DEBOUNCE_MS;
+ button->desc = (char *)
+ ui_expander_names[DA850_UI_PB8_OFFSET + i];
+ button->gpio = gpio + DA850_UI_PB8_OFFSET + i;
+ }
+}
+
static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
unsigned ngpio, void *c)
{
@@ -304,6 +363,14 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
gpio_direction_output(sel_b, 1);
gpio_direction_output(sel_c, 1);

+ da850_ui_pushbuttons_init(gpio);
+ ret = platform_device_register(&user_ui_pb_gpio_key_device);
+ if (ret) {
+ pr_warning("Could not register UI GPIO expander push-buttons"
+ " device\n");
+ goto exp_setup_keys_fail;
+ }
+
ui_card_detected = 1;
pr_info("DA850/OMAP-L138 EVM UI card detected\n");

@@ -313,6 +380,8 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,

return 0;

+exp_setup_keys_fail:
+ gpio_free(sel_c);
exp_setup_selc_fail:
gpio_free(sel_b);
exp_setup_selb_fail:
@@ -324,6 +393,8 @@ exp_setup_sela_fail:
static int da850_evm_ui_expander_teardown(struct i2c_client *client,
unsigned gpio, unsigned ngpio, void *c)
{
+ platform_device_unregister(&user_ui_pb_gpio_key_device);
+
/* deselect all functionalities */
gpio_set_value(gpio + 5, 1);
gpio_set_value(gpio + 6, 1);
@@ -340,6 +411,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
+ .names = ui_expander_names,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
@@ -349,6 +421,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
+ /*
+ * TODO : populate at runtime using
+ * .irq = gpio_to_irq(GPIO_TO_PIN(2,7)),
+ */
},
};

--
1.7.0.4

2010-11-16 19:46:18

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v2 3/4] da850-evm: extract defines for SEL{A,B,C} pins in UI expander

The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
pins by 'magic number' in each function. This patch extracts common #defines
for their offsets in the expander and uses them.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>

---

No changes since v1
---
arch/arm/mach-davinci/board-da850-evm.c | 27 +++++++++++++++------------
1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index ff71ffd..dcf21e5 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -292,6 +292,9 @@ static const char const *ui_expander_names[] = {
"pb7", "pb6", "pb5", "pb4", "pb3", "pb2", "pb1"
};

+#define DA850_SEL_A_OFFSET 7
+#define DA850_SEL_B_OFFSET 6
+#define DA850_SEL_C_OFFSET 5
#define DA850_UI_PB8_OFFSET 8
#define DA850_N_UI_PB 8

@@ -336,23 +339,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + 7;
- sel_b = gpio + 6;
- sel_c = gpio + 5;
+ sel_a = gpio + DA850_SEL_A_OFFSET;
+ sel_b = gpio + DA850_SEL_B_OFFSET;
+ sel_c = gpio + DA850_SEL_C_OFFSET;

- ret = gpio_request(sel_a, "sel_a");
+ ret = gpio_request(sel_a, ui_expander_names[DA850_SEL_A_OFFSET]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, "sel_b");
+ ret = gpio_request(sel_b, ui_expander_names[DA850_SEL_B_OFFSET]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, "sel_c");
+ ret = gpio_request(sel_c, ui_expander_names[DA850_SEL_C_OFFSET]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -396,13 +399,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&user_ui_pb_gpio_key_device);

/* deselect all functionalities */
- gpio_set_value(gpio + 5, 1);
- gpio_set_value(gpio + 6, 1);
- gpio_set_value(gpio + 7, 1);
+ gpio_set_value(gpio + DA850_SEL_C_OFFSET, 1);
+ gpio_set_value(gpio + DA850_SEL_B_OFFSET, 1);
+ gpio_set_value(gpio + DA850_SEL_A_OFFSET, 1);

- gpio_free(gpio + 5);
- gpio_free(gpio + 6);
- gpio_free(gpio + 7);
+ gpio_free(gpio + DA850_SEL_C_OFFSET);
+ gpio_free(gpio + DA850_SEL_B_OFFSET);
+ gpio_free(gpio + DA850_SEL_A_OFFSET);

return 0;
}
--
1.7.0.4

2010-11-19 09:58:24

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Ben,

Thanks for the patches. Some comments/questions below:

On Wed, Nov 17, 2010 at 01:09:35, Ben Gardiner wrote:
> This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
> gpio-key device.
>
> The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
> disable the peripherals found on the UI board in addition to the 8 pushbuttons
> mentioned above. The reason the existing tca6416-keypad driver is not employed
> is because there was no aparent way to keep the gpio lines used as
> SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
> tca6416-keypad instance.
>
> Some experimentation with the polling interval was performed; we were searching
> for the largest polling interval that did not affect the feel of the
> responsiveness of the buttons. It is very subjective but 200ms seems to be a
> good value that accepts firm pushes but rejects very light ones. The key values
> assigned to the buttons were arbitrarily chosen to be F1-F8.
>
> Signed-off-by: Ben Gardiner <[email protected]>
> Reviewed-by: Chris Cordahi <[email protected]>
> CC: Govindarajan, Sriramakrishnan <[email protected]>
>
> ---
>
> Changes since v1:
> * set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
> unconditionally

I didn't see the v1 posting (wonder why), but why is this
required? Why cant we depend on this being selected from
Device Drivers->Input device support in menuconfig?

[...]

> @@ -349,6 +421,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
> {
> I2C_BOARD_INFO("tca6416", 0x20),
> .platform_data = &da850_evm_ui_expander_info,
> + /*
> + * TODO : populate at runtime using
> + * .irq = gpio_to_irq(GPIO_TO_PIN(2,7)),
> + */

You seem to be adding this in this patch and removing
in 4/4.

Thanks,
Sekhar

2010-11-19 11:19:47

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 3/4] da850-evm: extract defines for SEL{A, B, C} pins in UI expander

Hi Ben,

On Wed, Nov 17, 2010 at 01:09:36, Ben Gardiner wrote:
> The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
> pins by 'magic number' in each function. This patch extracts common #defines
> for their offsets in the expander and uses them.
>
> Signed-off-by: Ben Gardiner <[email protected]>
> Reviewed-by: Chris Cordahi <[email protected]>
>
> ---
>
> No changes since v1
> ---
> arch/arm/mach-davinci/board-da850-evm.c | 27 +++++++++++++++------------
> 1 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index ff71ffd..dcf21e5 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -292,6 +292,9 @@ static const char const *ui_expander_names[] = {
> "pb7", "pb6", "pb5", "pb4", "pb3", "pb2", "pb1"
> };
>
> +#define DA850_SEL_A_OFFSET 7
> +#define DA850_SEL_B_OFFSET 6
> +#define DA850_SEL_C_OFFSET 5
> #define DA850_UI_PB8_OFFSET 8
> #define DA850_N_UI_PB 8

In this case, I think in this case indexed array initialization
will help keep the offsets and names matched. Here is an untested
patch on your patch, please consider using it.

Thanks,
Sekhar

---8<----
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index dcf21e5..508e5f2 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -287,16 +287,35 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
/* No need to poll switches anywhere near as fast */
#define DA850_SW_POLL_MS 700

-static const char const *ui_expander_names[] = {
- "dgnd", "dgnd", "dgnd", "dgnd", "nc", "sel_c", "sel_b", "sel_a", "pb8",
- "pb7", "pb6", "pb5", "pb4", "pb3", "pb2", "pb1"
-};
-
-#define DA850_SEL_A_OFFSET 7
-#define DA850_SEL_B_OFFSET 6
-#define DA850_SEL_C_OFFSET 5
-#define DA850_UI_PB8_OFFSET 8
-#define DA850_N_UI_PB 8
+static enum da850_evm_ui_exp_pins {
+ DA850_EVM_UI_EXP_SEL_C = 5,
+ DA850_EVM_UI_EXP_SEL_B,
+ DA850_EVM_UI_EXP_SEL_A,
+ DA850_EVM_UI_EXP_PB8,
+ DA850_EVM_UI_EXP_PB7,
+ DA850_EVM_UI_EXP_PB6,
+ DA850_EVM_UI_EXP_PB5,
+ DA850_EVM_UI_EXP_PB4,
+ DA850_EVM_UI_EXP_PB3,
+ DA850_EVM_UI_EXP_PB2,
+ DA850_EVM_UI_EXP_PB1,
+};
+
+static const char const *da850_evm_ui_exp[] = {
+ [DA850_EVM_UI_EXP_SEL_C] = "sel_c",
+ [DA850_EVM_UI_EXP_SEL_B] = "sel_b",
+ [DA850_EVM_UI_EXP_SEL_A] = "sel_a",
+ [DA850_EVM_UI_EXP_PB8] = "pb8",
+ [DA850_EVM_UI_EXP_PB7] = "pb7",
+ [DA850_EVM_UI_EXP_PB6] = "pb6",
+ [DA850_EVM_UI_EXP_PB5] = "pb5",
+ [DA850_EVM_UI_EXP_PB4] = "pb4",
+ [DA850_EVM_UI_EXP_PB3] = "pb3",
+ [DA850_EVM_UI_EXP_PB2] = "pb2",
+ [DA850_EVM_UI_EXP_PB1] = "pb1",
+};
+
+#define DA850_N_UI_PB 8

static struct gpio_keys_button user_ui_pbs_gpio_keys[DA850_N_UI_PB];

@@ -329,8 +348,8 @@ static void da850_ui_pushbuttons_init(unsigned gpio)
button->wakeup = 0;
button->debounce_interval = DA850_PB_DEBOUNCE_MS;
button->desc = (char *)
- ui_expander_names[DA850_UI_PB8_OFFSET + i];
- button->gpio = gpio + DA850_UI_PB8_OFFSET + i;
+ da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i];
+ button->gpio = gpio + DA850_EVM_UI_EXP_PB8 + i;
}
}

@@ -339,23 +358,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + DA850_SEL_A_OFFSET;
- sel_b = gpio + DA850_SEL_B_OFFSET;
- sel_c = gpio + DA850_SEL_C_OFFSET;
+ sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
+ sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
+ sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;

- ret = gpio_request(sel_a, ui_expander_names[DA850_SEL_A_OFFSET]);
+ ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, ui_expander_names[DA850_SEL_B_OFFSET]);
+ ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, ui_expander_names[DA850_SEL_C_OFFSET]);
+ ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -399,13 +418,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&user_ui_pb_gpio_key_device);

/* deselect all functionalities */
- gpio_set_value(gpio + DA850_SEL_C_OFFSET, 1);
- gpio_set_value(gpio + DA850_SEL_B_OFFSET, 1);
- gpio_set_value(gpio + DA850_SEL_A_OFFSET, 1);
+ gpio_set_value(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
+ gpio_set_value(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
+ gpio_set_value(gpio + DA850_EVM_UI_EXP_SEL_A, 1);

- gpio_free(gpio + DA850_SEL_C_OFFSET);
- gpio_free(gpio + DA850_SEL_B_OFFSET);
- gpio_free(gpio + DA850_SEL_A_OFFSET);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);

return 0;
}
@@ -414,7 +433,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
- .names = ui_expander_names,
+ .names = da850_evm_ui_exp,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {

2010-11-19 12:15:12

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Ben,

The board patches look good to me overall. Some minor comments below:

On Wed, Nov 17, 2010 at 01:09:37, Ben Gardiner wrote:
> This patch adds a pca953x platform device for the tca6416 found on the evm
> baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
> separate I2C address. The pins of the baseboard IO expander are connected to
> software reset, deep sleep enable, test points, a push button, DIP switches and
> LEDs.
>
> Add support for the push button, DIP switches and LEDs and test points (as
> free GPIOs). The reset and deep sleep enable connections are reserved by the
> setup routine so that userspace can't toggle those lines.
>
> The existing tca6416-keypad driver was not employed because there was no
> apararent way to register the LEDs connected to gpio's on the tca6416 while
> simultaneously registering the tca6416-keypad instance.
>
> Signed-off-by: Ben Gardiner <[email protected]>
> Reviewed-by: Chris Cordahi <[email protected]>
> CC: Govindarajan, Sriramakrishnan <[email protected]>
>
> ---
>
> Changes since v1:
> * adding note about why the tca6416-keypad driver was not used.
> * adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
> driver
> ---
> arch/arm/mach-davinci/board-da850-evm.c | 217 ++++++++++++++++++++++++++++++-
> 1 files changed, 213 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index dcf21e5..79f2c95 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -410,6 +410,208 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
> return 0;
> }
>
> +/* assign the baseboard expander's GPIOs after the UI board's */
> +#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(ui_expander_names)
> +#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
> +
> +static const char const *baseboard_expander_names[] = {
> + "deep_sleep_en", "sw_rst", "tp_23", "tp_22", "tp_21", "user_pb1",
> + "user_led2", "user_led1", "user_sw_1", "user_sw_2", "user_sw_3",
> + "user_sw_4", "user_sw_5", "user_sw_6", "user_sw_7", "user_sw_8"
> +};
> +
> +#define DA850_DEEP_SLEEP_EN_OFFSET 0
> +#define DA850_SW_RST_OFFSET 1
> +#define DA850_PB1_OFFSET 5
> +#define DA850_USER_LED2_OFFSET 6
> +#define DA850_USER_SW_1_OFFSET 8

Again, I think index initialized array will work much
better here. Currently it is error prone to keep the defines
and the array of names in sync.

> +
> +#define DA850_N_USER_SW 8
> +#define DA850_N_USER_LED 2
> +
> +static struct gpio_keys_button user_pb_gpio_key = {

"da850evm_bb_pb" might be a better name?

> + .code = KEY_PROG1,
> + .type = EV_KEY,
> + .active_low = 1,
> + .wakeup = 0,
> + .debounce_interval = DA850_PB_DEBOUNCE_MS,
> + .gpio = -1, /* assigned at runtime */
> + .desc = NULL, /* assigned at runtime */
> +};
> +
> +static struct gpio_keys_platform_data user_pb_gpio_key_platform_data = {

Similarly "da850evm_bb_pb_pdata" instead of the long name?

> + .buttons = &user_pb_gpio_key,
> + .nbuttons = 1,
> + .rep = 0, /* disable auto-repeat */
> + .poll_interval = DA850_PB_POLL_MS,
> +};
> +
> +static struct platform_device user_pb_gpio_key_device = {
> + .name = "gpio-keys",
> + .id = 1,
> + .dev = {
> + .platform_data = &user_pb_gpio_key_platform_data
> + }
> +};
> +
> +static struct gpio_keys_button user_sw_gpio_keys[DA850_N_USER_SW];

You could initialize most static fields here itself using:

static struct gpio_keys_button user_sw_gpio_keys[] = {
[0 ... DA850_N_USER_SW] = {
...
...
...
},
};

This way your runtime initialization will come down.

> +
> +static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
> + .buttons = user_sw_gpio_keys,
> + .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
> + .rep = 0, /* disable auto-repeat */
> + .poll_interval = DA850_SW_POLL_MS,
> +};

I wonder if we really have create to separate platform data
for switches and push buttons. If it is only the debounce period
that is different, it can be handled by initializing that field
differently.

> +
> +static struct platform_device user_sw_gpio_key_device = {
> + .name = "gpio-keys",
> + .id = 2,
> + .dev = {
> + .platform_data = &user_sw_gpio_key_platform_data

End with a ',' here.

> + }
> +};
> +
> +static void da850_user_switches_init(unsigned gpio)
> +{
> + int i;
> + struct gpio_keys_button *button;
> +
> + for (i = 0; i < DA850_N_USER_SW; i++) {
> + button = &user_sw_gpio_keys[i];
> +
> + button->code = SW_LID + i;
> + button->type = EV_SW;
> + button->active_low = 1;
> + button->wakeup = 0;
> + button->debounce_interval = DA850_PB_DEBOUNCE_MS;
> + button->desc = (char *)
> + baseboard_expander_names[DA850_USER_SW_1_OFFSET + i];

You could use some shorter names here. In the context of EVM file, 'bb'
will fit for "base board", "exp" works for expander. Also, here it is
clear the macro is used as an array offset, so _OFFSET can be dropped
altogether. Similarly with _names. Also, the global and static symbols
should be pre-fixed with da850evm_ so it is easy to look up the symbol
file or object dump.

> +
> + button->gpio = gpio + DA850_USER_SW_1_OFFSET + i;
> + }
> +}
> +
> +static struct gpio_led user_leds[DA850_N_USER_LED];
> +
> +static struct gpio_led_platform_data user_led_gpio_data = {
> + .leds = user_leds,
> + .num_leds = ARRAY_SIZE(user_leds),
> +};
> +
> +static struct platform_device user_leds_gpio_device = {
> + .name = "leds-gpio",
> + .id = -1,
> + .dev = {
> + .platform_data = &user_led_gpio_data
> + }
> +};
> +
> +static void da850_user_leds_init(unsigned gpio)
> +{
> + int i;
> + struct gpio_led *led;
> +
> + for (i = 0; i < DA850_N_USER_LED; i++) {
> + led = &user_leds[i];
> +
> + led->active_low = 1;
> + led->gpio = gpio + DA850_USER_LED2_OFFSET + i;
> + led->name =
> + baseboard_expander_names[DA850_USER_LED2_OFFSET + i];
> + }
> +}
> +
> +static int da850_evm_baseboard_expander_setup(struct i2c_client *client,
> + unsigned gpio, unsigned ngpio,
> + void *c)
> +{
> + int ret;
> + int deep_sleep_en, sw_rst;
> +
> + deep_sleep_en = gpio + DA850_DEEP_SLEEP_EN_OFFSET;
> + sw_rst = gpio + DA850_SW_RST_OFFSET;
> +
> + /* Do not allow sysfs control of deep_sleep_en */
> + ret = gpio_request(deep_sleep_en,
> + baseboard_expander_names[DA850_DEEP_SLEEP_EN_OFFSET]);

How does gpio_request prevent sysfs control?

Thanks,
Sekhar

2010-11-19 15:38:38

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] da850-evm: extract defines for SEL{A, B, C} pins in UI expander

Hi Sehkar,

On Fri, Nov 19, 2010 at 6:19 AM, Nori, Sekhar <[email protected]> wrote:
> [...]
> In this case, I think in this case indexed array initialization
> will help keep the offsets and names matched. Here is an untested
> patch on your patch, please consider using it.

I love it. I'll post a re-spin with this patch integrated and you SOB
added. Thank you, Sehkar.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-19 15:40:54

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Sehkar,

On Fri, Nov 19, 2010 at 7:14 AM, Nori, Sekhar <[email protected]> wrote:
> The board patches look good to me overall. Some minor comments below:

Thanks -- and I appreciate your input.

> On Wed, Nov 17, 2010 at 01:09:37, Ben Gardiner wrote:
>> [...]
>> +static const char const *baseboard_expander_names[] = {
>> + ? ? "deep_sleep_en", "sw_rst", "tp_23", "tp_22", "tp_21", "user_pb1",
>> + ? ? "user_led2", "user_led1", "user_sw_1", "user_sw_2", "user_sw_3",
>> + ? ? "user_sw_4", "user_sw_5", "user_sw_6", "user_sw_7", "user_sw_8"
>> +};
>> +
>> +#define DA850_DEEP_SLEEP_EN_OFFSET ? ? ? ? ? 0
>> +#define DA850_SW_RST_OFFSET ? ? ? ? ? ? ? ? ?1
>> +#define DA850_PB1_OFFSET ? ? ? ? ? ? ? ? ? ? 5
>> +#define DA850_USER_LED2_OFFSET ? ? ? ? ? ? ? ? ? ? ? 6
>> +#define DA850_USER_SW_1_OFFSET ? ? ? ? ? ? ? ? ? ? ? 8
>
> Again, I think index initialized array will work much
> better here. Currently it is error prone to keep the defines
> and the array of names in sync.

Agreed. Now that I've seen what you did with the previous patch I am
eager to apply that pattern also to the definitions in this patch.

>> [...]
>> +static struct gpio_keys_platform_data user_pb_gpio_key_platform_data = {
>
> Similarly "da850evm_bb_pb_pdata" instead of the long name?

Will do.

>> [...]
>> +static struct gpio_keys_button user_sw_gpio_keys[DA850_N_USER_SW];
>
> You could initialize most static fields here itself using:
>
> static struct gpio_keys_button user_sw_gpio_keys[] = {
> ? ? ? ?[0 ... DA850_N_USER_SW] = {
> ? ? ? ? ? ? ? ?...
> ? ? ? ? ? ? ? ?...
> ? ? ? ? ? ? ? ?...
> ? ? ? ?},
> };
>
> This way your runtime initialization will come down.

Indeed; I am eager to extend the pattern you introduced to this
initialization also.

>> +
>> +static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
>> + ? ? .buttons = user_sw_gpio_keys,
>> + ? ? .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
>> + ? ? .rep = 0, /* disable auto-repeat */
>> + ? ? .poll_interval = DA850_SW_POLL_MS,
>> +};
>
> I wonder if we really have create to separate platform data
> for switches and push buttons. If it is only the debounce period
> that is different, it can be handled by initializing that field
> differently.

I see. Good idea; we can declare an array of gpio_keys_platform_data.

Note; it is the polling interval which differs, not the debounce interval.

>> +
>> +static struct platform_device user_sw_gpio_key_device = {
>> + ? ? .name = "gpio-keys",
>> + ? ? .id = 2,
>> + ? ? .dev = {
>> + ? ? ? ? ? ? .platform_data = &user_sw_gpio_key_platform_data
>
> End with a ',' here.

Will do.

>> [...]
>> +static void da850_user_switches_init(unsigned gpio)
>> +{
>> + ? ? int i;
>> + ? ? struct gpio_keys_button *button;
>> +
>> + ? ? for (i = 0; i < DA850_N_USER_SW; i++) {
>> + ? ? ? ? ? ? button = &user_sw_gpio_keys[i];
>> +
>> + ? ? ? ? ? ? button->code = SW_LID + i;
>> + ? ? ? ? ? ? button->type = EV_SW;
>> + ? ? ? ? ? ? button->active_low = 1;
>> + ? ? ? ? ? ? button->wakeup = 0;
>> + ? ? ? ? ? ? button->debounce_interval = DA850_PB_DEBOUNCE_MS;
>> + ? ? ? ? ? ? button->desc = (char *)
>> + ? ? ? ? ? ? ? ? ? ? baseboard_expander_names[DA850_USER_SW_1_OFFSET + i];
>
> You could use some shorter names here. In the context of EVM file, 'bb'
> will fit for "base board", "exp" works for expander. Also, here it is
> clear the macro is used as an array offset, so _OFFSET can be dropped
> altogether. Similarly with _names. Also, the global and static symbols
> should be pre-fixed with da850evm_ so it is easy to look up the symbol
> file or object dump.

I see your points; 1) exp and bb for expander and baseboard variable
names, respectively 2) drop _OFFSET and _names 3) prefix statics and
globals with da850evm.

>> [...]
>
> How does gpio_request prevent sysfs control?

To obtain access to a gpio through the sysfs interface the user must
first write the gpio number to 'export'. When the gpio has been
gpio_request()'d the result of writing to 'export' is nothing; whereas
writing to export would normally result in the appearance of a named
gpio line alongside 'export'.

I hope the following commands and their output illustrate my point:

$ cd /sys/class/gpio/
$ ls
export gpiochip128 gpiochip160 gpiochip64 unexport
gpiochip0 gpiochip144 gpiochip32 gpiochip96
$ echo 160 > export
$ ls
export gpiochip128 gpiochip160 gpiochip64 unexport
gpiochip0 gpiochip144 gpiochip32 gpiochip96
$ echo 163 > export
$ ls
export gpiochip128 gpiochip160 gpiochip64 tp_22
gpiochip0 gpiochip144 gpiochip32 gpiochip96 unexport

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-19 15:44:35

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Sekhar,

Thank you for your reviews.

On Fri, Nov 19, 2010 at 4:58 AM, Nori, Sekhar <[email protected]> wrote:
> [...]
> On Wed, Nov 17, 2010 at 01:09:35, Ben Gardiner wrote:
>> [...]
>> Changes since v1:
>> ?* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
>> ? ?unconditionally
>
> I didn't see the v1 posting (wonder why), but why is this
> required? Why cant we depend on this being selected from
> Device Drivers->Input device support in menuconfig?

I'm sorry about that. I checked gmane and the v1 never made it to the
davinci-linux list. I think it was because of my (mis)use of git
send-email. Version 2 made it to the list so I've now found the magic
incantation.

The reason we should at least select INPUT_POLLDEV as a default option
is: when it is not set the gpio-keys instances registered by the
da850-evm board setup routine will fail probe() of the gpio-keys
driver since it will not have polled-input support enabled.

By setting INPUT_POLLDEV default for the da850-evm users will get
functioning pushbuttons and switches with the default config but they
will be able to disable INPUT_POLLDEV or gpio-keys drivers in their
defconfig at their convenience.

> [...]
>
>> @@ -349,6 +421,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
>> ? ? ? {
>> ? ? ? ? ? ? ? I2C_BOARD_INFO("tca6416", 0x20),
>> ? ? ? ? ? ? ? .platform_data = &da850_evm_ui_expander_info,
>> + ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ?* TODO : populate at runtime using
>> + ? ? ? ? ? ? ?* .irq = gpio_to_irq(GPIO_TO_PIN(2,7)),
>> + ? ? ? ? ? ? ?*/
>
> You seem to be adding this in this patch and removing
> in 4/4.

Oops. That's my fault. The internal reviewers also reminded me to
remove these before sending. Sorry for the noise/ I will remove this
block in _this_ patch in the re-spin.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-19 17:02:43

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Ben,

On Fri, Nov 19, 2010 at 10:40:50AM -0500, Ben Gardiner wrote:
>
> >> +
> >> +static struct gpio_keys_platform_data user_sw_gpio_key_platform_data = {
> >> + ? ? .buttons = user_sw_gpio_keys,
> >> + ? ? .nbuttons = ARRAY_SIZE(user_sw_gpio_keys),
> >> + ? ? .rep = 0, /* disable auto-repeat */
> >> + ? ? .poll_interval = DA850_SW_POLL_MS,
> >> +};
> >
> > I wonder if we really have create to separate platform data
> > for switches and push buttons. If it is only the debounce period
> > that is different, it can be handled by initializing that field
> > differently.
>
> I see. Good idea; we can declare an array of gpio_keys_platform_data.
>
> Note; it is the polling interval which differs, not the debounce interval.

Another question is why do you want to poll the same device at
different intervals? The processor is already woken up so it makes sense
to do as much as possible instead of going to sleep. In your case
200/700 for every 1400ms interval you wake an extra time to poll
switches so unless polling switches takes very long time it is better to
combine them together (and into one input device, probably).

--
Dmitry

2010-11-19 17:15:34

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Dmitry,

On Fri, Nov 19, 2010 at 12:02 PM, Dmitry Torokhov
<[email protected]> wrote:
> Another question is why do you want to poll the same device at
> different intervals? The processor is already woken up so it makes sense
> to do as much as possible instead of going to sleep. In your case
> 200/700 for every 1400ms interval you wake an extra time to poll
> switches so unless polling switches takes very long time it is better to
> combine them together (and into one input device, probably).

I see. That is a good question :)

I didn't realize that splitting into two separate input devices with
different polling intervals would have the opposite effect than what I
intended. Thank you for pointing this out. I will definitely integrate
them into one input device polled at 200ms.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-19 21:37:33

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v3 2/4] da850-evm: add UI Expander pushbuttons

This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
gpio-key device.

The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
disable the peripherals found on the UI board in addition to the 8 pushbuttons
mentioned above. The reason the existing tca6416-keypad driver is not employed
is because there was no aparent way to keep the gpio lines used as
SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
tca6416-keypad instance.

Some experimentation with the polling interval was performed; we were searching
for the largest polling interval that did not affect the feel of the
responsiveness of the buttons. It is very subjective but 200ms seems to be a
good value that accepts firm pushes but rejects very light ones. The key values
assigned to the buttons were arbitrarily chosen to be F1-F8.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>

---

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in this patch instead of 4/4
(Nori, Sekhar)
* integrated the static array initialization patch of Sekhar Nori
* use static array initialization ranges
* rename DA850_PB_POLL_MS to DA850_GPIO_KEYS_POLL_MS
* use shorter names prefixed with da850_evm

Changes since v1:
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver

---
arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 98 ++++++++++++++++++++++++++++++-
2 files changed, 100 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index 84066e8..1fbf73f 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -180,6 +180,9 @@ endchoice
config GPIO_PCA953X
default MACH_DAVINCI_DA850_EVM

+config INPUT_POLLDEV
+ default MACH_DAVINCI_DA850_EVM
+
config MACH_TNETV107X
bool "TI TNETV107X Reference Platform"
default ARCH_DAVINCI_TNETV107X
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index f89b0b7..b0763f7 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -17,8 +17,10 @@
#include <linux/i2c.h>
#include <linux/i2c/at24.h>
#include <linux/i2c/pca953x.h>
+#include <linux/input.h>
#include <linux/mfd/tps6507x.h>
#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
#include <linux/platform_device.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -272,6 +274,88 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel)
static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
#endif

+
+#define DA850_KEYS_DEBOUNCE_MS 10
+/*
+ * At 200ms polling interval it is possible to miss an
+ * event by tapping very lightly on the push button but most
+ * pushes do result in an event; longer intervals require the
+ * user to hold the button whereas shorter intervals require
+ * more CPU time for polling.
+ */
+#define DA850_GPIO_KEYS_POLL_MS 200
+
+enum da850_evm_ui_exp_pins {
+ DA850_EVM_UI_EXP_SEL_C = 5,
+ DA850_EVM_UI_EXP_SEL_B,
+ DA850_EVM_UI_EXP_SEL_A,
+ DA850_EVM_UI_EXP_PB8,
+ DA850_EVM_UI_EXP_PB7,
+ DA850_EVM_UI_EXP_PB6,
+ DA850_EVM_UI_EXP_PB5,
+ DA850_EVM_UI_EXP_PB4,
+ DA850_EVM_UI_EXP_PB3,
+ DA850_EVM_UI_EXP_PB2,
+ DA850_EVM_UI_EXP_PB1,
+};
+
+static const char const *da850_evm_ui_exp[] = {
+ [DA850_EVM_UI_EXP_SEL_C] = "sel_c",
+ [DA850_EVM_UI_EXP_SEL_B] = "sel_b",
+ [DA850_EVM_UI_EXP_SEL_A] = "sel_a",
+ [DA850_EVM_UI_EXP_PB8] = "pb8",
+ [DA850_EVM_UI_EXP_PB7] = "pb7",
+ [DA850_EVM_UI_EXP_PB6] = "pb6",
+ [DA850_EVM_UI_EXP_PB5] = "pb5",
+ [DA850_EVM_UI_EXP_PB4] = "pb4",
+ [DA850_EVM_UI_EXP_PB3] = "pb3",
+ [DA850_EVM_UI_EXP_PB2] = "pb2",
+ [DA850_EVM_UI_EXP_PB1] = "pb1",
+};
+
+#define DA850_N_UI_PB 8
+
+static struct gpio_keys_button da850_evm_ui_keys[] = {
+ [0 ... DA850_N_UI_PB - 1] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_ui_keys_pdata = {
+ .buttons = da850_evm_ui_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_ui_keys_device = {
+ .name = "gpio-keys",
+ .id = 0,
+ .dev = {
+ .platform_data = &da850_evm_ui_keys_pdata
+ },
+};
+
+static void da850_evm_ui_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_UI_PB; i++) {
+ button = &da850_evm_ui_keys[i];
+ button->code = KEY_F8 - i;
+ button->desc = (char *)
+ da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i];
+ button->gpio = gpio + DA850_EVM_UI_EXP_PB8 + i;
+ }
+}
+
static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
unsigned ngpio, void *c)
{
@@ -304,15 +388,24 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
gpio_direction_output(sel_b, 1);
gpio_direction_output(sel_c, 1);

+ da850_evm_ui_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_ui_keys_device);
+ if (ret) {
+ pr_warning("Could not register UI GPIO expander push-buttons"
+ " device\n");
+ goto exp_setup_keys_fail;
+ }
+
ui_card_detected = 1;
pr_info("DA850/OMAP-L138 EVM UI card detected\n");

da850_evm_setup_nor_nand();
-
da850_evm_setup_emac_rmii(sel_a);

return 0;

+exp_setup_keys_fail:
+ gpio_free(sel_c);
exp_setup_selc_fail:
gpio_free(sel_b);
exp_setup_selb_fail:
@@ -324,6 +417,8 @@ exp_setup_sela_fail:
static int da850_evm_ui_expander_teardown(struct i2c_client *client,
unsigned gpio, unsigned ngpio, void *c)
{
+ platform_device_unregister(&da850_evm_ui_keys_device);
+
/* deselect all functionalities */
gpio_set_value_cansleep(gpio + 5, 1);
gpio_set_value_cansleep(gpio + 6, 1);
@@ -340,6 +435,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
+ .names = da850_evm_ui_exp,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
--
1.7.0.4

2010-11-19 21:37:41

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v3 3/4] da850-evm: extract defines for SEL{A,B,C} pins in UI expander

The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
pins by 'magic number' in each function. This uses the common enum for their offsets
in the expander setup and teardown functions.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>

---

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* integrated the static array initialization patch provided by Sekhar Nori

Changes since v1:
* No changes since v1

---
arch/arm/mach-davinci/board-da850-evm.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index b0763f7..c689e7e 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -361,23 +361,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + 7;
- sel_b = gpio + 6;
- sel_c = gpio + 5;
+ sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
+ sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
+ sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;

- ret = gpio_request(sel_a, "sel_a");
+ ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, "sel_b");
+ ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, "sel_c");
+ ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_C]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -420,13 +420,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&da850_evm_ui_keys_device);

/* deselect all functionalities */
- gpio_set_value_cansleep(gpio + 5, 1);
- gpio_set_value_cansleep(gpio + 6, 1);
- gpio_set_value_cansleep(gpio + 7, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_A, 1);

- gpio_free(gpio + 5);
- gpio_free(gpio + 6);
- gpio_free(gpio + 7);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);

return 0;
}
--
1.7.0.4

2010-11-19 21:37:43

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v3 4/4] da850-evm: add baseboard GPIO expander buttons, switches and LEDs

This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches and
LEDs.

Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.

The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>

---

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in 1/4 instead of this patch
(Nori, Sekhar)
* ui_expander_names was renamed to da850_evm_ui_exp
* DA850_SW_POLL_MS definition moved to this patch from 3/4
* use indexed array initialization pattern introduced by Sekhar Nori in 3/4
* shorter names prefixed with da850_evm
* static array range intializers
* using only a single gpio-keys instance for the pushbutton and switches on
baseboard since there is no advantage to separate device instances with
different polling intervals (Dmitry Torokhov)

Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver

---
arch/arm/mach-davinci/board-da850-evm.c | 225 +++++++++++++++++++++++++++++++
1 files changed, 225 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index c689e7e..32c7a02 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -431,6 +431,220 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
return 0;
}

+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(da850_evm_ui_exp)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
+
+enum da850_evm_bb_exp_pins {
+ DA850_EVM_BB_EXP_DEEP_SLEEP_EN = 0,
+ DA850_EVM_BB_EXP_SW_RST,
+ DA850_EVM_BB_EXP_TP_23,
+ DA850_EVM_BB_EXP_TP_22,
+ DA850_EVM_BB_EXP_TP_21,
+ DA850_EVM_BB_EXP_USER_PB1,
+ DA850_EVM_BB_EXP_USER_LED2,
+ DA850_EVM_BB_EXP_USER_LED1,
+ DA850_EVM_BB_EXP_USER_SW1,
+ DA850_EVM_BB_EXP_USER_SW2,
+ DA850_EVM_BB_EXP_USER_SW3,
+ DA850_EVM_BB_EXP_USER_SW4,
+ DA850_EVM_BB_EXP_USER_SW5,
+ DA850_EVM_BB_EXP_USER_SW6,
+ DA850_EVM_BB_EXP_USER_SW7,
+ DA850_EVM_BB_EXP_USER_SW8
+};
+
+static const char const *da850_evm_bb_exp[] = {
+ [DA850_EVM_BB_EXP_DEEP_SLEEP_EN] = "deep_sleep_en",
+ [DA850_EVM_BB_EXP_SW_RST] = "sw_rst",
+ [DA850_EVM_BB_EXP_TP_23] = "tp_23",
+ [DA850_EVM_BB_EXP_TP_22] = "tp_22",
+ [DA850_EVM_BB_EXP_TP_21] = "tp_21",
+ [DA850_EVM_BB_EXP_USER_PB1] = "user_pb1",
+ [DA850_EVM_BB_EXP_USER_LED2] = "user_led2",
+ [DA850_EVM_BB_EXP_USER_LED1] = "user_led1",
+ [DA850_EVM_BB_EXP_USER_SW1] = "user_sw1",
+ [DA850_EVM_BB_EXP_USER_SW2] = "user_sw2",
+ [DA850_EVM_BB_EXP_USER_SW3] = "user_sw3",
+ [DA850_EVM_BB_EXP_USER_SW4] = "user_sw4",
+ [DA850_EVM_BB_EXP_USER_SW5] = "user_sw5",
+ [DA850_EVM_BB_EXP_USER_SW6] = "user_sw6",
+ [DA850_EVM_BB_EXP_USER_SW7] = "user_sw7",
+ [DA850_EVM_BB_EXP_USER_SW8] = "user_sw8",
+};
+
+#define DA850_N_BB_USER_SW 8
+
+static struct gpio_keys_button da850_evm_bb_keys[] = {
+ [0] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = KEY_PROG1,
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+ [1 ... DA850_N_BB_USER_SW] = {
+ .type = EV_SW,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_bb_keys_pdata = {
+ .buttons = da850_evm_bb_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_bb_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_bb_keys_device = {
+ .name = "gpio-keys",
+ .id = 1,
+ .dev = {
+ .platform_data = &da850_evm_bb_keys_pdata
+ },
+};
+
+static void da850_evm_bb_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ button = &da850_evm_bb_keys[0];
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_PB1];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_PB1;
+
+ for (i = 0; i < DA850_N_BB_USER_SW; i++) {
+ button = &da850_evm_bb_keys[i + 1];
+ button->code = SW_LID + i;
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_SW1 + i];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_SW1 + i;
+ }
+}
+
+#define DA850_N_BB_USER_LED 2
+
+static struct gpio_led da850_evm_bb_leds[] = {
+ [0 ... DA850_N_BB_USER_LED - 1] = {
+ .active_low = 1,
+ .gpio = -1, /* assigned at runtime */
+ .name = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_led_platform_data da850_evm_bb_leds_pdata = {
+ .leds = da850_evm_bb_leds,
+ .num_leds = ARRAY_SIZE(da850_evm_bb_leds),
+};
+
+static struct platform_device da850_evm_bb_leds_device = {
+ .name = "leds-gpio",
+ .id = -1,
+ .dev = {
+ .platform_data = &da850_evm_bb_leds_pdata
+ }
+};
+
+static void da850_evm_bb_leds_init(unsigned gpio)
+{
+ int i;
+ struct gpio_led *led;
+
+ for (i = 0; i < DA850_N_BB_USER_LED; i++) {
+ led = &da850_evm_bb_leds[i];
+
+ led->gpio = gpio + DA850_EVM_BB_EXP_USER_LED2 + i;
+ led->name =
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_LED2 + i];
+ }
+}
+
+static int da850_evm_bb_expander_setup(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *c)
+{
+ int ret;
+ int deep_sleep_en, sw_rst;
+
+ deep_sleep_en = gpio + DA850_EVM_BB_EXP_DEEP_SLEEP_EN;
+ sw_rst = gpio + DA850_EVM_BB_EXP_SW_RST;
+
+ /* Do not allow sysfs control of deep_sleep_en */
+ ret = gpio_request(deep_sleep_en,
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_DEEP_SLEEP_EN]);
+ if (ret) {
+ pr_warning("Cannot open IO expander pin %d\n", deep_sleep_en);
+ goto io_exp_setup_deep_sleep_en_fail;
+ }
+ /* do not drive a value on deep_sleep_en */
+ gpio_direction_input(deep_sleep_en);
+
+ /* Do not allow sysfs control of sw_rst */
+ ret = gpio_request(sw_rst,
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_SW_RST]);
+ if (ret) {
+ pr_warning("Cannot open IO expander pin %d\n", sw_rst);
+ goto io_exp_setup_sw_rst_fail;
+ }
+ /* do not drive a value on sw_rst */
+ gpio_direction_input(sw_rst);
+
+ /*
+ * Register the switches and pushbutton on the baseboard as a gpio-keys
+ * device.
+ */
+ da850_evm_bb_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_keys_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander switches"
+ " device\n");
+ goto io_exp_setup_sw_fail;
+ }
+
+ da850_evm_bb_leds_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_leds_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander LEDS "
+ "device\n");
+ goto io_exp_setup_leds_fail;
+ }
+
+ return 0;
+
+io_exp_setup_leds_fail:
+ platform_device_unregister(&da850_evm_bb_keys_device);
+io_exp_setup_sw_fail:
+ gpio_free(sw_rst);
+io_exp_setup_sw_rst_fail:
+ gpio_free(deep_sleep_en);
+io_exp_setup_deep_sleep_en_fail:
+ return ret;
+}
+
+static int da850_evm_bb_expander_teardown(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio, void *c)
+{
+ int deep_sleep_en, sw_rst;
+
+ deep_sleep_en = gpio + DA850_EVM_BB_EXP_DEEP_SLEEP_EN;
+ sw_rst = gpio + DA850_EVM_BB_EXP_SW_RST;
+
+ platform_device_unregister(&da850_evm_bb_leds_device);
+ platform_device_unregister(&da850_evm_bb_keys_device);
+ gpio_free(sw_rst);
+ gpio_free(deep_sleep_en);
+
+ return 0;
+}
+
static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
@@ -438,6 +652,13 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.names = da850_evm_ui_exp,
};

+static struct pca953x_platform_data da850_evm_bb_expander_info = {
+ .gpio_base = DA850_BB_EXPANDER_GPIO_BASE,
+ .setup = da850_evm_bb_expander_setup,
+ .teardown = da850_evm_bb_expander_teardown,
+ .names = da850_evm_bb_exp,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -446,6 +667,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
},
+ {
+ I2C_BOARD_INFO("tca6416", 0x21),
+ .platform_data = &da850_evm_bb_expander_info,
+ },
};

static struct davinci_i2c_platform_data da850_evm_i2c_0_pdata = {
--
1.7.0.4

2010-11-19 21:37:31

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v3 0/4] da850-evm: add gpio-{keys,leds} for UI and BB expanders

The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
They are bootstrapped to different I2C addresses so they can be used
concurrently.

The expander on the UI board is currently used to enable/disable the
peripherals that are available on the UI board. In addition to this
functionality the expander is also connected to 8 pushbuttons. The expander
on the baseboard is not currently used; it is connected to deep sleep enable,
sw reset, a push button, some switches and LEDs.

This proposed patch series enables the push buttons and switches on the UI and
BB expanders using the gpio-keys polling mode patch by Alexander Clouter. Some
work was performed to test irq-based gpio-keys support on the expanders (a WIP
patch can be posted on request) but I believe that it is not possible to use
irq-based gpio-keys on IO expanders for arm systems at this time.

The attempt started when I noticed the patch of Alek Du and Alan Cox [1] which
was recently committed [2]; a stab at integrating irq-based gpio-keys support
based on that patch was attempted. I found that I either got a warning that the
irq could not be mapped for the given gpio ; or, when N_IRQ was increased, a
system freeze.

>From what I have read (particularly the message by Grant Likely [3]) IRQs on
IO expanders are not ready in ARM yet. I _think_ that the sparse IRQ rework by
Thomas Gleixner [4] will resolve the blocker to irq-based gpio-keys support.

In the meantime we have buttons and switches that we would like to excersise
in our prototyping development. The patch to convert this series to irq-based
gpio-keys will be straighforward once the support in arch/arm is there.

There is an existing tca6416-keypad driver with polling support which I did not
employ because it isn't possible to keep the gpio's used for peripheral
enable/disable on the UI board or the LEDs on the baseboard registered while
simultaneously registering the pushbuttons or switches as a tca6416-keypad
instance.

I tested this patch series using evtest on the resulting /dev/input/eventN
devices and also on the event node of a non-polling gpio-keys instance to
ensure that irq-based input handling is not broken by the introduction of the
polling-mode gpio-keys patch. The non-polling instance creation and
registration is not included in this series since it uses one of the boot-mode
DIP switches and woult not (I think) be suitable for mainline.

Disclaimer:
I'm not an expert in irq's or gpio-keys; this is, in fact, my first proposed
feature. Please feel free to correct me -- I welcome the chance to learn from
your expertise.

[1] http://article.gmane.org/gmane.linux.kernel/1052551
[2] http://article.gmane.org/gmane.linux.kernel.commits.head/260919
[3] http://www.mail-archive.com/[email protected]/msg01974.html
[4] http://article.gmane.org/gmane.linux.kernel.cross-arch/7786

Alexander Clouter (1):
input: gpio_keys: polling mode support

Ben Gardiner (3):
da850-evm: add UI Expander pushbuttons
da850-evm: extract defines for SEL{A,B,C} pins in UI expander
da850-evm: add baseboard GPIO expander buttons, switches and LEDs

arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 347 +++++++++++++++++++++++++++++--
drivers/input/keyboard/gpio_keys.c | 120 +++++++++--
include/linux/gpio_keys.h | 1 +
4 files changed, 440 insertions(+), 31 deletions(-)

---
Changes since v2:
* register a single input device for switches and keys on the baseboard since
there is no benefit to separate devices with different polling intervals
(Dmitry Torokhov)
* use static array intialization and range intialization for platform data
structure to minimize the amount of runtime intialization needed:
(Sekhar Nori)
* Use the da850_evm variable name prefix for static symbols in
board-da850-evm.c

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used

2010-11-19 21:38:11

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v3 1/4] input: gpio_keys: polling mode support

From: Alexander Clouter <[email protected]>

This implements an optional polling mode for the gpio_keys driver,
necessary for GPIOs that are not able to generate IRQs.

gpio_keys_platform_data has been extended with poll_interval
which specifies the polling interval in ms, this is passed onto
input-polldev.

This work is a rebase of the patch by Alex Clouter [1] which was
based on the patch [2] originally conceived by Paul Mundt.

Signed-off-by: Paul Mundt <[email protected]>
Signed-off-by: Alexander Clouter <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Paul Mundt <[email protected]>

[1] http://article.gmane.org/gmane.linux.kernel.input/13919
[2] http://article.gmane.org/gmane.linux.kernel.input/5814

---

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin

Changes since [1]:
* rebased to 0b1c3ef1072f2b97c86351d3736d2b2d00293a11 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* use _cansleep variant of gpio_get_value in the polling task
to avoid WARN_ON when using I2C GPIO expanders
* prevent unitialized access to 'input' in gpio_keys_close()

Changes since [2]:
* absolute dependency on INPUT_POLLDEV removed

Tested with CONFIG_INPUT_POLLDEV={n,m,y} (gpio_keys as module for 'm').

---
drivers/input/keyboard/gpio_keys.c | 120 ++++++++++++++++++++++++++++++------
include/linux/gpio_keys.h | 1 +
2 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6069abe..d2f23d9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -1,7 +1,9 @@
/*
- * Driver for keys on GPIO lines capable of generating interrupts.
+ * Driver for keys on GPIO lines, either IRQ-driven or polled.
*
* Copyright 2005 Phil Blundell
+ * Copyright 2008 Paul Mundt
+ * Copyright 2010 Alexander Clouter <[email protected]>
*
* 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
@@ -25,6 +27,7 @@
#include <linux/gpio_keys.h>
#include <linux/workqueue.h>
#include <linux/gpio.h>
+#include <linux/input-polldev.h>

struct gpio_button_data {
struct gpio_keys_button *button;
@@ -37,6 +40,7 @@ struct gpio_button_data {

struct gpio_keys_drvdata {
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
struct mutex disable_lock;
unsigned int n_buttons;
int (*enable)(struct device *dev);
@@ -44,6 +48,30 @@ struct gpio_keys_drvdata {
struct gpio_button_data data[0];
};

+#if (!defined(CONFIG_INPUT_POLLDEV) && !defined(CONFIG_INPUT_POLLDEV_MODULE)) \
+ || (defined(CONFIG_INPUT_POLLDEV_MODULE) \
+ && !defined(CONFIG_KEYBOARD_GPIO_MODULE))
+
+static inline struct input_polled_dev *allocate_polled_device(
+ const struct device *dev)
+{
+ dev_err(dev, "device needs polling (enable INPUT_POLLDEV)\n");
+ return NULL;
+}
+
+#define free_polled_device(x) do { } while (0)
+#define register_polled_device(x) (-ENXIO)
+#define unregister_polled_device(x) do { } while (0)
+
+#else
+
+#define allocate_polled_device(x) input_allocate_polled_device()
+#define free_polled_device(x) input_free_polled_device(x)
+#define register_polled_device(x) input_register_polled_device(x)
+#define unregister_polled_device(x) input_unregister_polled_device(x)
+
+#endif
+
/*
* SYSFS interface for enabling/disabling keys and switches:
*
@@ -322,7 +350,8 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
struct gpio_keys_button *button = bdata->button;
struct input_dev *input = bdata->input;
unsigned int type = button->type ?: EV_KEY;
- int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
+ int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0)
+ ^ button->active_low;

input_event(input, type, button->code, !!state);
input_sync(input);
@@ -343,6 +372,16 @@ static void gpio_keys_timer(unsigned long _data)
schedule_work(&data->work);
}

+static void gpio_handle_button_event(struct gpio_keys_button *button,
+ struct gpio_button_data *bdata)
+{
+ if (bdata->timer_debounce)
+ mod_timer(&bdata->timer,
+ jiffies + msecs_to_jiffies(bdata->timer_debounce));
+ else
+ gpio_keys_report_event(bdata);
+}
+
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
@@ -350,15 +389,24 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)

BUG_ON(irq != gpio_to_irq(button->gpio));

- if (bdata->timer_debounce)
- mod_timer(&bdata->timer,
- jiffies + msecs_to_jiffies(bdata->timer_debounce));
- else
- schedule_work(&bdata->work);
+ gpio_handle_button_event(button, bdata);

return IRQ_HANDLED;
}

+static void gpio_keys_poll(struct input_polled_dev *dev)
+{
+ struct gpio_keys_drvdata *ddata = dev->private;
+ int i;
+
+ for (i = 0; i < ddata->n_buttons; i++) {
+ struct gpio_button_data *bdata = &ddata->data[i];
+ struct gpio_keys_button *button = bdata->button;
+
+ gpio_handle_button_event(button, bdata);
+ }
+}
+
static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
struct gpio_button_data *bdata,
struct gpio_keys_button *button)
@@ -446,20 +494,28 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
struct gpio_keys_drvdata *ddata;
struct device *dev = &pdev->dev;
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i, error;
int wakeup = 0;

ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
pdata->nbuttons * sizeof(struct gpio_button_data),
GFP_KERNEL);
- input = input_allocate_device();
+ if (pdata->poll_interval) {
+ poll_dev = allocate_polled_device(dev);
+ input = poll_dev ? poll_dev->input : 0;
+ } else
+ input = input_allocate_device();
if (!ddata || !input) {
dev_err(dev, "failed to allocate state\n");
error = -ENOMEM;
goto fail1;
}

- ddata->input = input;
+ if (pdata->poll_interval)
+ ddata->poll_dev = poll_dev;
+ else
+ ddata->input = input;
ddata->n_buttons = pdata->nbuttons;
ddata->enable = pdata->enable;
ddata->disable = pdata->disable;
@@ -468,6 +524,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ddata);
input_set_drvdata(input, ddata);

+ if (pdata->poll_interval) {
+ poll_dev->private = ddata;
+ poll_dev->poll = gpio_keys_poll;
+ poll_dev->poll_interval = pdata->poll_interval;
+ }
+
input->name = pdev->name;
input->phys = "gpio-keys/input0";
input->dev.parent = &pdev->dev;
@@ -491,14 +553,17 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
bdata->input = input;
bdata->button = button;

+ input_set_capability(input, type, button->code);
+
+ if (pdata->poll_interval)
+ continue;
+
error = gpio_keys_setup_key(pdev, bdata, button);
if (error)
goto fail2;

if (button->wakeup)
wakeup = 1;
-
- input_set_capability(input, type, button->code);
}

error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -508,7 +573,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
goto fail2;
}

- error = input_register_device(input);
+ if (pdata->poll_interval)
+ error = register_polled_device(poll_dev);
+ else
+ error = input_register_device(input);
if (error) {
dev_err(dev, "Unable to register input device, error: %d\n",
error);
@@ -528,7 +596,9 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
fail2:
while (--i >= 0) {
- free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
+ if (!pdata->poll_interval)
+ free_irq(gpio_to_irq(pdata->buttons[i].gpio),
+ &ddata->data[i]);
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
@@ -537,7 +607,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, NULL);
fail1:
- input_free_device(input);
+ if (pdata->poll_interval)
+ free_polled_device(poll_dev);
+ else
+ input_free_device(input);
kfree(ddata);

return error;
@@ -547,7 +620,8 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
{
struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
- struct input_dev *input = ddata->input;
+ struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i;

sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -555,15 +629,25 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, 0);

for (i = 0; i < pdata->nbuttons; i++) {
- int irq = gpio_to_irq(pdata->buttons[i].gpio);
- free_irq(irq, &ddata->data[i]);
+ if (!pdata->poll_interval) {
+ int irq = gpio_to_irq(pdata->buttons[i].gpio);
+ free_irq(irq, &ddata->data[i]);
+ }
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
gpio_free(pdata->buttons[i].gpio);
}

- input_unregister_device(input);
+ if (pdata->poll_interval) {
+ poll_dev = ddata->poll_dev;
+ unregister_polled_device(poll_dev);
+ free_polled_device(poll_dev);
+ } else {
+ input = ddata->input;
+ input_unregister_device(input);
+ input_free_device(input);
+ }

return 0;
}
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ce73a30..5fdd495 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -19,6 +19,7 @@ struct gpio_keys_platform_data {
unsigned int rep:1; /* enable input subsystem auto repeat */
int (*enable)(struct device *dev);
void (*disable)(struct device *dev);
+ unsigned int poll_interval; /* polling interval in ms */
};

#endif
--
1.7.0.4

2010-11-19 21:41:14

by Victor Rodriguez

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] da850-evm: extract defines for SEL{A, B, C} pins in UI expander

On Fri, Nov 19, 2010 at 3:37 PM, Ben Gardiner
<[email protected]> wrote:
> The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
> pins by 'magic number' in each function. This uses the common enum for their offsets
> in the expander setup and teardown functions.
>
> Signed-off-by: Ben Gardiner <[email protected]>
> Reviewed-by: Chris Cordahi <[email protected]>
> Reviewed-by: Sekhar Nori <[email protected]>
> Signed-off-by: Sekhar Nori <[email protected]>
>
> ---
>
> Changes since v2:
> ?* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
> ? git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
> ?* integrated the static array initialization patch provided by Sekhar Nori
>
> Changes since v1:
> ?* No changes since v1
>
> ---

I think that ther must be just one --- , This is extra

Regards

Victor Rodriguez

> ?arch/arm/mach-davinci/board-da850-evm.c | ? 24 ++++++++++++------------
> ?1 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index b0763f7..c689e7e 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -361,23 +361,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
> ?{
> ? ? ? ?int sel_a, sel_b, sel_c, ret;
>
> - ? ? ? sel_a = gpio + 7;
> - ? ? ? sel_b = gpio + 6;
> - ? ? ? sel_c = gpio + 5;
> + ? ? ? sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
> + ? ? ? sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
> + ? ? ? sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;
>
> - ? ? ? ret = gpio_request(sel_a, "sel_a");
> + ? ? ? ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
> ? ? ? ?if (ret) {
> ? ? ? ? ? ? ? ?pr_warning("Cannot open UI expander pin %d\n", sel_a);
> ? ? ? ? ? ? ? ?goto exp_setup_sela_fail;
> ? ? ? ?}
>
> - ? ? ? ret = gpio_request(sel_b, "sel_b");
> + ? ? ? ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
> ? ? ? ?if (ret) {
> ? ? ? ? ? ? ? ?pr_warning("Cannot open UI expander pin %d\n", sel_b);
> ? ? ? ? ? ? ? ?goto exp_setup_selb_fail;
> ? ? ? ?}
>
> - ? ? ? ret = gpio_request(sel_c, "sel_c");
> + ? ? ? ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_C]);
> ? ? ? ?if (ret) {
> ? ? ? ? ? ? ? ?pr_warning("Cannot open UI expander pin %d\n", sel_c);
> ? ? ? ? ? ? ? ?goto exp_setup_selc_fail;
> @@ -420,13 +420,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
> ? ? ? ?platform_device_unregister(&da850_evm_ui_keys_device);
>
> ? ? ? ?/* deselect all functionalities */
> - ? ? ? gpio_set_value_cansleep(gpio + 5, 1);
> - ? ? ? gpio_set_value_cansleep(gpio + 6, 1);
> - ? ? ? gpio_set_value_cansleep(gpio + 7, 1);
> + ? ? ? gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
> + ? ? ? gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
> + ? ? ? gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_A, 1);
>
> - ? ? ? gpio_free(gpio + 5);
> - ? ? ? gpio_free(gpio + 6);
> - ? ? ? gpio_free(gpio + 7);
> + ? ? ? gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
> + ? ? ? gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
> + ? ? ? gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);
>
> ? ? ? ?return 0;
> ?}
> --
> 1.7.0.4
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> [email protected]
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>

2010-11-19 22:25:49

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] da850-evm: extract defines for SEL{A, B, C} pins in UI expander

Hi Victor,

On Fri, Nov 19, 2010 at 4:41 PM, Victor Rodriguez <[email protected]> wrote:
[...]
>> ---
>
> I think that ther must be just one ? --- , This is extra

Sorry, that is my bad habit. I believe that the patch should still be
usable as-is.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-22 11:49:54

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Ben,

On Fri, Nov 19, 2010 at 21:08:10, Ben Gardiner wrote:

> On Fri, Nov 19, 2010 at 4:58 AM, Nori, Sekhar <[email protected]> wrote:
> > [...]
> > On Wed, Nov 17, 2010 at 01:09:35, Ben Gardiner wrote:
> >> [...]
> >> Changes since v1:
> >> * set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
> >> unconditionally
> >
> > I didn't see the v1 posting (wonder why), but why is this
> > required? Why cant we depend on this being selected from
> > Device Drivers->Input device support in menuconfig?
>
> I'm sorry about that. I checked gmane and the v1 never made it to the
> davinci-linux list. I think it was because of my (mis)use of git
> send-email. Version 2 made it to the list so I've now found the magic
> incantation.
>
> The reason we should at least select INPUT_POLLDEV as a default option
> is: when it is not set the gpio-keys instances registered by the
> da850-evm board setup routine will fail probe() of the gpio-keys
> driver since it will not have polled-input support enabled.
>
> By setting INPUT_POLLDEV default for the da850-evm users will get
> functioning pushbuttons and switches with the default config but they
> will be able to disable INPUT_POLLDEV or gpio-keys drivers in their
> defconfig at their convenience.

I guess we could also just modify the defconfig to switch on
INPUT_POLLDEV? Since only gpio-keys functionality is affected
by not enabling the correct options in kernel, it should be OK.

Thanks,
Sekhar

2010-11-22 12:00:42

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Ben,

On Fri, Nov 19, 2010 at 21:10:50, Ben Gardiner wrote:

> >> [...]
> >
> > How does gpio_request prevent sysfs control?
>
> To obtain access to a gpio through the sysfs interface the user must
> first write the gpio number to 'export'. When the gpio has been
> gpio_request()'d the result of writing to 'export' is nothing; whereas
> writing to export would normally result in the appearance of a named
> gpio line alongside 'export'.
>
> I hope the following commands and their output illustrate my point:
>
> $ cd /sys/class/gpio/
> $ ls
> export gpiochip128 gpiochip160 gpiochip64 unexport
> gpiochip0 gpiochip144 gpiochip32 gpiochip96
> $ echo 160 > export
> $ ls
> export gpiochip128 gpiochip160 gpiochip64 unexport
> gpiochip0 gpiochip144 gpiochip32 gpiochip96
> $ echo 163 > export
> $ ls
> export gpiochip128 gpiochip160 gpiochip64 tp_22
> gpiochip0 gpiochip144 gpiochip32 gpiochip96 unexport

Thanks for the explanation. I should have probably asked
earlier, why do we need to prevent sysfs access of
deep sleep enable and sw reset pins? We don't seem to be
using them in the kernel either.

Thanks,
Sekhar

2010-11-22 13:50:57

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

On Mon, Nov 22, 2010 at 6:49 AM, Nori, Sekhar <[email protected]> wrote:
> On Fri, Nov 19, 2010 at 21:08:10, Ben Gardiner wrote:
>> [...]
>> By setting INPUT_POLLDEV default for the da850-evm users will get
>> functioning pushbuttons and switches with the default config but they
>> will be able to disable INPUT_POLLDEV or gpio-keys drivers in their
>> defconfig at their convenience.
>
> I guess we could also just modify the defconfig to switch on
> INPUT_POLLDEV? Since only gpio-keys functionality is affected
> by not enabling the correct options in kernel, it should be OK.

Yes -- only gpio-keys is affected but enabling the option does
introduce an additional .o file:

drivers/input/Makefile:obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o

I agree that in its current state a user couls inadvertently disable
the gpio-keys support on da850-evm simply by disabling INPUT_POLLDEV
-- which is less than Ideal. How about I replace the current changes
to arch/arm/mach-davinci/Kconfig with:

config KEYBOARD_GPIO
default MACH_DAVINCI_DA850_EVM
select INPUT_POLLDEV

So 1) gpio-keys functionality is default for the da850evm and 2)
whenever gpio-keys is enabled so is INPUT_POLLDEV.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-22 14:15:50

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Sekhar,

On Mon, Nov 22, 2010 at 7:00 AM, Nori, Sekhar <[email protected]> wrote:
> Thanks for the explanation. I should have probably asked
> earlier, why do we need to prevent sysfs access of
> deep sleep enable and sw reset pins? We don't seem to be
> using them in the kernel either.

You're welcome.

I was assuming that those pins were not exported as gpio pins on
purpose; I was taking the prudent approach to prevent haphazard
toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it
could initiate a reset of the cpu when toggled and deep_sleep_en
because it can override the behaviour of davinci_pm_enter().

I'm not sure how they would be used by existing kernel classes either.
The sw_rst pin could be used for reset but since it is on the other
end of an i2c bus and there is an existing implementation of reset
using the on chip watchdog I don't think it would be benficial to
switch. Deep_sleep_en could override the behaviour in
davinci_pm_enter() -- _maybe_ (I don't really know) it could be used
as a hardware-assisted suspend-blocker? But I totally guessing here.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-23 12:38:48

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Ben,

On Mon, Nov 22, 2010 at 19:20:48, Ben Gardiner wrote:
> On Mon, Nov 22, 2010 at 6:49 AM, Nori, Sekhar <[email protected]> wrote:
> > On Fri, Nov 19, 2010 at 21:08:10, Ben Gardiner wrote:
> >> [...]
> >> By setting INPUT_POLLDEV default for the da850-evm users will get
> >> functioning pushbuttons and switches with the default config but they
> >> will be able to disable INPUT_POLLDEV or gpio-keys drivers in their
> >> defconfig at their convenience.
> >
> > I guess we could also just modify the defconfig to switch on
> > INPUT_POLLDEV? Since only gpio-keys functionality is affected
> > by not enabling the correct options in kernel, it should be OK.
>
> Yes -- only gpio-keys is affected but enabling the option does
> introduce an additional .o file:
>
> drivers/input/Makefile:obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
>
> I agree that in its current state a user couls inadvertently disable
> the gpio-keys support on da850-evm simply by disabling INPUT_POLLDEV
> -- which is less than Ideal. How about I replace the current changes
> to arch/arm/mach-davinci/Kconfig with:
>
> config KEYBOARD_GPIO
> default MACH_DAVINCI_DA850_EVM
> select INPUT_POLLDEV
>
> So 1) gpio-keys functionality is default for the da850evm and 2)
> whenever gpio-keys is enabled so is INPUT_POLLDEV.

This looks better than what was posted earlier, but I am not
sure if platforms should influence driver configuration this
way.

I guess I am just afraid that this makes a precedent for
many driver config symbols to get replicated in the platform
Kconfig files.

Lets see if others have an opinion on this.

Thanks,
Sekhar

2010-11-23 12:43:05

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Ben,

On Mon, Nov 22, 2010 at 19:45:46, Ben Gardiner wrote:
> Hi Sekhar,
>
> On Mon, Nov 22, 2010 at 7:00 AM, Nori, Sekhar <[email protected]> wrote:
> > Thanks for the explanation. I should have probably asked
> > earlier, why do we need to prevent sysfs access of
> > deep sleep enable and sw reset pins? We don't seem to be
> > using them in the kernel either.
>
> You're welcome.
>
> I was assuming that those pins were not exported as gpio pins on
> purpose; I was taking the prudent approach to prevent haphazard
> toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it
> could initiate a reset of the cpu when toggled and deep_sleep_en
> because it can override the behaviour of davinci_pm_enter().
>
> I'm not sure how they would be used by existing kernel classes either.
> The sw_rst pin could be used for reset but since it is on the other
> end of an i2c bus and there is an existing implementation of reset
> using the on chip watchdog I don't think it would be benficial to
> switch. Deep_sleep_en could override the behaviour in
> davinci_pm_enter() -- _maybe_ (I don't really know) it could be used
> as a hardware-assisted suspend-blocker? But I totally guessing here.

My preference would be to leave these pins as is
(don't call a gpio_request() on them) till someone
comes up with a use case for them. From what you
described, sysfs access cannot happen "accidently"
so someone accessing these pins from sysfs surely
knows what he is doing.

Thanks,
Sekhar

2010-11-23 13:29:21

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Sekhar,

On Tue, Nov 23, 2010 at 7:38 AM, Nori, Sekhar <[email protected]> wrote:
> On Mon, Nov 22, 2010 at 19:20:48, Ben Gardiner wrote:
>> [...]
>> config KEYBOARD_GPIO
>> ? ? ? ? default MACH_DAVINCI_DA850_EVM
>> ? ? ? ? select INPUT_POLLDEV
>>
>> So 1) gpio-keys functionality is default for the da850evm and 2)
>> whenever gpio-keys is enabled so is INPUT_POLLDEV.
>
> This looks better than what was posted earlier, but I am not
> sure if platforms should influence driver configuration this
> way.
>
> I guess I am just afraid that this makes a precedent for
> many driver config symbols to get replicated in the platform
> Kconfig files.

Ok . I understand your concerns.

> Lets see if others have an opinion on this.

Yes, good idea. I would welcome more opinions on this and any other
aspects of the patch series.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-23 13:32:36

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] da850-evm: add baseboard UI expander buttons, switches and LEDs

Hi Sekhar,

On Tue, Nov 23, 2010 at 7:42 AM, Nori, Sekhar <[email protected]> wrote:
> On Mon, Nov 22, 2010 at 19:45:46, Ben Gardiner wrote:
>> [...]
>> I was assuming that those pins were not exported as gpio pins on
>> purpose; I was taking the prudent approach to prevent haphazard
>> toggling of sw_rst and deep_sleep_en from userspace. sw_rst because it
>> could initiate a reset of the cpu when toggled and deep_sleep_en
>> because it can override the behaviour of davinci_pm_enter().
>>
>> I'm not sure how they would be used by existing kernel classes either.
>> The sw_rst pin could be used for reset but since it is on the other
>> end of an i2c bus and there is an existing implementation of reset
>> using the on chip watchdog I don't think it would be benficial to
>> switch. Deep_sleep_en could override the behaviour in
>> davinci_pm_enter() -- _maybe_ (I don't really know) it could be used
>> as a hardware-assisted suspend-blocker? But I totally guessing here.
>
> My preference would be to leave these pins as is
> (don't call a gpio_request() on them) till someone
> comes up with a use case for them. From what you
> described, sysfs access cannot happen "accidently"
> so someone accessing these pins from sysfs surely
> knows what he is doing.

No problem. I will re-spin the patch shortly with the deep_sleep_en
and sw_rst pins free for use by client code.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-23 15:48:29

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

"Nori, Sekhar" <[email protected]> writes:

> Hi Ben,
>
> On Mon, Nov 22, 2010 at 19:20:48, Ben Gardiner wrote:
>> On Mon, Nov 22, 2010 at 6:49 AM, Nori, Sekhar <[email protected]> wrote:
>> > On Fri, Nov 19, 2010 at 21:08:10, Ben Gardiner wrote:
>> >> [...]
>> >> By setting INPUT_POLLDEV default for the da850-evm users will get
>> >> functioning pushbuttons and switches with the default config but they
>> >> will be able to disable INPUT_POLLDEV or gpio-keys drivers in their
>> >> defconfig at their convenience.
>> >
>> > I guess we could also just modify the defconfig to switch on
>> > INPUT_POLLDEV? Since only gpio-keys functionality is affected
>> > by not enabling the correct options in kernel, it should be OK.
>>
>> Yes -- only gpio-keys is affected but enabling the option does
>> introduce an additional .o file:
>>
>> drivers/input/Makefile:obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
>>
>> I agree that in its current state a user couls inadvertently disable
>> the gpio-keys support on da850-evm simply by disabling INPUT_POLLDEV
>> -- which is less than Ideal. How about I replace the current changes
>> to arch/arm/mach-davinci/Kconfig with:
>>
>> config KEYBOARD_GPIO
>> default MACH_DAVINCI_DA850_EVM
>> select INPUT_POLLDEV
>>
>> So 1) gpio-keys functionality is default for the da850evm and 2)
>> whenever gpio-keys is enabled so is INPUT_POLLDEV.
>
> This looks better than what was posted earlier, but I am not
> sure if platforms should influence driver configuration this
> way.

Agreed. In general, we should not have machine/platform specific
conditionals in generic Kconfigs. Generally, this should be handled in
machine/platform specific Kconfigs.

Kevin


> I guess I am just afraid that this makes a precedent for
> many driver config symbols to get replicated in the platform
> Kconfig files.
>
> Lets see if others have an opinion on this.
>
> Thanks,
> Sekhar

2010-11-23 17:58:49

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Kevin,

Thank you for weighing in.

On Tue, Nov 23, 2010 at 10:48 AM, Kevin Hilman
<[email protected]> wrote:
> "Nori, Sekhar" <[email protected]> writes:
>>> [...]
>>> -- which is less than Ideal. How about I replace the current changes
>>> to arch/arm/mach-davinci/Kconfig with:
>>>
>>> config KEYBOARD_GPIO
>>> ? ? ? ? default MACH_DAVINCI_DA850_EVM
>>> ? ? ? ? select INPUT_POLLDEV
>>>
>>> So 1) gpio-keys functionality is default for the da850evm and 2)
>>> whenever gpio-keys is enabled so is INPUT_POLLDEV.
>>
>> This looks better than what was posted earlier, but I am not
>> sure if platforms should influence driver configuration this
>> way.
>
> Agreed. ?In general, we should not have machine/platform specific
> conditionals in generic Kconfigs. ? Generally, this should be handled in
> machine/platform specific Kconfigs.

My understanding is that Sekhar was expressing concern over putting
generic config conditionals into machine/platform specific Kconfigs.
It sounds like you are OK with generic config conditionals in
machine/platform specific Kconfigs ala recent pca953x module build
changes [1].

To be clear: I am proposing the following additions to
arch/arm/mach-davinci/Kconfig: (slightly different than above).

config KEYBOARD_GPIO
? ? ? ? default MACH_DAVINCI_DA850_EVM
? ? ? ? select INPUT_POLLDEV if MACH_DAVINCI_DA850_EVM

I know it is always better to show the code: I will extract these
contentious Kconfig changes into their own patch in the series when I
post a new version.

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.linux.davinci/20918

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-23 19:41:07

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
They are bootstrapped to different I2C addresses so they can be used
concurrently.

The expander on the UI board is currently used to enable/disable the
peripherals that are available on the UI board. In addition to this
functionality the expander is also connected to 8 pushbuttons. The expander
on the baseboard is not currently used; it is connected to deep sleep enable,
sw reset, a push button, some switches and LEDs.

This proposed patch series enables the push buttons and switches on the UI and
BB expanders using the gpio-keys polling mode patch by Alexander Clouter. Some
work was performed to test irq-based gpio-keys support on the expanders (a WIP
patch can be posted on request) but I believe that it is not possible to use
irq-based gpio-keys on IO expanders for arm systems at this time.

The attempt started when I noticed the patch of Alek Du and Alan Cox [1] which
was recently committed [2]; a stab at integrating irq-based gpio-keys support
based on that patch was attempted. I found that I either got a warning that the
irq could not be mapped for the given gpio ; or, when N_IRQ was increased, a
system freeze.

>From what I have read (particularly the message by Grant Likely [3]) IRQs on
IO expanders are not ready in ARM yet. I _think_ that the sparse IRQ rework by
Thomas Gleixner [4] will resolve the blocker to irq-based gpio-keys support.

In the meantime we have buttons and switches that we would like to excersise
in our prototyping development. The patch to convert this series to irq-based
gpio-keys will be straighforward once the support in arch/arm is there.

There is an existing tca6416-keypad driver with polling support which I did not
employ because it isn't possible to keep the gpio's used for peripheral
enable/disable on the UI board or the LEDs on the baseboard registered while
simultaneously registering the pushbuttons or switches as a tca6416-keypad
instance.

I tested this patch series using evtest on the resulting /dev/input/eventN
devices and also on the event node of a non-polling gpio-keys instance to
ensure that irq-based input handling is not broken by the introduction of the
polling-mode gpio-keys patch. The non-polling instance creation and
registration is not included in this series since it uses one of the boot-mode
DIP switches and woult not (I think) be suitable for mainline.

Disclaimer:
I'm not an expert in irq's or gpio-keys; this is, in fact, my first proposed
feature. Please feel free to correct me -- I welcome the chance to learn from
your expertise.

[1] http://article.gmane.org/gmane.linux.kernel/1052551
[2] http://article.gmane.org/gmane.linux.kernel.commits.head/260919
[3] http://www.mail-archive.com/[email protected]/msg01974.html
[4] http://article.gmane.org/gmane.linux.kernel.cross-arch/7786

Alexander Clouter (1):
input: gpio_keys: polling mode support

Ben Gardiner (4):
da850-evm: add UI Expander pushbuttons
da850-evm: extract defines for SEL{A,B,C} pins in UI expander
da850-evm: add baseboard GPIO expander buttons, switches and LEDs
da850-evm: KEYBOARD_GPIO and INPUT_POLLDEV Kconfig conditionals

arch/arm/mach-davinci/Kconfig | 4 +
arch/arm/mach-davinci/board-da850-evm.c | 312 +++++++++++++++++++++++++++++--
drivers/input/keyboard/gpio_keys.c | 120 ++++++++++--
include/linux/gpio_keys.h | 1 +
4 files changed, 406 insertions(+), 31 deletions(-)

---

Changes since v3:
* introduced patch 5 in the series by extracting the Kconfig changes proposed
in patch 2 of v3.
* not gpio_request()'ing the sw_rst and deep_sleep_en lines as requested
(Sekhar Nori)

Changes since v2:
* register a single input device for switches and keys on the baseboard since
there is no benefit to separate devices with different polling intervals
(Dmitry Torokhov)
* use static array intialization and range intialization for platform data
structure to minimize the amount of runtime intialization needed:
(Sekhar Nori)
* Use the da850_evm variable name prefix for static symbols in
board-da850-evm.c

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used

2010-11-23 19:41:10

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 2/5] da850-evm: add UI Expander pushbuttons

This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
gpio-key device.

The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
disable the peripherals found on the UI board in addition to the 8 pushbuttons
mentioned above. The reason the existing tca6416-keypad driver is not employed
is because there was no aparent way to keep the gpio lines used as
SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
tca6416-keypad instance.

Some experimentation with the polling interval was performed; we were searching
for the largest polling interval that did not affect the feel of the
responsiveness of the buttons. It is very subjective but 200ms seems to be a
good value that accepts firm pushes but rejects very light ones. The key values
assigned to the buttons were arbitrarily chosen to be F1-F8.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>

---

Changes since v3:
* extracted Kconfig changes to patch 5/5
* fixed leading whitespace problem

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in this patch instead of 4/4
(Nori, Sekhar)
* integrated the static array initialization patch of Sekhar Nori
* use static array initialization ranges
* rename DA850_PB_POLL_MS to DA850_GPIO_KEYS_POLL_MS
* use shorter names prefixed with da850_evm

Changes since v1:
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver

---
arch/arm/mach-davinci/board-da850-evm.c | 98 ++++++++++++++++++++++++++++++-
1 files changed, 97 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index f89b0b7..51f5ffa 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -17,8 +17,10 @@
#include <linux/i2c.h>
#include <linux/i2c/at24.h>
#include <linux/i2c/pca953x.h>
+#include <linux/input.h>
#include <linux/mfd/tps6507x.h>
#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
#include <linux/platform_device.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -272,6 +274,88 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel)
static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
#endif

+
+#define DA850_KEYS_DEBOUNCE_MS 10
+/*
+ * At 200ms polling interval it is possible to miss an
+ * event by tapping very lightly on the push button but most
+ * pushes do result in an event; longer intervals require the
+ * user to hold the button whereas shorter intervals require
+ * more CPU time for polling.
+ */
+#define DA850_GPIO_KEYS_POLL_MS 200
+
+enum da850_evm_ui_exp_pins {
+ DA850_EVM_UI_EXP_SEL_C = 5,
+ DA850_EVM_UI_EXP_SEL_B,
+ DA850_EVM_UI_EXP_SEL_A,
+ DA850_EVM_UI_EXP_PB8,
+ DA850_EVM_UI_EXP_PB7,
+ DA850_EVM_UI_EXP_PB6,
+ DA850_EVM_UI_EXP_PB5,
+ DA850_EVM_UI_EXP_PB4,
+ DA850_EVM_UI_EXP_PB3,
+ DA850_EVM_UI_EXP_PB2,
+ DA850_EVM_UI_EXP_PB1,
+};
+
+static const char const *da850_evm_ui_exp[] = {
+ [DA850_EVM_UI_EXP_SEL_C] = "sel_c",
+ [DA850_EVM_UI_EXP_SEL_B] = "sel_b",
+ [DA850_EVM_UI_EXP_SEL_A] = "sel_a",
+ [DA850_EVM_UI_EXP_PB8] = "pb8",
+ [DA850_EVM_UI_EXP_PB7] = "pb7",
+ [DA850_EVM_UI_EXP_PB6] = "pb6",
+ [DA850_EVM_UI_EXP_PB5] = "pb5",
+ [DA850_EVM_UI_EXP_PB4] = "pb4",
+ [DA850_EVM_UI_EXP_PB3] = "pb3",
+ [DA850_EVM_UI_EXP_PB2] = "pb2",
+ [DA850_EVM_UI_EXP_PB1] = "pb1",
+};
+
+#define DA850_N_UI_PB 8
+
+static struct gpio_keys_button da850_evm_ui_keys[] = {
+ [0 ... DA850_N_UI_PB - 1] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_ui_keys_pdata = {
+ .buttons = da850_evm_ui_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_ui_keys_device = {
+ .name = "gpio-keys",
+ .id = 0,
+ .dev = {
+ .platform_data = &da850_evm_ui_keys_pdata
+ },
+};
+
+static void da850_evm_ui_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_UI_PB; i++) {
+ button = &da850_evm_ui_keys[i];
+ button->code = KEY_F8 - i;
+ button->desc = (char *)
+ da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i];
+ button->gpio = gpio + DA850_EVM_UI_EXP_PB8 + i;
+ }
+}
+
static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
unsigned ngpio, void *c)
{
@@ -304,15 +388,24 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
gpio_direction_output(sel_b, 1);
gpio_direction_output(sel_c, 1);

+ da850_evm_ui_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_ui_keys_device);
+ if (ret) {
+ pr_warning("Could not register UI GPIO expander push-buttons"
+ " device\n");
+ goto exp_setup_keys_fail;
+ }
+
ui_card_detected = 1;
pr_info("DA850/OMAP-L138 EVM UI card detected\n");

da850_evm_setup_nor_nand();
-
da850_evm_setup_emac_rmii(sel_a);

return 0;

+exp_setup_keys_fail:
+ gpio_free(sel_c);
exp_setup_selc_fail:
gpio_free(sel_b);
exp_setup_selb_fail:
@@ -324,6 +417,8 @@ exp_setup_sela_fail:
static int da850_evm_ui_expander_teardown(struct i2c_client *client,
unsigned gpio, unsigned ngpio, void *c)
{
+ platform_device_unregister(&da850_evm_ui_keys_device);
+
/* deselect all functionalities */
gpio_set_value_cansleep(gpio + 5, 1);
gpio_set_value_cansleep(gpio + 6, 1);
@@ -340,6 +435,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
+ .names = da850_evm_ui_exp,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
--
1.7.0.4

2010-11-23 19:41:15

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 1/5] input: gpio_keys: polling mode support

From: Alexander Clouter <[email protected]>

This implements an optional polling mode for the gpio_keys driver,
necessary for GPIOs that are not able to generate IRQs.

gpio_keys_platform_data has been extended with poll_interval
which specifies the polling interval in ms, this is passed onto
input-polldev.

This work is a rebase of the patch by Alex Clouter [1] which was
based on the patch [2] originally conceived by Paul Mundt.

Signed-off-by: Paul Mundt <[email protected]>
Signed-off-by: Alexander Clouter <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Paul Mundt <[email protected]>

[1] http://article.gmane.org/gmane.linux.kernel.input/13919
[2] http://article.gmane.org/gmane.linux.kernel.input/5814

---

Changes since v3:
* no changes to this patch in the series

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin

Changes since [1]:
* rebased to 0b1c3ef1072f2b97c86351d3736d2b2d00293a11 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* use _cansleep variant of gpio_get_value in the polling task
to avoid WARN_ON when using I2C GPIO expanders
* prevent unitialized access to 'input' in gpio_keys_close()

Changes since [2]:
* absolute dependency on INPUT_POLLDEV removed

Tested with CONFIG_INPUT_POLLDEV={n,m,y} (gpio_keys as module for 'm').

---
drivers/input/keyboard/gpio_keys.c | 120 ++++++++++++++++++++++++++++++------
include/linux/gpio_keys.h | 1 +
2 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6069abe..d2f23d9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -1,7 +1,9 @@
/*
- * Driver for keys on GPIO lines capable of generating interrupts.
+ * Driver for keys on GPIO lines, either IRQ-driven or polled.
*
* Copyright 2005 Phil Blundell
+ * Copyright 2008 Paul Mundt
+ * Copyright 2010 Alexander Clouter <[email protected]>
*
* 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
@@ -25,6 +27,7 @@
#include <linux/gpio_keys.h>
#include <linux/workqueue.h>
#include <linux/gpio.h>
+#include <linux/input-polldev.h>

struct gpio_button_data {
struct gpio_keys_button *button;
@@ -37,6 +40,7 @@ struct gpio_button_data {

struct gpio_keys_drvdata {
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
struct mutex disable_lock;
unsigned int n_buttons;
int (*enable)(struct device *dev);
@@ -44,6 +48,30 @@ struct gpio_keys_drvdata {
struct gpio_button_data data[0];
};

+#if (!defined(CONFIG_INPUT_POLLDEV) && !defined(CONFIG_INPUT_POLLDEV_MODULE)) \
+ || (defined(CONFIG_INPUT_POLLDEV_MODULE) \
+ && !defined(CONFIG_KEYBOARD_GPIO_MODULE))
+
+static inline struct input_polled_dev *allocate_polled_device(
+ const struct device *dev)
+{
+ dev_err(dev, "device needs polling (enable INPUT_POLLDEV)\n");
+ return NULL;
+}
+
+#define free_polled_device(x) do { } while (0)
+#define register_polled_device(x) (-ENXIO)
+#define unregister_polled_device(x) do { } while (0)
+
+#else
+
+#define allocate_polled_device(x) input_allocate_polled_device()
+#define free_polled_device(x) input_free_polled_device(x)
+#define register_polled_device(x) input_register_polled_device(x)
+#define unregister_polled_device(x) input_unregister_polled_device(x)
+
+#endif
+
/*
* SYSFS interface for enabling/disabling keys and switches:
*
@@ -322,7 +350,8 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
struct gpio_keys_button *button = bdata->button;
struct input_dev *input = bdata->input;
unsigned int type = button->type ?: EV_KEY;
- int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
+ int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0)
+ ^ button->active_low;

input_event(input, type, button->code, !!state);
input_sync(input);
@@ -343,6 +372,16 @@ static void gpio_keys_timer(unsigned long _data)
schedule_work(&data->work);
}

+static void gpio_handle_button_event(struct gpio_keys_button *button,
+ struct gpio_button_data *bdata)
+{
+ if (bdata->timer_debounce)
+ mod_timer(&bdata->timer,
+ jiffies + msecs_to_jiffies(bdata->timer_debounce));
+ else
+ gpio_keys_report_event(bdata);
+}
+
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
@@ -350,15 +389,24 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)

BUG_ON(irq != gpio_to_irq(button->gpio));

- if (bdata->timer_debounce)
- mod_timer(&bdata->timer,
- jiffies + msecs_to_jiffies(bdata->timer_debounce));
- else
- schedule_work(&bdata->work);
+ gpio_handle_button_event(button, bdata);

return IRQ_HANDLED;
}

+static void gpio_keys_poll(struct input_polled_dev *dev)
+{
+ struct gpio_keys_drvdata *ddata = dev->private;
+ int i;
+
+ for (i = 0; i < ddata->n_buttons; i++) {
+ struct gpio_button_data *bdata = &ddata->data[i];
+ struct gpio_keys_button *button = bdata->button;
+
+ gpio_handle_button_event(button, bdata);
+ }
+}
+
static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
struct gpio_button_data *bdata,
struct gpio_keys_button *button)
@@ -446,20 +494,28 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
struct gpio_keys_drvdata *ddata;
struct device *dev = &pdev->dev;
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i, error;
int wakeup = 0;

ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
pdata->nbuttons * sizeof(struct gpio_button_data),
GFP_KERNEL);
- input = input_allocate_device();
+ if (pdata->poll_interval) {
+ poll_dev = allocate_polled_device(dev);
+ input = poll_dev ? poll_dev->input : 0;
+ } else
+ input = input_allocate_device();
if (!ddata || !input) {
dev_err(dev, "failed to allocate state\n");
error = -ENOMEM;
goto fail1;
}

- ddata->input = input;
+ if (pdata->poll_interval)
+ ddata->poll_dev = poll_dev;
+ else
+ ddata->input = input;
ddata->n_buttons = pdata->nbuttons;
ddata->enable = pdata->enable;
ddata->disable = pdata->disable;
@@ -468,6 +524,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ddata);
input_set_drvdata(input, ddata);

+ if (pdata->poll_interval) {
+ poll_dev->private = ddata;
+ poll_dev->poll = gpio_keys_poll;
+ poll_dev->poll_interval = pdata->poll_interval;
+ }
+
input->name = pdev->name;
input->phys = "gpio-keys/input0";
input->dev.parent = &pdev->dev;
@@ -491,14 +553,17 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
bdata->input = input;
bdata->button = button;

+ input_set_capability(input, type, button->code);
+
+ if (pdata->poll_interval)
+ continue;
+
error = gpio_keys_setup_key(pdev, bdata, button);
if (error)
goto fail2;

if (button->wakeup)
wakeup = 1;
-
- input_set_capability(input, type, button->code);
}

error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -508,7 +573,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
goto fail2;
}

- error = input_register_device(input);
+ if (pdata->poll_interval)
+ error = register_polled_device(poll_dev);
+ else
+ error = input_register_device(input);
if (error) {
dev_err(dev, "Unable to register input device, error: %d\n",
error);
@@ -528,7 +596,9 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
fail2:
while (--i >= 0) {
- free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
+ if (!pdata->poll_interval)
+ free_irq(gpio_to_irq(pdata->buttons[i].gpio),
+ &ddata->data[i]);
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
@@ -537,7 +607,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, NULL);
fail1:
- input_free_device(input);
+ if (pdata->poll_interval)
+ free_polled_device(poll_dev);
+ else
+ input_free_device(input);
kfree(ddata);

return error;
@@ -547,7 +620,8 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
{
struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
- struct input_dev *input = ddata->input;
+ struct input_dev *input;
+ struct input_polled_dev *poll_dev;
int i;

sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
@@ -555,15 +629,25 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, 0);

for (i = 0; i < pdata->nbuttons; i++) {
- int irq = gpio_to_irq(pdata->buttons[i].gpio);
- free_irq(irq, &ddata->data[i]);
+ if (!pdata->poll_interval) {
+ int irq = gpio_to_irq(pdata->buttons[i].gpio);
+ free_irq(irq, &ddata->data[i]);
+ }
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
gpio_free(pdata->buttons[i].gpio);
}

- input_unregister_device(input);
+ if (pdata->poll_interval) {
+ poll_dev = ddata->poll_dev;
+ unregister_polled_device(poll_dev);
+ free_polled_device(poll_dev);
+ } else {
+ input = ddata->input;
+ input_unregister_device(input);
+ input_free_device(input);
+ }

return 0;
}
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ce73a30..5fdd495 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -19,6 +19,7 @@ struct gpio_keys_platform_data {
unsigned int rep:1; /* enable input subsystem auto repeat */
int (*enable)(struct device *dev);
void (*disable)(struct device *dev);
+ unsigned int poll_interval; /* polling interval in ms */
};

#endif
--
1.7.0.4

2010-11-23 19:41:21

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 5/5] da850-evm: KEYBOARD_GPIO and INPUT_POLLDEV Kconfig conditionals

Use the mach-davinci/Kconfig to enable gpio-keys as default when da850-evm
machine is enabled and to also select INPUT_POLLDEV when gpio-keys is enabled.

INPUT_POLLDEV needs to be enabled for gpio-keys devices to function properly
on da850-evm.

Signed-off-by: Ben Gardiner <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: "Nori, Sekhar" <[email protected]>

---

Changes since v3:
* no changes in this patch of the series / this patch was introduced in v4 of
the series

---
arch/arm/mach-davinci/Kconfig | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index 84066e8..70d1758 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -180,6 +180,10 @@ endchoice
config GPIO_PCA953X
default MACH_DAVINCI_DA850_EVM

+config KEYBOARD_GPIO
+ default MACH_DAVINCI_DA850_EVM
+ select INPUT_POLLDEV if MACH_DAVINCI_DA850_EVM
+
config MACH_TNETV107X
bool "TI TNETV107X Reference Platform"
default ARCH_DAVINCI_TNETV107X
--
1.7.0.4

2010-11-23 19:41:29

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 4/5] da850-evm: add baseboard GPIO expander buttons, switches and LEDs

This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches and
LEDs.

Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.

The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>

---

Changes since v3:
* don't request sw_rst and deep_sleep_en gpio pins -- let clients use them
freely

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in 1/4 instead of this patch
(Nori, Sekhar)
* ui_expander_names was renamed to da850_evm_ui_exp
* DA850_SW_POLL_MS definition moved to this patch from 3/4
* use indexed array initialization pattern introduced by Sekhar Nori in 3/4
* shorter names prefixed with da850_evm
* static array range intializers
* using only a single gpio-keys instance for the pushbutton and switches on
baseboard since there is no advantage to separate device instances with
different polling intervals (Dmitry Torokhov)

Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver

---
arch/arm/mach-davinci/board-da850-evm.c | 190 +++++++++++++++++++++++++++++++
1 files changed, 190 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 420b628..3cff221 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -431,6 +431,185 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
return 0;
}

+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(da850_evm_ui_exp)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
+
+enum da850_evm_bb_exp_pins {
+ DA850_EVM_BB_EXP_DEEP_SLEEP_EN = 0,
+ DA850_EVM_BB_EXP_SW_RST,
+ DA850_EVM_BB_EXP_TP_23,
+ DA850_EVM_BB_EXP_TP_22,
+ DA850_EVM_BB_EXP_TP_21,
+ DA850_EVM_BB_EXP_USER_PB1,
+ DA850_EVM_BB_EXP_USER_LED2,
+ DA850_EVM_BB_EXP_USER_LED1,
+ DA850_EVM_BB_EXP_USER_SW1,
+ DA850_EVM_BB_EXP_USER_SW2,
+ DA850_EVM_BB_EXP_USER_SW3,
+ DA850_EVM_BB_EXP_USER_SW4,
+ DA850_EVM_BB_EXP_USER_SW5,
+ DA850_EVM_BB_EXP_USER_SW6,
+ DA850_EVM_BB_EXP_USER_SW7,
+ DA850_EVM_BB_EXP_USER_SW8
+};
+
+static const char const *da850_evm_bb_exp[] = {
+ [DA850_EVM_BB_EXP_DEEP_SLEEP_EN] = "deep_sleep_en",
+ [DA850_EVM_BB_EXP_SW_RST] = "sw_rst",
+ [DA850_EVM_BB_EXP_TP_23] = "tp_23",
+ [DA850_EVM_BB_EXP_TP_22] = "tp_22",
+ [DA850_EVM_BB_EXP_TP_21] = "tp_21",
+ [DA850_EVM_BB_EXP_USER_PB1] = "user_pb1",
+ [DA850_EVM_BB_EXP_USER_LED2] = "user_led2",
+ [DA850_EVM_BB_EXP_USER_LED1] = "user_led1",
+ [DA850_EVM_BB_EXP_USER_SW1] = "user_sw1",
+ [DA850_EVM_BB_EXP_USER_SW2] = "user_sw2",
+ [DA850_EVM_BB_EXP_USER_SW3] = "user_sw3",
+ [DA850_EVM_BB_EXP_USER_SW4] = "user_sw4",
+ [DA850_EVM_BB_EXP_USER_SW5] = "user_sw5",
+ [DA850_EVM_BB_EXP_USER_SW6] = "user_sw6",
+ [DA850_EVM_BB_EXP_USER_SW7] = "user_sw7",
+ [DA850_EVM_BB_EXP_USER_SW8] = "user_sw8",
+};
+
+#define DA850_N_BB_USER_SW 8
+
+static struct gpio_keys_button da850_evm_bb_keys[] = {
+ [0] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = KEY_PROG1,
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+ [1 ... DA850_N_BB_USER_SW] = {
+ .type = EV_SW,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_bb_keys_pdata = {
+ .buttons = da850_evm_bb_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_bb_keys),
+ .rep = 0, /* disable auto-repeat */
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_bb_keys_device = {
+ .name = "gpio-keys",
+ .id = 1,
+ .dev = {
+ .platform_data = &da850_evm_bb_keys_pdata
+ },
+};
+
+static void da850_evm_bb_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ button = &da850_evm_bb_keys[0];
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_PB1];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_PB1;
+
+ for (i = 0; i < DA850_N_BB_USER_SW; i++) {
+ button = &da850_evm_bb_keys[i + 1];
+ button->code = SW_LID + i;
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_SW1 + i];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_SW1 + i;
+ }
+}
+
+#define DA850_N_BB_USER_LED 2
+
+static struct gpio_led da850_evm_bb_leds[] = {
+ [0 ... DA850_N_BB_USER_LED - 1] = {
+ .active_low = 1,
+ .gpio = -1, /* assigned at runtime */
+ .name = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_led_platform_data da850_evm_bb_leds_pdata = {
+ .leds = da850_evm_bb_leds,
+ .num_leds = ARRAY_SIZE(da850_evm_bb_leds),
+};
+
+static struct platform_device da850_evm_bb_leds_device = {
+ .name = "leds-gpio",
+ .id = -1,
+ .dev = {
+ .platform_data = &da850_evm_bb_leds_pdata
+ }
+};
+
+static void da850_evm_bb_leds_init(unsigned gpio)
+{
+ int i;
+ struct gpio_led *led;
+
+ for (i = 0; i < DA850_N_BB_USER_LED; i++) {
+ led = &da850_evm_bb_leds[i];
+
+ led->gpio = gpio + DA850_EVM_BB_EXP_USER_LED2 + i;
+ led->name =
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_LED2 + i];
+ }
+}
+
+static int da850_evm_bb_expander_setup(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *c)
+{
+ int ret;
+
+ /*
+ * Register the switches and pushbutton on the baseboard as a gpio-keys
+ * device.
+ */
+ da850_evm_bb_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_keys_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander switches"
+ " device\n");
+ goto io_exp_setup_sw_fail;
+ }
+
+ da850_evm_bb_leds_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_leds_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander LEDS "
+ "device\n");
+ goto io_exp_setup_leds_fail;
+ }
+
+ return 0;
+
+io_exp_setup_leds_fail:
+ platform_device_unregister(&da850_evm_bb_keys_device);
+io_exp_setup_sw_fail:
+ return ret;
+}
+
+static int da850_evm_bb_expander_teardown(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio, void *c)
+{
+ platform_device_unregister(&da850_evm_bb_leds_device);
+ platform_device_unregister(&da850_evm_bb_keys_device);
+
+ return 0;
+}
+
static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
@@ -438,6 +617,13 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.names = da850_evm_ui_exp,
};

+static struct pca953x_platform_data da850_evm_bb_expander_info = {
+ .gpio_base = DA850_BB_EXPANDER_GPIO_BASE,
+ .setup = da850_evm_bb_expander_setup,
+ .teardown = da850_evm_bb_expander_teardown,
+ .names = da850_evm_bb_exp,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -446,6 +632,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
},
+ {
+ I2C_BOARD_INFO("tca6416", 0x21),
+ .platform_data = &da850_evm_bb_expander_info,
+ },
};

static struct davinci_i2c_platform_data da850_evm_i2c_0_pdata = {
--
1.7.0.4

2010-11-23 19:41:54

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v4 3/5] da850-evm: extract defines for SEL{A,B,C} pins in UI expander

The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
pins by 'magic number' in each function. This uses the common enum for their offsets
in the expander setup and teardown functions.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Victor Rodriguez <[email protected]>

---

Changes since v3:
* no changes in this patch of the series

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* integrated the static array initialization patch provided by Sekhar Nori

Changes since v1:
* No changes since v1

---
arch/arm/mach-davinci/board-da850-evm.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 51f5ffa..420b628 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -361,23 +361,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + 7;
- sel_b = gpio + 6;
- sel_c = gpio + 5;
+ sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
+ sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
+ sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;

- ret = gpio_request(sel_a, "sel_a");
+ ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, "sel_b");
+ ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, "sel_c");
+ ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_C]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -420,13 +420,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&da850_evm_ui_keys_device);

/* deselect all functionalities */
- gpio_set_value_cansleep(gpio + 5, 1);
- gpio_set_value_cansleep(gpio + 6, 1);
- gpio_set_value_cansleep(gpio + 7, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_A, 1);

- gpio_free(gpio + 5);
- gpio_free(gpio + 6);
- gpio_free(gpio + 7);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);

return 0;
}
--
1.7.0.4

2010-11-24 06:10:04

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

On Tue, Nov 23, 2010 at 07:48:21AM -0800, Kevin Hilman wrote:
> "Nori, Sekhar" <[email protected]> writes:
> > On Mon, Nov 22, 2010 at 19:20:48, Ben Gardiner wrote:
> >> Yes -- only gpio-keys is affected but enabling the option does
> >> introduce an additional .o file:
> >>
> >> drivers/input/Makefile:obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> >>
> >> I agree that in its current state a user couls inadvertently disable
> >> the gpio-keys support on da850-evm simply by disabling INPUT_POLLDEV
> >> -- which is less than Ideal. How about I replace the current changes
> >> to arch/arm/mach-davinci/Kconfig with:
> >>
> >> config KEYBOARD_GPIO
> >> default MACH_DAVINCI_DA850_EVM
> >> select INPUT_POLLDEV
> >>
> >> So 1) gpio-keys functionality is default for the da850evm and 2)
> >> whenever gpio-keys is enabled so is INPUT_POLLDEV.
> >
> > This looks better than what was posted earlier, but I am not
> > sure if platforms should influence driver configuration this
> > way.
>
> Agreed. In general, we should not have machine/platform specific
> conditionals in generic Kconfigs. Generally, this should be handled in
> machine/platform specific Kconfigs.
>
The patch that I originally wrote for this had the select under the
Kconfig option for the driver itself, with the decision to use it or not
being dynamically determined based on the platform data. I maintain that
this is the only sensible way to deal with things, but this was rejected
by the input folks at the time who felt that it was adding in extra
overhead for a corner case. The alternatives then are to either make an
identical copy of the driver that uses a polled interface, come up with
lame INPUT_POLLDEV wrapper shims, or simply accept the fact that drivers
using optional interfaces are going to have to have those interfaces
built in to the kernel. There has been zero progress on getting this
functionality integrated for years now because of this issue, and I don't
really see that changing until people accept that drivers will sometimes
require additional functionality to be built-in, or provide a suitable
alternative. Personally I don't care enough about this particular problem
to put any more cycles in to it, and the hardware that I wrote this patch
for initially will be end-of-lifed long before there's any coherent
consensus in driver land.

2010-11-24 13:16:21

by Sekhar Nori

[permalink] [raw]
Subject: RE: [PATCH v4 2/5] da850-evm: add UI Expander pushbuttons

Hi Ben,

I have some minor comments on this patch. You could
wait for other pending items to get resolved before
posting a re-spin.

On Wed, Nov 24, 2010 at 01:10:57, Ben Gardiner wrote:

> arch/arm/mach-davinci/board-da850-evm.c | 98 ++++++++++++++++++++++++++++++-
> 1 files changed, 97 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c

> +static struct gpio_keys_button da850_evm_ui_keys[] = {
> + [0 ... DA850_N_UI_PB - 1] = {
> + .type = EV_KEY,
> + .active_low = 1,
> + .wakeup = 0,
> + .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
> + .code = -1, /* assigned at runtime */
> + .gpio = -1, /* assigned at runtime */
> + .desc = NULL, /* assigned at runtime */
> + },
> +};
> +
> +static struct gpio_keys_platform_data da850_evm_ui_keys_pdata = {
> + .buttons = da850_evm_ui_keys,
> + .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
> + .rep = 0, /* disable auto-repeat */
> + .poll_interval = DA850_GPIO_KEYS_POLL_MS,
> +};
> +
> +static struct platform_device da850_evm_ui_keys_device = {
> + .name = "gpio-keys",
> + .id = 0,
> + .dev = {
> + .platform_data = &da850_evm_ui_keys_pdata
> + },
> +};

No need of zero/NULL initialization in structures above
since they are static.

> @@ -304,15 +388,24 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
> gpio_direction_output(sel_b, 1);
> gpio_direction_output(sel_c, 1);
>
> + da850_evm_ui_keys_init(gpio);
> + ret = platform_device_register(&da850_evm_ui_keys_device);
> + if (ret) {
> + pr_warning("Could not register UI GPIO expander push-buttons"
> + " device\n");

Line-breaking an error message is not preferred since it becomes
difficult to grep in code. Looking at this message, you could drop
" device" altogether.

> + goto exp_setup_keys_fail;
> + }
> +
> ui_card_detected = 1;
> pr_info("DA850/OMAP-L138 EVM UI card detected\n");
>
> da850_evm_setup_nor_nand();
> -

Random white space change?

Thanks,
Sekhar

2010-11-24 17:16:48

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] da850-evm: add UI Expander pushbuttons

Hi Sekhar,

On Wed, Nov 24, 2010 at 8:16 AM, Nori, Sekhar <[email protected]> wrote:
> Hi Ben,
>
> I have some minor comments on this patch. You could
> wait for other pending items to get resolved before
> posting a re-spin.

Thank you for your continued interest and input.

> On Wed, Nov 24, 2010 at 01:10:57, Ben Gardiner wrote:
>
>> ?arch/arm/mach-davinci/board-da850-evm.c | ? 98 ++++++++++++++++++++++++++++++-
>> ?1 files changed, 97 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
>
>> +static struct gpio_keys_button da850_evm_ui_keys[] = {
>> + ? ? [0 ... DA850_N_UI_PB - 1] = {
>> + ? ? ? ? ? ? .type ? ? ? ? ? ? ? ? ? = EV_KEY,
>> + ? ? ? ? ? ? .active_low ? ? ? ? ? ? = 1,
>> + ? ? ? ? ? ? .wakeup ? ? ? ? ? ? ? ? = 0,
>> + ? ? ? ? ? ? .debounce_interval ? ? ?= DA850_KEYS_DEBOUNCE_MS,
>> + ? ? ? ? ? ? .code ? ? ? ? ? ? ? ? ? = -1, /* assigned at runtime */
>> + ? ? ? ? ? ? .gpio ? ? ? ? ? ? ? ? ? = -1, /* assigned at runtime */
>> + ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = NULL, /* assigned at runtime */
>> + ? ? },
>> +};
>> +
>> +static struct gpio_keys_platform_data da850_evm_ui_keys_pdata = {
>> + ? ? .buttons = da850_evm_ui_keys,
>> + ? ? .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
>> + ? ? .rep = 0, /* disable auto-repeat */
>> + ? ? .poll_interval = DA850_GPIO_KEYS_POLL_MS,
>> +};
>> +
>> +static struct platform_device da850_evm_ui_keys_device = {
>> + ? ? .name = "gpio-keys",
>> + ? ? .id = 0,
>> + ? ? .dev = {
>> + ? ? ? ? ? ? .platform_data = &da850_evm_ui_keys_pdata
>> + ? ? },
>> +};
>
> No need of zero/NULL initialization in structures above
> since they are static.

It my opinion -- please tell me if it is wrong :) -- that explicit
initialization of platform data members is better than implicit
initialization; future developers and browsers of the code can see
that wakeup events are disabled as are auto-repeats.

I also included the .desc = NULL explicitly to indicate that it would
be populated at runtime so that future developers who grep the code
would know to look for a runtime initialization.

The .id = 0 is there since the bb keys get .id = 1.

As I said please tell me if you would still like me to remove this
expliciti initializations -- I would prefer to leave them as-is.

>> @@ -304,15 +388,24 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
>> ? ? ? gpio_direction_output(sel_b, 1);
>> ? ? ? gpio_direction_output(sel_c, 1);
>>
>> + ? ? da850_evm_ui_keys_init(gpio);
>> + ? ? ret = platform_device_register(&da850_evm_ui_keys_device);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? pr_warning("Could not register UI GPIO expander push-buttons"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? " device\n");
>
> Line-breaking an error message is not preferred since it becomes
> difficult to grep in code. Looking at this message, you could drop
> " device" altogether.

Interesting point. Thank you, I really apreciate all the knowledge you
are imparting to me throughout this process. I will remove the "
device" string from the error message entirely.

>> + ? ? ? ? ? ? goto exp_setup_keys_fail;
>> + ? ? }
>> +
>> ? ? ? ui_card_detected = 1;
>> ? ? ? pr_info("DA850/OMAP-L138 EVM UI card detected\n");
>>
>> ? ? ? da850_evm_setup_nor_nand();
>> -
>
> Random white space change?

Oops, yes -- I am relying to heavily on checkpatch.pl to tell me about
my mistakes. I should be reviewing my patches more closely. Thank you
again.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-24 17:17:57

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] da850-evm: add UI Expander pushbuttons

Hi Paul,

On Wed, Nov 24, 2010 at 1:09 AM, Paul Mundt <[email protected]> wrote:
> On Tue, Nov 23, 2010 at 07:48:21AM -0800, Kevin Hilman wrote:
>> [...]
>> Agreed. ?In general, we should not have machine/platform specific
>> conditionals in generic Kconfigs. ? Generally, this should be handled in
>> machine/platform specific Kconfigs.
>>
> The patch that I originally wrote for this had the select under the
> Kconfig option for the driver itself, with the decision to use it or not
> being dynamically determined based on the platform data. I maintain that
> this is the only sensible way to deal with things, but this was rejected
> by the input folks at the time who felt that it was adding in extra
> overhead for a corner case. The alternatives then are to either make an
> identical copy of the driver that uses a polled interface, come up with
> lame INPUT_POLLDEV wrapper shims, or simply accept the fact that drivers
> using optional interfaces are going to have to have those interfaces
> built in to the kernel.

Thanks for weighing-in.

>From the statements that Dmitry has made [1] [2], I gather that a
separate driver is preferred. But I realized this too late.

I've tested the recent re-send of "[PATCH 09/18] input: add input
driver for polled GPIO buttons" by Gabor Juhos [3] with this patch
series and it works well. The changes needed in this patch series are
minimal. I have some review comments for the gpio_buttons patch that I
will be posting shortly.

The upshot is there will be no need for Kconfig entries for both
INPUT_POLLDEV and KEYBOARD_GPIO in arch/arm/mach-davinci, just one to
enable the gpio_buttons driver.

--
Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.linux.kernel.input/14044
[2] http://article.gmane.org/gmane.linux.kernel.input/16478
[3] http://article.gmane.org/gmane.linux.kernel.input/16610

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-11-24 21:59:49

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 3/5] da850-evm: extract defines for SEL{A,B,C} pins in UI expander

The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
pins by 'magic number' in each function. This uses the common enum for their offsets
in the expander setup and teardown functions.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Victor Rodriguez <[email protected]>

---

Changes since v4:
* no changes in this patch of the series

Changes since v3:
* no changes in this patch of the series

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* integrated the static array initialization patch provided by Sekhar Nori

Changes since v1:
* No changes since v1
---
arch/arm/mach-davinci/board-da850-evm.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index f6490f8..8f7a605 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -360,23 +360,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + 7;
- sel_b = gpio + 6;
- sel_c = gpio + 5;
+ sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
+ sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
+ sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;

- ret = gpio_request(sel_a, "sel_a");
+ ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, "sel_b");
+ ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, "sel_c");
+ ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_C]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -419,13 +419,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&da850_evm_ui_keys_device);

/* deselect all functionalities */
- gpio_set_value_cansleep(gpio + 5, 1);
- gpio_set_value_cansleep(gpio + 6, 1);
- gpio_set_value_cansleep(gpio + 7, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_A, 1);

- gpio_free(gpio + 5);
- gpio_free(gpio + 6);
- gpio_free(gpio + 7);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);

return 0;
}
--
1.7.0.4

2010-11-24 21:59:58

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 4/5] da850-evm: add baseboard GPIO expander buttons, switches and LEDs

This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches and
LEDs.

Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.

The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
CC: Gabor Juhos <[email protected]>

---

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver
* removed extra indent (Sekhar Nori)
* don't line-break error messages (Sekhar Nori)
* left-in the explicit static initialization of structure members

Changes since v3:
* don't request sw_rst and deep_sleep_en gpio pins -- let clients use them
freely

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in 1/4 instead of this patch
(Nori, Sekhar)
* ui_expander_names was renamed to da850_evm_ui_exp
* DA850_SW_POLL_MS definition moved to this patch from 3/4
* use indexed array initialization pattern introduced by Sekhar Nori in 3/4
* shorter names prefixed with da850_evm
* static array range intializers
* using only a single gpio-keys instance for the pushbutton and switches on
baseboard since there is no advantage to separate device instances with
different polling intervals (Dmitry Torokhov)

Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 187 +++++++++++++++++++++++++++++++
1 files changed, 187 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 8f7a605..14d3381 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -430,6 +430,182 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
return 0;
}

+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(da850_evm_ui_exp)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
+
+enum da850_evm_bb_exp_pins {
+ DA850_EVM_BB_EXP_DEEP_SLEEP_EN = 0,
+ DA850_EVM_BB_EXP_SW_RST,
+ DA850_EVM_BB_EXP_TP_23,
+ DA850_EVM_BB_EXP_TP_22,
+ DA850_EVM_BB_EXP_TP_21,
+ DA850_EVM_BB_EXP_USER_PB1,
+ DA850_EVM_BB_EXP_USER_LED2,
+ DA850_EVM_BB_EXP_USER_LED1,
+ DA850_EVM_BB_EXP_USER_SW1,
+ DA850_EVM_BB_EXP_USER_SW2,
+ DA850_EVM_BB_EXP_USER_SW3,
+ DA850_EVM_BB_EXP_USER_SW4,
+ DA850_EVM_BB_EXP_USER_SW5,
+ DA850_EVM_BB_EXP_USER_SW6,
+ DA850_EVM_BB_EXP_USER_SW7,
+ DA850_EVM_BB_EXP_USER_SW8
+};
+
+static const char const *da850_evm_bb_exp[] = {
+ [DA850_EVM_BB_EXP_DEEP_SLEEP_EN] = "deep_sleep_en",
+ [DA850_EVM_BB_EXP_SW_RST] = "sw_rst",
+ [DA850_EVM_BB_EXP_TP_23] = "tp_23",
+ [DA850_EVM_BB_EXP_TP_22] = "tp_22",
+ [DA850_EVM_BB_EXP_TP_21] = "tp_21",
+ [DA850_EVM_BB_EXP_USER_PB1] = "user_pb1",
+ [DA850_EVM_BB_EXP_USER_LED2] = "user_led2",
+ [DA850_EVM_BB_EXP_USER_LED1] = "user_led1",
+ [DA850_EVM_BB_EXP_USER_SW1] = "user_sw1",
+ [DA850_EVM_BB_EXP_USER_SW2] = "user_sw2",
+ [DA850_EVM_BB_EXP_USER_SW3] = "user_sw3",
+ [DA850_EVM_BB_EXP_USER_SW4] = "user_sw4",
+ [DA850_EVM_BB_EXP_USER_SW5] = "user_sw5",
+ [DA850_EVM_BB_EXP_USER_SW6] = "user_sw6",
+ [DA850_EVM_BB_EXP_USER_SW7] = "user_sw7",
+ [DA850_EVM_BB_EXP_USER_SW8] = "user_sw8",
+};
+
+#define DA850_N_BB_USER_SW 8
+
+static struct gpio_keys_button da850_evm_bb_keys[] = {
+ [0] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = KEY_PROG1,
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+ [1 ... DA850_N_BB_USER_SW] = {
+ .type = EV_SW,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_polled_platform_data da850_evm_bb_keys_pdata = {
+ .buttons = da850_evm_bb_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_bb_keys),
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_bb_keys_device = {
+ .name = "gpio-keys-polled",
+ .id = 1,
+ .dev = {
+ .platform_data = &da850_evm_bb_keys_pdata
+ },
+};
+
+static void da850_evm_bb_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ button = &da850_evm_bb_keys[0];
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_PB1];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_PB1;
+
+ for (i = 0; i < DA850_N_BB_USER_SW; i++) {
+ button = &da850_evm_bb_keys[i + 1];
+ button->code = SW_LID + i;
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_SW1 + i];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_SW1 + i;
+ }
+}
+
+#define DA850_N_BB_USER_LED 2
+
+static struct gpio_led da850_evm_bb_leds[] = {
+ [0 ... DA850_N_BB_USER_LED - 1] = {
+ .active_low = 1,
+ .gpio = -1, /* assigned at runtime */
+ .name = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_led_platform_data da850_evm_bb_leds_pdata = {
+ .leds = da850_evm_bb_leds,
+ .num_leds = ARRAY_SIZE(da850_evm_bb_leds),
+};
+
+static struct platform_device da850_evm_bb_leds_device = {
+ .name = "leds-gpio",
+ .id = -1,
+ .dev = {
+ .platform_data = &da850_evm_bb_leds_pdata
+ }
+};
+
+static void da850_evm_bb_leds_init(unsigned gpio)
+{
+ int i;
+ struct gpio_led *led;
+
+ for (i = 0; i < DA850_N_BB_USER_LED; i++) {
+ led = &da850_evm_bb_leds[i];
+
+ led->gpio = gpio + DA850_EVM_BB_EXP_USER_LED2 + i;
+ led->name =
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_LED2 + i];
+ }
+}
+
+static int da850_evm_bb_expander_setup(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *c)
+{
+ int ret;
+
+ /*
+ * Register the switches and pushbutton on the baseboard as a gpio-keys
+ * device.
+ */
+ da850_evm_bb_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_keys_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander keys");
+ goto io_exp_setup_sw_fail;
+ }
+
+ da850_evm_bb_leds_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_leds_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander LEDS");
+ goto io_exp_setup_leds_fail;
+ }
+
+ return 0;
+
+io_exp_setup_leds_fail:
+ platform_device_unregister(&da850_evm_bb_keys_device);
+io_exp_setup_sw_fail:
+ return ret;
+}
+
+static int da850_evm_bb_expander_teardown(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio, void *c)
+{
+ platform_device_unregister(&da850_evm_bb_leds_device);
+ platform_device_unregister(&da850_evm_bb_keys_device);
+
+ return 0;
+}
+
static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
@@ -437,6 +613,13 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.names = da850_evm_ui_exp,
};

+static struct pca953x_platform_data da850_evm_bb_expander_info = {
+ .gpio_base = DA850_BB_EXPANDER_GPIO_BASE,
+ .setup = da850_evm_bb_expander_setup,
+ .teardown = da850_evm_bb_expander_teardown,
+ .names = da850_evm_bb_exp,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -445,6 +628,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
},
+ {
+ I2C_BOARD_INFO("tca6416", 0x21),
+ .platform_data = &da850_evm_bb_expander_info,
+ },
};

static struct davinci_i2c_platform_data da850_evm_i2c_0_pdata = {
--
1.7.0.4

2010-11-24 21:59:57

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 5/5] da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Use the mach-davinci/Kconfig to enable gpio-keys-polled as default when
da850-evm machine is enabled.

Signed-off-by: Ben Gardiner <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: "Nori, Sekhar" <[email protected]>
CC: Gabor Juhos <[email protected]>

---

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver

Changes since v3:
* no changes in this patch of the series / this patch was introduced in v4 of
the series
---
arch/arm/mach-davinci/Kconfig | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index 84066e8..b93c327 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -180,6 +180,9 @@ endchoice
config GPIO_PCA953X
default MACH_DAVINCI_DA850_EVM

+config KEYBOARD_GPIO_POLLED
+ default MACH_DAVINCI_DA850_EVM
+
config MACH_TNETV107X
bool "TI TNETV107X Reference Platform"
default ARCH_DAVINCI_TNETV107X
--
1.7.0.4

2010-11-24 21:59:44

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 1/5] [WIP] input: add input driver for polled GPIO buttons

From: Gabor Juhos <[email protected]>

The existing gpio-keys driver can be usable only for GPIO lines with
interrupt support. Several devices have buttons connected to a GPIO
line which is not capable to generate interrupts. This patch adds a
new input driver using the generic GPIO layer and the input-polldev
to support such buttons.

(WIP: this version has incorporated into it the changes I suggested in review
of the original patch by Gabor Junos. I am posting it as part of the da850-evm
series in the hopes that the final version produced by Gabor can be
substituted in its place when the time is right. I don't mean to imply that
this version of the patch should be integrated in the final series)

Signed-off-by: Gabor Juhos <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Mike Frysinger <[email protected]>
Cc: [email protected]
Signed-off-by: Ben Gardiner <[email protected]>
---
drivers/input/keyboard/Kconfig | 16 ++
drivers/input/keyboard/Makefile | 2 +
drivers/input/keyboard/gpio_keys_polled.c | 240 +++++++++++++++++++++++++++++
include/linux/gpio_keys_polled.h | 26 +++
4 files changed, 284 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/gpio_keys_polled.c
create mode 100644 include/linux/gpio_keys_polled.h

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b8c51b9..9648ff4 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -485,4 +485,20 @@ config KEYBOARD_W90P910
To compile this driver as a module, choose M here: the
module will be called w90p910_keypad.

+config KEYBOARD_GPIO_POLLED
+ tristate "Polled GPIO buttons"
+ depends on GENERIC_GPIO
+ select INPUT_POLLDEV
+ help
+ This driver implements support for buttons connected
+ to GPIO pins of various CPUs (and some other chips).
+
+ Say Y here if your device has buttons connected
+ directly to such GPIO pins. Your board-specific
+ setup logic must also provide a platform device,
+ with configuration data saying which GPIOs are used.
+
+ To compile this driver as a module, choose M here: the
+ module will be called gpio-buttons.
+
endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index a34452e..e6da817 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -44,3 +44,5 @@ obj-$(CONFIG_KEYBOARD_TNETV107X) += tnetv107x-keypad.o
obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
+obj-$(CONFIG_KEYBOARD_GPIO_POLLED) += gpio_keys_polled.o
+
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
new file mode 100644
index 0000000..390ed93
--- /dev/null
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -0,0 +1,240 @@
+/*
+ * Driver for buttons on GPIO lines not capable of generating interrupts
+ *
+ * Copyright (C) 2007-2010 Gabor Juhos <[email protected]>
+ * Copyright (C) 2010 Nuno Goncalves <[email protected]>
+ *
+ * This file was based on: /drivers/input/misc/cobalt_btns.c
+ * Copyright (C) 2007 Yoichi Yuasa <[email protected]>
+ *
+ * also was based on: /drivers/input/keyboard/gpio_keys.c
+ * Copyright 2005 Phil Blundell
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys_polled.h>
+
+#define DRV_NAME "gpio-keys-polled"
+
+struct gpio_keys_button_data {
+ int last_state;
+ int count;
+ int can_sleep;
+};
+
+struct gpio_keys_polled_dev {
+ struct input_polled_dev *poll_dev;
+ struct gpio_keys_polled_platform_data *pdata;
+ struct gpio_keys_button_data *data;
+};
+
+static void gpio_keys_polled_check_state(struct input_dev *input,
+ struct gpio_keys_button *button,
+ struct gpio_keys_button_data *bdata)
+{
+ int state;
+
+ if (bdata->can_sleep)
+ state = !!gpio_get_value_cansleep(button->gpio);
+ else
+ state = !!gpio_get_value(button->gpio);
+
+ if (state != bdata->last_state) {
+ unsigned int type = button->type ?: EV_KEY;
+
+ input_event(input, type, button->code,
+ !!(state ^ button->active_low));
+ input_sync(input);
+ bdata->count = 0;
+ bdata->last_state = state;
+ }
+}
+
+static void gpio_keys_polled_poll(struct input_polled_dev *dev)
+{
+ struct gpio_keys_polled_dev *bdev = dev->private;
+ struct gpio_keys_polled_platform_data *pdata = bdev->pdata;
+ struct input_dev *input = dev->input;
+ int i, threshold;
+
+ for (i = 0; i < bdev->pdata->nbuttons; i++) {
+ struct gpio_keys_button *button = &pdata->buttons[i];
+ struct gpio_keys_button_data *bdata = &bdev->data[i];
+
+ threshold = round_up(button->debounce_interval,
+ pdata->poll_interval) /
+ pdata->poll_interval;
+ if (bdata->count < threshold)
+ bdata->count++;
+ else
+ gpio_keys_polled_check_state(input, button, bdata);
+
+ }
+}
+
+static int __devinit gpio_keys_polled_probe(struct platform_device *pdev)
+{
+ struct gpio_keys_polled_platform_data *pdata = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct gpio_keys_polled_dev *bdev;
+ struct input_polled_dev *poll_dev;
+ struct input_dev *input;
+ int error;
+ int i;
+
+ if (!pdata)
+ return -ENXIO;
+
+ bdev = kzalloc(sizeof(struct gpio_keys_polled_dev) +
+ pdata->nbuttons * sizeof(struct gpio_keys_button_data),
+ GFP_KERNEL);
+ if (!bdev) {
+ dev_err(dev, "no memory for private data\n");
+ return -ENOMEM;
+ }
+
+ bdev->data = (struct gpio_keys_button_data *) &bdev[1];
+
+ poll_dev = input_allocate_polled_device();
+ if (!poll_dev) {
+ dev_err(dev, "no memory for polled device\n");
+ error = -ENOMEM;
+ goto err_free_bdev;
+ }
+
+ poll_dev->private = bdev;
+ poll_dev->poll = gpio_keys_polled_poll;
+ poll_dev->poll_interval = pdata->poll_interval;
+
+ input = poll_dev->input;
+
+ input->evbit[0] = BIT(EV_KEY);
+ input->name = pdev->name;
+ input->phys = DRV_NAME"/input0";
+ input->dev.parent = &pdev->dev;
+
+ input->id.bustype = BUS_HOST;
+ input->id.vendor = 0x0001;
+ input->id.product = 0x0001;
+ input->id.version = 0x0100;
+
+ for (i = 0; i < pdata->nbuttons; i++) {
+ struct gpio_keys_button *button = &pdata->buttons[i];
+ unsigned int gpio = button->gpio;
+ unsigned int type = button->type ?: EV_KEY;
+
+ if (button->wakeup) {
+ dev_err(dev, DRV_NAME " does not support wakeup\n");
+ goto err_free_gpio;
+ }
+
+ error = gpio_request(gpio,
+ button->desc ? button->desc : DRV_NAME);
+ if (error) {
+ dev_err(dev, "unable to claim gpio %u, err=%d\n",
+ gpio, error);
+ goto err_free_gpio;
+ }
+
+ error = gpio_direction_input(gpio);
+ if (error) {
+ dev_err(dev,
+ "unable to set direction on gpio %u, err=%d\n",
+ gpio, error);
+ goto err_free_gpio;
+ }
+
+ bdev->data[i].can_sleep = gpio_cansleep(gpio);
+ bdev->data[i].last_state = -1;
+
+ input_set_capability(input, type, button->code);
+ }
+
+ bdev->poll_dev = poll_dev;
+ bdev->pdata = pdata;
+ platform_set_drvdata(pdev, bdev);
+
+ error = input_register_polled_device(poll_dev);
+ if (error) {
+ dev_err(dev, "unable to register polled device, err=%d\n",
+ error);
+ goto err_free_gpio;
+ }
+
+ /* report initial state of the buttons */
+ for (i = 0; i < pdata->nbuttons; i++)
+ gpio_keys_polled_check_state(input, &pdata->buttons[i],
+ &bdev->data[i]);
+
+ return 0;
+
+err_free_gpio:
+ for (i = i - 1; i >= 0; i--)
+ gpio_free(pdata->buttons[i].gpio);
+
+ input_free_polled_device(poll_dev);
+
+err_free_bdev:
+ kfree(bdev);
+
+ platform_set_drvdata(pdev, NULL);
+ return error;
+}
+
+static int __devexit gpio_keys_polled_remove(struct platform_device *pdev)
+{
+ struct gpio_keys_polled_dev *bdev = platform_get_drvdata(pdev);
+ struct gpio_keys_polled_platform_data *pdata = bdev->pdata;
+ int i;
+
+ input_unregister_polled_device(bdev->poll_dev);
+
+ for (i = 0; i < pdata->nbuttons; i++)
+ gpio_free(pdata->buttons[i].gpio);
+
+ input_free_polled_device(bdev->poll_dev);
+
+ kfree(bdev);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+static struct platform_driver gpio_keys_polled_driver = {
+ .probe = gpio_keys_polled_probe,
+ .remove = __devexit_p(gpio_keys_polled_remove),
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init gpio_keys_polled_init(void)
+{
+ return platform_driver_register(&gpio_keys_polled_driver);
+}
+
+static void __exit gpio_keys_polled_exit(void)
+{
+ platform_driver_unregister(&gpio_keys_polled_driver);
+}
+
+module_init(gpio_keys_polled_init);
+module_exit(gpio_keys_polled_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Gabor Juhos <[email protected]>");
+MODULE_DESCRIPTION("Polled GPIO Buttons driver");
diff --git a/include/linux/gpio_keys_polled.h b/include/linux/gpio_keys_polled.h
new file mode 100644
index 0000000..bf7f94a
--- /dev/null
+++ b/include/linux/gpio_keys_polled.h
@@ -0,0 +1,26 @@
+/*
+ * Definitions for the GPIO buttons interface driver
+ *
+ * Copyright (C) 2007-2010 Gabor Juhos <[email protected]>
+ *
+ * This file was based on: /include/linux/gpio_keys.h
+ * The original gpio_keys.h seems not to have a license.
+ *
+ * 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 _GPIO_BUTTONS_H_
+#define _GPIO_BUTTONS_H_
+
+#include <linux/gpio_keys.h>
+
+struct gpio_keys_polled_platform_data {
+ struct gpio_keys_button *buttons;
+ int nbuttons; /* number of buttons */
+ int poll_interval; /* polling interval in msecs*/
+};
+
+#endif /* _GPIO_BUTTONS_H_ */
--
1.7.0.4

2010-11-24 21:59:40

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Note: I must regrettably move on to other projects for awhile and will thus
be unavailable to continue this integration effort. I am posting the most
recent version of the series with a modified version of Gabor Juho's driver
in the hopes that it will make the integration effort easier for whoever picks
up the task when the time is right -- whether or not that is me.

My apologies for the patchbomb. I will do my best to make time for reviews
and testing of any future versions of the series.

Best Regards,
Ben Gardiner

---

The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
They are bootstrapped to different I2C addresses so they can be used
concurrently.

The expander on the UI board is currently used to enable/disable the
peripherals that are available on the UI board. In addition to this
functionality the expander is also connected to 8 pushbuttons. The expander
on the baseboard is not currently used; it is connected to deep sleep enable,
sw reset, a push button, some switches and LEDs.

This proposed patch series enables the push buttons and switches on the UI and
BB expanders using the gpio-keys polling mode patch by Gabor Juhos. Some
work was performed to test irq-based gpio-keys support on the expanders (a WIP
patch can be posted on request) but I believe that it is not possible to use
irq-based gpio-keys on IO expanders for arm systems at this time.

The attempt started when I noticed the patch of Alek Du and Alan Cox [1] which
was recently committed [2]; a stab at integrating irq-based gpio-keys support
based on that patch was attempted. I found that I either got a warning that the
irq could not be mapped for the given gpio ; or, when N_IRQ was increased, a
system freeze.

>From what I have read (particularly the message by Grant Likely [3]) IRQs on
IO expanders are not ready in ARM yet. I _think_ that the sparse IRQ rework by
Thomas Gleixner [4] will resolve the blocker to irq-based gpio-keys support.

In the meantime we have buttons and switches that we would like to excersise
in our prototyping development. The patch to convert this series to irq-based
gpio-keys will be straighforward once the support in arch/arm is there.

There is an existing tca6416-keypad driver with polling support which I did not
employ because it isn't possible to keep the gpio's used for peripheral
enable/disable on the UI board or the LEDs on the baseboard registered while
simultaneously registering the pushbuttons or switches as a tca6416-keypad
instance.

I tested this patch series using evtest on the resulting /dev/input/eventN
devices and also on the event node of a non-polling gpio-keys instance to
ensure that irq-based input handling is not broken by the introduction of the
polling-mode gpio-keys patch. The non-polling instance creation and
registration is not included in this series since it uses one of the boot-mode
DIP switches and woult not (I think) be suitable for mainline.

Disclaimer:
I'm not an expert in irq's or gpio-keys; this is, in fact, my first proposed
feature. Please feel free to correct me -- I welcome the chance to learn from
your expertise.

Ben Gardiner (4):
da850-evm: add UI Expander pushbuttons
da850-evm: extract defines for SEL{A,B,C} pins in UI expander
da850-evm: add baseboard GPIO expander buttons, switches and LEDs
da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Gabor Juhos (1):
[WIP] input: add input driver for polled GPIO buttons

arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 306 +++++++++++++++++++++++++++--
drivers/input/keyboard/Kconfig | 16 ++
drivers/input/keyboard/Makefile | 2 +
drivers/input/keyboard/gpio_keys_polled.c | 240 ++++++++++++++++++++++
include/linux/gpio_keys_polled.h | 26 +++
6 files changed, 581 insertions(+), 12 deletions(-)
create mode 100644 drivers/input/keyboard/gpio_keys_polled.c
create mode 100644 include/linux/gpio_keys_polled.h

---

Changes since v4:
* integrated Gabor Juhos' polling gpio button driver in place of the
gpio-keys patch of Paul Mundt and Alex Clouter
* dont' line break error messages (Sekhar Nori)
* whitespace cleanup (Sekhar Nori)

Changes since v3:
* introduced patch 5 in the series by extracting the Kconfig changes proposed
in patch 2 of v3.
* not gpio_request()'ing the sw_rst and deep_sleep_en lines as requested
(Sekhar Nori)

Changes since v2:
* register a single input device for switches and keys on the baseboard since
there is no benefit to separate devices with different polling intervals
(Dmitry Torokhov)
* use static array intialization and range intialization for platform data
structure to minimize the amount of runtime intialization needed:
(Sekhar Nori)
* Use the da850_evm variable name prefix for static symbols in
board-da850-evm.c

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used

2010-11-24 22:00:40

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v5 2/5] da850-evm: add UI Expander pushbuttons

This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
gpio-key device.

The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
disable the peripherals found on the UI board in addition to the 8 pushbuttons
mentioned above. The reason the existing tca6416-keypad driver is not employed
is because there was no aparent way to keep the gpio lines used as
SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
tca6416-keypad instance.

Some experimentation with the polling interval was performed; we were searching
for the largest polling interval that did not affect the feel of the
responsiveness of the buttons. It is very subjective but 200ms seems to be a
good value that accepts firm pushes but rejects very light ones. The key values
assigned to the buttons were arbitrarily chosen to be F1-F8.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Gabor Juhos <[email protected]>

---

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver
* removed spurious whitespace change (Sekhar Nori)
* don't linebreak error messages (Sekhar Nori)
* kept the explicit static initialization of structure members in-place

Changes since v3:
* extracted Kconfig changes to patch 5/5
* fixed leading whitespace problem

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in this patch instead of 4/4
(Nori, Sekhar)
* integrated the static array initialization patch of Sekhar Nori
* use static array initialization ranges
* rename DA850_PB_POLL_MS to DA850_GPIO_KEYS_POLL_MS
* use shorter names prefixed with da850_evm

Changes since v1:
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 95 +++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index f89b0b7..f6490f8 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -17,8 +17,10 @@
#include <linux/i2c.h>
#include <linux/i2c/at24.h>
#include <linux/i2c/pca953x.h>
+#include <linux/input.h>
#include <linux/mfd/tps6507x.h>
#include <linux/gpio.h>
+#include <linux/gpio_keys_polled.h>
#include <linux/platform_device.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -272,6 +274,87 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel)
static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
#endif

+
+#define DA850_KEYS_DEBOUNCE_MS 10
+/*
+ * At 200ms polling interval it is possible to miss an
+ * event by tapping very lightly on the push button but most
+ * pushes do result in an event; longer intervals require the
+ * user to hold the button whereas shorter intervals require
+ * more CPU time for polling.
+ */
+#define DA850_GPIO_KEYS_POLL_MS 200
+
+enum da850_evm_ui_exp_pins {
+ DA850_EVM_UI_EXP_SEL_C = 5,
+ DA850_EVM_UI_EXP_SEL_B,
+ DA850_EVM_UI_EXP_SEL_A,
+ DA850_EVM_UI_EXP_PB8,
+ DA850_EVM_UI_EXP_PB7,
+ DA850_EVM_UI_EXP_PB6,
+ DA850_EVM_UI_EXP_PB5,
+ DA850_EVM_UI_EXP_PB4,
+ DA850_EVM_UI_EXP_PB3,
+ DA850_EVM_UI_EXP_PB2,
+ DA850_EVM_UI_EXP_PB1,
+};
+
+static const char const *da850_evm_ui_exp[] = {
+ [DA850_EVM_UI_EXP_SEL_C] = "sel_c",
+ [DA850_EVM_UI_EXP_SEL_B] = "sel_b",
+ [DA850_EVM_UI_EXP_SEL_A] = "sel_a",
+ [DA850_EVM_UI_EXP_PB8] = "pb8",
+ [DA850_EVM_UI_EXP_PB7] = "pb7",
+ [DA850_EVM_UI_EXP_PB6] = "pb6",
+ [DA850_EVM_UI_EXP_PB5] = "pb5",
+ [DA850_EVM_UI_EXP_PB4] = "pb4",
+ [DA850_EVM_UI_EXP_PB3] = "pb3",
+ [DA850_EVM_UI_EXP_PB2] = "pb2",
+ [DA850_EVM_UI_EXP_PB1] = "pb1",
+};
+
+#define DA850_N_UI_PB 8
+
+static struct gpio_keys_button da850_evm_ui_keys[] = {
+ [0 ... DA850_N_UI_PB - 1] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_polled_platform_data da850_evm_ui_keys_pdata = {
+ .buttons = da850_evm_ui_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_ui_keys_device = {
+ .name = "gpio-keys-polled",
+ .id = 0,
+ .dev = {
+ .platform_data = &da850_evm_ui_keys_pdata
+ },
+};
+
+static void da850_evm_ui_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_UI_PB; i++) {
+ button = &da850_evm_ui_keys[i];
+ button->code = KEY_F8 - i;
+ button->desc = (char *)
+ da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i];
+ button->gpio = gpio + DA850_EVM_UI_EXP_PB8 + i;
+ }
+}
+
static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
unsigned ngpio, void *c)
{
@@ -304,6 +387,13 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
gpio_direction_output(sel_b, 1);
gpio_direction_output(sel_c, 1);

+ da850_evm_ui_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_ui_keys_device);
+ if (ret) {
+ pr_warning("Could not register UI GPIO expander push-buttons");
+ goto exp_setup_keys_fail;
+ }
+
ui_card_detected = 1;
pr_info("DA850/OMAP-L138 EVM UI card detected\n");

@@ -313,6 +403,8 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,

return 0;

+exp_setup_keys_fail:
+ gpio_free(sel_c);
exp_setup_selc_fail:
gpio_free(sel_b);
exp_setup_selb_fail:
@@ -324,6 +416,8 @@ exp_setup_sela_fail:
static int da850_evm_ui_expander_teardown(struct i2c_client *client,
unsigned gpio, unsigned ngpio, void *c)
{
+ platform_device_unregister(&da850_evm_ui_keys_device);
+
/* deselect all functionalities */
gpio_set_value_cansleep(gpio + 5, 1);
gpio_set_value_cansleep(gpio + 6, 1);
@@ -340,6 +434,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
+ .names = da850_evm_ui_exp,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
--
1.7.0.4

2010-12-09 21:51:17

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
They are bootstrapped to different I2C addresses so they can be used
concurrently.

The expander on the UI board is currently used to enable/disable the
peripherals that are available on the UI board. In addition to this
functionality the expander is also connected to 8 pushbuttons. The expander
on the baseboard is not currently used; it is connected to deep sleep enable,
sw reset, a push button, some switches and LEDs.

This proposed patch series enables the push buttons and switches on the UI and
BB expanders using the gpio-keys polling mode patch by Gabor Juhos. Some
work was performed to test irq-based gpio-keys support on the expanders (a WIP
patch can be posted on request) but I believe that it is not possible to use
irq-based gpio-keys on IO expanders for arm systems at this time.

The attempt started when I noticed the patch of Alek Du and Alan Cox [1] which
was recently committed [2]; a stab at integrating irq-based gpio-keys support
based on that patch was attempted. I found that I either got a warning that the
irq could not be mapped for the given gpio ; or, when N_IRQ was increased, a
system freeze.

>From what I have read (particularly the message by Grant Likely [3]) IRQs on
IO expanders are not ready in ARM yet. I _think_ that the sparse IRQ rework by
Thomas Gleixner [4] will resolve the blocker to irq-based gpio-keys support.

In the meantime we have buttons and switches that we would like to excersise
in our prototyping development. The patch to convert this series to irq-based
gpio-keys will be straighforward once the support in arch/arm is there.

There is an existing tca6416-keypad driver with polling support which I did not
employ because it isn't possible to keep the gpio's used for peripheral
enable/disable on the UI board or the LEDs on the baseboard registered while
simultaneously registering the pushbuttons or switches as a tca6416-keypad
instance.

I tested this patch series using evtest on the resulting /dev/input/eventN
devices and also on the event node of a non-polling gpio-keys instance to
ensure that irq-based input handling is not broken by the introduction of the
polling-mode gpio-keys patch. The non-polling instance creation and
registration is not included in this series since it uses one of the boot-mode
DIP switches and woult not (I think) be suitable for mainline.

Disclaimer:
I'm not an expert in irq's or gpio-keys; this is, in fact, my first proposed
feature. Please feel free to correct me -- I welcome the chance to learn from
your expertise.

Ben Gardiner (4):
da850-evm: add UI Expander pushbuttons
da850-evm: extract defines for SEL{A,B,C} pins in UI expander
da850-evm: add baseboard GPIO expander buttons, switches and LEDs
da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Gabor Juhos (1):
Input: add input driver for polled GPIO buttons

arch/arm/mach-davinci/Kconfig | 3 +
arch/arm/mach-davinci/board-da850-evm.c | 306 +++++++++++++++++++++++++++--
drivers/input/keyboard/Kconfig | 16 ++
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/gpio_keys_polled.c | 261 ++++++++++++++++++++++++
include/linux/gpio_keys.h | 2 +
6 files changed, 577 insertions(+), 12 deletions(-)
create mode 100644 drivers/input/keyboard/gpio_keys_polled.c

---
Changes since v5:
* included the final polled gpio keys driver which is now in 2.6.37-rc5
at 0e7d0c860a0dee49dacb7bbb248d1eba637075ad
* small changes to includes and structure names due to driver changes

Changes since v4:
* integrated Gabor Juhos' polling gpio button driver in place of the
gpio-keys patch of Paul Mundt and Alex Clouter
* dont' line break error messages (Sekhar Nori)
* whitespace cleanup (Sekhar Nori)

Changes since v3:
* introduced patch 5 in the series by extracting the Kconfig changes proposed
in patch 2 of v3.
* not gpio_request()'ing the sw_rst and deep_sleep_en lines as requested
(Sekhar Nori)

Changes since v2:
* register a single input device for switches and keys on the baseboard since
there is no benefit to separate devices with different polling intervals
(Dmitry Torokhov)
* use static array intialization and range intialization for platform data
structure to minimize the amount of runtime intialization needed:
(Sekhar Nori)
* Use the da850_evm variable name prefix for static symbols in
board-da850-evm.c

Changes since v1:
* use locally defined functions that are no-ops/error checkers when
INPUT_POLLDEV is not defined.
* disable polling mode support when input-polldev is a module and gpio_keys
is builtin
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used

2010-12-09 21:51:23

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 1/5] Input: add input driver for polled GPIO buttons

From: Gabor Juhos <[email protected]>

The existing gpio-keys driver can be usable only for GPIO lines with
interrupt support. Several devices have buttons connected to a GPIO
line which is not capable to generate interrupts. This patch adds a
new input driver using the generic GPIO layer and the input-polldev
to support such buttons.

[Ben Gardiner <[email protected]: fold code to use more
of the original gpio_keys infrastructure; cleanups and other
improvements.]

Signed-off-by: Gabor Juhos <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Tested-by: Ben Gardiner <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
(cherry picked from commit 0e7d0c860a0dee49dacb7bbb248d1eba637075ad)

---

This a copy of the commit -- I included in the patch series since
linux-davinci/master does not currently have it; but 2.6.37-rc5 does.

---
drivers/input/keyboard/Kconfig | 16 ++
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/gpio_keys_polled.c | 261 +++++++++++++++++++++++++++++
include/linux/gpio_keys.h | 2 +
4 files changed, 280 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/gpio_keys_polled.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b8c51b9..3a87f3b 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -179,6 +179,22 @@ config KEYBOARD_GPIO
To compile this driver as a module, choose M here: the
module will be called gpio_keys.

+config KEYBOARD_GPIO_POLLED
+ tristate "Polled GPIO buttons"
+ depends on GENERIC_GPIO
+ select INPUT_POLLDEV
+ help
+ This driver implements support for buttons connected
+ to GPIO pins that are not capable of generating interrupts.
+
+ Say Y here if your device has buttons connected
+ directly to such GPIO pins. Your board-specific
+ setup logic must also provide a platform device,
+ with configuration data saying which GPIOs are used.
+
+ To compile this driver as a module, choose M here: the
+ module will be called gpio_keys_polled.
+
config KEYBOARD_TCA6416
tristate "TCA6416 Keypad Support"
depends on I2C
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index a34452e..622de73 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_KEYBOARD_BFIN) += bf54x-keys.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_GPIO_POLLED) += gpio_keys_polled.o
obj-$(CONFIG_KEYBOARD_TCA6416) += tca6416-keypad.o
obj-$(CONFIG_KEYBOARD_HIL) += hil_kbd.o
obj-$(CONFIG_KEYBOARD_HIL_OLD) += hilkbd.o
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
new file mode 100644
index 0000000..4c17aff
--- /dev/null
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -0,0 +1,261 @@
+/*
+ * Driver for buttons on GPIO lines not capable of generating interrupts
+ *
+ * Copyright (C) 2007-2010 Gabor Juhos <[email protected]>
+ * Copyright (C) 2010 Nuno Goncalves <[email protected]>
+ *
+ * This file was based on: /drivers/input/misc/cobalt_btns.c
+ * Copyright (C) 2007 Yoichi Yuasa <[email protected]>
+ *
+ * also was based on: /drivers/input/keyboard/gpio_keys.c
+ * Copyright 2005 Phil Blundell
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
+
+#define DRV_NAME "gpio-keys-polled"
+
+struct gpio_keys_button_data {
+ int last_state;
+ int count;
+ int threshold;
+ int can_sleep;
+};
+
+struct gpio_keys_polled_dev {
+ struct input_polled_dev *poll_dev;
+ struct device *dev;
+ struct gpio_keys_platform_data *pdata;
+ struct gpio_keys_button_data data[0];
+};
+
+static void gpio_keys_polled_check_state(struct input_dev *input,
+ struct gpio_keys_button *button,
+ struct gpio_keys_button_data *bdata)
+{
+ int state;
+
+ if (bdata->can_sleep)
+ state = !!gpio_get_value_cansleep(button->gpio);
+ else
+ state = !!gpio_get_value(button->gpio);
+
+ if (state != bdata->last_state) {
+ unsigned int type = button->type ?: EV_KEY;
+
+ input_event(input, type, button->code,
+ !!(state ^ button->active_low));
+ input_sync(input);
+ bdata->count = 0;
+ bdata->last_state = state;
+ }
+}
+
+static void gpio_keys_polled_poll(struct input_polled_dev *dev)
+{
+ struct gpio_keys_polled_dev *bdev = dev->private;
+ struct gpio_keys_platform_data *pdata = bdev->pdata;
+ struct input_dev *input = dev->input;
+ int i;
+
+ for (i = 0; i < bdev->pdata->nbuttons; i++) {
+ struct gpio_keys_button_data *bdata = &bdev->data[i];
+
+ if (bdata->count < bdata->threshold)
+ bdata->count++;
+ else
+ gpio_keys_polled_check_state(input, &pdata->buttons[i],
+ bdata);
+ }
+}
+
+static void gpio_keys_polled_open(struct input_polled_dev *dev)
+{
+ struct gpio_keys_polled_dev *bdev = dev->private;
+ struct gpio_keys_platform_data *pdata = bdev->pdata;
+
+ if (pdata->enable)
+ pdata->enable(bdev->dev);
+}
+
+static void gpio_keys_polled_close(struct input_polled_dev *dev)
+{
+ struct gpio_keys_polled_dev *bdev = dev->private;
+ struct gpio_keys_platform_data *pdata = bdev->pdata;
+
+ if (pdata->disable)
+ pdata->disable(bdev->dev);
+}
+
+static int __devinit gpio_keys_polled_probe(struct platform_device *pdev)
+{
+ struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct gpio_keys_polled_dev *bdev;
+ struct input_polled_dev *poll_dev;
+ struct input_dev *input;
+ int error;
+ int i;
+
+ if (!pdata || !pdata->poll_interval)
+ return -EINVAL;
+
+ bdev = kzalloc(sizeof(struct gpio_keys_polled_dev) +
+ pdata->nbuttons * sizeof(struct gpio_keys_button_data),
+ GFP_KERNEL);
+ if (!bdev) {
+ dev_err(dev, "no memory for private data\n");
+ return -ENOMEM;
+ }
+
+ poll_dev = input_allocate_polled_device();
+ if (!poll_dev) {
+ dev_err(dev, "no memory for polled device\n");
+ error = -ENOMEM;
+ goto err_free_bdev;
+ }
+
+ poll_dev->private = bdev;
+ poll_dev->poll = gpio_keys_polled_poll;
+ poll_dev->poll_interval = pdata->poll_interval;
+ poll_dev->open = gpio_keys_polled_open;
+ poll_dev->close = gpio_keys_polled_close;
+
+ input = poll_dev->input;
+
+ input->evbit[0] = BIT(EV_KEY);
+ input->name = pdev->name;
+ input->phys = DRV_NAME"/input0";
+ input->dev.parent = &pdev->dev;
+
+ input->id.bustype = BUS_HOST;
+ input->id.vendor = 0x0001;
+ input->id.product = 0x0001;
+ input->id.version = 0x0100;
+
+ for (i = 0; i < pdata->nbuttons; i++) {
+ struct gpio_keys_button *button = &pdata->buttons[i];
+ struct gpio_keys_button_data *bdata = &bdev->data[i];
+ unsigned int gpio = button->gpio;
+ unsigned int type = button->type ?: EV_KEY;
+
+ if (button->wakeup) {
+ dev_err(dev, DRV_NAME " does not support wakeup\n");
+ error = -EINVAL;
+ goto err_free_gpio;
+ }
+
+ error = gpio_request(gpio,
+ button->desc ? button->desc : DRV_NAME);
+ if (error) {
+ dev_err(dev, "unable to claim gpio %u, err=%d\n",
+ gpio, error);
+ goto err_free_gpio;
+ }
+
+ error = gpio_direction_input(gpio);
+ if (error) {
+ dev_err(dev,
+ "unable to set direction on gpio %u, err=%d\n",
+ gpio, error);
+ goto err_free_gpio;
+ }
+
+ bdata->can_sleep = gpio_cansleep(gpio);
+ bdata->last_state = -1;
+ bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
+ pdata->poll_interval);
+
+ input_set_capability(input, type, button->code);
+ }
+
+ bdev->poll_dev = poll_dev;
+ bdev->dev = dev;
+ bdev->pdata = pdata;
+ platform_set_drvdata(pdev, bdev);
+
+ error = input_register_polled_device(poll_dev);
+ if (error) {
+ dev_err(dev, "unable to register polled device, err=%d\n",
+ error);
+ goto err_free_gpio;
+ }
+
+ /* report initial state of the buttons */
+ for (i = 0; i < pdata->nbuttons; i++)
+ gpio_keys_polled_check_state(input, &pdata->buttons[i],
+ &bdev->data[i]);
+
+ return 0;
+
+err_free_gpio:
+ while (--i >= 0)
+ gpio_free(pdata->buttons[i].gpio);
+
+ input_free_polled_device(poll_dev);
+
+err_free_bdev:
+ kfree(bdev);
+
+ platform_set_drvdata(pdev, NULL);
+ return error;
+}
+
+static int __devexit gpio_keys_polled_remove(struct platform_device *pdev)
+{
+ struct gpio_keys_polled_dev *bdev = platform_get_drvdata(pdev);
+ struct gpio_keys_platform_data *pdata = bdev->pdata;
+ int i;
+
+ input_unregister_polled_device(bdev->poll_dev);
+
+ for (i = 0; i < pdata->nbuttons; i++)
+ gpio_free(pdata->buttons[i].gpio);
+
+ input_free_polled_device(bdev->poll_dev);
+
+ kfree(bdev);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+static struct platform_driver gpio_keys_polled_driver = {
+ .probe = gpio_keys_polled_probe,
+ .remove = __devexit_p(gpio_keys_polled_remove),
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init gpio_keys_polled_init(void)
+{
+ return platform_driver_register(&gpio_keys_polled_driver);
+}
+
+static void __exit gpio_keys_polled_exit(void)
+{
+ platform_driver_unregister(&gpio_keys_polled_driver);
+}
+
+module_init(gpio_keys_polled_init);
+module_exit(gpio_keys_polled_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Gabor Juhos <[email protected]>");
+MODULE_DESCRIPTION("Polled GPIO Buttons driver");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ce73a30..dd1a56f 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -16,6 +16,8 @@ struct gpio_keys_button {
struct gpio_keys_platform_data {
struct gpio_keys_button *buttons;
int nbuttons;
+ unsigned int poll_interval; /* polling interval in msecs -
+ for polling driver only */
unsigned int rep:1; /* enable input subsystem auto repeat */
int (*enable)(struct device *dev);
void (*disable)(struct device *dev);
--
1.7.0.4

2010-12-09 21:51:27

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 3/5] da850-evm: extract defines for SEL{A,B,C} pins in UI expander

The setup and teardown methods of the UI expander reference the SEL_{A,B,C}
pins by 'magic number' in each function. This uses the common enum for their offsets
in the expander setup and teardown functions.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Victor Rodriguez <[email protected]>

---

Changes since v5:
* no changes in this patch of the series

Changes since v4:
* no changes in this patch of the series

Changes since v3:
* no changes in this patch of the series

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* integrated the static array initialization patch provided by Sekhar Nori

Changes since v1:
* No changes since v1
---
arch/arm/mach-davinci/board-da850-evm.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 4377679..dede268 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -360,23 +360,23 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
{
int sel_a, sel_b, sel_c, ret;

- sel_a = gpio + 7;
- sel_b = gpio + 6;
- sel_c = gpio + 5;
+ sel_a = gpio + DA850_EVM_UI_EXP_SEL_A;
+ sel_b = gpio + DA850_EVM_UI_EXP_SEL_B;
+ sel_c = gpio + DA850_EVM_UI_EXP_SEL_C;

- ret = gpio_request(sel_a, "sel_a");
+ ret = gpio_request(sel_a, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_A]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_a);
goto exp_setup_sela_fail;
}

- ret = gpio_request(sel_b, "sel_b");
+ ret = gpio_request(sel_b, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_B]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_b);
goto exp_setup_selb_fail;
}

- ret = gpio_request(sel_c, "sel_c");
+ ret = gpio_request(sel_c, da850_evm_ui_exp[DA850_EVM_UI_EXP_SEL_C]);
if (ret) {
pr_warning("Cannot open UI expander pin %d\n", sel_c);
goto exp_setup_selc_fail;
@@ -419,13 +419,13 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
platform_device_unregister(&da850_evm_ui_keys_device);

/* deselect all functionalities */
- gpio_set_value_cansleep(gpio + 5, 1);
- gpio_set_value_cansleep(gpio + 6, 1);
- gpio_set_value_cansleep(gpio + 7, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_C, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_B, 1);
+ gpio_set_value_cansleep(gpio + DA850_EVM_UI_EXP_SEL_A, 1);

- gpio_free(gpio + 5);
- gpio_free(gpio + 6);
- gpio_free(gpio + 7);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_C);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_B);
+ gpio_free(gpio + DA850_EVM_UI_EXP_SEL_A);

return 0;
}
--
1.7.0.4

2010-12-09 21:51:29

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 4/5] da850-evm: add baseboard GPIO expander buttons, switches and LEDs

This patch adds a pca953x platform device for the tca6416 found on the evm
baseboard. The tca6416 is a GPIO expander, also found on the UI board at a
separate I2C address. The pins of the baseboard IO expander are connected to
software reset, deep sleep enable, test points, a push button, DIP switches and
LEDs.

Add support for the push button, DIP switches and LEDs and test points (as
free GPIOs). The reset and deep sleep enable connections are reserved by the
setup routine so that userspace can't toggle those lines.

The existing tca6416-keypad driver was not employed because there was no
apararent way to register the LEDs connected to gpio's on the tca6416 while
simultaneously registering the tca6416-keypad instance.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
CC: Gabor Juhos <[email protected]>

---

Changes since v5:
* use platform_data type from final polled gpio keys driver

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver
* removed extra indent (Sekhar Nori)
* don't line-break error messages (Sekhar Nori)
* left-in the explicit static initialization of structure members

Changes since v3:
* don't request sw_rst and deep_sleep_en gpio pins -- let clients use them
freely

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in 1/4 instead of this patch
(Nori, Sekhar)
* ui_expander_names was renamed to da850_evm_ui_exp
* DA850_SW_POLL_MS definition moved to this patch from 3/4
* use indexed array initialization pattern introduced by Sekhar Nori in 3/4
* shorter names prefixed with da850_evm
* static array range intializers
* using only a single gpio-keys instance for the pushbutton and switches on
baseboard since there is no advantage to separate device instances with
different polling intervals (Dmitry Torokhov)

Changes since v1:
* adding note about why the tca6416-keypad driver was not used.
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 187 +++++++++++++++++++++++++++++++
1 files changed, 187 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index dede268..0de789b 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -430,6 +430,182 @@ static int da850_evm_ui_expander_teardown(struct i2c_client *client,
return 0;
}

+/* assign the baseboard expander's GPIOs after the UI board's */
+#define DA850_UI_EXPANDER_N_GPIOS ARRAY_SIZE(da850_evm_ui_exp)
+#define DA850_BB_EXPANDER_GPIO_BASE (DAVINCI_N_GPIO + DA850_UI_EXPANDER_N_GPIOS)
+
+enum da850_evm_bb_exp_pins {
+ DA850_EVM_BB_EXP_DEEP_SLEEP_EN = 0,
+ DA850_EVM_BB_EXP_SW_RST,
+ DA850_EVM_BB_EXP_TP_23,
+ DA850_EVM_BB_EXP_TP_22,
+ DA850_EVM_BB_EXP_TP_21,
+ DA850_EVM_BB_EXP_USER_PB1,
+ DA850_EVM_BB_EXP_USER_LED2,
+ DA850_EVM_BB_EXP_USER_LED1,
+ DA850_EVM_BB_EXP_USER_SW1,
+ DA850_EVM_BB_EXP_USER_SW2,
+ DA850_EVM_BB_EXP_USER_SW3,
+ DA850_EVM_BB_EXP_USER_SW4,
+ DA850_EVM_BB_EXP_USER_SW5,
+ DA850_EVM_BB_EXP_USER_SW6,
+ DA850_EVM_BB_EXP_USER_SW7,
+ DA850_EVM_BB_EXP_USER_SW8
+};
+
+static const char const *da850_evm_bb_exp[] = {
+ [DA850_EVM_BB_EXP_DEEP_SLEEP_EN] = "deep_sleep_en",
+ [DA850_EVM_BB_EXP_SW_RST] = "sw_rst",
+ [DA850_EVM_BB_EXP_TP_23] = "tp_23",
+ [DA850_EVM_BB_EXP_TP_22] = "tp_22",
+ [DA850_EVM_BB_EXP_TP_21] = "tp_21",
+ [DA850_EVM_BB_EXP_USER_PB1] = "user_pb1",
+ [DA850_EVM_BB_EXP_USER_LED2] = "user_led2",
+ [DA850_EVM_BB_EXP_USER_LED1] = "user_led1",
+ [DA850_EVM_BB_EXP_USER_SW1] = "user_sw1",
+ [DA850_EVM_BB_EXP_USER_SW2] = "user_sw2",
+ [DA850_EVM_BB_EXP_USER_SW3] = "user_sw3",
+ [DA850_EVM_BB_EXP_USER_SW4] = "user_sw4",
+ [DA850_EVM_BB_EXP_USER_SW5] = "user_sw5",
+ [DA850_EVM_BB_EXP_USER_SW6] = "user_sw6",
+ [DA850_EVM_BB_EXP_USER_SW7] = "user_sw7",
+ [DA850_EVM_BB_EXP_USER_SW8] = "user_sw8",
+};
+
+#define DA850_N_BB_USER_SW 8
+
+static struct gpio_keys_button da850_evm_bb_keys[] = {
+ [0] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = KEY_PROG1,
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+ [1 ... DA850_N_BB_USER_SW] = {
+ .type = EV_SW,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_bb_keys_pdata = {
+ .buttons = da850_evm_bb_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_bb_keys),
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_bb_keys_device = {
+ .name = "gpio-keys-polled",
+ .id = 1,
+ .dev = {
+ .platform_data = &da850_evm_bb_keys_pdata
+ },
+};
+
+static void da850_evm_bb_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ button = &da850_evm_bb_keys[0];
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_PB1];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_PB1;
+
+ for (i = 0; i < DA850_N_BB_USER_SW; i++) {
+ button = &da850_evm_bb_keys[i + 1];
+ button->code = SW_LID + i;
+ button->desc = (char *)
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_SW1 + i];
+ button->gpio = gpio + DA850_EVM_BB_EXP_USER_SW1 + i;
+ }
+}
+
+#define DA850_N_BB_USER_LED 2
+
+static struct gpio_led da850_evm_bb_leds[] = {
+ [0 ... DA850_N_BB_USER_LED - 1] = {
+ .active_low = 1,
+ .gpio = -1, /* assigned at runtime */
+ .name = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_led_platform_data da850_evm_bb_leds_pdata = {
+ .leds = da850_evm_bb_leds,
+ .num_leds = ARRAY_SIZE(da850_evm_bb_leds),
+};
+
+static struct platform_device da850_evm_bb_leds_device = {
+ .name = "leds-gpio",
+ .id = -1,
+ .dev = {
+ .platform_data = &da850_evm_bb_leds_pdata
+ }
+};
+
+static void da850_evm_bb_leds_init(unsigned gpio)
+{
+ int i;
+ struct gpio_led *led;
+
+ for (i = 0; i < DA850_N_BB_USER_LED; i++) {
+ led = &da850_evm_bb_leds[i];
+
+ led->gpio = gpio + DA850_EVM_BB_EXP_USER_LED2 + i;
+ led->name =
+ da850_evm_bb_exp[DA850_EVM_BB_EXP_USER_LED2 + i];
+ }
+}
+
+static int da850_evm_bb_expander_setup(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio,
+ void *c)
+{
+ int ret;
+
+ /*
+ * Register the switches and pushbutton on the baseboard as a gpio-keys
+ * device.
+ */
+ da850_evm_bb_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_keys_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander keys");
+ goto io_exp_setup_sw_fail;
+ }
+
+ da850_evm_bb_leds_init(gpio);
+ ret = platform_device_register(&da850_evm_bb_leds_device);
+ if (ret) {
+ pr_warning("Could not register baseboard GPIO expander LEDS");
+ goto io_exp_setup_leds_fail;
+ }
+
+ return 0;
+
+io_exp_setup_leds_fail:
+ platform_device_unregister(&da850_evm_bb_keys_device);
+io_exp_setup_sw_fail:
+ return ret;
+}
+
+static int da850_evm_bb_expander_teardown(struct i2c_client *client,
+ unsigned gpio, unsigned ngpio, void *c)
+{
+ platform_device_unregister(&da850_evm_bb_leds_device);
+ platform_device_unregister(&da850_evm_bb_keys_device);
+
+ return 0;
+}
+
static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
@@ -437,6 +613,13 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.names = da850_evm_ui_exp,
};

+static struct pca953x_platform_data da850_evm_bb_expander_info = {
+ .gpio_base = DA850_BB_EXPANDER_GPIO_BASE,
+ .setup = da850_evm_bb_expander_setup,
+ .teardown = da850_evm_bb_expander_teardown,
+ .names = da850_evm_bb_exp,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -445,6 +628,10 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
I2C_BOARD_INFO("tca6416", 0x20),
.platform_data = &da850_evm_ui_expander_info,
},
+ {
+ I2C_BOARD_INFO("tca6416", 0x21),
+ .platform_data = &da850_evm_bb_expander_info,
+ },
};

static struct davinci_i2c_platform_data da850_evm_i2c_0_pdata = {
--
1.7.0.4

2010-12-09 21:51:57

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 5/5] da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Use the mach-davinci/Kconfig to enable gpio-keys-polled as default when
da850-evm machine is enabled.

Signed-off-by: Ben Gardiner <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: "Nori, Sekhar" <[email protected]>
CC: Gabor Juhos <[email protected]>

---

Changes since v5:
* no changes in this patch of the series

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver

Changes since v3:
* no changes in this patch of the series / this patch was introduced in v4 of
the series
---
arch/arm/mach-davinci/Kconfig | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
index 84066e8..b93c327 100644
--- a/arch/arm/mach-davinci/Kconfig
+++ b/arch/arm/mach-davinci/Kconfig
@@ -180,6 +180,9 @@ endchoice
config GPIO_PCA953X
default MACH_DAVINCI_DA850_EVM

+config KEYBOARD_GPIO_POLLED
+ default MACH_DAVINCI_DA850_EVM
+
config MACH_TNETV107X
bool "TI TNETV107X Reference Platform"
default ARCH_DAVINCI_TNETV107X
--
1.7.0.4

2010-12-09 21:51:35

by Ben Gardiner

[permalink] [raw]
Subject: [PATCH v6 2/5] da850-evm: add UI Expander pushbuttons

This patch adds EV_KEYs for each of the 8 pushbuttons on the UI board via a
gpio-key device.

The expander is a tca6416; it controls the SEL_{A,B,C} lines which enable and
disable the peripherals found on the UI board in addition to the 8 pushbuttons
mentioned above. The reason the existing tca6416-keypad driver is not employed
is because there was no aparent way to keep the gpio lines used as
SEL_{A,B,C} registered while simultaneously registering the pushbuttons as a
tca6416-keypad instance.

Some experimentation with the polling interval was performed; we were searching
for the largest polling interval that did not affect the feel of the
responsiveness of the buttons. It is very subjective but 200ms seems to be a
good value that accepts firm pushes but rejects very light ones. The key values
assigned to the buttons were arbitrarily chosen to be F1-F8.

Signed-off-by: Ben Gardiner <[email protected]>
Reviewed-by: Chris Cordahi <[email protected]>
CC: Govindarajan, Sriramakrishnan <[email protected]>
Reviewed-by: Sekhar Nori <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Gabor Juhos <[email protected]>

---
Changes since v5:
* use header and platform_data type from final polled gpio keys driver

Changes since v4:
* integrated the use of Gabor Juhos' polled gpio buttons driver
* removed spurious whitespace change (Sekhar Nori)
* don't linebreak error messages (Sekhar Nori)
* kept the explicit static initialization of structure members in-place

Changes since v3:
* extracted Kconfig changes to patch 5/5
* fixed leading whitespace problem

Changes since v2:
* rebased to 083eae3e28643e0eefc5243719f8b1572cf98299 of
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git
* remove the "TODO : populate at runtime using" in this patch instead of 4/4
(Nori, Sekhar)
* integrated the static array initialization patch of Sekhar Nori
* use static array initialization ranges
* rename DA850_PB_POLL_MS to DA850_GPIO_KEYS_POLL_MS
* use shorter names prefixed with da850_evm

Changes since v1:
* set INPUT_POLLDEV default for DA850_EVM machine, but don't select it
unconditionally
* adding note to description about why tca6416-keypad was not used
* adding Govindarajan, Sriramakrishnan, the author of the tca6416-keypad
driver
---
arch/arm/mach-davinci/board-da850-evm.c | 95 +++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index f89b0b7..4377679 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -17,8 +17,10 @@
#include <linux/i2c.h>
#include <linux/i2c/at24.h>
#include <linux/i2c/pca953x.h>
+#include <linux/input.h>
#include <linux/mfd/tps6507x.h>
#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
#include <linux/platform_device.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -272,6 +274,87 @@ static inline void da850_evm_setup_emac_rmii(int rmii_sel)
static inline void da850_evm_setup_emac_rmii(int rmii_sel) { }
#endif

+
+#define DA850_KEYS_DEBOUNCE_MS 10
+/*
+ * At 200ms polling interval it is possible to miss an
+ * event by tapping very lightly on the push button but most
+ * pushes do result in an event; longer intervals require the
+ * user to hold the button whereas shorter intervals require
+ * more CPU time for polling.
+ */
+#define DA850_GPIO_KEYS_POLL_MS 200
+
+enum da850_evm_ui_exp_pins {
+ DA850_EVM_UI_EXP_SEL_C = 5,
+ DA850_EVM_UI_EXP_SEL_B,
+ DA850_EVM_UI_EXP_SEL_A,
+ DA850_EVM_UI_EXP_PB8,
+ DA850_EVM_UI_EXP_PB7,
+ DA850_EVM_UI_EXP_PB6,
+ DA850_EVM_UI_EXP_PB5,
+ DA850_EVM_UI_EXP_PB4,
+ DA850_EVM_UI_EXP_PB3,
+ DA850_EVM_UI_EXP_PB2,
+ DA850_EVM_UI_EXP_PB1,
+};
+
+static const char const *da850_evm_ui_exp[] = {
+ [DA850_EVM_UI_EXP_SEL_C] = "sel_c",
+ [DA850_EVM_UI_EXP_SEL_B] = "sel_b",
+ [DA850_EVM_UI_EXP_SEL_A] = "sel_a",
+ [DA850_EVM_UI_EXP_PB8] = "pb8",
+ [DA850_EVM_UI_EXP_PB7] = "pb7",
+ [DA850_EVM_UI_EXP_PB6] = "pb6",
+ [DA850_EVM_UI_EXP_PB5] = "pb5",
+ [DA850_EVM_UI_EXP_PB4] = "pb4",
+ [DA850_EVM_UI_EXP_PB3] = "pb3",
+ [DA850_EVM_UI_EXP_PB2] = "pb2",
+ [DA850_EVM_UI_EXP_PB1] = "pb1",
+};
+
+#define DA850_N_UI_PB 8
+
+static struct gpio_keys_button da850_evm_ui_keys[] = {
+ [0 ... DA850_N_UI_PB - 1] = {
+ .type = EV_KEY,
+ .active_low = 1,
+ .wakeup = 0,
+ .debounce_interval = DA850_KEYS_DEBOUNCE_MS,
+ .code = -1, /* assigned at runtime */
+ .gpio = -1, /* assigned at runtime */
+ .desc = NULL, /* assigned at runtime */
+ },
+};
+
+static struct gpio_keys_platform_data da850_evm_ui_keys_pdata = {
+ .buttons = da850_evm_ui_keys,
+ .nbuttons = ARRAY_SIZE(da850_evm_ui_keys),
+ .poll_interval = DA850_GPIO_KEYS_POLL_MS,
+};
+
+static struct platform_device da850_evm_ui_keys_device = {
+ .name = "gpio-keys-polled",
+ .id = 0,
+ .dev = {
+ .platform_data = &da850_evm_ui_keys_pdata
+ },
+};
+
+static void da850_evm_ui_keys_init(unsigned gpio)
+{
+ int i;
+ struct gpio_keys_button *button;
+
+ for (i = 0; i < DA850_N_UI_PB; i++) {
+ button = &da850_evm_ui_keys[i];
+ button->code = KEY_F8 - i;
+ button->desc = (char *)
+ da850_evm_ui_exp[DA850_EVM_UI_EXP_PB8 + i];
+ button->gpio = gpio + DA850_EVM_UI_EXP_PB8 + i;
+ }
+}
+
static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
unsigned ngpio, void *c)
{
@@ -304,6 +387,13 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,
gpio_direction_output(sel_b, 1);
gpio_direction_output(sel_c, 1);

+ da850_evm_ui_keys_init(gpio);
+ ret = platform_device_register(&da850_evm_ui_keys_device);
+ if (ret) {
+ pr_warning("Could not register UI GPIO expander push-buttons");
+ goto exp_setup_keys_fail;
+ }
+
ui_card_detected = 1;
pr_info("DA850/OMAP-L138 EVM UI card detected\n");

@@ -313,6 +403,8 @@ static int da850_evm_ui_expander_setup(struct i2c_client *client, unsigned gpio,

return 0;

+exp_setup_keys_fail:
+ gpio_free(sel_c);
exp_setup_selc_fail:
gpio_free(sel_b);
exp_setup_selb_fail:
@@ -324,6 +416,8 @@ exp_setup_sela_fail:
static int da850_evm_ui_expander_teardown(struct i2c_client *client,
unsigned gpio, unsigned ngpio, void *c)
{
+ platform_device_unregister(&da850_evm_ui_keys_device);
+
/* deselect all functionalities */
gpio_set_value_cansleep(gpio + 5, 1);
gpio_set_value_cansleep(gpio + 6, 1);
@@ -340,6 +434,7 @@ static struct pca953x_platform_data da850_evm_ui_expander_info = {
.gpio_base = DAVINCI_N_GPIO,
.setup = da850_evm_ui_expander_setup,
.teardown = da850_evm_ui_expander_teardown,
+ .names = da850_evm_ui_exp,
};

static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
--
1.7.0.4

2010-12-10 15:50:26

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v6 1/5] Input: add input driver for polled GPIO buttons

Ben Gardiner <[email protected]> writes:

> From: Gabor Juhos <[email protected]>
>
> The existing gpio-keys driver can be usable only for GPIO lines with
> interrupt support. Several devices have buttons connected to a GPIO
> line which is not capable to generate interrupts. This patch adds a
> new input driver using the generic GPIO layer and the input-polldev
> to support such buttons.
>
> [Ben Gardiner <[email protected]: fold code to use more
> of the original gpio_keys infrastructure; cleanups and other
> improvements.]
>
> Signed-off-by: Gabor Juhos <[email protected]>
> Signed-off-by: Ben Gardiner <[email protected]>
> Tested-by: Ben Gardiner <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> (cherry picked from commit 0e7d0c860a0dee49dacb7bbb248d1eba637075ad)
>
> ---
>
> This a copy of the commit -- I included in the patch series since
> linux-davinci/master does not currently have it; but 2.6.37-rc5 does.

I'll be updating davinci master to .37-rc5 today, so will drop this
patch.

Kevin

2010-12-10 16:16:50

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Hi Ben,

Ben Gardiner <[email protected]> writes:

> The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
> They are bootstrapped to different I2C addresses so they can be used
> concurrently.
>
> The expander on the UI board is currently used to enable/disable the
> peripherals that are available on the UI board. In addition to this
> functionality the expander is also connected to 8 pushbuttons. The expander
> on the baseboard is not currently used; it is connected to deep sleep enable,
> sw reset, a push button, some switches and LEDs.
>
> This proposed patch series enables the push buttons and switches on the UI and
> BB expanders using the gpio-keys polling mode patch by Gabor Juhos. Some
> work was performed to test irq-based gpio-keys support on the expanders (a WIP
> patch can be posted on request) but I believe that it is not possible to use
> irq-based gpio-keys on IO expanders for arm systems at this time.

Thanks for your patience and persistence on this series, and thanks for
working closely with the input folks to get the issues worked through.

This series looks good to me, so I'll be queuing it in davinci-next for
2.6.38. It should show up in davinci git shortly. Please validate
things are working as expected there. Were there any changes needed to
the defaults in da8xx_omapl_defconfig to enable these features by
default? or does the Kconfig change in PATCH 5/5 cover it?

Also, I really appreciate the thorough patch descriptions and history
information. This greatly eases the work of maintainers. Thanks!

One minor question: the series has a couple of Signed-off-by tags from
Sekhar Nori. The s-o-b tag is for folks on the delivery path, and based
on what I saw, these should probably be Acked-by tags from Sekhar, since
he certainly helped on the review/test/validate side, but AFAICT, was
not an author or on the delivery path. If I'm wrong on this (e.g., if
Sekhar actually did author some of those patches) let me know, otherwise
I'll change the s-o-b to Acked-by for Sekhar.

Thanks,

Kevin

2010-12-10 16:33:44

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Hi Kevin,

On Fri, Dec 10, 2010 at 11:16 AM, Kevin Hilman
<[email protected]> wrote:
> Hi Ben,
>
> Ben Gardiner <[email protected]> writes:
>
>> The da850-evm baseboard (BB) and its UI board both have tca6416 IO expanders.
>> They are bootstrapped to different I2C addresses so they can be used
>> concurrently.
>>
>> The expander on the UI board is currently used to enable/disable the
>> peripherals that are available on the UI board. In addition to this
>> functionality the expander is also connected to 8 pushbuttons. The expander
>> on the baseboard is not currently used; it is connected to deep sleep enable,
>> sw reset, a push button, some switches and LEDs.
>>
>> This proposed patch series enables the push buttons and switches on the UI and
>> BB expanders using the gpio-keys polling mode patch by Gabor Juhos. Some
>> work was performed to test irq-based gpio-keys support on the expanders (a WIP
>> patch can be posted on request) but I believe that it is not possible to use
>> irq-based gpio-keys on IO expanders for arm systems at this time.
>
> Thanks for your patience and persistence on this series, and thanks for
> working closely with the input folks to get the issues worked through.

It is my pleasure.

> This series looks good to me, so I'll be queuing it in davinci-next for
> 2.6.38. ?It should show up in davinci git shortly. ?Please validate
> things are working as expected there. ?Were there any changes needed to
> the defaults in da8xx_omapl_defconfig to enable these features by
> default? ?or does the Kconfig change in PATCH 5/5 cover it?

Thank you very much, Kevin.

I will check linux-davinci/master on monday.

Yes, the 5/5 covers the necessary Kconfig changes; it makes the polled
gpio keys default-on for da850evm.

> Also, I really appreciate the thorough patch descriptions and history
> information. ? This greatly eases the work of maintainers. ?Thanks!

Again, you are most welcome.

> One minor question: the series has a couple of Signed-off-by tags from
> Sekhar Nori. ?The s-o-b tag is for folks on the delivery path, and based
> on what I saw, these should probably be Acked-by tags from Sekhar, since
> he certainly helped on the review/test/validate side, but AFAICT, was
> not an author or on the delivery path. ?If I'm wrong on this (e.g., if
> Sekhar actually did author some of those patches) let me know, otherwise
> I'll change the s-o-b to Acked-by for Sekhar.

The s-o-b 's for Sehkar are because I folded-in suggested changes
submitted in review by Sehkar in the form of a patch. I 'think' this
qualifies as authorship. I'll leave it to your good judgement.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-12-11 00:04:39

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Hi Ben,
Ben Gardiner <[email protected]> writes:

[...]

>> One minor question: the series has a couple of Signed-off-by tags from
>> Sekhar Nori.  The s-o-b tag is for folks on the delivery path, and based
>> on what I saw, these should probably be Acked-by tags from Sekhar, since
>> he certainly helped on the review/test/validate side, but AFAICT, was
>> not an author or on the delivery path.  If I'm wrong on this (e.g., if
>> Sekhar actually did author some of those patches) let me know, otherwise
>> I'll change the s-o-b to Acked-by for Sekhar.
>
> The s-o-b 's for Sehkar are because I folded-in suggested changes
> submitted in review by Sehkar in the form of a patch. I 'think' this
> qualifies as authorship. I'll leave it to your good judgement.

OK, thanks for clarification. I'll leave the s-o-b tags as is.

Kevin

2010-12-13 17:02:38

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Hi Kevin,

On Fri, Dec 10, 2010 at 11:33 AM, Ben Gardiner
<[email protected]> wrote:
> On Fri, Dec 10, 2010 at 11:16 AM, Kevin Hilman
>> [...]
>> This series looks good to me, so I'll be queuing it in davinci-next for
>> 2.6.38. ?It should show up in davinci git shortly. [...]
>
> Thank you very much, Kevin.
>
> I will check linux-davinci/master on monday.

I looked at git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git#davinci-next
; HEAD at the time was

commit 3004ce0d3a44525de63e18b01f7734bc8d64f2c5
Author: Ben Gardiner <[email protected]>
Date: Thu Dec 9 16:51:07 2010 -0500

da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Use the mach-davinci/Kconfig to enable gpio-keys-polled as default when
da850-evm machine is enabled.

Signed-off-by: Ben Gardiner <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: "Nori, Sekhar" <[email protected]>
CC: Gabor Juhos <[email protected]>
Signed-off-by: Kevin Hilman <[email protected]>

Everything seems to be in order there; I tested the resulting kernel
with evtest and the expected output was observed. Note that
davinci-next still contains the cherry-pick of the upstream commit of
the polled gpio keys driver:

commit 03b79201321d53acc56b43cdd9d43e869f62021c
Author: Gabor Juhos <[email protected]>
Date: Thu Dec 9 16:51:03 2010 -0500

Input: add input driver for polled GPIO buttons

The existing gpio-keys driver can be usable only for GPIO lines with
interrupt support. Several devices have buttons connected to a GPIO
line which is not capable to generate interrupts. This patch adds a
new input driver using the generic GPIO layer and the input-polldev
to support such buttons.

[Ben Gardiner <[email protected]: fold code to use more
of the original gpio_keys infrastructure; cleanups and other
improvements.]

Signed-off-by: Gabor Juhos <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Tested-by: Ben Gardiner <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
(cherry picked from commit 0e7d0c860a0dee49dacb7bbb248d1eba637075ad)
Signed-off-by: Kevin Hilman <[email protected]>

I also looked at
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git#master
; HEAD at the time was

commit 4bfbdddc0655a284a98304bc343c6bcaf81b8e4d
Merge: 771215c 7507fd3
Author: Kevin Hilman <[email protected]>
Date: Fri Dec 10 16:25:27 2010 -0800

rebuild linux-davinci from branches

There appears to be some double commits of the patch series. I tested
it with evtest anyways and also observed the expected output.

The following command and its output hopefully demonstrate what I am
seeing as double commits.

$git log --format="%s -- %Cgreen%an <%aE> %Cred%ai %Creset%h" khilman/master
rebuild linux-davinci from branches -- Kevin Hilman
<[email protected]> 2010-12-10 16:25:27 -0800 4bfbddd
Merge branch 'davinci-orphaned' into davinci-reset -- Kevin Hilman
<[email protected]> 2010-12-10 16:25:22 -0800 771215c
da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional -- Ben Gardiner
<[email protected]> 2010-12-09 16:51:07 -0500 3004ce0
da850-evm: add baseboard GPIO expander buttons, switches and LEDs --
Ben Gardiner <[email protected]> 2010-12-09 16:51:06 -0500
61eaa3a
da850-evm: extract defines for SEL{A,B,C} pins in UI expander -- Ben
Gardiner <[email protected]> 2010-12-09 16:51:05 -0500
e594a12
da850-evm: add UI Expander pushbuttons -- Ben Gardiner
<[email protected]> 2010-12-09 16:51:04 -0500 260660f
davinci: am18x/da850/omap-l138 evm: add support for higher speed
grades -- Sekhar Nori <[email protected]> 2010-12-09 14:11:34 +0530
3c812e7
davinci: am18x/da850/omap-l138: add support for higher speed grades
-- Sekhar Nori <[email protected]> 2010-12-09 14:11:33 +0530 d2e9976
rebuild linux-davinci from branches -- Kevin Hilman
<[email protected]> 2010-12-10 08:13:18 -0800 7507fd3
Merge branch 'davinci-orphaned' into davinci-reset -- Kevin Hilman
<[email protected]> 2010-12-10 08:13:13 -0800 66a897a
da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional -- Ben Gardiner
<[email protected]> 2010-12-09 16:51:07 -0500 bc1c63f
da850-evm: add baseboard GPIO expander buttons, switches and LEDs --
Ben Gardiner <[email protected]> 2010-12-09 16:51:06 -0500
c6c519c
da850-evm: extract defines for SEL{A,B,C} pins in UI expander -- Ben
Gardiner <[email protected]> 2010-12-09 16:51:05 -0500
22693bc
da850-evm: add UI Expander pushbuttons -- Ben Gardiner
<[email protected]> 2010-12-09 16:51:04 -0500 b481719
Input: add input driver for polled GPIO buttons -- Gabor Juhos
<[email protected]> 2010-12-09 16:51:03 -0500 03b7920
ARM: Add writethrough dcache support for ARM926EJS processor -- Mark
A. Greer <[email protected]> 2009-05-11 15:36:54 -0700 ecc2acc
fb: davinci: DMx driver -- Kevin Hilman
<[email protected]> 2009-10-12 14:05:30 -0700 577b52a
ASoC: davinci: SFFSDR board updates -- Kevin Hilman
<[email protected]> 2009-10-12 14:03:47 -0700 cdbc657
davinci: dm365evm_keys driver -- David Brownell
<[email protected]> 2009-06-25 17:03:21 -0700 cf4e892
davinci: kconfig: select at24 eeprom for selected boards -- Kevin
Hilman <[email protected]> 2010-11-19 07:25:30 -0800 22ca466
da850-evm, trivial: use da850_evm prefix for consistency -- Ben
Gardiner <[email protected]> 2010-11-19 16:43:04 -0500
3506f27
da850-evm: allow pca953x module build -- Ben Gardiner
<[email protected]> 2010-11-19 09:17:35 -0500 d5539ca
davinci: da850-evm: UI expander gpio_set_value can sleep, use
_cansleep -- Ben Gardiner <[email protected]> 2010-11-15
09:42:52 -0500 47e7cb1
davinci: aemif: signedness bug in davinci_aemif_setup_timing() --
Nicolas Kaiser <[email protected]> 2010-11-15 19:40:28 +0100 12cdd3d
davinci: psc: simplify if-statement -- Nicolas Kaiser
<[email protected]> 2010-10-25 14:41:18 +0200 1a07bfb
davinci: minor tnetv107x clock tree fixes -- Cyril Chemparathy
<[email protected]> 2010-10-20 17:49:57 -0400 ced9862
davinci: use divide ratio limits from pll_data -- Cyril Chemparathy
<[email protected]> 2010-10-20 17:49:56 -0400 b1d05be
davinci: Implement sched_clock() -- Andreas Gaeer
<[email protected]> 2010-10-06 10:38:55 +0200 6d1c57c
Linux 2.6.37-rc5 -- Linus Torvalds <[email protected]>
2010-12-06 20:09:04 -0800 cf7d7e5
[...]

Note that the cherry pick of the upstream commit of the polled gpio
keys driver is here in 'master' also.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-12-13 21:53:56

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Ben Gardiner <[email protected]> writes:

> Hi Kevin,
>
> On Fri, Dec 10, 2010 at 11:33 AM, Ben Gardiner
> <[email protected]> wrote:
>> On Fri, Dec 10, 2010 at 11:16 AM, Kevin Hilman
>>> [...]
>>> This series looks good to me, so I'll be queuing it in davinci-next for
>>> 2.6.38.  It should show up in davinci git shortly. [...]
>>
>> Thank you very much, Kevin.
>>
>> I will check linux-davinci/master on monday.
>
> I looked at git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git#davinci-next
> ; HEAD at the time was
>
> commit 3004ce0d3a44525de63e18b01f7734bc8d64f2c5
> Author: Ben Gardiner <[email protected]>
> Date: Thu Dec 9 16:51:07 2010 -0500
>
> da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional
>
> Use the mach-davinci/Kconfig to enable gpio-keys-polled as default when
> da850-evm machine is enabled.
>
> Signed-off-by: Ben Gardiner <[email protected]>
> CC: Kevin Hilman <[email protected]>
> CC: "Nori, Sekhar" <[email protected]>
> CC: Gabor Juhos <[email protected]>
> Signed-off-by: Kevin Hilman <[email protected]>
>
> Everything seems to be in order there; I tested the resulting kernel
> with evtest and the expected output was observed. Note that
> davinci-next still contains the cherry-pick of the upstream commit of
> the polled gpio keys driver:

oops... I've now removed that, since it is part of v2.6.36-rc5 already.
Thanks for checking.

[...]
> I also looked at
> git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git#master
> ; HEAD at the time was
>
> commit 4bfbdddc0655a284a98304bc343c6bcaf81b8e4d
> Merge: 771215c 7507fd3
> Author: Kevin Hilman <[email protected]>
> Date: Fri Dec 10 16:25:27 2010 -0800
>
> rebuild linux-davinci from branches
>
> There appears to be some double commits of the patch series. I tested
> it with evtest anyways and also observed the expected output.
>
> The following command and its output hopefully demonstrate what I am
> seeing as double commits.

Yes, there will be double commits in master, because of the way I manage
master using 'git merge -ours'. But there shouldn't be double commits
between my "rebuild from braches" merges. It can be confusing, but if
you look at the history with a graphical tool like 'gitk', it might shed
some light on what is going on.

I know it's confusing, but the davinci-next branch is the only important
branch in this tree for upstream purposes.

[...]

> Note that the cherry pick of the upstream commit of the polled gpio
> keys driver is here in 'master' also.

Yeah, that came from davinc-next. Now that it's removed from there, it
should be ok.

I just pushed an updated davinci-next and master branch. It sometimes
takes a bit to propagate to all the kernel.org mirrors, but the update
should be there shortly.

Thanks,

Kevin

2010-12-14 04:31:13

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

On Mon, Dec 13, 2010 at 4:53 PM, Kevin Hilman
<[email protected]> wrote:
> Ben Gardiner <[email protected]> writes:
> [...]
> Yes, there will be double commits in master, because of the way I manage
> master using 'git merge -ours'. ?But there shouldn't be double commits
> between my "rebuild from braches" merges. ?It can be confusing, but if
> you look at the history with a graphical tool like 'gitk', it might shed
> some light on what is going on.

Ok. I'll consider a graphical investigation. Thanks for confirming the
state of master is as-designed.

> I know it's confusing, but the davinci-next branch is the only important
> branch in this tree for upstream purposes.

Understood. Thanks for the clarification.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-12-14 16:17:17

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

On Mon, Dec 13, 2010 at 4:53 PM, Kevin Hilman
<[email protected]> wrote:
> Ben Gardiner <[email protected]> writes:
>> [...]
>> Everything seems to be in order there; I tested the resulting kernel
>> with evtest and the expected output was observed. Note that
>> davinci-next still contains the cherry-pick of the upstream commit of
>> the polled gpio keys driver:
>
> oops... I've now removed that, since it is part of v2.6.36-rc5 already.
> Thanks for checking.

Oops on my part...

I looked at git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git#davinci-next
again ; HEAD at the time was:

commit 5682cfebad5e68736f4c54a268b5b371698f5262
Author: Ben Gardiner <[email protected]>
Date: Thu Dec 9 16:51:07 2010 -0500

da850-evm: KEYBOARD_GPIO_POLLED Kconfig conditional

Use the mach-davinci/Kconfig to enable gpio-keys-polled as default when
da850-evm machine is enabled.

Signed-off-by: Ben Gardiner <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: "Nori, Sekhar" <[email protected]>
CC: Gabor Juhos <[email protected]>
Signed-off-by: Kevin Hilman <[email protected]>

and I was unable to build a kernel from da8xx_omapl_defconfig b/c:

arch/arm/mach-davinci/board-da850-evm.c:333: error: unknown field
'poll_interval' specified in initializer

It appears that dropping the cherry-pick caused the build failure.

The commit that introduces the polled gpio keys driver (which was
included in the series as a cherry pick) is commit
0e7d0c860a0dee49dacb7bbb248d1eba637075ad which is in
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git#master
_after_ the tag v2.6.37-rc5.

That's my fault. I incorrectly thought that commit
0e7d0c860a0dee49dacb7bbb248d1eba637075ad was _in_ 2.6.37-rc5 and
stated this is previous emails. I'm sorry for the confusion; I think I
jumped the gun there due to my excitement at getting this prerequisite
driver committed.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

2010-12-14 17:10:26

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Ben Gardiner <[email protected]> writes:

> On Mon, Dec 13, 2010 at 4:53 PM, Kevin Hilman
> <[email protected]> wrote:
>> Ben Gardiner <[email protected]> writes:
>>> [...]
>>> Everything seems to be in order there; I tested the resulting kernel
>>> with evtest and the expected output was observed. Note that
>>> davinci-next still contains the cherry-pick of the upstream commit of
>>> the polled gpio keys driver:
>>
>> oops... I've now removed that, since it is part of v2.6.36-rc5 already.
>> Thanks for checking.
>
> Oops on my part...
>

[...]

> It appears that dropping the cherry-pick caused the build failure.
>
> The commit that introduces the polled gpio keys driver (which was
> included in the series as a cherry pick) is commit
> 0e7d0c860a0dee49dacb7bbb248d1eba637075ad which is in
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git#master
> _after_ the tag v2.6.37-rc5.
>
> That's my fault. I incorrectly thought that commit
> 0e7d0c860a0dee49dacb7bbb248d1eba637075ad was _in_ 2.6.37-rc5 and
> stated this is previous emails. I'm sorry for the confusion; I think I
> jumped the gun there due to my excitement at getting this prerequisite
> driver committed.

OK, while waiting for it to arrive upstream, I've added it to my
'davinci-backports' branch, which is also merged into the master branch
of davinci git (but not in davinci-next, since it will go upstream via
another subsystem.)

Just pushed an updated version, and this time, I actually build tested for
davinci_all_defconfig and da8xx_omapl_defconfig. :)

Sorry for the churn,

Kevin

2010-12-17 15:15:37

by Ben Gardiner

[permalink] [raw]
Subject: Re: [PATCH v6 0/5] da850-evm: add gpio-{keys,leds} for UI and BB expanders

Hi Kevin,

On Tue, Dec 14, 2010 at 12:10 PM, Kevin Hilman
<[email protected]> wrote:
> Just pushed an updated version, and this time, I actually build tested for
> davinci_all_defconfig and da8xx_omapl_defconfig. :)
>
> Sorry for the churn,

I tested linux-davinci master; HEAD was
c63d0f1df0284f2fcb28c0943b17848500d01515. Everything is working as
expected.

Thanks again for taking up this series along with the complications
introduced by the cherry pick.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca