Skip to contentSkip to conversation

Input: add MAX7359 key switch controller driver

PATCH Kim Kyuwon 5 participants 10 messages May 6, 2009 – May 9, 2009

Subsystems (3)
HID I2C subsystem host drivers Input drivers
lore
Share & tools
lore

Kim Kyuwon

author
#0 lore raw

The Maxim MAX7359 is a I2C interfaced key switch controller which provides microprocessors with management of up to 64 key switches. This patch adds support for the MAX7359 key switch controller.

Signed-off-by Kim Kyuwon <[email protected]>
---
 drivers/input/keyboard/Kconfig          |    8 +
 drivers/input/keyboard/Makefile         |    1 +
 drivers/input/keyboard/max7359_keypad.c |  300 +++++++++++++++++++++++++++++++
 include/linux/max7359_keypad.h          |   30 +++
 4 files changed, 339 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/max7359_keypad.c
 create mode 100644 include/linux/max7359_keypad.h
drivers/input/keyboard/Kconfig +8 -0
index ea2638b..fb2dc08 100644
@@ -332,4 +332,12 @@ config KEYBOARD_SH_KEYSC
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called sh_keysc.
+
+config KEYBOARD_MAX7359
+	tristate "Maxim MAX7359 Key Switch Controller"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Maxim MAX7359 Key
+	  Switch Controller chip. This providers microprocessors with
+	  management of up to 64 key switches
 endif
drivers/input/keyboard/Makefile +1 -0
index 36351e1..a9e7502 100644
@@ -28,3 +28,4 @@ obj-$(CONFIG_KEYBOARD_HP7XX)		+= jornada720_kbd.o
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
 obj-$(CONFIG_KEYBOARD_BFIN)		+= bf54x-keys.o
 obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
+obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
drivers/input/keyboard/max7359_keypad.c new file +300 -0

300 lines not on this page · raw message

include/linux/max7359_keypad.h new file +30 -0
new file mode 100644
index 0000000..c9477a5
@@ -0,0 +1,30 @@
+/*
+ * max7359_keypad.h - MAX7359 Keypad Controller Driver
+ *
+ * Copyright (C) 2009 Samsung Electronics
+ * Kim Kyuwon <[email protected]>
+ *
+ * Based on pxa27x_keypad.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Datasheet: http://www.maxim-ic.com/quick_view2.cfm/qv_pk/5456
+ */
+
+#ifndef _MAX7359_KEYPAD_H_
+#define _MAX7359_KEYPAD_H_
+
+#define MAX_MATRIX_KEY_ROWS	8
+#define MAX_MATRIX_KEY_COLS	8
+
+struct max7359_keypad_platform_data {
+	/* code map for the keys */
+	unsigned int	*key_map;
+	unsigned int	key_map_size;
+};
+
+#define KEY(row, col, val)	(((row) << 28) | ((col) << 24) | (val))
+
+#endif /* _MAX7359_KEYPAD_H_ */
Signature · 1 line
1.5.2.5
#1 lore raw

Hi Kim,

Quoted 3 lines · Kim Kyuwon

The Maxim MAX7359 is a I2C interfaced key switch controller which provides microprocessors with management of up to 64 key switches. This patch adds support for the MAX7359 key switch controller.

Could you please restrict the commit text line to 80/72 columns? It will look good.

drivers/input/keyboard/max7359_keypad.c line 125

Quoted 3 lines
drivers/input/keyboard/max7359_keypad.c new file +3 -0
+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+{
+       struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;

No need of casting. Please remove.

Quoted 6 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+
+       if (!work_pending(&keypad->work)) {
+               disable_irq_nosync(keypad->irq);
+               schedule_work(&keypad->work);
+       } else {
+               dev_err(&keypad->client->dev,

drivers/input/keyboard/max7359_keypad.c line 153

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +9 -0
+                               "Another job is currently pending \n");
+       }
+
+       return IRQ_HANDLED;
+}
+
 
+
+static int __devinit max7359_probe(struct i2c_client *client,
+                                       const struct i2c_device_id *id)

You may want to remove __devinit as it is i2c client driver . I have added linux-i2c for i2c
specific review.

Quoted 5 lines
drivers/input/keyboard/max7359_keypad.c new file +5 -0
+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+       /* If no keys are pressed, enter sleep mode for 8192 ms */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
+

drivers/input/keyboard/max7359_keypad.c line 267

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+       return 0;
+}
+
+static int max7359_resume(struct i2c_client *client)
+{
+       /* Restore the default setting (autosleep for 256 ms) */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
+
+       return 0;
+}

Protect these two function with #ifdef CONFIG_PM please.

Quoted 7 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+
 
+
+static struct i2c_driver max7359_i2c_driver = {
+       .driver = {
+               .name = "max7359",
+       },

drivers/input/keyboard/max7359_keypad.c line 300

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +8 -0
+       .probe          = max7359_probe,
+       .remove         = __exit_p(max7359_remove),
 
 
+};
+
+
+MODULE_AUTHOR("Kim Kyuwon <[email protected]>");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

include/linux/max7359_keypad.h line 28

Quoted 4 lines
include/linux/max7359_keypad.h +2 -0
 
+
+#define KEY(row, col, val)     (((row) << 28) | ((col) << 24) | (val))

Looks like this macro is highly used by many input drivers, anyone looking at it to place it at common location?

Signature · 7 lines
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw
Quoted 5 lines · Trilok Soni

include/linux/max7359_keypad.h line 28

Quoted 2 lines
include/linux/max7359_keypad.h new file +2 -0
+
+#define KEY(row, col, val)     (((row) << 28) | ((col) << 24) | (val))

Looks like this macro is highly used by many input drivers, anyone looking at it to place it at common location?

Hello,

I submitted a patch series around early Feb that did just that. The only archive for linux-input that I can find to reference the original submit is at http://markmail.org/browse/org.kernel.vger.linux-input/2009-01.
The first message in the series is $SUBJECT
"[PATCH 0/5] input: Common macro for keypad drivers".

At this point the series would probably need to be updated. If wanted I will create a new one.

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to Trilok Soni

Hi Trilok,

Thank you for reviewing this driver and I have something to make sure for the next better patch :)

Quoted 9 lines · Trilok Soni

Hi Kim,

Quoted 3 lines · Kim Kyuwon

The Maxim MAX7359 is a I2C interfaced key switch controller which provides microprocessors with management of up to 64 key switches. This patch adds support for the MAX7359 key switch controller.

Could you please restrict the commit text line to 80/72 columns? It will look good.

Is this restriction for commit text mandatory? It seems like I got an email from Andrew Morton that my patch was wordwraped even commit text.

Quoted 5 lines

drivers/input/keyboard/max7359_keypad.c line 125

Quoted 3 lines
drivers/input/keyboard/max7359_keypad.c new file +3 -0
+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+{
+       struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;

No need of casting. Please remove.

Ok, Thanks I'll remove it.

Quoted 13 lines
Quoted 12 lines
drivers/input/keyboard/max7359_keypad.c new file +12 -0
+
+       if (!work_pending(&keypad->work)) {
+               disable_irq_nosync(keypad->irq);
+               schedule_work(&keypad->work);
+       } else {
+               dev_err(&keypad->client->dev,
+                               "Another job is currently pending \n");
+       }
+
+       return IRQ_HANDLED;
+}
+
Quoted 6 lines

drivers/input/keyboard/max7359_keypad.c line 153

Quoted 3 lines
drivers/input/keyboard/max7359_keypad.c new file +3 -0
+
+static int __devinit max7359_probe(struct i2c_client *client,
+                                       const struct i2c_device_id *id)

You may want to remove __devinit as it is i2c client driver . I have added linux-i2c for i2c specific review.

Actually I don't want to remove it :), Is there any reason why you think so?

Quoted 9 lines
Quoted 9 lines
drivers/input/keyboard/max7359_keypad.c new file +9 -0
+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+       /* If no keys are pressed, enter sleep mode for 8192 ms */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
+
+       return 0;
+}
+
+static int max7359_resume(struct i2c_client *client)
Quoted 8 lines

drivers/input/keyboard/max7359_keypad.c line 267

Quoted 6 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+{
+       /* Restore the default setting (autosleep for 256 ms) */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
+
+       return 0;
+}

Protect these two function with #ifdef CONFIG_PM please.

This is what I really wanted to ask. I'm trying not to use '#ifdef'. By the way, why most of drivers use protect suspend/resume functions with #ifdef? Just for saving code size?

