diff mbox

input: add mpr121 capacitive touchkey driver

Message ID 1302541337-2934-1-git-send-email-kzjeef@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

JieJing.Zhang April 11, 2011, 5:02 p.m. UTC
From: Zhang Jiejing <jiejing.zhang@freescale.com>

This touchkey driver is based on Freescale mpr121 capacitive
touch sensor controller.

It can support up to 12 keys, it use i2c interface.

The product infomation(data sheet, application notes) can
be found under this link:
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121

Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
---
 drivers/input/keyboard/Kconfig  |   12 ++
 drivers/input/keyboard/Makefile |    1 +
 drivers/input/keyboard/mpr121.c |  301 +++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/mpr.h         |   64 ++++++++
 4 files changed, 378 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/mpr121.c
 create mode 100644 include/linux/i2c/mpr.h

Comments

Christoph Fritz April 11, 2011, 11:20 p.m. UTC | #1
Hi Zhang,

 this is just a superficial review of your driver.

thanks,
 Christoph

On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> From: Zhang Jiejing <jiejing.zhang@freescale.com>
> 
> This touchkey driver is based on Freescale mpr121 capacitive
> touch sensor controller.
> 
> It can support up to 12 keys, it use i2c interface.

typo

> 
> The product infomation(data sheet, application notes) can
> be found under this link:
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121
> 
> Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
> ---
>  drivers/input/keyboard/Kconfig  |   12 ++
>  drivers/input/keyboard/Makefile |    1 +
>  drivers/input/keyboard/mpr121.c |  301 +++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/mpr.h         |   64 ++++++++
>  4 files changed, 378 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/keyboard/mpr121.c
>  create mode 100644 include/linux/i2c/mpr.h
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index b16bed0..0666e1e 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -520,6 +520,18 @@ config KEYBOARD_XTKBD
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called xtkbd.
>  
> +config KEYBOARD_MPR121
> +	tristate "Freescale MPR121 Touchkey Driver"
> +	depends on I2C
> +	help
> +	  Say Y here if you have the  touchkey Freescale mpr121 capacitive

typo: two spaces

> +	  touch sensor controller in your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here
> +endif
> +
>  config KEYBOARD_W90P910
>  	tristate "W90P910 Matrix Keypad support"
>  	depends on ARCH_W90X900

- add: "To compile this driver as a module, choose M here: the
  module will be called mpr121."
- "endif" is wrong here, it breaks 'make config'
- maybe a more alphabetic approach in this list of drivers would
  be nice

> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 878e6c2..b110ca8 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -47,4 +47,5 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>  obj-$(CONFIG_KEYBOARD_TNETV107X)	+= tnetv107x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
> +obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121.o
>  obj-$(CONFIG_KEYBOARD_W90P910)		+= w90p910_keypad.o

for better examination, please mind alphabetic order

> diff --git a/drivers/input/keyboard/mpr121.c b/drivers/input/keyboard/mpr121.c
> new file mode 100644
> index 0000000..ee149a8
> --- /dev/null
> +++ b/drivers/input/keyboard/mpr121.c
> @@ -0,0 +1,301 @@
> +/*
> + * Touchkey driver for Freescale MPR121 Controllor
> + *
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + * Author: Zhang Jiejing <jiejing.zhang@freescale.com>
> + *
> + * Based on mcs_touchkey.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.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/mpr.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/irq.h>

do not use irq.h directly

> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/bitops.h>
> +
> +struct mpr121_touchkey_data {
> +	struct i2c_client	*client;
> +	struct input_dev	*input_dev;
> +	unsigned int		key_val;
> +	int			statusbits;
> +	int			keycount;
> +	u16			keycodes[MPR121_MAX_KEY_COUNT];
> +};
> +
> +struct mpr121_init_register {
> +	int addr;
> +	u8 val;
> +};
> +
> +static struct mpr121_init_register init_reg_table[] = {
> +	{MHD_RISING_ADDR,	0x1},
> +	{NHD_RISING_ADDR,	0x1},
> +	{MHD_FALLING_ADDR,	0x1},
> +	{NHD_FALLING_ADDR,	0x1},
> +	{NCL_FALLING_ADDR,	0xff},
> +	{FDL_FALLING_ADDR,	0x02},
> +	{FILTER_CONF_ADDR,	0x04},
> +	{AFE_CONF_ADDR,		0x0b},
> +	{AUTO_CONFIG_CTRL_ADDR, 0x0b},
> +};
> +
> +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> +{
> +	struct mpr121_touchkey_data *data = dev_id;
> +	struct i2c_client *client = data->client;
> +	struct input_dev *input = data->input_dev;
> +	unsigned int key_num, pressed;
> +	int reg;
> +
> +	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
> +	if (reg < 0) {
> +		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> +		goto out;
> +	}
> +
> +	reg <<= 8;
> +	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
> +	if (reg < 0) {
> +		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> +		goto out;
> +	}
> +
> +	reg &= TOUCH_STATUS_MASK;
> +	/* use old press bit to figure out which bit changed */
> +	key_num = ffs(reg ^ data->statusbits) - 1;
> +	/* use the bit check the press status */

typo

> +	pressed = (reg & (1 << (key_num))) >> key_num;
> +	data->statusbits = reg;
> +	data->key_val = data->keycodes[key_num];
> +
> +	input_event(input, EV_MSC, MSC_SCAN, data->key_val);
> +	input_report_key(input, data->key_val, pressed);
> +	input_sync(input);
> +
> +	dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
> +		pressed ? "pressed" : "released");
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +static int mpr121_phys_init(struct mpr121_platform_data *pdata,
> +			    struct mpr121_touchkey_data *data,
> +			    struct i2c_client *client)
> +{
> +	struct mpr121_init_register *reg;
> +	unsigned char usl, lsl, tl;
> +	int i, t, vdd, ret;
> +
> +	/* setup touch/release threshold for ele0-ele11 */
> +	for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
> +		t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
> +		ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +		ret = i2c_smbus_write_byte_data(client, t + 1,
> +						RELEASE_THRESHOLD);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +	}
> +	/* setup init register */
> +	for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
> +		reg = &init_reg_table[i];
> +		ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +	}
> +	/* setup auto-register by vdd,the formula please ref: AN3889
> +	 * These register *must* set acrodding your VDD voltage supply
> +	 * to the chip*/

typo

> +	vdd = pdata->vdd_uv / 1000;
> +	usl = ((vdd - 700) * 256) / vdd;
> +	lsl = (usl * 65) / 100;
> +	tl = (usl * 90) / 100;
> +	ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
> +	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
> +	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
> +	ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
> +					data->keycount);
> +	if (ret != 0)
> +		goto err_i2c_write;
> +
> +	dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",

typo ?

> +		 data->keycount);
> +
> +	return 0;
> +
> +err_i2c_write:
> +	dev_err(&client->dev, "i2c write error: %d\n", ret);
> +	return ret;
> +}
> +
> +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
> +					const struct i2c_device_id *id)
> +{
> +	struct mpr121_platform_data *pdata;
> +	struct mpr121_touchkey_data *data;
> +	struct input_dev *input_dev;
> +	int error;
> +	int i;
> +
> +	pdata = client->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&client->dev, "no platform data defined\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "The irq number should not be zero\n");
> +		return -EINVAL;
> +	}
> +
> +	data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +	if (!data || !input_dev) {
> +		dev_err(&client->dev, "Falied to allocate memory\n");
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	data->client = client;
> +	data->input_dev = input_dev;
> +	data->keycount = pdata->keycount;
> +
> +	if (data->keycount > MPR121_MAX_KEY_COUNT) {
> +		dev_err(&client->dev, "Too many key defined\n");

typo

> +		error = -EINVAL;
> +		goto err_free_mem;
> +	}
> +
> +	error = mpr121_phys_init(pdata, data, client);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to init register\n");
> +		goto err_free_mem;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +
> +	input_dev->name = "FSL MPR121 Touchkey";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &client->dev;
> +	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
> +	input_dev->keycode = pdata->matrix;
> +	input_dev->keycodesize = sizeof(pdata->matrix[0]);
> +	input_dev->keycodemax = data->keycount;
> +
> +	for (i = 0; i < input_dev->keycodemax; i++) {
> +		__set_bit(pdata->matrix[i], input_dev->keybit);
> +		data->keycodes[i] = pdata->matrix[i];
> +	}
> +
> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +	input_set_drvdata(input_dev, data);
> +
> +	error = request_threaded_irq(client->irq, NULL,
> +				     mpr_touchkey_interrupt,
> +				     IRQF_TRIGGER_FALLING,
> +				     client->dev.driver->name, data);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		goto err_free_mem;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error)
> +		goto err_free_irq;
> +	i2c_set_clientdata(client, data);
> +	device_init_wakeup(&client->dev, pdata->wakeup);
> +	dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(client->irq, data);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(data);
> +	return error;
> +}
> +
> +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
> +{
> +	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> +
> +	free_irq(client->irq, data);
> +	input_unregister_device(data->input_dev);
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mpr_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(client->irq);
> +
> +	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
> +
> +	return 0;
> +}
> +
> +static int mpr_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> +
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(client->irq);
> +
> +	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct i2c_device_id mpr121_id[] = {
> +	{"mpr121_touchkey", 0},
> +	{ }
> +};
> +
> +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
> +
> +static struct i2c_driver mpr_touchkey_driver = {
> +	.driver = {
> +		.name	= "mpr121",
> +		.owner	= THIS_MODULE,
> +		.pm	= &mpr121_touchkey_pm_ops,

I would add a ifdef CONFIG_PM_SLEEP

> +	},
> +	.id_table	= mpr121_id,
> +	.probe		= mpr_touchkey_probe,
> +	.remove		= __devexit_p(mpr_touchkey_remove),
> +};
> +
> +static int __init mpr_touchkey_init(void)
> +{
> +	return i2c_add_driver(&mpr_touchkey_driver);
> +}
> +
> +static void __exit mpr_touchkey_exit(void)
> +{
> +	i2c_del_driver(&mpr_touchkey_driver);
> +}
> +
> +module_init(mpr_touchkey_init);
> +module_exit(mpr_touchkey_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
> +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
> diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
> new file mode 100644
> index 0000000..52d6e33
> --- /dev/null
> +++ b/include/linux/i2c/mpr.h
> @@ -0,0 +1,64 @@
> +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor

no filename please

> + * controllor */
> +
> +#ifndef MPR_H
> +#define MPR_H
> +
> +/* Register definitions */
> +#define ELE_TOUCH_STATUS_0_ADDR	0x0
> +#define ELE_TOUCH_STATUS_1_ADDR	0X1
> +#define MHD_RISING_ADDR		0x2b
> +#define NHD_RISING_ADDR		0x2c
> +#define NCL_RISING_ADDR		0x2d
> +#define FDL_RISING_ADDR		0x2e
> +#define MHD_FALLING_ADDR	0x2f
> +#define NHD_FALLING_ADDR	0x30
> +#define NCL_FALLING_ADDR	0x31
> +#define FDL_FALLING_ADDR	0x32
> +#define ELE0_TOUCH_THRESHOLD_ADDR	0x41
> +#define ELE0_RELEASE_THRESHOLD_ADDR	0x42
> +/* ELE0...ELE11's threshold will set in a loop */

typo

> +#define AFE_CONF_ADDR			0x5c
> +#define FILTER_CONF_ADDR		0x5d
> +
> +/* ELECTRODE_CONF: this register is most important register, it
> + * control how many of electrode is enabled. If you set this register
> + * to 0x0, it make the sensor going to suspend mode. Other value(low
> + * bit is non-zero) will make the sensor into Run mode.  This register
> + * should be write at last.

typo

> + */
> +#define ELECTRODE_CONF_ADDR		0x5e
> +#define AUTO_CONFIG_CTRL_ADDR		0x7b
> +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
> + * register is related to VDD supplied on your board, the value of
> + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.

typo

> + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
> + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
> + * is 90% of USL  */

typo

> +#define AUTO_CONFIG_USL_ADDR		0x7d
> +#define AUTO_CONFIG_LSL_ADDR		0x7e
> +#define AUTO_CONFIG_TL_ADDR		0x7f
> +
> +/* Threshold of touch/release trigger */
> +#define TOUCH_THRESHOLD			0x0f
> +#define RELEASE_THRESHOLD		0x0a
> +/* Mask Button bits of STATUS_0 & STATUS_1 register */

and or bit-and?

> +#define TOUCH_STATUS_MASK		0xfff
> +/* MPR121 have 12 electrodes */
> +#define MPR121_MAX_KEY_COUNT		12
> +
> +
> +/**
> + * @keycount: how many key maped

typo: "keys" ?

> + * @vdd_uv: voltage of vdd supply the chip in uV

- typo
- uppercase for vdd

> + * @matrix: maxtrix of keys
> + * @wakeup: can key wake up system.

typo

> + */
> +struct mpr121_platform_data {
> +	u16 keycount;
> +	u16 *matrix;
> +	int wakeup;
> +	int vdd_uv;
> +};
> +
> +#endif
> -- 
> 1.7.1
> 
> --
> 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.htmln Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
--
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
Dmitry Torokhov April 12, 2011, 6:01 a.m. UTC | #2
On Tue, Apr 12, 2011 at 01:20:11AM +0200, Christoph Fritz wrote:
> On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> > +
> > +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
> > +
> > +static struct i2c_driver mpr_touchkey_driver = {
> > +	.driver = {
> > +		.name	= "mpr121",
> > +		.owner	= THIS_MODULE,
> > +		.pm	= &mpr121_touchkey_pm_ops,
> 
> I would add a ifdef CONFIG_PM_SLEEP
> 

No, since we could try supporting CONFIG_PM_RUNTIME for example (not
necessarily in this driver, but in general case). dev_pm_ops structire
is defined always but it contents change de3pending on CONFIG_PM_SLEEP.
CONFIG_PM_RUNTILE and so forth. At a minium it is an empty structure,
but it woudl still be defined.

Thanks.
Dmitry Torokhov April 12, 2011, 6:18 a.m. UTC | #3
Hi Jiejing,

On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> From: Zhang Jiejing <jiejing.zhang@freescale.com>
> 
> This touchkey driver is based on Freescale mpr121 capacitive
> touch sensor controller.
> 
> It can support up to 12 keys, it use i2c interface.
> 
> The product infomation(data sheet, application notes) can
> be found under this link:
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121
> 

MOstly reasonable. In addition to Christoph's comments:

> +
> +static struct mpr121_init_register init_reg_table[] = {

const? And also __devinitconst?

> +	{MHD_RISING_ADDR,	0x1},
> +	{NHD_RISING_ADDR,	0x1},
> +	{MHD_FALLING_ADDR,	0x1},
> +	{NHD_FALLING_ADDR,	0x1},
> +	{NCL_FALLING_ADDR,	0xff},
> +	{FDL_FALLING_ADDR,	0x02},
> +	{FILTER_CONF_ADDR,	0x04},
> +	{AFE_CONF_ADDR,		0x0b},
> +	{AUTO_CONFIG_CTRL_ADDR, 0x0b},
> +};
> +
> +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> +{
> +	struct mpr121_touchkey_data *data = dev_id;
> +	struct i2c_client *client = data->client;
> +	struct input_dev *input = data->input_dev;
> +	unsigned int key_num, pressed;
> +	int reg;
> +
> +	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
> +	if (reg < 0) {
> +		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> +		goto out;
> +	}
> +
> +	reg <<= 8;
> +	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
> +	if (reg < 0) {
> +		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> +		goto out;
> +	}
> +
> +	reg &= TOUCH_STATUS_MASK;
> +	/* use old press bit to figure out which bit changed */
> +	key_num = ffs(reg ^ data->statusbits) - 1;
> +	/* use the bit check the press status */
> +	pressed = (reg & (1 << (key_num))) >> key_num;

No need to shift by key_num, input_report_key normalizes to 'bool'.

> +	data->statusbits = reg;
> +	data->key_val = data->keycodes[key_num];
> +
> +	input_event(input, EV_MSC, MSC_SCAN, data->key_val);

sScancode is not data->key_val as it is KEY_XXX value but rather key_num.

> +	input_report_key(input, data->key_val, pressed);
> +	input_sync(input);
> +
> +	dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
> +		pressed ? "pressed" : "released");
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +static int mpr121_phys_init(

__devinit

			    struct mpr121_platform_data *pdata,

pdata should be const.

> +			    struct mpr121_touchkey_data *data,
> +			    struct i2c_client *client)
> +{
> +	struct mpr121_init_register *reg;
> +	unsigned char usl, lsl, tl;
> +	int i, t, vdd, ret;
> +
> +	/* setup touch/release threshold for ele0-ele11 */
> +	for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
> +		t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
> +		ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +		ret = i2c_smbus_write_byte_data(client, t + 1,
> +						RELEASE_THRESHOLD);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +	}
> +	/* setup init register */
> +	for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
> +		reg = &init_reg_table[i];
> +		ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
> +		if (ret < 0)
> +			goto err_i2c_write;
> +	}
> +	/* setup auto-register by vdd,the formula please ref: AN3889
> +	 * These register *must* set acrodding your VDD voltage supply
> +	 * to the chip*/
> +	vdd = pdata->vdd_uv / 1000;
> +	usl = ((vdd - 700) * 256) / vdd;
> +	lsl = (usl * 65) / 100;
> +	tl = (usl * 90) / 100;
> +	ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
> +	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
> +	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
> +	ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
> +					data->keycount);
> +	if (ret != 0)
> +		goto err_i2c_write;
> +
> +	dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
> +		 data->keycount);

