[PATCH 0/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's

[PATCH 0/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's

[Hans de Goede]

Hi Dimitri,

Here is a new driver for the resistive touchscreen controller found in
Allwinner sunxi SoC's. Please review, and when ready pull this driver into
your tree for Linus.

This series consist of 5 patches, the 1st patch is the actual new driver,
the 2nd patch extends it with support for the temperature sensor built into
the rtp controller. Since this depends on the 1st one I would like to ask
you to pull it into your tree too. I've written and maintain several hwmon
drivers, so I'm confident the hwmon bits are solid, I've also CC-ed the
sensors list so that they can review and if they've any comments I will of
course fix those and send a new version.

Patch 3-5 are dts patches adding the controller to the dts files describing
the sunxi SoC variants. These will be merged through Maxime Ripard's tree,
and are here so that the dts bits can be reviewed at the same time. I'll ask
Maxime to merge them once you're happy with Patch 1 + 2.

Thanks & Regards,

Hans

[PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Hans de Goede]

Note the sun4i-ts controller is capable of detecting a second touch, but when
a second touch is present then the accuracy becomes so bad the reported touch
location is not useable.

The original android driver contains some complicated heuristics using the
aprox. distance between the 2 touches to see if the user is making a pinch
open / close movement, and then reports emulated multi-touch events around
the last touch coordinate (as the dual-touch coordinates are worthless).

These kinds of heuristics are just asking for trouble (and don't belong
in the kernel). So this driver offers straight forward, reliable single
touch functionality only.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/input/touchscreen/Kconfig    |  10 ++
 drivers/input/touchscreen/Makefile   |   1 +
 drivers/input/touchscreen/sun4i-ts.c | 272 +++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+)
 create mode 100644 drivers/input/touchscreen/sun4i-ts.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 961d58d..95023de 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -906,6 +906,16 @@ config TOUCHSCREEN_STMPE
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmpe-ts.
 
+config TOUCHSCREEN_SUN4I
+	tristate "Allwinner sun4i resistive touchscreen controller support"
+	depends on ARCH_SUNXI
+	help
+	  This selects support for the resistive touchscreen controller
+	  found on Allwinner sunxi SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sun4i-ts.
+
 config TOUCHSCREEN_SUR40
 	tristate "Samsung SUR40 (Surface 2.0/PixelSense) touchscreen"
 	depends on USB
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62801f2..c8f7375 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TOUCHSCREEN_PIXCIR)	+= pixcir_i2c_ts.o
 obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
 obj-$(CONFIG_TOUCHSCREEN_ST1232)	+= st1232.o
 obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
+obj-$(CONFIG_TOUCHSCREEN_SUN4I)		+= sun4i-ts.o
 obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
 obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
 obj-$(CONFIG_TOUCHSCREEN_TNETV107X)	+= tnetv107x-ts.o
diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
new file mode 100644
index 0000000..b3f01c0
--- /dev/null
+++ b/drivers/input/touchscreen/sun4i-ts.c
@@ -0,0 +1,272 @@
+/*
+ * sun4i-ts.c Allwinner sunxi resistive touchscreen controller driver
+ *
+ * Copyright (C) 2013 - 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * The sun4i-ts controller is capable of detecting a second touch, but when a
+ * second touch is present then the accuracy becomes so bad the reported touch
+ * location is not useable.
+ *
+ * The original android driver contains some complicated heuristics using the
+ * aprox. distance between the 2 touches to see if the user is making a pinch
+ * open / close movement, and then reports emulated multi-touch events around
+ * the last touch coordinate (as the dual-touch coordinates are worthless).
+ *
+ * These kinds of heuristics are just asking for trouble (and don't belong
+ * in the kernel). So this driver offers straight forward, reliable single
+ * touch functionality only.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define TP_CTRL0		0x00
+#define TP_CTRL1		0x04
+#define TP_CTRL2		0x08
+#define TP_CTRL3		0x0c
+#define TP_INT_FIFOC		0x10
+#define TP_INT_FIFOS		0x14
+#define TP_TPR			0x18
+#define TP_CDAT			0x1c
+#define TEMP_DATA		0x20
+#define TP_DATA			0x24
+
+/* TP_CTRL0 bits */
+#define ADC_FIRST_DLY(x)	((x) << 24) /* 8 bits */
+#define ADC_FIRST_DLY_MODE(x)	((x) << 23)
+#define ADC_CLK_SEL(x)		((x) << 22)
+#define ADC_CLK_DIV(x)		((x) << 20) /* 3 bits */
+#define FS_DIV(x)		((x) << 16) /* 4 bits */
+#define T_ACQ(x)		((x) << 0) /* 16 bits */
+
+/* TP_CTRL1 bits */
+#define STYLUS_UP_DEBOUN(x)	((x) << 12) /* 8 bits */
+#define STYLUS_UP_DEBOUN_EN(x)	((x) << 9)
+#define TOUCH_PAN_CALI_EN(x)	((x) << 6)
+#define TP_DUAL_EN(x)		((x) << 5)
+#define TP_MODE_EN(x)		((x) << 4)
+#define TP_ADC_SELECT(x)	((x) << 3)
+#define ADC_CHAN_SELECT(x)	((x) << 0)  /* 3 bits */
+
+/* TP_CTRL2 bits */
+#define TP_SENSITIVE_ADJUST(x)	((x) << 28) /* 4 bits */
+#define TP_MODE_SELECT(x)	((x) << 26) /* 2 bits */
+#define PRE_MEA_EN(x)		((x) << 24)
+#define PRE_MEA_THRE_CNT(x)	((x) << 0) /* 24 bits */
+
+/* TP_CTRL3 bits */
+#define FILTER_EN(x)		((x) << 2)
+#define FILTER_TYPE(x)		((x) << 0)  /* 2 bits */
+
+/* TP_INT_FIFOC irq and fifo mask / control bits */
+#define TEMP_IRQ_EN(x)		((x) << 18)
+#define OVERRUN_IRQ_EN(x)	((x) << 17)
+#define DATA_IRQ_EN(x)		((x) << 16)
+#define TP_DATA_XY_CHANGE(x)	((x) << 13)
+#define FIFO_TRIG(x)		((x) << 8)  /* 5 bits */
+#define DATA_DRQ_EN(x)		((x) << 7)
+#define FIFO_FLUSH(x)		((x) << 4)
+#define TP_UP_IRQ_EN(x)		((x) << 1)
+#define TP_DOWN_IRQ_EN(x)	((x) << 0)
+
+/* TP_INT_FIFOS irq and fifo status bits */
+#define TEMP_DATA_PENDING	(1 << 18)
+#define FIFO_OVERRUN_PENDING	(1 << 17)
+#define FIFO_DATA_PENDING	(1 << 16)
+#define TP_IDLE_FLG		(1 << 2)
+#define TP_UP_PENDING		(1 << 1)
+#define TP_DOWN_PENDING		(1 << 0)
+
+struct sun4i_ts_data {
+	struct device *dev;
+	void __iomem *base;
+	struct input_dev *input;
+	int ignore_fifo_data;
+};
+
+static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
+{
+	struct sun4i_ts_data *ts = dev_id;
+	u32 reg_val, x, y;
+
+	reg_val  = readl(ts->base + TP_INT_FIFOS);
+
+	if (reg_val & FIFO_DATA_PENDING) {
+		x = readl(ts->base + TP_DATA);
+		y = readl(ts->base + TP_DATA);
+		/* The 1st location reported after an up event is unreliable */
+		if (!ts->ignore_fifo_data) {
+			input_report_abs(ts->input, ABS_X, x);
+			input_report_abs(ts->input, ABS_Y, y);
+			/*
+			 * The hardware has a separate down status bit, but
+			 * that gets set before we get the first location,
+			 * resulting in reporting a click on the old location.
+			 */
+			input_report_key(ts->input, BTN_TOUCH, 1);
+			input_sync(ts->input);
+		} else
+			ts->ignore_fifo_data = 0;
+	}
+
+	if (reg_val & TP_UP_PENDING) {
+		ts->ignore_fifo_data = 1;
+		input_report_key(ts->input, BTN_TOUCH, 0);
+		input_sync(ts->input);
+	}
+
+	writel(reg_val, ts->base + TP_INT_FIFOS);
+
+	return IRQ_HANDLED;
+}
+
+static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
+					     const char *name)
+{
+	int ret;
+	struct input_dev *input;
+
+	input = devm_input_allocate_device(ts->dev);
+	if (!input)
+		return -ENOMEM;
+
+	input->name = name;
+	input->phys = "sun4i_ts/input0";
+	input->id.bustype = BUS_HOST;
+	input->id.vendor = 0x0001;
+	input->id.product = 0x0001;
+	input->id.version = 0x0100;
+	input->dev.parent = ts->dev;
+	input->evbit[0] =  BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS);
+	set_bit(BTN_TOUCH, input->keybit);
+	input_set_abs_params(input, ABS_X, 0, 4095, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, 4095, 0, 0);
+
+	ret = input_register_device(input);
+	if (ret)
+		return ret;
+
+	ts->input = input;
+	return 0;
+}
+
+static int sun4i_ts_remove(struct platform_device *pdev)
+{
+	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
+
+	/* Deactivate all IRQs */
+	writel(0, ts->base + TP_INT_FIFOC);
+	msleep(20);
+
+	if (ts->input)
+		input_unregister_device(ts->input);
+
+	kfree(ts);
+
+	return 0;
+}
+
+static int sun4i_ts_probe(struct platform_device *pdev)
+{
+	struct sun4i_ts_data *ts;
+	int ret = -ENOMEM;
+
+	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ts);
+
+	ts->dev = &pdev->dev;
+	ts->base = devm_ioremap_resource(&pdev->dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(ts->base)) {
+		ret = PTR_ERR(ts->base);
+		goto error;
+	}
+
+	ret = devm_request_irq(&pdev->dev, platform_get_irq(pdev, 0),
+			       sun4i_ts_irq, 0, "sun4i-ts", ts);
+	if (ret)
+		goto error;
+
+	ret = sun4i_ts_register_input(ts, pdev->name);
+	if (ret)
+		goto error;
+
+	/*
+	 * Select HOSC clk, clkin = clk / 6, adc samplefreq = clkin / 8192,
+	 * t_acq = clkin / (16 * 64)
+	 */
+	writel(ADC_CLK_SEL(0) | ADC_CLK_DIV(2) | FS_DIV(7) | T_ACQ(63),
+	       ts->base + TP_CTRL0);
+
+	/*
+	 * sensitive_adjust = 15 : max, which is not all that sensitive,
+	 * tp_mode = 0 : only x and y coordinates, as we don't use dual touch
+	 */
+	writel(TP_SENSITIVE_ADJUST(15) | TP_MODE_SELECT(0),
+	       ts->base + TP_CTRL2);
+
+	/* Enable median filter, type 1 : 5/3 */
+	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
+
+	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
+	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
+	       ts->base + TP_INT_FIFOC);
+
+	/*
+	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
+	 * finally enable tp mode.
+	 */
+	writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1),
+	       ts->base + TP_CTRL1);
+
+	return 0;
+
+error:
+	sun4i_ts_remove(pdev);
+	return ret;
+}
+
+static const struct of_device_id sun4i_ts_of_match[] = {
+	{ .compatible = "allwinner,sun4i-ts", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun4i_ts_of_match);
+
+static struct platform_driver sun4i_ts_driver = {
+	.driver = {
+		.owner	= THIS_MODULE,
+		.name	= "sun4i-ts",
+		.of_match_table = of_match_ptr(sun4i_ts_of_match),
+	},
+	.probe	= sun4i_ts_probe,
+	.remove	= sun4i_ts_remove,
+};
+
+module_platform_driver(sun4i_ts_driver);
+
+MODULE_DESCRIPTION("Allwinner sun4i resistive touchscreen controller driver");
+MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
1.8.4.2

[PATCH 2/5] input: sun4i-ts: Add support for temperature sensor

[Hans de Goede]

The sun4i resisitive touchscreen controller also comes with a built-in
temperature sensor. This commit adds support for it.

This commit also introduces a new "ts-attached" device-tree attribute,
when this is not set, the input part of the driver won't register. This way
the internal temperature sensor can be used to measure the SoC temperature
independent of there actually being a touchscreen attached to the controller.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/input/touchscreen/sun4i-ts.c | 106 +++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
index b3f01c0..15a486d 100644
--- a/drivers/input/touchscreen/sun4i-ts.c
+++ b/drivers/input/touchscreen/sun4i-ts.c
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2013 - 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  *
+ * The hwmon parts are based on work by Corentin LABBE which is:
+ * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -31,6 +34,7 @@
 
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
@@ -97,10 +101,16 @@
 #define TP_UP_PENDING		(1 << 1)
 #define TP_DOWN_PENDING		(1 << 0)
 
+/* TP_TPR bits */
+#define TEMP_ENABLE(x)		((x) << 16)
+#define TEMP_PERIOD(x)		((x) << 0)  /* t = x * 256 * 16 / clkin */
+
 struct sun4i_ts_data {
 	struct device *dev;
 	void __iomem *base;
 	struct input_dev *input;
+	struct device *hwmon;
+	atomic_t temp_data;
 	int ignore_fifo_data;
 };
 
@@ -111,6 +121,12 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
 
 	reg_val  = readl(ts->base + TP_INT_FIFOS);
 
+	if (reg_val & TEMP_DATA_PENDING)
+		atomic_set(&ts->temp_data, readl(ts->base + TEMP_DATA));
+
+	if (!ts->input)
+		goto leave;
+
 	if (reg_val & FIFO_DATA_PENDING) {
 		x = readl(ts->base + TP_DATA);
 		y = readl(ts->base + TP_DATA);
@@ -135,6 +151,7 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
 		input_sync(ts->input);
 	}
 
+leave:
 	writel(reg_val, ts->base + TP_INT_FIFOS);
 
 	return IRQ_HANDLED;
@@ -170,6 +187,66 @@ static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
 	return 0;
 }
 