Quoted 11 lines
Quoted 9 lines
drivers/input/keyboard/max7359_keypad.c new file +8 -0
+
 
+
+static struct i2c_driver max7359_i2c_driver = {
+       .driver = {
+               .name = "max7359",
+       },
+       .probe          = max7359_probe,
+       .remove         = __exit_p(max7359_remove),
Quoted 8 lines

drivers/input/keyboard/max7359_keypad.c line 300

Quoted 6 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+};
+
+
+MODULE_AUTHOR("Kim Kyuwon <[email protected]>");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

In 'include/linux/module.h'

96 /* For userspace: you can also call me... */ 97 #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)

Quoted 1 line

From comment above, I think MODULE_ALIAS is optional. Do you still

think MODULE_ALIAS needs?

And when I send new version of patch, I will add [PATCH] in front of subject. Thank you for reviewing and letting me ask.

Regards,

#4 lore raw

Hi Kim,

Quoted 7 lines
Quoted 3 lines

Could you please restrict the commit text line to 80/72 columns? It will look good.

Is this restriction for commit text mandatory? It seems like I got an email from Andrew Morton that my patch was wordwraped even commit text.

This helps reader of the commit text to not give much stretch on his eyes :)

Quoted 9 lines
Quoted 7 lines

drivers/input/keyboard/max7359_keypad.c line 153

Quoted 4 lines
drivers/input/keyboard/max7359_keypad.c new file +3 -0
+
+static int __devinit max7359_probe(struct i2c_client *client,
+                                       const struct i2c_device_id *id)

You may want to remove __devinit as it is i2c client driver . I have added linux-i2c for i2c specific review.

Actually I don't want to remove it :), Is there any reason why you think so?

Sorry I have mistaken it as __init. This is fine.

Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +9 -0
+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+       /* If no keys are pressed, enter sleep mode for 8192 ms */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
+
+       return 0;
+}
+
+static int max7359_resume(struct i2c_client *client)
Quoted 8 lines

drivers/input/keyboard/max7359_keypad.c line 267

Quoted 6 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+{
+       /* Restore the default setting (autosleep for 256 ms) */
+       max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
+
+       return 0;
+}

Protect these two function with #ifdef CONFIG_PM please.

Quoted 3 lines

This is what I really wanted to ask. I'm trying not to use '#ifdef'. By the way, why most of drivers use protect suspend/resume functions with #ifdef? Just for saving code size?

suspend and resume is only important if the user of this driver has CONFIG_PM enabled in it's
defconfig, right? So, if CONFIG_PM is not defined suspend/resume should equal to NULL.

Quoted 11 lines
Quoted 10 lines

drivers/input/keyboard/max7359_keypad.c line 300

Quoted 8 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+};
+
+
+MODULE_AUTHOR("Kim Kyuwon <[email protected]>");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

Quoted 8 lines

In 'include/linux/module.h'

 96 /* For userspace: you can also call me... */ 97 #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)

From comment above, I think MODULE_ALIAS is optional. Do you still think MODULE_ALIAS needs?

Yes, it is not __must__.

Signature · 3 lines
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
maintainer
#5 lore raw
In reply to Kim Kyuwon

Hi Kim,

Quoted 19 lines · Kim Kyuwon

The Maxim MAX7359 is a I2C interfaced key switch controller which provides microprocessors with management of up to 64 key switches. This patch adds support for the MAX7359 key switch controller.

Signed-off-by Kim Kyuwon <[email protected]>
---
 drivers/input/keyboard/Kconfig          |    8 +
 drivers/input/keyboard/Makefile         |    1 +
 drivers/input/keyboard/max7359_keypad.c |  300 +++++++++++++++++++++++++++++++
 include/linux/max7359_keypad.h          |   30 +++
 4 files changed, 339 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/max7359_keypad.c
 create mode 100644 include/linux/max7359_keypad.h
drivers/input/keyboard/Kconfig +0 -0
index ea2638b..fb2dc08 100644
@@ -332,4 +332,12 @@ config KEYBOARD_SH_KEYSC

drivers/input/keyboard/Kconfig config KEYBOARD_SH_KEYSC · line 342

Quoted 10 lines
drivers/input/keyboard/Kconfig +8 -0
 	  To compile this driver as a module, choose M here: the
 	  module will be called sh_keysc.
