Date:	Thu, 7 May 2009 20:19:12 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Kim Kyuwon <q1.kim@...sung.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, linux-input@...r.kernel.org,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: Input: add MAX7359 key switch controller driver

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@...sung.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@...sung.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@...sung.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@...sung.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists