Input: add MAX7359 key switch controller driver

Input: add MAX7359 key switch controller driver

[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 <q1.kim@samsung.com>
---
 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

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index ea2638b..fb2dc08 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -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
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 36351e1..a9e7502 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -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
diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
new file mode 100644
index 0000000..3c2ec01
--- /dev/null
+++ b/drivers/input/keyboard/max7359_keypad.c
@@ -0,0 +1,300 @@
+/*
+ * max7359_keypad.c - MAX7359 Key Switch Controller Driver
+ *
+ * Copyright (C) 2009 Samsung Electronics
+ * Kim Kyuwon <q1.kim@samsung.com>
+ *
+ * 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
+ */
+
+#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)
+
+/*
+ * 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;
+
+	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);
+	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"));
+}
+
+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");
+	}
+
+	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:
+	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)
+{
+	struct max7359_keypad *keypad = i2c_get_clientdata(client);
+
+	cancel_work_sync(&keypad->work);
+	input_unregister_device(keypad->input_dev);
+	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 */
+	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;
+}
+
+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),
+	.suspend	= max7359_suspend,
+	.resume		= max7359_resume,
+	.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 <q1.kim@samsung.com>");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/max7359_keypad.h b/include/linux/max7359_keypad.h
new file mode 100644
index 0000000..c9477a5
--- /dev/null
+++ b/include/linux/max7359_keypad.h
@@ -0,0 +1,30 @@
+/*
+ * max7359_keypad.h - MAX7359 Keypad Controller Driver
+ *
+ * Copyright (C) 2009 Samsung Electronics
+ * Kim Kyuwon <q1.kim@samsung.com>
+ *
+ * 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_ */
-- 
1.5.2.5

Re: Input: add MAX7359 key switch controller driver

[Trilok Soni]

Hi Kim,

On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon <q1.kim@samsung.com> wrote:
> 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.

> +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.

> +
> +       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;
> +}
> +

> +
> +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.

