adp5588-keys datasheet and driver question

Hi Michael,

 I had a look at the adp5588-keys driver and its datasheet.

There are some "slips of the pen" in it (adp5588 Rev.b):
  - Page 8, Table 12, "E4 pressed" refers to binary 1000 instead of 100
  - Page 17, Table 22, Register Descriptions are mostly wrong

Another thing I'm curious about is in adp5588_report_events():

int key = adp5588_read(kpad->client, Key_EVENTA + i);
int key_val = key & KEY_EV_MASK;
[...]
input_report_key(kpad->input, kpad->keycode[key_val - 1], key & KEY_EV_PRESSED);

Why is there a -1 for key_val?

Due to lack of hardware and understanding the datasheet, I'm unsure
about my patch below.

Thanks,
   Christoph

---
Subject: [PATCH] Input: adp5588-keys - fix handling of Key A0

This patch removes a -1 subtraction from the device its transmitted key
event register.  This allows correct handling of Key A0 which reports 0.
---
 drivers/input/keyboard/adp5588-keys.c |   10 +++++-----
 include/linux/i2c/adp5588.h           |    1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 4a7f534..23f9417 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c

@@ -261,8 +261,8 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i, j;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key = adp5588_read(kpad->client, Key_EVENTA + i);
-		int key_val = key & KEY_EV_MASK;
+		unsigned int key = adp5588_read(kpad->client, Key_EVENTA + i);
+		unsigned int key_val = key & KEY_EV_MASK;
 
 		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
 			for (j = 0; j < kpad->gpimapsize; j++) {

@@ -273,9 +273,9 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 					break;
 				}
 			}
-		} else {
+		} else if (key_val <= ADP5588_KEYMAPSIZE) {
 			input_report_key(kpad->input,
-					 kpad->keycode[key_val - 1],
+					 kpad->keycode[key_val],
 					 key & KEY_EV_PRESSED);
 		}
 	}

@@ -295,7 +295,7 @@ static void adp5588_work(struct work_struct *work)
 
 	if (status & ADP5588_KE_INT) {
 		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt) {
+		if (ev_cnt && ev_cnt <= ADP5588_MAXEVENTS) {
 			adp5588_report_events(kpad, ev_cnt);
 			input_sync(kpad->input);
 		}

diff --git a/include/linux/i2c/adp5588.h b/include/linux/i2c/adp5588.h
index cec17cf..394c07f 100644
--- a/include/linux/i2c/adp5588.h
+++ b/include/linux/i2c/adp5588.h

@@ -103,6 +103,7 @@
 
 /* Put one of these structures in i2c_board_info platform_data */
 
+#define ADP5588_MAXEVENTS	10
 #define ADP5588_KEYMAPSIZE	80
 
 #define GPI_PIN_ROW0 97

-- 
1.7.2.5

forgot to add to commit:
-	unsigned short keycode[ADP5588_KEYMAPSIZE];
+	unsigned short keycode[ADP5588_KEYMAPSIZE + 1];

On Sat, 2011-12-03 at 17:00 +0100, Christoph Fritz wrote:

Hi Michael,

 I had a look at the adp5588-keys driver and its datasheet.

There are some "slips of the pen" in it (adp5588 Rev.b):
  - Page 8, Table 12, "E4 pressed" refers to binary 1000 instead of 100
  - Page 17, Table 22, Register Descriptions are mostly wrong

Another thing I'm curious about is in adp5588_report_events():

int key = adp5588_read(kpad->client, Key_EVENTA + i);
int key_val = key & KEY_EV_MASK;
[...]
input_report_key(kpad->input, kpad->keycode[key_val - 1], key & KEY_EV_PRESSED);

Why is there a -1 for key_val?

Due to lack of hardware and understanding the datasheet, I'm unsure
about my patch below.

Thanks,
   Christoph

---
Subject: [PATCH] Input: adp5588-keys - fix handling of Key A0

This patch removes a -1 subtraction from the device its transmitted key
event register.  This allows correct handling of Key A0 which reports 0.
---
 drivers/input/keyboard/adp5588-keys.c |   12 ++++++------
 include/linux/i2c/adp5588.h           |    1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 4a7f534..c0f0276 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c

@@ -43,7 +43,7 @@ struct adp5588_kpad {
 	struct input_dev *input;
 	struct delayed_work work;
 	unsigned long delay;
-	unsigned short keycode[ADP5588_KEYMAPSIZE];
+	unsigned short keycode[ADP5588_KEYMAPSIZE + 1];
 	const struct adp5588_gpi_map *gpimap;
 	unsigned short gpimapsize;
 #ifdef CONFIG_GPIOLIB

@@ -261,8 +261,8 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i, j;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key = adp5588_read(kpad->client, Key_EVENTA + i);
-		int key_val = key & KEY_EV_MASK;
+		unsigned int key = adp5588_read(kpad->client, Key_EVENTA + i);
+		unsigned int key_val = key & KEY_EV_MASK;
 
 		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
 			for (j = 0; j < kpad->gpimapsize; j++) {

@@ -273,9 +273,9 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 					break;
 				}
 			}
-		} else {
+		} else if (key_val <= ADP5588_KEYMAPSIZE) {
 			input_report_key(kpad->input,
-					 kpad->keycode[key_val - 1],
+					 kpad->keycode[key_val],
 					 key & KEY_EV_PRESSED);
 		}
 	}