+static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	return sprintf(buf, "sun4i_ts\n");
+}
+
+static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct sun4i_ts_data *ts = dev_get_drvdata(dev);
+	int temp_data;
+
+	temp_data = atomic_read(&ts->temp_data);
+	/* No temp_data until the first irq */
+	if (temp_data == -1)
+		return -EAGAIN;
+
+	return sprintf(buf, "%d\n", (temp_data - 1447) * 100);
+}
+
+static ssize_t show_temp_label(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	return sprintf(buf, "SoC temperature\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
+static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp_label, NULL);
+
+static struct attribute *sun4i_ts_attributes[] = {
+	&dev_attr_name.attr,
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_label.attr,
+	NULL
+};
+
+static const struct attribute_group sun4i_ts_attr_group = {
+	.attrs = sun4i_ts_attributes,
+};
+
+static int sun4i_ts_register_hwmon(struct sun4i_ts_data *ts)
+{
+	int ret;
+
+	atomic_set(&ts->temp_data, -1);
+
+	ret = sysfs_create_group(&ts->dev->kobj, &sun4i_ts_attr_group);
+	if (ret)
+		return ret;
+
+	ts->hwmon = hwmon_device_register(ts->dev);
+	if (IS_ERR(ts->hwmon)) {
+		ret = PTR_ERR(ts->hwmon);
+		ts->hwmon = NULL;
+	}
+
+	return ret;
+}
+
 static int sun4i_ts_remove(struct platform_device *pdev)
 {
 	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
@@ -178,9 +255,13 @@ static int sun4i_ts_remove(struct platform_device *pdev)
 	writel(0, ts->base + TP_INT_FIFOC);
 	msleep(20);
 
+	if (ts->hwmon)
+		hwmon_device_unregister(ts->hwmon);
+
 	if (ts->input)
 		input_unregister_device(ts->input);
 
+	sysfs_remove_group(&ts->dev->kobj, &sun4i_ts_attr_group);
 	kfree(ts);
 
 	return 0;
@@ -189,7 +270,11 @@ static int sun4i_ts_remove(struct platform_device *pdev)
 static int sun4i_ts_probe(struct platform_device *pdev)
 {
 	struct sun4i_ts_data *ts;
+	struct device_node *np = pdev->dev.of_node;
 	int ret = -ENOMEM;
+	u32 ts_attached = 0;
+
+	of_property_read_u32(np, "ts-attached", &ts_attached);
 
 	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);
 	if (!ts)
@@ -210,7 +295,13 @@ static int sun4i_ts_probe(struct platform_device *pdev)
 	if (ret)
 		goto error;
 
-	ret = sun4i_ts_register_input(ts, pdev->name);
+	if (ts_attached) {
+		ret = sun4i_ts_register_input(ts, pdev->name);
+		if (ret)
+			goto error;
+	}
+
+	ret = sun4i_ts_register_hwmon(ts);
 	if (ret)
 		goto error;
 
@@ -231,9 +322,16 @@ static int sun4i_ts_probe(struct platform_device *pdev)
 	/* Enable median filter, type 1 : 5/3 */
 	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
 
-	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
-	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
-	       ts->base + TP_INT_FIFOC);
+	if (ts_attached) {
+		/* Flush, set trig level to 1, enable temp, data and up irqs */
+		writel(TEMP_IRQ_EN(1) | DATA_IRQ_EN(1) | FIFO_TRIG(1) |
+			FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
+		       ts->base + TP_INT_FIFOC);
+	} else
+		writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC);
+
+	/* Enable temperature measurement, period 1953 (2 seconds) */
+	writel(TEMP_ENABLE(1) | TEMP_PERIOD(1953), ts->base + TP_TPR);
 
 	/*
 	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
-- 
1.8.4.2

[PATCH 3/5] ARM: dts: sun4i: Add rtp controller node

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index de1dd73..67ccdec 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -452,6 +452,13 @@
 			reg = <0x01c23800 0x10>;
 		};
 
+		rtp: rtp@01c25000 {
+			compatible = "allwinner,sun4i-ts";
+			reg = <0x01c25000 0x100>;
+			interrupts = <29>;
+			ts-attached = <0>;
+		};
+
 		uart0: serial@01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
-- 
1.8.4.2

[PATCH 4/5] ARM: dts: sun5i: Add rtp controller node

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-a10s.dtsi | 7 +++++++
 arch/arm/boot/dts/sun5i-a13.dtsi  | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
index 948eb10..df9ba3b 100644
--- a/arch/arm/boot/dts/sun5i-a10s.dtsi
+++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
@@ -409,6 +409,13 @@
 			reg = <0x01c23800 0x10>;
 		};
 
+		rtp: rtp@01c25000 {
+			compatible = "allwinner,sun4i-ts";
+			reg = <0x01c25000 0x100>;
+			interrupts = <29>;
+			ts-attached = <0>;
+		};
+
 		uart0: serial@01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index ca15672..4de2adb 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -367,6 +367,13 @@
 			reg = <0x01c23800 0x10>;
 		};
 
+		rtp: rtp@01c25000 {
+			compatible = "allwinner,sun4i-ts";
+			reg = <0x01c25000 0x100>;
+			interrupts = <29>;
+			ts-attached = <0>;
+		};
+
 		uart1: serial@01c28400 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28400 0x400>;
-- 
1.8.4.2

[PATCH 5/5] ARM: dts: sun7i: Add rtp controller node

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 3231789..fdfe4b2 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -536,6 +536,13 @@
 			reg = <0x01c23800 0x200>;
 		};
 
+		rtp: rtp@01c25000 {
+			compatible = "allwinner,sun4i-ts";
+			reg = <0x01c25000 0x100>;
+			interrupts = <0 29 4>;
+			ts-attached = <0>;
+		};
+
 		uart0: serial@01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
-- 
1.8.4.2

Re: [lm-sensors] [PATCH 2/5] input: sun4i-ts: Add support for temperature sensor

[Guenter Roeck]

On Tue, Dec 24, 2013 at 11:24:04PM +0100, Hans de Goede wrote:
> The sun4i resisitive touchscreen controller also comes with a built-in
> temperature sensor. This commit adds support for it.
> 
> This commit also introduces a new "ts-attached" device-tree attribute,
> when this is not set, the input part of the driver won't register. This way
> the internal temperature sensor can be used to measure the SoC temperature
> independent of there actually being a touchscreen attached to the controller.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Hi Hans,

can you use the new hwmon_device_register_with_groups API instead,
or even devm_hwmon_device_register_with_groups ?

This way you don't have to create and remove the sysfs group, you don't have to
create the name attribute, and if you use the devm_ API you don't have to remove
anything. Also, the hwmon attributes will be attached to the hwmon device,
meaning the platform device won't be cluttered with hwmon attributes and
vice versa.

Couple of related comments inline below.

Thanks,
Guenter

> ---
>  drivers/input/touchscreen/sun4i-ts.c | 106 +++++++++++++++++++++++++++++++++--
>  1 file changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
> index b3f01c0..15a486d 100644
> --- a/drivers/input/touchscreen/sun4i-ts.c
> +++ b/drivers/input/touchscreen/sun4i-ts.c
> @@ -3,6 +3,9 @@
>   *
>   * Copyright (C) 2013 - 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>   *
> + * The hwmon parts are based on work by Corentin LABBE which is:
> + * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the License, or
> @@ -31,6 +34,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/err.h>
> +#include <linux/hwmon.h>
>  #include <linux/init.h>
>  #include <linux/input.h>
>  #include <linux/interrupt.h>
> @@ -97,10 +101,16 @@
>  #define TP_UP_PENDING		(1 << 1)
>  #define TP_DOWN_PENDING		(1 << 0)
>  
> +/* TP_TPR bits */
> +#define TEMP_ENABLE(x)		((x) << 16)
> +#define TEMP_PERIOD(x)		((x) << 0)  /* t = x * 256 * 16 / clkin */
> +
>  struct sun4i_ts_data {
>  	struct device *dev;
>  	void __iomem *base;
>  	struct input_dev *input;
> +	struct device *hwmon;
> +	atomic_t temp_data;

Unless I am missing something, this variable does not have to be an atomic.
Either it was written or it wasn't, but the order should not matter.

>  	int ignore_fifo_data;
>  };
>  
> @@ -111,6 +121,12 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
>  
>  	reg_val  = readl(ts->base + TP_INT_FIFOS);
>  
> +	if (reg_val & TEMP_DATA_PENDING)
> +		atomic_set(&ts->temp_data, readl(ts->base + TEMP_DATA));
> +
> +	if (!ts->input)
> +		goto leave;
> +
>  	if (reg_val & FIFO_DATA_PENDING) {
>  		x = readl(ts->base + TP_DATA);
>  		y = readl(ts->base + TP_DATA);
> @@ -135,6 +151,7 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
>  		input_sync(ts->input);
>  	}
>  
> +leave:
>  	writel(reg_val, ts->base + TP_INT_FIFOS);
>  
>  	return IRQ_HANDLED;
> @@ -170,6 +187,66 @@ static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
>  	return 0;
>  }
>  
> +static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	return sprintf(buf, "sun4i_ts\n");
> +}
> +
> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct sun4i_ts_data *ts = dev_get_drvdata(dev);
> +	int temp_data;
> +
> +	temp_data = atomic_read(&ts->temp_data);
> +	/* No temp_data until the first irq */
> +	if (temp_data == -1)
> +		return -EAGAIN;
> +
> +	return sprintf(buf, "%d\n", (temp_data - 1447) * 100);
> +}
> +
> +static ssize_t show_temp_label(struct device *dev,
> +			      struct device_attribute *devattr, char *buf)
> +{
> +	return sprintf(buf, "SoC temperature\n");
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
> +static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp_label, NULL);
> +
> +static struct attribute *sun4i_ts_attributes[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_temp1_input.attr,
> +	&dev_attr_temp1_label.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group sun4i_ts_attr_group = {
> +	.attrs = sun4i_ts_attributes,
> +};

If you rename the attributes to sun4i_ts_attrs, you should be able to use
ATTRIBUTE_GROUPS() here (this gives you a list of groups which you'll need
if you use the new API).

> +
> +static int sun4i_ts_register_hwmon(struct sun4i_ts_data *ts)
> +{
> +	int ret;
> +
> +	atomic_set(&ts->temp_data, -1);
> +
> +	ret = sysfs_create_group(&ts->dev->kobj, &sun4i_ts_attr_group);
> +	if (ret)
> +		return ret;
> +
> +	ts->hwmon = hwmon_device_register(ts->dev);
> +	if (IS_ERR(ts->hwmon)) {
> +		ret = PTR_ERR(ts->hwmon);
> +		ts->hwmon = NULL;
> +	}
> +
> +	return ret;

With the new API, this would be something like

	hwmon_dev = devm_hwmon_device_register_with_groups(ts->dev, "sun4i_ts",
							   ts, sun4i_ts_attrs);
	return PTR_ERR_OR_ZERO(hwmon_dev);

With the devm_ API, you don't have to store the hwmon device as you don't need
to access it anymore from within the driver. Note that you can only use it if
you also use devm_kzalloc to allocate ts; otherwise you would have a race condition
during cleanup.

> +}
> +
>  static int sun4i_ts_remove(struct platform_device *pdev)
>  {
>  	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
> @@ -178,9 +255,13 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>  	writel(0, ts->base + TP_INT_FIFOC);
>  	msleep(20);
>  
> +	if (ts->hwmon)
> +		hwmon_device_unregister(ts->hwmon);
> +
>  	if (ts->input)
>  		input_unregister_device(ts->input);
>  
> +	sysfs_remove_group(&ts->dev->kobj, &sun4i_ts_attr_group);
>  	kfree(ts);
>  
>  	return 0;
> @@ -189,7 +270,11 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>  static int sun4i_ts_probe(struct platform_device *pdev)
>  {
>  	struct sun4i_ts_data *ts;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret = -ENOMEM;
> +	u32 ts_attached = 0;
> +
> +	of_property_read_u32(np, "ts-attached", &ts_attached);
>  
>  	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);

Different patch, but why not just use devm_kzalloc() ? Since you are using devm_
functions elsewhere, that would seem natural. I'll also comment on that in the
other patch.

>  	if (!ts)
> @@ -210,7 +295,13 @@ static int sun4i_ts_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto error;
>  
> -	ret = sun4i_ts_register_input(ts, pdev->name);
> +	if (ts_attached) {
> +		ret = sun4i_ts_register_input(ts, pdev->name);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	ret = sun4i_ts_register_hwmon(ts);
>  	if (ret)
>  		goto error;
>  
> @@ -231,9 +322,16 @@ static int sun4i_ts_probe(struct platform_device *pdev)
>  	/* Enable median filter, type 1 : 5/3 */
>  	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
>  
> -	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
> -	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
> -	       ts->base + TP_INT_FIFOC);
> +	if (ts_attached) {
> +		/* Flush, set trig level to 1, enable temp, data and up irqs */
> +		writel(TEMP_IRQ_EN(1) | DATA_IRQ_EN(1) | FIFO_TRIG(1) |
> +			FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
> +		       ts->base + TP_INT_FIFOC);
> +	} else
> +		writel(TEMP_IRQ_EN(1), ts->base + TP_INT_FIFOC);
> +
> +	/* Enable temperature measurement, period 1953 (2 seconds) */
> +	writel(TEMP_ENABLE(1) | TEMP_PERIOD(1953), ts->base + TP_TPR);
>  
>  	/*
>  	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
> -- 
> 1.8.4.2
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 

Re: [lm-sensors] [PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Guenter Roeck]

On Tue, Dec 24, 2013 at 11:24:03PM +0100, Hans de Goede wrote:
> Note the sun4i-ts controller is capable of detecting a second touch, but when
> a second touch is present then the accuracy becomes so bad the reported touch
> location is not useable.
> 
> The original android driver contains some complicated heuristics using the
> aprox. distance between the 2 touches to see if the user is making a pinch
> open / close movement, and then reports emulated multi-touch events around
> the last touch coordinate (as the dual-touch coordinates are worthless).
> 
> These kinds of heuristics are just asking for trouble (and don't belong
> in the kernel). So this driver offers straight forward, reliable single
> touch functionality only.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/input/touchscreen/Kconfig    |  10 ++
>  drivers/input/touchscreen/Makefile   |   1 +
>  drivers/input/touchscreen/sun4i-ts.c | 272 +++++++++++++++++++++++++++++++++++

[ ... ]

> +
> +static int sun4i_ts_probe(struct platform_device *pdev)
> +{
> +	struct sun4i_ts_data *ts;
> +	int ret = -ENOMEM;
> +
> +	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);

How about using devm_kzalloc() here ? That would simplify the cleanup
path significantly as you could just return from most error cases.

Guenter

Re: Re: [lm-sensors] [PATCH 2/5] input: sun4i-ts: Add support for temperature sensor

[Hans de Goede]

On 12/25/2013 11:37 AM, Guenter Roeck wrote:
> On Tue, Dec 24, 2013 at 11:24:04PM +0100, Hans de Goede wrote:
>> The sun4i resisitive touchscreen controller also comes with a built-in
>> temperature sensor. This commit adds support for it.
>>
>> This commit also introduces a new "ts-attached" device-tree attribute,
>> when this is not set, the input part of the driver won't register. This way
>> the internal temperature sensor can be used to measure the SoC temperature
>> independent of there actually being a touchscreen attached to the controller.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Hi Hans,

Hi,

Merry Christmas and thanks for the review.

>
> can you use the new hwmon_device_register_with_groups API instead,
> or even devm_hwmon_device_register_with_groups ?

Its been a while since I last did a hwmon driver, so I did not know about
those, I'm a great fan of devm* so I'll use that one in my next revision.

>
> This way you don't have to create and remove the sysfs group, you don't have to
> create the name attribute, and if you use the devm_ API you don't have to remove
> anything. Also, the hwmon attributes will be attached to the hwmon device,
> meaning the platform device won't be cluttered with hwmon attributes and
> vice versa.
>
> Couple of related comments inline below.
>
> Thanks,
> Guenter
>
>> ---
>>   drivers/input/touchscreen/sun4i-ts.c | 106 +++++++++++++++++++++++++++++++++--
>>   1 file changed, 102 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
>> index b3f01c0..15a486d 100644
>> --- a/drivers/input/touchscreen/sun4i-ts.c
>> +++ b/drivers/input/touchscreen/sun4i-ts.c
>> @@ -3,6 +3,9 @@
>>    *
>>    * Copyright (C) 2013 - 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>    *
>> + * The hwmon parts are based on work by Corentin LABBE which is:
>> + * Copyright (C) 2013 Corentin LABBE <clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>>    * This program is free software; you can redistribute it and/or modify
>>    * it under the terms of the GNU General Public License as published by
>>    * the Free Software Foundation; either version 2 of the License, or
>> @@ -31,6 +34,7 @@
>>
>>   #include <linux/delay.h>
>>   #include <linux/err.h>
>> +#include <linux/hwmon.h>
>>   #include <linux/init.h>
>>   #include <linux/input.h>
>>   #include <linux/interrupt.h>
>> @@ -97,10 +101,16 @@
>>   #define TP_UP_PENDING		(1 << 1)
>>   #define TP_DOWN_PENDING		(1 << 0)
>>
>> +/* TP_TPR bits */
>> +#define TEMP_ENABLE(x)		((x) << 16)
>> +#define TEMP_PERIOD(x)		((x) << 0)  /* t = x * 256 * 16 / clkin */
>> +
>>   struct sun4i_ts_data {
>>   	struct device *dev;
>>   	void __iomem *base;
>>   	struct input_dev *input;
>> +	struct device *hwmon;
>> +	atomic_t temp_data;
>
> Unless I am missing something, this variable does not have to be an atomic.
> Either it was written or it wasn't, but the order should not matter.

That is true, but it does assume that 32 bit int accesses are atomic, in the
sense that we can never have a case where one core has only written ie the
lower 8 bits when the other reads the variable. I believe that is the
case on ARM (the only architecture relevant here), but rather safe then
sorry. So I plan to keep this as is until someone, who knows more about
these things then me, tells me it is safe to turn it into a regular int.

>
>>   	int ignore_fifo_data;
>>   };
>>
>> @@ -111,6 +121,12 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
>>
>>   	reg_val  = readl(ts->base + TP_INT_FIFOS);
>>
>> +	if (reg_val & TEMP_DATA_PENDING)
>> +		atomic_set(&ts->temp_data, readl(ts->base + TEMP_DATA));
>> +
>> +	if (!ts->input)
>> +		goto leave;
>> +
>>   	if (reg_val & FIFO_DATA_PENDING) {
>>   		x = readl(ts->base + TP_DATA);
>>   		y = readl(ts->base + TP_DATA);
>> @@ -135,6 +151,7 @@ static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
>>   		input_sync(ts->input);
>>   	}
>>
>> +leave:
>>   	writel(reg_val, ts->base + TP_INT_FIFOS);
>>
>>   	return IRQ_HANDLED;
>> @@ -170,6 +187,66 @@ static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
>>   	return 0;
>>   }
>>
>> +static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
>> +			 char *buf)
>> +{
>> +	return sprintf(buf, "sun4i_ts\n");
>> +}
>> +
>> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
>> +			 char *buf)
>> +{
>> +	struct sun4i_ts_data *ts = dev_get_drvdata(dev);
>> +	int temp_data;
>> +
>> +	temp_data = atomic_read(&ts->temp_data);
>> +	/* No temp_data until the first irq */
>> +	if (temp_data == -1)
>> +		return -EAGAIN;
>> +
>> +	return sprintf(buf, "%d\n", (temp_data - 1447) * 100);
>> +}
>> +
>> +static ssize_t show_temp_label(struct device *dev,
>> +			      struct device_attribute *devattr, char *buf)
>> +{
>> +	return sprintf(buf, "SoC temperature\n");
>> +}
>> +
>> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
>> +static DEVICE_ATTR(temp1_label, S_IRUGO, show_temp_label, NULL);
>> +
>> +static struct attribute *sun4i_ts_attributes[] = {
>> +	&dev_attr_name.attr,
>> +	&dev_attr_temp1_input.attr,
>> +	&dev_attr_temp1_label.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group sun4i_ts_attr_group = {
>> +	.attrs = sun4i_ts_attributes,
>> +};
>
> If you rename the attributes to sun4i_ts_attrs, you should be able to use
> ATTRIBUTE_GROUPS() here (this gives you a list of groups which you'll need
> if you use the new API).

Ok, sounds good.

>
>> +
>> +static int sun4i_ts_register_hwmon(struct sun4i_ts_data *ts)
>> +{
>> +	int ret;
>> +
>> +	atomic_set(&ts->temp_data, -1);
>> +
>> +	ret = sysfs_create_group(&ts->dev->kobj, &sun4i_ts_attr_group);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ts->hwmon = hwmon_device_register(ts->dev);
>> +	if (IS_ERR(ts->hwmon)) {
>> +		ret = PTR_ERR(ts->hwmon);
>> +		ts->hwmon = NULL;
>> +	}
>> +
>> +	return ret;
>
> With the new API, this would be something like
>
> 	hwmon_dev = devm_hwmon_device_register_with_groups(ts->dev, "sun4i_ts",
> 							   ts, sun4i_ts_attrs);
> 	return PTR_ERR_OR_ZERO(hwmon_dev);
>
> With the devm_ API, you don't have to store the hwmon device as you don't need
> to access it anymore from within the driver. Note that you can only use it if
> you also use devm_kzalloc to allocate ts; otherwise you would have a race condition
> during cleanup.

Sounds good again, I'll do a v2 with these changes one of the coming days.

>> +}
>> +
>>   static int sun4i_ts_remove(struct platform_device *pdev)
>>   {
>>   	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
>> @@ -178,9 +255,13 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>>   	writel(0, ts->base + TP_INT_FIFOC);
>>   	msleep(20);
>>
>> +	if (ts->hwmon)
>> +		hwmon_device_unregister(ts->hwmon);
>> +
>>   	if (ts->input)
>>   		input_unregister_device(ts->input);
>>
>> +	sysfs_remove_group(&ts->dev->kobj, &sun4i_ts_attr_group);
>>   	kfree(ts);
>>
>>   	return 0;
>> @@ -189,7 +270,11 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>>   static int sun4i_ts_probe(struct platform_device *pdev)
>>   {
>>   	struct sun4i_ts_data *ts;
>> +	struct device_node *np = pdev->dev.of_node;
>>   	int ret = -ENOMEM;
>> +	u32 ts_attached = 0;
>> +
>> +	of_property_read_u32(np, "ts-attached", &ts_attached);
>>
>>   	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);
>
> Different patch, but why not just use devm_kzalloc() ? Since you are using devm_
> functions elsewhere, that would seem natural. I'll also comment on that in the
> other patch.

I did not use devm_kzalloc simply because I did not knew about it until now :)
Using it sounds like an excellent idea so I'll do that.

<snip>

Regards,

Hans

Re: [PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Thomas Petazzoni]

Dear Hans de Goede,

On Tue, 24 Dec 2013 23:24:03 +0100, Hans de Goede wrote:

> +/* TP_INT_FIFOS irq and fifo status bits */
> +#define TEMP_DATA_PENDING	(1 << 18)
> +#define FIFO_OVERRUN_PENDING	(1 << 17)
> +#define FIFO_DATA_PENDING	(1 << 16)
> +#define TP_IDLE_FLG		(1 << 2)
> +#define TP_UP_PENDING		(1 << 1)
> +#define TP_DOWN_PENDING		(1 << 0)

You could use the BIT() macro for these definitions.


> +	ret = sun4i_ts_register_input(ts, pdev->name);
> +	if (ret)
> +		goto error;
> +
> +	/*
> +	 * Select HOSC clk, clkin = clk / 6, adc samplefreq = clkin / 8192,
> +	 * t_acq = clkin / (16 * 64)
> +	 */
> +	writel(ADC_CLK_SEL(0) | ADC_CLK_DIV(2) | FS_DIV(7) | T_ACQ(63),
> +	       ts->base + TP_CTRL0);
> +
> +	/*
> +	 * sensitive_adjust = 15 : max, which is not all that sensitive,
> +	 * tp_mode = 0 : only x and y coordinates, as we don't use dual touch
> +	 */
> +	writel(TP_SENSITIVE_ADJUST(15) | TP_MODE_SELECT(0),
> +	       ts->base + TP_CTRL2);
> +
> +	/* Enable median filter, type 1 : 5/3 */
> +	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
> +
> +	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
> +	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
> +	       ts->base + TP_INT_FIFOC);
> +
> +	/*
> +	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
> +	 * finally enable tp mode.
> +	 */
> +	writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1),
> +	       ts->base + TP_CTRL1);