+
+config KEYBOARD_MAX7359
+	tristate "Maxim MAX7359 Key Switch Controller"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Maxim MAX7359 Key
+	  Switch Controller chip. This providers microprocessors with
+	  management of up to 64 key switches

"To compile this driver as a module..."

Quoted 28 lines
drivers/input/keyboard/Kconfig +0 -0
 endif
drivers/input/keyboard/Makefile +1 -0
index 36351e1..a9e7502 100644
@@ -28,3 +28,4 @@ obj-$(CONFIG_KEYBOARD_HP7XX)		+= jornada720_kbd.o
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
 obj-$(CONFIG_KEYBOARD_BFIN)		+= bf54x-keys.o
 obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
+obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
drivers/input/keyboard/max7359_keypad.c new file +12 -0
new file mode 100644
index 0000000..3c2ec01
@@ -0,0 +1,300 @@
+/*
+ * max7359_keypad.c - MAX7359 Key Switch Controller Driver
+ *
+ * Copyright (C) 2009 Samsung Electronics
+ * Kim Kyuwon <[email protected]>
+ *
+ * Based on pxa27x_keypad.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *

drivers/input/keyboard/max7359_keypad.c line 22

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+ * Datasheet: http://www.maxim-ic.com/quick_view2.cfm/qv_pk/5456
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/max7359_keypad.h>
+
+#define MAX_KEY_NUM			(8 * 8)

Please do:

#define MAX_KEY_NUM (MAX_MATRIX_KEY_ROWS * MAX_MATRIX_KEY_COLS)

Quoted 27 lines
drivers/input/keyboard/max7359_keypad.c new file +27 -0
+
+/*
+ * MAX7359 registers
+ */
+#define MAX7359_REG_KEYFIFO		0x00
+#define MAX7359_REG_CONFIG		0x01
+#define MAX7359_REG_DEBOUNCE		0x02
+#define MAX7359_REG_INTERRUPT		0x03
+#define MAX7359_REG_PORTS		0x04
+#define MAX7359_REG_KEYREP		0x05
+#define MAX7359_REG_SLEEP		0x06
+
+/*
+ * Configuration register bits
+ */
+#define MAX7359_CFG_SLEEP		(1 << 7)
+#define MAX7359_CFG_INTERRUPT		(1 << 5)
+#define MAX7359_CFG_KEY_RELEASE		(1 << 3)
+#define MAX7359_CFG_WAKEUP		(1 << 1)
+#define MAX7359_CFG_TIMEOUT		(1 << 0)
+
+struct max7359_keypad {
+	/* matrix key code map */
+	u16					keycodes[MAX_KEY_NUM];
+
+	struct work_struct			work;
+

drivers/input/keyboard/max7359_keypad.c line 59

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+	struct max7359_keypad_platform_data	*pdata;
+	struct input_dev			*input_dev;
+	struct i2c_client			*client;
+
+	u32					irq;
+};
+
+static int max7359_write_reg(struct i2c_client *client, u8 reg, u8 val)
+{
+	int ret = i2c_smbus_write_byte_data(client, reg, val);

Empty line after declarations please.

Quoted 63 lines
drivers/input/keyboard/max7359_keypad.c new file +63 -0
+	if (ret >= 0)
+		return 0;
+
+	dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
+						__func__, reg, val, ret);
+
+	return ret;
+}
+
+static int max7359_read_reg(struct i2c_client *client, int reg)
+{
+	int ret = i2c_smbus_read_byte_data(client, reg);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
+						__func__, reg, ret);
+	return ret;
+}
+
+static void max7359_build_keycode(struct max7359_keypad *keypad)
+{
+	struct max7359_keypad_platform_data *pdata = keypad->pdata;
+	struct input_dev *input_dev = keypad->input_dev;
+	unsigned int *key;
+	int i;
+
+	key = pdata->key_map;
+	for (i = 0; i < pdata->key_map_size; i++, key++) {
+		int row = ((*key) >> 28) & 0xf;
+		int col = ((*key) >> 24) & 0xf;
+		short code = (*key) & 0xffff;
+
+		keypad->keycodes[(row << 3) + col] = code;
+		set_bit(code, input_dev->keybit);
+	}
+}
+
+static inline unsigned int max7359_lookup_matrix_keycode(
+			struct max7359_keypad *keypad, int row, int col)
+{
+	return keypad->keycodes[(row << 3) + col];
+}
+
+static void max7359_worker(struct work_struct *work)
+{
+	struct max7359_keypad *keypad =
+			container_of(work, struct max7359_keypad, work);
+	int val, row, col, release;
+
+	val = max7359_read_reg(keypad->client, MAX7359_REG_KEYFIFO);
+	row = val & 0x7;
+	col = (val >> 3) & 0x7;
+	release = val & 0x40;
+
+	input_report_key(keypad->input_dev,
+		max7359_lookup_matrix_keycode(keypad, row, col), !release);
+	input_sync(keypad->input_dev);
+
+	enable_irq(keypad->irq);
+
+	dev_dbg(&keypad->client->dev, "key[%d:%d] %s\n", row, col,
+					(release ? "release" : "press"));
+}
+

