Hi Dmitry, 2011/4/12 Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>: > Hi Jiejing, > > 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. >> >> 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@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 >> + * 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
