[PATCH 0/9] Input: pixcir_i2c_ts: Add Type-B Multitouch support

[PATCH 0/9] Input: pixcir_i2c_ts: Add Type-B Multitouch support

[Roger Quadros]

Hi,

Some variants of the Pixcir I2C touch controller are more suitable for
Type-B multi-touch reporting (e.g. Tango C). This series enhances the driver
to support Type-B multi-touch reports. It also adds device tree support
and power management.

cheers,
-roger

Roger Quadros (9):
  Input: pixcir_i2c_ts: Add device tree support
  Input: pixcir_i2c_ts: Add register definitions
  Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
  Input: pixcir_i2c_ts: Use devres managed resource allocations
  Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
  Input: pixcir_i2c_ts: Add chip specific data structure
  Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
  Input: pixcir_i2c_ts: Add support for TangoC family
  Input: pixcir_i2c_ts: Implement wakeup from suspend

 .../bindings/input/touchscreen/pixcir_i2c_ts.txt   |  26 +
 drivers/input/touchscreen/pixcir_i2c_ts.c          | 570 +++++++++++++++++----
 include/linux/input/pixcir_ts.h                    |  59 ++-
 3 files changed, 564 insertions(+), 91 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt

-- 
1.8.3.2

[PATCH 0/9] Input: pixcir_i2c_ts: Add Type-B Multitouch support

[Roger Quadros]

Hi,

Some variants of the Pixcir I2C touch controller are more suitable for
Type-B multi-touch reporting (e.g. Tango C). This series enhances the driver
to support Type-B multi-touch reports. It also adds device tree support
and power management.

cheers,
-roger

Roger Quadros (9):
  Input: pixcir_i2c_ts: Add device tree support
  Input: pixcir_i2c_ts: Add register definitions
  Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
  Input: pixcir_i2c_ts: Use devres managed resource allocations
  Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
  Input: pixcir_i2c_ts: Add chip specific data structure
  Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
  Input: pixcir_i2c_ts: Add support for TangoC family
  Input: pixcir_i2c_ts: Implement wakeup from suspend

 .../bindings/input/touchscreen/pixcir_i2c_ts.txt   |  26 +
 drivers/input/touchscreen/pixcir_i2c_ts.c          | 570 +++++++++++++++++----
 include/linux/input/pixcir_ts.h                    |  59 ++-
 3 files changed, 564 insertions(+), 91 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt

-- 
1.8.3.2

[PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Roger Quadros]

Provide device tree support and binding information.
Change platform data parameters from x/y_max to x/y_size..

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
 drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
 include/linux/input/pixcir_ts.h                    |  5 +-
 3 files changed, 101 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
new file mode 100644
index 0000000..c0b0b270
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
@@ -0,0 +1,26 @@
+* Pixcir I2C touchscreen controllers
+
+Required properties:
+- compatible: must be "pixcir,pixcir_ts"
+- reg: I2C address of the chip
+- interrupts: interrupt to which the chip is connected
+- attb-gpio: GPIO connected to the ATTB line of the chip
+- x-size: horizontal resolution of touchscreen
+- y-size: vertical resolution of touchscreen
+
+Example:
+
+	i2c@00000000 {
+		/* ... */
+
+		pixcir_ts@5c {
+			compatible = "pixcir,pixcir_ts";
+			reg = <0x5c>;
+			interrupts = <2 0>;
+			attb-gpio = <&gpf 2 0 2>;
+			x-size = <800>;
+			y-size = <600>;
+		};
+
+		/* ... */
+	};
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 6cc6b36..3a447c9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -24,6 +24,10 @@
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/input/pixcir_ts.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
 
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
@@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
 			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
 
