[PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Christian Gmeiner]

This driver is quite simple and only supports the Touch
Reporting Protocol.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 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.
+
 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@gmail.com>
+ *
+ * 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();
+	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() */
+	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);
+	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;
+}
+
+#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);
+
+	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@gmail.com>");
+MODULE_DESCRIPTION("Microchip AR1021 I2C Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ar1021_i2c");
-- 
1.7.10.4

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Dmitry Torokhov]

Hi Christian,

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.

> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  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

> +
>  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@gmail.com>
> + *
> + * 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.

> +	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...

> +	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);
> +	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.

> +
> +#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).

> +
> +	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@gmail.com>");
> +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.

Thanks.

-- 
Dmitry

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Christian Gmeiner]

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@gmail.com>
>> ---
>>  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@gmail.com>
>> + *
>> + * 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@gmail.com>");
>> +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

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Dmitry Torokhov]

Hi Chrisitian,

On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
> >> --- /dev/null
> >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> >> @@ -0,0 +1,201 @@
> >> +/*
> >> + * Microchip AR1021 driver for I2C
> >> + *
> >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
> >> + *
> >> + * License: GPL as published by the FSF.

By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
or GPL v2 and later (depending on your preference and the license of the
code you used as a base)?

> >> +
> >> +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?

No, I think you still need the PM methods, you just need to check if
device is opened (take dev->mutex, check dev->users) and decide if you
need to enable/disable IRQ or not.

-- 
Dmitry

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Dmitry Torokhov]

On Fri, Jan 31, 2014 at 09:15:21AM -0800, Dmitry Torokhov wrote:
> Hi Chrisitian,
> 
> On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
> > >> --- /dev/null
> > >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> > >> @@ -0,0 +1,201 @@
> > >> +/*
> > >> + * Microchip AR1021 driver for I2C
> > >> + *
> > >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
> > >> + *
> > >> + * License: GPL as published by the FSF.
> 
> By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
> or GPL v2 and later (depending on your preference and the license of the
> code you used as a base)?
> 
> > >> +
> > >> +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?
> 
> No, I think you still need the PM methods, you just need to check if
> device is opened (take dev->mutex, check dev->users) and decide if you
> need to enable/disable IRQ or not.

Hmm, on the other hand enable/disable does the counting for you so maybe
you should leave it all as it was.

-- 
Dmitry

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Christian Gmeiner]

Hi Dmitry

2014-01-31 18:16 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Fri, Jan 31, 2014 at 09:15:21AM -0800, Dmitry Torokhov wrote:
>> Hi Chrisitian,
>>
>> On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
>> > >> --- /dev/null
>> > >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
>> > >> @@ -0,0 +1,201 @@
>> > >> +/*
>> > >> + * Microchip AR1021 driver for I2C
>> > >> + *
>> > >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
>> > >> + *
>> > >> + * License: GPL as published by the FSF.
>>
>> By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
>> or GPL v2 and later (depending on your preference and the license of the
>> code you used as a base)?
>>
>> > >> +
>> > >> +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?
>>
>> No, I think you still need the PM methods, you just need to check if
>> device is opened (take dev->mutex, check dev->users) and decide if you
>> need to enable/disable IRQ or not.
>
> Hmm, on the other hand enable/disable does the counting for you so maybe
> you should leave it all as it was.

ok

At the moment I am doing the final tests of the driver and it fails to
load via device tree :(
I changed the compatible string from "mc,ar1021-i2c" to "microchip,ar1021-i2c".

and this is how my dts looks like on an imx6d board:

&i2c2 {
       status = "okay";
       clock-frequency = <100000>;
       pinctrl-names = "default";
       pinctrl-0 = <&pinctrl_i2c2_1>;

       ar1021@4d {
               compatible = "microchip,ar1021-i2c";
               reg = <0x4d>;
               interrupt-parent = <&gpio3>;
               interrupts = <26 0x2>;
       };
};


Any hints?
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner

Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Dmitry Torokhov]

On Tue, Feb 11, 2014 at 02:10:50PM +0100, Christian Gmeiner wrote:
> Hi Dmitry
> 
> 2014-01-31 18:16 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > On Fri, Jan 31, 2014 at 09:15:21AM -0800, Dmitry Torokhov wrote:
> >> Hi Chrisitian,
> >>
> >> On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
> >> > >> --- /dev/null
> >> > >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> >> > >> @@ -0,0 +1,201 @@
> >> > >> +/*
> >> > >> + * Microchip AR1021 driver for I2C
> >> > >> + *
> >> > >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
> >> > >> + *
> >> > >> + * License: GPL as published by the FSF.
> >>
> >> By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
> >> or GPL v2 and later (depending on your preference and the license of the
> >> code you used as a base)?
> >>
> >> > >> +
> >> > >> +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?
> >>
> >> No, I think you still need the PM methods, you just need to check if
> >> device is opened (take dev->mutex, check dev->users) and decide if you
> >> need to enable/disable IRQ or not.
> >
> > Hmm, on the other hand enable/disable does the counting for you so maybe
> > you should leave it all as it was.
> 
> ok
> 
> At the moment I am doing the final tests of the driver and it fails to
> load via device tree :(
> I changed the compatible string from "mc,ar1021-i2c" to "microchip,ar1021-i2c".
> 
> and this is how my dts looks like on an imx6d board:
> 
> &i2c2 {
>        status = "okay";
>        clock-frequency = <100000>;
>        pinctrl-names = "default";
>        pinctrl-0 = <&pinctrl_i2c2_1>;
> 
>        ar1021@4d {
>                compatible = "microchip,ar1021-i2c";
>                reg = <0x4d>;
>                interrupt-parent = <&gpio3>;
>                interrupts = <26 0x2>;
>        };
> };
> 
> 
> Any hints?

Not really. Does the driver bind to the device if you define I2C device
in the board code? Are there any errors reported by the kernel?

Thanks.

-- 
Dmitry