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@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxx> > > > + * > > > + * 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@xxxxxxxxxxxxx>"); > > > +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@xxxxxxxxxxxxxxx > > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