+#if defined(CONFIG_OF)
+static const struct of_device_id pixcir_of_match[];
+
+static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
+{
+	struct pixcir_ts_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
+	if (!match)
+		return ERR_PTR(-EINVAL);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
+	if (!gpio_is_valid(pdata->gpio_attb))
+		dev_err(dev, "Failed to get ATTB GPIO\n");
+
+	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
+		dev_err(dev, "Failed to get x-size property\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
+		dev_err(dev, "Failed to get y-size property\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
+				pdata->x_size, pdata->y_size, pdata->gpio_attb);
+
+	return pdata;
+}
+#else
+static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int pixcir_i2c_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
 	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node;
 	struct pixcir_i2c_ts_data *tsdata;
 	struct input_dev *input;
 	int error;
 
-	if (!pdata) {
+	if (np) {
+		pdata = pixcir_parse_dt(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+
+	} else if (!pdata) {
 		dev_err(&client->dev, "platform data not defined\n");
 		return -EINVAL;
 	}
@@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(BTN_TOUCH, input->keybit);
-	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
-	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
+	input_set_abs_params(input, ABS_X,
+					0, pdata->x_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_Y,
+					0, pdata->y_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_X,
+					0, pdata->x_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_Y,
+					0, pdata->y_size - 1, 0, 0);
 
 	input_set_drvdata(input, tsdata);
 
@@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
 
+#if defined(CONFIG_OF)
+static const struct of_device_id pixcir_of_match[] = {
+	{ .compatible = "pixcir,pixcir_ts", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pixcir_of_match);
+#endif
+
 static struct i2c_driver pixcir_i2c_ts_driver = {
 	.driver = {
 		.owner	= THIS_MODULE,
 		.name	= "pixcir_ts",
 		.pm	= &pixcir_dev_pm_ops,
+		.of_match_table = of_match_ptr(pixcir_of_match),
 	},
 	.probe		= pixcir_i2c_ts_probe,
 	.remove		= pixcir_i2c_ts_remove,
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 7163d91..b34ff7e 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -3,8 +3,9 @@
 
 struct pixcir_ts_platform_data {
 	int (*attb_read_val)(void);
-	int x_max;
-	int y_max;
+	unsigned int x_size;	/* X axis resolution */
+	unsigned int y_size;	/* Y axis resolution */
+	int gpio_attb;		/* GPIO connected to ATTB line */
 };
 
 #endif
-- 
1.8.3.2

[PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Roger Quadros]

Provide device tree support and binding information.
Change platform data parameters from x/y_max to x/y_size..

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
 drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
 include/linux/input/pixcir_ts.h                    |  5 +-
 3 files changed, 101 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
new file mode 100644
index 0000000..c0b0b270
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
@@ -0,0 +1,26 @@
+* Pixcir I2C touchscreen controllers
+
+Required properties:
+- compatible: must be "pixcir,pixcir_ts"
+- reg: I2C address of the chip
+- interrupts: interrupt to which the chip is connected
+- attb-gpio: GPIO connected to the ATTB line of the chip
+- x-size: horizontal resolution of touchscreen
+- y-size: vertical resolution of touchscreen
+
+Example:
+
+	i2c@00000000 {
+		/* ... */
+
+		pixcir_ts@5c {
+			compatible = "pixcir,pixcir_ts";
+			reg = <0x5c>;
+			interrupts = <2 0>;
+			attb-gpio = <&gpf 2 0 2>;
+			x-size = <800>;
+			y-size = <600>;
+		};
+
+		/* ... */
+	};
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 6cc6b36..3a447c9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -24,6 +24,10 @@
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/input/pixcir_ts.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
 
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
@@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
 			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
 
+#if defined(CONFIG_OF)
+static const struct of_device_id pixcir_of_match[];
+
+static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
+{
+	struct pixcir_ts_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
+	if (!match)
+		return ERR_PTR(-EINVAL);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
+	if (!gpio_is_valid(pdata->gpio_attb))
+		dev_err(dev, "Failed to get ATTB GPIO\n");
+
+	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
+		dev_err(dev, "Failed to get x-size property\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
+		dev_err(dev, "Failed to get y-size property\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
+				pdata->x_size, pdata->y_size, pdata->gpio_attb);
+
+	return pdata;
+}
+#else
+static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int pixcir_i2c_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
 	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node;
 	struct pixcir_i2c_ts_data *tsdata;
 	struct input_dev *input;
 	int error;
 
-	if (!pdata) {
+	if (np) {
+		pdata = pixcir_parse_dt(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+
+	} else if (!pdata) {
 		dev_err(&client->dev, "platform data not defined\n");
 		return -EINVAL;
 	}
@@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(BTN_TOUCH, input->keybit);
-	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
-	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
+	input_set_abs_params(input, ABS_X,
+					0, pdata->x_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_Y,
+					0, pdata->y_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_X,
+					0, pdata->x_size - 1, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_Y,
+					0, pdata->y_size - 1, 0, 0);
 
 	input_set_drvdata(input, tsdata);
 
@@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
 
+#if defined(CONFIG_OF)
+static const struct of_device_id pixcir_of_match[] = {
+	{ .compatible = "pixcir,pixcir_ts", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pixcir_of_match);
+#endif
+
 static struct i2c_driver pixcir_i2c_ts_driver = {
 	.driver = {
 		.owner	= THIS_MODULE,
 		.name	= "pixcir_ts",
 		.pm	= &pixcir_dev_pm_ops,
+		.of_match_table = of_match_ptr(pixcir_of_match),
 	},
 	.probe		= pixcir_i2c_ts_probe,
 	.remove		= pixcir_i2c_ts_remove,
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 7163d91..b34ff7e 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -3,8 +3,9 @@
 
 struct pixcir_ts_platform_data {
 	int (*attb_read_val)(void);
-	int x_max;
-	int y_max;
+	unsigned int x_size;	/* X axis resolution */
+	unsigned int y_size;	/* Y axis resolution */
+	int gpio_attb;		/* GPIO connected to ATTB line */
 };
 
 #endif
-- 
1.8.3.2

[PATCH 2/9] Input: pixcir_i2c_ts: Add register definitions

[Roger Quadros]

Add power and interrupt register definitions.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 include/linux/input/pixcir_ts.h | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index b34ff7e..f17c192 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -1,6 +1,48 @@
 #ifndef	_PIXCIR_I2C_TS_H
 #define	_PIXCIR_I2C_TS_H
 
+/*
+ * Register map
+ */
+#define PIXCIR_REG_POWER_MODE	51
+#define PIXCIR_REG_INT_MODE	52
+
+/*
+ * Power modes:
+ * active: max scan speed
+ * idle: lower scan speed with automatic transition to active on touch
+ * halt: datasheet says sleep but this is more like halt as the chip
+ *       clocks are cut and it can only be brought out of this mode
+ *	 using the RESET pin.
+ */
+enum pixcir_power_mode {
+	PIXCIR_POWER_ACTIVE,
+	PIXCIR_POWER_IDLE,
+	PIXCIR_POWER_HALT,
+};
+
+#define PIXCIR_POWER_MODE_MASK	0x03
+#define PIXCIR_POWER_ALLOW_IDLE (1UL << 2)
+
+/*
+ * Interrupt modes:
+ * periodical: interrupt is asserted periodicaly
+ * diff coordinates: interrupt is asserted when coordinates change
+ * level on touch: interrupt level asserted during touch
+ * pulse on touch: interrupt pulse asserted druing touch
+ *
+ */
+enum pixcir_int_mode {
+	PIXCIR_INT_PERIODICAL,
+	PIXCIR_INT_DIFF_COORD,
+	PIXCIR_INT_LEVEL_TOUCH,
+	PIXCIR_INT_PULSE_TOUCH,
+};
+
+#define PIXCIR_INT_MODE_MASK	0x03
+#define PIXCIR_INT_ENABLE	(1UL << 3)
+#define PIXCIR_INT_POL_HIGH	(1UL << 2)
+
 struct pixcir_ts_platform_data {
 	int (*attb_read_val)(void);
 	unsigned int x_size;	/* X axis resolution */
-- 
1.8.3.2

[PATCH 2/9] Input: pixcir_i2c_ts: Add register definitions

[Roger Quadros]

Add power and interrupt register definitions.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 include/linux/input/pixcir_ts.h | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index b34ff7e..f17c192 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -1,6 +1,48 @@
 #ifndef	_PIXCIR_I2C_TS_H
 #define	_PIXCIR_I2C_TS_H
 
+/*
+ * Register map
+ */
+#define PIXCIR_REG_POWER_MODE	51
+#define PIXCIR_REG_INT_MODE	52
+
+/*
+ * Power modes:
+ * active: max scan speed
+ * idle: lower scan speed with automatic transition to active on touch
+ * halt: datasheet says sleep but this is more like halt as the chip
+ *       clocks are cut and it can only be brought out of this mode
+ *	 using the RESET pin.
+ */
+enum pixcir_power_mode {
+	PIXCIR_POWER_ACTIVE,
+	PIXCIR_POWER_IDLE,
+	PIXCIR_POWER_HALT,
+};
+
+#define PIXCIR_POWER_MODE_MASK	0x03
+#define PIXCIR_POWER_ALLOW_IDLE (1UL << 2)
+
+/*
+ * Interrupt modes:
+ * periodical: interrupt is asserted periodicaly
+ * diff coordinates: interrupt is asserted when coordinates change
+ * level on touch: interrupt level asserted during touch
+ * pulse on touch: interrupt pulse asserted druing touch
+ *
+ */
+enum pixcir_int_mode {
+	PIXCIR_INT_PERIODICAL,
+	PIXCIR_INT_DIFF_COORD,
+	PIXCIR_INT_LEVEL_TOUCH,
+	PIXCIR_INT_PULSE_TOUCH,
+};
+
+#define PIXCIR_INT_MODE_MASK	0x03
+#define PIXCIR_INT_ENABLE	(1UL << 3)
+#define PIXCIR_INT_POL_HIGH	(1UL << 2)
+
 struct pixcir_ts_platform_data {
 	int (*attb_read_val)(void);
 	unsigned int x_size;	/* X axis resolution */
-- 
1.8.3.2

[PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Roger Quadros]

Introduce helper functions to configure power and interrupt
registers. Default to IDLE mode on probe as device supports
auto wakeup to ACVIE mode on detecting finger touch.

Configure interrupt mode and polarity on start up.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 168 ++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3a447c9..ce8abcd 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -104,6 +104,160 @@ static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int pixcir_set_power_mode(struct pixcir_i2c_ts_data *ts,
+						enum pixcir_power_mode mode)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_POWER_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_POWER_MODE, ret);
+		return ret;
+	}
+
+	ret &= ~PIXCIR_POWER_MODE_MASK;
+	ret |= mode;
+
+	/* Always AUTO_IDLE */
+	ret |= PIXCIR_POWER_ALLOW_IDLE;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_POWER_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_POWER_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Set the interrupt mode for the device i.e. ATTB line behaviour
+ *
+ * @polarity : 1 for active high, 0 for active low.
+ */
+static int pixcir_set_int_mode(struct pixcir_i2c_ts_data *ts,
+						enum pixcir_int_mode mode,
+					bool polarity)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	ret &= ~PIXCIR_INT_MODE_MASK;
+	ret |= mode;
+
+	if (polarity)
+		ret |= PIXCIR_INT_POL_HIGH;
+	else
+		ret &= ~PIXCIR_INT_POL_HIGH;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Enable/disable interrupt generation
+ */
+static int pixcir_int_enable(struct pixcir_i2c_ts_data *ts, bool enable)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	if (enable)
+		ret |= PIXCIR_INT_ENABLE;
+	else
+		ret &= ~PIXCIR_INT_ENABLE;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pixcir_start(struct pixcir_i2c_ts_data *ts)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	/* LEVEL_TOUCH interrupt with active low polarity */
+	ret = pixcir_set_int_mode(ts, PIXCIR_INT_LEVEL_TOUCH, 0);
+	if (ret) {
+		dev_err(dev, "Failed to set interrupt mode\n");
+		return ret;
+	}
+
+	enable_irq(ts->client->irq);
+
+	/* enable interrupt generation */
+	ret = pixcir_int_enable(ts, 1);
+	if (ret) {
+		dev_err(dev, "Failed to enable interrupt generation\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	/* disable interrupt generation */
+	ret = pixcir_int_enable(ts, 0);
+	if (ret) {
+		dev_err(dev, "Failed to disable interrupt generation\n");
+		return ret;
+	}
+
+	disable_irq(ts->client->irq);
+
+	return 0;
+}
+
+static int pixcir_input_open(struct input_dev *dev)
+{
+	struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+	return pixcir_start(ts);
+}
+
+static void pixcir_input_close(struct input_dev *dev)
+{
+	struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+	pixcir_stop(ts);
+
+	return;
+}
+
+
 #ifdef CONFIG_PM_SLEEP
 static int pixcir_i2c_ts_suspend(struct device *dev)
 {
@@ -207,6 +361,8 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input->name = client->name;
 	input->id.bustype = BUS_I2C;
 	input->dev.parent = &client->dev;
+	input->open = pixcir_input_open;
+	input->close = pixcir_input_close;
 
 	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
@@ -230,6 +386,18 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 		goto err_free_mem;
 	}
 
+	/* Always be in IDLE mode to save power, device supports auto wake */
+	error = pixcir_set_power_mode(tsdata, PIXCIR_POWER_IDLE);
+	if (error) {
+		dev_err(dev, "Failed to set IDLE mode\n");
+		return error;
+	}
+
+	/* Stop device till opened */
+	error = pixcir_stop(tsdata);
+	if (error)
+		goto err_free_irq;
+
 	error = input_register_device(input);
 	if (error)
 		goto err_free_irq;
-- 
1.8.3.2

[PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Roger Quadros]

Introduce helper functions to configure power and interrupt
registers. Default to IDLE mode on probe as device supports
auto wakeup to ACVIE mode on detecting finger touch.

Configure interrupt mode and polarity on start up.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 168 ++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3a447c9..ce8abcd 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -104,6 +104,160 @@ static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int pixcir_set_power_mode(struct pixcir_i2c_ts_data *ts,
+						enum pixcir_power_mode mode)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_POWER_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_POWER_MODE, ret);
+		return ret;
+	}
+
+	ret &= ~PIXCIR_POWER_MODE_MASK;
+	ret |= mode;
+
+	/* Always AUTO_IDLE */
+	ret |= PIXCIR_POWER_ALLOW_IDLE;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_POWER_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_POWER_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Set the interrupt mode for the device i.e. ATTB line behaviour
+ *
+ * @polarity : 1 for active high, 0 for active low.
+ */
+static int pixcir_set_int_mode(struct pixcir_i2c_ts_data *ts,
+						enum pixcir_int_mode mode,
+					bool polarity)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	ret &= ~PIXCIR_INT_MODE_MASK;
+	ret |= mode;
+
+	if (polarity)
+		ret |= PIXCIR_INT_POL_HIGH;
+	else
+		ret &= ~PIXCIR_INT_POL_HIGH;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Enable/disable interrupt generation
+ */
+static int pixcir_int_enable(struct pixcir_i2c_ts_data *ts, bool enable)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	if (enable)
+		ret |= PIXCIR_INT_ENABLE;
+	else
+		ret &= ~PIXCIR_INT_ENABLE;
+
+	ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+	if (ret < 0) {
+		dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+					__func__, PIXCIR_REG_INT_MODE, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pixcir_start(struct pixcir_i2c_ts_data *ts)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	/* LEVEL_TOUCH interrupt with active low polarity */
+	ret = pixcir_set_int_mode(ts, PIXCIR_INT_LEVEL_TOUCH, 0);
+	if (ret) {
+		dev_err(dev, "Failed to set interrupt mode\n");
+		return ret;
+	}
+
+	enable_irq(ts->client->irq);
+
+	/* enable interrupt generation */
+	ret = pixcir_int_enable(ts, 1);
+	if (ret) {
+		dev_err(dev, "Failed to enable interrupt generation\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
+{
+	struct device *dev = &ts->client->dev;
+	int ret;
+
+	/* disable interrupt generation */
+	ret = pixcir_int_enable(ts, 0);
+	if (ret) {
+		dev_err(dev, "Failed to disable interrupt generation\n");
+		return ret;
+	}
+
+	disable_irq(ts->client->irq);
+
+	return 0;
+}
+
+static int pixcir_input_open(struct input_dev *dev)
+{
+	struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+	return pixcir_start(ts);
+}
+
+static void pixcir_input_close(struct input_dev *dev)
+{
+	struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+	pixcir_stop(ts);
+
+	return;
+}
+
+
 #ifdef CONFIG_PM_SLEEP
 static int pixcir_i2c_ts_suspend(struct device *dev)
 {
@@ -207,6 +361,8 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input->name = client->name;
 	input->id.bustype = BUS_I2C;
 	input->dev.parent = &client->dev;
+	input->open = pixcir_input_open;
+	input->close = pixcir_input_close;
 
 	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
@@ -230,6 +386,18 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 		goto err_free_mem;
 	}
 
+	/* Always be in IDLE mode to save power, device supports auto wake */
+	error = pixcir_set_power_mode(tsdata, PIXCIR_POWER_IDLE);
+	if (error) {
+		dev_err(dev, "Failed to set IDLE mode\n");
+		return error;
+	}
+
+	/* Stop device till opened */
+	error = pixcir_stop(tsdata);
+	if (error)
+		goto err_free_irq;
+
 	error = input_register_device(input);
 	if (error)
 		goto err_free_irq;
-- 
1.8.3.2

[PATCH 4/9] Input: pixcir_i2c_ts: Use devres managed resource allocations

[Roger Quadros]

Use devm_() and friends for allocating memory, input device
and IRQ.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 35 ++++++++++++-------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index ce8abcd..3370fd9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -346,12 +346,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
-	input = input_allocate_device();
-	if (!tsdata || !input) {
-		dev_err(&client->dev, "Failed to allocate driver data!\n");
-		error = -ENOMEM;
-		goto err_free_mem;
+	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
+	if (!tsdata)
+		return -ENOMEM;
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(&client->dev, "Failed to allocate input device\n");
+		return -ENOMEM;
 	}
 
 	tsdata->client = client;
@@ -378,12 +380,12 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	input_set_drvdata(input, tsdata);
 
-	error = request_threaded_irq(client->irq, NULL, pixcir_ts_isr,
+	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
 				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				     client->name, tsdata);
 	if (error) {
-		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
-		goto err_free_mem;
+		dev_err(dev, "failed to request irq %d\n", client->irq);
+		return error;
 	}
 
 	/* Always be in IDLE mode to save power, device supports auto wake */
@@ -396,23 +398,16 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	/* Stop device till opened */
 	error = pixcir_stop(tsdata);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	error = input_register_device(input);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	i2c_set_clientdata(client, tsdata);
 	device_init_wakeup(&client->dev, 1);
 
 	return 0;
-
-err_free_irq:
-	free_irq(client->irq, tsdata);
-err_free_mem:
-	input_free_device(input);
-	kfree(tsdata);
-	return error;
 }
 
 static int pixcir_i2c_ts_remove(struct i2c_client *client)
@@ -423,10 +418,6 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 
 	tsdata->exiting = true;
 	mb();
-	free_irq(client->irq, tsdata);
-
-	input_unregister_device(tsdata->input);
-	kfree(tsdata);
 
 	return 0;
 }
-- 
1.8.3.2

[PATCH 4/9] Input: pixcir_i2c_ts: Use devres managed resource allocations

[Roger Quadros]

Use devm_() and friends for allocating memory, input device
and IRQ.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 35 ++++++++++++-------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index ce8abcd..3370fd9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -346,12 +346,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
-	input = input_allocate_device();
-	if (!tsdata || !input) {
-		dev_err(&client->dev, "Failed to allocate driver data!\n");
-		error = -ENOMEM;
-		goto err_free_mem;
+	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
+	if (!tsdata)
+		return -ENOMEM;
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(&client->dev, "Failed to allocate input device\n");
+		return -ENOMEM;
 	}
 
 	tsdata->client = client;
@@ -378,12 +380,12 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	input_set_drvdata(input, tsdata);
 
-	error = request_threaded_irq(client->irq, NULL, pixcir_ts_isr,
+	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
 				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				     client->name, tsdata);
 	if (error) {
-		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
-		goto err_free_mem;
+		dev_err(dev, "failed to request irq %d\n", client->irq);
+		return error;
 	}
 
 	/* Always be in IDLE mode to save power, device supports auto wake */
@@ -396,23 +398,16 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	/* Stop device till opened */
 	error = pixcir_stop(tsdata);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	error = input_register_device(input);
 	if (error)
-		goto err_free_irq;
+		return error;
 
 	i2c_set_clientdata(client, tsdata);
 	device_init_wakeup(&client->dev, 1);
 
 	return 0;
-
-err_free_irq:
-	free_irq(client->irq, tsdata);
-err_free_mem:
-	input_free_device(input);
-	kfree(tsdata);
-	return error;
 }
 
 static int pixcir_i2c_ts_remove(struct i2c_client *client)
@@ -423,10 +418,6 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 
 	tsdata->exiting = true;
 	mb();
-	free_irq(client->irq, tsdata);
-
-	input_unregister_device(tsdata->input);
-	kfree(tsdata);
 
 	return 0;
 }
-- 
1.8.3.2

[PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()

[Roger Quadros]

Get rid of the attb_read_val() platform hook. Instead,
read the ATTB gpio directly from the driver.

Fail if valid ATTB gpio is not provided by patform data.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
 include/linux/input/pixcir_ts.h           |  1 -
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3370fd9..a783d94 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
 static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 {
 	struct pixcir_i2c_ts_data *tsdata = dev_id;
+	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
 
 	while (!tsdata->exiting) {
 		pixcir_ts_poscheck(tsdata);
 
-		if (tsdata->chip->attb_read_val())
+		if (gpio_get_value(pdata->gpio_attb))
 			break;
 
 		msleep(20);
@@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
-	if (!gpio_is_valid(pdata->gpio_attb))
+	if (!gpio_is_valid(pdata->gpio_attb)) {
 		dev_err(dev, "Failed to get ATTB GPIO\n");
+		return ERR_PTR(-EINVAL);
+	}
 
 	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
 		dev_err(dev, "Failed to get x-size property\n");
@@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	} else if (!pdata) {
 		dev_err(&client->dev, "platform data not defined\n");
 		return -EINVAL;
+	} else {
+		if (!gpio_is_valid(pdata->gpio_attb)) {
+			dev_err(dev, "Invalid gpio_attb in pdata\n");
+			return -EINVAL;
+		}
 	}
 
 	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
@@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	input_set_drvdata(input, tsdata);
 
+	error = devm_gpio_request_one(dev, pdata->gpio_attb,
+			GPIOF_DIR_IN, "pixcir_i2c_attb");
+	if (error) {
+		dev_err(dev, "Failed to request ATTB gpio\n");
+		return error;
+	}
+
 	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
 				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				     client->name, tsdata);
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index f17c192..88ffdb50 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -44,7 +44,6 @@ enum pixcir_int_mode {
 #define PIXCIR_INT_POL_HIGH	(1UL << 2)
 
 struct pixcir_ts_platform_data {
-	int (*attb_read_val)(void);
 	unsigned int x_size;	/* X axis resolution */
 	unsigned int y_size;	/* Y axis resolution */
 	int gpio_attb;		/* GPIO connected to ATTB line */
-- 
1.8.3.2

[PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()

[Roger Quadros]

Get rid of the attb_read_val() platform hook. Instead,
read the ATTB gpio directly from the driver.

Fail if valid ATTB gpio is not provided by patform data.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
 include/linux/input/pixcir_ts.h           |  1 -
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3370fd9..a783d94 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
 static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 {
 	struct pixcir_i2c_ts_data *tsdata = dev_id;
+	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
 
 	while (!tsdata->exiting) {
 		pixcir_ts_poscheck(tsdata);
 
-		if (tsdata->chip->attb_read_val())
+		if (gpio_get_value(pdata->gpio_attb))
 			break;
 
 		msleep(20);
@@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
-	if (!gpio_is_valid(pdata->gpio_attb))
+	if (!gpio_is_valid(pdata->gpio_attb)) {
 		dev_err(dev, "Failed to get ATTB GPIO\n");
+		return ERR_PTR(-EINVAL);
+	}
 
 	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
 		dev_err(dev, "Failed to get x-size property\n");
@@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	} else if (!pdata) {
 		dev_err(&client->dev, "platform data not defined\n");
 		return -EINVAL;
+	} else {
+		if (!gpio_is_valid(pdata->gpio_attb)) {
+			dev_err(dev, "Invalid gpio_attb in pdata\n");
+			return -EINVAL;
+		}
 	}
 
 	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
@@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	input_set_drvdata(input, tsdata);
 
+	error = devm_gpio_request_one(dev, pdata->gpio_attb,
+			GPIOF_DIR_IN, "pixcir_i2c_attb");
+	if (error) {
+		dev_err(dev, "Failed to request ATTB gpio\n");
+		return error;
+	}
+
 	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
 				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				     client->name, tsdata);
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index f17c192..88ffdb50 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -44,7 +44,6 @@ enum pixcir_int_mode {
 #define PIXCIR_INT_POL_HIGH	(1UL << 2)
 
 struct pixcir_ts_platform_data {
-	int (*attb_read_val)(void);
 	unsigned int x_size;	/* X axis resolution */
 	unsigned int y_size;	/* Y axis resolution */
 	int gpio_attb;		/* GPIO connected to ATTB line */
-- 
1.8.3.2

[PATCH 6/9] Input: pixcir_i2c_ts: Add chip specific data structure

[Roger Quadros]

This is the data that differentiates different pixcir
chips.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c |  8 +++++---
 include/linux/input/pixcir_ts.h           | 11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index a783d94..ff68246 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -32,7 +32,7 @@
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input;
-	const struct pixcir_ts_platform_data *chip;
+	const struct pixcir_ts_platform_data *pdata;
 	bool exiting;
 };
 
@@ -91,7 +91,7 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
 static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 {
 	struct pixcir_i2c_ts_data *tsdata = dev_id;
-	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
+	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
 
 	while (!tsdata->exiting) {
 		pixcir_ts_poscheck(tsdata);
@@ -301,6 +301,8 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	pdata->chip = *(const struct pixcir_i2c_chip_data *)match->data;
+
 	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
 	if (!gpio_is_valid(pdata->gpio_attb)) {
 		dev_err(dev, "Failed to get ATTB GPIO\n");
@@ -366,7 +368,7 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	tsdata->client = client;
 	tsdata->input = input;
-	tsdata->chip = pdata;
+	tsdata->pdata = pdata;
 
 	input->name = client->name;
 	input->id.bustype = BUS_I2C;
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 88ffdb50..b9a2f6f 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -43,10 +43,21 @@ enum pixcir_int_mode {
 #define PIXCIR_INT_ENABLE	(1UL << 3)
 #define PIXCIR_INT_POL_HIGH	(1UL << 2)
 
+/**
+ * struct pixcir_irc_chip_data - chip related data
+ * @num_report_ids:     Max number of finger ids reported simultaneously.
+ *                      if 0 it means chip doesn't support finger id reporting
+ *                      and driver will resort to Type A Multi-Touch reporting.
+ */
+struct pixcir_i2c_chip_data {
+	u8 num_report_ids;
+};
+
 struct pixcir_ts_platform_data {
 	unsigned int x_size;	/* X axis resolution */
 	unsigned int y_size;	/* Y axis resolution */
 	int gpio_attb;		/* GPIO connected to ATTB line */
+	struct pixcir_i2c_chip_data chip;
 };
 
 #endif
-- 
1.8.3.2

[PATCH 6/9] Input: pixcir_i2c_ts: Add chip specific data structure

[Roger Quadros]

This is the data that differentiates different pixcir
chips.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c |  8 +++++---
 include/linux/input/pixcir_ts.h           | 11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index a783d94..ff68246 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -32,7 +32,7 @@
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input;
-	const struct pixcir_ts_platform_data *chip;
+	const struct pixcir_ts_platform_data *pdata;
 	bool exiting;
 };
 
@@ -91,7 +91,7 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
 static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
 {
 	struct pixcir_i2c_ts_data *tsdata = dev_id;
-	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
+	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
 
 	while (!tsdata->exiting) {
 		pixcir_ts_poscheck(tsdata);
@@ -301,6 +301,8 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	pdata->chip = *(const struct pixcir_i2c_chip_data *)match->data;
+
 	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
 	if (!gpio_is_valid(pdata->gpio_attb)) {
 		dev_err(dev, "Failed to get ATTB GPIO\n");
@@ -366,7 +368,7 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 
 	tsdata->client = client;
 	tsdata->input = input;
-	tsdata->chip = pdata;
+	tsdata->pdata = pdata;
 
 	input->name = client->name;
 	input->id.bustype = BUS_I2C;
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 88ffdb50..b9a2f6f 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -43,10 +43,21 @@ enum pixcir_int_mode {
 #define PIXCIR_INT_ENABLE	(1UL << 3)
 #define PIXCIR_INT_POL_HIGH	(1UL << 2)
 
+/**
+ * struct pixcir_irc_chip_data - chip related data
+ * @num_report_ids:     Max number of finger ids reported simultaneously.
+ *                      if 0 it means chip doesn't support finger id reporting
+ *                      and driver will resort to Type A Multi-Touch reporting.
+ */
+struct pixcir_i2c_chip_data {
+	u8 num_report_ids;
+};
+
 struct pixcir_ts_platform_data {
 	unsigned int x_size;	/* X axis resolution */
 	unsigned int y_size;	/* Y axis resolution */
 	int gpio_attb;		/* GPIO connected to ATTB line */
+	struct pixcir_i2c_chip_data chip;
 };
 
 #endif
-- 
1.8.3.2

[PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Roger Quadros]

Some pixcir controllers e.g. tangoC family report finger IDs with
the co-ordinates and are more suitable for Type-B MT protocol.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
 1 file changed, 155 insertions(+), 47 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index ff68246..9e14415 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -23,84 +23,173 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
+#include <linux/input/mt.h>
 #include <linux/input/pixcir_ts.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
 
+#define MAX_FINGERS	5	/* Maximum supported by the driver */
+
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input;
 	const struct pixcir_ts_platform_data *pdata;
 	bool exiting;
+	u8 max_fingers;		/* Maximum supported by the chip */
 };
 
-static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
+static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
 {
-	struct pixcir_i2c_ts_data *tsdata = data;
+	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
 	u8 rdbuf[10], wrbuf[1] = { 0 };
 	u8 touch;
 	int ret;
 
-	ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
-	if (ret != sizeof(wrbuf)) {
-		dev_err(&tsdata->client->dev,
-			"%s: i2c_master_send failed(), ret=%d\n",
-			__func__, ret);
-		return;
-	}
+	while (!tsdata->exiting) {
 
-	ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
-	if (ret != sizeof(rdbuf)) {
-		dev_err(&tsdata->client->dev,
-			"%s: i2c_master_recv failed(), ret=%d\n",
-			__func__, ret);
-		return;
-	}
+		ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
+		if (ret != sizeof(wrbuf)) {
+			dev_err(&tsdata->client->dev,
+				 "%s: i2c_master_send failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
+
+		ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
+		if (ret != sizeof(rdbuf)) {
+			dev_err(&tsdata->client->dev,
+				 "%s: i2c_master_recv failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
 
-	touch = rdbuf[0];
-	if (touch) {
-		u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
-		u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
-		u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
-		u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
-
-		input_report_key(tsdata->input, BTN_TOUCH, 1);
-		input_report_abs(tsdata->input, ABS_X, posx1);
-		input_report_abs(tsdata->input, ABS_Y, posy1);
-
-		input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
-		input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
-		input_mt_sync(tsdata->input);
-
-		if (touch == 2) {
-			input_report_abs(tsdata->input,
-					 ABS_MT_POSITION_X, posx2);
-			input_report_abs(tsdata->input,
-					 ABS_MT_POSITION_Y, posy2);
+		touch = rdbuf[0];
+		if (touch) {
+			u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
+			u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
+			u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
+			u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
+
+			input_report_key(tsdata->input, BTN_TOUCH, 1);
+			input_report_abs(tsdata->input, ABS_X, posx1);
+			input_report_abs(tsdata->input, ABS_Y, posy1);
+
+			input_report_abs(tsdata->input, ABS_MT_POSITION_X,
+									posx1);
+			input_report_abs(tsdata->input, ABS_MT_POSITION_Y,
+									posy1);
 			input_mt_sync(tsdata->input);
+
+			if (touch == 2) {
+				input_report_abs(tsdata->input,
+						ABS_MT_POSITION_X, posx2);
+				input_report_abs(tsdata->input,
+						ABS_MT_POSITION_Y, posy2);
+				input_mt_sync(tsdata->input);
+			}
+		} else {
+			input_report_key(tsdata->input, BTN_TOUCH, 0);
 		}
-	} else {
-		input_report_key(tsdata->input, BTN_TOUCH, 0);
-	}
 
-	input_sync(tsdata->input);
+		input_sync(tsdata->input);
+
+		if (gpio_get_value(pdata->gpio_attb))
+			break;
+
+		msleep(20);
+	}
 }
 
-static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
+static void pixcir_ts_typeb_report(struct pixcir_i2c_ts_data *ts)
 {
-	struct pixcir_i2c_ts_data *tsdata = dev_id;
-	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
+	const struct pixcir_ts_platform_data *pdata = ts->pdata;
+	struct device *dev = &ts->client->dev;
+	u8 rdbuf[32], wrbuf[1] = { 0 };
+	u8 *bufptr;
+	u8 num_fingers;
+	u8 unreliable;
+	int ret, i;
+
+	while (!ts->exiting) {
+
+		ret = i2c_master_send(ts->client, wrbuf, sizeof(wrbuf));
+		if (ret != sizeof(wrbuf)) {
+			dev_err(dev, "%s: i2c_master_send failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
 
-	while (!tsdata->exiting) {
-		pixcir_ts_poscheck(tsdata);
+		ret = i2c_master_recv(ts->client, rdbuf, sizeof(rdbuf));
+		if (ret != sizeof(rdbuf)) {
+			dev_err(dev, "%s: i2c_master_recv failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
+
+		unreliable = rdbuf[0] & 0xe0;
+
+		if (unreliable)
+			goto next;	/* ignore unreliable data */
+
+		num_fingers = rdbuf[0] & 0x7;
+		bufptr = &rdbuf[2];
 
+		if (num_fingers > ts->max_fingers) {
+			num_fingers = ts->max_fingers;
+			dev_dbg(dev, "limiting num_fingers to %d\n",
+								num_fingers);
+		}
+
+		for (i = 0; i < num_fingers; i++) {
+			u8 id;
+			unsigned int x, y;
+			int slot;
+
+			id = bufptr[4];
+			slot = input_mt_get_slot_by_key(ts->input, id);
+			if (slot < 0) {
+				dev_dbg(dev, "no free slot for id 0x%x\n", id);
+				continue;
+			}
+
+
+			x = bufptr[1] << 8 | bufptr[0];
+			y = bufptr[3] << 8 | bufptr[2];
+
+			input_mt_slot(ts->input, slot);
+			input_mt_report_slot_state(ts->input,
+							MT_TOOL_FINGER, true);
+
+			input_event(ts->input, EV_ABS, ABS_MT_POSITION_X, x);
+			input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, y);
+
+			bufptr = &bufptr[5];
+			dev_dbg(dev, "%d: id 0x%x slot %d, x %d, y %d\n",
+							i, id, slot, x, y);
+		}
+
+		/* One frame is complete so sync it */
+		input_mt_sync_frame(ts->input);
+		input_sync(ts->input);
+
+next:
 		if (gpio_get_value(pdata->gpio_attb))
 			break;
 
-		msleep(20);
+		usleep_range(2000, 5000);
 	}
+}
+
+static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
+{
+	struct pixcir_i2c_ts_data *tsdata = dev_id;
+
+	if (tsdata->input->mt)
+		pixcir_ts_typeb_report(tsdata);
+	else
+		pixcir_ts_typea_report(tsdata);
 
 	return IRQ_HANDLED;
 }
@@ -376,9 +465,9 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input->open = pixcir_input_open;
 	input->close = pixcir_input_close;
 
-	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(BTN_TOUCH, input->keybit);
+
 	input_set_abs_params(input, ABS_X,
 					0, pdata->x_size - 1, 0, 0);
 	input_set_abs_params(input, ABS_Y,
@@ -388,6 +477,25 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_MT_POSITION_Y,
 					0, pdata->y_size - 1, 0, 0);
 
+	/* Type-B Multi-Touch support */
+	if (pdata->chip.num_report_ids) {
+		const struct pixcir_i2c_chip_data *chip = &pdata->chip;
+
+		tsdata->max_fingers = chip->num_report_ids;
+		if (tsdata->max_fingers > MAX_FINGERS) {
+			dev_info(dev, "Limiting maximum fingers to %d\n",
+								MAX_FINGERS);
+			tsdata->max_fingers = MAX_FINGERS;
+		}
+
+		error = input_mt_init_slots(input, tsdata->max_fingers,
+					INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+		if (error) {
+			dev_err(dev, "Error initializing Multi-Touch slots\n");
+			return error;
+		}
+	}
+
 	input_set_drvdata(input, tsdata);
 
 	error = devm_gpio_request_one(dev, pdata->gpio_attb,
-- 
1.8.3.2

[PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Roger Quadros]

Some pixcir controllers e.g. tangoC family report finger IDs with
the co-ordinates and are more suitable for Type-B MT protocol.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
 1 file changed, 155 insertions(+), 47 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index ff68246..9e14415 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -23,84 +23,173 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
+#include <linux/input/mt.h>
 #include <linux/input/pixcir_ts.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
 
+#define MAX_FINGERS	5	/* Maximum supported by the driver */
+
 struct pixcir_i2c_ts_data {
 	struct i2c_client *client;
 	struct input_dev *input;
 	const struct pixcir_ts_platform_data *pdata;
 	bool exiting;
+	u8 max_fingers;		/* Maximum supported by the chip */
 };
 
-static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
+static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
 {
-	struct pixcir_i2c_ts_data *tsdata = data;
+	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
 	u8 rdbuf[10], wrbuf[1] = { 0 };
 	u8 touch;
 	int ret;
 
-	ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
-	if (ret != sizeof(wrbuf)) {
-		dev_err(&tsdata->client->dev,
-			"%s: i2c_master_send failed(), ret=%d\n",
-			__func__, ret);
-		return;
-	}
+	while (!tsdata->exiting) {
 
-	ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
-	if (ret != sizeof(rdbuf)) {
-		dev_err(&tsdata->client->dev,
-			"%s: i2c_master_recv failed(), ret=%d\n",
-			__func__, ret);
-		return;
-	}
+		ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
+		if (ret != sizeof(wrbuf)) {
+			dev_err(&tsdata->client->dev,
+				 "%s: i2c_master_send failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
+
+		ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
+		if (ret != sizeof(rdbuf)) {
+			dev_err(&tsdata->client->dev,
+				 "%s: i2c_master_recv failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
 
-	touch = rdbuf[0];
-	if (touch) {
-		u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
-		u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
-		u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
-		u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
-
-		input_report_key(tsdata->input, BTN_TOUCH, 1);
-		input_report_abs(tsdata->input, ABS_X, posx1);
-		input_report_abs(tsdata->input, ABS_Y, posy1);
-
-		input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
-		input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
-		input_mt_sync(tsdata->input);
-
-		if (touch == 2) {
-			input_report_abs(tsdata->input,
-					 ABS_MT_POSITION_X, posx2);
-			input_report_abs(tsdata->input,
-					 ABS_MT_POSITION_Y, posy2);
+		touch = rdbuf[0];
+		if (touch) {
+			u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
+			u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
+			u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
+			u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
+
+			input_report_key(tsdata->input, BTN_TOUCH, 1);
+			input_report_abs(tsdata->input, ABS_X, posx1);
+			input_report_abs(tsdata->input, ABS_Y, posy1);
+
+			input_report_abs(tsdata->input, ABS_MT_POSITION_X,
+									posx1);
+			input_report_abs(tsdata->input, ABS_MT_POSITION_Y,
+									posy1);
 			input_mt_sync(tsdata->input);
+
+			if (touch == 2) {
+				input_report_abs(tsdata->input,
+						ABS_MT_POSITION_X, posx2);
+				input_report_abs(tsdata->input,
+						ABS_MT_POSITION_Y, posy2);
+				input_mt_sync(tsdata->input);
+			}
+		} else {
+			input_report_key(tsdata->input, BTN_TOUCH, 0);
 		}
-	} else {
-		input_report_key(tsdata->input, BTN_TOUCH, 0);
-	}
 
-	input_sync(tsdata->input);
+		input_sync(tsdata->input);
+
+		if (gpio_get_value(pdata->gpio_attb))
+			break;
+
+		msleep(20);
+	}
 }
 
-static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
+static void pixcir_ts_typeb_report(struct pixcir_i2c_ts_data *ts)
 {
-	struct pixcir_i2c_ts_data *tsdata = dev_id;
-	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
+	const struct pixcir_ts_platform_data *pdata = ts->pdata;
+	struct device *dev = &ts->client->dev;
+	u8 rdbuf[32], wrbuf[1] = { 0 };
+	u8 *bufptr;
+	u8 num_fingers;
+	u8 unreliable;
+	int ret, i;
+
+	while (!ts->exiting) {
+
+		ret = i2c_master_send(ts->client, wrbuf, sizeof(wrbuf));
+		if (ret != sizeof(wrbuf)) {
+			dev_err(dev, "%s: i2c_master_send failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
 
-	while (!tsdata->exiting) {
-		pixcir_ts_poscheck(tsdata);
+		ret = i2c_master_recv(ts->client, rdbuf, sizeof(rdbuf));
+		if (ret != sizeof(rdbuf)) {
+			dev_err(dev, "%s: i2c_master_recv failed(), ret=%d\n",
+				 __func__, ret);
+			return;
+		}
+
+		unreliable = rdbuf[0] & 0xe0;
+
+		if (unreliable)
+			goto next;	/* ignore unreliable data */
+
+		num_fingers = rdbuf[0] & 0x7;
+		bufptr = &rdbuf[2];
 
+		if (num_fingers > ts->max_fingers) {
+			num_fingers = ts->max_fingers;
+			dev_dbg(dev, "limiting num_fingers to %d\n",
+								num_fingers);
+		}
+
+		for (i = 0; i < num_fingers; i++) {
+			u8 id;
+			unsigned int x, y;
+			int slot;
+
+			id = bufptr[4];
+			slot = input_mt_get_slot_by_key(ts->input, id);
+			if (slot < 0) {
+				dev_dbg(dev, "no free slot for id 0x%x\n", id);
+				continue;
+			}
+
+
+			x = bufptr[1] << 8 | bufptr[0];
+			y = bufptr[3] << 8 | bufptr[2];
+
+			input_mt_slot(ts->input, slot);
+			input_mt_report_slot_state(ts->input,
+							MT_TOOL_FINGER, true);
+
+			input_event(ts->input, EV_ABS, ABS_MT_POSITION_X, x);
+			input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, y);
+
+			bufptr = &bufptr[5];
+			dev_dbg(dev, "%d: id 0x%x slot %d, x %d, y %d\n",
+							i, id, slot, x, y);
+		}
+
+		/* One frame is complete so sync it */
+		input_mt_sync_frame(ts->input);
+		input_sync(ts->input);
+
+next:
 		if (gpio_get_value(pdata->gpio_attb))
 			break;
 
-		msleep(20);
+		usleep_range(2000, 5000);
 	}
+}
+
+static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
+{
+	struct pixcir_i2c_ts_data *tsdata = dev_id;
+
+	if (tsdata->input->mt)
+		pixcir_ts_typeb_report(tsdata);
+	else
+		pixcir_ts_typea_report(tsdata);
 
 	return IRQ_HANDLED;
 }
@@ -376,9 +465,9 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input->open = pixcir_input_open;
 	input->close = pixcir_input_close;
 
-	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(BTN_TOUCH, input->keybit);
+
 	input_set_abs_params(input, ABS_X,
 					0, pdata->x_size - 1, 0, 0);
 	input_set_abs_params(input, ABS_Y,
@@ -388,6 +477,25 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_MT_POSITION_Y,
 					0, pdata->y_size - 1, 0, 0);
 
+	/* Type-B Multi-Touch support */
+	if (pdata->chip.num_report_ids) {
+		const struct pixcir_i2c_chip_data *chip = &pdata->chip;
+
+		tsdata->max_fingers = chip->num_report_ids;
+		if (tsdata->max_fingers > MAX_FINGERS) {
+			dev_info(dev, "Limiting maximum fingers to %d\n",
+								MAX_FINGERS);
+			tsdata->max_fingers = MAX_FINGERS;
+		}
+
+		error = input_mt_init_slots(input, tsdata->max_fingers,
+					INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+		if (error) {
+			dev_err(dev, "Error initializing Multi-Touch slots\n");
+			return error;
+		}
+	}
+
 	input_set_drvdata(input, tsdata);
 
 	error = devm_gpio_request_one(dev, pdata->gpio_attb,
-- 
1.8.3.2

[PATCH 8/9] Input: pixcir_i2c_ts: Add support for TangoC family

[Roger Quadros]

Add support for Pixcir TangoC controller.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 .../devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt         | 2 +-
 drivers/input/touchscreen/pixcir_i2c_ts.c                           | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
index c0b0b270..0ab9505 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
@@ -1,7 +1,7 @@
 * Pixcir I2C touchscreen controllers
 
 Required properties:
-- compatible: must be "pixcir,pixcir_ts"
+- compatible: must be "pixcir,pixcir_ts" or "pixcir,pixcir_tangoc"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected
 - attb-gpio: GPIO connected to the ATTB line of the chip
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 9e14415..3575eab 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -549,13 +549,19 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 
 static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 	{ "pixcir_ts", 0 },
+	{ "pixcir_tangoc", 0},
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
 
 #if defined(CONFIG_OF)
+static const struct pixcir_i2c_chip_data tangoc_data = {
+	.num_report_ids = 5,
+};
+
 static const struct of_device_id pixcir_of_match[] = {
 	{ .compatible = "pixcir,pixcir_ts", },
+	{ .compatible = "pixcir,pixcir_tangoc", .data = &tangoc_data, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pixcir_of_match);
-- 
1.8.3.2

[PATCH 8/9] Input: pixcir_i2c_ts: Add support for TangoC family

[Roger Quadros]

Add support for Pixcir TangoC controller.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 .../devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt         | 2 +-
 drivers/input/touchscreen/pixcir_i2c_ts.c                           | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
index c0b0b270..0ab9505 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
@@ -1,7 +1,7 @@
 * Pixcir I2C touchscreen controllers
 
 Required properties:
-- compatible: must be "pixcir,pixcir_ts"
+- compatible: must be "pixcir,pixcir_ts" or "pixcir,pixcir_tangoc"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected
 - attb-gpio: GPIO connected to the ATTB line of the chip
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 9e14415..3575eab 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -549,13 +549,19 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 
 static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 	{ "pixcir_ts", 0 },
+	{ "pixcir_tangoc", 0},
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
 
 #if defined(CONFIG_OF)
+static const struct pixcir_i2c_chip_data tangoc_data = {
+	.num_report_ids = 5,
+};
+
 static const struct of_device_id pixcir_of_match[] = {
 	{ .compatible = "pixcir,pixcir_ts", },
+	{ .compatible = "pixcir,pixcir_tangoc", .data = &tangoc_data, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pixcir_of_match);
-- 
1.8.3.2

[PATCH 9/9] Input: pixcir_i2c_ts: Implement wakeup from suspend

[Roger Quadros]

Improve the suspend and resume handlers to allow the device
to wakeup the system from suspend.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 89 ++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 26 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3575eab..a7725b9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -347,32 +347,6 @@ static void pixcir_input_close(struct input_dev *dev)
 	return;
 }
 
-
-#ifdef CONFIG_PM_SLEEP
-static int pixcir_i2c_ts_suspend(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(client->irq);
-
-	return 0;
-}
-
-static int pixcir_i2c_ts_resume(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(&client->dev))
-		disable_irq_wake(client->irq);
-
-	return 0;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
-			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
-
 #if defined(CONFIG_OF)
 static const struct of_device_id pixcir_of_match[];
 
@@ -547,6 +521,69 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int pixcir_i2c_ts_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+	struct input_dev *input = ts->input;
+	int ret = 0;
+
+	mutex_lock(&input->mutex);
+
+	if (device_may_wakeup(&client->dev)) {
+		/* need to start device if not open, to be wakeup source */
+		if (!input->users) {
+			ret = pixcir_start(ts);
+			if (ret)
+				goto unlock;
+		}
+
+		enable_irq_wake(client->irq);
+
+	} else if (input->users) {
+		ret = pixcir_stop(ts);
+	}
+
+unlock:
+	mutex_unlock(&input->mutex);
+
+	return ret;
+}
+
+static int pixcir_i2c_ts_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+	struct input_dev *input = ts->input;
+	int ret = 0;
+
+	mutex_lock(&input->mutex);
+
+	if (device_may_wakeup(&client->dev)) {
+		disable_irq_wake(client->irq);
+
+		/* need to stop device if it was not open on suspend */
+		if (!input->users) {
+			ret = pixcir_stop(ts);
+			if (ret)
+				goto unlock;
+		}
+
+	} else if (input->users) {
+		ret = pixcir_start(ts);
+	}
+
+unlock:
+	mutex_unlock(&input->mutex);
+
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
+				pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
+
 static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 	{ "pixcir_ts", 0 },
 	{ "pixcir_tangoc", 0},
-- 
1.8.3.2

[PATCH 9/9] Input: pixcir_i2c_ts: Implement wakeup from suspend

[Roger Quadros]

Improve the suspend and resume handlers to allow the device
to wakeup the system from suspend.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/input/touchscreen/pixcir_i2c_ts.c | 89 ++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 26 deletions(-)

diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 3575eab..a7725b9 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -347,32 +347,6 @@ static void pixcir_input_close(struct input_dev *dev)
 	return;
 }
 
-
-#ifdef CONFIG_PM_SLEEP
-static int pixcir_i2c_ts_suspend(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(client->irq);
-
-	return 0;
-}
-
-static int pixcir_i2c_ts_resume(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-
-	if (device_may_wakeup(&client->dev))
-		disable_irq_wake(client->irq);
-
-	return 0;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
-			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
-
 #if defined(CONFIG_OF)
 static const struct of_device_id pixcir_of_match[];
 
@@ -547,6 +521,69 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int pixcir_i2c_ts_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+	struct input_dev *input = ts->input;
+	int ret = 0;
+
+	mutex_lock(&input->mutex);
+
+	if (device_may_wakeup(&client->dev)) {
+		/* need to start device if not open, to be wakeup source */
+		if (!input->users) {
+			ret = pixcir_start(ts);
+			if (ret)
+				goto unlock;
+		}
+
+		enable_irq_wake(client->irq);
+
+	} else if (input->users) {
+		ret = pixcir_stop(ts);
+	}
+
+unlock:
+	mutex_unlock(&input->mutex);
+
+	return ret;
+}
+
+static int pixcir_i2c_ts_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+	struct input_dev *input = ts->input;
+	int ret = 0;
+
+	mutex_lock(&input->mutex);
+
+	if (device_may_wakeup(&client->dev)) {
+		disable_irq_wake(client->irq);
+
+		/* need to stop device if it was not open on suspend */
+		if (!input->users) {
+			ret = pixcir_stop(ts);
+			if (ret)
+				goto unlock;
+		}
+
+	} else if (input->users) {
+		ret = pixcir_start(ts);
+	}
+
+unlock:
+	mutex_unlock(&input->mutex);
+
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
+				pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
+
 static const struct i2c_device_id pixcir_i2c_ts_id[] = {
 	{ "pixcir_ts", 0 },
 	{ "pixcir_tangoc", 0},
-- 
1.8.3.2

Re: [PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Dmitry Torokhov]

Hi Roger,

On Wed, Dec 18, 2013 at 02:51:12PM +0530, Roger Quadros wrote:
> Provide device tree support and binding information.
> Change platform data parameters from x/y_max to x/y_size..

I'd rather keep them as they were.

> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
>  drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
>  include/linux/input/pixcir_ts.h                    |  5 +-
>  3 files changed, 101 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
> new file mode 100644
> index 0000000..c0b0b270
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
> @@ -0,0 +1,26 @@
> +* Pixcir I2C touchscreen controllers
> +
> +Required properties:
> +- compatible: must be "pixcir,pixcir_ts"
> +- reg: I2C address of the chip
> +- interrupts: interrupt to which the chip is connected
> +- attb-gpio: GPIO connected to the ATTB line of the chip
> +- x-size: horizontal resolution of touchscreen
> +- y-size: vertical resolution of touchscreen
> +
> +Example:
> +
> +	i2c@00000000 {
> +		/* ... */
> +
> +		pixcir_ts@5c {
> +			compatible = "pixcir,pixcir_ts";
> +			reg = <0x5c>;
> +			interrupts = <2 0>;
> +			attb-gpio = <&gpf 2 0 2>;
> +			x-size = <800>;
> +			y-size = <600>;
> +		};
> +
> +		/* ... */
> +	};
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index 6cc6b36..3a447c9 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -24,6 +24,10 @@
>  #include <linux/i2c.h>
>  #include <linux/input.h>
>  #include <linux/input/pixcir_ts.h>
> +#include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_device.h>
>  
>  struct pixcir_i2c_ts_data {
>  	struct i2c_client *client;
> @@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
>  			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
>  
> +#if defined(CONFIG_OF)

#ifdef is preferred for simple conditions.

> +static const struct of_device_id pixcir_of_match[];
> +
> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
> +{
> +	struct pixcir_ts_platform_data *pdata;
> +	struct device_node *np = dev->of_node;
> +	const struct of_device_id *match;
> +
> +	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
> +	if (!match)
> +		return ERR_PTR(-EINVAL);

Why do you need an explicit match here?

> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
> +	if (!gpio_is_valid(pdata->gpio_attb))
> +		dev_err(dev, "Failed to get ATTB GPIO\n");
> +
> +	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
> +		dev_err(dev, "Failed to get x-size property\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
> +		dev_err(dev, "Failed to get y-size property\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
> +				pdata->x_size, pdata->y_size, pdata->gpio_attb);
> +
> +	return pdata;
> +}
> +#else
> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
> +{
> +	return NULL;

This should be

	return ERR_PTR(-EINVAL);

since you test it with IS_ERR() later.

> +}
> +#endif
> +
>  static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  					 const struct i2c_device_id *id)
>  {
>  	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node;
>  	struct pixcir_i2c_ts_data *tsdata;
>  	struct input_dev *input;
>  	int error;
>  
> -	if (!pdata) {
> +	if (np) {
> +		pdata = pixcir_parse_dt(dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);

We should be favoring kernel-provided platform data if it is pesent even
if there is dt-data available. This way user can override firmware,m if
needed.

> +
> +	} else if (!pdata) {
>  		dev_err(&client->dev, "platform data not defined\n");
>  		return -EINVAL;
>  	}
> @@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	__set_bit(EV_KEY, input->evbit);
>  	__set_bit(EV_ABS, input->evbit);
>  	__set_bit(BTN_TOUCH, input->keybit);
> -	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
> -	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
> +	input_set_abs_params(input, ABS_X,
> +					0, pdata->x_size - 1, 0, 0);
> +	input_set_abs_params(input, ABS_Y,
> +					0, pdata->y_size - 1, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_X,
> +					0, pdata->x_size - 1, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
> +					0, pdata->y_size - 1, 0, 0);
>  
>  	input_set_drvdata(input, tsdata);
>  
> @@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id pixcir_of_match[] = {
> +	{ .compatible = "pixcir,pixcir_ts", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pixcir_of_match);
> +#endif

#ifdef is preferred for simple conditions.

> +
>  static struct i2c_driver pixcir_i2c_ts_driver = {
>  	.driver = {
>  		.owner	= THIS_MODULE,
>  		.name	= "pixcir_ts",
>  		.pm	= &pixcir_dev_pm_ops,
> +		.of_match_table = of_match_ptr(pixcir_of_match),
>  	},
>  	.probe		= pixcir_i2c_ts_probe,
>  	.remove		= pixcir_i2c_ts_remove,
> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
> index 7163d91..b34ff7e 100644
> --- a/include/linux/input/pixcir_ts.h
> +++ b/include/linux/input/pixcir_ts.h
> @@ -3,8 +3,9 @@
>  
>  struct pixcir_ts_platform_data {
>  	int (*attb_read_val)(void);
> -	int x_max;
> -	int y_max;
> +	unsigned int x_size;	/* X axis resolution */
> +	unsigned int y_size;	/* Y axis resolution */
> +	int gpio_attb;		/* GPIO connected to ATTB line */
>  };

OK, it looks like the series were split awkwardly: you are defining data
that is used by follow-up patches and its purpose is unclear when
reviewing patch-by-patch. I'd recommend rearranging so that DT support
patch is the very last in the series.

>  
>  #endif
> -- 
> 1.8.3.2
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/9] Input: pixcir_i2c_ts: Add register definitions

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:13PM +0530, Roger Quadros wrote:
> Add power and interrupt register definitions.
> 

Please fold into the next patch that uses these definitions.

> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  include/linux/input/pixcir_ts.h | 42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
> index b34ff7e..f17c192 100644
> --- a/include/linux/input/pixcir_ts.h
> +++ b/include/linux/input/pixcir_ts.h
> @@ -1,6 +1,48 @@
>  #ifndef	_PIXCIR_I2C_TS_H
>  #define	_PIXCIR_I2C_TS_H
>  
> +/*
> + * Register map
> + */
> +#define PIXCIR_REG_POWER_MODE	51
> +#define PIXCIR_REG_INT_MODE	52
> +
> +/*
> + * Power modes:
> + * active: max scan speed
> + * idle: lower scan speed with automatic transition to active on touch
> + * halt: datasheet says sleep but this is more like halt as the chip
> + *       clocks are cut and it can only be brought out of this mode
> + *	 using the RESET pin.
> + */
> +enum pixcir_power_mode {
> +	PIXCIR_POWER_ACTIVE,
> +	PIXCIR_POWER_IDLE,
> +	PIXCIR_POWER_HALT,
> +};
> +
> +#define PIXCIR_POWER_MODE_MASK	0x03
> +#define PIXCIR_POWER_ALLOW_IDLE (1UL << 2)
> +
> +/*
> + * Interrupt modes:
> + * periodical: interrupt is asserted periodicaly
> + * diff coordinates: interrupt is asserted when coordinates change
> + * level on touch: interrupt level asserted during touch
> + * pulse on touch: interrupt pulse asserted druing touch
> + *
> + */
> +enum pixcir_int_mode {
> +	PIXCIR_INT_PERIODICAL,
> +	PIXCIR_INT_DIFF_COORD,
> +	PIXCIR_INT_LEVEL_TOUCH,
> +	PIXCIR_INT_PULSE_TOUCH,
> +};
> +
> +#define PIXCIR_INT_MODE_MASK	0x03
> +#define PIXCIR_INT_ENABLE	(1UL << 3)
> +#define PIXCIR_INT_POL_HIGH	(1UL << 2)
> +
>  struct pixcir_ts_platform_data {
>  	int (*attb_read_val)(void);
>  	unsigned int x_size;	/* X axis resolution */
> -- 
> 1.8.3.2
> 

-- 
Dmitry

Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
> +
> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
> +{
> +	struct device *dev = &ts->client->dev;
> +	int ret;
> +
> +	/* disable interrupt generation */
> +	ret = pixcir_int_enable(ts, 0);
> +	if (ret) {
> +		dev_err(dev, "Failed to disable interrupt generation\n");
> +		return ret;
> +	}
> +
> +	disable_irq(ts->client->irq);

Why do you need to disable IRQ? If you disable interrupt generation in
the chip I think you only need to call synchronize_irq() to make sure
it's completed if it happens to be running. Also you need to move the
code:

	tsdata->exiting = true;
	mb();

here from pixcir_i2c_ts_remove() to make sure handler exits promptly.

You will also need to reset tsdata->exiting in your start method.

Thanks.

-- 
Dmitry

Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
> +
> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
> +{
> +	struct device *dev = &ts->client->dev;
> +	int ret;
> +
> +	/* disable interrupt generation */
> +	ret = pixcir_int_enable(ts, 0);
> +	if (ret) {
> +		dev_err(dev, "Failed to disable interrupt generation\n");
> +		return ret;
> +	}
> +
> +	disable_irq(ts->client->irq);

Why do you need to disable IRQ? If you disable interrupt generation in
the chip I think you only need to call synchronize_irq() to make sure
it's completed if it happens to be running. Also you need to move the
code:

	tsdata->exiting = true;
	mb();

here from pixcir_i2c_ts_remove() to make sure handler exits promptly.

You will also need to reset tsdata->exiting in your start method.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] Input: pixcir_i2c_ts: Use devres managed resource allocations

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:15PM +0530, Roger Quadros wrote:
> Use devm_() and friends for allocating memory, input device
> and IRQ.

This one looks good, maybe have it first in the series when you
resubmit.

> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/input/touchscreen/pixcir_i2c_ts.c | 35 ++++++++++++-------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index ce8abcd..3370fd9 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -346,12 +346,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>  
> -	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
> -	input = input_allocate_device();
> -	if (!tsdata || !input) {
> -		dev_err(&client->dev, "Failed to allocate driver data!\n");
> -		error = -ENOMEM;
> -		goto err_free_mem;
> +	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
> +	if (!tsdata)
> +		return -ENOMEM;
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(&client->dev, "Failed to allocate input device\n");
> +		return -ENOMEM;
>  	}
>  
>  	tsdata->client = client;
> @@ -378,12 +380,12 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  
>  	input_set_drvdata(input, tsdata);
>  
> -	error = request_threaded_irq(client->irq, NULL, pixcir_ts_isr,
> +	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
>  				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  				     client->name, tsdata);
>  	if (error) {
> -		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> -		goto err_free_mem;
> +		dev_err(dev, "failed to request irq %d\n", client->irq);
> +		return error;
>  	}
>  
>  	/* Always be in IDLE mode to save power, device supports auto wake */
> @@ -396,23 +398,16 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	/* Stop device till opened */
>  	error = pixcir_stop(tsdata);
>  	if (error)
> -		goto err_free_irq;
> +		return error;
>  
>  	error = input_register_device(input);
>  	if (error)
> -		goto err_free_irq;
> +		return error;
>  
>  	i2c_set_clientdata(client, tsdata);
>  	device_init_wakeup(&client->dev, 1);
>  
>  	return 0;
> -
> -err_free_irq:
> -	free_irq(client->irq, tsdata);
> -err_free_mem:
> -	input_free_device(input);
> -	kfree(tsdata);
> -	return error;
>  }
>  
>  static int pixcir_i2c_ts_remove(struct i2c_client *client)
> @@ -423,10 +418,6 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
>  
>  	tsdata->exiting = true;
>  	mb();
> -	free_irq(client->irq, tsdata);
> -
> -	input_unregister_device(tsdata->input);
> -	kfree(tsdata);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.2
> 

-- 
Dmitry

Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:18PM +0530, Roger Quadros wrote:
> Some pixcir controllers e.g. tangoC family report finger IDs with
> the co-ordinates and are more suitable for Type-B MT protocol.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
>  1 file changed, 155 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index ff68246..9e14415 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -23,84 +23,173 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/input.h>
> +#include <linux/input/mt.h>
>  #include <linux/input/pixcir_ts.h>
>  #include <linux/gpio.h>
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/of_device.h>
>  
> +#define MAX_FINGERS	5	/* Maximum supported by the driver */
> +
>  struct pixcir_i2c_ts_data {
>  	struct i2c_client *client;
>  	struct input_dev *input;
>  	const struct pixcir_ts_platform_data *pdata;
>  	bool exiting;
> +	u8 max_fingers;		/* Maximum supported by the chip */
>  };
>  
> -static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
> +static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)

Hmm, I do not think we should keep Type A reports if we can do Type B.
The protocols are not new and userspace should be able to handle MT-B by
now.

>  {
> -	struct pixcir_i2c_ts_data *tsdata = data;
> +	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
>  	u8 rdbuf[10], wrbuf[1] = { 0 };
>  	u8 touch;
>  	int ret;
>  
> -	ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
> -	if (ret != sizeof(wrbuf)) {
> -		dev_err(&tsdata->client->dev,
> -			"%s: i2c_master_send failed(), ret=%d\n",
> -			__func__, ret);
> -		return;
> -	}
> +	while (!tsdata->exiting) {
>  
> -	ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
> -	if (ret != sizeof(rdbuf)) {
> -		dev_err(&tsdata->client->dev,
> -			"%s: i2c_master_recv failed(), ret=%d\n",
> -			__func__, ret);
> -		return;
> -	}
> +		ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
> +		if (ret != sizeof(wrbuf)) {
> +			dev_err(&tsdata->client->dev,
> +				 "%s: i2c_master_send failed(), ret=%d\n",
> +				 __func__, ret);
> +			return;
> +		}
> +
> +		ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
> +		if (ret != sizeof(rdbuf)) {
> +			dev_err(&tsdata->client->dev,
> +				 "%s: i2c_master_recv failed(), ret=%d\n",
> +				 __func__, ret);
> +			return;
> +		}
>  
> -	touch = rdbuf[0];
> -	if (touch) {
> -		u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
> -		u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
> -		u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
> -		u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
> -
> -		input_report_key(tsdata->input, BTN_TOUCH, 1);
> -		input_report_abs(tsdata->input, ABS_X, posx1);
> -		input_report_abs(tsdata->input, ABS_Y, posy1);
> -
> -		input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
> -		input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
> -		input_mt_sync(tsdata->input);
> -
> -		if (touch == 2) {
> -			input_report_abs(tsdata->input,
> -					 ABS_MT_POSITION_X, posx2);
> -			input_report_abs(tsdata->input,
> -					 ABS_MT_POSITION_Y, posy2);
> +		touch = rdbuf[0];
> +		if (touch) {
> +			u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
> +			u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
> +			u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
> +			u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
> +
> +			input_report_key(tsdata->input, BTN_TOUCH, 1);
> +			input_report_abs(tsdata->input, ABS_X, posx1);
> +			input_report_abs(tsdata->input, ABS_Y, posy1);
> +
> +			input_report_abs(tsdata->input, ABS_MT_POSITION_X,
> +									posx1);
> +			input_report_abs(tsdata->input, ABS_MT_POSITION_Y,
> +									posy1);
>  			input_mt_sync(tsdata->input);
> +
> +			if (touch == 2) {
> +				input_report_abs(tsdata->input,
> +						ABS_MT_POSITION_X, posx2);
> +				input_report_abs(tsdata->input,
> +						ABS_MT_POSITION_Y, posy2);
> +				input_mt_sync(tsdata->input);
> +			}
> +		} else {
> +			input_report_key(tsdata->input, BTN_TOUCH, 0);
>  		}
> -	} else {
> -		input_report_key(tsdata->input, BTN_TOUCH, 0);
> -	}
>  
> -	input_sync(tsdata->input);
> +		input_sync(tsdata->input);
> +
> +		if (gpio_get_value(pdata->gpio_attb))
> +			break;
> +
> +		msleep(20);
> +	}
>  }
>  
> -static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> +static void pixcir_ts_typeb_report(struct pixcir_i2c_ts_data *ts)
>  {
> -	struct pixcir_i2c_ts_data *tsdata = dev_id;
> -	const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
> +	const struct pixcir_ts_platform_data *pdata = ts->pdata;
> +	struct device *dev = &ts->client->dev;
> +	u8 rdbuf[32], wrbuf[1] = { 0 };
> +	u8 *bufptr;
> +	u8 num_fingers;
> +	u8 unreliable;
> +	int ret, i;
> +
> +	while (!ts->exiting) {
> +
> +		ret = i2c_master_send(ts->client, wrbuf, sizeof(wrbuf));
> +		if (ret != sizeof(wrbuf)) {
> +			dev_err(dev, "%s: i2c_master_send failed(), ret=%d\n",
> +				 __func__, ret);
> +			return;
> +		}
>  
> -	while (!tsdata->exiting) {
> -		pixcir_ts_poscheck(tsdata);
> +		ret = i2c_master_recv(ts->client, rdbuf, sizeof(rdbuf));
> +		if (ret != sizeof(rdbuf)) {
> +			dev_err(dev, "%s: i2c_master_recv failed(), ret=%d\n",
> +				 __func__, ret);
> +			return;
> +		}
> +
> +		unreliable = rdbuf[0] & 0xe0;
> +
> +		if (unreliable)
> +			goto next;	/* ignore unreliable data */
> +
> +		num_fingers = rdbuf[0] & 0x7;
> +		bufptr = &rdbuf[2];
>  
> +		if (num_fingers > ts->max_fingers) {
> +			num_fingers = ts->max_fingers;
> +			dev_dbg(dev, "limiting num_fingers to %d\n",
> +								num_fingers);
> +		}
> +
> +		for (i = 0; i < num_fingers; i++) {
> +			u8 id;
> +			unsigned int x, y;
> +			int slot;
> +
> +			id = bufptr[4];
> +			slot = input_mt_get_slot_by_key(ts->input, id);
> +			if (slot < 0) {
> +				dev_dbg(dev, "no free slot for id 0x%x\n", id);
> +				continue;
> +			}
> +
> +
> +			x = bufptr[1] << 8 | bufptr[0];
> +			y = bufptr[3] << 8 | bufptr[2];
> +
> +			input_mt_slot(ts->input, slot);
> +			input_mt_report_slot_state(ts->input,
> +							MT_TOOL_FINGER, true);
> +
> +			input_event(ts->input, EV_ABS, ABS_MT_POSITION_X, x);
> +			input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, y);
> +
> +			bufptr = &bufptr[5];
> +			dev_dbg(dev, "%d: id 0x%x slot %d, x %d, y %d\n",
> +							i, id, slot, x, y);
> +		}
> +
> +		/* One frame is complete so sync it */
> +		input_mt_sync_frame(ts->input);
> +		input_sync(ts->input);
> +
> +next:
>  		if (gpio_get_value(pdata->gpio_attb))
>  			break;
>  
> -		msleep(20);
> +		usleep_range(2000, 5000);
>  	}
> +}
> +
> +static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> +{
> +	struct pixcir_i2c_ts_data *tsdata = dev_id;
> +
> +	if (tsdata->input->mt)
> +		pixcir_ts_typeb_report(tsdata);
> +	else
> +		pixcir_ts_typea_report(tsdata);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -376,9 +465,9 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	input->open = pixcir_input_open;
>  	input->close = pixcir_input_close;
>  
> -	__set_bit(EV_KEY, input->evbit);
>  	__set_bit(EV_ABS, input->evbit);
>  	__set_bit(BTN_TOUCH, input->keybit);
> +
>  	input_set_abs_params(input, ABS_X,
>  					0, pdata->x_size - 1, 0, 0);
>  	input_set_abs_params(input, ABS_Y,
> @@ -388,6 +477,25 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	input_set_abs_params(input, ABS_MT_POSITION_Y,
>  					0, pdata->y_size - 1, 0, 0);
>  
> +	/* Type-B Multi-Touch support */
> +	if (pdata->chip.num_report_ids) {
> +		const struct pixcir_i2c_chip_data *chip = &pdata->chip;
> +
> +		tsdata->max_fingers = chip->num_report_ids;
> +		if (tsdata->max_fingers > MAX_FINGERS) {
> +			dev_info(dev, "Limiting maximum fingers to %d\n",
> +								MAX_FINGERS);
> +			tsdata->max_fingers = MAX_FINGERS;
> +		}
> +
> +		error = input_mt_init_slots(input, tsdata->max_fingers,
> +					INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +		if (error) {
> +			dev_err(dev, "Error initializing Multi-Touch slots\n");
> +			return error;
> +		}
> +	}
> +
>  	input_set_drvdata(input, tsdata);
>  
>  	error = devm_gpio_request_one(dev, pdata->gpio_attb,
> -- 
> 1.8.3.2
> 

-- 
Dmitry

Re: [PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()

[Dmitry Torokhov]

On Wed, Dec 18, 2013 at 02:51:16PM +0530, Roger Quadros wrote:
> Get rid of the attb_read_val() platform hook. Instead,
> read the ATTB gpio directly from the driver.
> 
> Fail if valid ATTB gpio is not provided by patform data.

Do you also need to define polarity?

> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
>  include/linux/input/pixcir_ts.h           |  1 -
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index 3370fd9..a783d94 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>  static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
>  {
>  	struct pixcir_i2c_ts_data *tsdata = dev_id;
> +	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>  
>  	while (!tsdata->exiting) {
>  		pixcir_ts_poscheck(tsdata);
>  
> -		if (tsdata->chip->attb_read_val())
> +		if (gpio_get_value(pdata->gpio_attb))
>  			break;
>  
>  		msleep(20);
> @@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>  		return ERR_PTR(-ENOMEM);
>  
>  	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
> -	if (!gpio_is_valid(pdata->gpio_attb))
> +	if (!gpio_is_valid(pdata->gpio_attb)) {
>  		dev_err(dev, "Failed to get ATTB GPIO\n");
> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>  		dev_err(dev, "Failed to get x-size property\n");
> @@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  	} else if (!pdata) {
>  		dev_err(&client->dev, "platform data not defined\n");
>  		return -EINVAL;
> +	} else {
> +		if (!gpio_is_valid(pdata->gpio_attb)) {
> +			dev_err(dev, "Invalid gpio_attb in pdata\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
> @@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>  
>  	input_set_drvdata(input, tsdata);
>  
> +	error = devm_gpio_request_one(dev, pdata->gpio_attb,
> +			GPIOF_DIR_IN, "pixcir_i2c_attb");
> +	if (error) {
> +		dev_err(dev, "Failed to request ATTB gpio\n");
> +		return error;
> +	}
> +
>  	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
>  				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  				     client->name, tsdata);
> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
> index f17c192..88ffdb50 100644
> --- a/include/linux/input/pixcir_ts.h
> +++ b/include/linux/input/pixcir_ts.h
> @@ -44,7 +44,6 @@ enum pixcir_int_mode {
>  #define PIXCIR_INT_POL_HIGH	(1UL << 2)
>  
>  struct pixcir_ts_platform_data {
> -	int (*attb_read_val)(void);
>  	unsigned int x_size;	/* X axis resolution */
>  	unsigned int y_size;	/* Y axis resolution */
>  	int gpio_attb;		/* GPIO connected to ATTB line */
> -- 
> 1.8.3.2
> 

-- 
Dmitry

Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Roger Quadros]

Hi Dmitry,

On 12/18/2013 07:48 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:18PM +0530, Roger Quadros wrote:
>> Some pixcir controllers e.g. tangoC family report finger IDs with
>> the co-ordinates and are more suitable for Type-B MT protocol.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>>  drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
>>  1 file changed, 155 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index ff68246..9e14415 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -23,84 +23,173 @@
>>  #include <linux/slab.h>
>>  #include <linux/i2c.h>
>>  #include <linux/input.h>
>> +#include <linux/input/mt.h>
>>  #include <linux/input/pixcir_ts.h>
>>  #include <linux/gpio.h>
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>>  #include <linux/of_device.h>
>>  
>> +#define MAX_FINGERS	5	/* Maximum supported by the driver */
>> +
>>  struct pixcir_i2c_ts_data {
>>  	struct i2c_client *client;
>>  	struct input_dev *input;
>>  	const struct pixcir_ts_platform_data *pdata;
>>  	bool exiting;
>> +	u8 max_fingers;		/* Maximum supported by the chip */
>>  };
>>  
>> -static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>> +static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
> 
> Hmm, I do not think we should keep Type A reports if we can do Type B.
> The protocols are not new and userspace should be able to handle MT-B by
> now.
> 

It seems the controller the original driver was written for does not report
touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits into
Type B reporting model.

cheers,
-roger

Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Roger Quadros]

Hi Dmitry,

On 12/18/2013 07:48 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:18PM +0530, Roger Quadros wrote:
>> Some pixcir controllers e.g. tangoC family report finger IDs with
>> the co-ordinates and are more suitable for Type-B MT protocol.
>>
>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>> Acked-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
>>  1 file changed, 155 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index ff68246..9e14415 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -23,84 +23,173 @@
>>  #include <linux/slab.h>
>>  #include <linux/i2c.h>
>>  #include <linux/input.h>
>> +#include <linux/input/mt.h>
>>  #include <linux/input/pixcir_ts.h>
>>  #include <linux/gpio.h>
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>>  #include <linux/of_device.h>
>>  
>> +#define MAX_FINGERS	5	/* Maximum supported by the driver */
>> +
>>  struct pixcir_i2c_ts_data {
>>  	struct i2c_client *client;
>>  	struct input_dev *input;
>>  	const struct pixcir_ts_platform_data *pdata;
>>  	bool exiting;
>> +	u8 max_fingers;		/* Maximum supported by the chip */
>>  };
>>  
>> -static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>> +static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
> 
> Hmm, I do not think we should keep Type A reports if we can do Type B.
> The protocols are not new and userspace should be able to handle MT-B by
> now.
> 

It seems the controller the original driver was written for does not report
touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits into
Type B reporting model.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()

[Roger Quadros]

On 12/18/2013 07:50 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:16PM +0530, Roger Quadros wrote:
>> Get rid of the attb_read_val() platform hook. Instead,
>> read the ATTB gpio directly from the driver.
>>
>> Fail if valid ATTB gpio is not provided by patform data.
> 
> Do you also need to define polarity?
> 

In patch 3, in pixcir_start(), we explicitly configure the GPIO output
polarity as /* LEVEL_TOUCH interrupt with active low polarity */

So I don't think we need to define polarity.

cheers,
-roger

>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>>  drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
>>  include/linux/input/pixcir_ts.h           |  1 -
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 3370fd9..a783d94 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>>  static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
>>  {
>>  	struct pixcir_i2c_ts_data *tsdata = dev_id;
>> +	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>>  
>>  	while (!tsdata->exiting) {
>>  		pixcir_ts_poscheck(tsdata);
>>  
>> -		if (tsdata->chip->attb_read_val())
>> +		if (gpio_get_value(pdata->gpio_attb))
>>  			break;
>>  
>>  		msleep(20);
>> @@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>>  		return ERR_PTR(-ENOMEM);
>>  
>>  	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> -	if (!gpio_is_valid(pdata->gpio_attb))
>> +	if (!gpio_is_valid(pdata->gpio_attb)) {
>>  		dev_err(dev, "Failed to get ATTB GPIO\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>>  
>>  	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>>  		dev_err(dev, "Failed to get x-size property\n");
>> @@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	} else if (!pdata) {
>>  		dev_err(&client->dev, "platform data not defined\n");
>>  		return -EINVAL;
>> +	} else {
>> +		if (!gpio_is_valid(pdata->gpio_attb)) {
>> +			dev_err(dev, "Invalid gpio_attb in pdata\n");
>> +			return -EINVAL;
>> +		}
>>  	}
>>  
>>  	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
>> @@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  
>>  	input_set_drvdata(input, tsdata);
>>  
>> +	error = devm_gpio_request_one(dev, pdata->gpio_attb,
>> +			GPIOF_DIR_IN, "pixcir_i2c_attb");
>> +	if (error) {
>> +		dev_err(dev, "Failed to request ATTB gpio\n");
>> +		return error;
>> +	}
>> +
>>  	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
>>  				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>  				     client->name, tsdata);
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index f17c192..88ffdb50 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -44,7 +44,6 @@ enum pixcir_int_mode {
>>  #define PIXCIR_INT_POL_HIGH	(1UL << 2)
>>  
>>  struct pixcir_ts_platform_data {
>> -	int (*attb_read_val)(void);
>>  	unsigned int x_size;	/* X axis resolution */
>>  	unsigned int y_size;	/* Y axis resolution */
>>  	int gpio_attb;		/* GPIO connected to ATTB line */
>> -- 
>> 1.8.3.2
>>
> 

Re: [PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()

[Roger Quadros]

On 12/18/2013 07:50 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:16PM +0530, Roger Quadros wrote:
>> Get rid of the attb_read_val() platform hook. Instead,
>> read the ATTB gpio directly from the driver.
>>
>> Fail if valid ATTB gpio is not provided by patform data.
> 
> Do you also need to define polarity?
> 

In patch 3, in pixcir_start(), we explicitly configure the GPIO output
polarity as /* LEVEL_TOUCH interrupt with active low polarity */

So I don't think we need to define polarity.

cheers,
-roger

>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>>  drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
>>  include/linux/input/pixcir_ts.h           |  1 -
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 3370fd9..a783d94 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>>  static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
>>  {
>>  	struct pixcir_i2c_ts_data *tsdata = dev_id;
>> +	const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>>  
>>  	while (!tsdata->exiting) {
>>  		pixcir_ts_poscheck(tsdata);
>>  
>> -		if (tsdata->chip->attb_read_val())
>> +		if (gpio_get_value(pdata->gpio_attb))
>>  			break;
>>  
>>  		msleep(20);
>> @@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>>  		return ERR_PTR(-ENOMEM);
>>  
>>  	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> -	if (!gpio_is_valid(pdata->gpio_attb))
>> +	if (!gpio_is_valid(pdata->gpio_attb)) {
>>  		dev_err(dev, "Failed to get ATTB GPIO\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>>  
>>  	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>>  		dev_err(dev, "Failed to get x-size property\n");
>> @@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	} else if (!pdata) {
>>  		dev_err(&client->dev, "platform data not defined\n");
>>  		return -EINVAL;
>> +	} else {
>> +		if (!gpio_is_valid(pdata->gpio_attb)) {
>> +			dev_err(dev, "Invalid gpio_attb in pdata\n");
>> +			return -EINVAL;
>> +		}
>>  	}
>>  
>>  	tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
>> @@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  
>>  	input_set_drvdata(input, tsdata);
>>  
>> +	error = devm_gpio_request_one(dev, pdata->gpio_attb,
>> +			GPIOF_DIR_IN, "pixcir_i2c_attb");
>> +	if (error) {
>> +		dev_err(dev, "Failed to request ATTB gpio\n");
>> +		return error;
>> +	}
>> +
>>  	error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
>>  				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>  				     client->name, tsdata);
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index f17c192..88ffdb50 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -44,7 +44,6 @@ enum pixcir_int_mode {
>>  #define PIXCIR_INT_POL_HIGH	(1UL << 2)
>>  
>>  struct pixcir_ts_platform_data {
>> -	int (*attb_read_val)(void);
>>  	unsigned int x_size;	/* X axis resolution */
>>  	unsigned int y_size;	/* Y axis resolution */
>>  	int gpio_attb;		/* GPIO connected to ATTB line */
>> -- 
>> 1.8.3.2
>>
> 

Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Roger Quadros]

On 12/18/2013 07:44 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
>> +
>> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
>> +{
>> +	struct device *dev = &ts->client->dev;
>> +	int ret;
>> +
>> +	/* disable interrupt generation */
>> +	ret = pixcir_int_enable(ts, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to disable interrupt generation\n");
>> +		return ret;
>> +	}
>> +
>> +	disable_irq(ts->client->irq);
> 
> Why do you need to disable IRQ? If you disable interrupt generation in
> the chip I think you only need to call synchronize_irq() to make sure
> it's completed if it happens to be running. Also you need to move the
> code:

Agreed, no need to call disable_irq().

> 
> 	tsdata->exiting = true;
> 	mb();
> 
> here from pixcir_i2c_ts_remove() to make sure handler exits promptly.
> 
> You will also need to reset tsdata->exiting in your start method.
> 

OK.

cheers,
-roger

Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode

[Roger Quadros]

On 12/18/2013 07:44 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
>> +
>> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
>> +{
>> +	struct device *dev = &ts->client->dev;
>> +	int ret;
>> +
>> +	/* disable interrupt generation */
>> +	ret = pixcir_int_enable(ts, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to disable interrupt generation\n");
>> +		return ret;
>> +	}
>> +
>> +	disable_irq(ts->client->irq);
> 
> Why do you need to disable IRQ? If you disable interrupt generation in
> the chip I think you only need to call synchronize_irq() to make sure
> it's completed if it happens to be running. Also you need to move the
> code:

Agreed, no need to call disable_irq().

> 
> 	tsdata->exiting = true;
> 	mb();
> 
> here from pixcir_i2c_ts_remove() to make sure handler exits promptly.
> 
> You will also need to reset tsdata->exiting in your start method.
> 

OK.

cheers,
-roger

Re: [PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Roger Quadros]

On 12/18/2013 07:39 PM, Dmitry Torokhov wrote:
> Hi Roger,
> 
> On Wed, Dec 18, 2013 at 02:51:12PM +0530, Roger Quadros wrote:
>> Provide device tree support and binding information.
>> Change platform data parameters from x/y_max to x/y_size..
> 
> I'd rather keep them as they were.

OK.

> 
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>>  .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
>>  drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
>>  include/linux/input/pixcir_ts.h                    |  5 +-
>>  3 files changed, 101 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> new file mode 100644
>> index 0000000..c0b0b270
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> @@ -0,0 +1,26 @@
>> +* Pixcir I2C touchscreen controllers
>> +
>> +Required properties:
>> +- compatible: must be "pixcir,pixcir_ts"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +- attb-gpio: GPIO connected to the ATTB line of the chip
>> +- x-size: horizontal resolution of touchscreen
>> +- y-size: vertical resolution of touchscreen
>> +
>> +Example:
>> +
>> +	i2c@00000000 {
>> +		/* ... */
>> +
>> +		pixcir_ts@5c {
>> +			compatible = "pixcir,pixcir_ts";
>> +			reg = <0x5c>;
>> +			interrupts = <2 0>;
>> +			attb-gpio = <&gpf 2 0 2>;
>> +			x-size = <800>;
>> +			y-size = <600>;
>> +		};
>> +
>> +		/* ... */
>> +	};
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 6cc6b36..3a447c9 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/i2c.h>
>>  #include <linux/input.h>
>>  #include <linux/input/pixcir_ts.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_device.h>
>>  
>>  struct pixcir_i2c_ts_data {
>>  	struct i2c_client *client;
>> @@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
>>  			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
>>  
>> +#if defined(CONFIG_OF)
> 
> #ifdef is preferred for simple conditions.
> 
>> +static const struct of_device_id pixcir_of_match[];
>> +
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	struct pixcir_ts_platform_data *pdata;
>> +	struct device_node *np = dev->of_node;
>> +	const struct of_device_id *match;
>> +
>> +	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
>> +	if (!match)
>> +		return ERR_PTR(-EINVAL);
> 
> Why do you need an explicit match here?
> 

Right, it is not needed here for this patch but used later in patch 6
to get chip specific data. I'll fix this while re-arranging the patches.

>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> +	if (!gpio_is_valid(pdata->gpio_attb))
>> +		dev_err(dev, "Failed to get ATTB GPIO\n");
>> +
>> +	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>> +		dev_err(dev, "Failed to get x-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
>> +		dev_err(dev, "Failed to get y-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
>> +				pdata->x_size, pdata->y_size, pdata->gpio_attb);
>> +
>> +	return pdata;
>> +}
>> +#else
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	return NULL;
> 
> This should be
> 
> 	return ERR_PTR(-EINVAL);
> 
> since you test it with IS_ERR() later.

OK.

> 
>> +}
>> +#endif
>> +
>>  static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  					 const struct i2c_device_id *id)
>>  {
>>  	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
>> +	struct device *dev = &client->dev;
>> +	struct device_node *np = dev->of_node;
>>  	struct pixcir_i2c_ts_data *tsdata;
>>  	struct input_dev *input;
>>  	int error;
>>  
>> -	if (!pdata) {
>> +	if (np) {
>> +		pdata = pixcir_parse_dt(dev);
>> +		if (IS_ERR(pdata))
>> +			return PTR_ERR(pdata);
> 
> We should be favoring kernel-provided platform data if it is pesent even
> if there is dt-data available. This way user can override firmware,m if
> needed.
> 
OK. But how can the user override kernel provided platform data? Can't device
tree overlays be used to patch DT data in the same fashion.

I've not tried either so I don't know which one is more user friendly.

>> +
>> +	} else if (!pdata) {
>>  		dev_err(&client->dev, "platform data not defined\n");
>>  		return -EINVAL;
>>  	}
>> @@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	__set_bit(EV_KEY, input->evbit);
>>  	__set_bit(EV_ABS, input->evbit);
>>  	__set_bit(BTN_TOUCH, input->keybit);
>> -	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
>> +	input_set_abs_params(input, ABS_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>>  
>>  	input_set_drvdata(input, tsdata);
>>  
>> @@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
>>  
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id pixcir_of_match[] = {
>> +	{ .compatible = "pixcir,pixcir_ts", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pixcir_of_match);
>> +#endif
> 
> #ifdef is preferred for simple conditions.
> 
OK.

>> +
>>  static struct i2c_driver pixcir_i2c_ts_driver = {
>>  	.driver = {
>>  		.owner	= THIS_MODULE,
>>  		.name	= "pixcir_ts",
>>  		.pm	= &pixcir_dev_pm_ops,
>> +		.of_match_table = of_match_ptr(pixcir_of_match),
>>  	},
>>  	.probe		= pixcir_i2c_ts_probe,
>>  	.remove		= pixcir_i2c_ts_remove,
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index 7163d91..b34ff7e 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -3,8 +3,9 @@
>>  
>>  struct pixcir_ts_platform_data {
>>  	int (*attb_read_val)(void);
>> -	int x_max;
>> -	int y_max;
>> +	unsigned int x_size;	/* X axis resolution */
>> +	unsigned int y_size;	/* Y axis resolution */
>> +	int gpio_attb;		/* GPIO connected to ATTB line */
>>  };
> 
> OK, it looks like the series were split awkwardly: you are defining data
> that is used by follow-up patches and its purpose is unclear when
> reviewing patch-by-patch. I'd recommend rearranging so that DT support
> patch is the very last in the series.
> 
Got it.

>>  
>>  #endif
>> -- 
>> 1.8.3.2

cheers,
-roger

Re: [PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Roger Quadros]

On 12/18/2013 07:39 PM, Dmitry Torokhov wrote:
> Hi Roger,
> 
> On Wed, Dec 18, 2013 at 02:51:12PM +0530, Roger Quadros wrote:
>> Provide device tree support and binding information.
>> Change platform data parameters from x/y_max to x/y_size..
> 
> I'd rather keep them as they were.

OK.

> 
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>>  .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
>>  drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
>>  include/linux/input/pixcir_ts.h                    |  5 +-
>>  3 files changed, 101 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> new file mode 100644
>> index 0000000..c0b0b270
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> @@ -0,0 +1,26 @@
>> +* Pixcir I2C touchscreen controllers
>> +
>> +Required properties:
>> +- compatible: must be "pixcir,pixcir_ts"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +- attb-gpio: GPIO connected to the ATTB line of the chip
>> +- x-size: horizontal resolution of touchscreen
>> +- y-size: vertical resolution of touchscreen
>> +
>> +Example:
>> +
>> +	i2c@00000000 {
>> +		/* ... */
>> +
>> +		pixcir_ts@5c {
>> +			compatible = "pixcir,pixcir_ts";
>> +			reg = <0x5c>;
>> +			interrupts = <2 0>;
>> +			attb-gpio = <&gpf 2 0 2>;
>> +			x-size = <800>;
>> +			y-size = <600>;
>> +		};
>> +
>> +		/* ... */
>> +	};
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 6cc6b36..3a447c9 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/i2c.h>
>>  #include <linux/input.h>
>>  #include <linux/input/pixcir_ts.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_device.h>
>>  
>>  struct pixcir_i2c_ts_data {
>>  	struct i2c_client *client;
>> @@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
>>  			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
>>  
>> +#if defined(CONFIG_OF)
> 
> #ifdef is preferred for simple conditions.
> 
>> +static const struct of_device_id pixcir_of_match[];
>> +
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	struct pixcir_ts_platform_data *pdata;
>> +	struct device_node *np = dev->of_node;
>> +	const struct of_device_id *match;
>> +
>> +	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
>> +	if (!match)
>> +		return ERR_PTR(-EINVAL);
> 
> Why do you need an explicit match here?
> 

Right, it is not needed here for this patch but used later in patch 6
to get chip specific data. I'll fix this while re-arranging the patches.

>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> +	if (!gpio_is_valid(pdata->gpio_attb))
>> +		dev_err(dev, "Failed to get ATTB GPIO\n");
>> +
>> +	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>> +		dev_err(dev, "Failed to get x-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
>> +		dev_err(dev, "Failed to get y-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
>> +				pdata->x_size, pdata->y_size, pdata->gpio_attb);
>> +
>> +	return pdata;
>> +}
>> +#else
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	return NULL;
> 
> This should be
> 
> 	return ERR_PTR(-EINVAL);
> 
> since you test it with IS_ERR() later.

OK.

> 
>> +}
>> +#endif
>> +
>>  static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  					 const struct i2c_device_id *id)
>>  {
>>  	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
>> +	struct device *dev = &client->dev;
>> +	struct device_node *np = dev->of_node;
>>  	struct pixcir_i2c_ts_data *tsdata;
>>  	struct input_dev *input;
>>  	int error;
>>  
>> -	if (!pdata) {
>> +	if (np) {
>> +		pdata = pixcir_parse_dt(dev);
>> +		if (IS_ERR(pdata))
>> +			return PTR_ERR(pdata);
> 
> We should be favoring kernel-provided platform data if it is pesent even
> if there is dt-data available. This way user can override firmware,m if
> needed.
> 
OK. But how can the user override kernel provided platform data? Can't device
tree overlays be used to patch DT data in the same fashion.

I've not tried either so I don't know which one is more user friendly.

>> +
>> +	} else if (!pdata) {
>>  		dev_err(&client->dev, "platform data not defined\n");
>>  		return -EINVAL;
>>  	}
>> @@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	__set_bit(EV_KEY, input->evbit);
>>  	__set_bit(EV_ABS, input->evbit);
>>  	__set_bit(BTN_TOUCH, input->keybit);
>> -	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
>> +	input_set_abs_params(input, ABS_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>>  
>>  	input_set_drvdata(input, tsdata);
>>  
>> @@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
>>  
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id pixcir_of_match[] = {
>> +	{ .compatible = "pixcir,pixcir_ts", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pixcir_of_match);
>> +#endif
> 
> #ifdef is preferred for simple conditions.
> 
OK.

>> +
>>  static struct i2c_driver pixcir_i2c_ts_driver = {
>>  	.driver = {
>>  		.owner	= THIS_MODULE,
>>  		.name	= "pixcir_ts",
>>  		.pm	= &pixcir_dev_pm_ops,
>> +		.of_match_table = of_match_ptr(pixcir_of_match),
>>  	},
>>  	.probe		= pixcir_i2c_ts_probe,
>>  	.remove		= pixcir_i2c_ts_remove,
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index 7163d91..b34ff7e 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -3,8 +3,9 @@
>>  
>>  struct pixcir_ts_platform_data {
>>  	int (*attb_read_val)(void);
>> -	int x_max;
>> -	int y_max;
>> +	unsigned int x_size;	/* X axis resolution */
>> +	unsigned int y_size;	/* Y axis resolution */
>> +	int gpio_attb;		/* GPIO connected to ATTB line */
>>  };
> 
> OK, it looks like the series were split awkwardly: you are defining data
> that is used by follow-up patches and its purpose is unclear when
> reviewing patch-by-patch. I'd recommend rearranging so that DT support
> patch is the very last in the series.
> 
Got it.

>>  
>>  #endif
>> -- 
>> 1.8.3.2

cheers,
-roger

Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting

[Henrik Rydberg]

Hi Roger,

> It seems the controller the original driver was written for does not report
> touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits into
> Type B reporting model.

Look in drivers/input/mouse/cypress_ps2.c for an example of how to deal with
anonymous touch coordinates.

Thanks,
Henrik