Does it have to be dev_info? dev_dbg maybe?

> +
> +	return 0;
> +
> +err_i2c_write:
> +	dev_err(&client->dev, "i2c write error: %d\n", ret);
> +	return ret;
> +}
> +
> +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
> +					const struct i2c_device_id *id)
> +{
> +	struct mpr121_platform_data *pdata;

const.

> +	struct mpr121_touchkey_data *data;
> +	struct input_dev *input_dev;
> +	int error;
> +	int i;
> +
> +	pdata = client->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&client->dev, "no platform data defined\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "The irq number should not be zero\n");
> +		return -EINVAL;
> +	}
> +
> +	data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +	if (!data || !input_dev) {
> +		dev_err(&client->dev, "Falied to allocate memory\n");
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	data->client = client;
> +	data->input_dev = input_dev;
> +	data->keycount = pdata->keycount;
> +
> +	if (data->keycount > MPR121_MAX_KEY_COUNT) {
> +		dev_err(&client->dev, "Too many key defined\n");
> +		error = -EINVAL;
> +		goto err_free_mem;
> +	}
> +
> +	error = mpr121_phys_init(pdata, data, client);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to init register\n");
> +		goto err_free_mem;
> +	}
> +
> +	i2c_set_clientdata(client, data);

You should do this at the very end, before returning success (0).
Actually, you do do this, so just remove this instance.

> +
> +	input_dev->name = "FSL MPR121 Touchkey";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &client->dev;
> +	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
> +	input_dev->keycode = pdata->matrix;

No, you are assigning pointer to the platform data, instead of pointer
to the data in device structure. That is why I'd rather you renamed
'struct mpr121_touchkey_data' to 'struct mpr121_touchkey' and 'data' to
'mpr121' so as to avoid confusion with platform data.

> +	input_dev->keycodesize = sizeof(pdata->matrix[0]);
> +	input_dev->keycodemax = data->keycount;
> +
> +	for (i = 0; i < input_dev->keycodemax; i++) {
> +		__set_bit(pdata->matrix[i], input_dev->keybit);
> +		data->keycodes[i] = pdata->matrix[i];
> +	}
> +
> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +	input_set_drvdata(input_dev, data);
> +
> +	error = request_threaded_irq(client->irq, NULL,
> +				     mpr_touchkey_interrupt,
> +				     IRQF_TRIGGER_FALLING,
> +				     client->dev.driver->name, data);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		goto err_free_mem;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error)
> +		goto err_free_irq;
> +	i2c_set_clientdata(client, data);
> +	device_init_wakeup(&client->dev, pdata->wakeup);
> +	dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");

No need for having this message. You can see the device bound to the
driver in sysfs. The boot is noisy enough.

> +	return 0;
> +
> +err_free_irq:
> +	free_irq(client->irq, data);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(data);
> +	return error;
> +}
> +
> +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
> +{
> +	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> +
> +	free_irq(client->irq, data);
> +	input_unregister_device(data->input_dev);
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mpr_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(client->irq);
> +
> +	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
> +
> +	return 0;
> +}
> +
> +static int mpr_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> +
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(client->irq);
> +
> +	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct i2c_device_id mpr121_id[] = {
> +	{"mpr121_touchkey", 0},
> +	{ }
> +};
> +
> +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
> +
> +static struct i2c_driver mpr_touchkey_driver = {
> +	.driver = {
> +		.name	= "mpr121",
> +		.owner	= THIS_MODULE,
> +		.pm	= &mpr121_touchkey_pm_ops,
> +	},
> +	.id_table	= mpr121_id,
> +	.probe		= mpr_touchkey_probe,
> +	.remove		= __devexit_p(mpr_touchkey_remove),
> +};
> +
> +static int __init mpr_touchkey_init(void)
> +{
> +	return i2c_add_driver(&mpr_touchkey_driver);
> +}
> +
> +static void __exit mpr_touchkey_exit(void)
> +{
> +	i2c_del_driver(&mpr_touchkey_driver);
> +}
> +
> +module_init(mpr_touchkey_init);
> +module_exit(mpr_touchkey_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
> +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
> diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
> new file mode 100644
> index 0000000..52d6e33
> --- /dev/null
> +++ b/include/linux/i2c/mpr.h
> @@ -0,0 +1,64 @@
> +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
> + * controllor */
> +
> +#ifndef MPR_H
> +#define MPR_H
> +
> +/* Register definitions */
> +#define ELE_TOUCH_STATUS_0_ADDR	0x0
> +#define ELE_TOUCH_STATUS_1_ADDR	0X1
> +#define MHD_RISING_ADDR		0x2b
> +#define NHD_RISING_ADDR		0x2c
> +#define NCL_RISING_ADDR		0x2d
> +#define FDL_RISING_ADDR		0x2e
> +#define MHD_FALLING_ADDR	0x2f
> +#define NHD_FALLING_ADDR	0x30
> +#define NCL_FALLING_ADDR	0x31
> +#define FDL_FALLING_ADDR	0x32
> +#define ELE0_TOUCH_THRESHOLD_ADDR	0x41
> +#define ELE0_RELEASE_THRESHOLD_ADDR	0x42
> +/* ELE0...ELE11's threshold will set in a loop */
> +#define AFE_CONF_ADDR			0x5c
> +#define FILTER_CONF_ADDR		0x5d
> +
> +/* ELECTRODE_CONF: this register is most important register, it
> + * control how many of electrode is enabled. If you set this register
> + * to 0x0, it make the sensor going to suspend mode. Other value(low
> + * bit is non-zero) will make the sensor into Run mode.  This register
> + * should be write at last.

Hmm, most of the comments require proper grammar. If you need help with
it - let the list know, we'll try to help.

> + */
> +#define ELECTRODE_CONF_ADDR		0x5e
> +#define AUTO_CONFIG_CTRL_ADDR		0x7b
> +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
> + * register is related to VDD supplied on your board, the value of
> + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
> + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
> + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
> + * is 90% of USL  */
> +#define AUTO_CONFIG_USL_ADDR		0x7d
> +#define AUTO_CONFIG_LSL_ADDR		0x7e
> +#define AUTO_CONFIG_TL_ADDR		0x7f
> +
> +/* Threshold of touch/release trigger */
> +#define TOUCH_THRESHOLD			0x0f
> +#define RELEASE_THRESHOLD		0x0a
> +/* Mask Button bits of STATUS_0 & STATUS_1 register */
> +#define TOUCH_STATUS_MASK		0xfff
> +/* MPR121 have 12 electrodes */
> +#define MPR121_MAX_KEY_COUNT		12
> +
> +
> +/**
> + * @keycount: how many key maped
> + * @vdd_uv: voltage of vdd supply the chip in uV
> + * @matrix: maxtrix of keys
> + * @wakeup: can key wake up system.
> + */
> +struct mpr121_platform_data {
> +	u16 keycount;
> +	u16 *matrix;

If this device is indeed organized as a matrix (lookslike this) maybe
you should look at definitions provided by
include/linux/input/matrix_keypad.h

> +	int wakeup;

bool?

> +	int vdd_uv;
> +};
> +
> +#endif

Thanks.
JieJing.Zhang April 12, 2011, 11:41 a.m. UTC | #4
Hi Christoph,

I'm sorry for my poor English grammar.
I'll try to fix these typo, If still have error, please point me out.

2011/4/12 Christoph Fritz <chf.fritz@googlemail.com>
>
> Hi Zhang,
>
>  this is just a superficial review of your driver.
>
> thanks,
>  Christoph
>
> On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> > From: Zhang Jiejing <jiejing.zhang@freescale.com>
> >
> > This touchkey driver is based on Freescale mpr121 capacitive
> > touch sensor controller.
> >
> > It can support up to 12 keys, it use i2c interface.
>
> typo

-> It can supports up to 12 electrodes, and using i2c interface.
>
> >
> > The product infomation(data sheet, application notes) can
> > be found under this link:
> > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121
> >
> > Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
> > ---
> >  drivers/input/keyboard/Kconfig  |   12 ++
> >  drivers/input/keyboard/Makefile |    1 +
> >  drivers/input/keyboard/mpr121.c |  301 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/i2c/mpr.h         |   64 ++++++++
> >  4 files changed, 378 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/input/keyboard/mpr121.c
> >  create mode 100644 include/linux/i2c/mpr.h
> >
> > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> > index b16bed0..0666e1e 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -520,6 +520,18 @@ config KEYBOARD_XTKBD
> >         To compile this driver as a module, choose M here: the
> >         module will be called xtkbd.
> >
> > +config KEYBOARD_MPR121
> > +     tristate "Freescale MPR121 Touchkey Driver"
> > +     depends on I2C
> > +     help
> > +       Say Y here if you have the  touchkey Freescale mpr121 capacitive
>
> typo: two spaces

will fix.
>
> > +       touch sensor controller in your system.
> > +
> > +       If unsure, say N.
> > +
> > +       To compile this driver as a module, choose M here
> > +endif
> > +
> >  config KEYBOARD_W90P910
> >       tristate "W90P910 Matrix Keypad support"
> >       depends on ARCH_W90X900
>
> - add: "To compile this driver as a module, choose M here: the
>  module will be called mpr121."
> - "endif" is wrong here, it breaks 'make config'
> - maybe a more alphabetic approach in this list of drivers would
>  be nice

yes. I will.
>
> > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> > index 878e6c2..b110ca8 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -47,4 +47,5 @@ obj-$(CONFIG_KEYBOARD_TEGRA)                += tegra-kbc.o
> >  obj-$(CONFIG_KEYBOARD_TNETV107X)     += tnetv107x-keypad.o
> >  obj-$(CONFIG_KEYBOARD_TWL4030)               += twl4030_keypad.o
> >  obj-$(CONFIG_KEYBOARD_XTKBD)         += xtkbd.o
> > +obj-$(CONFIG_KEYBOARD_MPR121)                += mpr121.o
> >  obj-$(CONFIG_KEYBOARD_W90P910)               += w90p910_keypad.o
>
> for better examination, please mind alphabetic order

will do.
>
> > diff --git a/drivers/input/keyboard/mpr121.c b/drivers/input/keyboard/mpr121.c
> > new file mode 100644
> > index 0000000..ee149a8
> > --- /dev/null
> > +++ b/drivers/input/keyboard/mpr121.c
> > @@ -0,0 +1,301 @@
> > +/*
> > + * Touchkey driver for Freescale MPR121 Controllor
> > + *
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + * Author: Zhang Jiejing <jiejing.zhang@freescale.com>
> > + *
> > + * Based on mcs_touchkey.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.
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/i2c.h>
> > +#include <linux/i2c/mpr.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/input.h>
> > +#include <linux/irq.h>
>
> do not use irq.h directly

ok.
>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/bitops.h>
> > +
> > +struct mpr121_touchkey_data {
> > +     struct i2c_client       *client;
> > +     struct input_dev        *input_dev;
> > +     unsigned int            key_val;
> > +     int                     statusbits;
> > +     int                     keycount;
> > +     u16                     keycodes[MPR121_MAX_KEY_COUNT];
> > +};
> > +
> > +struct mpr121_init_register {
> > +     int addr;
> > +     u8 val;
> > +};
> > +
> > +static struct mpr121_init_register init_reg_table[] = {
> > +     {MHD_RISING_ADDR,       0x1},
> > +     {NHD_RISING_ADDR,       0x1},
> > +     {MHD_FALLING_ADDR,      0x1},
> > +     {NHD_FALLING_ADDR,      0x1},
> > +     {NCL_FALLING_ADDR,      0xff},
> > +     {FDL_FALLING_ADDR,      0x02},
> > +     {FILTER_CONF_ADDR,      0x04},
> > +     {AFE_CONF_ADDR,         0x0b},
> > +     {AUTO_CONFIG_CTRL_ADDR, 0x0b},
> > +};
> > +
> > +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> > +{
> > +     struct mpr121_touchkey_data *data = dev_id;
> > +     struct i2c_client *client = data->client;
> > +     struct input_dev *input = data->input_dev;
> > +     unsigned int key_num, pressed;
> > +     int reg;
> > +
> > +     reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
> > +     if (reg < 0) {
> > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
> > +             goto out;
> > +     }
> > +
> > +     reg <<= 8;
> > +     reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
> > +     if (reg < 0) {
> > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
> > +             goto out;
> > +     }
> > +
> > +     reg &= TOUCH_STATUS_MASK;
> > +     /* use old press bit to figure out which bit changed */
> > +     key_num = ffs(reg ^ data->statusbits) - 1;
> > +     /* use the bit check the press status */
>
> typo

this comment will remove.
>
> > +     pressed = (reg & (1 << (key_num))) >> key_num;
> > +     data->statusbits = reg;
> > +     data->key_val = data->keycodes[key_num];
> > +
> > +     input_event(input, EV_MSC, MSC_SCAN, data->key_val);
> > +     input_report_key(input, data->key_val, pressed);
> > +     input_sync(input);
> > +
> > +     dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
> > +             pressed ? "pressed" : "released");
> > +
> > +out:
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int mpr121_phys_init(struct mpr121_platform_data *pdata,
> > +                         struct mpr121_touchkey_data *data,
> > +                         struct i2c_client *client)
> > +{
> > +     struct mpr121_init_register *reg;
> > +     unsigned char usl, lsl, tl;
> > +     int i, t, vdd, ret;
> > +
> > +     /* setup touch/release threshold for ele0-ele11 */
> > +     for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
> > +             t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
> > +             ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
> > +             if (ret < 0)
> > +                     goto err_i2c_write;
> > +             ret = i2c_smbus_write_byte_data(client, t + 1,
> > +                                             RELEASE_THRESHOLD);
> > +             if (ret < 0)
> > +                     goto err_i2c_write;
> > +     }
> > +     /* setup init register */
> > +     for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
> > +             reg = &init_reg_table[i];
> > +             ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
> > +             if (ret < 0)
> > +                     goto err_i2c_write;
> > +     }
> > +     /* setup auto-register by vdd,the formula please ref: AN3889
> > +      * These register *must* set acrodding your VDD voltage supply
> > +      * to the chip*/
>
> typo

is this right?
    /* setup Auto Config Registers by VDD.
     * The formula was written in AN3889.
     * These registers *must* be set by VDD voltage.
     */
>
> > +     vdd = pdata->vdd_uv / 1000;
> > +     usl = ((vdd - 700) * 256) / vdd;
> > +     lsl = (usl * 65) / 100;
> > +     tl = (usl * 90) / 100;
> > +     ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
> > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
> > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
> > +     ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
> > +                                     data->keycount);
> > +     if (ret != 0)
> > +             goto err_i2c_write;
> > +
> > +     dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
>
> typo ?

-> dev_dbg(&client->dev, "mpr121: setup with %x electrodes.\n",

>
> > +              data->keycount);
> > +
> > +     return 0;
> > +
> > +err_i2c_write:
> > +     dev_err(&client->dev, "i2c write error: %d\n", ret);
> > +     return ret;
> > +}
> > +
> > +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
> > +                                     const struct i2c_device_id *id)
> > +{
> > +     struct mpr121_platform_data *pdata;
> > +     struct mpr121_touchkey_data *data;
> > +     struct input_dev *input_dev;
> > +     int error;
> > +     int i;
> > +
> > +     pdata = client->dev.platform_data;
> > +     if (!pdata) {
> > +             dev_err(&client->dev, "no platform data defined\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!client->irq) {
> > +             dev_err(&client->dev, "The irq number should not be zero\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
> > +     input_dev = input_allocate_device();
> > +     if (!data || !input_dev) {
> > +             dev_err(&client->dev, "Falied to allocate memory\n");
> > +             error = -ENOMEM;
> > +             goto err_free_mem;
> > +     }
> > +
> > +     data->client = client;
> > +     data->input_dev = input_dev;
> > +     data->keycount = pdata->keycount;
> > +
> > +     if (data->keycount > MPR121_MAX_KEY_COUNT) {
> > +             dev_err(&client->dev, "Too many key defined\n");
>
> typo

-> dev_err(&client->dev, "too much keys defined\n");
>
> > +             error = -EINVAL;
> > +             goto err_free_mem;
> > +     }
> > +
> > +     error = mpr121_phys_init(pdata, data, client);
> > +     if (error) {
> > +             dev_err(&client->dev, "Failed to init register\n");
> > +             goto err_free_mem;
> > +     }
> > +
> > +     i2c_set_clientdata(client, data);
> > +
> > +     input_dev->name = "FSL MPR121 Touchkey";
> > +     input_dev->id.bustype = BUS_I2C;
> > +     input_dev->dev.parent = &client->dev;
> > +     input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
> > +     input_dev->keycode = pdata->matrix;
> > +     input_dev->keycodesize = sizeof(pdata->matrix[0]);
> > +     input_dev->keycodemax = data->keycount;
> > +
> > +     for (i = 0; i < input_dev->keycodemax; i++) {
> > +             __set_bit(pdata->matrix[i], input_dev->keybit);
> > +             data->keycodes[i] = pdata->matrix[i];
> > +     }
> > +
> > +     input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > +     input_set_drvdata(input_dev, data);
> > +
> > +     error = request_threaded_irq(client->irq, NULL,
> > +                                  mpr_touchkey_interrupt,
> > +                                  IRQF_TRIGGER_FALLING,
> > +                                  client->dev.driver->name, data);
> > +     if (error) {
> > +             dev_err(&client->dev, "Failed to register interrupt\n");
> > +             goto err_free_mem;
> > +     }
> > +
> > +     error = input_register_device(input_dev);
> > +     if (error)
> > +             goto err_free_irq;
> > +     i2c_set_clientdata(client, data);
> > +     device_init_wakeup(&client->dev, pdata->wakeup);
> > +     dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
> > +     return 0;
> > +
> > +err_free_irq:
> > +     free_irq(client->irq, data);
> > +err_free_mem:
> > +     input_free_device(input_dev);
> > +     kfree(data);
> > +     return error;
> > +}
> > +
> > +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
> > +{
> > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> > +
> > +     free_irq(client->irq, data);
> > +     input_unregister_device(data->input_dev);
> > +     kfree(data);
> > +
> > +     return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mpr_suspend(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +
> > +     if (device_may_wakeup(&client->dev))
> > +             enable_irq_wake(client->irq);
> > +
> > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpr_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> > +
> > +     if (device_may_wakeup(&client->dev))
> > +             disable_irq_wake(client->irq);
> > +
> > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
> > +
> > +     return 0;
> > +}
> > +#endif
> > +
> > +static const struct i2c_device_id mpr121_id[] = {
> > +     {"mpr121_touchkey", 0},
> > +     { }
> > +};
> > +
> > +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
> > +
> > +static struct i2c_driver mpr_touchkey_driver = {
> > +     .driver = {
> > +             .name   = "mpr121",
> > +             .owner  = THIS_MODULE,
> > +             .pm     = &mpr121_touchkey_pm_ops,
>
> I would add a ifdef CONFIG_PM_SLEEP
>
> > +     },
> > +     .id_table       = mpr121_id,
> > +     .probe          = mpr_touchkey_probe,
> > +     .remove         = __devexit_p(mpr_touchkey_remove),
> > +};
> > +
> > +static int __init mpr_touchkey_init(void)
> > +{
> > +     return i2c_add_driver(&mpr_touchkey_driver);
> > +}
> > +
> > +static void __exit mpr_touchkey_exit(void)
> > +{
> > +     i2c_del_driver(&mpr_touchkey_driver);
> > +}
> > +
> > +module_init(mpr_touchkey_init);
> > +module_exit(mpr_touchkey_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
> > +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
> > diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
> > new file mode 100644
> > index 0000000..52d6e33
> > --- /dev/null
> > +++ b/include/linux/i2c/mpr.h
> > @@ -0,0 +1,64 @@
> > +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
>
> no filename please

will do.
>
> > + * controllor */
> > +
> > +#ifndef MPR_H
> > +#define MPR_H
> > +
> > +/* Register definitions */
> > +#define ELE_TOUCH_STATUS_0_ADDR      0x0
> > +#define ELE_TOUCH_STATUS_1_ADDR      0X1
> > +#define MHD_RISING_ADDR              0x2b
> > +#define NHD_RISING_ADDR              0x2c
> > +#define NCL_RISING_ADDR              0x2d
> > +#define FDL_RISING_ADDR              0x2e
> > +#define MHD_FALLING_ADDR     0x2f
> > +#define NHD_FALLING_ADDR     0x30
> > +#define NCL_FALLING_ADDR     0x31
> > +#define FDL_FALLING_ADDR     0x32
> > +#define ELE0_TOUCH_THRESHOLD_ADDR    0x41
> > +#define ELE0_RELEASE_THRESHOLD_ADDR  0x42
> > +/* ELE0...ELE11's threshold will set in a loop */
>
> typo

-> /* ELE0...ELE11's threshold will be set in a loop */
>
> > +#define AFE_CONF_ADDR                        0x5c
> > +#define FILTER_CONF_ADDR             0x5d
> > +
> > +/* ELECTRODE_CONF: this register is most important register, it
> > + * control how many of electrode is enabled. If you set this register
> > + * to 0x0, it make the sensor going to suspend mode. Other value(low
> > + * bit is non-zero) will make the sensor into Run mode.  This register
> > + * should be write at last.
>
> typo

->  * should be written in the end.

>
> > + */
> > +#define ELECTRODE_CONF_ADDR          0x5e
> > +#define AUTO_CONFIG_CTRL_ADDR                0x7b
> > +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
> > + * register is related to VDD supplied on your board, the value of
> > + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
>
> typo

->  * this register is calculated by EQ: `((VDD-0.7)/VDD) * 256`.

>
> > + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
> > + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
> > + * is 90% of USL  */
>
> typo

->  * USL AUTO_CONFIG_TL: The traget level of auto baseline search, it is
->  * 90% of USL */

>
> > +#define AUTO_CONFIG_USL_ADDR         0x7d
> > +#define AUTO_CONFIG_LSL_ADDR         0x7e
> > +#define AUTO_CONFIG_TL_ADDR          0x7f
> > +
> > +/* Threshold of touch/release trigger */
> > +#define TOUCH_THRESHOLD                      0x0f
> > +#define RELEASE_THRESHOLD            0x0a
> > +/* Mask Button bits of STATUS_0 & STATUS_1 register */
>
> and or bit-and?

don't need and or bit-and, it just means low 12 bits of status
register is touch key's status.
-> /* low 12 bits of status register is touch key's status */
>
> > +#define TOUCH_STATUS_MASK            0xfff
> > +/* MPR121 have 12 electrodes */
> > +#define MPR121_MAX_KEY_COUNT         12
> > +
> > +
> > +/**
> > + * @keycount: how many key maped
>
> typo: "keys" ?

->  * @keycount: how many keys mapped
>
> > + * @vdd_uv: voltage of vdd supply the chip in uV
>
> - typo
> - uppercase for vdd

->  * @vdd_uv: VDD voltage in uV

>
> > + * @matrix: maxtrix of keys
> > + * @wakeup: can key wake up system.
>
> typo

->  * @wakeup: can wake up system or not
>
> > + */
> > +struct mpr121_platform_data {
> > +     u16 keycount;
> > +     u16 *matrix;
> > +     int wakeup;
> > +     int vdd_uv;
> > +};
> > +
> > +#endif
> > --
> > 1.7.1
> >
> > --
> > 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.htmln Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:

:-(
sorry again for my horrible grammar.

Jiejing
--
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
Christoph Fritz April 12, 2011, 2:29 p.m. UTC | #5
On Tue, 2011-04-12 at 19:41 +0800, Jiejing.Zhang wrote:
> Hi Christoph,
> 
> I'm sorry for my poor English grammar.

I suppose the list can help. Here is my input:

> I'll try to fix these typo, If still have error, please point me out.
> 
> 2011/4/12 Christoph Fritz <chf.fritz@googlemail.com>
> >
> > Hi Zhang,
> >
> >  this is just a superficial review of your driver.
> >
> > thanks,
> >  Christoph
> >
> > On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> > > From: Zhang Jiejing <jiejing.zhang@freescale.com>
> > >
> > > This touchkey driver is based on Freescale mpr121 capacitive
> > > touch sensor controller.
> > >
> > > It can support up to 12 keys, it use i2c interface.
> >
> > typo
> 
> -> It can supports up to 12 electrodes, and using i2c interface.

This patch adds basic support for Freescale MPR121 capacitive touch
sensor.
It's an i2c controller with up to 12 capacitance sensing inputs.

> >
> > >
> > > The product infomation(data sheet, application notes) can
> > > be found under this link:
> > > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121

Product information (data sheet, application notes) can be found here:

> > >
> > > Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
> > > ---
> > >  drivers/input/keyboard/Kconfig  |   12 ++
> > >  drivers/input/keyboard/Makefile |    1 +
> > >  drivers/input/keyboard/mpr121.c |  301 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/i2c/mpr.h         |   64 ++++++++
> > >  4 files changed, 378 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/input/keyboard/mpr121.c
> > >  create mode 100644 include/linux/i2c/mpr.h
> > >
> > > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> > > index b16bed0..0666e1e 100644
> > > --- a/drivers/input/keyboard/Kconfig
> > > +++ b/drivers/input/keyboard/Kconfig
> > > @@ -520,6 +520,18 @@ config KEYBOARD_XTKBD
> > >         To compile this driver as a module, choose M here: the
> > >         module will be called xtkbd.
> > >
> > > +config KEYBOARD_MPR121
> > > +     tristate "Freescale MPR121 Touchkey Driver"
> > > +     depends on I2C
> > > +     help
> > > +       Say Y here if you have the  touchkey Freescale mpr121 capacitive
> >
> > typo: two spaces
> 
> will fix.
> >
> > > +       touch sensor controller in your system.
> > > +
> > > +       If unsure, say N.
> > > +
> > > +       To compile this driver as a module, choose M here
> > > +endif
> > > +
> > >  config KEYBOARD_W90P910
> > >       tristate "W90P910 Matrix Keypad support"
> > >       depends on ARCH_W90X900
> >
> > - add: "To compile this driver as a module, choose M here: the
> >  module will be called mpr121."
> > - "endif" is wrong here, it breaks 'make config'
> > - maybe a more alphabetic approach in this list of drivers would
> >  be nice
> 
> yes. I will.
> >
> > > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> > > index 878e6c2..b110ca8 100644
> > > --- a/drivers/input/keyboard/Makefile
> > > +++ b/drivers/input/keyboard/Makefile
> > > @@ -47,4 +47,5 @@ obj-$(CONFIG_KEYBOARD_TEGRA)                += tegra-kbc.o
> > >  obj-$(CONFIG_KEYBOARD_TNETV107X)     += tnetv107x-keypad.o
> > >  obj-$(CONFIG_KEYBOARD_TWL4030)               += twl4030_keypad.o
> > >  obj-$(CONFIG_KEYBOARD_XTKBD)         += xtkbd.o
> > > +obj-$(CONFIG_KEYBOARD_MPR121)                += mpr121.o
> > >  obj-$(CONFIG_KEYBOARD_W90P910)               += w90p910_keypad.o
> >
> > for better examination, please mind alphabetic order
> 
> will do.
> >
> > > diff --git a/drivers/input/keyboard/mpr121.c b/drivers/input/keyboard/mpr121.c
> > > new file mode 100644
> > > index 0000000..ee149a8
> > > --- /dev/null
> > > +++ b/drivers/input/keyboard/mpr121.c
> > > @@ -0,0 +1,301 @@
> > > +/*
> > > + * Touchkey driver for Freescale MPR121 Controllor
> > > + *
> > > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > > + * Author: Zhang Jiejing <jiejing.zhang@freescale.com>
> > > + *
> > > + * Based on mcs_touchkey.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.
> > > + *
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/init.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/i2c/mpr.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/input.h>
> > > +#include <linux/irq.h>
> >
> > do not use irq.h directly
> 
> ok.
> >
> > > +#include <linux/slab.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/bitops.h>
> > > +
> > > +struct mpr121_touchkey_data {
> > > +     struct i2c_client       *client;
> > > +     struct input_dev        *input_dev;
> > > +     unsigned int            key_val;
> > > +     int                     statusbits;
> > > +     int                     keycount;
> > > +     u16                     keycodes[MPR121_MAX_KEY_COUNT];
> > > +};
> > > +
> > > +struct mpr121_init_register {
> > > +     int addr;
> > > +     u8 val;
> > > +};
> > > +
> > > +static struct mpr121_init_register init_reg_table[] = {
> > > +     {MHD_RISING_ADDR,       0x1},
> > > +     {NHD_RISING_ADDR,       0x1},
> > > +     {MHD_FALLING_ADDR,      0x1},
> > > +     {NHD_FALLING_ADDR,      0x1},
> > > +     {NCL_FALLING_ADDR,      0xff},
> > > +     {FDL_FALLING_ADDR,      0x02},
> > > +     {FILTER_CONF_ADDR,      0x04},
> > > +     {AFE_CONF_ADDR,         0x0b},
> > > +     {AUTO_CONFIG_CTRL_ADDR, 0x0b},
> > > +};
> > > +
> > > +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> > > +{
> > > +     struct mpr121_touchkey_data *data = dev_id;
> > > +     struct i2c_client *client = data->client;
> > > +     struct input_dev *input = data->input_dev;
> > > +     unsigned int key_num, pressed;
> > > +     int reg;
> > > +
> > > +     reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
> > > +     if (reg < 0) {
> > > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
> > > +             goto out;
> > > +     }
> > > +
> > > +     reg <<= 8;
> > > +     reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
> > > +     if (reg < 0) {
> > > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
> > > +             goto out;
> > > +     }
> > > +
> > > +     reg &= TOUCH_STATUS_MASK;
> > > +     /* use old press bit to figure out which bit changed */
> > > +     key_num = ffs(reg ^ data->statusbits) - 1;
> > > +     /* use the bit check the press status */
> >
> > typo
> 
> this comment will remove.
> >
> > > +     pressed = (reg & (1 << (key_num))) >> key_num;
> > > +     data->statusbits = reg;
> > > +     data->key_val = data->keycodes[key_num];
> > > +
> > > +     input_event(input, EV_MSC, MSC_SCAN, data->key_val);
> > > +     input_report_key(input, data->key_val, pressed);
> > > +     input_sync(input);
> > > +
> > > +     dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
> > > +             pressed ? "pressed" : "released");
> > > +
> > > +out:
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int mpr121_phys_init(struct mpr121_platform_data *pdata,
> > > +                         struct mpr121_touchkey_data *data,
> > > +                         struct i2c_client *client)
> > > +{
> > > +     struct mpr121_init_register *reg;
> > > +     unsigned char usl, lsl, tl;
> > > +     int i, t, vdd, ret;
> > > +
> > > +     /* setup touch/release threshold for ele0-ele11 */
> > > +     for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
> > > +             t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
> > > +             ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
> > > +             if (ret < 0)
> > > +                     goto err_i2c_write;
> > > +             ret = i2c_smbus_write_byte_data(client, t + 1,
> > > +                                             RELEASE_THRESHOLD);
> > > +             if (ret < 0)
> > > +                     goto err_i2c_write;
> > > +     }
> > > +     /* setup init register */
> > > +     for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
> > > +             reg = &init_reg_table[i];
> > > +             ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
> > > +             if (ret < 0)
> > > +                     goto err_i2c_write;
> > > +     }
> > > +     /* setup auto-register by vdd,the formula please ref: AN3889
> > > +      * These register *must* set acrodding your VDD voltage supply
> > > +      * to the chip*/
> >
> > typo
> 
> is this right?
>     /* setup Auto Config Registers by VDD.
>      * The formula was written in AN3889.
>      * These registers *must* be set by VDD voltage.
>      */

Capacitance on sensing input varies and needs to be compensated. The
internal MPR121-auto-configuration can do this if it's registers are set
properly (based on pdata->vdd_uv).

> >
> > > +     vdd = pdata->vdd_uv / 1000;
> > > +     usl = ((vdd - 700) * 256) / vdd;
> > > +     lsl = (usl * 65) / 100;
> > > +     tl = (usl * 90) / 100;
> > > +     ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
> > > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
> > > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
> > > +     ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
> > > +                                     data->keycount);
> > > +     if (ret != 0)
> > > +             goto err_i2c_write;
> > > +
> > > +     dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
> >
> > typo ?
> 
> -> dev_dbg(&client->dev, "mpr121: setup with %x electrodes.\n",

Do you mean keys or "capacitance sensing inputs"?

> >
> > > +              data->keycount);
> > > +
> > > +     return 0;
> > > +
> > > +err_i2c_write:
> > > +     dev_err(&client->dev, "i2c write error: %d\n", ret);
> > > +     return ret;
> > > +}
> > > +
> > > +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
> > > +                                     const struct i2c_device_id *id)
> > > +{
> > > +     struct mpr121_platform_data *pdata;
> > > +     struct mpr121_touchkey_data *data;
> > > +     struct input_dev *input_dev;
> > > +     int error;
> > > +     int i;
> > > +
> > > +     pdata = client->dev.platform_data;
> > > +     if (!pdata) {
> > > +             dev_err(&client->dev, "no platform data defined\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     if (!client->irq) {
> > > +             dev_err(&client->dev, "The irq number should not be zero\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
> > > +     input_dev = input_allocate_device();
> > > +     if (!data || !input_dev) {
> > > +             dev_err(&client->dev, "Falied to allocate memory\n");
> > > +             error = -ENOMEM;
> > > +             goto err_free_mem;
> > > +     }
> > > +
> > > +     data->client = client;
> > > +     data->input_dev = input_dev;
> > > +     data->keycount = pdata->keycount;
> > > +
> > > +     if (data->keycount > MPR121_MAX_KEY_COUNT) {
> > > +             dev_err(&client->dev, "Too many key defined\n");
> >
> > typo
> 
> -> dev_err(&client->dev, "too much keys defined\n");

"too many keys defined"

> >
> > > +             error = -EINVAL;
> > > +             goto err_free_mem;
> > > +     }
> > > +
> > > +     error = mpr121_phys_init(pdata, data, client);
> > > +     if (error) {
> > > +             dev_err(&client->dev, "Failed to init register\n");
> > > +             goto err_free_mem;
> > > +     }
> > > +
> > > +     i2c_set_clientdata(client, data);
> > > +
> > > +     input_dev->name = "FSL MPR121 Touchkey";
> > > +     input_dev->id.bustype = BUS_I2C;
> > > +     input_dev->dev.parent = &client->dev;
> > > +     input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
> > > +     input_dev->keycode = pdata->matrix;
> > > +     input_dev->keycodesize = sizeof(pdata->matrix[0]);
> > > +     input_dev->keycodemax = data->keycount;
> > > +
> > > +     for (i = 0; i < input_dev->keycodemax; i++) {
> > > +             __set_bit(pdata->matrix[i], input_dev->keybit);
> > > +             data->keycodes[i] = pdata->matrix[i];
> > > +     }
> > > +
> > > +     input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > > +     input_set_drvdata(input_dev, data);
> > > +
> > > +     error = request_threaded_irq(client->irq, NULL,
> > > +                                  mpr_touchkey_interrupt,
> > > +                                  IRQF_TRIGGER_FALLING,
> > > +                                  client->dev.driver->name, data);
> > > +     if (error) {
> > > +             dev_err(&client->dev, "Failed to register interrupt\n");
> > > +             goto err_free_mem;
> > > +     }
> > > +
> > > +     error = input_register_device(input_dev);
> > > +     if (error)
> > > +             goto err_free_irq;
> > > +     i2c_set_clientdata(client, data);
> > > +     device_init_wakeup(&client->dev, pdata->wakeup);
> > > +     dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
> > > +     return 0;
> > > +
> > > +err_free_irq:
> > > +     free_irq(client->irq, data);
> > > +err_free_mem:
> > > +     input_free_device(input_dev);
> > > +     kfree(data);
> > > +     return error;
> > > +}
> > > +
> > > +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
> > > +{
> > > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> > > +
> > > +     free_irq(client->irq, data);
> > > +     input_unregister_device(data->input_dev);
> > > +     kfree(data);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int mpr_suspend(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +
> > > +     if (device_may_wakeup(&client->dev))
> > > +             enable_irq_wake(client->irq);
> > > +
> > > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int mpr_resume(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
> > > +
> > > +     if (device_may_wakeup(&client->dev))
> > > +             disable_irq_wake(client->irq);
> > > +
> > > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
> > > +
> > > +     return 0;
> > > +}
> > > +#endif
> > > +
> > > +static const struct i2c_device_id mpr121_id[] = {
> > > +     {"mpr121_touchkey", 0},
> > > +     { }
> > > +};
> > > +
> > > +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
> > > +
> > > +static struct i2c_driver mpr_touchkey_driver = {
> > > +     .driver = {
> > > +             .name   = "mpr121",
> > > +             .owner  = THIS_MODULE,
> > > +             .pm     = &mpr121_touchkey_pm_ops,
> >
> > I would add a ifdef CONFIG_PM_SLEEP
> >
> > > +     },
> > > +     .id_table       = mpr121_id,
> > > +     .probe          = mpr_touchkey_probe,
> > > +     .remove         = __devexit_p(mpr_touchkey_remove),
> > > +};
> > > +
> > > +static int __init mpr_touchkey_init(void)
> > > +{
> > > +     return i2c_add_driver(&mpr_touchkey_driver);
> > > +}
> > > +
> > > +static void __exit mpr_touchkey_exit(void)
> > > +{
> > > +     i2c_del_driver(&mpr_touchkey_driver);
> > > +}
> > > +
> > > +module_init(mpr_touchkey_init);
> > > +module_exit(mpr_touchkey_exit);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
> > > +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
> > > diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
> > > new file mode 100644
> > > index 0000000..52d6e33
> > > --- /dev/null
> > > +++ b/include/linux/i2c/mpr.h
> > > @@ -0,0 +1,64 @@
> > > +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
> >
> > no filename please
> 
> will do.
> >
> > > + * controllor */
> > > +
> > > +#ifndef MPR_H
> > > +#define MPR_H
> > > +
> > > +/* Register definitions */
> > > +#define ELE_TOUCH_STATUS_0_ADDR      0x0
> > > +#define ELE_TOUCH_STATUS_1_ADDR      0X1
> > > +#define MHD_RISING_ADDR              0x2b
> > > +#define NHD_RISING_ADDR              0x2c
> > > +#define NCL_RISING_ADDR              0x2d
> > > +#define FDL_RISING_ADDR              0x2e
> > > +#define MHD_FALLING_ADDR     0x2f
> > > +#define NHD_FALLING_ADDR     0x30
> > > +#define NCL_FALLING_ADDR     0x31
> > > +#define FDL_FALLING_ADDR     0x32
> > > +#define ELE0_TOUCH_THRESHOLD_ADDR    0x41
> > > +#define ELE0_RELEASE_THRESHOLD_ADDR  0x42
> > > +/* ELE0...ELE11's threshold will set in a loop */
> >
> > typo
> 
> -> /* ELE0...ELE11's threshold will be set in a loop */
> >
> > > +#define AFE_CONF_ADDR                        0x5c
> > > +#define FILTER_CONF_ADDR             0x5d
> > > +
> > > +/* ELECTRODE_CONF: this register is most important register, it
> > > + * control how many of electrode is enabled. If you set this register
> > > + * to 0x0, it make the sensor going to suspend mode. Other value(low
> > > + * bit is non-zero) will make the sensor into Run mode.  This register
> > > + * should be write at last.
> >
> > typo
> 
> ->  * should be written in the end.

ELECTRODE_CONF: This register mainly configures the number of enabled
capacitance sensing inputs and its run/suspend mode.

> 
> >
> > > + */
> > > +#define ELECTRODE_CONF_ADDR          0x5e
> > > +#define AUTO_CONFIG_CTRL_ADDR                0x7b
> > > +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
> > > + * register is related to VDD supplied on your board, the value of
> > > + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
> >
> > typo
> 
> ->  * this register is calculated by EQ: `((VDD-0.7)/VDD) * 256`.

It's in the source how it gets calculated, so I don't think you need to
document it here.

> 
> >
> > > + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
> > > + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
> > > + * is 90% of USL  */
> >
> > typo
> 
> ->  * USL AUTO_CONFIG_TL: The traget level of auto baseline search, it is
> ->  * 90% of USL */

just remove 'is', typo in target

> 
> >
> > > +#define AUTO_CONFIG_USL_ADDR         0x7d
> > > +#define AUTO_CONFIG_LSL_ADDR         0x7e
> > > +#define AUTO_CONFIG_TL_ADDR          0x7f

Why do they need to be in the header file?
Couldn't be a lot of defines get moved into the actual driver? 

> > > +
> > > +/* Threshold of touch/release trigger */
> > > +#define TOUCH_THRESHOLD                      0x0f
> > > +#define RELEASE_THRESHOLD            0x0a
> > > +/* Mask Button bits of STATUS_0 & STATUS_1 register */
> >
> > and or bit-and?
> 
> don't need and or bit-and, it just means low 12 bits of status
> register is touch key's status.
> -> /* low 12 bits of status register is touch key's status */

/* Masks for touch and release triggers */

should be enough

> >
> > > +#define TOUCH_STATUS_MASK            0xfff
> > > +/* MPR121 have 12 electrodes */
> > > +#define MPR121_MAX_KEY_COUNT         12
> > > +
> > > +
> > > +/**
> > > + * @keycount: how many key maped
> >
> > typo: "keys" ?
> 
> ->  * @keycount: how many keys mapped

Do you mean "keys" or "capacitance sensing inputs"?
> >
> > > + * @vdd_uv: voltage of vdd supply the chip in uV
> >
> > - typo
> > - uppercase for vdd
> 
> ->  * @vdd_uv: VDD voltage in uV
> 
> >
> > > + * @matrix: maxtrix of keys
> > > + * @wakeup: can key wake up system.
> >
> > typo
> 
> ->  * @wakeup: can wake up system or not

"enable wakeup"

Just out of curiosity, why does platform data need to enable/disable
wakeup?

> >
> > > + */
> > > +struct mpr121_platform_data {
> > > +     u16 keycount;
> > > +     u16 *matrix;
> > > +     int wakeup;
> > > +     int vdd_uv;
> > > +};

Please make the order of comments equal to this struct.

> > > +
> > > +#endif
> > > --
> > > 1.7.1
> > >
> > > --
> > > 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.htmln Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
> 
> :-(
> sorry again for my horrible grammar.
> 
> Jiejing



--
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
JieJing.Zhang April 13, 2011, 1:23 a.m. UTC | #6
Hi Dmitry,

2011/4/12 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Jiejing,
>
> On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
>> From: Zhang Jiejing <jiejing.zhang@freescale.com>
>>
>> This touchkey driver is based on Freescale mpr121 capacitive
>> touch sensor controller.
>>
>> It can support up to 12 keys, it use i2c interface.
>>
>> The product infomation(data sheet, application notes) can
>> be found under this link:
>> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121
>>
>
> MOstly reasonable. In addition to Christoph's comments:
>
>> +
>> +static struct mpr121_init_register init_reg_table[] = {
>
> const? And also __devinitconst?
will do.
>
>> +     {MHD_RISING_ADDR,       0x1},
>> +     {NHD_RISING_ADDR,       0x1},
>> +     {MHD_FALLING_ADDR,      0x1},
>> +     {NHD_FALLING_ADDR,      0x1},
>> +     {NCL_FALLING_ADDR,      0xff},
>> +     {FDL_FALLING_ADDR,      0x02},
>> +     {FILTER_CONF_ADDR,      0x04},
>> +     {AFE_CONF_ADDR,         0x0b},
>> +     {AUTO_CONFIG_CTRL_ADDR, 0x0b},
>> +};
>> +
>> +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
>> +{
>> +     struct mpr121_touchkey_data *data = dev_id;
>> +     struct i2c_client *client = data->client;
>> +     struct input_dev *input = data->input_dev;
>> +     unsigned int key_num, pressed;
>> +     int reg;
>> +
>> +     reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
>> +     if (reg < 0) {
>> +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
>> +             goto out;
>> +     }
>> +
>> +     reg <<= 8;
>> +     reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
>> +     if (reg < 0) {
>> +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
>> +             goto out;
>> +     }
>> +
>> +     reg &= TOUCH_STATUS_MASK;
>> +     /* use old press bit to figure out which bit changed */
>> +     key_num = ffs(reg ^ data->statusbits) - 1;
>> +     /* use the bit check the press status */
>> +     pressed = (reg & (1 << (key_num))) >> key_num;
>
> No need to shift by key_num, input_report_key normalizes to 'bool'.
>
yes, will change.
>> +     data->statusbits = reg;
>> +     data->key_val = data->keycodes[key_num];
>> +
>> +     input_event(input, EV_MSC, MSC_SCAN, data->key_val);
>
> sScancode is not data->key_val as it is KEY_XXX value but rather key_num.
>
thanks for point out, will change to key_num.

>> +     input_report_key(input, data->key_val, pressed);
>> +     input_sync(input);
>> +
>> +     dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
>> +             pressed ? "pressed" : "released");
>> +
>> +out:
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int mpr121_phys_init(
>
> __devinit
will do.
>
>                            struct mpr121_platform_data *pdata,
>
> pdata should be const.
>
>> +                         struct mpr121_touchkey_data *data,
>> +                         struct i2c_client *client)
>> +{
>> +     struct mpr121_init_register *reg;
>> +     unsigned char usl, lsl, tl;
>> +     int i, t, vdd, ret;
>> +
>> +     /* setup touch/release threshold for ele0-ele11 */
>> +     for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
>> +             t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
>> +             ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
>> +             if (ret < 0)
>> +                     goto err_i2c_write;
>> +             ret = i2c_smbus_write_byte_data(client, t + 1,
>> +                                             RELEASE_THRESHOLD);
>> +             if (ret < 0)
>> +                     goto err_i2c_write;
>> +     }
>> +     /* setup init register */
>> +     for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
>> +             reg = &init_reg_table[i];
>> +             ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
>> +             if (ret < 0)
>> +                     goto err_i2c_write;
>> +     }
>> +     /* setup auto-register by vdd,the formula please ref: AN3889
>> +      * These register *must* set acrodding your VDD voltage supply
>> +      * to the chip*/
>> +     vdd = pdata->vdd_uv / 1000;
>> +     usl = ((vdd - 700) * 256) / vdd;
>> +     lsl = (usl * 65) / 100;
>> +     tl = (usl * 90) / 100;
>> +     ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
>> +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
>> +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
>> +     ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
>> +                                     data->keycount);
>> +     if (ret != 0)
>> +             goto err_i2c_write;
>> +
>> +     dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
>> +              data->keycount);
>
> Does it have to be dev_info? dev_dbg maybe?
>
will change to dev_dbg.
>> +
>> +     return 0;
>> +
>> +err_i2c_write:
>> +     dev_err(&client->dev, "i2c write error: %d\n", ret);
>> +     return ret;
>> +}
>> +
>> +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
>> +                                     const struct i2c_device_id *id)
>> +{
>> +     struct mpr121_platform_data *pdata;
>
> const.
will do.
>
>> +     struct mpr121_touchkey_data *data;
>> +     struct input_dev *input_dev;
>> +     int error;
>> +     int i;
>> +
>> +     pdata = client->dev.platform_data;
>> +     if (!pdata) {
>> +             dev_err(&client->dev, "no platform data defined\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!client->irq) {
>> +             dev_err(&client->dev, "The irq number should not be zero\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
>> +     input_dev = input_allocate_device();
>> +     if (!data || !input_dev) {
>> +             dev_err(&client->dev, "Falied to allocate memory\n");
>> +             error = -ENOMEM;
>> +             goto err_free_mem;
>> +     }
>> +
>> +     data->client = client;
>> +     data->input_dev = input_dev;
>> +     data->keycount = pdata->keycount;
>> +
>> +     if (data->keycount > MPR121_MAX_KEY_COUNT) {
>> +             dev_err(&client->dev, "Too many key defined\n");
>> +             error = -EINVAL;
>> +             goto err_free_mem;
>> +     }
>> +
>> +     error = mpr121_phys_init(pdata, data, client);
>> +     if (error) {
>> +             dev_err(&client->dev, "Failed to init register\n");
>> +             goto err_free_mem;
>> +     }
>> +
>> +     i2c_set_clientdata(client, data);
>
> You should do this at the very end, before returning success (0).
> Actually, you do do this, so just remove this instance.
>
will remove.
>> +
>> +     input_dev->name = "FSL MPR121 Touchkey";
>> +     input_dev->id.bustype = BUS_I2C;
>> +     input_dev->dev.parent = &client->dev;
>> +     input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
>> +     input_dev->keycode = pdata->matrix;
>
> No, you are assigning pointer to the platform data, instead of pointer
> to the data in device structure. That is why I'd rather you renamed
> 'struct mpr121_touchkey_data' to 'struct mpr121_touchkey' and 'data' to
> 'mpr121' so as to avoid confusion with platform data.
>
I've changed all struct mpr121_touchkey_data to struct mpr121_touchkey.
and change the "data" to mpr121.

I'm truly confuse with platform data.

>> +     input_dev->keycodesize = sizeof(pdata->matrix[0]);
>> +     input_dev->keycodemax = data->keycount;
>> +
>> +     for (i = 0; i < input_dev->keycodemax; i++) {
>> +             __set_bit(pdata->matrix[i], input_dev->keybit);
>> +             data->keycodes[i] = pdata->matrix[i];
>> +     }
>> +
>> +     input_set_capability(input_dev, EV_MSC, MSC_SCAN);
>> +     input_set_drvdata(input_dev, data);
>> +
>> +     error = request_threaded_irq(client->irq, NULL,
>> +                                  mpr_touchkey_interrupt,
>> +                                  IRQF_TRIGGER_FALLING,
>> +                                  client->dev.driver->name, data);
>> +     if (error) {
>> +             dev_err(&client->dev, "Failed to register interrupt\n");
>> +             goto err_free_mem;
>> +     }
>> +
>> +     error = input_register_device(input_dev);
>> +     if (error)
>> +             goto err_free_irq;
>> +     i2c_set_clientdata(client, data);
>> +     device_init_wakeup(&client->dev, pdata->wakeup);
>> +     dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
>
> No need for having this message. You can see the device bound to the
> driver in sysfs. The boot is noisy enough.
ok, will do.
>
>> +     return 0;
>> +
>> +err_free_irq:
>> +     free_irq(client->irq, data);
>> +err_free_mem:
>> +     input_free_device(input_dev);
>> +     kfree(data);
>> +     return error;
>> +}
>> +
>> +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
>> +{
>> +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
>> +
>> +     free_irq(client->irq, data);
>> +     input_unregister_device(data->input_dev);
>> +     kfree(data);
>> +
>> +     return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int mpr_suspend(struct device *dev)
>> +{
>> +     struct i2c_client *client = to_i2c_client(dev);
>> +
>> +     if (device_may_wakeup(&client->dev))
>> +             enable_irq_wake(client->irq);
>> +
>> +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mpr_resume(struct device *dev)
>> +{
>> +     struct i2c_client *client = to_i2c_client(dev);
>> +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
>> +
>> +     if (device_may_wakeup(&client->dev))
>> +             disable_irq_wake(client->irq);
>> +
>> +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
>> +
>> +     return 0;
>> +}
>> +#endif
>> +
>> +static const struct i2c_device_id mpr121_id[] = {
>> +     {"mpr121_touchkey", 0},
>> +     { }
>> +};
>> +
>> +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
>> +
>> +static struct i2c_driver mpr_touchkey_driver = {
>> +     .driver = {
>> +             .name   = "mpr121",
>> +             .owner  = THIS_MODULE,
>> +             .pm     = &mpr121_touchkey_pm_ops,
>> +     },
>> +     .id_table       = mpr121_id,
>> +     .probe          = mpr_touchkey_probe,
>> +     .remove         = __devexit_p(mpr_touchkey_remove),
>> +};
>> +
>> +static int __init mpr_touchkey_init(void)
>> +{
>> +     return i2c_add_driver(&mpr_touchkey_driver);
>> +}
>> +
>> +static void __exit mpr_touchkey_exit(void)
>> +{
>> +     i2c_del_driver(&mpr_touchkey_driver);
>> +}
>> +
>> +module_init(mpr_touchkey_init);
>> +module_exit(mpr_touchkey_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
>> +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
>> diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
>> new file mode 100644
>> index 0000000..52d6e33
>> --- /dev/null
>> +++ b/include/linux/i2c/mpr.h
>> @@ -0,0 +1,64 @@
>> +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
>> + * controllor */
>> +
>> +#ifndef MPR_H
>> +#define MPR_H
>> +
>> +/* Register definitions */
>> +#define ELE_TOUCH_STATUS_0_ADDR      0x0
>> +#define ELE_TOUCH_STATUS_1_ADDR      0X1
>> +#define MHD_RISING_ADDR              0x2b
>> +#define NHD_RISING_ADDR              0x2c
>> +#define NCL_RISING_ADDR              0x2d
>> +#define FDL_RISING_ADDR              0x2e
>> +#define MHD_FALLING_ADDR     0x2f
>> +#define NHD_FALLING_ADDR     0x30
>> +#define NCL_FALLING_ADDR     0x31
>> +#define FDL_FALLING_ADDR     0x32
>> +#define ELE0_TOUCH_THRESHOLD_ADDR    0x41
>> +#define ELE0_RELEASE_THRESHOLD_ADDR  0x42
>> +/* ELE0...ELE11's threshold will set in a loop */
>> +#define AFE_CONF_ADDR                        0x5c
>> +#define FILTER_CONF_ADDR             0x5d
>> +
>> +/* ELECTRODE_CONF: this register is most important register, it
>> + * control how many of electrode is enabled. If you set this register
>> + * to 0x0, it make the sensor going to suspend mode. Other value(low
>> + * bit is non-zero) will make the sensor into Run mode.  This register
>> + * should be write at last.
>
> Hmm, most of the comments require proper grammar. If you need help with
> it - let the list know, we'll try to help.
>
>> + */
>> +#define ELECTRODE_CONF_ADDR          0x5e
>> +#define AUTO_CONFIG_CTRL_ADDR                0x7b
>> +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
>> + * register is related to VDD supplied on your board, the value of
>> + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
>> + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
>> + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
>> + * is 90% of USL  */
>> +#define AUTO_CONFIG_USL_ADDR         0x7d
>> +#define AUTO_CONFIG_LSL_ADDR         0x7e
>> +#define AUTO_CONFIG_TL_ADDR          0x7f
>> +
>> +/* Threshold of touch/release trigger */
>> +#define TOUCH_THRESHOLD                      0x0f
>> +#define RELEASE_THRESHOLD            0x0a
>> +/* Mask Button bits of STATUS_0 & STATUS_1 register */
>> +#define TOUCH_STATUS_MASK            0xfff
>> +/* MPR121 have 12 electrodes */
>> +#define MPR121_MAX_KEY_COUNT         12
>> +
>> +
>> +/**
>> + * @keycount: how many key maped
>> + * @vdd_uv: voltage of vdd supply the chip in uV
>> + * @matrix: maxtrix of keys
>> + * @wakeup: can key wake up system.
>> + */
>> +struct mpr121_platform_data {
>> +     u16 keycount;
>> +     u16 *matrix;
>
> If this device is indeed organized as a matrix (lookslike this) maybe
> you should look at definitions provided by
> include/linux/input/matrix_keypad.h

OK, will change platform data to use matrix_keypad_data structure, I
will convert them into an array into struct mpr121_touchkey: u16
keycodes[12]. The reason is it's will more similar to controller's
register map.

>
>> +     int wakeup;
>
> bool?
wiil do.
>
>> +     int vdd_uv;
>> +};
>> +
>> +#endif
>
> Thanks.
>
> --
> Dmitry
>
--
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
JieJing.Zhang April 13, 2011, 1:42 a.m. UTC | #7
Hi Christonph,

Thanks a lot for your input, I will use them, they are better.:-)

2011/4/12 Christoph Fritz <chf.fritz@googlemail.com>:
> On Tue, 2011-04-12 at 19:41 +0800, Jiejing.Zhang wrote:
>> Hi Christoph,
>>
>> I'm sorry for my poor English grammar.
>
> I suppose the list can help. Here is my input:
>
>> I'll try to fix these typo, If still have error, please point me out.
>>
>> 2011/4/12 Christoph Fritz <chf.fritz@googlemail.com>
>> >
>> > Hi Zhang,
>> >
>> >  this is just a superficial review of your driver.
>> >
>> > thanks,
>> >  Christoph
>> >
>> > On Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
>> > > From: Zhang Jiejing <jiejing.zhang@freescale.com>
>> > >
>> > > This touchkey driver is based on Freescale mpr121 capacitive
>> > > touch sensor controller.
>> > >
>> > > It can support up to 12 keys, it use i2c interface.
>> >
>> > typo
>>
>> -> It can supports up to 12 electrodes, and using i2c interface.
>
> This patch adds basic support for Freescale MPR121 capacitive touch
> sensor.
> It's an i2c controller with up to 12 capacitance sensing inputs.
>
>> >
>> > >
>> > > The product infomation(data sheet, application notes) can
>> > > be found under this link:
>> > > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MPR121
>
> Product information (data sheet, application notes) can be found here:
>
>> > >
>> > > Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
>> > > ---
>> > >  drivers/input/keyboard/Kconfig  |   12 ++
>> > >  drivers/input/keyboard/Makefile |    1 +
>> > >  drivers/input/keyboard/mpr121.c |  301 +++++++++++++++++++++++++++++++++++++++
>> > >  include/linux/i2c/mpr.h         |   64 ++++++++
>> > >  4 files changed, 378 insertions(+), 0 deletions(-)
>> > >  create mode 100644 drivers/input/keyboard/mpr121.c
>> > >  create mode 100644 include/linux/i2c/mpr.h
>> > >
>> > > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> > > index b16bed0..0666e1e 100644
>> > > --- a/drivers/input/keyboard/Kconfig
>> > > +++ b/drivers/input/keyboard/Kconfig
>> > > @@ -520,6 +520,18 @@ config KEYBOARD_XTKBD
>> > >         To compile this driver as a module, choose M here: the
>> > >         module will be called xtkbd.
>> > >
>> > > +config KEYBOARD_MPR121
>> > > +     tristate "Freescale MPR121 Touchkey Driver"
>> > > +     depends on I2C
>> > > +     help
>> > > +       Say Y here if you have the  touchkey Freescale mpr121 capacitive
>> >
>> > typo: two spaces
>>
>> will fix.
>> >
>> > > +       touch sensor controller in your system.
>> > > +
>> > > +       If unsure, say N.
>> > > +
>> > > +       To compile this driver as a module, choose M here
>> > > +endif
>> > > +
>> > >  config KEYBOARD_W90P910
>> > >       tristate "W90P910 Matrix Keypad support"
>> > >       depends on ARCH_W90X900
>> >
>> > - add: "To compile this driver as a module, choose M here: the
>> >  module will be called mpr121."
>> > - "endif" is wrong here, it breaks 'make config'
>> > - maybe a more alphabetic approach in this list of drivers would
>> >  be nice
>>
>> yes. I will.
>> >
>> > > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> > > index 878e6c2..b110ca8 100644
>> > > --- a/drivers/input/keyboard/Makefile
>> > > +++ b/drivers/input/keyboard/Makefile
>> > > @@ -47,4 +47,5 @@ obj-$(CONFIG_KEYBOARD_TEGRA)                += tegra-kbc.o
>> > >  obj-$(CONFIG_KEYBOARD_TNETV107X)     += tnetv107x-keypad.o
>> > >  obj-$(CONFIG_KEYBOARD_TWL4030)               += twl4030_keypad.o
>> > >  obj-$(CONFIG_KEYBOARD_XTKBD)         += xtkbd.o
>> > > +obj-$(CONFIG_KEYBOARD_MPR121)                += mpr121.o
>> > >  obj-$(CONFIG_KEYBOARD_W90P910)               += w90p910_keypad.o
>> >
>> > for better examination, please mind alphabetic order
>>
>> will do.
>> >
>> > > diff --git a/drivers/input/keyboard/mpr121.c b/drivers/input/keyboard/mpr121.c
>> > > new file mode 100644
>> > > index 0000000..ee149a8
>> > > --- /dev/null
>> > > +++ b/drivers/input/keyboard/mpr121.c
>> > > @@ -0,0 +1,301 @@
>> > > +/*
>> > > + * Touchkey driver for Freescale MPR121 Controllor
>> > > + *
>> > > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
>> > > + * Author: Zhang Jiejing <jiejing.zhang@freescale.com>
>> > > + *
>> > > + * Based on mcs_touchkey.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.
>> > > + *
>> > > + */
>> > > +
>> > > +#include <linux/module.h>
>> > > +#include <linux/init.h>
>> > > +#include <linux/i2c.h>
>> > > +#include <linux/i2c/mpr.h>
>> > > +#include <linux/interrupt.h>
>> > > +#include <linux/input.h>
>> > > +#include <linux/irq.h>
>> >
>> > do not use irq.h directly
>>
>> ok.
>> >
>> > > +#include <linux/slab.h>
>> > > +#include <linux/delay.h>
>> > > +#include <linux/bitops.h>
>> > > +
>> > > +struct mpr121_touchkey_data {
>> > > +     struct i2c_client       *client;
>> > > +     struct input_dev        *input_dev;
>> > > +     unsigned int            key_val;
>> > > +     int                     statusbits;
>> > > +     int                     keycount;
>> > > +     u16                     keycodes[MPR121_MAX_KEY_COUNT];
>> > > +};
>> > > +
>> > > +struct mpr121_init_register {
>> > > +     int addr;
>> > > +     u8 val;
>> > > +};
>> > > +
>> > > +static struct mpr121_init_register init_reg_table[] = {
>> > > +     {MHD_RISING_ADDR,       0x1},
>> > > +     {NHD_RISING_ADDR,       0x1},
>> > > +     {MHD_FALLING_ADDR,      0x1},
>> > > +     {NHD_FALLING_ADDR,      0x1},
>> > > +     {NCL_FALLING_ADDR,      0xff},
>> > > +     {FDL_FALLING_ADDR,      0x02},
>> > > +     {FILTER_CONF_ADDR,      0x04},
>> > > +     {AFE_CONF_ADDR,         0x0b},
>> > > +     {AUTO_CONFIG_CTRL_ADDR, 0x0b},
>> > > +};
>> > > +
>> > > +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
>> > > +{
>> > > +     struct mpr121_touchkey_data *data = dev_id;
>> > > +     struct i2c_client *client = data->client;
>> > > +     struct input_dev *input = data->input_dev;
>> > > +     unsigned int key_num, pressed;
>> > > +     int reg;
>> > > +
>> > > +     reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
>> > > +     if (reg < 0) {
>> > > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
>> > > +             goto out;
>> > > +     }
>> > > +
>> > > +     reg <<= 8;
>> > > +     reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
>> > > +     if (reg < 0) {
>> > > +             dev_err(&client->dev, "i2c read error [%d]\n", reg);
>> > > +             goto out;
>> > > +     }
>> > > +
>> > > +     reg &= TOUCH_STATUS_MASK;
>> > > +     /* use old press bit to figure out which bit changed */
>> > > +     key_num = ffs(reg ^ data->statusbits) - 1;
>> > > +     /* use the bit check the press status */
>> >
>> > typo
>>
>> this comment will remove.
>> >
>> > > +     pressed = (reg & (1 << (key_num))) >> key_num;
>> > > +     data->statusbits = reg;
>> > > +     data->key_val = data->keycodes[key_num];
>> > > +
>> > > +     input_event(input, EV_MSC, MSC_SCAN, data->key_val);
>> > > +     input_report_key(input, data->key_val, pressed);
>> > > +     input_sync(input);
>> > > +
>> > > +     dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
>> > > +             pressed ? "pressed" : "released");
>> > > +
>> > > +out:
>> > > +     return IRQ_HANDLED;
>> > > +}
>> > > +
>> > > +static int mpr121_phys_init(struct mpr121_platform_data *pdata,
>> > > +                         struct mpr121_touchkey_data *data,
>> > > +                         struct i2c_client *client)
>> > > +{
>> > > +     struct mpr121_init_register *reg;
>> > > +     unsigned char usl, lsl, tl;
>> > > +     int i, t, vdd, ret;
>> > > +
>> > > +     /* setup touch/release threshold for ele0-ele11 */
>> > > +     for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
>> > > +             t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
>> > > +             ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
>> > > +             if (ret < 0)
>> > > +                     goto err_i2c_write;
>> > > +             ret = i2c_smbus_write_byte_data(client, t + 1,
>> > > +                                             RELEASE_THRESHOLD);
>> > > +             if (ret < 0)
>> > > +                     goto err_i2c_write;
>> > > +     }
>> > > +     /* setup init register */
>> > > +     for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
>> > > +             reg = &init_reg_table[i];
>> > > +             ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
>> > > +             if (ret < 0)
>> > > +                     goto err_i2c_write;
>> > > +     }
>> > > +     /* setup auto-register by vdd,the formula please ref: AN3889
>> > > +      * These register *must* set acrodding your VDD voltage supply
>> > > +      * to the chip*/
>> >
>> > typo
>>
>> is this right?
>>     /* setup Auto Config Registers by VDD.
>>      * The formula was written in AN3889.
>>      * These registers *must* be set by VDD voltage.
>>      */
>
> Capacitance on sensing input varies and needs to be compensated. The
> internal MPR121-auto-configuration can do this if it's registers are set
> properly (based on pdata->vdd_uv).
>
>> >
>> > > +     vdd = pdata->vdd_uv / 1000;
>> > > +     usl = ((vdd - 700) * 256) / vdd;
>> > > +     lsl = (usl * 65) / 100;
>> > > +     tl = (usl * 90) / 100;
>> > > +     ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
>> > > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
>> > > +     ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
>> > > +     ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
>> > > +                                     data->keycount);
>> > > +     if (ret != 0)
>> > > +             goto err_i2c_write;
>> > > +
>> > > +     dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
>> >
>> > typo ?
>>
>> -> dev_dbg(&client->dev, "mpr121: setup with %x electrodes.\n",
>
> Do you mean keys or "capacitance sensing inputs"?
>
>> >
>> > > +              data->keycount);
>> > > +
>> > > +     return 0;
>> > > +
>> > > +err_i2c_write:
>> > > +     dev_err(&client->dev, "i2c write error: %d\n", ret);
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +static int __devinit mpr_touchkey_probe(struct i2c_client *client,
>> > > +                                     const struct i2c_device_id *id)
>> > > +{
>> > > +     struct mpr121_platform_data *pdata;
>> > > +     struct mpr121_touchkey_data *data;
>> > > +     struct input_dev *input_dev;
>> > > +     int error;
>> > > +     int i;
>> > > +
>> > > +     pdata = client->dev.platform_data;
>> > > +     if (!pdata) {
>> > > +             dev_err(&client->dev, "no platform data defined\n");
>> > > +             return -EINVAL;
>> > > +     }
>> > > +
>> > > +     if (!client->irq) {
>> > > +             dev_err(&client->dev, "The irq number should not be zero\n");
>> > > +             return -EINVAL;
>> > > +     }
>> > > +
>> > > +     data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
>> > > +     input_dev = input_allocate_device();
>> > > +     if (!data || !input_dev) {
>> > > +             dev_err(&client->dev, "Falied to allocate memory\n");
>> > > +             error = -ENOMEM;
>> > > +             goto err_free_mem;
>> > > +     }
>> > > +
>> > > +     data->client = client;
>> > > +     data->input_dev = input_dev;
>> > > +     data->keycount = pdata->keycount;
>> > > +
>> > > +     if (data->keycount > MPR121_MAX_KEY_COUNT) {
>> > > +             dev_err(&client->dev, "Too many key defined\n");
>> >
>> > typo
>>
>> -> dev_err(&client->dev, "too much keys defined\n");
>
> "too many keys defined"
>
>> >
>> > > +             error = -EINVAL;
>> > > +             goto err_free_mem;
>> > > +     }
>> > > +
>> > > +     error = mpr121_phys_init(pdata, data, client);
>> > > +     if (error) {
>> > > +             dev_err(&client->dev, "Failed to init register\n");
>> > > +             goto err_free_mem;
>> > > +     }
>> > > +
>> > > +     i2c_set_clientdata(client, data);
>> > > +
>> > > +     input_dev->name = "FSL MPR121 Touchkey";
>> > > +     input_dev->id.bustype = BUS_I2C;
>> > > +     input_dev->dev.parent = &client->dev;
>> > > +     input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
>> > > +     input_dev->keycode = pdata->matrix;
>> > > +     input_dev->keycodesize = sizeof(pdata->matrix[0]);
>> > > +     input_dev->keycodemax = data->keycount;
>> > > +
>> > > +     for (i = 0; i < input_dev->keycodemax; i++) {
>> > > +             __set_bit(pdata->matrix[i], input_dev->keybit);
>> > > +             data->keycodes[i] = pdata->matrix[i];
>> > > +     }
>> > > +
>> > > +     input_set_capability(input_dev, EV_MSC, MSC_SCAN);
>> > > +     input_set_drvdata(input_dev, data);
>> > > +
>> > > +     error = request_threaded_irq(client->irq, NULL,
>> > > +                                  mpr_touchkey_interrupt,
>> > > +                                  IRQF_TRIGGER_FALLING,
>> > > +                                  client->dev.driver->name, data);
>> > > +     if (error) {
>> > > +             dev_err(&client->dev, "Failed to register interrupt\n");
>> > > +             goto err_free_mem;
>> > > +     }
>> > > +
>> > > +     error = input_register_device(input_dev);
>> > > +     if (error)
>> > > +             goto err_free_irq;
>> > > +     i2c_set_clientdata(client, data);
>> > > +     device_init_wakeup(&client->dev, pdata->wakeup);
>> > > +     dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
>> > > +     return 0;
>> > > +
>> > > +err_free_irq:
>> > > +     free_irq(client->irq, data);
>> > > +err_free_mem:
>> > > +     input_free_device(input_dev);
>> > > +     kfree(data);
>> > > +     return error;
>> > > +}
>> > > +
>> > > +static int __devexit mpr_touchkey_remove(struct i2c_client *client)
>> > > +{
>> > > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
>> > > +
>> > > +     free_irq(client->irq, data);
>> > > +     input_unregister_device(data->input_dev);
>> > > +     kfree(data);
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +#ifdef CONFIG_PM_SLEEP
>> > > +static int mpr_suspend(struct device *dev)
>> > > +{
>> > > +     struct i2c_client *client = to_i2c_client(dev);
>> > > +
>> > > +     if (device_may_wakeup(&client->dev))
>> > > +             enable_irq_wake(client->irq);
>> > > +
>> > > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static int mpr_resume(struct device *dev)
>> > > +{
>> > > +     struct i2c_client *client = to_i2c_client(dev);
>> > > +     struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
>> > > +
>> > > +     if (device_may_wakeup(&client->dev))
>> > > +             disable_irq_wake(client->irq);
>> > > +
>> > > +     i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +#endif
>> > > +
>> > > +static const struct i2c_device_id mpr121_id[] = {
>> > > +     {"mpr121_touchkey", 0},
>> > > +     { }
>> > > +};
>> > > +
>> > > +static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
>> > > +
>> > > +static struct i2c_driver mpr_touchkey_driver = {
>> > > +     .driver = {
>> > > +             .name   = "mpr121",
>> > > +             .owner  = THIS_MODULE,
>> > > +             .pm     = &mpr121_touchkey_pm_ops,
>> >
>> > I would add a ifdef CONFIG_PM_SLEEP
>> >
>> > > +     },
>> > > +     .id_table       = mpr121_id,
>> > > +     .probe          = mpr_touchkey_probe,
>> > > +     .remove         = __devexit_p(mpr_touchkey_remove),
>> > > +};
>> > > +
>> > > +static int __init mpr_touchkey_init(void)
>> > > +{
>> > > +     return i2c_add_driver(&mpr_touchkey_driver);
>> > > +}
>> > > +
>> > > +static void __exit mpr_touchkey_exit(void)
>> > > +{
>> > > +     i2c_del_driver(&mpr_touchkey_driver);
>> > > +}
>> > > +
>> > > +module_init(mpr_touchkey_init);
>> > > +module_exit(mpr_touchkey_exit);
>> > > +
>> > > +MODULE_LICENSE("GPL");
>> > > +MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
>> > > +MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
>> > > diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
>> > > new file mode 100644
>> > > index 0000000..52d6e33
>> > > --- /dev/null
>> > > +++ b/include/linux/i2c/mpr.h
>> > > @@ -0,0 +1,64 @@
>> > > +/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
>> >
>> > no filename please
>>
>> will do.
>> >
>> > > + * controllor */
>> > > +
>> > > +#ifndef MPR_H
>> > > +#define MPR_H
>> > > +
>> > > +/* Register definitions */
>> > > +#define ELE_TOUCH_STATUS_0_ADDR      0x0
>> > > +#define ELE_TOUCH_STATUS_1_ADDR      0X1
>> > > +#define MHD_RISING_ADDR              0x2b
>> > > +#define NHD_RISING_ADDR              0x2c
>> > > +#define NCL_RISING_ADDR              0x2d
>> > > +#define FDL_RISING_ADDR              0x2e
>> > > +#define MHD_FALLING_ADDR     0x2f
>> > > +#define NHD_FALLING_ADDR     0x30
>> > > +#define NCL_FALLING_ADDR     0x31
>> > > +#define FDL_FALLING_ADDR     0x32
>> > > +#define ELE0_TOUCH_THRESHOLD_ADDR    0x41
>> > > +#define ELE0_RELEASE_THRESHOLD_ADDR  0x42
>> > > +/* ELE0...ELE11's threshold will set in a loop */
>> >
>> > typo
>>
>> -> /* ELE0...ELE11's threshold will be set in a loop */
>> >
>> > > +#define AFE_CONF_ADDR                        0x5c
>> > > +#define FILTER_CONF_ADDR             0x5d
>> > > +
>> > > +/* ELECTRODE_CONF: this register is most important register, it
>> > > + * control how many of electrode is enabled. If you set this register
>> > > + * to 0x0, it make the sensor going to suspend mode. Other value(low
>> > > + * bit is non-zero) will make the sensor into Run mode.  This register
>> > > + * should be write at last.
>> >
>> > typo
>>
>> ->  * should be written in the end.
>
> ELECTRODE_CONF: This register mainly configures the number of enabled
> capacitance sensing inputs and its run/suspend mode.
>
>>
>> >
>> > > + */
>> > > +#define ELECTRODE_CONF_ADDR          0x5e
>> > > +#define AUTO_CONFIG_CTRL_ADDR                0x7b
>> > > +/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
>> > > + * register is related to VDD supplied on your board, the value of
>> > > + * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
>> >
>> > typo
>>
>> ->  * this register is calculated by EQ: `((VDD-0.7)/VDD) * 256`.
>
> It's in the source how it gets calculated, so I don't think you need to
> document it here.
>
>>
>> >
>> > > + * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
>> > > + * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
>> > > + * is 90% of USL  */
>> >
>> > typo
>>
>> ->  * USL AUTO_CONFIG_TL: The traget level of auto baseline search, it is
>> ->  * 90% of USL */
>
> just remove 'is', typo in target
>
>>
>> >
>> > > +#define AUTO_CONFIG_USL_ADDR         0x7d
>> > > +#define AUTO_CONFIG_LSL_ADDR         0x7e
>> > > +#define AUTO_CONFIG_TL_ADDR          0x7f
>
> Why do they need to be in the header file?
> Couldn't be a lot of defines get moved into the actual driver?
>
>> > > +
>> > > +/* Threshold of touch/release trigger */
>> > > +#define TOUCH_THRESHOLD                      0x0f
>> > > +#define RELEASE_THRESHOLD            0x0a
>> > > +/* Mask Button bits of STATUS_0 & STATUS_1 register */
>> >
>> > and or bit-and?
>>
>> don't need and or bit-and, it just means low 12 bits of status
>> register is touch key's status.
>> -> /* low 12 bits of status register is touch key's status */
>
> /* Masks for touch and release triggers */
>
> should be enough
>
>> >
>> > > +#define TOUCH_STATUS_MASK            0xfff
>> > > +/* MPR121 have 12 electrodes */
>> > > +#define MPR121_MAX_KEY_COUNT         12
>> > > +
>> > > +
>> > > +/**
>> > > + * @keycount: how many key maped
>> >
>> > typo: "keys" ?
>>
>> ->  * @keycount: how many keys mapped
>
> Do you mean "keys" or "capacitance sensing inputs"?
I will change keys, and keycount to a const struct matrix_keymap_data *matrix;
to resue the struct already in kernel.
the comments will change to @matrix: pointer to &matrix_keymap_data.
>> >
>> > > + * @vdd_uv: voltage of vdd supply the chip in uV
>> >
>> > - typo
>> > - uppercase for vdd
>>
>> ->  * @vdd_uv: VDD voltage in uV
>>
>> >
>> > > + * @matrix: maxtrix of keys
>> > > + * @wakeup: can key wake up system.
>> >
>> > typo
>>
>> ->  * @wakeup: can wake up system or not
>
> "enable wakeup"
>
> Just out of curiosity, why does platform data need to enable/disable
> wakeup?

It's should be configure the button as a wake-up source.

I think better defined in platform_data, like linux/input/gpio_keys.h
struct gpio_keys_button.

>
>> >
>> > > + */
>> > > +struct mpr121_platform_data {
>> > > +     u16 keycount;
>> > > +     u16 *matrix;
>> > > +     int wakeup;
>> > > +     int vdd_uv;
>> > > +};
>
> Please make the order of comments equal to this struct.
>
>> > > +
>> > > +#endif
>> > > --
>> > > 1.7.1
>> > >
>> > > --
>> > > 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.htmln Tue, Apr 12, 2011 at 01:02:17AM +0800, Jiejing Zhang wrote:
>>
>> :-(
>> sorry again for my horrible grammar.
>>
>> Jiejing
>
>
>
>
--
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
diff mbox

Patch

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b16bed0..0666e1e 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -520,6 +520,18 @@  config KEYBOARD_XTKBD
 	  To compile this driver as a module, choose M here: the
 	  module will be called xtkbd.
 
+config KEYBOARD_MPR121
+	tristate "Freescale MPR121 Touchkey Driver"
+	depends on I2C
+	help
+	  Say Y here if you have the  touchkey Freescale mpr121 capacitive
+	  touch sensor controller in your system.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here
+endif
+
 config KEYBOARD_W90P910
 	tristate "W90P910 Matrix Keypad support"
 	depends on ARCH_W90X900
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 878e6c2..b110ca8 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -47,4 +47,5 @@  obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
 obj-$(CONFIG_KEYBOARD_TNETV107X)	+= tnetv107x-keypad.o
 obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
 obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
+obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121.o
 obj-$(CONFIG_KEYBOARD_W90P910)		+= w90p910_keypad.o
diff --git a/drivers/input/keyboard/mpr121.c b/drivers/input/keyboard/mpr121.c
new file mode 100644
index 0000000..ee149a8
--- /dev/null
+++ b/drivers/input/keyboard/mpr121.c
@@ -0,0 +1,301 @@ 
+/*
+ * Touchkey driver for Freescale MPR121 Controllor
+ *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ * Author: Zhang Jiejing <jiejing.zhang@freescale.com>
+ *
+ * Based on mcs_touchkey.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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/i2c/mpr.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/bitops.h>
+
+struct mpr121_touchkey_data {
+	struct i2c_client	*client;
+	struct input_dev	*input_dev;
+	unsigned int		key_val;
+	int			statusbits;
+	int			keycount;
+	u16			keycodes[MPR121_MAX_KEY_COUNT];
+};
+
+struct mpr121_init_register {
+	int addr;
+	u8 val;
+};
+
+static struct mpr121_init_register init_reg_table[] = {
+	{MHD_RISING_ADDR,	0x1},
+	{NHD_RISING_ADDR,	0x1},
+	{MHD_FALLING_ADDR,	0x1},
+	{NHD_FALLING_ADDR,	0x1},
+	{NCL_FALLING_ADDR,	0xff},
+	{FDL_FALLING_ADDR,	0x02},
+	{FILTER_CONF_ADDR,	0x04},
+	{AFE_CONF_ADDR,		0x0b},
+	{AUTO_CONFIG_CTRL_ADDR, 0x0b},
+};
+
+static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
+{
+	struct mpr121_touchkey_data *data = dev_id;
+	struct i2c_client *client = data->client;
+	struct input_dev *input = data->input_dev;
+	unsigned int key_num, pressed;
+	int reg;
+
+	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
+	if (reg < 0) {
+		dev_err(&client->dev, "i2c read error [%d]\n", reg);
+		goto out;
+	}
+
+	reg <<= 8;
+	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
+	if (reg < 0) {
+		dev_err(&client->dev, "i2c read error [%d]\n", reg);
+		goto out;
+	}
+
+	reg &= TOUCH_STATUS_MASK;
+	/* use old press bit to figure out which bit changed */
+	key_num = ffs(reg ^ data->statusbits) - 1;
+	/* use the bit check the press status */
+	pressed = (reg & (1 << (key_num))) >> key_num;
+	data->statusbits = reg;
+	data->key_val = data->keycodes[key_num];
+
+	input_event(input, EV_MSC, MSC_SCAN, data->key_val);
+	input_report_key(input, data->key_val, pressed);
+	input_sync(input);
+
+	dev_dbg(&client->dev, "key %d %d %s\n", key_num, data->key_val,
+		pressed ? "pressed" : "released");
+
+out:
+	return IRQ_HANDLED;
+}
+
+static int mpr121_phys_init(struct mpr121_platform_data *pdata,
+			    struct mpr121_touchkey_data *data,
+			    struct i2c_client *client)
+{
+	struct mpr121_init_register *reg;
+	unsigned char usl, lsl, tl;
+	int i, t, vdd, ret;
+
+	/* setup touch/release threshold for ele0-ele11 */
+	for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
+		t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
+		ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
+		if (ret < 0)
+			goto err_i2c_write;
+		ret = i2c_smbus_write_byte_data(client, t + 1,
+						RELEASE_THRESHOLD);
+		if (ret < 0)
+			goto err_i2c_write;
+	}
+	/* setup init register */
+	for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
+		reg = &init_reg_table[i];
+		ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
+		if (ret < 0)
+			goto err_i2c_write;
+	}
+	/* setup auto-register by vdd,the formula please ref: AN3889
+	 * These register *must* set acrodding your VDD voltage supply
+	 * to the chip*/
+	vdd = pdata->vdd_uv / 1000;
+	usl = ((vdd - 700) * 256) / vdd;
+	lsl = (usl * 65) / 100;
+	tl = (usl * 90) / 100;
+	ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
+	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
+	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
+	ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
+					data->keycount);
+	if (ret != 0)
+		goto err_i2c_write;
+
+	dev_info(&client->dev, "mpr121: config as enable %x of electrode.\n",
+		 data->keycount);
+
+	return 0;
+
+err_i2c_write:
+	dev_err(&client->dev, "i2c write error: %d\n", ret);
+	return ret;
+}
+
+static int __devinit mpr_touchkey_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	struct mpr121_platform_data *pdata;
+	struct mpr121_touchkey_data *data;
+	struct input_dev *input_dev;
+	int error;
+	int i;
+
+	pdata = client->dev.platform_data;
+	if (!pdata) {
+		dev_err(&client->dev, "no platform data defined\n");
+		return -EINVAL;
+	}
+
+	if (!client->irq) {
+		dev_err(&client->dev, "The irq number should not be zero\n");
+		return -EINVAL;
+	}
+
+	data = kzalloc(sizeof(struct mpr121_touchkey_data), GFP_KERNEL);
+	input_dev = input_allocate_device();
+	if (!data || !input_dev) {
+		dev_err(&client->dev, "Falied to allocate memory\n");
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	data->client = client;
+	data->input_dev = input_dev;
+	data->keycount = pdata->keycount;
+
+	if (data->keycount > MPR121_MAX_KEY_COUNT) {
+		dev_err(&client->dev, "Too many key defined\n");
+		error = -EINVAL;
+		goto err_free_mem;
+	}
+
+	error = mpr121_phys_init(pdata, data, client);
+	if (error) {
+		dev_err(&client->dev, "Failed to init register\n");
+		goto err_free_mem;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	input_dev->name = "FSL MPR121 Touchkey";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = &client->dev;
+	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+	input_dev->keycode = pdata->matrix;
+	input_dev->keycodesize = sizeof(pdata->matrix[0]);
+	input_dev->keycodemax = data->keycount;
+
+	for (i = 0; i < input_dev->keycodemax; i++) {
+		__set_bit(pdata->matrix[i], input_dev->keybit);
+		data->keycodes[i] = pdata->matrix[i];
+	}
+
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+	input_set_drvdata(input_dev, data);
+
+	error = request_threaded_irq(client->irq, NULL,
+				     mpr_touchkey_interrupt,
+				     IRQF_TRIGGER_FALLING,
+				     client->dev.driver->name, data);
+	if (error) {
+		dev_err(&client->dev, "Failed to register interrupt\n");
+		goto err_free_mem;
+	}
+
+	error = input_register_device(input_dev);
+	if (error)
+		goto err_free_irq;
+	i2c_set_clientdata(client, data);
+	device_init_wakeup(&client->dev, pdata->wakeup);
+	dev_info(&client->dev, "Mpr121 touch keyboard init success.\n");
+	return 0;
+
+err_free_irq:
+	free_irq(client->irq, data);
+err_free_mem:
+	input_free_device(input_dev);
+	kfree(data);
+	return error;
+}
+
+static int __devexit mpr_touchkey_remove(struct i2c_client *client)
+{
+	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
+
+	free_irq(client->irq, data);
+	input_unregister_device(data->input_dev);
+	kfree(data);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mpr_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	if (device_may_wakeup(&client->dev))
+		enable_irq_wake(client->irq);
+
+	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
+
+	return 0;
+}
+
+static int mpr_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct mpr121_touchkey_data *data = i2c_get_clientdata(client);
+
+	if (device_may_wakeup(&client->dev))
+		disable_irq_wake(client->irq);
+
+	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, data->keycount);
+
+	return 0;
+}
+#endif
+
+static const struct i2c_device_id mpr121_id[] = {
+	{"mpr121_touchkey", 0},
+	{ }
+};
+
+static SIMPLE_DEV_PM_OPS(mpr121_touchkey_pm_ops, mpr_suspend, mpr_resume);
+
+static struct i2c_driver mpr_touchkey_driver = {
+	.driver = {
+		.name	= "mpr121",
+		.owner	= THIS_MODULE,
+		.pm	= &mpr121_touchkey_pm_ops,
+	},
+	.id_table	= mpr121_id,
+	.probe		= mpr_touchkey_probe,
+	.remove		= __devexit_p(mpr_touchkey_remove),
+};
+
+static int __init mpr_touchkey_init(void)
+{
+	return i2c_add_driver(&mpr_touchkey_driver);
+}
+
+static void __exit mpr_touchkey_exit(void)
+{
+	i2c_del_driver(&mpr_touchkey_driver);
+}
+
+module_init(mpr_touchkey_init);
+module_exit(mpr_touchkey_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Zhang Jiejing <jiejing.zhang@freescale.com>");
+MODULE_DESCRIPTION("Touch Key driver for FSL MPR121 Chip");
diff --git a/include/linux/i2c/mpr.h b/include/linux/i2c/mpr.h
new file mode 100644
index 0000000..52d6e33
--- /dev/null
+++ b/include/linux/i2c/mpr.h
@@ -0,0 +1,64 @@ 
+/* mpr.h - Header file for Freescale mpr121 capacitive touch sensor
+ * controllor */
+
+#ifndef MPR_H
+#define MPR_H
+
+/* Register definitions */
+#define ELE_TOUCH_STATUS_0_ADDR	0x0
+#define ELE_TOUCH_STATUS_1_ADDR	0X1
+#define MHD_RISING_ADDR		0x2b
+#define NHD_RISING_ADDR		0x2c
+#define NCL_RISING_ADDR		0x2d
+#define FDL_RISING_ADDR		0x2e
+#define MHD_FALLING_ADDR	0x2f
+#define NHD_FALLING_ADDR	0x30
+#define NCL_FALLING_ADDR	0x31
+#define FDL_FALLING_ADDR	0x32
+#define ELE0_TOUCH_THRESHOLD_ADDR	0x41
+#define ELE0_RELEASE_THRESHOLD_ADDR	0x42
+/* ELE0...ELE11's threshold will set in a loop */
+#define AFE_CONF_ADDR			0x5c
+#define FILTER_CONF_ADDR		0x5d
+
+/* ELECTRODE_CONF: this register is most important register, it
+ * control how many of electrode is enabled. If you set this register
+ * to 0x0, it make the sensor going to suspend mode. Other value(low
+ * bit is non-zero) will make the sensor into Run mode.  This register
+ * should be write at last.
+ */
+#define ELECTRODE_CONF_ADDR		0x5e
+#define AUTO_CONFIG_CTRL_ADDR		0x7b
+/* AUTO_CONFIG_USL: Upper Limit for auto baseline search, this
+ * register is related to VDD supplied on your board, the value of
+ * this register is calc by EQ: `((VDD-0.7)/VDD) * 256`.
+ * AUTO_CONFIG_LSL: Low Limit of auto baseline search. This is 65% of
+ * USL AUTO_CONFIG_TL: The Traget Level of auto baseline search, This
+ * is 90% of USL  */
+#define AUTO_CONFIG_USL_ADDR		0x7d
+#define AUTO_CONFIG_LSL_ADDR		0x7e
+#define AUTO_CONFIG_TL_ADDR		0x7f
+
+/* Threshold of touch/release trigger */
+#define TOUCH_THRESHOLD			0x0f
+#define RELEASE_THRESHOLD		0x0a
+/* Mask Button bits of STATUS_0 & STATUS_1 register */
+#define TOUCH_STATUS_MASK		0xfff
+/* MPR121 have 12 electrodes */
+#define MPR121_MAX_KEY_COUNT		12
+
+
+/**
+ * @keycount: how many key maped
+ * @vdd_uv: voltage of vdd supply the chip in uV
+ * @matrix: maxtrix of keys
+ * @wakeup: can key wake up system.
+ */
+struct mpr121_platform_data {
+	u16 keycount;
+	u16 *matrix;
+	int wakeup;
+	int vdd_uv;
+};
+
+#endif