Atmel AT42QT2160 sensor chip input driver
diff mbox

Message ID 20090915042612.GB1132@core.coreip.homeip.net
State Accepted
Headers show

Commit Message

Dmitry Torokhov Sept. 15, 2009, 4:26 a.m. UTC
Hi Raphael,

On Thu, Aug 20, 2009 at 05:15:52PM -0300, Raphael Derosso Pereira wrote:
> From: Raphael Derosso Pereira <raphaelpereira@gmail.com>
> 
> Initial version of AT42QT2160 Atmel Sensor Chip support. This version only
> supports individual cells (no slide support yet). The code has been tested
> on proprietary development ARM board, but should work fine on other machines.
> 
> Signed-off-by: Raphael Derosso Pereira <raphaelpereira@gmail.com>

This version is much better, thank you very much for making the changes
I requested.

> +
> +#define QT2160_CMD_CHIPID    (00)
> +#define QT2160_CMD_CODEVER   (01)
> +#define QT2160_CMD_GSTAT     (02)
> +#define QT2160_CMD_KEYS3     (03)
> +#define QT2160_CMD_KEYS4     (04)
> +#define QT2160_CMD_SLIDE     (05)
> +#define QT2160_CMD_GPIOS     (06)
> +#define QT2160_CMD_SUBVER    (07)
> +#define QT2160_CMD_CALIBRATE (10)

I am a bit concerned about this chunk. The first 8 commands are written
as octal while the last (calibrate) as decimal. Is this intentional?

I also made a few more changes. Could you please trythe patch below and
if everything is still working I will apply to my tree.

Thanks!

Comments

Raphael Derosso Pereira Sept. 20, 2009, 1:03 p.m. UTC | #1
Hello,

I have made some other working on this code, so I'll sync with your
differences and send you the complete code on monday, as it is in my
office computer.

Best Regards,