> +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)
> +{
> +       /* 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.

> +

> +
> +static struct i2c_driver max7359_i2c_driver = {
> +       .driver = {
> +               .name = "max7359",
> +       },
> +       .probe          = max7359_probe,
> +       .remove         = __exit_p(max7359_remove),


> +};
> +
> +
> +MODULE_AUTHOR("Kim Kyuwon <q1.kim@samsung.com>");
> +MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
> +MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

> diff --git a/include/linux/max7359_keypad.h b/include/linux/max7359_keypad.h

> +
> +#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?


-- 
---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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Input: add MAX7359 key switch controller driver

[H Hartley Sweeten]

On Wednesday, May 06, 2009 6:38 AM, Trilok Soni wrote:
>> +
>> +#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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Input: add MAX7359 key switch controller driver

[Kim Kyuwon]

Hi Trilok,

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

On Wed, May 6, 2009 at 10:38 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> Hi Kim,
>
> On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon <q1.kim@samsung.com> wrote:
>> 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.

>> +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.

>> +
>> +       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;
>> +}
>> +
>
>> +
>> +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?

>> +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)
>> +{
>> +       /* 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?

>> +
>
>> +
>> +static struct i2c_driver max7359_i2c_driver = {
>> +       .driver = {
>> +               .name = "max7359",
>> +       },
>> +       .probe          = max7359_probe,
>> +       .remove         = __exit_p(max7359_remove),
>
>
>> +};
>> +
>> +
>> +MODULE_AUTHOR("Kim Kyuwon <q1.kim@samsung.com>");
>> +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)

>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,

Re: Input: add MAX7359 key switch controller driver

[Trilok Soni]

Hi Kim,

>>
>> 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 :)

>>
>>> +
>>> +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.

>
>>> +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)
>>> +{
>>> +       /* 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?

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.

>>
>>
>>> +};
>>> +
>>> +
>>> +MODULE_AUTHOR("Kim Kyuwon <q1.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
>>> +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)
>
> From comment above, I think MODULE_ALIAS is optional. Do you still
> think MODULE_ALIAS needs?
>

Yes, it is not __must__.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Re: Input: add MAX7359 key switch controller driver

[Dmitry Torokhov]

Hi Kim,

On Wed, May 06, 2009 at 03:21:24PM +0900, Kim Kyuwon wrote:
> 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 <q1.kim@samsung.com>
> ---
>  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
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index ea2638b..fb2dc08 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -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

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

>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 36351e1..a9e7502 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -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
> diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
> new file mode 100644
> index 0000000..3c2ec01
> --- /dev/null
> +++ b/drivers/input/keyboard/max7359_keypad.c
> @@ -0,0 +1,300 @@
> +/*
> + * max7359_keypad.c - MAX7359 Key Switch Controller Driver
> + *
> + * Copyright (C) 2009 Samsung Electronics
> + * Kim Kyuwon <q1.kim@samsung.com>
> + *
> + * 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
> + */
> +
> +#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)

> +
> +/*
> + * 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;
> +
> +	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.

> +	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"));
> +}
> +
> +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?

> +	}
> +
> +	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:
> +	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.

> +{
> +	struct max7359_keypad *keypad = i2c_get_clientdata(client);
> +
> +	cancel_work_sync(&keypad->work);
> +	input_unregister_device(keypad->input_dev);
> +	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?

> +	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.

> +
> +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();

> +	.suspend	= max7359_suspend,
> +	.resume		= max7359_resume,

Guard suspend/resume with #ifdef CONFIG_PM please.

> +	.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 <q1.kim@samsung.com>");
> +MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
> +MODULE_LICENSE("GPL v2");

> diff --git a/include/linux/max7359_keypad.h b/include/linux/max7359_keypad.h
> new file mode 100644
> index 0000000..c9477a5
> --- /dev/null
> +++ b/include/linux/max7359_keypad.h
> @@ -0,0 +1,30 @@
> +/*
> + * max7359_keypad.h - MAX7359 Keypad Controller Driver
> + *
> + * Copyright (C) 2009 Samsung Electronics
> + * Kim Kyuwon <q1.kim@samsung.com>
> + *
> + * 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

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

> +
> +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_ */
> -- 
> 1.5.2.5

Thanks.

-- 
Dmitry

Re: Input: add MAX7359 key switch controller driver

[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.
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

On Fri, May 8, 2009 at 12:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Kim,
>
> On Wed, May 06, 2009 at 03:21:24PM +0900, Kim Kyuwon wrote:
>> @@ -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
>
> "To compile this driver as a module..."

I added this sentence.

>> +
>> +#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)

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

>> +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.

>> +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?

I changed dev_err to dev_warn.

>> +
>> +static int __exit max7359_remove(struct i2c_client *client)
>
> __devexit, not __exit.

Fixed.

>> +
>> +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.
 */

>> +     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.

Sure we can. I added max7359_open, max7359_close

>> +
>> +static struct i2c_driver max7359_i2c_driver = {
>> +     .driver = {
>> +             .name = "max7359",
>> +     },
>> +     .probe          = max7359_probe,
>> +     .remove         = __exit_p(max7359_remove),
>
> __devexit_p();

Fixed.

>> +     .suspend        = max7359_suspend,
>> +     .resume         = max7359_resume,
>
> Guard suspend/resume with #ifdef CONFIG_PM please.

I guarded them.

>> +
>> +#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.

Re: Input: add MAX7359 key switch controller driver

[Dmitry Torokhov]

Hi Kim,

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.

-- 
Dmitry

Re: Input: add MAX7359 key switch controller driver

[Kim Kyuwon]

On Sat, May 9, 2009 at 12:36 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Kim,
>
> 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.

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

Kyuwon

Re: Input: add MAX7359 key switch controller driver

[Trilok Soni]

Hi Kim,

On Sat, May 9, 2009 at 7:28 AM, Kim Kyuwon <chammoru@gmail.com> wrote:
> 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.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni