Re: [PATCH] input: add mpr121 capacitive touchkey driver

["Jiejing.Zhang " <kzjeef@gmail.com>]

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