@@ -295,7 +295,7 @@ static void adp5588_work(struct work_struct *work)
 
 	if (status & ADP5588_KE_INT) {
 		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt) {
+		if (ev_cnt && ev_cnt <= ADP5588_MAXEVENTS) {
 			adp5588_report_events(kpad, ev_cnt);
 			input_sync(kpad->input);
 		}

diff --git a/include/linux/i2c/adp5588.h b/include/linux/i2c/adp5588.h
index cec17cf..394c07f 100644
--- a/include/linux/i2c/adp5588.h
+++ b/include/linux/i2c/adp5588.h

@@ -103,6 +103,7 @@
 
 /* Put one of these structures in i2c_board_info platform_data */
 
+#define ADP5588_MAXEVENTS	10
 #define ADP5588_KEYMAPSIZE	80
 
 #define GPI_PIN_ROW0 97

-- 
1.7.2.5

Christoph Fritz wrote on 2011-12-03:

Hi Michael,

 I had a look at the adp5588-keys driver and its datasheet.
There are some "slips of the pen" in it (adp5588 Rev.b):

Hi Christoph,

Good catch - however typos in datasheets shouldn't be discussed here, unless it
influences the vital state of this driver.

  - Page 8, Table 12, "E4 pressed" refers to binary 1000 instead of 100

Not only this obvious one - all others are wrong as well...

  - Page 17, Table 22, Register Descriptions are mostly wrong

What's wrong with Table 22?

Another thing I'm curious about is in adp5588_report_events():

int key = adp5588_read(kpad->client, Key_EVENTA + i); int key_val = key
& KEY_EV_MASK; [...] input_report_key(kpad->input, kpad->keycode[key_val
- 1], key & KEY_EV_PRESSED);

Why is there a -1 for key_val?

Key_val == 0, is not a valid event code!
The part emits 1..80 for keys and 97..114 for GPI events.

So bottom line - the driver is correct but Table 12 is completely wrong.

Best regards,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif

On Sun, 2011-12-04 at 21:12 +0000, Hennerich, Michael wrote:

Christoph Fritz wrote on 2011-12-03:

quoted

Hi Michael,

 I had a look at the adp5588-keys driver and its datasheet.
There are some "slips of the pen" in it (adp5588 Rev.b):

Hi Christoph,

Good catch - however typos in datasheets shouldn't be discussed here, unless it
influences the vital state of this driver.

quoted

  - Page 8, Table 12, "E4 pressed" refers to binary 1000 instead of 100

Not only this obvious one - all others are wrong as well...

quoted

  - Page 17, Table 22, Register Descriptions are mostly wrong

What's wrong with Table 22?

The column "Register Description" says "Key Event Register B [..]" for
register names KEY_EVENTD to KEY_EVENTJ. But for row KEY_EVENTA to
KEY_EVENTC it seems to be correct.

quoted

Another thing I'm curious about is in adp5588_report_events():

int key = adp5588_read(kpad->client, Key_EVENTA + i); int key_val = key
& KEY_EV_MASK; [...] input_report_key(kpad->input, kpad->keycode[key_val
- 1], key & KEY_EV_PRESSED);

Why is there a -1 for key_val?

Key_val == 0, is not a valid event code!
The part emits 1..80 for keys and 97..114 for GPI events.

ah, so mark ² below Table 22 is correct :)

So bottom line - the driver is correct but Table 12 is completely wrong.

Thanks,
 -- Christoph


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