2009/9/15 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Raphael,
>
> On Thu, Aug 20, 2009 at 05:15:52PM -0300, Raphael Derosso Pereira wrote:
>> From: Raphael Derosso Pereira <raphaelpereira@gmail.com>
>>
>> Initial version of AT42QT2160 Atmel Sensor Chip support. This version only
>> supports individual cells (no slide support yet). The code has been tested
>> on proprietary development ARM board, but should work fine on other machines.
>>
>> Signed-off-by: Raphael Derosso Pereira <raphaelpereira@gmail.com>
>
> This version is much better, thank you very much for making the changes
> I requested.
>
>> +
>> +#define QT2160_CMD_CHIPID    (00)
>> +#define QT2160_CMD_CODEVER   (01)
>> +#define QT2160_CMD_GSTAT     (02)
>> +#define QT2160_CMD_KEYS3     (03)
>> +#define QT2160_CMD_KEYS4     (04)
>> +#define QT2160_CMD_SLIDE     (05)
>> +#define QT2160_CMD_GPIOS     (06)
>> +#define QT2160_CMD_SUBVER    (07)
>> +#define QT2160_CMD_CALIBRATE (10)
>
> I am a bit concerned about this chunk. The first 8 commands are written
> as octal while the last (calibrate) as decimal. Is this intentional?
>
> I also made a few more changes. Could you please trythe patch below and
> if everything is still working I will apply to my tree.
>
> Thanks!
>
> --
> Dmitry
>
> Input: AT42QT2160 - various fixups
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  drivers/input/keyboard/qt2160.c |  374 ++++++++++++++++++---------------------
>  1 files changed, 171 insertions(+), 203 deletions(-)
>
>
> diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
> index 1f9882b..c1b80da 100644
> --- a/drivers/input/keyboard/qt2160.c
> +++ b/drivers/input/keyboard/qt2160.c
> @@ -1,22 +1,22 @@
>  /*
> -    qt2160.c - Atmel AT42QT2160 Touch Sense Controller
> -
> -    Copyright (C) 2009 Raphael Derosso Pereira <raphaelpereira@gmail.com>
> -
> -    This program is free software; you can redistribute it and/or modify
> -    it under the terms of the GNU General Public License as published by
> -    the Free Software Foundation; either version 2 of the License, or
> -    (at your option) any later version.
> -
> -    This program is distributed in the hope that it will be useful,
> -    but WITHOUT ANY WARRANTY; without even the implied warranty of
> -    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> -    GNU General Public License for more details.
> -
> -    You should have received a copy of the GNU General Public License
> -    along with this program; if not, write to the Free Software
> -    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> -*/
> + * qt2160.c - Atmel AT42QT2160 Touch Sense Controller
> + *
> + *  Copyright (C) 2009 Raphael Derosso Pereira <raphaelpereira@gmail.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> @@ -27,117 +27,110 @@
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>
>  #include <linux/input.h>
> -#include <linux/mutex.h>
>
> -#define QT2160_VALID_CHIPID (0x11)
> +#define QT2160_VALID_CHIPID    0x11
>
> -#define QT2160_CMD_CHIPID    (00)
> -#define QT2160_CMD_CODEVER   (01)
> -#define QT2160_CMD_GSTAT     (02)
> -#define QT2160_CMD_KEYS3     (03)
> -#define QT2160_CMD_KEYS4     (04)
> -#define QT2160_CMD_SLIDE     (05)
> -#define QT2160_CMD_GPIOS     (06)
> -#define QT2160_CMD_SUBVER    (07)
> -#define QT2160_CMD_CALIBRATE (10)
> +#define QT2160_CMD_CHIPID      00
> +#define QT2160_CMD_CODEVER     01
> +#define QT2160_CMD_GSTAT       02
> +#define QT2160_CMD_KEYS3       03
> +#define QT2160_CMD_KEYS4       04
> +#define QT2160_CMD_SLIDE       05
> +#define QT2160_CMD_GPIOS       06
> +#define QT2160_CMD_SUBVER      07
> +#define QT2160_CMD_CALIBRATE   10
>
>  #define QT2160_CYCLE_INTERVAL  (HZ)
>
> -static unsigned char qt2160_key2code[] = {
> -               KEY_0, KEY_1, KEY_2, KEY_3,
> -               KEY_4, KEY_5, KEY_6, KEY_7,
> -               KEY_8, KEY_9, KEY_A, KEY_B,
> -               KEY_C, KEY_D, KEY_E, KEY_F,
> +static const unsigned short qt2160_keycodes[] = {
> +       KEY_0, KEY_1, KEY_2, KEY_3,
> +       KEY_4, KEY_5, KEY_6, KEY_7,
> +       KEY_8, KEY_9, KEY_A, KEY_B,
> +       KEY_C, KEY_D, KEY_E, KEY_F,
>  };
>
>  struct qt2160_data {
> -       struct mutex mlock;
>        struct i2c_client *client;
> -       struct delayed_work handle_work;
> -       struct work_struct handle_irq_work;
>        struct input_dev *input;
> -       u8 version_major;
> -       u8 version_minor;
> -       u8 version_rev;
> +       struct delayed_work dwork;
> +       spinlock_t lock;        /* Protects canceling/rescheduling of dwork */
> +       unsigned short keycodes[ARRAY_SIZE(qt2160_keycodes)];
>        u16 key_matrix;
>  };
>
>  static int qt2160_read(struct i2c_client *client, u8 reg)
>  {
> -       int ret;
> +       int error;
>
> -       ret = i2c_smbus_write_byte(client, reg);
> -       if (ret) {
> +       error = i2c_smbus_write_byte(client, reg);
> +       if (error) {
>                dev_err(&client->dev,
> -                               "couldn't send request. Returned %d\n", ret);
> -               return ret;
> +                       "couldn't send request. Returned %d\n", error);
> +               return error;
>        }
>
> -       ret = i2c_smbus_read_byte(client);
> -       if (ret < 0) {
> +       error = i2c_smbus_read_byte(client);
> +       if (error < 0) {
>                dev_err(&client->dev,
> -                               "couldn't read register. Returned %d\n", ret);
> -               return ret;
> +                       "couldn't read register. Returned %d\n", error);
> +               return error;
>        }
>
> -       return ret;
> +       return 0;
>  }
>
>  static int qt2160_write(struct i2c_client *client, u8 reg, u8 data)
>  {
> -       int ret;
> +       int error;
>
> -       ret = i2c_smbus_write_byte(client, reg);
> -       if (ret) {
> +       error = i2c_smbus_write_byte(client, reg);
> +       if (error) {
>                dev_err(&client->dev,
> -                               "couldn't send request. Returned %d\n", ret);
> -               return ret;
> +                       "couldn't send request. Returned %d\n", error);
> +               return error;
>        }
>
> -       ret = i2c_smbus_write_byte(client, data);
> -       if (ret) {
> +       error = i2c_smbus_write_byte(client, data);
> +       if (error) {
>                dev_err(&client->dev,
> -                               "couldn't write data. Returned %d\n", ret);
> -               return ret;
> +                       "couldn't write data. Returned %d\n", error);
> +               return error;
>        }
>
> -       return ret;
> +       return 0;
>  }
>
> -static int qt2160_get_key_matrix(struct i2c_client *client)
> +static int qt2160_get_key_matrix(struct qt2160_data *qt2160)
>  {
> -       int ret, i;
> -       struct qt2160_data *qt2160;
> +       struct i2c_client *client = qt2160->client;
> +       struct input_dev *input = qt2160->input;
>        u16 old_matrix;
> +       int ret, i;
>
>        dev_dbg(&client->dev, "requesting keys...\n");
> -       qt2160 = i2c_get_clientdata(client);
> -       mutex_lock(&qt2160->mlock);
>
>        /* Read General Status Register */
>        ret = qt2160_read(client, QT2160_CMD_GSTAT);
>        if (ret < 0) {
>                dev_err(&client->dev,
> -                               "could not get general status register\n");
> -               goto err_unlock;
> +                       "could not get general status register\n");
> +               return ret;
>        }
>
>        old_matrix = qt2160->key_matrix;
>
>        ret = qt2160_read(client, QT2160_CMD_KEYS3);
>        if (ret < 0) {
> -               dev_err(&client->dev,
> -                               "could not get keys from register 3\n");
> -               goto err_unlock;
> +               dev_err(&client->dev, "could not get keys from register 3\n");
> +               return ret;
>        }
>
>        qt2160->key_matrix = ret;
>
>        ret = qt2160_read(client, QT2160_CMD_KEYS4);
>        if (ret < 0) {
> -               dev_err(&client->dev,
> -                               "could not get keys from register 4\n");
> -               goto err_unlock;
> +               dev_err(&client->dev, "could not get keys from register 4\n");
> +               return ret;
>        }
>
>        qt2160->key_matrix |= (ret << 8);
> @@ -145,199 +138,183 @@ static int qt2160_get_key_matrix(struct i2c_client *client)
>        /* Read slide and GPIOs to clear CHANGE pin */
>        ret = qt2160_read(client, QT2160_CMD_SLIDE);
>        if (ret < 0) {
> -               dev_err(&client->dev,
> -                               "could not get slide status\n");
> -               goto err_unlock;
> +               dev_err(&client->dev, "could not get slide status\n");
> +               return ret;
>        }
>
>        ret = qt2160_read(client, QT2160_CMD_GPIOS);
>        if (ret < 0) {
>                dev_err(&client->dev, "could not get GPIOs\n");
> -               goto err_unlock;
> +               return ret;
>        }
>
>        for (i = 0; i < 16; ++i) {
>                int keyval = (qt2160->key_matrix >> i) & 0x01;
>
>                if (((old_matrix >> i) & 0x01) != keyval) {
> -                       input_report_key(qt2160->input,
> -                               ((unsigned char *)qt2160->input->keycode)[i],
> -                               keyval);
> -                       input_sync(qt2160->input);
> -
> -                       if (keyval)
> -                               dev_dbg(&client->dev, "key %d pressed\n", i);
> -                       else
> -                               dev_dbg(&client->dev, "key %d released\n", i);
> +                       dev_dbg(&client->dev, "key %d %s\n",
> +                               i, keyval ? "pressed" : "released");
> +
> +                       input_report_key(input, qt2160->keycodes[i], keyval);
> +                       input_sync(input);
>                }
>        }
>
> -       mutex_unlock(&qt2160->mlock);
>        return 0;
> -
> -err_unlock:
> -       mutex_unlock(&qt2160->mlock);
> -       return ret;
>  }
>
>  static irqreturn_t qt2160_irq(int irq, void *_qt2160)
>  {
>        struct qt2160_data *qt2160 = _qt2160;
> +       unsigned long flags;
>
> -       schedule_work(&qt2160->handle_irq_work);
> -       return IRQ_HANDLED;
> -}
> +       spin_lock_irqsave(&qt2160->lock, flags);
>
> -static void qt2160_worker(struct work_struct *work)
> -{
> -       struct qt2160_data *qt2160;
> +       __cancel_delayed_work(&qt2160->dwork);
> +       schedule_delayed_work(&qt2160->dwork, 0);
>
> -       qt2160 = container_of(work, struct qt2160_data,
> -                       handle_irq_work);
> +       spin_unlock_irqrestore(&qt2160->lock, flags);
>
> -       dev_dbg(&qt2160->client->dev, "irq worker\n");
> -       qt2160_get_key_matrix(qt2160->client);
> +       return IRQ_HANDLED;
>  }
>
> -static void qt2160_cycle_worker(struct work_struct *work)
> +static void qt2160_worker(struct work_struct *work)
>  {
> -       struct qt2160_data *qt2160;
> +       struct qt2160_data *qt2160 =
> +               container_of(work, struct qt2160_data, dwork.work);
>
> -       qt2160 = container_of(work, struct qt2160_data,
> -                       handle_work.work);
> +       dev_dbg(&qt2160->client->dev, "worker\n");
>
> -       dev_dbg(&qt2160->client->dev, "cycling worker\n");
> -       qt2160_get_key_matrix(qt2160->client);
> +       qt2160_get_key_matrix(qt2160);
>
>        /* Avoid lock checking every 500ms */
> -       schedule_delayed_work(&qt2160->handle_work, QT2160_CYCLE_INTERVAL);
> +       spin_lock_irq(&qt2160->lock);
> +       schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
> +       spin_unlock_irq(&qt2160->lock);
>  }
>
> -static int __devinit qt2160_probe(struct i2c_client *client,
> -               const struct i2c_device_id *id)
> -{
> -       int ret, i;
> -       struct qt2160_data *qt2160;
>
> -       dev_info(&client->dev, "probing device...\n");
> +static bool __devinit qt2160_identify(struct i2c_client *client)
> +{
> +       int id, ver, rev;
>
>        /* Read Chid ID to check if chip is valid */
> -       ret = qt2160_read(client, QT2160_CMD_CHIPID);
> -       if (ret != QT2160_VALID_CHIPID) {
> -               dev_err(&client->dev, "ID %d not supported\n", ret);
> -               return ret;
> +       id = qt2160_read(client, QT2160_CMD_CHIPID);
> +       if (id != QT2160_VALID_CHIPID) {
> +               dev_err(&client->dev, "ID %d not supported\n", id);
> +               return false;
>        }
>
> -       /* Chip is valid and active. Allocate structure */
> -       qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL);
> -       if (!qt2160) {
> -               dev_err(&client->dev, "insufficient memory\n");
> -               return -ENOMEM;
> -       }
> -
> -       i2c_set_clientdata(client, qt2160);
> -       qt2160->client = client;
> -
>        /* Read chip firmware version */
> -       ret = qt2160_read(client, QT2160_CMD_CODEVER);
> -       if (ret < 0) {
> +       ver = qt2160_read(client, QT2160_CMD_CODEVER);
> +       if (ver < 0) {
>                dev_err(&client->dev, "could not get firmware version\n");
> -               goto err_free_qtdata;
> +               return false;
>        }
>
> -       qt2160->version_major = ret >> 4;
> -       qt2160->version_minor = ret & 0xf;
> -
>        /* Read chip firmware revision */
> -       ret = qt2160_read(client, QT2160_CMD_SUBVER);
> -       if (ret < 0) {
> +       rev = qt2160_read(client, QT2160_CMD_SUBVER);
> +       if (rev < 0) {
>                dev_err(&client->dev, "could not get firmware revision\n");
> -               goto err_free_qtdata;
> +               return false;
>        }
>
> -       qt2160->version_rev = ret;
> -
>        dev_info(&client->dev, "AT42QT2160 firmware version %d.%d.%d\n",
> -                       qt2160->version_major, qt2160->version_minor,
> -                       qt2160->version_rev);
> +                ver >> 4, ver &0xf, rev);
>
> -       /* Calibrate device */
> -       ret = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
> -       if (ret) {
> -               dev_err(&client->dev,
> -                       "Failed to calibrate device\n");
> -               goto err_free_input;
> -       }
> +       return true;
> +}
>
> -       /* Initialize input structure */
> -       qt2160->input = input_allocate_device();
> -       if (!qt2160->input) {
> -               dev_err(&client->dev, "input device: Not enough memory\n");
> -               ret = -ENOMEM;
> -               goto err_free_qtdata;
> -       }
> +static int __devinit qt2160_probe(struct i2c_client *client,
> +                                 const struct i2c_device_id *id)
> +{
> +       struct qt2160_data *qt2160;
> +       struct input_dev *input;
> +       int i;
> +       int error;
>
> -       qt2160->input->name = "AT42QT2160 Touch Sense Keyboard";
> -       qt2160->input->id.bustype = BUS_I2C;
> -       qt2160->input->keycode = qt2160_key2code;
> -       qt2160->input->keycodesize = sizeof(unsigned char);
> -       qt2160->input->keycodemax = ARRAY_SIZE(qt2160_key2code);
> +       dev_dbg(&client->dev, "probing device...\n");
>
> -       __set_bit(EV_KEY, qt2160->input->evbit);
> -       __clear_bit(EV_REP, qt2160->input->evbit);
> -       for (i = 0; i < ARRAY_SIZE(qt2160_key2code); i++)
> -               __set_bit(qt2160_key2code[i], qt2160->input->keybit);
> +       if (!qt2160_identify(client))
> +               return -ENODEV;
>
> -       ret = input_register_device(qt2160->input);
> -       if (ret) {
> +       /* Calibrate device */
> +       error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
> +       if (error) {
>                dev_err(&client->dev,
> -                       "input device: Failed to register device\n");
> -               goto err_free_input;
> +                       "Failed to calibrate device\n");
> +               return error;
>        }
>
> -       /* Initialize IRQ structure */
> -       mutex_init(&qt2160->mlock);
> +       /* Chip is valid and active. Allocate structures */
> +       qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL);
> +       input = input_allocate_device();
> +       if (!qt2160 || !input) {
> +               dev_err(&client->dev, "insufficient memory\n");
> +               error = -ENOMEM;
> +               goto err_free_mem;
> +       }
>
> -       INIT_DELAYED_WORK(&qt2160->handle_work, qt2160_cycle_worker);
> -       INIT_WORK(&qt2160->handle_irq_work, qt2160_worker);
> -       schedule_delayed_work(&qt2160->handle_work, QT2160_CYCLE_INTERVAL);
> +       qt2160->client = client;
> +       qt2160->input = input;
> +       INIT_DELAYED_WORK(&qt2160->dwork, qt2160_worker);
> +       spin_lock_init(&qt2160->lock);
> +
> +       input->name = "AT42QT2160 Touch Sense Keyboard";
> +       input->id.bustype = BUS_I2C;
> +
> +       input->keycode = qt2160->keycodes;
> +       input->keycodesize = sizeof(qt2160->keycodes[0]);
> +       input->keycodemax = ARRAY_SIZE(qt2160->keycodes);
> +
> +       __set_bit(EV_KEY, input->evbit);
> +       __clear_bit(EV_REP, input->evbit);
> +       for (i = 0; i < ARRAY_SIZE(qt2160_keycodes); i++) {
> +               qt2160->keycodes[i] = qt2160_keycodes[i];
> +               __set_bit(qt2160_keycodes[i], input->keybit);
> +       }
>
>        if (client->irq) {
> -               ret = request_irq(client->irq, qt2160_irq,
> -                                 (IRQF_TRIGGER_FALLING), "qt2160", qt2160);
> +               error = request_irq(client->irq, qt2160_irq,
> +                                   IRQF_TRIGGER_FALLING, "qt2160", qt2160);
>
> -               if (ret) {
> +               if (error) {
>                        dev_err(&client->dev,
> -                               "failed to allocate irq %d\n",
> -                               client->irq);
> -                       goto err_free_input;
> +                               "failed to allocate irq %d\n", client->irq);
> +                       goto err_free_mem;
>                }
>        }
>
> -       dev_info(&client->dev, "AT42QT2160 reset completed\n");
> +       error = input_register_device(qt2160->input);
> +       if (error) {
> +               dev_err(&client->dev, "failed to register input device\n");
> +               goto err_free_irq;
> +       }
> +
> +       schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
> +
> +       i2c_set_clientdata(client, qt2160);
>        return 0;
>
> -err_free_input:
> +err_free_irq:
> +       if (client->irq)
> +               free_irq(client->irq, qt2160);
> +err_free_mem:
>        input_free_device(qt2160->input);
> -
> -err_free_qtdata:
>        kfree(qt2160);
> -       i2c_set_clientdata(client, NULL);
> -       return ret;
> +       return error;
>  }
>
>  static int __devexit qt2160_remove(struct i2c_client *client)
>  {
> -       struct qt2160_data *qt2160;
> -
> -       qt2160 = i2c_get_clientdata(client);
> +       struct qt2160_data *qt2160 = i2c_get_clientdata(client);
>
>        /* Release IRQ so no queue will be scheduled */
> -       free_irq(client->irq, qt2160);
> +       if (client->irq)
> +               free_irq(client->irq, qt2160);
>
> -       /* Wait all pending works */
> -       cancel_delayed_work_sync(&qt2160->handle_work);
> -       cancel_work_sync(&qt2160->handle_irq_work);
> +       /* Wait for delayed work to complete */
> +       cancel_delayed_work_sync(&qt2160->dwork);
>
>        /* Unregister input device */
>        input_unregister_device(qt2160->input);
> @@ -346,7 +323,6 @@ static int __devexit qt2160_remove(struct i2c_client *client)
>        kfree(qt2160);
>        i2c_set_clientdata(client, NULL);
>
> -       dev_info(&client->dev, "AT42QT2160 removed.\n");
>        return 0;
>  }
>
> @@ -365,20 +341,12 @@ static struct i2c_driver qt2160_driver = {
>
>        .id_table       = qt2160_idtable,
>        .probe          = qt2160_probe,
> -       .remove         = qt2160_remove,
> +       .remove         = __devexit_p(qt2160_remove),
>  };
>
>  static int __init qt2160_init(void)
>  {
> -       int res;
> -
> -       res = i2c_add_driver(&qt2160_driver);
> -       if (res) {
> -               printk(KERN_ERR "qt2160: Driver registration failed, "
> -                               "module not inserted.\n");
> -               return res;
> -       }
> -       return 0;
> +       return i2c_add_driver(&qt2160_driver);
>  }
>
>  static void __exit qt2160_cleanup(void)
>
Raphael Derosso Pereira Sept. 21, 2009, 3:35 p.m. UTC | #2
Hello Dimitry,

2009/9/15 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
> This version is much better, thank you very much for making the changes
> I requested.

You're very welcomed, let's see this version I'm sending now!

>
>> +
>> +#define QT2160_CMD_CHIPID    (00)
>> +#define QT2160_CMD_CODEVER   (01)
>> +#define QT2160_CMD_GSTAT     (02)
>> +#define QT2160_CMD_KEYS3     (03)
>> +#define QT2160_CMD_KEYS4     (04)
>> +#define QT2160_CMD_SLIDE     (05)
>> +#define QT2160_CMD_GPIOS     (06)
>> +#define QT2160_CMD_SUBVER    (07)
>> +#define QT2160_CMD_CALIBRATE (10)
>
> I am a bit concerned about this chunk. The first 8 commands are written
> as octal while the last (calibrate) as decimal. Is this intentional?

You are right. There's no need to declare them as octal.

>
> I also made a few more changes. Could you please trythe patch below and
> if everything is still working I will apply to my tree.

As I had already made other modifications, I added your changes into
my code, correct a bug you inserted (qt2160_read must return the
variable value and not 0 at the end), and I'm sending a patch against
2.6.31. Hope you don't mind.

--

From: Raphael Derosso Pereira <raphaelpereira@gmail.com>

Input: AT42QT2160

Inclusion of input->open and input->close plus Dmitry fixups plus other cleanups
needed after checking code.

Signed-off-by: Raphael Derosso Pereira <raphaelpereira@gmail.com>
--
diff -pruN linux-2.6.31_orig/drivers/input/keyboard/Kconfig
linux-2.6.31_rp/drivers/input/keyboard/Kconfig
--- linux-2.6.31_orig/drivers/input/keyboard/Kconfig	2009-09-09
19:13:59.000000000 -0300
+++ linux-2.6.31_rp/drivers/input/keyboard/Kconfig	2009-09-21
12:09:49.000000000 -0300
@@ -361,4 +361,22 @@ config KEYBOARD_XTKBD
 	  To compile this driver as a module, choose M here: the
 	  module will be called xtkbd.

+config QT2160
+	tristate "Atmel AT42QT2160 Touch Sensor Chip"
+	depends on EXPERIMENTAL
+	select I2C
+	help
+	  If you say yes here you get support for Atmel AT42QT2160 Touch
+	  Sensor chip as a keyboard input.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called qt2160.
+
+config QT2160_DEBUG
+	bool "AT42QT2160 Debug Messages"
+	depends on QT2160
+	help
+	  Generates lots of debug from driver to help show what is going
+	  on under the hoods.
+
 endif
diff -pruN linux-2.6.31_orig/drivers/input/keyboard/Makefile
linux-2.6.31_rp/drivers/input/keyboard/Makefile
--- linux-2.6.31_orig/drivers/input/keyboard/Makefile	2009-09-09
19:13:59.000000000 -0300
+++ linux-2.6.31_rp/drivers/input/keyboard/Makefile	2009-09-21
10:19:23.000000000 -0300
@@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stow
 obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
 obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
 obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
+obj-$(CONFIG_QT2160)		+= qt2160.o
diff -pruN linux-2.6.31_orig/drivers/input/keyboard/qt2160.c
linux-2.6.31_rp/drivers/input/keyboard/qt2160.c
--- linux-2.6.31_orig/drivers/input/keyboard/qt2160.c	1969-12-31
21:00:00.000000000 -0300
+++ linux-2.6.31_rp/drivers/input/keyboard/qt2160.c	2009-09-21
12:13:43.000000000 -0300
@@ -0,0 +1,432 @@
+/*
+ *  qt2160.c - Atmel AT42QT2160 Touch Sense Controller
+ *
+ *  Copyright (C) 2009 Raphael Derosso Pereira <raphaelpereira@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/autoconf.h>
+#ifdef CONFIG_QT2160_DEBUG
+#define DEBUG
+#endif
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+
+#define QT2160_VALID_CHIPID  0x11
+
+#define QT2160_CMD_CHIPID     0
+#define QT2160_CMD_CODEVER    1
+#define QT2160_CMD_GSTAT      2
+#define QT2160_CMD_KEYS3      3
+#define QT2160_CMD_KEYS4      4
+#define QT2160_CMD_SLIDE      5
+#define QT2160_CMD_GPIOS      6
+#define QT2160_CMD_SUBVER     7
+#define QT2160_CMD_CALIBRATE  10
+
+#define QT2160_CYCLE_INTERVAL	(2*HZ)
+
+static unsigned char qt2160_key2code[] = {
+	KEY_0, KEY_1, KEY_2, KEY_3,
+	KEY_4, KEY_5, KEY_6, KEY_7,
+	KEY_8, KEY_9, KEY_A, KEY_B,
+	KEY_C, KEY_D, KEY_E, KEY_F,
+};
+
+struct qt2160_data {
+	struct i2c_client *client;
+	struct delayed_work dwork;
+	struct input_dev *input;
+	spinlock_t lock;        /* Protects canceling/rescheduling of dwork */
+	unsigned short keycodes[ARRAY_SIZE(qt2160_key2code)];
+	u16 key_matrix;
+};
+
+static int qt2160_read(struct i2c_client *client, u8 reg)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte(client, reg);
+	if (ret) {
+		dev_err(&client->dev,
+			"couldn't send request. Returned %d\n", ret);
+		return ret;
+	}
+
+	ret = i2c_smbus_read_byte(client);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"couldn't read register. Returned %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int qt2160_read_block(struct i2c_client *client, u8 inireg, u8 *buffer,
+	unsigned int count)
+{
+	int error, idx = 0;
+
+	/*
+	 * Can't use SMBus block data read. Check for I2C functionality to speed
+	 * things up whenever possible. Otherwise we will be forced to read
+	 * sequentially.
+	 */
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))	{
+
+		error = i2c_smbus_write_byte(client, inireg + idx);
+		if (error) {
+			dev_err(&client->dev,
+				"couldn't send request. Returned %d\n", error);
+			return error;
+		}
+
+		error = i2c_master_recv(client, buffer, count);
+		if (error != count) {
+			dev_err(&client->dev,
+				"couldn't read registers. Returned %d bytes\n", error);
+			return error;
+		}
+	} else {
+
+		while (count--) {
+			int data;
+
+			error = i2c_smbus_write_byte(client, inireg + idx);
+			if (error) {
+				dev_err(&client->dev,
+					"couldn't send request. Returned %d\n", error);
+				return error;
+			}
+
+			data = i2c_smbus_read_byte(client);
+			if (data < 0) {
+				dev_err(&client->dev,
+					"couldn't read register. Returned %d\n", data);
+				return data;
+			}
+
+			buffer[idx++] = data;
+		}
+	}
+
+	return 0;
+}
+
+static int qt2160_write(struct i2c_client *client, u8 reg, u8 data)
+{
+	int error;
+
+	error = i2c_smbus_write_byte(client, reg);
+	if (error) {
+		dev_err(&client->dev,
+			"couldn't send request. Returned %d\n", error);
+		return error;
+	}
+
+	error = i2c_smbus_write_byte(client, data);
+	if (error) {
+		dev_err(&client->dev,
+			"couldn't write data. Returned %d\n", error);
+		return error;
+	}
+
+	return error;
+}
+
+static int qt2160_get_key_matrix(struct qt2160_data *qt2160)
+{
+	struct i2c_client *client = qt2160->client;
+	struct input_dev *input = qt2160->input;
+	u8 regs[6];
+	u16 old_matrix, new_matrix;
+	int ret, i, mask;
+
+	dev_dbg(&client->dev, "requesting keys...\n");
+
+	/* Read all registers from General Status Register
+	 * to GPIOs register
+	 */
+	ret = qt2160_read_block(client, QT2160_CMD_GSTAT, regs, 6);
+	if (ret) {
+		dev_err(&client->dev,
+			"could not perform chip read.\n");
+		return ret;
+	}
+
+	old_matrix = qt2160->key_matrix;
+	qt2160->key_matrix = new_matrix = (regs[2] << 8) | regs[1];
+
+	mask = 0x01;
+	for (i = 0; i < 16; ++i, mask <<= 1) {
+		int keyval = new_matrix & mask;
+
+		if ((old_matrix & mask) != keyval) {
+
+			dev_dbg(&client->dev, "key %d %s\n",
+				i, keyval ? "pressed" : "released");
+
+			input_report_key(input, qt2160->keycodes[i], keyval);
+
+			input_sync(input);
+		}
+	}
+
+	return 0;
+}
+
+static irqreturn_t qt2160_irq(int irq, void *_qt2160)
+{
+	struct qt2160_data *qt2160 = _qt2160;
+	unsigned long flags;
+
+	spin_lock_irqsave(&qt2160->lock, flags);
+
+	__cancel_delayed_work(&qt2160->dwork);
+	schedule_delayed_work(&qt2160->dwork, 0);
+
+	spin_unlock_irqrestore(&qt2160->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static void qt2160_worker(struct work_struct *work)
+{
+	struct qt2160_data *qt2160 =
+		container_of(work, struct qt2160_data, dwork.work);
+
+	dev_dbg(&qt2160->client->dev, "worker\n");
+
+	qt2160_get_key_matrix(qt2160);
+
+	/* Avoid lock, checking every QT2160_CYCLE_INTERVAL */
+	spin_lock_irq(&qt2160->lock);
+	schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
+	spin_unlock_irq(&qt2160->lock);
+}
+
+
+static int qt2160_open(struct input_dev *dev)
+{
+	int error;
+	struct qt2160_data *qt2160;
+	struct i2c_client *client;
+
+	qt2160 = input_get_drvdata(dev);
+	client = qt2160->client;
+
+	/* Calibrate device */
+	error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
+	if (error) {
+		dev_err(&client->dev,
+			"failed to calibrate device\n");
+		return error;
+	}
+
+	/* Initialize IRQ structure */
+	schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
+
+	if (client->irq) {
+		error = request_irq(client->irq, qt2160_irq,
+			(IRQF_TRIGGER_FALLING), "qt2160", qt2160);
+
+		if (error) {
+			dev_err(&client->dev,
+				"failed to allocate irq %d\n", client->irq);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
+void qt2160_close(struct input_dev *dev)
+{
+	struct qt2160_data *qt2160;
+
+	qt2160 = input_get_drvdata(dev);
+
+	/* Release IRQ so no queue will be scheduled */
+	if (qt2160->client->irq)
+		free_irq(qt2160->client->irq, qt2160);
+
+	/* Wait all pending works */
+	cancel_delayed_work_sync(&qt2160->dwork);
+}
+
+static bool __devinit qt2160_identify(struct i2c_client *client)
+{
+	int id, ver, rev;
+
+	/* Read Chid ID to check if chip is valid */
+	id = qt2160_read(client, QT2160_CMD_CHIPID);
+	if (id != QT2160_VALID_CHIPID) {
+		dev_err(&client->dev, "ID %d not supported\n", id);
+		return false;
+	}
+
+	/* Read chip firmware version */
+	ver = qt2160_read(client, QT2160_CMD_CODEVER);
+	if (ver < 0) {
+		dev_err(&client->dev, "could not get firmware version\n");
+		return false;
+	}
+
+	/* Read chip firmware revision */
+	rev = qt2160_read(client, QT2160_CMD_SUBVER);
+	if (rev < 0) {
+		dev_err(&client->dev, "could not get firmware revision\n");
+		return false;
+	}
+
+	dev_info(&client->dev, "AT42QT2160 firmware version %d.%d.%d\n",
+			ver >> 4, ver & 0xf, rev);
+
+	return true;
+}
+
+static int __devinit
+	qt2160_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct qt2160_data *qt2160 = NULL;
+	struct input_dev *input = NULL;
+	int i;
+	int error;
+
+	/* Check functionality */
+	error = i2c_check_functionality(client->adapter,
+			I2C_FUNC_SMBUS_BYTE);
+	if (!error) {
+		dev_err(&client->dev, "%s adapter not supported\n",
+				dev_driver_string(&client->adapter->dev));
+		return -ENODEV;
+	}
+
+	dev_info(&client->dev, "probing device...\n");
+
+	if (!qt2160_identify(client))
+		return -ENODEV;
+
+	/* Chip is valid and active. Allocate structure */
+	qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL);
+	input = input_allocate_device();
+	if (!qt2160 || !input) {
+		dev_err(&client->dev, "insufficient memory\n");
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	qt2160->client = client;
+	qt2160->input = input;
+	INIT_DELAYED_WORK(&qt2160->dwork, qt2160_worker);
+	spin_lock_init(&qt2160->lock);
+
+	qt2160->input->name = "AT42QT2160 Touch Sense Keyboard";
+	qt2160->input->id.bustype = BUS_I2C;
+
+	qt2160->input->keycode = qt2160->keycodes;
+	qt2160->input->keycodesize = sizeof(unsigned char);
+	qt2160->input->keycodemax = ARRAY_SIZE(qt2160_key2code);
+
+	qt2160->input->open  = qt2160_open;
+	qt2160->input->close = qt2160_close;
+
+	__set_bit(EV_KEY, qt2160->input->evbit);
+	__clear_bit(EV_REP, qt2160->input->evbit);
+	for (i = 0; i < ARRAY_SIZE(qt2160_key2code); i++) {
+		qt2160->keycodes[i] = qt2160_key2code[i];
+		__set_bit(qt2160_key2code[i], qt2160->input->keybit);
+	}
+
+	/* Must define drvdata to be used by qt2160_open */
+	i2c_set_clientdata(client, qt2160);
+	input_set_drvdata(input, qt2160);
+
+	error = input_register_device(qt2160->input);
+	if (error) {
+		dev_err(&client->dev,
+			"input device: Failed to register device\n");
+		goto err_free_drvdata;
+	}
+
+	dev_info(&client->dev, "AT42QT2160 activated\n");
+	return 0;
+
+err_free_drvdata:
+	i2c_set_clientdata(client, NULL);
+
+err_free_mem:
+	input_free_device(qt2160->input);
+	kfree(qt2160);
+	return error;
+}
+
+static int __devexit qt2160_remove(struct i2c_client *client)
+{
+	struct qt2160_data *qt2160 = i2c_get_clientdata(client);
+
+	/* Unregister input device */
+	input_unregister_device(qt2160->input);
+
+	/* Free client data */
+	kfree(qt2160);
+	i2c_set_clientdata(client, NULL);
+	return 0;
+}
+
+static struct i2c_device_id qt2160_idtable[] = {
+	{ "qt2160", 0, },
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, qt2160_idtable);
+
+static struct i2c_driver qt2160_driver = {
+	.driver = {
+		.name	= "qt2160",
+		.owner  = THIS_MODULE,
+	},
+
+	.id_table	= qt2160_idtable,
+	.probe		= qt2160_probe,
+	.remove		= __devexit_p(qt2160_remove),
+};
+
+static int __init qt2160_init(void)
+{
+	return i2c_add_driver(&qt2160_driver);
+}
+
+static void __exit qt2160_cleanup(void)
+{
+	i2c_del_driver(&qt2160_driver);
+}
+
+MODULE_AUTHOR("Raphael Derosso Pereira <raphaelpereira@gmail.com>");
+MODULE_DESCRIPTION("Driver for AT42QT2160 Touch Sensor");
+MODULE_LICENSE("GPL");
+
+module_init(qt2160_init);
+module_exit(qt2160_cleanup);
Dmitry Torokhov Sept. 22, 2009, 5:52 a.m. UTC | #3
Hi Raphael,

On Mon, Sep 21, 2009 at 12:35:21PM -0300, Raphael Derosso Pereira wrote:
> Hello Dimitry,
> 
> 2009/9/15 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> >
> > This version is much better, thank you very much for making the changes
> > I requested.
> 
> You're very welcomed, let's see this version I'm sending now!
> 
> >
> >> +
> >> +#define QT2160_CMD_CHIPID    (00)
> >> +#define QT2160_CMD_CODEVER   (01)
> >> +#define QT2160_CMD_GSTAT     (02)
> >> +#define QT2160_CMD_KEYS3     (03)
> >> +#define QT2160_CMD_KEYS4     (04)
> >> +#define QT2160_CMD_SLIDE     (05)
> >> +#define QT2160_CMD_GPIOS     (06)
> >> +#define QT2160_CMD_SUBVER    (07)
> >> +#define QT2160_CMD_CALIBRATE (10)
> >
> > I am a bit concerned about this chunk. The first 8 commands are written
> > as octal while the last (calibrate) as decimal. Is this intentional?
> 
> You are right. There's no need to declare them as octal.
> 
> >
> > I also made a few more changes. Could you please trythe patch below and
> > if everything is still working I will apply to my tree.
> 
> As I had already made other modifications, I added your changes into
> my code, correct a bug you inserted (qt2160_read must return the
> variable value and not 0 at the end), and I'm sending a patch against
> 2.6.31. Hope you don't mind.
> 
> --
> 
> From: Raphael Derosso Pereira <raphaelpereira@gmail.com>
> 
> Input: AT42QT2160
> 
> Inclusion of input->open and input->close plus Dmitry fixups plus other cleanups
> needed after checking code.
> 
> Signed-off-by: Raphael Derosso Pereira <raphaelpereira@gmail.com>
> --
> diff -pruN linux-2.6.31_orig/drivers/input/keyboard/Kconfig
> linux-2.6.31_rp/drivers/input/keyboard/Kconfig
> --- linux-2.6.31_orig/drivers/input/keyboard/Kconfig	2009-09-09
> 19:13:59.000000000 -0300
> +++ linux-2.6.31_rp/drivers/input/keyboard/Kconfig	2009-09-21
> 12:09:49.000000000 -0300
> @@ -361,4 +361,22 @@ config KEYBOARD_XTKBD
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called xtkbd.
> 
> +config QT2160
> +	tristate "Atmel AT42QT2160 Touch Sensor Chip"
> +	depends on EXPERIMENTAL
> +	select I2C
> +	help
> +	  If you say yes here you get support for Atmel AT42QT2160 Touch
> +	  Sensor chip as a keyboard input.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called qt2160.
> +
> +config QT2160_DEBUG
> +	bool "AT42QT2160 Debug Messages"
> +	depends on QT2160
> +	help
> +	  Generates lots of debug from driver to help show what is going
> +	  on under the hoods.

I don't think we need a Kconfig entry for debugging. A person who wants
to debug such driver can easily turn on debugging manually.

> +
> +static int qt2160_open(struct input_dev *dev)
> +{
> +	int error;
> +	struct qt2160_data *qt2160;
> +	struct i2c_client *client;
> +
> +	qt2160 = input_get_drvdata(dev);
> +	client = qt2160->client;
> +
> +	/* Calibrate device */
> +	error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to calibrate device\n");
> +		return error;
> +	}
> +
> +	/* Initialize IRQ structure */
> +	schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
> +
> +	if (client->irq) {
> +		error = request_irq(client->irq, qt2160_irq,
> +			(IRQF_TRIGGER_FALLING), "qt2160", qt2160);
> +
> +		if (error) {
> +			dev_err(&client->dev,
> +				"failed to allocate irq %d\n", client->irq);
> +			return error;
> +		}
> +	}
> +

We normally try to allocate all necessary resources in probe() so that
if device is bound to a driver it should work unless it is broken. Since
there is no way to shut off IRQs while IRQ handler is registered it does
not make sense to have open and close if we allocate IRQ in probe().

Please take a look at the latest version that I have in 'for-linus'
branch in my tree on kernel.org and shout if you see something wrong.

Thanks.
Raphael Derosso Pereira Sept. 22, 2009, 11:41 a.m. UTC | #4
Hello,

2009/9/22 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Raphael,
>
>
> I don't think we need a Kconfig entry for debugging. A person who wants
> to debug such driver can easily turn on debugging manually.

Ok.

>
> We normally try to allocate all necessary resources in probe() so that
> if device is bound to a driver it should work unless it is broken. Since
> there is no way to shut off IRQs while IRQ handler is registered it does
> not make sense to have open and close if we allocate IRQ in probe().

Agreed.

>
> Please take a look at the latest version that I have in 'for-linus'
> branch in my tree on kernel.org and shout if you see something wrong.

How am I supposed to do that?

>
> Thanks.
>
> --
> Dmitry
>

Patch
diff mbox

diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index 1f9882b..c1b80da 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -1,22 +1,22 @@ 
 /*
-    qt2160.c - Atmel AT42QT2160 Touch Sense Controller
-
-    Copyright (C) 2009 Raphael Derosso Pereira <raphaelpereira@gmail.com>
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
-*/
+ * qt2160.c - Atmel AT42QT2160 Touch Sense Controller
+ *
+ *  Copyright (C) 2009 Raphael Derosso Pereira <raphaelpereira@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
 
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -27,117 +27,110 @@ 
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/input.h>
-#include <linux/mutex.h>
 
-#define QT2160_VALID_CHIPID (0x11)
+#define QT2160_VALID_CHIPID	0x11
 
-#define QT2160_CMD_CHIPID    (00)
-#define QT2160_CMD_CODEVER   (01)
-#define QT2160_CMD_GSTAT     (02)
-#define QT2160_CMD_KEYS3     (03)
-#define QT2160_CMD_KEYS4     (04)
-#define QT2160_CMD_SLIDE     (05)
-#define QT2160_CMD_GPIOS     (06)
-#define QT2160_CMD_SUBVER    (07)
-#define QT2160_CMD_CALIBRATE (10)
+#define QT2160_CMD_CHIPID	00
+#define QT2160_CMD_CODEVER	01
+#define QT2160_CMD_GSTAT	02
+#define QT2160_CMD_KEYS3	03
+#define QT2160_CMD_KEYS4	04
+#define QT2160_CMD_SLIDE	05
+#define QT2160_CMD_GPIOS	06
+#define QT2160_CMD_SUBVER	07
+#define QT2160_CMD_CALIBRATE	10
 
 #define QT2160_CYCLE_INTERVAL	(HZ)
 
-static unsigned char qt2160_key2code[] = {
-		KEY_0, KEY_1, KEY_2, KEY_3,
-		KEY_4, KEY_5, KEY_6, KEY_7,
-		KEY_8, KEY_9, KEY_A, KEY_B,
-		KEY_C, KEY_D, KEY_E, KEY_F,
+static const unsigned short qt2160_keycodes[] = {
+	KEY_0, KEY_1, KEY_2, KEY_3,
+	KEY_4, KEY_5, KEY_6, KEY_7,
+	KEY_8, KEY_9, KEY_A, KEY_B,
+	KEY_C, KEY_D, KEY_E, KEY_F,
 };
 
 struct qt2160_data {
-	struct mutex mlock;
 	struct i2c_client *client;
-	struct delayed_work handle_work;
-	struct work_struct handle_irq_work;
 	struct input_dev *input;
-	u8 version_major;
-	u8 version_minor;
-	u8 version_rev;
+	struct delayed_work dwork;
+	spinlock_t lock;	/* Protects canceling/rescheduling of dwork */
+	unsigned short keycodes[ARRAY_SIZE(qt2160_keycodes)];
 	u16 key_matrix;
 };
 
 static int qt2160_read(struct i2c_client *client, u8 reg)
 {
-	int ret;
+	int error;
 
-	ret = i2c_smbus_write_byte(client, reg);
-	if (ret) {
+	error = i2c_smbus_write_byte(client, reg);
+	if (error) {
 		dev_err(&client->dev,
-				"couldn't send request. Returned %d\n", ret);
-		return ret;
+			"couldn't send request. Returned %d\n", error);
+		return error;
 	}
 
-	ret = i2c_smbus_read_byte(client);
-	if (ret < 0) {
+	error = i2c_smbus_read_byte(client);
+	if (error < 0) {
 		dev_err(&client->dev,
-				"couldn't read register. Returned %d\n", ret);
-		return ret;
+			"couldn't read register. Returned %d\n", error);
+		return error;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int qt2160_write(struct i2c_client *client, u8 reg, u8 data)
 {
-	int ret;
+	int error;
 
-	ret = i2c_smbus_write_byte(client, reg);
-	if (ret) {
+	error = i2c_smbus_write_byte(client, reg);
+	if (error) {
 		dev_err(&client->dev,
-				"couldn't send request. Returned %d\n", ret);
-		return ret;
+			"couldn't send request. Returned %d\n", error);
+		return error;
 	}
 
-	ret = i2c_smbus_write_byte(client, data);
-	if (ret) {
+	error = i2c_smbus_write_byte(client, data);
+	if (error) {
 		dev_err(&client->dev,
-				"couldn't write data. Returned %d\n", ret);
-		return ret;
+			"couldn't write data. Returned %d\n", error);
+		return error;
 	}
 
-	return ret;
+	return 0;
 }
 
-static int qt2160_get_key_matrix(struct i2c_client *client)
+static int qt2160_get_key_matrix(struct qt2160_data *qt2160)
 {
-	int ret, i;
-	struct qt2160_data *qt2160;
+	struct i2c_client *client = qt2160->client;
+	struct input_dev *input = qt2160->input;
 	u16 old_matrix;
+	int ret, i;
 
 	dev_dbg(&client->dev, "requesting keys...\n");
-	qt2160 = i2c_get_clientdata(client);
-	mutex_lock(&qt2160->mlock);
 
 	/* Read General Status Register */
 	ret = qt2160_read(client, QT2160_CMD_GSTAT);
 	if (ret < 0) {
 		dev_err(&client->dev,
-				"could not get general status register\n");
-		goto err_unlock;
+			"could not get general status register\n");
+		return ret;
 	}
 
 	old_matrix = qt2160->key_matrix;
 
 	ret = qt2160_read(client, QT2160_CMD_KEYS3);
 	if (ret < 0) {
-		dev_err(&client->dev,
-				"could not get keys from register 3\n");
-		goto err_unlock;
+		dev_err(&client->dev, "could not get keys from register 3\n");
+		return ret;
 	}
 
 	qt2160->key_matrix = ret;
 
 	ret = qt2160_read(client, QT2160_CMD_KEYS4);
 	if (ret < 0) {
-		dev_err(&client->dev,
-				"could not get keys from register 4\n");
-		goto err_unlock;
+		dev_err(&client->dev, "could not get keys from register 4\n");
+		return ret;
 	}
 
 	qt2160->key_matrix |= (ret << 8);
@@ -145,199 +138,183 @@  static int qt2160_get_key_matrix(struct i2c_client *client)
 	/* Read slide and GPIOs to clear CHANGE pin */
 	ret = qt2160_read(client, QT2160_CMD_SLIDE);
 	if (ret < 0) {
-		dev_err(&client->dev,
-				"could not get slide status\n");
-		goto err_unlock;
+		dev_err(&client->dev, "could not get slide status\n");
+		return ret;
 	}
 
 	ret = qt2160_read(client, QT2160_CMD_GPIOS);
 	if (ret < 0) {
 		dev_err(&client->dev, "could not get GPIOs\n");
-		goto err_unlock;
+		return ret;
 	}
 
 	for (i = 0; i < 16; ++i) {
 		int keyval = (qt2160->key_matrix >> i) & 0x01;
 
 		if (((old_matrix >> i) & 0x01) != keyval) {
-			input_report_key(qt2160->input,
-				((unsigned char *)qt2160->input->keycode)[i],
-				keyval);
-			input_sync(qt2160->input);
-
-			if (keyval)
-				dev_dbg(&client->dev, "key %d pressed\n", i);
-			else
-				dev_dbg(&client->dev, "key %d released\n", i);
+			dev_dbg(&client->dev, "key %d %s\n",
+				i, keyval ? "pressed" : "released");
+
+			input_report_key(input, qt2160->keycodes[i], keyval);
+			input_sync(input);
 		}
 	}
 
-	mutex_unlock(&qt2160->mlock);
 	return 0;
-
-err_unlock:
-	mutex_unlock(&qt2160->mlock);
-	return ret;
 }
 
 static irqreturn_t qt2160_irq(int irq, void *_qt2160)
 {
 	struct qt2160_data *qt2160 = _qt2160;
+	unsigned long flags;
 
-	schedule_work(&qt2160->handle_irq_work);
-	return IRQ_HANDLED;
-}
+	spin_lock_irqsave(&qt2160->lock, flags);
 
-static void qt2160_worker(struct work_struct *work)
-{
-	struct qt2160_data *qt2160;
+	__cancel_delayed_work(&qt2160->dwork);
+	schedule_delayed_work(&qt2160->dwork, 0);
 
-	qt2160 = container_of(work, struct qt2160_data,
-			handle_irq_work);
+	spin_unlock_irqrestore(&qt2160->lock, flags);
 
-	dev_dbg(&qt2160->client->dev, "irq worker\n");
-	qt2160_get_key_matrix(qt2160->client);
+	return IRQ_HANDLED;
 }
 
-static void qt2160_cycle_worker(struct work_struct *work)
+static void qt2160_worker(struct work_struct *work)
 {
-	struct qt2160_data *qt2160;
+	struct qt2160_data *qt2160 =
+		container_of(work, struct qt2160_data, dwork.work);
 
-	qt2160 = container_of(work, struct qt2160_data,
-			handle_work.work);
+	dev_dbg(&qt2160->client->dev, "worker\n");
 
-	dev_dbg(&qt2160->client->dev, "cycling worker\n");
-	qt2160_get_key_matrix(qt2160->client);
+	qt2160_get_key_matrix(qt2160);
 
 	/* Avoid lock checking every 500ms */
-	schedule_delayed_work(&qt2160->handle_work, QT2160_CYCLE_INTERVAL);
+	spin_lock_irq(&qt2160->lock);
+	schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
+	spin_unlock_irq(&qt2160->lock);
 }
 
-static int __devinit qt2160_probe(struct i2c_client *client,
-		const struct i2c_device_id *id)
-{
-	int ret, i;
-	struct qt2160_data *qt2160;
 
-	dev_info(&client->dev, "probing device...\n");
+static bool __devinit qt2160_identify(struct i2c_client *client)
+{
+	int id, ver, rev;
 
 	/* Read Chid ID to check if chip is valid */
-	ret = qt2160_read(client, QT2160_CMD_CHIPID);
-	if (ret != QT2160_VALID_CHIPID) {
-		dev_err(&client->dev, "ID %d not supported\n", ret);
-		return ret;
+	id = qt2160_read(client, QT2160_CMD_CHIPID);
+	if (id != QT2160_VALID_CHIPID) {
+		dev_err(&client->dev, "ID %d not supported\n", id);
+		return false;
 	}
 
-	/* Chip is valid and active. Allocate structure */
-	qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL);
-	if (!qt2160) {
-		dev_err(&client->dev, "insufficient memory\n");
-		return -ENOMEM;
-	}
-
-	i2c_set_clientdata(client, qt2160);
-	qt2160->client = client;
-
 	/* Read chip firmware version */
-	ret = qt2160_read(client, QT2160_CMD_CODEVER);
-	if (ret < 0) {
+	ver = qt2160_read(client, QT2160_CMD_CODEVER);
+	if (ver < 0) {
 		dev_err(&client->dev, "could not get firmware version\n");
-		goto err_free_qtdata;
+		return false;
 	}
 
-	qt2160->version_major = ret >> 4;
-	qt2160->version_minor = ret & 0xf;
-
 	/* Read chip firmware revision */
-	ret = qt2160_read(client, QT2160_CMD_SUBVER);
-	if (ret < 0) {
+	rev = qt2160_read(client, QT2160_CMD_SUBVER);
+	if (rev < 0) {
 		dev_err(&client->dev, "could not get firmware revision\n");
-		goto err_free_qtdata;
+		return false;
 	}
 
-	qt2160->version_rev = ret;
-
 	dev_info(&client->dev, "AT42QT2160 firmware version %d.%d.%d\n",
-			qt2160->version_major, qt2160->version_minor,
-			qt2160->version_rev);
+		 ver >> 4, ver &0xf, rev);
 
-	/* Calibrate device */
-	ret = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
-	if (ret) {
-		dev_err(&client->dev,
-			"Failed to calibrate device\n");
-		goto err_free_input;
-	}
+	return true;
+}
 
-	/* Initialize input structure */
-	qt2160->input = input_allocate_device();
-	if (!qt2160->input) {
-		dev_err(&client->dev, "input device: Not enough memory\n");
-		ret = -ENOMEM;
-		goto err_free_qtdata;
-	}
+static int __devinit qt2160_probe(struct i2c_client *client,
+				  const struct i2c_device_id *id)
+{
+	struct qt2160_data *qt2160;
+	struct input_dev *input;
+	int i;
+	int error;
 
-	qt2160->input->name = "AT42QT2160 Touch Sense Keyboard";
-	qt2160->input->id.bustype = BUS_I2C;
-	qt2160->input->keycode = qt2160_key2code;
-	qt2160->input->keycodesize = sizeof(unsigned char);
-	qt2160->input->keycodemax = ARRAY_SIZE(qt2160_key2code);
+	dev_dbg(&client->dev, "probing device...\n");
 
-	__set_bit(EV_KEY, qt2160->input->evbit);
-	__clear_bit(EV_REP, qt2160->input->evbit);
-	for (i = 0; i < ARRAY_SIZE(qt2160_key2code); i++)
-		__set_bit(qt2160_key2code[i], qt2160->input->keybit);
+	if (!qt2160_identify(client))
+		return -ENODEV;
 
-	ret = input_register_device(qt2160->input);
-	if (ret) {
+	/* Calibrate device */
+	error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1);
+	if (error) {
 		dev_err(&client->dev,
-			"input device: Failed to register device\n");
-		goto err_free_input;
+			"Failed to calibrate device\n");
+		return error;
 	}
 
-	/* Initialize IRQ structure */
-	mutex_init(&qt2160->mlock);
+	/* Chip is valid and active. Allocate structures */
+	qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL);
+	input = input_allocate_device();
+	if (!qt2160 || !input) {
+		dev_err(&client->dev, "insufficient memory\n");
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
 
-	INIT_DELAYED_WORK(&qt2160->handle_work, qt2160_cycle_worker);
-	INIT_WORK(&qt2160->handle_irq_work, qt2160_worker);
-	schedule_delayed_work(&qt2160->handle_work, QT2160_CYCLE_INTERVAL);
+	qt2160->client = client;
+	qt2160->input = input;
+	INIT_DELAYED_WORK(&qt2160->dwork, qt2160_worker);
+	spin_lock_init(&qt2160->lock);
+
+	input->name = "AT42QT2160 Touch Sense Keyboard";
+	input->id.bustype = BUS_I2C;
+
+	input->keycode = qt2160->keycodes;
+	input->keycodesize = sizeof(qt2160->keycodes[0]);
+	input->keycodemax = ARRAY_SIZE(qt2160->keycodes);
+
+	__set_bit(EV_KEY, input->evbit);
+	__clear_bit(EV_REP, input->evbit);
+	for (i = 0; i < ARRAY_SIZE(qt2160_keycodes); i++) {
+		qt2160->keycodes[i] = qt2160_keycodes[i];
+		__set_bit(qt2160_keycodes[i], input->keybit);
+	}
 
 	if (client->irq) {
-		ret = request_irq(client->irq, qt2160_irq,
-				  (IRQF_TRIGGER_FALLING), "qt2160", qt2160);
+		error = request_irq(client->irq, qt2160_irq,
+				    IRQF_TRIGGER_FALLING, "qt2160", qt2160);
 
-		if (ret) {
+		if (error) {
 			dev_err(&client->dev,
-				"failed to allocate irq %d\n",
-				client->irq);
-			goto err_free_input;
+				"failed to allocate irq %d\n", client->irq);
+			goto err_free_mem;
 		}
 	}
 
-	dev_info(&client->dev, "AT42QT2160 reset completed\n");
+	error = input_register_device(qt2160->input);
+	if (error) {
+		dev_err(&client->dev, "failed to register input device\n");
+		goto err_free_irq;
+	}
+
+	schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
+
+	i2c_set_clientdata(client, qt2160);
 	return 0;
 
-err_free_input:
+err_free_irq:
+	if (client->irq)
+		free_irq(client->irq, qt2160);
+err_free_mem:
 	input_free_device(qt2160->input);
-
-err_free_qtdata:
 	kfree(qt2160);
-	i2c_set_clientdata(client, NULL);
-	return ret;
+	return error;
 }
 
 static int __devexit qt2160_remove(struct i2c_client *client)
 {
-	struct qt2160_data *qt2160;
-
-	qt2160 = i2c_get_clientdata(client);
+	struct qt2160_data *qt2160 = i2c_get_clientdata(client);
 
 	/* Release IRQ so no queue will be scheduled */
-	free_irq(client->irq, qt2160);
+	if (client->irq)
+		free_irq(client->irq, qt2160);
 
-	/* Wait all pending works */
-	cancel_delayed_work_sync(&qt2160->handle_work);
-	cancel_work_sync(&qt2160->handle_irq_work);
+	/* Wait for delayed work to complete */
+	cancel_delayed_work_sync(&qt2160->dwork);
 
 	/* Unregister input device */
 	input_unregister_device(qt2160->input);
@@ -346,7 +323,6 @@  static int __devexit qt2160_remove(struct i2c_client *client)
 	kfree(qt2160);
 	i2c_set_clientdata(client, NULL);
 
-	dev_info(&client->dev, "AT42QT2160 removed.\n");
 	return 0;
 }
 
@@ -365,20 +341,12 @@  static struct i2c_driver qt2160_driver = {
 
 	.id_table	= qt2160_idtable,
 	.probe		= qt2160_probe,
-	.remove		= qt2160_remove,
+	.remove		= __devexit_p(qt2160_remove),
 };
 
 static int __init qt2160_init(void)
 {
-	int res;
-
-	res = i2c_add_driver(&qt2160_driver);
-	if (res) {
-		printk(KERN_ERR "qt2160: Driver registration failed, "
-				"module not inserted.\n");
-		return res;
-	}
-	return 0;
+	return i2c_add_driver(&qt2160_driver);
 }
 
 static void __exit qt2160_cleanup(void)