It seems weird to do all the hardware initialization *after* the input
device has been registered. We normally initialize all the hardware
*and* then register the device, so that we guarantee that userspace
cannot access the device before it is properly initialized.


> +static const struct of_device_id sun4i_ts_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-ts", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_ts_of_match);

You're introducing a new DT binding, so the appropriate documentation
should be written for it.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/5] input: sun4i-ts: Add support for temperature sensor

[Thomas Petazzoni]

Dear Hans de Goede,

On Tue, 24 Dec 2013 23:24:04 +0100, Hans de Goede wrote:

> +	of_property_read_u32(np, "ts-attached", &ts_attached);

ts-attached should be a boolean property, and therefore you should use
of_property_read_bool() here.

And in your Device Tree, you should use:

	ts-attached;

when a touchscreen is attached. And nothing when no touchscreen is
attached.

Note that this patch is also changing the DT binding, so it should
update the DT binding documentation created in the first patch.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Dmitry Torokhov]

Hi Hans,

On Tue, Dec 24, 2013 at 11:24:03PM +0100, Hans de Goede wrote:
> Note the sun4i-ts controller is capable of detecting a second touch, but when
> a second touch is present then the accuracy becomes so bad the reported touch
> location is not useable.
> 
> The original android driver contains some complicated heuristics using the
> aprox. distance between the 2 touches to see if the user is making a pinch
> open / close movement, and then reports emulated multi-touch events around
> the last touch coordinate (as the dual-touch coordinates are worthless).
> 
> These kinds of heuristics are just asking for trouble (and don't belong
> in the kernel). So this driver offers straight forward, reliable single
> touch functionality only.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/touchscreen/Kconfig    |  10 ++
>  drivers/input/touchscreen/Makefile   |   1 +
>  drivers/input/touchscreen/sun4i-ts.c | 272 +++++++++++++++++++++++++++++++++++
>  3 files changed, 283 insertions(+)
>  create mode 100644 drivers/input/touchscreen/sun4i-ts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 961d58d..95023de 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -906,6 +906,16 @@ config TOUCHSCREEN_STMPE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stmpe-ts.
>  
> +config TOUCHSCREEN_SUN4I
> +	tristate "Allwinner sun4i resistive touchscreen controller support"
> +	depends on ARCH_SUNXI
> +	help
> +	  This selects support for the resistive touchscreen controller
> +	  found on Allwinner sunxi SoCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sun4i-ts.
> +
>  config TOUCHSCREEN_SUR40
>  	tristate "Samsung SUR40 (Surface 2.0/PixelSense) touchscreen"
>  	depends on USB
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62801f2..c8f7375 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TOUCHSCREEN_PIXCIR)	+= pixcir_i2c_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ST1232)	+= st1232.o
>  obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SUN4I)		+= sun4i-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
>  obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
>  obj-$(CONFIG_TOUCHSCREEN_TNETV107X)	+= tnetv107x-ts.o
> diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
> new file mode 100644
> index 0000000..b3f01c0
> --- /dev/null
> +++ b/drivers/input/touchscreen/sun4i-ts.c
> @@ -0,0 +1,272 @@
> +/*
> + * sun4i-ts.c Allwinner sunxi resistive touchscreen controller driver

No file names please.

> + *
> + * Copyright (C) 2013 - 2014 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * The sun4i-ts controller is capable of detecting a second touch, but when a
> + * second touch is present then the accuracy becomes so bad the reported touch
> + * location is not useable.
> + *
> + * The original android driver contains some complicated heuristics using the
> + * aprox. distance between the 2 touches to see if the user is making a pinch
> + * open / close movement, and then reports emulated multi-touch events around
> + * the last touch coordinate (as the dual-touch coordinates are worthless).
> + *
> + * These kinds of heuristics are just asking for trouble (and don't belong
> + * in the kernel). So this driver offers straight forward, reliable single
> + * touch functionality only.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define TP_CTRL0		0x00
> +#define TP_CTRL1		0x04
> +#define TP_CTRL2		0x08
> +#define TP_CTRL3		0x0c
> +#define TP_INT_FIFOC		0x10
> +#define TP_INT_FIFOS		0x14
> +#define TP_TPR			0x18
> +#define TP_CDAT			0x1c
> +#define TEMP_DATA		0x20
> +#define TP_DATA			0x24
> +
> +/* TP_CTRL0 bits */
> +#define ADC_FIRST_DLY(x)	((x) << 24) /* 8 bits */
> +#define ADC_FIRST_DLY_MODE(x)	((x) << 23)
> +#define ADC_CLK_SEL(x)		((x) << 22)
> +#define ADC_CLK_DIV(x)		((x) << 20) /* 3 bits */
> +#define FS_DIV(x)		((x) << 16) /* 4 bits */
> +#define T_ACQ(x)		((x) << 0) /* 16 bits */
> +
> +/* TP_CTRL1 bits */
> +#define STYLUS_UP_DEBOUN(x)	((x) << 12) /* 8 bits */
> +#define STYLUS_UP_DEBOUN_EN(x)	((x) << 9)
> +#define TOUCH_PAN_CALI_EN(x)	((x) << 6)
> +#define TP_DUAL_EN(x)		((x) << 5)
> +#define TP_MODE_EN(x)		((x) << 4)
> +#define TP_ADC_SELECT(x)	((x) << 3)
> +#define ADC_CHAN_SELECT(x)	((x) << 0)  /* 3 bits */
> +
> +/* TP_CTRL2 bits */
> +#define TP_SENSITIVE_ADJUST(x)	((x) << 28) /* 4 bits */
> +#define TP_MODE_SELECT(x)	((x) << 26) /* 2 bits */
> +#define PRE_MEA_EN(x)		((x) << 24)
> +#define PRE_MEA_THRE_CNT(x)	((x) << 0) /* 24 bits */
> +
> +/* TP_CTRL3 bits */
> +#define FILTER_EN(x)		((x) << 2)
> +#define FILTER_TYPE(x)		((x) << 0)  /* 2 bits */
> +
> +/* TP_INT_FIFOC irq and fifo mask / control bits */
> +#define TEMP_IRQ_EN(x)		((x) << 18)
> +#define OVERRUN_IRQ_EN(x)	((x) << 17)
> +#define DATA_IRQ_EN(x)		((x) << 16)
> +#define TP_DATA_XY_CHANGE(x)	((x) << 13)
> +#define FIFO_TRIG(x)		((x) << 8)  /* 5 bits */
> +#define DATA_DRQ_EN(x)		((x) << 7)
> +#define FIFO_FLUSH(x)		((x) << 4)
> +#define TP_UP_IRQ_EN(x)		((x) << 1)
> +#define TP_DOWN_IRQ_EN(x)	((x) << 0)
> +
> +/* TP_INT_FIFOS irq and fifo status bits */
> +#define TEMP_DATA_PENDING	(1 << 18)
> +#define FIFO_OVERRUN_PENDING	(1 << 17)
> +#define FIFO_DATA_PENDING	(1 << 16)
> +#define TP_IDLE_FLG		(1 << 2)
> +#define TP_UP_PENDING		(1 << 1)
> +#define TP_DOWN_PENDING		(1 << 0)
> +
> +struct sun4i_ts_data {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct input_dev *input;

bool?

> +	int ignore_fifo_data;
> +};
> +
> +static irqreturn_t sun4i_ts_irq(int irq, void *dev_id)
> +{
> +	struct sun4i_ts_data *ts = dev_id;
> +	u32 reg_val, x, y;
> +
> +	reg_val  = readl(ts->base + TP_INT_FIFOS);
> +
> +	if (reg_val & FIFO_DATA_PENDING) {
> +		x = readl(ts->base + TP_DATA);
> +		y = readl(ts->base + TP_DATA);
> +		/* The 1st location reported after an up event is unreliable */
> +		if (!ts->ignore_fifo_data) {
> +			input_report_abs(ts->input, ABS_X, x);
> +			input_report_abs(ts->input, ABS_Y, y);
> +			/*
> +			 * The hardware has a separate down status bit, but
> +			 * that gets set before we get the first location,
> +			 * resulting in reporting a click on the old location.
> +			 */
> +			input_report_key(ts->input, BTN_TOUCH, 1);
> +			input_sync(ts->input);
> +		} else
> +			ts->ignore_fifo_data = 0;

Please use braces either in none, or in both branches.

> +	}
> +
> +	if (reg_val & TP_UP_PENDING) {
> +		ts->ignore_fifo_data = 1;
> +		input_report_key(ts->input, BTN_TOUCH, 0);
> +		input_sync(ts->input);
> +	}
> +
> +	writel(reg_val, ts->base + TP_INT_FIFOS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
> +					     const char *name)
> +{
> +	int ret;
> +	struct input_dev *input;
> +
> +	input = devm_input_allocate_device(ts->dev);
> +	if (!input)
> +		return -ENOMEM;
> +
> +	input->name = name;
> +	input->phys = "sun4i_ts/input0";
> +	input->id.bustype = BUS_HOST;
> +	input->id.vendor = 0x0001;
> +	input->id.product = 0x0001;
> +	input->id.version = 0x0100;
> +	input->dev.parent = ts->dev;
> +	input->evbit[0] =  BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS);
> +	set_bit(BTN_TOUCH, input->keybit);
> +	input_set_abs_params(input, ABS_X, 0, 4095, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, 4095, 0, 0);
> +
> +	ret = input_register_device(input);
> +	if (ret)
> +		return ret;
> +
> +	ts->input = input;
> +	return 0;
> +}
> +
> +static int sun4i_ts_remove(struct platform_device *pdev)
> +{
> +	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
> +
> +	/* Deactivate all IRQs */
> +	writel(0, ts->base + TP_INT_FIFOC);

Should this be moved into close() method of input device?
 
> +	msleep(20);

Why do you need msleep here? Are you trying to ensure that interrupts
are not happening? In that case synchronize_irq() is better option I
think.

> +
> +	if (ts->input)
> +		input_unregister_device(ts->input);

Since you are using managed input device you do not need to call
input_unregister_device() explicitly.

> +
> +	kfree(ts);
> +
> +	return 0;
> +}
> +
> +static int sun4i_ts_probe(struct platform_device *pdev)
> +{
> +	struct sun4i_ts_data *ts;
> +	int ret = -ENOMEM;
> +
> +	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;

I believe someone already mentioned devm_kzalloc()...

> +
> +	platform_set_drvdata(pdev, ts);
> +
> +	ts->dev = &pdev->dev;
> +	ts->base = devm_ioremap_resource(&pdev->dev,
> +			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +	if (IS_ERR(ts->base)) {
> +		ret = PTR_ERR(ts->base);
> +		goto error;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, platform_get_irq(pdev, 0),
> +			       sun4i_ts_irq, 0, "sun4i-ts", ts);
> +	if (ret)
> +		goto error;
> +

Are we 1000% sure that the device is quiesced here and we will not get a
stray interrupt? Even if we are sure I'd still rather allocate input
device earlier, together with ts structure.

> +	ret = sun4i_ts_register_input(ts, pdev->name);
> +	if (ret)
> +		goto error;
> +
> +	/*
> +	 * Select HOSC clk, clkin = clk / 6, adc samplefreq = clkin / 8192,
> +	 * t_acq = clkin / (16 * 64)
> +	 */
> +	writel(ADC_CLK_SEL(0) | ADC_CLK_DIV(2) | FS_DIV(7) | T_ACQ(63),
> +	       ts->base + TP_CTRL0);
> +
> +	/*
> +	 * sensitive_adjust = 15 : max, which is not all that sensitive,
> +	 * tp_mode = 0 : only x and y coordinates, as we don't use dual touch
> +	 */
> +	writel(TP_SENSITIVE_ADJUST(15) | TP_MODE_SELECT(0),
> +	       ts->base + TP_CTRL2);
> +
> +	/* Enable median filter, type 1 : 5/3 */
> +	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
> +
> +	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
> +	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
> +	       ts->base + TP_INT_FIFOC);
> +
> +	/*
> +	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
> +	 * finally enable tp mode.
> +	 */
> +	writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1),
> +	       ts->base + TP_CTRL1);

