Hi Shubhrajyoti
Thank you for your comment on this patch.
On Sat, Mar 24, 2012 at 3:02 AM, Shubhrajyoti <shubhrajyoti@xxxxxx> wrote:
> Hi ,
> Some comments / doubts.
>
> On Friday 23 March 2012 11:10 AM, Tatsunosuke Tobita wrote:
>> From: Tatsunosuke Tobita <tobita.tatsunosuke@xxxxxxxxxxx>
>>
>> This adds support for Wacom Stylus device with I2C interface.
>> The modification from the previous update is replacing module_init and
>> module_exit with module_i2c_driver.
>>
>> Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@xxxxxxxxxxx>
>> ---
>> drivers/input/touchscreen/wacom_i2c.c | 238 +++++++++++++++++++++++++++++++++
>> drivers/input/touchscreen/wacom_i2c.h | 55 ++++++++
>> 2 files changed, 293 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/input/touchscreen/wacom_i2c.c
>> create mode 100644 drivers/input/touchscreen/wacom_i2c.h
>>
>> diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
>> new file mode 100644
>> index 0000000..f5a3845
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/wacom_i2c.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Wacom Penabled Driver for I2C
>> + *
>> + * Copyright (c) 2011 Tatsunosuke Tobita, Wacom.
>> + * <tobita.tatsunosuke@xxxxxxxxxxx>
>> + *
>> + * 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 of 2 of the License,
>> + * or (at your option) any later version.
>> + */
>> +
>> +#include "wacom_i2c.h"
>> +
>> +static int wacom_send_query(struct wacom_i2c *wac_i2c)
>> +{
>> + int ret;
>> + u8 cmd[4] = {CMD_QUERY0, CMD_QUERY1, CMD_QUERY2, CMD_QUERY3};
>> + u8 data[COM_COORD_NUM];
>> +
>> + ret = i2c_master_send(wac_i2c->client, cmd, sizeof(cmd));
>> + if (ret < 0) {
>> + dev_printk(KERN_ERR, &wac_i2c->client->dev,
>> + "Sending 1st Query failed \n");
> May be use dev_err . Feel free to ignore if you feel so. Just a suggestion
I guess I need to look up dev_err and decide which I should use; thanks.
>> + return -1;
>
> Why mask the error?
Masking here is because whenever wacom_send_query returns the negative
number, that means
the connection and communication between the hardware and our device
is somehow missed.
The cause was always setting of I2C of our device our our developing
environment while we were testing.
So, I thought returning the -EIO
(Error for Input/Output) is understandable for us. But, I come to
realize that this should be not based on
our thought, but should be considered for someone else, so I will
modify it as more appropriate.
>> + }
>> +
>> + cmd[0] = CMD_THROW0; cmd[1] = CMD_THROW1;
>> + ret = i2c_master_send(wac_i2c->client, cmd, 2);
>> + if (ret < 0) {
>> + dev_printk(KERN_ERR, &wac_i2c->client->dev,
>> + "Sending 2nd Query failed \n");
>> + return -1;
> same
>> + }
>> +
>> + ret = i2c_master_recv(wac_i2c->client, data, QUERY_SIZE);
>> + if (ret < 0) {
>> + dev_printk(KERN_ERR, &wac_i2c->client->dev,
>> + "Receiving data failed 1\n");
>> + return -1;
> same
>> + }
>> +
>> + wac_i2c->wac_feature.x_max = get_unaligned_le16(&data[3]);
>> + wac_i2c->wac_feature.y_max = get_unaligned_le16(&data[5]);
>> + wac_i2c->wac_feature.pressure_max = get_unaligned_le16(&data[11]);
>> + wac_i2c->wac_feature.fw_version = get_unaligned_le16(&data[13]);
>> +
>> + dev_dbg(&wac_i2c->client->dev,
>> + "x_max:%d, y_max:%d, pressure:%d, fw:%d\n",
>> + wac_i2c->wac_feature.x_max, wac_i2c->wac_feature.y_max,
>> + wac_i2c->wac_feature.pressure_max, wac_i2c->wac_feature.fw_version);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t wacom_i2c_irq(int irqno, void *param)
>> +{
>> + struct wacom_i2c *wac_i2c = param;
>> + int ret;
>> + unsigned int x, y, pressure;
>> + unsigned char tsw, f1, f2, ers;
>> + u8 data[COM_COORD_NUM];
>> +
>> + do {
>> + ret = i2c_master_recv(wac_i2c->client, data, QUERY_SIZE);
>> + } while (gpio_get_value(irq_to_gpio(wac_i2c->client->irq)) == 0);
>> +
>> + if (ret > 0) {
>> + tsw = data[3]&0x01;
>> + ers = data[3]&0x04;
>> + f1 = data[3]&0x02;
>> + f2 = data[3]&0x10;
>> + x = le16_to_cpup((__le16 *)&data[4]);
>> + y = le16_to_cpup((__le16 *)&data[6]);
>> + pressure = le16_to_cpup((__le16 *)&data[8]);
>> +
>> + input_report_abs(wac_i2c->input, ABS_X, x);
>> + input_report_abs(wac_i2c->input, ABS_Y, y);
>> + input_report_key(wac_i2c->input, BTN_TOOL_PEN, tsw);
>> + input_report_key(wac_i2c->input, BTN_TOOL_RUBBER, ers);
>> + input_report_abs(wac_i2c->input, ABS_PRESSURE, pressure);
>> + input_report_key(wac_i2c->input, BTN_TOUCH, (tsw || ers));
>> + input_report_key(wac_i2c->input, BTN_STYLUS, f1);
>> + input_report_key(wac_i2c->input, BTN_STYLUS2, f2);
>> +
>> + input_sync(wac_i2c->input);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int wacom_i2c_input_open(struct input_dev *dev)
>> +{
>> + struct wacom_i2c *wac_i2c = input_get_drvdata(dev);
>> + int ret;
>> + ret = wacom_send_query(wac_i2c);
>> + if (ret < 0)
>> + return -EIO;
>> +
>> + wac_i2c->input->id.version = wac_i2c->wac_feature.fw_version;
>> + input_set_abs_params(wac_i2c->input, ABS_X, 0,
>> + wac_i2c->wac_feature.x_max, 0, 0);
>> + input_set_abs_params(wac_i2c->input, ABS_Y, 0,
>> + wac_i2c->wac_feature.y_max, 0, 0);
>> + input_set_abs_params(wac_i2c->input, ABS_PRESSURE, 0,
>> + wac_i2c->wac_feature.pressure_max, 0, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void wacom_i2c_input_close(struct input_dev *dev)
>> +{
>> +}
> May be this could be cleaned up?
I see, so I shouldn't leave a function without any operation, right?
Leaving here is because the input_open and input_close are considered to
be registered as set. I will delete this function. Thanks!
>> +
>> +static int __devinit wacom_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct wacom_i2c *wac_i2c;
>> + int err;
>> + u8 data[COM_COORD_NUM];
>> +
>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> + err = -EIO;
>> + goto fail1;
>> + }
>> +
>> + wac_i2c = kzalloc(sizeof(*wac_i2c), GFP_KERNEL);
>> + if (wac_i2c == NULL) {
>> + err = -ENOMEM;
>> + goto fail2;
>> + }
>> +
>> + wac_i2c->input = input_allocate_device();
>> + if (wac_i2c->input == NULL) {
>> + err = -ENOMEM;
>> + goto fail2;
> We are leaking wac_i2c here ?
Sorry, can you tell more detail?
I guess you are talking about the registration of wac_i2c->input when
it errors, but not completely sure if
you mean either input registration or wac_i2c itself, or both.
>> + }
>> +
>> + wac_i2c->client = client;
>> + i2c_set_clientdata(client, wac_i2c);
>> + mutex_init(&wac_i2c->lock);
>> +
>> + __set_bit(BTN_TOOL_PEN, wac_i2c->input->keybit);
>> + __set_bit(BTN_TOOL_RUBBER, wac_i2c->input->keybit);
>> + __set_bit(BTN_STYLUS, wac_i2c->input->keybit);
>> + __set_bit(BTN_STYLUS2, wac_i2c->input->keybit);
>> + __set_bit(BTN_TOUCH, wac_i2c->input->keybit);
>> +
>> + wac_i2c->input->name = "Wacom I2C Digitizer";
>> + wac_i2c->input->id.bustype = BUS_I2C;
>> + wac_i2c->input->id.vendor = 0x56a;
>> + wac_i2c->input->dev.parent = &client->dev;
>> + wac_i2c->input->open = wacom_i2c_input_open;
>> + wac_i2c->input->close = wacom_i2c_input_close;
>> + wac_i2c->input->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> + input_set_drvdata(wac_i2c->input, wac_i2c);
>> +
>> + if (input_register_device(wac_i2c->input)) {
>> + dev_printk(KERN_ERR, &wac_i2c->client->dev, "Failed to register input device \n");
>> + err = -ENODEV;
>> + goto fail3;
>> + }
>> +
>> +
>> + err = request_threaded_irq(wac_i2c->client->irq, NULL,
>> + wacom_i2c_irq, IRQF_TRIGGER_FALLING, "WACOM_I2C_IRQ", wac_i2c);
>> + if (err) {
>> + dev_printk(KERN_ERR, &wac_i2c->client->dev, "Failed to enable IRQ for EMR \n");
>> + goto fail3;
>> + }
>> +
>> + /*Clear the buffer in the firmware*/
>> + do {
>> + i2c_master_recv(wac_i2c->client, data, QUERY_SIZE);
>> + } while (gpio_get_value(irq_to_gpio(wac_i2c->client->irq)) == 0);
>> +
>> + return 0;
>> +
>> + fail3:
>> + input_free_device(wac_i2c->input);
>> + fail2:
>> + dev_err(&wac_i2c->client->dev, "Freeing device \n");
>> + fail1:
>> + return err;
>> +}
>> +
>> +static int __devexit wacom_i2c_remove(struct i2c_client *client)
>> +{
>> + struct wacom_i2c *wac_i2c = i2c_get_clientdata(client);
>> +
>> + free_irq(wac_i2c->client->irq, wac_i2c);
>> + input_unregister_device(wac_i2c->input);
>> + kfree(wac_i2c);
>> +
>> + return 0;
>> +}
>> +
>> +static int wacom_i2c_suspend(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + disable_irq(client->irq);
> Why is this disable needed?
Our device especially for the tablet PCs, sometimes we were asked to implement
for entering the mode that reduces the cost of the electric current,
which is similar to sleep mode
when systems are in suspend. The waking up from this mode are easy;
just get our stylus close to the screen
even when the systems in suspend mode. However, the waking up
shouldn't happen when the systems in the suspend mode,
so disabling irq allows our device not to detect that our pen is
getting close to it since the irq is the line detecting our stylus's
signal.
We haven't used this mode a lot these days, but not sure what will be
in the future.
As a result, I leave the operation for disabling irq here.
>> +
>> + return 0;
>> +}
>> +
>> +static int wacom_i2c_resume(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + enable_irq(client->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(wacom_i2c_pm, wacom_i2c_suspend, wacom_i2c_resume);
>> +
>> +static const struct i2c_device_id wacom_i2c_id[] = {
>> + {WACNAME, 0},
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, wacom_i2c_id);
>> +
>> +static struct i2c_driver wacom_i2c_driver = {
>> + .driver = {
>> + .name = "wacom_i2c",
>> + .owner = THIS_MODULE,
>> + .pm = &wacom_i2c_pm,
>> + },
>> +
>> + .probe = wacom_i2c_probe,
>> + .remove = __devexit_p(wacom_i2c_remove),
>> + .id_table = wacom_i2c_id,
>> +};
>> +module_i2c_driver(wacom_i2c_driver);
>> +
>> +MODULE_AUTHOR("Tatsunosuke Tobita <tobita.tatsunosuke@xxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("WACOM EMR I2C Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/input/touchscreen/wacom_i2c.h b/drivers/input/touchscreen/wacom_i2c.h
>> new file mode 100644
>> index 0000000..b5c2d07
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/wacom_i2c.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Wacom Penabled Driver for I2C
>> + *
>> + * Copyright (c) 2011 Tatsunosuke Tobita, Wacom.
>> +
> nit : 2012?
I almost forget I sign it last year.
Thanks!
Tats
--
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
