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

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

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