drivers/input/keyboard/max7359_keypad.c line 132

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+{
+	struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;
+
+	if (!work_pending(&keypad->work)) {
+		disable_irq_nosync(keypad->irq);
+		schedule_work(&keypad->work);
+	} else {
+		dev_err(&keypad->client->dev,
+				"Another job is currently pending \n");

Is this truly an error?

Quoted 98 lines
drivers/input/keyboard/max7359_keypad.c new file +98 -0
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void max7359_initialize(struct i2c_client *client)
+{
+	max7359_write_reg(client, MAX7359_REG_CONFIG,
+		MAX7359_CFG_INTERRUPT | /* Irq clears after host read */
+		MAX7359_CFG_KEY_RELEASE | /* Key release enable */
+		MAX7359_CFG_WAKEUP); /* Key press wakeup enable */
+
+	/* Full key-scan functionality */
+	max7359_write_reg(client, MAX7359_REG_DEBOUNCE, 0x1F);
+
+	/* nINT asserts every debounce cycles */
+	max7359_write_reg(client, MAX7359_REG_INTERRUPT, 0x01);
+}
+
+static int __devinit max7359_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	struct max7359_keypad *keypad;
+	struct max7359_keypad_platform_data *pdata;
+	struct input_dev *input_dev;
+	int ret;
+
+	keypad = kzalloc(sizeof(struct max7359_keypad), GFP_KERNEL);
+	if (keypad == NULL) {
+		dev_err(&client->dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	keypad->client = client;
+	pdata = keypad->pdata = client->dev.platform_data;
+	i2c_set_clientdata(client, keypad);
+
+	/* Detect MAX7359: The initial Keys FIFO value is '0x3F' */
+	ret = max7359_read_reg(client, MAX7359_REG_KEYFIFO);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to detect device\n");
+		goto failed_free;
+	} else {
+		dev_info(&client->dev, "keys FIFO is 0x%02x"
+				", succeeded in detecting device\n", ret);
+	}
+
+	/* Initialize MAX7359 */
+	max7359_initialize(client);
+
+	/* Create and register the input driver. */
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		dev_err(&client->dev, "failed to allocate input device\n");
+		ret = -ENOMEM;
+		goto failed_free;
+	}
+
+	input_dev->name = client->name;
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = &client->dev;
+
+	input_set_drvdata(input_dev, keypad);
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+	input_dev->keycodesize = sizeof(keypad->keycodes[0]);
+	input_dev->keycodemax = ARRAY_SIZE(keypad->keycodes);
+	input_dev->keycode = keypad->keycodes;
+
+	keypad->input_dev = input_dev;
+
+	max7359_build_keycode(keypad);
+
+	INIT_WORK(&keypad->work, max7359_worker);
+
+	keypad->irq = client->irq;
+	if (!keypad->irq) {
+		dev_err(&client->dev, "The irq number should not be zero\n");
+		goto failed_free_dev;
+	}
+
+	ret = request_irq(keypad->irq, max7359_interrupt,
+				IRQF_TRIGGER_LOW, client->name, keypad);
+	if (ret) {
+		dev_err(&client->dev, "failed to register interrupt\n");
+		goto failed_free_dev;
+	}
+
+	/* Register the input device */
+	ret = input_register_device(input_dev);
+	if (ret) {
+		dev_err(&client->dev, "failed to register input device\n");
+		goto failed_free_irq;
+	}
+
+	return 0;
+
+failed_free_irq:

drivers/input/keyboard/max7359_keypad.c line 240

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+	free_irq(keypad->irq, keypad);
+failed_free_dev:
+	input_free_device(input_dev);
+failed_free:
+	i2c_set_clientdata(client, NULL);
+	kfree(keypad);
+	return ret;
+}
+
+static int __exit max7359_remove(struct i2c_client *client)

__devexit, not __exit.

Quoted 5 lines
drivers/input/keyboard/max7359_keypad.c new file +5 -0
+{
+	struct max7359_keypad *keypad = i2c_get_clientdata(client);
+
+	cancel_work_sync(&keypad->work);
+	input_unregister_device(keypad->input_dev);

drivers/input/keyboard/max7359_keypad.c line 255

Quoted 10 lines
drivers/input/keyboard/max7359_keypad.c new file +10 -0
+	free_irq(keypad->irq, keypad);
+	i2c_set_clientdata(client, NULL);
+	kfree(keypad);
+
+	return 0;
+}
+
+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	/* If no keys are pressed, enter sleep mode for 8192 ms */

What happens if there are keys that are pressed?

drivers/input/keyboard/max7359_keypad.c line 267

Quoted 12 lines
drivers/input/keyboard/max7359_keypad.c new file +12 -0
+	max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
+
+	return 0;
+}
+
+static int max7359_resume(struct i2c_client *client)
+{
+	/* Restore the default setting (autosleep for 256 ms) */
+	max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
+
+	return 0;
+}