Should all of some of this be moved into open() method for your input
device?

> +
> +	return 0;
> +
> +error:
> +	sun4i_ts_remove(pdev);
> +	return ret;
> +}
> +
> +static const struct of_device_id sun4i_ts_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-ts", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_ts_of_match);
> +
> +static struct platform_driver sun4i_ts_driver = {
> +	.driver = {
> +		.owner	= THIS_MODULE,
> +		.name	= "sun4i-ts",
> +		.of_match_table = of_match_ptr(sun4i_ts_of_match),
> +	},
> +	.probe	= sun4i_ts_probe,
> +	.remove	= sun4i_ts_remove,
> +};
> +
> +module_platform_driver(sun4i_ts_driver);
> +
> +MODULE_DESCRIPTION("Allwinner sun4i resistive touchscreen controller driver");
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.4.2
> 

Thanks.

-- 
Dmitry

Re: Re: [lm-sensors] [PATCH 2/5] input: sun4i-ts: Add support for temperature sensor

[Dmitry Torokhov]

On Wed, Dec 25, 2013 at 11:54:27AM +0100, Hans de Goede wrote:
> On 12/25/2013 11:37 AM, Guenter Roeck wrote:
> >On Tue, Dec 24, 2013 at 11:24:04PM +0100, Hans de Goede wrote:
> >>  struct sun4i_ts_data {
> >>  	struct device *dev;
> >>  	void __iomem *base;
> >>  	struct input_dev *input;
> >>+	struct device *hwmon;
> >>+	atomic_t temp_data;
> >
> >Unless I am missing something, this variable does not have to be an atomic.
> >Either it was written or it wasn't, but the order should not matter.
> 
> That is true, but it does assume that 32 bit int accesses are atomic, in the
> sense that we can never have a case where one core has only written ie the
> lower 8 bits when the other reads the variable. I believe that is the
> case on ARM (the only architecture relevant here), but rather safe then
> sorry. So I plan to keep this as is until someone, who knows more about
> these things then me, tells me it is safe to turn it into a regular int.

I believe on all arches that we support writes to aligned double word
memory locations will not be split.

Thanks.

-- 
Dmitry

Re: [PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Hans de Goede]

Hi Dimitri,

Thanks for the review. I've one or two small questions / remarks.
I'll do a v2 addressing your and others comments tomorrow.

On 12/26/2013 11:15 PM, Dmitry Torokhov wrote:
>> +static int sun4i_ts_register_input(struct sun4i_ts_data *ts,
>> +					     const char *name)
>> +{
>> +	int ret;
>> +	struct input_dev *input;
>> +
>> +	input = devm_input_allocate_device(ts->dev);
>> +	if (!input)
>> +		return -ENOMEM;
>> +
>> +	input->name = name;
>> +	input->phys = "sun4i_ts/input0";
>> +	input->id.bustype = BUS_HOST;
>> +	input->id.vendor = 0x0001;
>> +	input->id.product = 0x0001;
>> +	input->id.version = 0x0100;
>> +	input->dev.parent = ts->dev;
>> +	input->evbit[0] =  BIT(EV_SYN) | BIT(EV_KEY) | BIT(EV_ABS);
>> +	set_bit(BTN_TOUCH, input->keybit);
>> +	input_set_abs_params(input, ABS_X, 0, 4095, 0, 0);
>> +	input_set_abs_params(input, ABS_Y, 0, 4095, 0, 0);
>> +
>> +	ret = input_register_device(input);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ts->input = input;
>> +	return 0;
>> +}
>> +
>> +static int sun4i_ts_remove(struct platform_device *pdev)
>> +{
>> +	struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
>> +
>> +	/* Deactivate all IRQs */
>> +	writel(0, ts->base + TP_INT_FIFOC);
>
> Should this be moved into close() method of input device?

Yes and no, given patch 2/5 we should leave the temp_data irq enabled in the
close method. But disabling the other irqs on close is a good idea.


>
>> +	msleep(20);
>
> Why do you need msleep here? Are you trying to ensure that interrupts
> are not happening?

Yes.

> In that case synchronize_irq() is better option I think.

I did not know about synchronize_irq(), that definitely is a better
option.


>
>> +
>> +	if (ts->input)
>> +		input_unregister_device(ts->input);
>
> Since you are using managed input device you do not need to call
> input_unregister_device() explicitly.

Yes I was already wondering about that and wanted to check this for v2,
now I can skip the checking and simply remove this :)

>
>> +
>> +	kfree(ts);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sun4i_ts_probe(struct platform_device *pdev)
>> +{
>> +	struct sun4i_ts_data *ts;
>> +	int ret = -ENOMEM;
>> +
>> +	ts = kzalloc(sizeof(struct sun4i_ts_data), GFP_KERNEL);
>> +	if (!ts)
>> +		return -ENOMEM;
>
> I believe someone already mentioned devm_kzalloc()...

Yep, already fixed locally.

>
>> +
>> +	platform_set_drvdata(pdev, ts);
>> +
>> +	ts->dev = &pdev->dev;
>> +	ts->base = devm_ioremap_resource(&pdev->dev,
>> +			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
>> +	if (IS_ERR(ts->base)) {
>> +		ret = PTR_ERR(ts->base);
>> +		goto error;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, platform_get_irq(pdev, 0),
>> +			       sun4i_ts_irq, 0, "sun4i-ts", ts);
>> +	if (ret)
>> +		goto error;
>> +
>
> Are we 1000% sure that the device is quiesced here and we will not get a
> stray interrupt? Even if we are sure I'd still rather allocate input
> device earlier, together with ts structure.

I will change things to allocate the input device earlier. What about
registering it, when is the best time to do that?

>
>> +	ret = sun4i_ts_register_input(ts, pdev->name);
>> +	if (ret)
>> +		goto error;
>> +
>> +	/*
>> +	 * Select HOSC clk, clkin = clk / 6, adc samplefreq = clkin / 8192,
>> +	 * t_acq = clkin / (16 * 64)
>> +	 */
>> +	writel(ADC_CLK_SEL(0) | ADC_CLK_DIV(2) | FS_DIV(7) | T_ACQ(63),
>> +	       ts->base + TP_CTRL0);
>> +
>> +	/*
>> +	 * sensitive_adjust = 15 : max, which is not all that sensitive,
>> +	 * tp_mode = 0 : only x and y coordinates, as we don't use dual touch
>> +	 */
>> +	writel(TP_SENSITIVE_ADJUST(15) | TP_MODE_SELECT(0),
>> +	       ts->base + TP_CTRL2);
>> +
>> +	/* Enable median filter, type 1 : 5/3 */
>> +	writel(FILTER_EN(1) | FILTER_TYPE(1), ts->base + TP_CTRL3);
>> +
>> +	/* Flush fifo, set trig level to 1, enable data and pen up irqs */
>> +	writel(DATA_IRQ_EN(1) | FIFO_TRIG(1) | FIFO_FLUSH(1) | TP_UP_IRQ_EN(1),
>> +	       ts->base + TP_INT_FIFOC);
>> +
>> +	/*
>> +	 * Set stylus up debounce to aprox 10 ms, enable debounce, and
>> +	 * finally enable tp mode.
>> +	 */
>> +	writel(STYLUS_UP_DEBOUN(5) | STYLUS_UP_DEBOUN_EN(1) | TP_MODE_EN(1),
>> +	       ts->base + TP_CTRL1);
>
> Should all of some of this be moved into open() method for your input
> device?

The flushing of the fifo, and enabling of the input related irqs is probably
best done in the open method, the rest is also needed for the temp sensor
parts. I'll move the fifo-flush + irq enabling to the open method.

<snip>

Regards,

Hans

Re: [PATCH 1/5] input: Add new sun4i-ts driver for Allwinner sunxi SoC's rtp controller

[Dmitry Torokhov]

On Thu, Dec 26, 2013 at 11:33:06PM +0100, Hans de Goede wrote:
> On 12/26/2013 11:15 PM, Dmitry Torokhov wrote:
> >
> >Are we 1000% sure that the device is quiesced here and we will not get a
> >stray interrupt? Even if we are sure I'd still rather allocate input
> >device earlier, together with ts structure.
> 
> I will change things to allocate the input device earlier. What about
> registering it, when is the best time to do that?

The common pattern for the most input drivers is to register it last,
but that usually matters when you are not using managed input devices
since it helps making error unwinding path simpler. I'd still register
it last just so it looks the same as the rest of the drivers.

-- 
Dmitry