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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v4 2/5] da850-evm: add UI Expander pushbuttons
Hi Sekhar,
On Wed, Nov 24, 2010 at 8:16 AM, Nori, Sekhar nsek...@ti.com 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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[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 bengardi...@nanometrics.ca
Reviewed-by: Chris Cordahi christophercord...@nanometrics.ca
CC: Govindarajan, Sriramakrishnan s...@ti.com
Reviewed-by: Sekhar Nori nsek...@ti.com
Signed-off-by: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
---
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_MS200
+
+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 =