Could we also have similar open and close? It seems prudent to be in low power mode if there are no users of the device.

drivers/input/keyboard/max7359_keypad.c line 280

Quoted 13 lines
drivers/input/keyboard/max7359_keypad.c new file +13 -0
+
+static const struct i2c_device_id max7359_ids[] = {
+	{ "max7359", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max7359_ids);
+
+static struct i2c_driver max7359_i2c_driver = {
+	.driver = {
+		.name = "max7359",
+	},
+	.probe		= max7359_probe,
+	.remove		= __exit_p(max7359_remove),

__devexit_p();

drivers/input/keyboard/max7359_keypad.c line 282

Quoted 2 lines
drivers/input/keyboard/max7359_keypad.c new file +2 -0
+	.suspend	= max7359_suspend,
+	.resume		= max7359_resume,

Guard suspend/resume with #ifdef CONFIG_PM please.

Quoted 35 lines
drivers/input/keyboard/max7359_keypad.c new file +18 -0
+	.id_table	= max7359_ids,
+};
+
+static int __init max7359_init(void)
+{
+	return i2c_add_driver(&max7359_i2c_driver);
+}
+module_init(max7359_init);
+
+static void __exit max7359_exit(void)
+{
+	i2c_del_driver(&max7359_i2c_driver);
+}
+module_exit(max7359_exit);
+
+MODULE_AUTHOR("Kim Kyuwon <[email protected]>");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");
include/linux/max7359_keypad.h new file +10 -0
new file mode 100644
index 0000000..c9477a5
@@ -0,0 +1,30 @@
+/*
+ * max7359_keypad.h - MAX7359 Keypad Controller Driver
+ *
+ * Copyright (C) 2009 Samsung Electronics
+ * Kim Kyuwon <[email protected]>
+ *
+ * Based on pxa27x_keypad.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as

include/linux/max7359_keypad.h line 20

Quoted 10 lines
include/linux/max7359_keypad.h new file +10 -0
+ * published by the Free Software Foundation.
+ *
+ * Datasheet: http://www.maxim-ic.com/quick_view2.cfm/qv_pk/5456
+ */
+
+#ifndef _MAX7359_KEYPAD_H_
+#define _MAX7359_KEYPAD_H_
+
+#define MAX_MATRIX_KEY_ROWS	8
+#define MAX_MATRIX_KEY_COLS	8

These names are too generic, may clash with other #includes eventially... adding MAX7359_ prefix seems prudent.

Quoted 12 lines
include/linux/max7359_keypad.h new file +10 -0
+
+struct max7359_keypad_platform_data {
+	/* code map for the keys */
+	unsigned int	*key_map;
+	unsigned int	key_map_size;
+};
+
+#define KEY(row, col, val)	(((row) << 28) | ((col) << 24) | (val))
+
+#endif /* _MAX7359_KEYPAD_H_ */
Signature · 1 line
1.5.2.5

Thanks.

Signature · 1 line
Dmitry
#6 lore raw

Hi Dmitry,

Thank you for good comments. I fixed all as you recommended. More detailed answers are below your comments.
In addition, I also added wakeup related statements (device_init_wakeup, device_may_wakeup, [enable|disable]_irq_wake).

I'm sending this revised patch right now.

By the way, can I ask the same question which I ask to Trilok. Even though I guard suspend/resume with #ifdef CONFIG_PM in the new patch, Could I know the good reason for this protection? Because '/Documentation/SubmittingPatches' says "ifdefs are ugly"

Thank you Dmitry again for your comments and time. Kyuwon

Quoted 5 lines · Dmitry Torokhov

Hi Kim,

Quoted 2 lines · Kim Kyuwon
(quoted diff) +0 -0
@@ -332,4 +332,12 @@ config KEYBOARD_SH_KEYSC
Quoted 12 lines

drivers/input/keyboard/Kconfig config KEYBOARD_SH_KEYSC · line 342

Quoted 10 lines
drivers/input/keyboard/Kconfig +8 -0
        To compile this driver as a module, choose M here: the
        module will be called sh_keysc.
+
+config KEYBOARD_MAX7359
+     tristate "Maxim MAX7359 Key Switch Controller"
+     depends on I2C
+     help
+       If you say yes here you get support for the Maxim MAX7359 Key
+       Switch Controller chip. This providers microprocessors with
+       management of up to 64 key switches

"To compile this driver as a module..."

I added this sentence.

Quoted 11 lines

drivers/input/keyboard/max7359_keypad.c line 22

Quoted 8 lines
drivers/input/keyboard/max7359_keypad.c new file +8 -0
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/max7359_keypad.h>
+
+#define MAX_KEY_NUM                  (8 * 8)

Please do:

Quoted 1 line

#define MAX_KEY_NUM (MAX_MATRIX_KEY_ROWS * MAX_MATRIX_KEY_COLS)

I did
#define MAX7359_MAX_KEY_NUM (MAX7359_MAX_KEY_ROWS * MAX7359_MAX_KEY_COLS)
Thanks.

Quoted 5 lines

drivers/input/keyboard/max7359_keypad.c line 59

Quoted 3 lines
drivers/input/keyboard/max7359_keypad.c new file +3 -0
+static int max7359_write_reg(struct i2c_client *client, u8 reg, u8 val)
+{
+     int ret = i2c_smbus_write_byte_data(client, reg, val);

Empty line after declarations please.

Fixed.

Quoted 4 lines
Quoted 4 lines
drivers/input/keyboard/max7359_keypad.c new file +4 -0
+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+{
+     struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;
+
Quoted 8 lines

drivers/input/keyboard/max7359_keypad.c line 132

Quoted 6 lines
drivers/input/keyboard/max7359_keypad.c new file +6 -0
+     if (!work_pending(&keypad->work)) {
+             disable_irq_nosync(keypad->irq);
+             schedule_work(&keypad->work);
+     } else {
+             dev_err(&keypad->client->dev,
+                             "Another job is currently pending \n");

Is this truly an error?

I changed dev_err to dev_warn.

Quoted 4 lines

drivers/input/keyboard/max7359_keypad.c line 240

Quoted 2 lines
drivers/input/keyboard/max7359_keypad.c new file +2 -0
+
+static int __exit max7359_remove(struct i2c_client *client)

__devexit, not __exit.

Fixed.

Quoted 6 lines

drivers/input/keyboard/max7359_keypad.c line 255

Quoted 4 lines
drivers/input/keyboard/max7359_keypad.c new file +4 -0
+
+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+     /* If no keys are pressed, enter sleep mode for 8192 ms */

What happens if there are keys that are pressed?

I added more comments in new function(max7359_fall_deepsleep) as below:
/*
* Let MAX7359 fall into a deep sleep:
* If no keys are pressed, enter sleep mode for 8192 ms. And if any
* key is pressed, the MAX7359 returns to normal operating mode. */

Quoted 7 lines
Quoted 7 lines
drivers/input/keyboard/max7359_keypad.c new file +7 -0
+     max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
+
+     return 0;
+}
+
+static int max7359_resume(struct i2c_client *client)
+{
Quoted 8 lines

drivers/input/keyboard/max7359_keypad.c line 267

Quoted 5 lines
drivers/input/keyboard/max7359_keypad.c new file +5 -0
+     /* Restore the default setting (autosleep for 256 ms) */
+     max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
+
+     return 0;
+}

Could we also have similar open and close? It seems prudent to be in low power mode if there are no users of the device.

Sure we can. I added max7359_open, max7359_close

Quoted 9 lines

drivers/input/keyboard/max7359_keypad.c line 280

Quoted 7 lines
drivers/input/keyboard/max7359_keypad.c new file +7 -0
+
+static struct i2c_driver max7359_i2c_driver = {
+     .driver = {
+             .name = "max7359",
+     },
+     .probe          = max7359_probe,
+     .remove         = __exit_p(max7359_remove),

__devexit_p();

Fixed.

Quoted 4 lines

drivers/input/keyboard/max7359_keypad.c line 282

Quoted 2 lines
drivers/input/keyboard/max7359_keypad.c new file +2 -0
+     .suspend        = max7359_suspend,
+     .resume         = max7359_resume,

Guard suspend/resume with #ifdef CONFIG_PM please.

I guarded them.

Quoted 6 lines

include/linux/max7359_keypad.h line 20

Quoted 3 lines
include/linux/max7359_keypad.h new file +3 -0
+
+#define MAX_MATRIX_KEY_ROWS  8
+#define MAX_MATRIX_KEY_COLS  8

These names are too generic, may clash with other #includes eventially... adding MAX7359_ prefix seems prudent.

I added MAX7539_ prefix and these macros are moved to max7359_keypad.c file.

maintainer
#7 lore raw

Hi Kim,

Quoted 4 lines · On Friday 08 May 2009 18:58:45 Kim Kyuwon wrote:

By the way, can I ask the same question which I ask to Trilok. Even though I guard suspend/resume with #ifdef CONFIG_PM in the new patch, Could I know the good reason for this protection? Because '/Documentation/SubmittingPatches' says "ifdefs are ugly"

If kernel is compiled without CONFIG_PM then these functions would be just dead weight. Generally speaking, #ifdefs are considered ugly if they are in the middle of function code, affecting logic. But to to compile out unneeded functionality they are OK. That's why you often see in the kernel

#ifdef CONFIG_BAZ
void do_baz()

{
	.. real code ..
}

#else
void do_baz()
{
}
#endif

and then...

int foo()

{
	bar1();
	bar2();
	do_baz();
	bar3();
}

As you can see, foo()'s logic stays the same, there are no #ifdefs cluttering it, but baz code either executed or not.

Hope this helps.

Signature · 1 line
Dmitry
#8 lore raw
Quoted 36 lines · Dmitry Torokhov

Hi Kim,

Quoted 4 lines · On Friday 08 May 2009 18:58:45 Kim Kyuwon wrote:

By the way, can I ask the same question which I ask to Trilok. Even though I guard suspend/resume with #ifdef CONFIG_PM in the new patch, Could I know the good reason for this protection? Because '/Documentation/SubmittingPatches' says "ifdefs are ugly"

If kernel is compiled without CONFIG_PM then these functions would be just dead weight. Generally speaking, #ifdefs are considered ugly if they are in the middle of function code, affecting logic. But to to compile out unneeded functionality they are OK. That's why you often see in the kernel

#ifdef CONFIG_BAZ
void do_baz()

{
       .. real code ..
}

#else
void do_baz()
{
}
#endif

and then...

int foo()

{
       bar1();
       bar2();
       do_baz();
       bar3();
}
Quoted 4 lines

As you can see, foo()'s logic stays the same, there are no #ifdefs cluttering it, but baz code either executed or not.

Hope this helps.

Thank you for your kind explanation. It is very helpful to me and my colleagues.

Kyuwon

#9 lore raw
In reply to Kim Kyuwon

Hi Kim,

Quoted 10 lines · Kim Kyuwon

Hi Dmitry,

Thank you for good comments. I fixed all as you recommended. More detailed answers are below your comments.
In addition, I also added wakeup related statements (device_init_wakeup, device_may_wakeup, [enable|disable]_irq_wake).

I'm sending this revised patch right now.

By the way, can I ask the same question which I ask to Trilok.

I thought I had explained it in my e-mail but Dmitry's reply is more explanatory.

Signature · 3 lines
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni