Hi Dmitry.
> On Thu, Jan 30, 2014 at 10:29:45AM +0100, Christian Gmeiner wrote:
>> This driver is quite simple and only supports the Touch
>> Reporting Protocol.
>
> Pretty clean and neat, just a few comments.
>
Thanks for the review.
>>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
>> ---
>> drivers/input/touchscreen/Kconfig | 12 ++
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/ar1021_i2c.c | 201 ++++++++++++++++++++++++++++++++
>> 3 files changed, 214 insertions(+)
>> create mode 100644 drivers/input/touchscreen/ar1021_i2c.c
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 07e9e82..15cc9a1 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -86,6 +86,18 @@ config TOUCHSCREEN_AD7879_SPI
>> To compile this driver as a module, choose M here: the
>> module will be called ad7879-spi.
>>
>> +config TOUCHSCREEN_AR1021_I2C
>> + tristate "Microchip AR1021 i2c touchscreen"
>> + depends on I2C && OF
>> + help
>> + Say Y here if you have the Microchip AR1021 touchscreen controller
>> + chip in your system.
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called microchip_ar1021_i2c.
>
> s/microchip_ar1021_i2c/ar1021_i2c
>
ups
>> +
>> config TOUCHSCREEN_ATMEL_MXT
>> tristate "Atmel mXT I2C Touchscreen"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 62801f2..efaa328 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879) += ad7879.o
>> obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C) += ad7879-i2c.o
>> obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI) += ad7879-spi.o
>> obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
>> +obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
>> obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC) += atmel_tsadcc.o
>> obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
>> diff --git a/drivers/input/touchscreen/ar1021_i2c.c b/drivers/input/touchscreen/ar1021_i2c.c
>> new file mode 100644
>> index 0000000..c77ce05
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/ar1021_i2c.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Microchip AR1021 driver for I2C
>> + *
>> + * Author: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
>> + *
>> + * License: GPL as published by the FSF.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/input.h>
>> +#include <linux/of.h>
>> +#include <linux/i2c.h>
>> +#include <linux/slab.h>
>> +#include <linux/irq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/gpio.h>
>> +#include <asm/unaligned.h>
>> +
>> +#define AR1021_TOCUH_PKG_SIZE 5
>> +
>> +struct ar1021_i2c {
>> + struct i2c_client *client;
>> + struct input_dev *input;
>> + u8 data[AR1021_TOCUH_PKG_SIZE];
>> +};
>> +
>> +static irqreturn_t ar1021_i2c_irq(int irq, void *dev_id)
>> +{
>> + struct ar1021_i2c *ar1021 = dev_id;
>> + struct input_dev *input = ar1021->input;
>> + u8 *data = ar1021->data;
>> + unsigned int x, y, button;
>> + int error;
>> +
>> + error = i2c_master_recv(ar1021->client,
>> + ar1021->data, sizeof(ar1021->data));
>> + if (error < 0)
>> + goto out;
>> +
>> + button = !(data[0] & BIT(0));
>> + x = ((data[2] & 0x1f) << 7) | (data[1] & 0x7f);
>> + y = ((data[4] & 0x1f) << 7) | (data[3] & 0x7f);
>> +
>> + input_report_key(input, BTN_TOUCH, button);
>> + input_report_abs(input, ABS_X, x);
>> + input_report_abs(input, ABS_Y, y);
>> + input_sync(input);
>> +
>> +out:
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int ar1021_i2c_open(struct input_dev *dev)
>> +{
>> + struct ar1021_i2c *wac_i2c = input_get_drvdata(dev);
>> + struct i2c_client *client = wac_i2c->client;
>> +
>> + enable_irq(client->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static void ar1021_i2c_close(struct input_dev *dev)
>> +{
>> + struct ar1021_i2c *wac_i2c = input_get_drvdata(dev);
>> + struct i2c_client *client = wac_i2c->client;
>> +
>> + disable_irq(client->irq);
>> +}
>> +
>> +static int ar1021_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct ar1021_i2c *ar1021;
>> + struct input_dev *input;
>> + int error;
>> +
>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> + dev_err(&client->dev, "i2c_check_functionality error\n");
>> + return -EIO;
>> + }
>> +
>> + ar1021 = devm_kzalloc(client->dev, sizeof(*ar1021), GFP_KERNEL);
>> + input = input_allocate_device();
>
> Use devm_input_allocate_device() and later devm_request_threaded_irq()
> as well.
>
Thats a very good idea...
>> + if (!ar1021 || !input) {
>> + error = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> + ar1021->client = client;
>> + ar1021->input = input;
>> +
>> + input->name = "ar1021 I2C Touchscreen";
>> + input->id.bustype = BUS_I2C;
>> + input->dev.parent = &client->dev;
>> + input->open = ar1021_i2c_open;
>> + input->close = ar1021_i2c_close;
>> +
>> + input->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> +
>> + __set_bit(BTN_TOOL_PEN, input->keybit);
>> +
>> + input_set_abs_params(input, ABS_X, 0, 4095, 0, 0);
>> + input_set_abs_params(input, ABS_Y, 0, 4095, 0, 0);
>> +
>> + input_set_drvdata(input, ar1021);
>> +
>> + error = request_threaded_irq(client->irq, NULL, ar1021_i2c_irq,
>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> + "ar1021_i2c", ar1021);
>> + if (error) {
>> + dev_err(&client->dev,
>> + "Failed to enable IRQ, error: %d\n", error);
>> + goto err_free_mem;
>> + }
>> +
>> + /* Disable the IRQ, we'll enable it in wac_i2c_open() */
>
> No, not in wac_i2c_open ;) It looks like you might want to update
> copyright notice to mentioned what driver you used as a base...
>
Will do :)
>> + disable_irq(client->irq);
>> +
>> + error = input_register_device(ar1021->input);
>> + if (error) {
>> + dev_err(&client->dev,
>> + "Failed to register input device, error: %d\n", error);
>> + goto err_free_irq;
>> + }
>> +
>> + i2c_set_clientdata(client, ar1021);
Do I need the i2c_set_clientdata(..) call at all?
>> + return 0;
>> +
>> +err_free_irq:
>> + free_irq(client->irq, ar1021);
>> +err_free_mem:
>> + input_free_device(input);
>> +
>> + return error;
>> +}
>> +
>> +static int ar1021_i2c_remove(struct i2c_client *client)
>> +{
>> + struct ar1021_i2c *ar1021 = i2c_get_clientdata(client);
>> +
>> + free_irq(client->irq, ar1021);
>> + input_unregister_device(ar1021->input);
>> +
>> + return 0;
>> +}
>
> If you use devm throughout you won't need ar1021_i2c_remove method at
> all.
>
Correct.. will do it in the final patch.
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int ar1021_i2c_suspend(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> +
>> + disable_irq(client->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static int ar1021_i2c_resume(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> +
>> + enable_irq(client->irq);
>
> You do not want to enable IRQ if there are no users (nobody opened
> device).
>
Okay.. but then I also do not need the disable_irq(..) call in
ar1021_i2c_suspend
and can totally remove the PM stuff - or?
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(ar1021_i2c_pm, ar1021_i2c_suspend, ar1021_i2c_resume);
>> +
>> +static const struct i2c_device_id ar1021_i2c_id[] = {
>> + { "MICROCHIP_AR1021_I2C", 0 },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ar1021_i2c_id);
>> +
>> +#ifdef CONFIG_OF
>> +static struct of_device_id ar1021_i2c_of_match[] = {
>> + { .compatible = "mc,ar1021-i2c", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, ar1021_i2c_of_match);
>> +#endif
>> +
>> +static struct i2c_driver ar1021_i2c_driver = {
>> + .driver = {
>> + .name = "ar1021_i2c",
>> + .owner = THIS_MODULE,
>> + .pm = &ar1021_i2c_pm,
>> + .of_match_table = of_match_ptr(ar1021_i2c_of_match),
>> + },
>> +
>> + .probe = ar1021_i2c_probe,
>> + .remove = ar1021_i2c_remove,
>> + .id_table = ar1021_i2c_id,
>> +};
>> +module_i2c_driver(ar1021_i2c_driver);
>> +
>> +MODULE_AUTHOR("Christian Gmeiner <christian.gmeiner@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("Microchip AR1021 I2C Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:ar1021_i2c");
>
> Why platform if it is I2C driver? This MODULE_ALIAS is not needed at
> all.
Ok
Thanks
--
Christian Gmeiner, MSc
--
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
