[PATCH v3 0/8] drivers: touchscreen: tsc2007 and ads7846/tsc2046 improvements (use common touchscreen bindings, pre-calibration, spi fix and provide iio raw values)

[PATCH v3 0/8] drivers: touchscreen: tsc2007 and ads7846/tsc2046 improvements (use common touchscreen bindings, pre-calibration, spi fix and provide iio raw values)

[H. Nikolaus Schaller]

It was quite a while ago that this patch set was in review, but here is a new version for consideration.

Changes V3:
* fix an issue with swapping
* remove hard clipping to min/max rectangle - some systems expect to handle negative coordinates
* make use of commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")

2015-11-13 21:36:07: Changes V2:
* add a patch to make drivers still recognise the old "ti,swap-xy" property (suggested by Rob Herring)

2015-11-06 16:14:53: This patch series improves the drivers for the tsc2007 and
ads7846/tsc2046 touchscreen controllers which are e.g. used by the GTA04
OpenPandora and Pyra devices.

New common bindings have been defined by
commit b98abe52fa8e ("Input: add common DT binding for touchscreens"):

	Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

which also defines a helper function to parse the DT. These new parameters
allow to specify the fuzz factors (jitter suppression), inversion of x or y axis and
swapping of x and y to achieve inversion and rotation so that the touch
coordinate axes match the natural orientation of the display panel.

Another improvement is to better use the min/max ADC values and
scale to the screen size as defined by the DT. This allows to coarsely
calibrate the touch to match the LCD to which it is glued on so that the
touch can quite precisely be operated before any user-space fine-calibration
can be (and needs to be) started.

For the adc7846 we fix an issue with the spi module table.

Finally we add an iio interface for the AUX and temperature ADC channels of
the tsc2007 and also provide the touch screen raw values. This allows to read
an optional ambient light sensor installed on the gta04 board and improves
calibration and hardware monitoring.


H. Nikolaus Schaller (8):
  drivers:input:tsc2007: add new common binding names, pre-calibration,
    flipping and rotation
  drivers:input:tsc2007: send pendown and penup only once like
    ads7846(+tsc2046) driver does
  drivers:input:tsc2007: add iio interface to read external ADC input,
    temperature and raw conversion values
  drivers:input:tsc2007: check for presence and power down tsc2007
    during probe
  DT:omap3+tsc2007: use new common touchscreen bindings
  drivers:input:ads7846(+tsc2046): add new common binding names,
    pre-calibration and flipping
  drivers:input:ads7846(+tsc2046): fix spi module table
  DT:omap3+ads7846: use new common touchscreen bindings

 .../devicetree/bindings/input/ads7846.txt          |   9 +-
 .../bindings/input/touchscreen/tsc2007.txt         |  20 +-
 arch/arm/boot/dts/omap3-gta04.dtsi                 |  25 +-
 arch/arm/boot/dts/omap3-lilly-a83x.dtsi            |   2 +-
 arch/arm/boot/dts/omap3-pandora-common.dtsi        |  17 +-
 .../boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi    |   3 +-
 drivers/input/touchscreen/Kconfig                  |   1 +
 drivers/input/touchscreen/ads7846.c                |  71 +++++-
 drivers/input/touchscreen/tsc2007.c                | 281 +++++++++++++++++++--
 include/linux/i2c/tsc2007.h                        |   8 +
 10 files changed, 382 insertions(+), 55 deletions(-)

-- 
2.7.3

[PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
introduced common DT bindings for touchscreens [1] and a helper function to
parse the DT.

commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
added another helper for parsing axis inversion and swapping
and applying them to x and y coordinates.

Both helpers have been integrated to accommodate any orientation of the
touch panel in relation to the LCD.

A new feature is to introduce scaling the min/max ADC values to the screen
size.

This makes it possible to pre-calibrate the touch so that is (almost)
exactly matches the LCD pixel coordinates it is glued onto. This allows to
well enough operate the touch before a user space calibration step can
improve the precision.

Finally, calculate_pressure has been renamed to calculate_resistance
because that is what it is doing.

[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
 drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
 include/linux/i2c/tsc2007.h                        |   8 ++
 3 files changed, 123 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
index ec365e1..6e9fd55 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
@@ -6,6 +6,7 @@ Required properties:
 - ti,x-plate-ohms: X-plate resistance in ohms.
 
 Optional properties:
+- generic touch screen properties: see touchscreen binding [2].
 - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
   The penirq pin goes to low when the panel is touched.
   (see GPIO binding[1] for more details).
@@ -13,17 +14,20 @@ Optional properties:
   (see interrupt binding[0]).
 - interrupts: (gpio) interrupt to which the chip is connected
   (see interrupt binding[0]).
-- ti,max-rt: maximum pressure.
-- ti,fuzzx: specifies the absolute input fuzz x value.
-  If set, it will permit noise in the data up to +- the value given to the fuzz
-  parameter, that is used to filter noise from the event stream.
-- ti,fuzzy: specifies the absolute input fuzz y value.
-- ti,fuzzz: specifies the absolute input fuzz z value.
+- ti,max-rt: maximum pressure resistance above which samples are ignored
+  (default: 4095).
+- ti,report-resistance: report resistance (no pressure = max_rt) instead
+  of pressure (no pressure = 0).
+- ti,min-x: minimum value reported by X axis ADC (default 0).
+- ti,max-x: maximum value reported by X axis ADC (default 4095).
+- ti,min-y: minimum value reported by Y axis ADC (default 0).
+- ti,max-y: maximum value reported by Y axis ADC (default 4095).
 - ti,poll-period: how much time to wait (in milliseconds) before reading again the
-  values from the tsc2007.
+  values from the tsc2007 (default 1).
 
 [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
+[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
 
 Example:
 	&i2c1 {
@@ -35,6 +39,8 @@ Example:
 			interrupts = <0x0 0x8>;
 			gpios = <&gpio4 0 0>;
 			ti,x-plate-ohms = <180>;
+			touchscreen-size-x = <640>;
+			touchscreen-size-y = <480>;
 		};
 
 		/* ... */
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 5d0cd51..9a11509 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -29,6 +29,7 @@
 #include <linux/of_device.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/input/touchscreen.h>
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
@@ -74,6 +75,14 @@ struct tsc2007 {
 
 	u16			model;
 	u16			x_plate_ohms;
+
+	struct touchscreen_properties prop;
+
+	bool			report_resistance;
+	u16			min_x;
+	u16			min_y;
+	u16			max_x;
+	u16			max_y;
 	u16			max_rt;
 	unsigned long		poll_period; /* in jiffies */
 	int			fuzzx;
@@ -128,7 +137,8 @@ static void tsc2007_read_values(struct tsc2007 *tsc, struct ts_event *tc)
 	tsc2007_xfer(tsc, PWRDOWN);
 }
 
-static u32 tsc2007_calculate_pressure(struct tsc2007 *tsc, struct ts_event *tc)
+static u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
+					struct ts_event *tc)
 {
 	u32 rt = 0;
 
@@ -177,12 +187,13 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	struct ts_event tc;
 	u32 rt;
 
+	dev_dbg(&ts->client->dev, "soft irq %d\n", irq);
 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
 		/* pen is down, continue with the measurement */
 		tsc2007_read_values(ts, &tc);
 
-		rt = tsc2007_calculate_pressure(ts, &tc);
+		rt = tsc2007_calculate_resistance(ts, &tc);
 
 		if (!rt && !ts->get_pendown_state) {
 			/*
@@ -194,21 +205,45 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 		}
 
 		if (rt <= ts->max_rt) {
+			int sx, sy;
+
 			dev_dbg(&ts->client->dev,
 				"DOWN point(%4d,%4d), pressure (%4u)\n",
 				tc.x, tc.y, rt);
 
-			input_report_key(input, BTN_TOUCH, 1);
-			input_report_abs(input, ABS_X, tc.x);
-			input_report_abs(input, ABS_Y, tc.y);
+			if (!ts->report_resistance)
+				rt = ts->max_rt - rt;
+
+			/* scale ADC values to desired output range */
+			sx = (ts->prop.max_x * (tc.x - ts->min_x))
+				/ (ts->max_x - ts->min_x);
+			sy = (ts->prop.max_y * (tc.y - ts->min_y))
+				/ (ts->max_y - ts->min_y);
+			rt = (input->absinfo[ABS_PRESSURE].maximum * rt) /
+				ts->max_rt;
+
+			dev_dbg(&ts->client->dev,
+				"Scaled point(%4d,%4d), pressure (%4u)\n",
+				sx, sy, rt);
+
+			/* report event */
+			if (!ts->pendown) {
+				input_report_key(input, BTN_TOUCH, 1);
+				ts->pendown = true;
+			}
+
+			touchscreen_report_pos(ts->input, &ts->prop,
+						(unsigned int) sx,
+						(unsigned int) sy,
+						false);
 			input_report_abs(input, ABS_PRESSURE, rt);
 
 			input_sync(input);
 
 		} else {
 			/*
-			 * Sample found inconsistent by debouncing or pressure is
-			 * beyond the maximum. Don't report it to user space,
+			 * Sample found inconsistent by debouncing or resistance
+			 * is beyond the maximum. Don't report it to user space,
 			 * repeat at least once more the measurement.
 			 */
 			dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
@@ -233,6 +268,7 @@ static irqreturn_t tsc2007_hard_irq(int irq, void *handle)
 {
 	struct tsc2007 *ts = handle;
 
+	dev_dbg(&ts->client->dev, "hard irq %d\n", irq);
 	if (tsc2007_is_pen_down(ts))
 		return IRQ_WAKE_THREAD;
 
@@ -303,14 +339,24 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 	else
 		ts->max_rt = MAX_12BIT;
 
-	if (!of_property_read_u32(np, "ti,fuzzx", &val32))
-		ts->fuzzx = val32;
+	ts->report_resistance =
+		       of_property_read_bool(np, "ti,report-resistance");
 
-	if (!of_property_read_u32(np, "ti,fuzzy", &val32))
-		ts->fuzzy = val32;
+	touchscreen_parse_properties(ts->input, false, &ts->prop);
 
-	if (!of_property_read_u32(np, "ti,fuzzz", &val32))
-		ts->fuzzz = val32;
+	if (!of_property_read_u32(np, "ti,min-x", &val32))
+		ts->min_x = val32;
+	if (!of_property_read_u32(np, "ti,max-x", &val32))
+		ts->max_x = val32;
+	else
+		ts->max_x = MAX_12BIT;
+
+	if (!of_property_read_u32(np, "ti,min-y", &val32))
+		ts->min_y = val32;
+	if (!of_property_read_u32(np, "ti,max-y", &val32))
+		ts->max_y = val32;
+	else
+		ts->max_y = MAX_12BIT;
 
 	if (!of_property_read_u64(np, "ti,poll-period", &val64))
 		ts->poll_period = msecs_to_jiffies(val64);
@@ -332,6 +378,22 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 			 "GPIO not specified in DT (of_get_gpio returned %d)\n",
 			 ts->gpio);
 
+	dev_dbg(&client->dev,
+			"min/max_x (%4d,%4d)\n",
+			ts->min_x, ts->max_x);
+	dev_dbg(&client->dev,
+			"min/max_y (%4d,%4d)\n",
+			ts->min_y, ts->max_y);
+	dev_dbg(&client->dev,
+			"max_rt (%4d)\n",
+			ts->max_rt);
+	dev_dbg(&client->dev,
+			"size (%4d,%4d)\n",
+			ts->prop.max_x, ts->prop.max_y);
+	dev_dbg(&client->dev,
+			"ts-gpio: %d\n",
+			ts->gpio);
+
 	return 0;
 }
 #else
@@ -349,6 +411,14 @@ static int tsc2007_probe_pdev(struct i2c_client *client, struct tsc2007 *ts,
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->max_rt            = pdata->max_rt ? : MAX_12BIT;
+	ts->prop.swap_x_y     = pdata->swap_xy;
+	ts->prop.invert_x     = pdata->invert_x;
+	ts->prop.invert_y     = pdata->invert_y;
+	ts->report_resistance = pdata->report_resistance;
+	ts->min_x             = pdata->min_x ? : 0;
+	ts->min_y             = pdata->min_y ? : 0;
+	ts->max_x             = pdata->max_x ? : MAX_12BIT;
+	ts->max_y             = pdata->max_y ? : MAX_12BIT;
 	ts->poll_period       = msecs_to_jiffies(pdata->poll_period ? : 1);
 	ts->get_pendown_state = pdata->get_pendown_state;
 	ts->clear_penirq      = pdata->clear_penirq;
@@ -388,13 +458,6 @@ static int tsc2007_probe(struct i2c_client *client,
 	if (!ts)
 		return -ENOMEM;
 
-	if (pdata)
-		err = tsc2007_probe_pdev(client, ts, pdata, id);
-	else
-		err = tsc2007_probe_dt(client, ts);
-	if (err)
-		return err;
-
 	input_dev = devm_input_allocate_device(&client->dev);
 	if (!input_dev)
 		return -ENOMEM;
@@ -419,12 +482,25 @@ static int tsc2007_probe(struct i2c_client *client,
 	input_set_drvdata(input_dev, ts);
 
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input_dev->absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) |
+				BIT_MASK(ABS_PRESSURE);
 	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
 
-	input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, ts->fuzzx, 0);
-	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, ts->fuzzy, 0);
-	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT,
-			     ts->fuzzz, 0);
+	if (pdata) {
+		err = tsc2007_probe_pdev(client, ts, pdata, id);
+		if (err)
+			return err;
+		input_set_abs_params(input_dev, ABS_X, 0, ts->max_x-ts->min_x,
+							  ts->fuzzx, 0);
+		input_set_abs_params(input_dev, ABS_Y, 0, ts->max_y-ts->min_y,
+							  ts->fuzzy, 0);
+		input_set_abs_params(input_dev, ABS_PRESSURE, 0, ts->max_rt,
+							  ts->fuzzz, 0);
+	} else {
+		err = tsc2007_probe_dt(client, ts);
+		if (err)
+			return err;
+	}
 
 	if (pdata) {
 		if (pdata->exit_platform_hw) {
@@ -443,6 +519,8 @@ static int tsc2007_probe(struct i2c_client *client,
 			pdata->init_platform_hw();
 	}
 
+	dev_dbg(&client->dev, "request irq %d\n",
+			ts->irq);
 	err = devm_request_threaded_irq(&client->dev, ts->irq,
 					tsc2007_hard_irq, tsc2007_soft_irq,
 					IRQF_ONESHOT,
diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
index 4f35b6a..632db20 100644
--- a/include/linux/i2c/tsc2007.h
+++ b/include/linux/i2c/tsc2007.h
@@ -6,6 +6,14 @@
 struct tsc2007_platform_data {
 	u16	model;				/* 2007. */
 	u16	x_plate_ohms;	/* must be non-zero value */
+	bool	swap_xy;	/* swap x and y axis */
+	bool	invert_x;
+	bool	invert_y;
+	bool	report_resistance;
+	u16	min_x;	/* min and max values reported by ADC */
+	u16	min_y;
+	u16	max_x;
+	u16	max_y;
 	u16	max_rt; /* max. resistance above which samples are ignored */
 	unsigned long poll_period; /* time (in ms) between samples */
 	int	fuzzx; /* fuzz factor for X, Y and pressure axes */
-- 
2.7.3

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Rob Herring]

On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
> introduced common DT bindings for touchscreens [1] and a helper function to
> parse the DT.
> 
> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
> added another helper for parsing axis inversion and swapping
> and applying them to x and y coordinates.
> 
> Both helpers have been integrated to accommodate any orientation of the
> touch panel in relation to the LCD.
> 
> A new feature is to introduce scaling the min/max ADC values to the screen
> size.
> 
> This makes it possible to pre-calibrate the touch so that is (almost)
> exactly matches the LCD pixel coordinates it is glued onto. This allows to
> well enough operate the touch before a user space calibration step can
> improve the precision.
> 
> Finally, calculate_pressure has been renamed to calculate_resistance
> because that is what it is doing.

Seems like you are breaking compatibility with old DTs. I can't tell for 
sure though.

> 
> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>  drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>  include/linux/i2c/tsc2007.h                        |   8 ++
>  3 files changed, 123 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> index ec365e1..6e9fd55 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> @@ -6,6 +6,7 @@ Required properties:
>  - ti,x-plate-ohms: X-plate resistance in ohms.
>  
>  Optional properties:
> +- generic touch screen properties: see touchscreen binding [2].
>  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>    The penirq pin goes to low when the panel is touched.
>    (see GPIO binding[1] for more details).
> @@ -13,17 +14,20 @@ Optional properties:
>    (see interrupt binding[0]).
>  - interrupts: (gpio) interrupt to which the chip is connected
>    (see interrupt binding[0]).
> -- ti,max-rt: maximum pressure.
> -- ti,fuzzx: specifies the absolute input fuzz x value.
> -  If set, it will permit noise in the data up to +- the value given to the fuzz
> -  parameter, that is used to filter noise from the event stream.
> -- ti,fuzzy: specifies the absolute input fuzz y value.
> -- ti,fuzzz: specifies the absolute input fuzz z value.
> +- ti,max-rt: maximum pressure resistance above which samples are ignored
> +  (default: 4095).
> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
> +  of pressure (no pressure = 0).
> +- ti,min-x: minimum value reported by X axis ADC (default 0).
> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).

Seems like these could be common too. They make more sense than giving x 
and y sizes in pixel units which really should come from the panel.

>  - ti,poll-period: how much time to wait (in milliseconds) before reading again the
> -  values from the tsc2007.
> +  values from the tsc2007 (default 1).
>  
>  [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> +[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>  
>  Example:
>  	&i2c1 {
> @@ -35,6 +39,8 @@ Example:
>  			interrupts = <0x0 0x8>;
>  			gpios = <&gpio4 0 0>;
>  			ti,x-plate-ohms = <180>;
> +			touchscreen-size-x = <640>;
> +			touchscreen-size-y = <480>;
>  		};
>  
>  		/* ... */

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Rob Herring]

On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
> introduced common DT bindings for touchscreens [1] and a helper function to
> parse the DT.
> 
> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
> added another helper for parsing axis inversion and swapping
> and applying them to x and y coordinates.
> 
> Both helpers have been integrated to accommodate any orientation of the
> touch panel in relation to the LCD.
> 
> A new feature is to introduce scaling the min/max ADC values to the screen
> size.
> 
> This makes it possible to pre-calibrate the touch so that is (almost)
> exactly matches the LCD pixel coordinates it is glued onto. This allows to
> well enough operate the touch before a user space calibration step can
> improve the precision.
> 
> Finally, calculate_pressure has been renamed to calculate_resistance
> because that is what it is doing.

Seems like you are breaking compatibility with old DTs. I can't tell for 
sure though.

> 
> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
>  .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>  drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>  include/linux/i2c/tsc2007.h                        |   8 ++
>  3 files changed, 123 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> index ec365e1..6e9fd55 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> @@ -6,6 +6,7 @@ Required properties:
>  - ti,x-plate-ohms: X-plate resistance in ohms.
>  
>  Optional properties:
> +- generic touch screen properties: see touchscreen binding [2].
>  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>    The penirq pin goes to low when the panel is touched.
>    (see GPIO binding[1] for more details).
> @@ -13,17 +14,20 @@ Optional properties:
>    (see interrupt binding[0]).
>  - interrupts: (gpio) interrupt to which the chip is connected
>    (see interrupt binding[0]).
> -- ti,max-rt: maximum pressure.
> -- ti,fuzzx: specifies the absolute input fuzz x value.
> -  If set, it will permit noise in the data up to +- the value given to the fuzz
> -  parameter, that is used to filter noise from the event stream.
> -- ti,fuzzy: specifies the absolute input fuzz y value.
> -- ti,fuzzz: specifies the absolute input fuzz z value.
> +- ti,max-rt: maximum pressure resistance above which samples are ignored
> +  (default: 4095).
> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
> +  of pressure (no pressure = 0).
> +- ti,min-x: minimum value reported by X axis ADC (default 0).
> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).

Seems like these could be common too. They make more sense than giving x 
and y sizes in pixel units which really should come from the panel.

>  - ti,poll-period: how much time to wait (in milliseconds) before reading again the
> -  values from the tsc2007.
> +  values from the tsc2007 (default 1).
>  
>  [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> +[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>  
>  Example:
>  	&i2c1 {
> @@ -35,6 +39,8 @@ Example:
>  			interrupts = <0x0 0x8>;
>  			gpios = <&gpio4 0 0>;
>  			ti,x-plate-ohms = <180>;
> +			touchscreen-size-x = <640>;
> +			touchscreen-size-y = <480>;
>  		};
>  
>  		/* ... */
--
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 v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4296 bytes --]

On Fri, Sep 23, 2016 at 05:47:26PM -0500, Rob Herring wrote:
> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
> > commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
> > introduced common DT bindings for touchscreens [1] and a helper function to
> > parse the DT.
> > 
> > commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
> > added another helper for parsing axis inversion and swapping
> > and applying them to x and y coordinates.
> > 
> > Both helpers have been integrated to accommodate any orientation of the
> > touch panel in relation to the LCD.
> > 
> > A new feature is to introduce scaling the min/max ADC values to the screen
> > size.
> > 
> > This makes it possible to pre-calibrate the touch so that is (almost)
> > exactly matches the LCD pixel coordinates it is glued onto. This allows to
> > well enough operate the touch before a user space calibration step can
> > improve the precision.
> > 
> > Finally, calculate_pressure has been renamed to calculate_resistance
> > because that is what it is doing.
> 
> Seems like you are breaking compatibility with old DTs. I can't tell for 
> sure though.
> 
> > 
> > [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> > 
> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> > ---
> >  .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
> >  drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
> >  include/linux/i2c/tsc2007.h                        |   8 ++
> >  3 files changed, 123 insertions(+), 31 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > index ec365e1..6e9fd55 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> > @@ -6,6 +6,7 @@ Required properties:
> >  - ti,x-plate-ohms: X-plate resistance in ohms.
> >  
> >  Optional properties:
> > +- generic touch screen properties: see touchscreen binding [2].
> >  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
> >    The penirq pin goes to low when the panel is touched.
> >    (see GPIO binding[1] for more details).
> > @@ -13,17 +14,20 @@ Optional properties:
> >    (see interrupt binding[0]).
> >  - interrupts: (gpio) interrupt to which the chip is connected
> >    (see interrupt binding[0]).
> > -- ti,max-rt: maximum pressure.
> > -- ti,fuzzx: specifies the absolute input fuzz x value.
> > -  If set, it will permit noise in the data up to +- the value given to the fuzz
> > -  parameter, that is used to filter noise from the event stream.
> > -- ti,fuzzy: specifies the absolute input fuzz y value.
> > -- ti,fuzzz: specifies the absolute input fuzz z value.
> > +- ti,max-rt: maximum pressure resistance above which samples are ignored
> > +  (default: 4095).
> > +- ti,report-resistance: report resistance (no pressure = max_rt) instead
> > +  of pressure (no pressure = 0).
> > +- ti,min-x: minimum value reported by X axis ADC (default 0).
> > +- ti,max-x: maximum value reported by X axis ADC (default 4095).
> > +- ti,min-y: minimum value reported by Y axis ADC (default 0).
> > +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
> 
> Seems like these could be common too. They make more sense than giving x 
> and y sizes in pixel units which really should come from the panel.

The generic touchscreen properties are described "(in pixels)" in
the DT, but they are used in the same way.

So ti,max-[xy] is basically the same as touchscreen-size-[xy],
except, that the generic bindings don't support min-[xy] != 0.

So maybe change the generic bindings like this:

touchscreen-min-x: minimum value reported by X axis ADC (default 0)
touchscreen-max-x: maximum value reported by Y axis ADC
touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
touchscreen-max-y: maximum value reported by Y axis ADC
touchscreen-size-x: deprecated alias for touchscreen-max-x
touchscreen-size-y: deprecated alias for touchscreen-max-y

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 7133 bytes --]

Hi,

> Am 24.09.2016 um 02:31 schrieb Sebastian Reichel <sre@kernel.org>:
> 
> On Fri, Sep 23, 2016 at 05:47:26PM -0500, Rob Herring wrote:
>> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
>>> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
>>> introduced common DT bindings for touchscreens [1] and a helper function to
>>> parse the DT.
>>> 
>>> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
>>> added another helper for parsing axis inversion and swapping
>>> and applying them to x and y coordinates.
>>> 
>>> Both helpers have been integrated to accommodate any orientation of the
>>> touch panel in relation to the LCD.
>>> 
>>> A new feature is to introduce scaling the min/max ADC values to the screen
>>> size.
>>> 
>>> This makes it possible to pre-calibrate the touch so that is (almost)
>>> exactly matches the LCD pixel coordinates it is glued onto. This allows to
>>> well enough operate the touch before a user space calibration step can
>>> improve the precision.
>>> 
>>> Finally, calculate_pressure has been renamed to calculate_resistance
>>> because that is what it is doing.
>> 
>> Seems like you are breaking compatibility with old DTs. I can't tell for
>> sure though.
>> 
>>> 
>>> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>> 
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>>> drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>>> include/linux/i2c/tsc2007.h                        |   8 ++
>>> 3 files changed, 123 insertions(+), 31 deletions(-)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> index ec365e1..6e9fd55 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> @@ -6,6 +6,7 @@ Required properties:
>>> - ti,x-plate-ohms: X-plate resistance in ohms.
>>> 
>>> Optional properties:
>>> +- generic touch screen properties: see touchscreen binding [2].
>>> - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>>>   The penirq pin goes to low when the panel is touched.
>>>   (see GPIO binding[1] for more details).
>>> @@ -13,17 +14,20 @@ Optional properties:
>>>   (see interrupt binding[0]).
>>> - interrupts: (gpio) interrupt to which the chip is connected
>>>   (see interrupt binding[0]).
>>> -- ti,max-rt: maximum pressure.
>>> -- ti,fuzzx: specifies the absolute input fuzz x value.
>>> -  If set, it will permit noise in the data up to +- the value given to the fuzz
>>> -  parameter, that is used to filter noise from the event stream.
>>> -- ti,fuzzy: specifies the absolute input fuzz y value.
>>> -- ti,fuzzz: specifies the absolute input fuzz z value.
>>> +- ti,max-rt: maximum pressure resistance above which samples are ignored
>>> +  (default: 4095).
>>> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
>>> +  of pressure (no pressure = 0).
>>> +- ti,min-x: minimum value reported by X axis ADC (default 0).
>>> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
>>> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
>>> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
>> 
>> Seems like these could be common too. They make more sense than giving x
>> and y sizes in pixel units which really should come from the panel.
> 
> The generic touchscreen properties are described "(in pixels)" in
> the DT, but they are used in the same way.
> 
> So ti,max-[xy] is basically the same as touchscreen-size-[xy],

No it is not the same and should be kept separate.

> except, that the generic bindings don't support min-[xy] != 0.

What would be the purpose of this? Every user-space I know
about (X11, Replicant) expects coordinates in some range
0..max so setting min in device tree makes no sense to me.

> 
> So maybe change the generic bindings like this:
> 
> touchscreen-min-x: minimum value reported by X axis ADC (default 0)
> touchscreen-max-x: maximum value reported by Y axis ADC
> touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
> touchscreen-max-y: maximum value reported by Y axis ADC
> touchscreen-size-x: deprecated alias for touchscreen-max-x
> touchscreen-size-y: deprecated alias for touchscreen-max-y
> 

Initially I had thought about this but it does not solve the problems
with touch pre-calibration. Since it mixes raw coordinates with
system coordinates.

To achieve the goal of having a roughly precalibrated touch which
should provide (0,0) at the lower left corner and (touchscreen-size-x,touchscreen-size-y)
in pixel coordinates of the panel. Hence it roughly works without
a calibration matrix in user space (e.g. xorg.conf or Replicant).

Why do we need pre-calibration? Because some systems might need
touch interaction before they can offer (force) the user into
a touch calibration step. We use these drivers and approach in
our production kernels for GTA04, OpenPandora and Pyra for a while
and nobody was even missing a user-space calibration tool any more.

The underlaying problem is that you can have the same controller chip
in different board designs and there are different touch panel types.
Each one has certain physical properties but they can differ.
But you certainly want touchscreen-size-x/y to be a constant.

Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
and change the panel, we have to adjust user space transformation
each time we change the panel. This does not seem to be right
and can be done better by keeping them separately.

This is what this approach does: the roughly correct scaling of
raw values to pixel values.

ti,min-x   ->   0
...
x          ->   some value between 0 and touchscreen-size-x
                calculated by
			touchscreen-size-x * (x - min-x) / (max-x - min-x)
...
ti,max-x   ->   touchscreen-size-x

Hence the ti,min/max values describe the range of expected input
values from the ADC and the touchscreen-size-x describes the touch
in LCD pixels passed as input events.

Example:

ti,min-x = 64
ti,max-x = 4016
touchscreen-size-x = 480

If we change the panel type which presents a slightly different ADC range:

ti,min-x = 100
ti,max-x = 3900
touchscreen-size-x = 480

and we still get a coordinate range (0 .. 480).

Note that this feature can be effectively disabled if ti,min-x=0 and
ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
range of ADC values because then it multiplies by 1.

Our proposed driver does use these values if they are missing from DT
and therefore it should not break old DT files which expect raw values
to be reported.

I hope this clarifies what we need to achieve and you can
agree.

BR and thanks,
Nikolaus

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4909 bytes --]

Hi,

On Sat, Sep 24, 2016 at 07:55:27AM +0200, H. Nikolaus Schaller wrote:
> > So ti,max-[xy] is basically the same as touchscreen-size-[xy],
> 
> No it is not the same and should be kept separate.
> 
> > except, that the generic bindings don't support min-[xy] != 0.
> 
> What would be the purpose of this? Every user-space I know
> about (X11, Replicant) expects coordinates in some range
> 0..max so setting min in device tree makes no sense to me.
> 
> > 
> > So maybe change the generic bindings like this:
> > 
> > touchscreen-min-x: minimum value reported by X axis ADC (default 0)
> > touchscreen-max-x: maximum value reported by Y axis ADC
> > touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
> > touchscreen-max-y: maximum value reported by Y axis ADC
> > touchscreen-size-x: deprecated alias for touchscreen-max-x
> > touchscreen-size-y: deprecated alias for touchscreen-max-y
> > 
> 
> Initially I had thought about this but it does not solve the problems
> with touch pre-calibration. Since it mixes raw coordinates with
> system coordinates.

touchscreen-size-x was actually refering to your definition of
touchscreen-max-x and not system coordinates. For that it would
make much more sense to use a phandle to the screen IMHO.

> To achieve the goal of having a roughly precalibrated touch which
> should provide (0,0) at the lower left corner and
> (touchscreen-size-x,touchscreen-size-y) in pixel coordinates of
> the panel. Hence it roughly works without a calibration matrix in
> user space (e.g. xorg.conf or Replicant).

well I did not mean to use touchscreen-size-x/y for describing the
size of screen, as visible in n900.dts (first implementation of the
common binding), which sets the value to 4096.

> Why do we need pre-calibration? Because some systems might need
> touch interaction before they can offer (force) the user into
> a touch calibration step. We use these drivers and approach in
> our production kernels for GTA04, OpenPandora and Pyra for a while
> and nobody was even missing a user-space calibration tool any more.

I have nothing against the feature. OTOH I'm quite in of kernel
based TS calibration. Note, that you can only add it for hardware
without pre-existing touchscreen support, since you break peoples
systems otherwise (We have that problem for N900).

> The underlaying problem is that you can have the same controller chip
> in different board designs and there are different touch panel types.
> Each one has certain physical properties but they can differ.
> But you certainly want touchscreen-size-x/y to be a constant.
>
> Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
> and change the panel, we have to adjust user space transformation
> each time we change the panel. This does not seem to be right
> and can be done better by keeping them separately.
>
> This is what this approach does: the roughly correct scaling of
> raw values to pixel values.
> 
> ti,min-x   ->   0
> ...
> x          ->   some value between 0 and touchscreen-size-x
>                 calculated by
> 			touchscreen-size-x * (x - min-x) / (max-x - min-x)
> ...
> ti,max-x   ->   touchscreen-size-x
> 
> Hence the ti,min/max values describe the range of expected input
> values from the ADC and the touchscreen-size-x describes the touch
> in LCD pixels passed as input events.

so basically you use touchscreen-size-x to describe the screen and
not the touchscreen. When I added it, I did mean the max ADC value.
Actually I was under the impression, that X drivers would scale this
to screen size automatically. Since all my touchscreen HW required
calibration I did never test this, though.

> Example:
> 
> ti,min-x = 64
> ti,max-x = 4016
> touchscreen-size-x = 480
> 
> If we change the panel type which presents a slightly different ADC range:
> 
> ti,min-x = 100
> ti,max-x = 3900
> touchscreen-size-x = 480
> 
> and we still get a coordinate range (0 .. 480).
> 
> Note that this feature can be effectively disabled if ti,min-x=0 and
> ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
> range of ADC values because then it multiplies by 1.
> 
> Our proposed driver does use these values if they are missing from DT
> and therefore it should not break old DT files which expect raw values
> to be reported.
> 
> I hope this clarifies what we need to achieve and you can
> agree.

I did understand what you want, but I disagreed about
using touchscreen-size-x/y for system coordinates. I
now see, that it's too late for that, as other people
already did so.

I do agree with Rob, that the ti,min/max-x/y should become common,
though. Also I would do s/minimum value/minimum raw value/g.
Additionally touchscreen-size-x/y should mention, that it's used to
scale the raw values.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4909 bytes --]

Hi,

On Sat, Sep 24, 2016 at 07:55:27AM +0200, H. Nikolaus Schaller wrote:
> > So ti,max-[xy] is basically the same as touchscreen-size-[xy],
> 
> No it is not the same and should be kept separate.
> 
> > except, that the generic bindings don't support min-[xy] != 0.
> 
> What would be the purpose of this? Every user-space I know
> about (X11, Replicant) expects coordinates in some range
> 0..max so setting min in device tree makes no sense to me.
> 
> > 
> > So maybe change the generic bindings like this:
> > 
> > touchscreen-min-x: minimum value reported by X axis ADC (default 0)
> > touchscreen-max-x: maximum value reported by Y axis ADC
> > touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
> > touchscreen-max-y: maximum value reported by Y axis ADC
> > touchscreen-size-x: deprecated alias for touchscreen-max-x
> > touchscreen-size-y: deprecated alias for touchscreen-max-y
> > 
> 
> Initially I had thought about this but it does not solve the problems
> with touch pre-calibration. Since it mixes raw coordinates with
> system coordinates.

touchscreen-size-x was actually refering to your definition of
touchscreen-max-x and not system coordinates. For that it would
make much more sense to use a phandle to the screen IMHO.

> To achieve the goal of having a roughly precalibrated touch which
> should provide (0,0) at the lower left corner and
> (touchscreen-size-x,touchscreen-size-y) in pixel coordinates of
> the panel. Hence it roughly works without a calibration matrix in
> user space (e.g. xorg.conf or Replicant).

well I did not mean to use touchscreen-size-x/y for describing the
size of screen, as visible in n900.dts (first implementation of the
common binding), which sets the value to 4096.

> Why do we need pre-calibration? Because some systems might need
> touch interaction before they can offer (force) the user into
> a touch calibration step. We use these drivers and approach in
> our production kernels for GTA04, OpenPandora and Pyra for a while
> and nobody was even missing a user-space calibration tool any more.

I have nothing against the feature. OTOH I'm quite in of kernel
based TS calibration. Note, that you can only add it for hardware
without pre-existing touchscreen support, since you break peoples
systems otherwise (We have that problem for N900).

> The underlaying problem is that you can have the same controller chip
> in different board designs and there are different touch panel types.
> Each one has certain physical properties but they can differ.
> But you certainly want touchscreen-size-x/y to be a constant.
>
> Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
> and change the panel, we have to adjust user space transformation
> each time we change the panel. This does not seem to be right
> and can be done better by keeping them separately.
>
> This is what this approach does: the roughly correct scaling of
> raw values to pixel values.
> 
> ti,min-x   ->   0
> ...
> x          ->   some value between 0 and touchscreen-size-x
>                 calculated by
> 			touchscreen-size-x * (x - min-x) / (max-x - min-x)
> ...
> ti,max-x   ->   touchscreen-size-x
> 
> Hence the ti,min/max values describe the range of expected input
> values from the ADC and the touchscreen-size-x describes the touch
> in LCD pixels passed as input events.

so basically you use touchscreen-size-x to describe the screen and
not the touchscreen. When I added it, I did mean the max ADC value.
Actually I was under the impression, that X drivers would scale this
to screen size automatically. Since all my touchscreen HW required
calibration I did never test this, though.

> Example:
> 
> ti,min-x = 64
> ti,max-x = 4016
> touchscreen-size-x = 480
> 
> If we change the panel type which presents a slightly different ADC range:
> 
> ti,min-x = 100
> ti,max-x = 3900
> touchscreen-size-x = 480
> 
> and we still get a coordinate range (0 .. 480).
> 
> Note that this feature can be effectively disabled if ti,min-x=0 and
> ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
> range of ADC values because then it multiplies by 1.
> 
> Our proposed driver does use these values if they are missing from DT
> and therefore it should not break old DT files which expect raw values
> to be reported.
> 
> I hope this clarifies what we need to achieve and you can
> agree.

I did understand what you want, but I disagreed about
using touchscreen-size-x/y for system coordinates. I
now see, that it's too late for that, as other people
already did so.

I do agree with Rob, that the ti,min/max-x/y should become common,
though. Also I would do s/minimum value/minimum raw value/g.
Additionally touchscreen-size-x/y should mention, that it's used to
scale the raw values.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]

Hi Sebastian,

> Am 30.09.2016 um 16:16 schrieb Sebastian Reichel <sre@kernel.org>:
> 
> Hi,
> 
> On Sat, Sep 24, 2016 at 07:55:27AM +0200, H. Nikolaus Schaller wrote:
>>> So ti,max-[xy] is basically the same as touchscreen-size-[xy],
>> 
>> No it is not the same and should be kept separate.
>> 
>>> except, that the generic bindings don't support min-[xy] != 0.
>> 
>> What would be the purpose of this? Every user-space I know
>> about (X11, Replicant) expects coordinates in some range
>> 0..max so setting min in device tree makes no sense to me.
>> 
>>> 
>>> So maybe change the generic bindings like this:
>>> 
>>> touchscreen-min-x: minimum value reported by X axis ADC (default 0)
>>> touchscreen-max-x: maximum value reported by Y axis ADC
>>> touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
>>> touchscreen-max-y: maximum value reported by Y axis ADC
>>> touchscreen-size-x: deprecated alias for touchscreen-max-x
>>> touchscreen-size-y: deprecated alias for touchscreen-max-y
>>> 
>> 
>> Initially I had thought about this but it does not solve the problems
>> with touch pre-calibration. Since it mixes raw coordinates with
>> system coordinates.
> 
> touchscreen-size-x was actually refering to your definition of
> touchscreen-max-x and not system coordinates.

Ah, ok I see. It looks as if you have provided a hidden feature by mis-using
it to describe LCD size.

> For that it would
> make much more sense to use a phandle to the screen IMHO.

Well... phandles are evil as I have learned in different context.
And, there are many LCD devices that do not specify screen sizes in
pixels.

I would more see it as a definition like a key matrix. You still have
both, the x/y raw coordinates and the processed KEY_something codes
in a single table.

> 
>> To achieve the goal of having a roughly precalibrated touch which
>> should provide (0,0) at the lower left corner and
>> (touchscreen-size-x,touchscreen-size-y) in pixel coordinates of
>> the panel. Hence it roughly works without a calibration matrix in
>> user space (e.g. xorg.conf or Replicant).
> 
> well I did not mean to use touchscreen-size-x/y for describing the
> size of screen, as visible in n900.dts (first implementation of the
> common binding), which sets the value to 4096.

Well, for my scheme it still works if you use it that way. Like keycodes
which can still be mapped in user space to anything desired.

> 
>> Why do we need pre-calibration? Because some systems might need
>> touch interaction before they can offer (force) the user into
>> a touch calibration step. We use these drivers and approach in
>> our production kernels for GTA04, OpenPandora and Pyra for a while
>> and nobody was even missing a user-space calibration tool any more.
> 
> I have nothing against the feature. OTOH I'm quite in of kernel
> based TS calibration. Note, that you can only add it for hardware
> without pre-existing touchscreen support, since you break peoples
> systems otherwise (We have that problem for N900).

Unless it takes missing ADC values as the same defaults that have been
the assumption. I.e. if it is missing, take e.g. 0 and 4095.

> 
>> The underlaying problem is that you can have the same controller chip
>> in different board designs and there are different touch panel types.
>> Each one has certain physical properties but they can differ.
>> But you certainly want touchscreen-size-x/y to be a constant.
>> 
>> Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
>> and change the panel, we have to adjust user space transformation
>> each time we change the panel. This does not seem to be right
>> and can be done better by keeping them separately.
>> 
>> This is what this approach does: the roughly correct scaling of
>> raw values to pixel values.
>> 
>> ti,min-x   ->   0
>> ...
>> x          ->   some value between 0 and touchscreen-size-x
>>                calculated by
>> 			touchscreen-size-x * (x - min-x) / (max-x - min-x)
>> ...
>> ti,max-x   ->   touchscreen-size-x
>> 
>> Hence the ti,min/max values describe the range of expected input
>> values from the ADC and the touchscreen-size-x describes the touch
>> in LCD pixels passed as input events.
> 
> so basically you use touchscreen-size-x to describe the screen and
> not the touchscreen.

> When I added it, I did mean the max ADC value.
> Actually I was under the impression, that X drivers would scale this
> to screen size automatically.

There is some default calibration or it can be set by entries in xorg.conf,
but since there is no intermediate "constant" scale, you promote any hardware
variation (different ADC max values) to user space.

> Since all my touchscreen HW required
> calibration I did never test this, though.
> 
>> Example:
>> 
>> ti,min-x = 64
>> ti,max-x = 4016
>> touchscreen-size-x = 480
>> 
>> If we change the panel type which presents a slightly different ADC range:
>> 
>> ti,min-x = 100
>> ti,max-x = 3900
>> touchscreen-size-x = 480
>> 
>> and we still get a coordinate range (0 .. 480).
>> 
>> Note that this feature can be effectively disabled if ti,min-x=0 and
>> ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
>> range of ADC values because then it multiplies by 1.
>> 
>> Our proposed driver does use these values if they are missing from DT
>> and therefore it should not break old DT files which expect raw values
>> to be reported.
>> 
>> I hope this clarifies what we need to achieve and you can
>> agree.
> 
> I did understand what you want, but I disagreed about
> using touchscreen-size-x/y for system coordinates. I
> now see, that it's too late for that, as other people
> already did so.
> 
> I do agree with Rob, that the ti,min/max-x/y should become common,
> though. Also I would do s/minimum value/minimum raw value/g.

Yes, that is a good idea!

To me it seems as if "minimum-raw-value-x", "maximum-raw-value-y" could be good.

Or is the string too long (then we can drop the "-value" or the "imum")? It
might blow up the DTB and string constant for finding the property?

If we come to some consensus on this, I can update the patch set.

> Additionally touchscreen-size-x/y should mention, that it's used to
> scale the raw values.

Ok. It is also used as the default for maximim-raw-value-x/y if not specified
which means 1:1 scaling.

BR and thanks,
Nikolaus


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 6546 bytes --]

Hi Sebastian,

> Am 30.09.2016 um 16:16 schrieb Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> 
> Hi,
> 
> On Sat, Sep 24, 2016 at 07:55:27AM +0200, H. Nikolaus Schaller wrote:
>>> So ti,max-[xy] is basically the same as touchscreen-size-[xy],
>> 
>> No it is not the same and should be kept separate.
>> 
>>> except, that the generic bindings don't support min-[xy] != 0.
>> 
>> What would be the purpose of this? Every user-space I know
>> about (X11, Replicant) expects coordinates in some range
>> 0..max so setting min in device tree makes no sense to me.
>> 
>>> 
>>> So maybe change the generic bindings like this:
>>> 
>>> touchscreen-min-x: minimum value reported by X axis ADC (default 0)
>>> touchscreen-max-x: maximum value reported by Y axis ADC
>>> touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
>>> touchscreen-max-y: maximum value reported by Y axis ADC
>>> touchscreen-size-x: deprecated alias for touchscreen-max-x
>>> touchscreen-size-y: deprecated alias for touchscreen-max-y
>>> 
>> 
>> Initially I had thought about this but it does not solve the problems
>> with touch pre-calibration. Since it mixes raw coordinates with
>> system coordinates.
> 
> touchscreen-size-x was actually refering to your definition of
> touchscreen-max-x and not system coordinates.

Ah, ok I see. It looks as if you have provided a hidden feature by mis-using
it to describe LCD size.

> For that it would
> make much more sense to use a phandle to the screen IMHO.

Well... phandles are evil as I have learned in different context.
And, there are many LCD devices that do not specify screen sizes in
pixels.

I would more see it as a definition like a key matrix. You still have
both, the x/y raw coordinates and the processed KEY_something codes
in a single table.

> 
>> To achieve the goal of having a roughly precalibrated touch which
>> should provide (0,0) at the lower left corner and
>> (touchscreen-size-x,touchscreen-size-y) in pixel coordinates of
>> the panel. Hence it roughly works without a calibration matrix in
>> user space (e.g. xorg.conf or Replicant).
> 
> well I did not mean to use touchscreen-size-x/y for describing the
> size of screen, as visible in n900.dts (first implementation of the
> common binding), which sets the value to 4096.

Well, for my scheme it still works if you use it that way. Like keycodes
which can still be mapped in user space to anything desired.

> 
>> Why do we need pre-calibration? Because some systems might need
>> touch interaction before they can offer (force) the user into
>> a touch calibration step. We use these drivers and approach in
>> our production kernels for GTA04, OpenPandora and Pyra for a while
>> and nobody was even missing a user-space calibration tool any more.
> 
> I have nothing against the feature. OTOH I'm quite in of kernel
> based TS calibration. Note, that you can only add it for hardware
> without pre-existing touchscreen support, since you break peoples
> systems otherwise (We have that problem for N900).

Unless it takes missing ADC values as the same defaults that have been
the assumption. I.e. if it is missing, take e.g. 0 and 4095.

> 
>> The underlaying problem is that you can have the same controller chip
>> in different board designs and there are different touch panel types.
>> Each one has certain physical properties but they can differ.
>> But you certainly want touchscreen-size-x/y to be a constant.
>> 
>> Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
>> and change the panel, we have to adjust user space transformation
>> each time we change the panel. This does not seem to be right
>> and can be done better by keeping them separately.
>> 
>> This is what this approach does: the roughly correct scaling of
>> raw values to pixel values.
>> 
>> ti,min-x   ->   0
>> ...
>> x          ->   some value between 0 and touchscreen-size-x
>>                calculated by
>> 			touchscreen-size-x * (x - min-x) / (max-x - min-x)
>> ...
>> ti,max-x   ->   touchscreen-size-x
>> 
>> Hence the ti,min/max values describe the range of expected input
>> values from the ADC and the touchscreen-size-x describes the touch
>> in LCD pixels passed as input events.
> 
> so basically you use touchscreen-size-x to describe the screen and
> not the touchscreen.

> When I added it, I did mean the max ADC value.
> Actually I was under the impression, that X drivers would scale this
> to screen size automatically.

There is some default calibration or it can be set by entries in xorg.conf,
but since there is no intermediate "constant" scale, you promote any hardware
variation (different ADC max values) to user space.

> Since all my touchscreen HW required
> calibration I did never test this, though.
> 
>> Example:
>> 
>> ti,min-x = 64
>> ti,max-x = 4016
>> touchscreen-size-x = 480
>> 
>> If we change the panel type which presents a slightly different ADC range:
>> 
>> ti,min-x = 100
>> ti,max-x = 3900
>> touchscreen-size-x = 480
>> 
>> and we still get a coordinate range (0 .. 480).
>> 
>> Note that this feature can be effectively disabled if ti,min-x=0 and
>> ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
>> range of ADC values because then it multiplies by 1.
>> 
>> Our proposed driver does use these values if they are missing from DT
>> and therefore it should not break old DT files which expect raw values
>> to be reported.
>> 
>> I hope this clarifies what we need to achieve and you can
>> agree.
> 
> I did understand what you want, but I disagreed about
> using touchscreen-size-x/y for system coordinates. I
> now see, that it's too late for that, as other people
> already did so.
> 
> I do agree with Rob, that the ti,min/max-x/y should become common,
> though. Also I would do s/minimum value/minimum raw value/g.

Yes, that is a good idea!

To me it seems as if "minimum-raw-value-x", "maximum-raw-value-y" could be good.

Or is the string too long (then we can drop the "-value" or the "imum")? It
might blow up the DTB and string constant for finding the property?

If we come to some consensus on this, I can update the patch set.

> Additionally touchscreen-size-x/y should mention, that it's used to
> scale the raw values.

Ok. It is also used as the default for maximim-raw-value-x/y if not specified
which means 1:1 scaling.

BR and thanks,
Nikolaus


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

Hi Sebastian,
I did wait for the 4.9-rc1 merge window to be closed until I bring up this patch
set again.

> Am 30.09.2016 um 16:40 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>> 
>> I do agree with Rob, that the ti,min/max-x/y should become common,
>> though. Also I would do s/minimum value/minimum raw value/g.
> 
> Yes, that is a good idea!
> 
> To me it seems as if "minimum-raw-value-x", "maximum-raw-value-y" could be good.
> 
> Or is the string too long (then we can drop the "-value" or the "imum")? It
> might blow up the DTB and string constant for finding the property?
> 
> If we come to some consensus on this, I can update the patch set.

Now when updating the patch set, I found that using ti,min/max-x/y was not really
a new invention. It is already upstream for a while for the ads7846/2046 driver!

This means: by introducing these properties to the tsc2007 as well we just extend the
already defined use of ti,min/max-x/y.

Simply changing that as well to use some minimum-raw-value-x etc. would break ads7846/2046
boards not in mainline or need ugly code to handle property alias names.

And since they are raw data and quite chip-specific, I think there is no urgent need
for harmonization of raw device data. While using the common bindings for output
scaling and fuzz, swap etc. is very important and helpful for board designers.

So I would prefer if we can keep it as is for the moment. And make property name
harmonization and evolution a separate task and discussion.

I will submit v4 in a couple of minutes. It contains some other small fixes as
suggested by the previous discussion.

BR and thanks,
Nikolaus


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

Hi Sebastian,
I did wait for the 4.9-rc1 merge window to be closed until I bring up this patch
set again.

> Am 30.09.2016 um 16:40 schrieb H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>:
>> 
>> I do agree with Rob, that the ti,min/max-x/y should become common,
>> though. Also I would do s/minimum value/minimum raw value/g.
> 
> Yes, that is a good idea!
> 
> To me it seems as if "minimum-raw-value-x", "maximum-raw-value-y" could be good.
> 
> Or is the string too long (then we can drop the "-value" or the "imum")? It
> might blow up the DTB and string constant for finding the property?
> 
> If we come to some consensus on this, I can update the patch set.

Now when updating the patch set, I found that using ti,min/max-x/y was not really
a new invention. It is already upstream for a while for the ads7846/2046 driver!

This means: by introducing these properties to the tsc2007 as well we just extend the
already defined use of ti,min/max-x/y.

Simply changing that as well to use some minimum-raw-value-x etc. would break ads7846/2046
boards not in mainline or need ugly code to handle property alias names.

And since they are raw data and quite chip-specific, I think there is no urgent need
for harmonization of raw device data. While using the common bindings for output
scaling and fuzz, swap etc. is very important and helpful for board designers.

So I would prefer if we can keep it as is for the moment. And make property name
harmonization and evolution a separate task and discussion.

I will submit v4 in a couple of minutes. It contains some other small fixes as
suggested by the previous discussion.

BR and thanks,
Nikolaus


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi,

> Am 24.09.2016 um 00:47 schrieb Rob Herring <robh@kernel.org>:
> 
> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
>> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
>> introduced common DT bindings for touchscreens [1] and a helper function to
>> parse the DT.
>> 
>> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
>> added another helper for parsing axis inversion and swapping
>> and applying them to x and y coordinates.
>> 
>> Both helpers have been integrated to accommodate any orientation of the
>> touch panel in relation to the LCD.
>> 
>> A new feature is to introduce scaling the min/max ADC values to the screen
>> size.
>> 
>> This makes it possible to pre-calibrate the touch so that is (almost)
>> exactly matches the LCD pixel coordinates it is glued onto. This allows to
>> well enough operate the touch before a user space calibration step can
>> improve the precision.
>> 
>> Finally, calculate_pressure has been renamed to calculate_resistance
>> because that is what it is doing.
> 
> Seems like you are breaking compatibility with old DTs. I can't tell for 
> sure though.

There is code to take missing values as 0 or maximum. So this will scale 1:1
and should not break old DTs.

> 
>> 
>> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>> drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>> include/linux/i2c/tsc2007.h                        |   8 ++
>> 3 files changed, 123 insertions(+), 31 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> index ec365e1..6e9fd55 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> @@ -6,6 +6,7 @@ Required properties:
>> - ti,x-plate-ohms: X-plate resistance in ohms.
>> 
>> Optional properties:
>> +- generic touch screen properties: see touchscreen binding [2].
>> - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>>   The penirq pin goes to low when the panel is touched.
>>   (see GPIO binding[1] for more details).
>> @@ -13,17 +14,20 @@ Optional properties:
>>   (see interrupt binding[0]).
>> - interrupts: (gpio) interrupt to which the chip is connected
>>   (see interrupt binding[0]).
>> -- ti,max-rt: maximum pressure.
>> -- ti,fuzzx: specifies the absolute input fuzz x value.
>> -  If set, it will permit noise in the data up to +- the value given to the fuzz
>> -  parameter, that is used to filter noise from the event stream.
>> -- ti,fuzzy: specifies the absolute input fuzz y value.
>> -- ti,fuzzz: specifies the absolute input fuzz z value.
>> +- ti,max-rt: maximum pressure resistance above which samples are ignored
>> +  (default: 4095).
>> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
>> +  of pressure (no pressure = 0).
>> +- ti,min-x: minimum value reported by X axis ADC (default 0).
>> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
>> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
>> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
> 
> Seems like these could be common too. They make more sense than giving x 
> and y sizes in pixel units which really should come from the panel.

No. They have a different purpose!

You need to scale values like this:

ti,min-x   ->   0
x          ->   some value between 0 and touchscreen-size-x
ti,max-x   ->   touchscreen-size-x

So the ti, values describe the ADC raw values while the common bindings
describe the size in input-event coordinates.

So we need both specified: raw values and pixel values.

> 
>> - ti,poll-period: how much time to wait (in milliseconds) before reading again the
>> -  values from the tsc2007.
>> +  values from the tsc2007 (default 1).
>> 
>> [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
>> +[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> 
>> Example:
>> 	&i2c1 {
>> @@ -35,6 +39,8 @@ Example:
>> 			interrupts = <0x0 0x8>;
>> 			gpios = <&gpio4 0 0>;
>> 			ti,x-plate-ohms = <180>;
>> +			touchscreen-size-x = <640>;
>> +			touchscreen-size-y = <480>;
>> 		};
>> 
>> 		/* ... */

BR,
Nikolaus

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi,

> Am 24.09.2016 um 00:47 schrieb Rob Herring <robh@kernel.org>:
>=20
> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
>> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
>> introduced common DT bindings for touchscreens [1] and a helper =
function to
>> parse the DT.
>>=20
>> commit ed7c9870c9bc ("Input: of_touchscreen - add support for =
inverted / swapped axes")
>> added another helper for parsing axis inversion and swapping
>> and applying them to x and y coordinates.
>>=20
>> Both helpers have been integrated to accommodate any orientation of =
the
>> touch panel in relation to the LCD.
>>=20
>> A new feature is to introduce scaling the min/max ADC values to the =
screen
>> size.
>>=20
>> This makes it possible to pre-calibrate the touch so that is (almost)
>> exactly matches the LCD pixel coordinates it is glued onto. This =
allows to
>> well enough operate the touch before a user space calibration step =
can
>> improve the precision.
>>=20
>> Finally, calculate_pressure has been renamed to calculate_resistance
>> because that is what it is doing.
>=20
> Seems like you are breaking compatibility with old DTs. I can't tell =
for=20
> sure though.

There is code to take missing values as 0 or maximum. So this will scale =
1:1
and should not break old DTs.

>=20
>>=20
>> [1]: =
Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>=20
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>> drivers/input/touchscreen/tsc2007.c                | 126 =
+++++++++++++++++----
>> include/linux/i2c/tsc2007.h                        |   8 ++
>> 3 files changed, 123 insertions(+), 31 deletions(-)
>>=20
>> diff --git =
a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt =
b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> index ec365e1..6e9fd55 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> @@ -6,6 +6,7 @@ Required properties:
>> - ti,x-plate-ohms: X-plate resistance in ohms.
>>=20
>> Optional properties:
>> +- generic touch screen properties: see touchscreen binding [2].
>> - gpios: the interrupt gpio the chip is connected to (trough the =
penirq pin).
>>   The penirq pin goes to low when the panel is touched.
>>   (see GPIO binding[1] for more details).
>> @@ -13,17 +14,20 @@ Optional properties:
>>   (see interrupt binding[0]).
>> - interrupts: (gpio) interrupt to which the chip is connected
>>   (see interrupt binding[0]).
>> -- ti,max-rt: maximum pressure.
>> -- ti,fuzzx: specifies the absolute input fuzz x value.
>> -  If set, it will permit noise in the data up to +- the value given =
to the fuzz
>> -  parameter, that is used to filter noise from the event stream.
>> -- ti,fuzzy: specifies the absolute input fuzz y value.
>> -- ti,fuzzz: specifies the absolute input fuzz z value.
>> +- ti,max-rt: maximum pressure resistance above which samples are =
ignored
>> +  (default: 4095).
>> +- ti,report-resistance: report resistance (no pressure =3D max_rt) =
instead
>> +  of pressure (no pressure =3D 0).
>> +- ti,min-x: minimum value reported by X axis ADC (default 0).
>> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
>> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
>> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
>=20
> Seems like these could be common too. They make more sense than giving =
x=20
> and y sizes in pixel units which really should come from the panel.

No. They have a different purpose!

You need to scale values like this:

ti,min-x   ->   0
x          ->   some value between 0 and touchscreen-size-x
ti,max-x   ->   touchscreen-size-x

So the ti, values describe the ADC raw values while the common bindings
describe the size in input-event coordinates.

So we need both specified: raw values and pixel values.

>=20
>> - ti,poll-period: how much time to wait (in milliseconds) before =
reading again the
>> -  values from the tsc2007.
>> +  values from the tsc2007 (default 1).
>>=20
>> [0]: =
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
>> +[2]: =
Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>=20
>> Example:
>> 	&i2c1 {
>> @@ -35,6 +39,8 @@ Example:
>> 			interrupts =3D <0x0 0x8>;
>> 			gpios =3D <&gpio4 0 0>;
>> 			ti,x-plate-ohms =3D <180>;
>> +			touchscreen-size-x =3D <640>;
>> +			touchscreen-size-y =3D <480>;
>> 		};
>>=20
>> 		/* ... */

BR,
Nikolaus

Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi,

> Am 24.09.2016 um 00:47 schrieb Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> 
> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
>> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
>> introduced common DT bindings for touchscreens [1] and a helper function to
>> parse the DT.
>> 
>> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
>> added another helper for parsing axis inversion and swapping
>> and applying them to x and y coordinates.
>> 
>> Both helpers have been integrated to accommodate any orientation of the
>> touch panel in relation to the LCD.
>> 
>> A new feature is to introduce scaling the min/max ADC values to the screen
>> size.
>> 
>> This makes it possible to pre-calibrate the touch so that is (almost)
>> exactly matches the LCD pixel coordinates it is glued onto. This allows to
>> well enough operate the touch before a user space calibration step can
>> improve the precision.
>> 
>> Finally, calculate_pressure has been renamed to calculate_resistance
>> because that is what it is doing.
> 
> Seems like you are breaking compatibility with old DTs. I can't tell for 
> sure though.

There is code to take missing values as 0 or maximum. So this will scale 1:1
and should not break old DTs.

> 
>> 
>> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>> ---
>> .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>> drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>> include/linux/i2c/tsc2007.h                        |   8 ++
>> 3 files changed, 123 insertions(+), 31 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> index ec365e1..6e9fd55 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> @@ -6,6 +6,7 @@ Required properties:
>> - ti,x-plate-ohms: X-plate resistance in ohms.
>> 
>> Optional properties:
>> +- generic touch screen properties: see touchscreen binding [2].
>> - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>>   The penirq pin goes to low when the panel is touched.
>>   (see GPIO binding[1] for more details).
>> @@ -13,17 +14,20 @@ Optional properties:
>>   (see interrupt binding[0]).
>> - interrupts: (gpio) interrupt to which the chip is connected
>>   (see interrupt binding[0]).
>> -- ti,max-rt: maximum pressure.
>> -- ti,fuzzx: specifies the absolute input fuzz x value.
>> -  If set, it will permit noise in the data up to +- the value given to the fuzz
>> -  parameter, that is used to filter noise from the event stream.
>> -- ti,fuzzy: specifies the absolute input fuzz y value.
>> -- ti,fuzzz: specifies the absolute input fuzz z value.
>> +- ti,max-rt: maximum pressure resistance above which samples are ignored
>> +  (default: 4095).
>> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
>> +  of pressure (no pressure = 0).
>> +- ti,min-x: minimum value reported by X axis ADC (default 0).
>> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
>> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
>> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
> 
> Seems like these could be common too. They make more sense than giving x 
> and y sizes in pixel units which really should come from the panel.

No. They have a different purpose!

You need to scale values like this:

ti,min-x   ->   0
x          ->   some value between 0 and touchscreen-size-x
ti,max-x   ->   touchscreen-size-x

So the ti, values describe the ADC raw values while the common bindings
describe the size in input-event coordinates.

So we need both specified: raw values and pixel values.

> 
>> - ti,poll-period: how much time to wait (in milliseconds) before reading again the
>> -  values from the tsc2007.
>> +  values from the tsc2007 (default 1).
>> 
>> [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
>> +[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> 
>> Example:
>> 	&i2c1 {
>> @@ -35,6 +39,8 @@ Example:
>> 			interrupts = <0x0 0x8>;
>> 			gpios = <&gpio4 0 0>;
>> 			ti,x-plate-ohms = <180>;
>> +			touchscreen-size-x = <640>;
>> +			touchscreen-size-y = <480>;
>> 		};
>> 
>> 		/* ... */

BR,
Nikolaus

--
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: [Letux-kernel] [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Christ van Willegen]

Hi,

I saw this earlier, but didn't think it important to mention, but:


On Fri, Sep 23, 2016 at 2:41 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 5d0cd51..9a11509 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c

> +                       /* scale ADC values to desired output range */
> +                       sx = (ts->prop.max_x * (tc.x - ts->min_x))
> +                               / (ts->max_x - ts->min_x);
> +                       sy = (ts->prop.max_y * (tc.y - ts->min_y))
> +                               / (ts->max_y - ts->min_y);
> +                       rt = (input->absinfo[ABS_PRESSURE].maximum * rt) /
> +                               ts->max_rt;

If ts->max_rt is zero, or ts->max_x == ts->min_x, or ts->max_y ==
ts->min_y, these yield a division by zero error.

Ofcourse, this is under control of the DT-maintainer(s) of the device
(if I'm not mistaking), but throwing an error on DT parse if (one of)
these condition(s) is met yields better info to the DT-maintainer than
a /0 error...

If you (or others) agree, could you add this to the patch?

Regards,

Christ van Willegen

Re: [Letux-kernel] [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Christ van Willegen]

Hi,

I saw this earlier, but didn't think it important to mention, but:


On Fri, Sep 23, 2016 at 2:41 PM, H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> wrote:

> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 5d0cd51..9a11509 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c

> +                       /* scale ADC values to desired output range */
> +                       sx = (ts->prop.max_x * (tc.x - ts->min_x))
> +                               / (ts->max_x - ts->min_x);
> +                       sy = (ts->prop.max_y * (tc.y - ts->min_y))
> +                               / (ts->max_y - ts->min_y);
> +                       rt = (input->absinfo[ABS_PRESSURE].maximum * rt) /
> +                               ts->max_rt;

If ts->max_rt is zero, or ts->max_x == ts->min_x, or ts->max_y ==
ts->min_y, these yield a division by zero error.

Ofcourse, this is under control of the DT-maintainer(s) of the device
(if I'm not mistaking), but throwing an error on DT parse if (one of)
these condition(s) is met yields better info to the DT-maintainer than
a /0 error...

If you (or others) agree, could you add this to the patch?

Regards,

Christ van Willegen
--
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: [Letux-kernel] [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi Christ,

> Am 30.09.2016 um 18:23 schrieb Christ van Willegen <cvwillegen@gmail.com>:
> 
> Hi,
> 
> I saw this earlier, but didn't think it important to mention, but:
> 
> 
> On Fri, Sep 23, 2016 at 2:41 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 5d0cd51..9a11509 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
> 
>> +                       /* scale ADC values to desired output range */
>> +                       sx = (ts->prop.max_x * (tc.x - ts->min_x))
>> +                               / (ts->max_x - ts->min_x);
>> +                       sy = (ts->prop.max_y * (tc.y - ts->min_y))
>> +                               / (ts->max_y - ts->min_y);
>> +                       rt = (input->absinfo[ABS_PRESSURE].maximum * rt) /
>> +                               ts->max_rt;
> 
> If ts->max_rt is zero, or ts->max_x == ts->min_x, or ts->max_y ==
> ts->min_y, these yield a division by zero error.

Ah, that is a good observation!

> 
> Ofcourse, this is under control of the DT-maintainer(s) of the device
> (if I'm not mistaking)

Yes, the DT should be designed properly.

> , but throwing an error on DT parse if (one of)
> these condition(s) is met yields better info to the DT-maintainer than
> a /0 error...

Yes, but I wonder how likely it is to happen at all.

In an case there is an indicative message (either /0 or an explaining one) in
the boot log which goes away as soon as it has been fixed.

This is already better than in many other cases where things simply fail without
any message...

So I would weight the likelihood of happening vs. the additional code
to check every time.

> 
> If you (or others) agree, could you add this to the patch?

If maintainers request, I can easily add it. But personally I do not see
it as absolutely necessary.

BR and many thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi Christ,

> Am 30.09.2016 um 18:23 schrieb Christ van Willegen =
<cvwillegen@gmail.com>:
>=20
> Hi,
>=20
> I saw this earlier, but didn't think it important to mention, but:
>=20
>=20
> On Fri, Sep 23, 2016 at 2:41 PM, H. Nikolaus Schaller =
<hns@goldelico.com> wrote:
>=20
>> diff --git a/drivers/input/touchscreen/tsc2007.c =
b/drivers/input/touchscreen/tsc2007.c
>> index 5d0cd51..9a11509 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>=20
>> +                       /* scale ADC values to desired output range =
*/
>> +                       sx =3D (ts->prop.max_x * (tc.x - ts->min_x))
>> +                               / (ts->max_x - ts->min_x);
>> +                       sy =3D (ts->prop.max_y * (tc.y - ts->min_y))
>> +                               / (ts->max_y - ts->min_y);
>> +                       rt =3D (input->absinfo[ABS_PRESSURE].maximum =
* rt) /
>> +                               ts->max_rt;
>=20
> If ts->max_rt is zero, or ts->max_x =3D=3D ts->min_x, or ts->max_y =3D=3D=

> ts->min_y, these yield a division by zero error.

Ah, that is a good observation!

>=20
> Ofcourse, this is under control of the DT-maintainer(s) of the device
> (if I'm not mistaking)

Yes, the DT should be designed properly.

> , but throwing an error on DT parse if (one of)
> these condition(s) is met yields better info to the DT-maintainer than
> a /0 error...

Yes, but I wonder how likely it is to happen at all.

In an case there is an indicative message (either /0 or an explaining =
one) in
the boot log which goes away as soon as it has been fixed.

This is already better than in many other cases where things simply fail =
without
any message...

So I would weight the likelihood of happening vs. the additional code
to check every time.

>=20
> If you (or others) agree, could you add this to the patch?

If maintainers request, I can easily add it. But personally I do not see
it as absolutely necessary.

BR and many thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[H. Nikolaus Schaller]

Hi Christ,

> Am 30.09.2016 um 18:23 schrieb Christ van Willegen <cvwillegen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 
> Hi,
> 
> I saw this earlier, but didn't think it important to mention, but:
> 
> 
> On Fri, Sep 23, 2016 at 2:41 PM, H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> wrote:
> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 5d0cd51..9a11509 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
> 
>> +                       /* scale ADC values to desired output range */
>> +                       sx = (ts->prop.max_x * (tc.x - ts->min_x))
>> +                               / (ts->max_x - ts->min_x);
>> +                       sy = (ts->prop.max_y * (tc.y - ts->min_y))
>> +                               / (ts->max_y - ts->min_y);
>> +                       rt = (input->absinfo[ABS_PRESSURE].maximum * rt) /
>> +                               ts->max_rt;
> 
> If ts->max_rt is zero, or ts->max_x == ts->min_x, or ts->max_y ==
> ts->min_y, these yield a division by zero error.

Ah, that is a good observation!

> 
> Ofcourse, this is under control of the DT-maintainer(s) of the device
> (if I'm not mistaking)

Yes, the DT should be designed properly.

> , but throwing an error on DT parse if (one of)
> these condition(s) is met yields better info to the DT-maintainer than
> a /0 error...

Yes, but I wonder how likely it is to happen at all.

In an case there is an indicative message (either /0 or an explaining one) in
the boot log which goes away as soon as it has been fixed.

This is already better than in many other cases where things simply fail without
any message...

So I would weight the likelihood of happening vs. the additional code
to check every time.

> 
> If you (or others) agree, could you add this to the patch?

If maintainers request, I can easily add it. But personally I do not see
it as absolutely necessary.

BR and many thanks,
Nikolaus

[PATCH v3 2/8] drivers:input:tsc2007: send pendown and penup only once like ads7846(+tsc2046) driver does

[H. Nikolaus Schaller]

this should reduce unnecessary input events.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/touchscreen/tsc2007.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 9a11509..e9d5086 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -94,6 +94,7 @@ struct tsc2007 {
 
 	wait_queue_head_t	wait;
 	bool			stopped;
+	bool			pendown;
 
 	int			(*get_pendown_state)(struct device *);
 	void			(*clear_penirq)(void);
@@ -254,9 +255,13 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 
 	dev_dbg(&ts->client->dev, "UP\n");
 
-	input_report_key(input, BTN_TOUCH, 0);
-	input_report_abs(input, ABS_PRESSURE, 0);
-	input_sync(input);
+	if (ts->pendown) {
+		input_report_key(input, BTN_TOUCH, 0);
+		input_report_abs(input, ABS_PRESSURE, 0);
+		input_sync(input);
+
+		ts->pendown = false;
+	}
 
 	if (ts->clear_penirq)
 		ts->clear_penirq();
-- 
2.7.3

[PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

The tsc2007 chip not only has a resistive touch screen controller but
also an external AUX adc imput which can be used for an ambient
light sensor, battery voltage monitoring or any general purpose.

Additionally it can measure the chip temperature.

This extension provides an iio interface for these adc channels
in addition to the raw x, y, z values and the estimated touch
screen resistance. This can be used for debugging or special
applications.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/touchscreen/Kconfig   |   1 +
 drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
 2 files changed, 136 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 2fb1f43..84c28e8 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
 config TOUCHSCREEN_TSC2007
 	tristate "TSC2007 based touchscreens"
 	depends on I2C
+	select IIO
 	help
 	  Say Y here if you have a TSC2007 based touchscreen.
 
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index e9d5086..559cbc6 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,9 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
@@ -61,6 +64,16 @@
 #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+	.datasheet_name = _name, \
+	.type = _type, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+			BIT(_chan_info), \
+	.indexed = 1, \
+	.channel = _chan, \
+}
+
 struct ts_event {
 	u16	x;
 	u16	y;
@@ -69,9 +82,11 @@ struct ts_event {
 
 struct tsc2007 {
 	struct input_dev	*input;
+	struct iio_dev		*indio;
 	char			phys[32];
 
 	struct i2c_client	*client;
+	struct mutex		mlock;
 
 	u16			model;
 	u16			x_plate_ohms;
@@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
 		/* pen is down, continue with the measurement */
+
+		mutex_lock(&ts->mlock);
 		tsc2007_read_values(ts, &tc);
+		mutex_unlock(&ts->mlock);
 
 		rt = tsc2007_calculate_resistance(ts, &tc);
 
@@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
 	tsc2007_stop(ts);
 }
 
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+	struct  tsc2007 *tsc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+		return -EINVAL;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	mutex_lock(&tsc->mlock);
+
+	switch (chan->channel) {
+	case 0:
+		*val = tsc2007_xfer(tsc, READ_X);
+		break;
+	case 1:
+		*val = tsc2007_xfer(tsc, READ_Y);
+		break;
+	case 2:
+		*val = tsc2007_xfer(tsc, READ_Z1);
+		break;
+	case 3:
+		*val = tsc2007_xfer(tsc, READ_Z2);
+		break;
+	case 4:
+		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
+		break;
+	case 5: {
+		struct ts_event tc;
+
+		tc.x = tsc2007_xfer(tsc, READ_X);
+		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
+		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
+		*val = tsc2007_calculate_resistance(tsc, &tc);
+		break;
+	}
+	case 6:
+		*val = tsc2007_is_pen_down(tsc);
+		break;
+	case 7:
+		*val = tsc2007_xfer(tsc,
+				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
+		break;
+	case 8:
+		*val = tsc2007_xfer(tsc,
+				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
+		break;
+	}
+
+	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
+	tsc2007_xfer(tsc, PWRDOWN);
+
+	mutex_unlock(&tsc->mlock);
+
+	ret = IIO_VAL_INT;
+
+	return ret;
+}
+
+static const struct iio_info tsc2007_iio_info = {
+	.read_raw = tsc2007_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
 #ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
@@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct tsc2007 *ts;
 	struct input_dev *input_dev;
+	struct iio_dev *indio_dev;
 	int err;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -EIO;
 
-	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
-	if (!ts)
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
+	if (!indio_dev) {
+		dev_err(&client->dev, "iio_device_alloc failed\n");
 		return -ENOMEM;
+	}
+
+	ts = iio_priv(indio_dev);
 
 	input_dev = devm_input_allocate_device(&client->dev);
 	if (!input_dev)
@@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ts);
 
+	indio_dev->name = "tsc2007";
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &tsc2007_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = tsc2007_iio_channel;
+	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
+
+	err = iio_device_register(indio_dev);
+	if (err < 0) {
+		dev_err(&client->dev, "iio_device_register() failed: %d\n",
+			err);
+		return err;
+	}
+
 	ts->client = client;
 	ts->irq = client->irq;
 	ts->input = input_dev;
+	ts->indio = indio_dev;
 	init_waitqueue_head(&ts->wait);
+	mutex_init(&ts->mlock);
 
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));
@@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
 	return 0;
 }
 
+static int tsc2007_remove(struct i2c_client *client)
+{
+	struct tsc2007 *ts = i2c_get_clientdata(client);
+	struct input_dev *input_dev = ts->input;
+	struct iio_dev *indio_dev = ts->indio;
+
+	input_unregister_device(input_dev);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
 static const struct i2c_device_id tsc2007_idtable[] = {
 	{ "tsc2007", 0 },
 	{ }
@@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
 	},
 	.id_table	= tsc2007_idtable,
 	.probe		= tsc2007_probe,
+	.remove		= tsc2007_remove,
 };
 
 module_i2c_driver(tsc2007_driver);
-- 
2.7.3

[PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

The tsc2007 chip not only has a resistive touch screen controller but
also an external AUX adc imput which can be used for an ambient
light sensor, battery voltage monitoring or any general purpose.

Additionally it can measure the chip temperature.

This extension provides an iio interface for these adc channels
in addition to the raw x, y, z values and the estimated touch
screen resistance. This can be used for debugging or special
applications.

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 drivers/input/touchscreen/Kconfig   |   1 +
 drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
 2 files changed, 136 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 2fb1f43..84c28e8 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
 config TOUCHSCREEN_TSC2007
 	tristate "TSC2007 based touchscreens"
 	depends on I2C
+	select IIO
 	help
 	  Say Y here if you have a TSC2007 based touchscreen.
 
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index e9d5086..559cbc6 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,9 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
@@ -61,6 +64,16 @@
 #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+	.datasheet_name = _name, \
+	.type = _type, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+			BIT(_chan_info), \
+	.indexed = 1, \
+	.channel = _chan, \
+}
+
 struct ts_event {
 	u16	x;
 	u16	y;
@@ -69,9 +82,11 @@ struct ts_event {
 
 struct tsc2007 {
 	struct input_dev	*input;
+	struct iio_dev		*indio;
 	char			phys[32];
 
 	struct i2c_client	*client;
+	struct mutex		mlock;
 
 	u16			model;
 	u16			x_plate_ohms;
@@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
 		/* pen is down, continue with the measurement */
+
+		mutex_lock(&ts->mlock);
 		tsc2007_read_values(ts, &tc);
+		mutex_unlock(&ts->mlock);
 
 		rt = tsc2007_calculate_resistance(ts, &tc);
 
@@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
 	tsc2007_stop(ts);
 }
 
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+	struct  tsc2007 *tsc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+		return -EINVAL;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	mutex_lock(&tsc->mlock);
+
+	switch (chan->channel) {
+	case 0:
+		*val = tsc2007_xfer(tsc, READ_X);
+		break;
+	case 1:
+		*val = tsc2007_xfer(tsc, READ_Y);
+		break;
+	case 2:
+		*val = tsc2007_xfer(tsc, READ_Z1);
+		break;
+	case 3:
+		*val = tsc2007_xfer(tsc, READ_Z2);
+		break;
+	case 4:
+		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
+		break;
+	case 5: {
+		struct ts_event tc;
+
+		tc.x = tsc2007_xfer(tsc, READ_X);
+		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
+		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
+		*val = tsc2007_calculate_resistance(tsc, &tc);
+		break;
+	}
+	case 6:
+		*val = tsc2007_is_pen_down(tsc);
+		break;
+	case 7:
+		*val = tsc2007_xfer(tsc,
+				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
+		break;
+	case 8:
+		*val = tsc2007_xfer(tsc,
+				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
+		break;
+	}
+
+	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
+	tsc2007_xfer(tsc, PWRDOWN);
+
+	mutex_unlock(&tsc->mlock);
+
+	ret = IIO_VAL_INT;
+
+	return ret;
+}
+
+static const struct iio_info tsc2007_iio_info = {
+	.read_raw = tsc2007_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
 #ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
@@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct tsc2007 *ts;
 	struct input_dev *input_dev;
+	struct iio_dev *indio_dev;
 	int err;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -EIO;
 
-	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
-	if (!ts)
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
+	if (!indio_dev) {
+		dev_err(&client->dev, "iio_device_alloc failed\n");
 		return -ENOMEM;
+	}
+
+	ts = iio_priv(indio_dev);
 
 	input_dev = devm_input_allocate_device(&client->dev);
 	if (!input_dev)
@@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ts);
 
+	indio_dev->name = "tsc2007";
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &tsc2007_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = tsc2007_iio_channel;
+	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
+
+	err = iio_device_register(indio_dev);
+	if (err < 0) {
+		dev_err(&client->dev, "iio_device_register() failed: %d\n",
+			err);
+		return err;
+	}
+
 	ts->client = client;
 	ts->irq = client->irq;
 	ts->input = input_dev;
+	ts->indio = indio_dev;
 	init_waitqueue_head(&ts->wait);
+	mutex_init(&ts->mlock);
 
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));
@@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
 	return 0;
 }
 
+static int tsc2007_remove(struct i2c_client *client)
+{
+	struct tsc2007 *ts = i2c_get_clientdata(client);
+	struct input_dev *input_dev = ts->input;
+	struct iio_dev *indio_dev = ts->indio;
+
+	input_unregister_device(input_dev);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
 static const struct i2c_device_id tsc2007_idtable[] = {
 	{ "tsc2007", 0 },
 	{ }
@@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
 	},
 	.id_table	= tsc2007_idtable,
 	.probe		= tsc2007_probe,
+	.remove		= tsc2007_remove,
 };
 
 module_i2c_driver(tsc2007_driver);
-- 
2.7.3

--
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 v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Jonathan Cameron]

On 23/09/16 13:41, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
> 
> Additionally it can measure the chip temperature.
> 
> This extension provides an iio interface for these adc channels
> in addition to the raw x, y, z values and the estimated touch
> screen resistance. This can be used for debugging or special
> applications.
I'm unconvinced that exposing the touch related channels is terribly
useful.  Fair enough on the aux channel and temperature though.

Otherwise a few minor bits and bobs.

Jonathan
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/input/touchscreen/Kconfig   |   1 +
>  drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 2fb1f43..84c28e8 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>  config TOUCHSCREEN_TSC2007
>  	tristate "TSC2007 based touchscreens"
>  	depends on I2C
> +	select IIO
This is pulling quite a bit of core IIO code in for a feature a lot
of users of this chip won't care about...
>  	help
>  	  Say Y here if you have a TSC2007 based touchscreen.
>  
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index e9d5086..559cbc6 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,9 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
Why machine.h? (or driver.h for that reason).

> +#include <linux/iio/driver.h>
>  
>  #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>  #define TSC2007_MEASURE_AUX		(0x2 << 4)
> @@ -61,6 +64,16 @@
>  #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>  #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>  
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> +	.datasheet_name = _name, \
> +	.type = _type, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +			BIT(_chan_info), \
> +	.indexed = 1, \
> +	.channel = _chan, \
> +}
> +
>  struct ts_event {
>  	u16	x;
>  	u16	y;
> @@ -69,9 +82,11 @@ struct ts_event {
>  
>  struct tsc2007 {
>  	struct input_dev	*input;
> +	struct iio_dev		*indio;
>  	char			phys[32];
>  
>  	struct i2c_client	*client;
> +	struct mutex		mlock;
>  
>  	u16			model;
>  	u16			x_plate_ohms;
> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>  		/* pen is down, continue with the measurement */
> +
> +		mutex_lock(&ts->mlock);
>  		tsc2007_read_values(ts, &tc);
> +		mutex_unlock(&ts->mlock);
>  
>  		rt = tsc2007_calculate_resistance(ts, &tc);
>  
> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>  	tsc2007_stop(ts);
>  }
>  
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
This one needs some explanation.  What is it measuring?

> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
At the moment this is defined as barometric pressure.  I suppose there
is no reason it can't extend to touch pressure though, even if it's a boolean
like this...

> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
I would be tempted to only expose those channels that have a meaning
outside of touch screen usage (e.g. adc, temp0 and temp1).  
> +};
> +
> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> +{
> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
> +		return -EINVAL;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	mutex_lock(&tsc->mlock);
> +
> +	switch (chan->channel) {
> +	case 0:
> +		*val = tsc2007_xfer(tsc, READ_X);
> +		break;
> +	case 1:
> +		*val = tsc2007_xfer(tsc, READ_Y);
> +		break;
> +	case 2:
> +		*val = tsc2007_xfer(tsc, READ_Z1);
> +		break;
> +	case 3:
> +		*val = tsc2007_xfer(tsc, READ_Z2);
> +		break;
> +	case 4:
> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
> +		break;
> +	case 5: {
> +		struct ts_event tc;
> +
> +		tc.x = tsc2007_xfer(tsc, READ_X);
> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
> +		*val = tsc2007_calculate_resistance(tsc, &tc);
> +		break;
> +	}
> +	case 6:
> +		*val = tsc2007_is_pen_down(tsc);
> +		break;
> +	case 7:
> +		*val = tsc2007_xfer(tsc,
> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
> +		break;
> +	case 8:
> +		*val = tsc2007_xfer(tsc,
> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
> +		break;
> +	}
> +
> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
> +	tsc2007_xfer(tsc, PWRDOWN);
> +
> +	mutex_unlock(&tsc->mlock);
> +
> +	ret = IIO_VAL_INT;
> +
> +	return ret;
> +}
> +
> +static const struct iio_info tsc2007_iio_info = {
> +	.read_raw = tsc2007_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
>  #ifdef CONFIG_OF
>  static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  {
> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>  	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct tsc2007 *ts;
>  	struct input_dev *input_dev;
> +	struct iio_dev *indio_dev;
>  	int err;
>  
>  	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>  		return -EIO;
>  
> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> -	if (!ts)
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
> +	if (!indio_dev) {
> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>  		return -ENOMEM;
> +	}
> +
> +	ts = iio_priv(indio_dev);
>  
>  	input_dev = devm_input_allocate_device(&client->dev);
>  	if (!input_dev)
> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, ts);
>  
> +	indio_dev->name = "tsc2007";
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &tsc2007_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = tsc2007_iio_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
> +
> +	err = iio_device_register(indio_dev);
> +	if (err < 0) {
> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
> +			err);
> +		return err;
> +	}
> +
>  	ts->client = client;
>  	ts->irq = client->irq;
>  	ts->input = input_dev;
> +	ts->indio = indio_dev;
>  	init_waitqueue_head(&ts->wait);
> +	mutex_init(&ts->mlock);
>  
>  	snprintf(ts->phys, sizeof(ts->phys),
>  		 "%s/input0", dev_name(&client->dev));
> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int tsc2007_remove(struct i2c_client *client)
> +{
> +	struct tsc2007 *ts = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = ts->input;
> +	struct iio_dev *indio_dev = ts->indio;
> +
> +	input_unregister_device(input_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
>  static const struct i2c_device_id tsc2007_idtable[] = {
>  	{ "tsc2007", 0 },
>  	{ }
> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>  	},
>  	.id_table	= tsc2007_idtable,
>  	.probe		= tsc2007_probe,
> +	.remove		= tsc2007_remove,
>  };
>  
>  module_i2c_driver(tsc2007_driver);
> 

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Jonathan Cameron]

On 23/09/16 13:41, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
> 
> Additionally it can measure the chip temperature.
> 
> This extension provides an iio interface for these adc channels
> in addition to the raw x, y, z values and the estimated touch
> screen resistance. This can be used for debugging or special
> applications.
I'm unconvinced that exposing the touch related channels is terribly
useful.  Fair enough on the aux channel and temperature though.

Otherwise a few minor bits and bobs.

Jonathan
> 
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
>  drivers/input/touchscreen/Kconfig   |   1 +
>  drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 2fb1f43..84c28e8 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>  config TOUCHSCREEN_TSC2007
>  	tristate "TSC2007 based touchscreens"
>  	depends on I2C
> +	select IIO
This is pulling quite a bit of core IIO code in for a feature a lot
of users of this chip won't care about...
>  	help
>  	  Say Y here if you have a TSC2007 based touchscreen.
>  
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index e9d5086..559cbc6 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,9 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
Why machine.h? (or driver.h for that reason).

> +#include <linux/iio/driver.h>
>  
>  #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>  #define TSC2007_MEASURE_AUX		(0x2 << 4)
> @@ -61,6 +64,16 @@
>  #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>  #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>  
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> +	.datasheet_name = _name, \
> +	.type = _type, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +			BIT(_chan_info), \
> +	.indexed = 1, \
> +	.channel = _chan, \
> +}
> +
>  struct ts_event {
>  	u16	x;
>  	u16	y;
> @@ -69,9 +82,11 @@ struct ts_event {
>  
>  struct tsc2007 {
>  	struct input_dev	*input;
> +	struct iio_dev		*indio;
>  	char			phys[32];
>  
>  	struct i2c_client	*client;
> +	struct mutex		mlock;
>  
>  	u16			model;
>  	u16			x_plate_ohms;
> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>  		/* pen is down, continue with the measurement */
> +
> +		mutex_lock(&ts->mlock);
>  		tsc2007_read_values(ts, &tc);
> +		mutex_unlock(&ts->mlock);
>  
>  		rt = tsc2007_calculate_resistance(ts, &tc);
>  
> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>  	tsc2007_stop(ts);
>  }
>  
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
This one needs some explanation.  What is it measuring?

> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
At the moment this is defined as barometric pressure.  I suppose there
is no reason it can't extend to touch pressure though, even if it's a boolean
like this...

> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
I would be tempted to only expose those channels that have a meaning
outside of touch screen usage (e.g. adc, temp0 and temp1).  
> +};
> +
> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> +{
> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
> +		return -EINVAL;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	mutex_lock(&tsc->mlock);
> +
> +	switch (chan->channel) {
> +	case 0:
> +		*val = tsc2007_xfer(tsc, READ_X);
> +		break;
> +	case 1:
> +		*val = tsc2007_xfer(tsc, READ_Y);
> +		break;
> +	case 2:
> +		*val = tsc2007_xfer(tsc, READ_Z1);
> +		break;
> +	case 3:
> +		*val = tsc2007_xfer(tsc, READ_Z2);
> +		break;
> +	case 4:
> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
> +		break;
> +	case 5: {
> +		struct ts_event tc;
> +
> +		tc.x = tsc2007_xfer(tsc, READ_X);
> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
> +		*val = tsc2007_calculate_resistance(tsc, &tc);
> +		break;
> +	}
> +	case 6:
> +		*val = tsc2007_is_pen_down(tsc);
> +		break;
> +	case 7:
> +		*val = tsc2007_xfer(tsc,
> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
> +		break;
> +	case 8:
> +		*val = tsc2007_xfer(tsc,
> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
> +		break;
> +	}
> +
> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
> +	tsc2007_xfer(tsc, PWRDOWN);
> +
> +	mutex_unlock(&tsc->mlock);
> +
> +	ret = IIO_VAL_INT;
> +
> +	return ret;
> +}
> +
> +static const struct iio_info tsc2007_iio_info = {
> +	.read_raw = tsc2007_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
>  #ifdef CONFIG_OF
>  static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  {
> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>  	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct tsc2007 *ts;
>  	struct input_dev *input_dev;
> +	struct iio_dev *indio_dev;
>  	int err;
>  
>  	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>  		return -EIO;
>  
> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> -	if (!ts)
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
> +	if (!indio_dev) {
> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>  		return -ENOMEM;
> +	}
> +
> +	ts = iio_priv(indio_dev);
>  
>  	input_dev = devm_input_allocate_device(&client->dev);
>  	if (!input_dev)
> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, ts);
>  
> +	indio_dev->name = "tsc2007";
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &tsc2007_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = tsc2007_iio_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
> +
> +	err = iio_device_register(indio_dev);
> +	if (err < 0) {
> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
> +			err);
> +		return err;
> +	}
> +
>  	ts->client = client;
>  	ts->irq = client->irq;
>  	ts->input = input_dev;
> +	ts->indio = indio_dev;
>  	init_waitqueue_head(&ts->wait);
> +	mutex_init(&ts->mlock);
>  
>  	snprintf(ts->phys, sizeof(ts->phys),
>  		 "%s/input0", dev_name(&client->dev));
> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int tsc2007_remove(struct i2c_client *client)
> +{
> +	struct tsc2007 *ts = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = ts->input;
> +	struct iio_dev *indio_dev = ts->indio;
> +
> +	input_unregister_device(input_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
>  static const struct i2c_device_id tsc2007_idtable[] = {
>  	{ "tsc2007", 0 },
>  	{ }
> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>  	},
>  	.id_table	= tsc2007_idtable,
>  	.probe		= tsc2007_probe,
> +	.remove		= tsc2007_remove,
>  };
>  
>  module_i2c_driver(tsc2007_driver);
> 

--
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 v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>> The tsc2007 chip not only has a resistive touch screen controller but
>> also an external AUX adc imput which can be used for an ambient
>> light sensor, battery voltage monitoring or any general purpose.
>> 
>> Additionally it can measure the chip temperature.
>> 
>> This extension provides an iio interface for these adc channels
>> in addition to the raw x, y, z values and the estimated touch
>> screen resistance. This can be used for debugging or special
>> applications.
> I'm unconvinced that exposing the touch related channels is terribly
> useful.

Mostly for debugging of if someone wants to do a (better) touch detection
in user space.

>  Fair enough on the aux channel and temperature though.
> 
> Otherwise a few minor bits and bobs.
> 
> Jonathan
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/input/touchscreen/Kconfig   |   1 +
>> drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>> 2 files changed, 136 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 2fb1f43..84c28e8 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>> config TOUCHSCREEN_TSC2007
>> 	tristate "TSC2007 based touchscreens"
>> 	depends on I2C
>> +	select IIO
> This is pulling quite a bit of core IIO code in for a feature a lot
> of users of this chip won't care about...

Ah, yes.

We didn't notice because we use many other IIO devices on the same board (GTA04).

So I will add some #ifdef CONFIG_IIO where necessary.


>> 	help
>> 	  Say Y here if you have a TSC2007 based touchscreen.
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index e9d5086..559cbc6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,9 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/input/touchscreen.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
> Why machine.h? (or driver.h for that reason).

Good question. Header files sometimes get in during
development and are forgotten to take out... Or had
been necessary when development started a while ago.
Indeed I don't know why they are there.

> 
>> +#include <linux/iio/driver.h>

I have checked and we really neither need machine.h nor driver.h.

>> 
>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>> @@ -61,6 +64,16 @@
>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>> #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>> 
>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>> +{ \
>> +	.datasheet_name = _name, \
>> +	.type = _type, \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>> +			BIT(_chan_info), \
>> +	.indexed = 1, \
>> +	.channel = _chan, \
>> +}
>> +
>> struct ts_event {
>> 	u16	x;
>> 	u16	y;
>> @@ -69,9 +82,11 @@ struct ts_event {
>> 
>> struct tsc2007 {
>> 	struct input_dev	*input;
>> +	struct iio_dev		*indio;
>> 	char			phys[32];
>> 
>> 	struct i2c_client	*client;
>> +	struct mutex		mlock;
>> 
>> 	u16			model;
>> 	u16			x_plate_ohms;
>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>> 
>> 		/* pen is down, continue with the measurement */
>> +
>> +		mutex_lock(&ts->mlock);
>> 		tsc2007_read_values(ts, &tc);
>> +		mutex_unlock(&ts->mlock);
>> 
>> 		rt = tsc2007_calculate_resistance(ts, &tc);
>> 
>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>> 	tsc2007_stop(ts);
>> }
>> 
>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
> This one needs some explanation.  What is it measuring?

Well, it is reporting the raw conductivity of the touch under certain
conditions. But basically it is also a voltage measured by the ADC.

The real pressure value reported to user space is calculated
using this and the z1 and z2 values.

If not pressed, conductivity is 0 and if lightly touched it jumps
to some base value. When more pressed, conductivity increases
while resistance goes down to some 300 Ohms or so.

But I have not found an IIO_OHM or IIO_MHO constant. Which would also
not be the best description because it is not scaled to real ohms.

> 
>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
> At the moment this is defined as barometric pressure.

Ok, I see.

>  I suppose there
> is no reason it can't extend to touch pressure though, even if it's a boolean
> like this...

So let's keep it this way?

> 
>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
> I would be tempted to only expose those channels that have a meaning
> outside of touch screen usage (e.g. adc, temp0 and temp1).  

Well, for debugging and checking for non-responding or defective touch
it is very handy to have them in /sys/bus/iio for direct inspection.
For example we run a device test tool reading such values.

You might argue that we should run our own fork for this purpose to provide
a patched driver. But we want people to use the kernel from kernel.org
and still be able to do such debugging without asking them to replace the
kernel first.

>> +};
>> +
>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>> +{
>> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
>> +	int adc_chan = chan->channel;
>> +	int ret = 0;
>> +
>> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>> +		return -EINVAL;
>> +
>> +	if (mask != IIO_CHAN_INFO_RAW)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&tsc->mlock);
>> +
>> +	switch (chan->channel) {
>> +	case 0:
>> +		*val = tsc2007_xfer(tsc, READ_X);
>> +		break;
>> +	case 1:
>> +		*val = tsc2007_xfer(tsc, READ_Y);
>> +		break;
>> +	case 2:
>> +		*val = tsc2007_xfer(tsc, READ_Z1);
>> +		break;
>> +	case 3:
>> +		*val = tsc2007_xfer(tsc, READ_Z2);
>> +		break;
>> +	case 4:
>> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>> +		break;
>> +	case 5: {
>> +		struct ts_event tc;
>> +
>> +		tc.x = tsc2007_xfer(tsc, READ_X);
>> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>> +		*val = tsc2007_calculate_resistance(tsc, &tc);
>> +		break;
>> +	}
>> +	case 6:
>> +		*val = tsc2007_is_pen_down(tsc);
>> +		break;
>> +	case 7:
>> +		*val = tsc2007_xfer(tsc,
>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>> +		break;
>> +	case 8:
>> +		*val = tsc2007_xfer(tsc,
>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>> +		break;
>> +	}
>> +
>> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
>> +	tsc2007_xfer(tsc, PWRDOWN);
>> +
>> +	mutex_unlock(&tsc->mlock);
>> +
>> +	ret = IIO_VAL_INT;
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct iio_info tsc2007_iio_info = {
>> +	.read_raw = tsc2007_read_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> #ifdef CONFIG_OF
>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>> {
>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>> 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>> 	struct tsc2007 *ts;
>> 	struct input_dev *input_dev;
>> +	struct iio_dev *indio_dev;
>> 	int err;
>> 
>> 	if (!i2c_check_functionality(client->adapter,
>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>> 		return -EIO;
>> 
>> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>> -	if (!ts)
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> +	if (!indio_dev) {
>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>> 		return -ENOMEM;
>> +	}
>> +
>> +	ts = iio_priv(indio_dev);
>> 
>> 	input_dev = devm_input_allocate_device(&client->dev);
>> 	if (!input_dev)
>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>> 
>> 	i2c_set_clientdata(client, ts);
>> 
>> +	indio_dev->name = "tsc2007";
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->info = &tsc2007_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = tsc2007_iio_channel;
>> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>> +
>> +	err = iio_device_register(indio_dev);
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
>> +			err);
>> +		return err;
>> +	}
>> +
>> 	ts->client = client;
>> 	ts->irq = client->irq;
>> 	ts->input = input_dev;
>> +	ts->indio = indio_dev;
>> 	init_waitqueue_head(&ts->wait);
>> +	mutex_init(&ts->mlock);
>> 
>> 	snprintf(ts->phys, sizeof(ts->phys),
>> 		 "%s/input0", dev_name(&client->dev));
>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>> 	return 0;
>> }
>> 
>> +static int tsc2007_remove(struct i2c_client *client)
>> +{
>> +	struct tsc2007 *ts = i2c_get_clientdata(client);
>> +	struct input_dev *input_dev = ts->input;
>> +	struct iio_dev *indio_dev = ts->indio;
>> +
>> +	input_unregister_device(input_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> static const struct i2c_device_id tsc2007_idtable[] = {
>> 	{ "tsc2007", 0 },
>> 	{ }
>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>> 	},
>> 	.id_table	= tsc2007_idtable,
>> 	.probe		= tsc2007_probe,
>> +	.remove		= tsc2007_remove,
>> };
>> 
>> module_i2c_driver(tsc2007_driver);
>> 
> 

BR and thanks,
Nikolaus

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>=20
> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>> The tsc2007 chip not only has a resistive touch screen controller but
>> also an external AUX adc imput which can be used for an ambient
>> light sensor, battery voltage monitoring or any general purpose.
>>=20
>> Additionally it can measure the chip temperature.
>>=20
>> This extension provides an iio interface for these adc channels
>> in addition to the raw x, y, z values and the estimated touch
>> screen resistance. This can be used for debugging or special
>> applications.
> I'm unconvinced that exposing the touch related channels is terribly
> useful.

Mostly for debugging of if someone wants to do a (better) touch =
detection
in user space.

>  Fair enough on the aux channel and temperature though.
>=20
> Otherwise a few minor bits and bobs.
>=20
> Jonathan
>>=20
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/input/touchscreen/Kconfig   |   1 +
>> drivers/input/touchscreen/tsc2007.c | 137 =
+++++++++++++++++++++++++++++++++++-
>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/drivers/input/touchscreen/Kconfig =
b/drivers/input/touchscreen/Kconfig
>> index 2fb1f43..84c28e8 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>> config TOUCHSCREEN_TSC2007
>> 	tristate "TSC2007 based touchscreens"
>> 	depends on I2C
>> +	select IIO
> This is pulling quite a bit of core IIO code in for a feature a lot
> of users of this chip won't care about...

Ah, yes.

We didn't notice because we use many other IIO devices on the same board =
(GTA04).

So I will add some #ifdef CONFIG_IIO where necessary.


>> 	help
>> 	  Say Y here if you have a TSC2007 based touchscreen.
>>=20
>> diff --git a/drivers/input/touchscreen/tsc2007.c =
b/drivers/input/touchscreen/tsc2007.c
>> index e9d5086..559cbc6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,9 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/input/touchscreen.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
> Why machine.h? (or driver.h for that reason).

Good question. Header files sometimes get in during
development and are forgotten to take out... Or had
been necessary when development started a while ago.
Indeed I don't know why they are there.

>=20
>> +#include <linux/iio/driver.h>

I have checked and we really neither need machine.h nor driver.h.

>>=20
>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>> @@ -61,6 +64,16 @@
>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>> #define PWRDOWN		(TSC2007_12BIT | =
TSC2007_POWER_OFF_IRQ_EN)
>>=20
>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>> +{ \
>> +	.datasheet_name =3D _name, \
>> +	.type =3D _type, \
>> +	.info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW) |	\
>> +			BIT(_chan_info), \
>> +	.indexed =3D 1, \
>> +	.channel =3D _chan, \
>> +}
>> +
>> struct ts_event {
>> 	u16	x;
>> 	u16	y;
>> @@ -69,9 +82,11 @@ struct ts_event {
>>=20
>> struct tsc2007 {
>> 	struct input_dev	*input;
>> +	struct iio_dev		*indio;
>> 	char			phys[32];
>>=20
>> 	struct i2c_client	*client;
>> +	struct mutex		mlock;
>>=20
>> 	u16			model;
>> 	u16			x_plate_ohms;
>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, =
void *handle)
>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>=20
>> 		/* pen is down, continue with the measurement */
>> +
>> +		mutex_lock(&ts->mlock);
>> 		tsc2007_read_values(ts, &tc);
>> +		mutex_unlock(&ts->mlock);
>>=20
>> 		rt =3D tsc2007_calculate_resistance(ts, &tc);
>>=20
>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev =
*input_dev)
>> 	tsc2007_stop(ts);
>> }
>>=20
>> +static const struct iio_chan_spec tsc2007_iio_channel[] =3D {
>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* =
Ohms? */
> This one needs some explanation.  What is it measuring?

Well, it is reporting the raw conductivity of the touch under certain
conditions. But basically it is also a voltage measured by the ADC.

The real pressure value reported to user space is calculated
using this and the z1 and z2 values.

If not pressed, conductivity is 0 and if lightly touched it jumps
to some base value. When more pressed, conductivity increases
while resistance goes down to some 300 Ohms or so.

But I have not found an IIO_OHM or IIO_MHO constant. Which would also
not be the best description because it is not scaled to real ohms.

>=20
>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
> At the moment this is defined as barometric pressure.

Ok, I see.

>  I suppose there
> is no reason it can't extend to touch pressure though, even if it's a =
boolean
> like this...

So let's keep it this way?

>=20
>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
> I would be tempted to only expose those channels that have a meaning
> outside of touch screen usage (e.g. adc, temp0 and temp1). =20

Well, for debugging and checking for non-responding or defective touch
it is very handy to have them in /sys/bus/iio for direct inspection.
For example we run a device test tool reading such values.

You might argue that we should run our own fork for this purpose to =
provide
a patched driver. But we want people to use the kernel from kernel.org
and still be able to do such debugging without asking them to replace =
the
kernel first.

>> +};
>> +
>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>> +	struct iio_chan_spec const *chan, int *val, int *val2, long =
mask)
>> +{
>> +	struct  tsc2007 *tsc =3D iio_priv(indio_dev);
>> +	int adc_chan =3D chan->channel;
>> +	int ret =3D 0;
>> +
>> +	if (adc_chan >=3D ARRAY_SIZE(tsc2007_iio_channel))
>> +		return -EINVAL;
>> +
>> +	if (mask !=3D IIO_CHAN_INFO_RAW)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&tsc->mlock);
>> +
>> +	switch (chan->channel) {
>> +	case 0:
>> +		*val =3D tsc2007_xfer(tsc, READ_X);
>> +		break;
>> +	case 1:
>> +		*val =3D tsc2007_xfer(tsc, READ_Y);
>> +		break;
>> +	case 2:
>> +		*val =3D tsc2007_xfer(tsc, READ_Z1);
>> +		break;
>> +	case 3:
>> +		*val =3D tsc2007_xfer(tsc, READ_Z2);
>> +		break;
>> +	case 4:
>> +		*val =3D tsc2007_xfer(tsc, (ADC_ON_12BIT | =
TSC2007_MEASURE_AUX));
>> +		break;
>> +	case 5: {
>> +		struct ts_event tc;
>> +
>> +		tc.x =3D tsc2007_xfer(tsc, READ_X);
>> +		tc.z1 =3D tsc2007_xfer(tsc, READ_Z1);
>> +		tc.z2 =3D tsc2007_xfer(tsc, READ_Z2);
>> +		*val =3D tsc2007_calculate_resistance(tsc, &tc);
>> +		break;
>> +	}
>> +	case 6:
>> +		*val =3D tsc2007_is_pen_down(tsc);
>> +		break;
>> +	case 7:
>> +		*val =3D tsc2007_xfer(tsc,
>> +				    (ADC_ON_12BIT | =
TSC2007_MEASURE_TEMP0));
>> +		break;
>> +	case 8:
>> +		*val =3D tsc2007_xfer(tsc,
>> +				    (ADC_ON_12BIT | =
TSC2007_MEASURE_TEMP1));
>> +		break;
>> +	}
>> +
>> +	/* Prepare for next touch reading - power down ADC, enable =
PENIRQ */
>> +	tsc2007_xfer(tsc, PWRDOWN);
>> +
>> +	mutex_unlock(&tsc->mlock);
>> +
>> +	ret =3D IIO_VAL_INT;
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct iio_info tsc2007_iio_info =3D {
>> +	.read_raw =3D tsc2007_read_raw,
>> +	.driver_module =3D THIS_MODULE,
>> +};
>> +
>> #ifdef CONFIG_OF
>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>> {
>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client =
*client,
>> 	const struct tsc2007_platform_data *pdata =3D =
dev_get_platdata(&client->dev);
>> 	struct tsc2007 *ts;
>> 	struct input_dev *input_dev;
>> +	struct iio_dev *indio_dev;
>> 	int err;
>>=20
>> 	if (!i2c_check_functionality(client->adapter,
>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>> 		return -EIO;
>>=20
>> -	ts =3D devm_kzalloc(&client->dev, sizeof(struct tsc2007), =
GFP_KERNEL);
>> -	if (!ts)
>> +	indio_dev =3D devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> +	if (!indio_dev) {
>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>> 		return -ENOMEM;
>> +	}
>> +
>> +	ts =3D iio_priv(indio_dev);
>>=20
>> 	input_dev =3D devm_input_allocate_device(&client->dev);
>> 	if (!input_dev)
>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client =
*client,
>>=20
>> 	i2c_set_clientdata(client, ts);
>>=20
>> +	indio_dev->name =3D "tsc2007";
>> +	indio_dev->dev.parent =3D &client->dev;
>> +	indio_dev->info =3D &tsc2007_iio_info;
>> +	indio_dev->modes =3D INDIO_DIRECT_MODE;
>> +	indio_dev->channels =3D tsc2007_iio_channel;
>> +	indio_dev->num_channels =3D ARRAY_SIZE(tsc2007_iio_channel);
>> +
>> +	err =3D iio_device_register(indio_dev);
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "iio_device_register() failed: =
%d\n",
>> +			err);
>> +		return err;
>> +	}
>> +
>> 	ts->client =3D client;
>> 	ts->irq =3D client->irq;
>> 	ts->input =3D input_dev;
>> +	ts->indio =3D indio_dev;
>> 	init_waitqueue_head(&ts->wait);
>> +	mutex_init(&ts->mlock);
>>=20
>> 	snprintf(ts->phys, sizeof(ts->phys),
>> 		 "%s/input0", dev_name(&client->dev));
>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client =
*client,
>> 	return 0;
>> }
>>=20
>> +static int tsc2007_remove(struct i2c_client *client)
>> +{
>> +	struct tsc2007 *ts =3D i2c_get_clientdata(client);
>> +	struct input_dev *input_dev =3D ts->input;
>> +	struct iio_dev *indio_dev =3D ts->indio;
>> +
>> +	input_unregister_device(input_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> static const struct i2c_device_id tsc2007_idtable[] =3D {
>> 	{ "tsc2007", 0 },
>> 	{ }
>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver =3D {
>> 	},
>> 	.id_table	=3D tsc2007_idtable,
>> 	.probe		=3D tsc2007_probe,
>> +	.remove		=3D tsc2007_remove,
>> };
>>=20
>> module_i2c_driver(tsc2007_driver);
>>=20
>=20

BR and thanks,
Nikolaus

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Jonathan Cameron]

On 24/09/16 18:07, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels
>>> in addition to the raw x, y, z values and the estimated touch
>>> screen resistance. This can be used for debugging or special
>>> applications.
>> I'm unconvinced that exposing the touch related channels is terribly
>> useful.
> 
> Mostly for debugging of if someone wants to do a (better) touch detection
> in user space.
Not going to do anything at the rates you will get out of that interface.
I suspect this is a non starter. Lets see what Dmitry thinks.

Jonathan
> 
>>  Fair enough on the aux channel and temperature though.
>>
>> Otherwise a few minor bits and bobs.
>>
>> Jonathan
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/input/touchscreen/Kconfig   |   1 +
>>> drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>> index 2fb1f43..84c28e8 100644
>>> --- a/drivers/input/touchscreen/Kconfig
>>> +++ b/drivers/input/touchscreen/Kconfig
>>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>>> config TOUCHSCREEN_TSC2007
>>> 	tristate "TSC2007 based touchscreens"
>>> 	depends on I2C
>>> +	select IIO
>> This is pulling quite a bit of core IIO code in for a feature a lot
>> of users of this chip won't care about...
> 
> Ah, yes.
> 
> We didn't notice because we use many other IIO devices on the same board (GTA04).
> 
> So I will add some #ifdef CONFIG_IIO where necessary.
Cool
> 
> 
>>> 	help
>>> 	  Say Y here if you have a TSC2007 based touchscreen.
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>>> index e9d5086..559cbc6 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,9 @@
>>> #include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> #include <linux/input/touchscreen.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>> Why machine.h? (or driver.h for that reason).
> 
> Good question. Header files sometimes get in during
> development and are forgotten to take out... Or had
> been necessary when development started a while ago.
> Indeed I don't know why they are there.
> 
>>
>>> +#include <linux/iio/driver.h>
> 
> I have checked and we really neither need machine.h nor driver.h.
> 
>>>
>>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>>> @@ -61,6 +64,16 @@
>>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>>> #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>>
>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>> +{ \
>>> +	.datasheet_name = _name, \
>>> +	.type = _type, \
>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>>> +			BIT(_chan_info), \
>>> +	.indexed = 1, \
>>> +	.channel = _chan, \
>>> +}
>>> +
>>> struct ts_event {
>>> 	u16	x;
>>> 	u16	y;
>>> @@ -69,9 +82,11 @@ struct ts_event {
>>>
>>> struct tsc2007 {
>>> 	struct input_dev	*input;
>>> +	struct iio_dev		*indio;
>>> 	char			phys[32];
>>>
>>> 	struct i2c_client	*client;
>>> +	struct mutex		mlock;
>>>
>>> 	u16			model;
>>> 	u16			x_plate_ohms;
>>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>
>>> 		/* pen is down, continue with the measurement */
>>> +
>>> +		mutex_lock(&ts->mlock);
>>> 		tsc2007_read_values(ts, &tc);
>>> +		mutex_unlock(&ts->mlock);
>>>
>>> 		rt = tsc2007_calculate_resistance(ts, &tc);
>>>
>>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>>> 	tsc2007_stop(ts);
>>> }
>>>
>>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
>> This one needs some explanation.  What is it measuring?
> 
> Well, it is reporting the raw conductivity of the touch under certain
> conditions. But basically it is also a voltage measured by the ADC.
> 
> The real pressure value reported to user space is calculated
> using this and the z1 and z2 values.
> 
> If not pressed, conductivity is 0 and if lightly touched it jumps
> to some base value. When more pressed, conductivity increases
> while resistance goes down to some 300 Ohms or so.
> 
> But I have not found an IIO_OHM or IIO_MHO constant. Which would also
> not be the best description because it is not scaled to real ohms.
> 
>>
>>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>> At the moment this is defined as barometric pressure.
> 
> Ok, I see.
> 
>>  I suppose there
>> is no reason it can't extend to touch pressure though, even if it's a boolean
>> like this...
> 
> So let's keep it this way?
Except I'm saying we should drop it entirely.
> 
>>
>>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>> I would be tempted to only expose those channels that have a meaning
>> outside of touch screen usage (e.g. adc, temp0 and temp1).  
> 
> Well, for debugging and checking for non-responding or defective touch
> it is very handy to have them in /sys/bus/iio for direct inspection.
> For example we run a device test tool reading such values.
> 
> You might argue that we should run our own fork for this purpose to provide
> a patched driver. But we want people to use the kernel from kernel.org
> and still be able to do such debugging without asking them to replace the
> kernel first.
> 
>>> +};
>>> +
>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>>> +{
>>> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
>>> +	int adc_chan = chan->channel;
>>> +	int ret = 0;
>>> +
>>> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>>> +		return -EINVAL;
>>> +
>>> +	if (mask != IIO_CHAN_INFO_RAW)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&tsc->mlock);
>>> +
>>> +	switch (chan->channel) {
>>> +	case 0:
>>> +		*val = tsc2007_xfer(tsc, READ_X);
>>> +		break;
>>> +	case 1:
>>> +		*val = tsc2007_xfer(tsc, READ_Y);
>>> +		break;
>>> +	case 2:
>>> +		*val = tsc2007_xfer(tsc, READ_Z1);
>>> +		break;
>>> +	case 3:
>>> +		*val = tsc2007_xfer(tsc, READ_Z2);
>>> +		break;
>>> +	case 4:
>>> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>>> +		break;
>>> +	case 5: {
>>> +		struct ts_event tc;
>>> +
>>> +		tc.x = tsc2007_xfer(tsc, READ_X);
>>> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>> +		*val = tsc2007_calculate_resistance(tsc, &tc);
>>> +		break;
>>> +	}
>>> +	case 6:
>>> +		*val = tsc2007_is_pen_down(tsc);
>>> +		break;
>>> +	case 7:
>>> +		*val = tsc2007_xfer(tsc,
>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>>> +		break;
>>> +	case 8:
>>> +		*val = tsc2007_xfer(tsc,
>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>>> +		break;
>>> +	}
>>> +
>>> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
>>> +	tsc2007_xfer(tsc, PWRDOWN);
>>> +
>>> +	mutex_unlock(&tsc->mlock);
>>> +
>>> +	ret = IIO_VAL_INT;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct iio_info tsc2007_iio_info = {
>>> +	.read_raw = tsc2007_read_raw,
>>> +	.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> #ifdef CONFIG_OF
>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>> {
>>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>>> 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>>> 	struct tsc2007 *ts;
>>> 	struct input_dev *input_dev;
>>> +	struct iio_dev *indio_dev;
>>> 	int err;
>>>
>>> 	if (!i2c_check_functionality(client->adapter,
>>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> 		return -EIO;
>>>
>>> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> -	if (!ts)
>>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>> +	if (!indio_dev) {
>>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>>> 		return -ENOMEM;
>>> +	}
>>> +
>>> +	ts = iio_priv(indio_dev);
>>>
>>> 	input_dev = devm_input_allocate_device(&client->dev);
>>> 	if (!input_dev)
>>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>>>
>>> 	i2c_set_clientdata(client, ts);
>>>
>>> +	indio_dev->name = "tsc2007";
>>> +	indio_dev->dev.parent = &client->dev;
>>> +	indio_dev->info = &tsc2007_iio_info;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +	indio_dev->channels = tsc2007_iio_channel;
>>> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>>> +
>>> +	err = iio_device_register(indio_dev);
>>> +	if (err < 0) {
>>> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
>>> +			err);
>>> +		return err;
>>> +	}
>>> +
>>> 	ts->client = client;
>>> 	ts->irq = client->irq;
>>> 	ts->input = input_dev;
>>> +	ts->indio = indio_dev;
>>> 	init_waitqueue_head(&ts->wait);
>>> +	mutex_init(&ts->mlock);
>>>
>>> 	snprintf(ts->phys, sizeof(ts->phys),
>>> 		 "%s/input0", dev_name(&client->dev));
>>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>>> 	return 0;
>>> }
>>>
>>> +static int tsc2007_remove(struct i2c_client *client)
>>> +{
>>> +	struct tsc2007 *ts = i2c_get_clientdata(client);
>>> +	struct input_dev *input_dev = ts->input;
>>> +	struct iio_dev *indio_dev = ts->indio;
>>> +
>>> +	input_unregister_device(input_dev);
>>> +
>>> +	iio_device_unregister(indio_dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> static const struct i2c_device_id tsc2007_idtable[] = {
>>> 	{ "tsc2007", 0 },
>>> 	{ }
>>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>>> 	},
>>> 	.id_table	= tsc2007_idtable,
>>> 	.probe		= tsc2007_probe,
>>> +	.remove		= tsc2007_remove,
>>> };
>>>
>>> module_i2c_driver(tsc2007_driver);
>>>
>>
> 
> BR and thanks,
> Nikolaus
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Jonathan Cameron]

On 24/09/16 18:07, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>
>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels
>>> in addition to the raw x, y, z values and the estimated touch
>>> screen resistance. This can be used for debugging or special
>>> applications.
>> I'm unconvinced that exposing the touch related channels is terribly
>> useful.
> 
> Mostly for debugging of if someone wants to do a (better) touch detection
> in user space.
Not going to do anything at the rates you will get out of that interface.
I suspect this is a non starter. Lets see what Dmitry thinks.

Jonathan
> 
>>  Fair enough on the aux channel and temperature though.
>>
>> Otherwise a few minor bits and bobs.
>>
>> Jonathan
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>>> ---
>>> drivers/input/touchscreen/Kconfig   |   1 +
>>> drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>> index 2fb1f43..84c28e8 100644
>>> --- a/drivers/input/touchscreen/Kconfig
>>> +++ b/drivers/input/touchscreen/Kconfig
>>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>>> config TOUCHSCREEN_TSC2007
>>> 	tristate "TSC2007 based touchscreens"
>>> 	depends on I2C
>>> +	select IIO
>> This is pulling quite a bit of core IIO code in for a feature a lot
>> of users of this chip won't care about...
> 
> Ah, yes.
> 
> We didn't notice because we use many other IIO devices on the same board (GTA04).
> 
> So I will add some #ifdef CONFIG_IIO where necessary.
Cool
> 
> 
>>> 	help
>>> 	  Say Y here if you have a TSC2007 based touchscreen.
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>>> index e9d5086..559cbc6 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,9 @@
>>> #include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> #include <linux/input/touchscreen.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>> Why machine.h? (or driver.h for that reason).
> 
> Good question. Header files sometimes get in during
> development and are forgotten to take out... Or had
> been necessary when development started a while ago.
> Indeed I don't know why they are there.
> 
>>
>>> +#include <linux/iio/driver.h>
> 
> I have checked and we really neither need machine.h nor driver.h.
> 
>>>
>>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>>> @@ -61,6 +64,16 @@
>>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>>> #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>>
>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>> +{ \
>>> +	.datasheet_name = _name, \
>>> +	.type = _type, \
>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>>> +			BIT(_chan_info), \
>>> +	.indexed = 1, \
>>> +	.channel = _chan, \
>>> +}
>>> +
>>> struct ts_event {
>>> 	u16	x;
>>> 	u16	y;
>>> @@ -69,9 +82,11 @@ struct ts_event {
>>>
>>> struct tsc2007 {
>>> 	struct input_dev	*input;
>>> +	struct iio_dev		*indio;
>>> 	char			phys[32];
>>>
>>> 	struct i2c_client	*client;
>>> +	struct mutex		mlock;
>>>
>>> 	u16			model;
>>> 	u16			x_plate_ohms;
>>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>
>>> 		/* pen is down, continue with the measurement */
>>> +
>>> +		mutex_lock(&ts->mlock);
>>> 		tsc2007_read_values(ts, &tc);
>>> +		mutex_unlock(&ts->mlock);
>>>
>>> 		rt = tsc2007_calculate_resistance(ts, &tc);
>>>
>>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>>> 	tsc2007_stop(ts);
>>> }
>>>
>>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
>> This one needs some explanation.  What is it measuring?
> 
> Well, it is reporting the raw conductivity of the touch under certain
> conditions. But basically it is also a voltage measured by the ADC.
> 
> The real pressure value reported to user space is calculated
> using this and the z1 and z2 values.
> 
> If not pressed, conductivity is 0 and if lightly touched it jumps
> to some base value. When more pressed, conductivity increases
> while resistance goes down to some 300 Ohms or so.
> 
> But I have not found an IIO_OHM or IIO_MHO constant. Which would also
> not be the best description because it is not scaled to real ohms.
> 
>>
>>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>> At the moment this is defined as barometric pressure.
> 
> Ok, I see.
> 
>>  I suppose there
>> is no reason it can't extend to touch pressure though, even if it's a boolean
>> like this...
> 
> So let's keep it this way?
Except I'm saying we should drop it entirely.
> 
>>
>>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>> I would be tempted to only expose those channels that have a meaning
>> outside of touch screen usage (e.g. adc, temp0 and temp1).  
> 
> Well, for debugging and checking for non-responding or defective touch
> it is very handy to have them in /sys/bus/iio for direct inspection.
> For example we run a device test tool reading such values.
> 
> You might argue that we should run our own fork for this purpose to provide
> a patched driver. But we want people to use the kernel from kernel.org
> and still be able to do such debugging without asking them to replace the
> kernel first.
> 
>>> +};
>>> +
>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>>> +{
>>> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
>>> +	int adc_chan = chan->channel;
>>> +	int ret = 0;
>>> +
>>> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>>> +		return -EINVAL;
>>> +
>>> +	if (mask != IIO_CHAN_INFO_RAW)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&tsc->mlock);
>>> +
>>> +	switch (chan->channel) {
>>> +	case 0:
>>> +		*val = tsc2007_xfer(tsc, READ_X);
>>> +		break;
>>> +	case 1:
>>> +		*val = tsc2007_xfer(tsc, READ_Y);
>>> +		break;
>>> +	case 2:
>>> +		*val = tsc2007_xfer(tsc, READ_Z1);
>>> +		break;
>>> +	case 3:
>>> +		*val = tsc2007_xfer(tsc, READ_Z2);
>>> +		break;
>>> +	case 4:
>>> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>>> +		break;
>>> +	case 5: {
>>> +		struct ts_event tc;
>>> +
>>> +		tc.x = tsc2007_xfer(tsc, READ_X);
>>> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>> +		*val = tsc2007_calculate_resistance(tsc, &tc);
>>> +		break;
>>> +	}
>>> +	case 6:
>>> +		*val = tsc2007_is_pen_down(tsc);
>>> +		break;
>>> +	case 7:
>>> +		*val = tsc2007_xfer(tsc,
>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>>> +		break;
>>> +	case 8:
>>> +		*val = tsc2007_xfer(tsc,
>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>>> +		break;
>>> +	}
>>> +
>>> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
>>> +	tsc2007_xfer(tsc, PWRDOWN);
>>> +
>>> +	mutex_unlock(&tsc->mlock);
>>> +
>>> +	ret = IIO_VAL_INT;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct iio_info tsc2007_iio_info = {
>>> +	.read_raw = tsc2007_read_raw,
>>> +	.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> #ifdef CONFIG_OF
>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>> {
>>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>>> 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>>> 	struct tsc2007 *ts;
>>> 	struct input_dev *input_dev;
>>> +	struct iio_dev *indio_dev;
>>> 	int err;
>>>
>>> 	if (!i2c_check_functionality(client->adapter,
>>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> 		return -EIO;
>>>
>>> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> -	if (!ts)
>>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>> +	if (!indio_dev) {
>>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>>> 		return -ENOMEM;
>>> +	}
>>> +
>>> +	ts = iio_priv(indio_dev);
>>>
>>> 	input_dev = devm_input_allocate_device(&client->dev);
>>> 	if (!input_dev)
>>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>>>
>>> 	i2c_set_clientdata(client, ts);
>>>
>>> +	indio_dev->name = "tsc2007";
>>> +	indio_dev->dev.parent = &client->dev;
>>> +	indio_dev->info = &tsc2007_iio_info;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +	indio_dev->channels = tsc2007_iio_channel;
>>> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>>> +
>>> +	err = iio_device_register(indio_dev);
>>> +	if (err < 0) {
>>> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
>>> +			err);
>>> +		return err;
>>> +	}
>>> +
>>> 	ts->client = client;
>>> 	ts->irq = client->irq;
>>> 	ts->input = input_dev;
>>> +	ts->indio = indio_dev;
>>> 	init_waitqueue_head(&ts->wait);
>>> +	mutex_init(&ts->mlock);
>>>
>>> 	snprintf(ts->phys, sizeof(ts->phys),
>>> 		 "%s/input0", dev_name(&client->dev));
>>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>>> 	return 0;
>>> }
>>>
>>> +static int tsc2007_remove(struct i2c_client *client)
>>> +{
>>> +	struct tsc2007 *ts = i2c_get_clientdata(client);
>>> +	struct input_dev *input_dev = ts->input;
>>> +	struct iio_dev *indio_dev = ts->indio;
>>> +
>>> +	input_unregister_device(input_dev);
>>> +
>>> +	iio_device_unregister(indio_dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> static const struct i2c_device_id tsc2007_idtable[] = {
>>> 	{ "tsc2007", 0 },
>>> 	{ }
>>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>>> 	},
>>> 	.id_table	= tsc2007_idtable,
>>> 	.probe		= tsc2007_probe,
>>> +	.remove		= tsc2007_remove,
>>> };
>>>
>>> module_i2c_driver(tsc2007_driver);
>>>
>>
> 
> BR and thanks,
> Nikolaus
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
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 v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 24.09.2016 um 19:26 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 24/09/16 18:07, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>> 
>>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>>> The tsc2007 chip not only has a resistive touch screen controller but
>>>> also an external AUX adc imput which can be used for an ambient
>>>> light sensor, battery voltage monitoring or any general purpose.
>>>> 
>>>> Additionally it can measure the chip temperature.
>>>> 
>>>> This extension provides an iio interface for these adc channels
>>>> in addition to the raw x, y, z values and the estimated touch
>>>> screen resistance. This can be used for debugging or special
>>>> applications.
>>> I'm unconvinced that exposing the touch related channels is terribly
>>> useful.
>> 
>> Mostly for debugging of if someone wants to do a (better) touch detection
>> in user space.
> Not going to do anything at the rates you will get out of that interface.

Touch detection and processing does not need a fast interface. In standard
operation (input device) the tsc2007 is polled in multiples of jiffies.

And I don't know what people will do with it - in addition to debugging
and hardware testing which is what we are doing (and need in the upstream
driver - see below).

> I suspect this is a non starter. Lets see what Dmitry thinks.

Yes. More opinions are welcome.

> 
> Jonathan
>> 
>>> Fair enough on the aux channel and temperature though.
>>> 
>>> Otherwise a few minor bits and bobs.
>>> 
>>> Jonathan
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/input/touchscreen/Kconfig   |   1 +
>>>> drivers/input/touchscreen/tsc2007.c | 137 +++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>>> index 2fb1f43..84c28e8 100644
>>>> --- a/drivers/input/touchscreen/Kconfig
>>>> +++ b/drivers/input/touchscreen/Kconfig
>>>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>>>> config TOUCHSCREEN_TSC2007
>>>> 	tristate "TSC2007 based touchscreens"
>>>> 	depends on I2C
>>>> +	select IIO
>>> This is pulling quite a bit of core IIO code in for a feature a lot
>>> of users of this chip won't care about...
>> 
>> Ah, yes.
>> 
>> We didn't notice because we use many other IIO devices on the same board (GTA04).
>> 
>> So I will add some #ifdef CONFIG_IIO where necessary.
> Cool

I have already prepared for PATCH V4 and tested w/o CONFIG_IIO and with.

>> 
>> 
>>>> 	help
>>>> 	  Say Y here if you have a TSC2007 based touchscreen.
>>>> 
>>>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>>>> index e9d5086..559cbc6 100644
>>>> --- a/drivers/input/touchscreen/tsc2007.c
>>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>>> @@ -30,6 +30,9 @@
>>>> #include <linux/of.h>
>>>> #include <linux/of_gpio.h>
>>>> #include <linux/input/touchscreen.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/machine.h>
>>> Why machine.h? (or driver.h for that reason).
>> 
>> Good question. Header files sometimes get in during
>> development and are forgotten to take out... Or had
>> been necessary when development started a while ago.
>> Indeed I don't know why they are there.
>> 
>>> 
>>>> +#include <linux/iio/driver.h>
>> 
>> I have checked and we really neither need machine.h nor driver.h.
>> 
>>>> 
>>>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>>>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>>>> @@ -61,6 +64,16 @@
>>>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>>>> #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>>> 
>>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>>> +{ \
>>>> +	.datasheet_name = _name, \
>>>> +	.type = _type, \
>>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>>>> +			BIT(_chan_info), \
>>>> +	.indexed = 1, \
>>>> +	.channel = _chan, \
>>>> +}
>>>> +
>>>> struct ts_event {
>>>> 	u16	x;
>>>> 	u16	y;
>>>> @@ -69,9 +82,11 @@ struct ts_event {
>>>> 
>>>> struct tsc2007 {
>>>> 	struct input_dev	*input;
>>>> +	struct iio_dev		*indio;
>>>> 	char			phys[32];
>>>> 
>>>> 	struct i2c_client	*client;
>>>> +	struct mutex		mlock;
>>>> 
>>>> 	u16			model;
>>>> 	u16			x_plate_ohms;
>>>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>>>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>> 
>>>> 		/* pen is down, continue with the measurement */
>>>> +
>>>> +		mutex_lock(&ts->mlock);
>>>> 		tsc2007_read_values(ts, &tc);
>>>> +		mutex_unlock(&ts->mlock);
>>>> 
>>>> 		rt = tsc2007_calculate_resistance(ts, &tc);
>>>> 
>>>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev *input_dev)
>>>> 	tsc2007_stop(ts);
>>>> }
>>>> 
>>>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>>>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
>>> This one needs some explanation.  What is it measuring?
>> 
>> Well, it is reporting the raw conductivity of the touch under certain
>> conditions. But basically it is also a voltage measured by the ADC.
>> 
>> The real pressure value reported to user space is calculated
>> using this and the z1 and z2 values.
>> 
>> If not pressed, conductivity is 0 and if lightly touched it jumps
>> to some base value. When more pressed, conductivity increases
>> while resistance goes down to some 300 Ohms or so.
>> 
>> But I have not found an IIO_OHM or IIO_MHO constant. Which would also
>> not be the best description because it is not scaled to real ohms.
>> 
>>> 
>>>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>>> At the moment this is defined as barometric pressure.
>> 
>> Ok, I see.
>> 
>>> I suppose there
>>> is no reason it can't extend to touch pressure though, even if it's a boolean
>>> like this...
>> 
>> So let's keep it this way?
> Except I'm saying we should drop it entirely.
>> 
>>> 
>>>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> I would be tempted to only expose those channels that have a meaning
>>> outside of touch screen usage (e.g. adc, temp0 and temp1).  
>> 

The following is my main argument why we should not drop it (it also is
neither complex nor harmful if not used):

>> Well, for debugging and checking for non-responding or defective touch
>> it is very handy to have them in /sys/bus/iio for direct inspection.
>> For example we run a device test tool reading such values.

> 
>> You might argue that we should run our own fork for this purpose to provide
>> a patched driver. But we want people to use the kernel from kernel.org
>> and still be able to do such debugging without asking them to replace the
>> kernel first.

In other words: we do neither want to run our own kernel fork (which might not
be the standard case) but also not loose (practically required) features we
need...


>> 
>>>> +};
>>>> +
>>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>>> +	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>>>> +{
>>>> +	struct  tsc2007 *tsc = iio_priv(indio_dev);
>>>> +	int adc_chan = chan->channel;
>>>> +	int ret = 0;
>>>> +
>>>> +	if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (mask != IIO_CHAN_INFO_RAW)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&tsc->mlock);
>>>> +
>>>> +	switch (chan->channel) {
>>>> +	case 0:
>>>> +		*val = tsc2007_xfer(tsc, READ_X);
>>>> +		break;
>>>> +	case 1:
>>>> +		*val = tsc2007_xfer(tsc, READ_Y);
>>>> +		break;
>>>> +	case 2:
>>>> +		*val = tsc2007_xfer(tsc, READ_Z1);
>>>> +		break;
>>>> +	case 3:
>>>> +		*val = tsc2007_xfer(tsc, READ_Z2);
>>>> +		break;
>>>> +	case 4:
>>>> +		*val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>>>> +		break;
>>>> +	case 5: {
>>>> +		struct ts_event tc;
>>>> +
>>>> +		tc.x = tsc2007_xfer(tsc, READ_X);
>>>> +		tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>>> +		tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>>> +		*val = tsc2007_calculate_resistance(tsc, &tc);
>>>> +		break;
>>>> +	}
>>>> +	case 6:
>>>> +		*val = tsc2007_is_pen_down(tsc);
>>>> +		break;
>>>> +	case 7:
>>>> +		*val = tsc2007_xfer(tsc,
>>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>>>> +		break;
>>>> +	case 8:
>>>> +		*val = tsc2007_xfer(tsc,
>>>> +				    (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
>>>> +	tsc2007_xfer(tsc, PWRDOWN);
>>>> +
>>>> +	mutex_unlock(&tsc->mlock);
>>>> +
>>>> +	ret = IIO_VAL_INT;
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static const struct iio_info tsc2007_iio_info = {
>>>> +	.read_raw = tsc2007_read_raw,
>>>> +	.driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> #ifdef CONFIG_OF
>>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>>> {
>>>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client *client,
>>>> 	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
>>>> 	struct tsc2007 *ts;
>>>> 	struct input_dev *input_dev;
>>>> +	struct iio_dev *indio_dev;
>>>> 	int err;
>>>> 
>>>> 	if (!i2c_check_functionality(client->adapter,
>>>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>> 		return -EIO;
>>>> 
>>>> -	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>>> -	if (!ts)
>>>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>>> +	if (!indio_dev) {
>>>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>>>> 		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	ts = iio_priv(indio_dev);
>>>> 
>>>> 	input_dev = devm_input_allocate_device(&client->dev);
>>>> 	if (!input_dev)
>>>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client *client,
>>>> 
>>>> 	i2c_set_clientdata(client, ts);
>>>> 
>>>> +	indio_dev->name = "tsc2007";
>>>> +	indio_dev->dev.parent = &client->dev;
>>>> +	indio_dev->info = &tsc2007_iio_info;
>>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +	indio_dev->channels = tsc2007_iio_channel;
>>>> +	indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>>>> +
>>>> +	err = iio_device_register(indio_dev);
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "iio_device_register() failed: %d\n",
>>>> +			err);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> 	ts->client = client;
>>>> 	ts->irq = client->irq;
>>>> 	ts->input = input_dev;
>>>> +	ts->indio = indio_dev;
>>>> 	init_waitqueue_head(&ts->wait);
>>>> +	mutex_init(&ts->mlock);
>>>> 
>>>> 	snprintf(ts->phys, sizeof(ts->phys),
>>>> 		 "%s/input0", dev_name(&client->dev));
>>>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client *client,
>>>> 	return 0;
>>>> }
>>>> 
>>>> +static int tsc2007_remove(struct i2c_client *client)
>>>> +{
>>>> +	struct tsc2007 *ts = i2c_get_clientdata(client);
>>>> +	struct input_dev *input_dev = ts->input;
>>>> +	struct iio_dev *indio_dev = ts->indio;
>>>> +
>>>> +	input_unregister_device(input_dev);
>>>> +
>>>> +	iio_device_unregister(indio_dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> static const struct i2c_device_id tsc2007_idtable[] = {
>>>> 	{ "tsc2007", 0 },
>>>> 	{ }
>>>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver = {
>>>> 	},
>>>> 	.id_table	= tsc2007_idtable,
>>>> 	.probe		= tsc2007_probe,
>>>> +	.remove		= tsc2007_remove,
>>>> };
>>>> 
>>>> module_i2c_driver(tsc2007_driver);
>>>> 
>>> 
>> 
>> BR and thanks,
>> Nikolaus

BR and thanks,
Nikolaus

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 24.09.2016 um 19:26 schrieb Jonathan Cameron <jic23@kernel.org>:
>=20
> On 24/09/16 18:07, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>>=20
>>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>>=20
>>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>>> The tsc2007 chip not only has a resistive touch screen controller =
but
>>>> also an external AUX adc imput which can be used for an ambient
>>>> light sensor, battery voltage monitoring or any general purpose.
>>>>=20
>>>> Additionally it can measure the chip temperature.
>>>>=20
>>>> This extension provides an iio interface for these adc channels
>>>> in addition to the raw x, y, z values and the estimated touch
>>>> screen resistance. This can be used for debugging or special
>>>> applications.
>>> I'm unconvinced that exposing the touch related channels is terribly
>>> useful.
>>=20
>> Mostly for debugging of if someone wants to do a (better) touch =
detection
>> in user space.
> Not going to do anything at the rates you will get out of that =
interface.

Touch detection and processing does not need a fast interface. In =
standard
operation (input device) the tsc2007 is polled in multiples of jiffies.

And I don't know what people will do with it - in addition to debugging
and hardware testing which is what we are doing (and need in the =
upstream
driver - see below).

> I suspect this is a non starter. Lets see what Dmitry thinks.

Yes. More opinions are welcome.

>=20
> Jonathan
>>=20
>>> Fair enough on the aux channel and temperature though.
>>>=20
>>> Otherwise a few minor bits and bobs.
>>>=20
>>> Jonathan
>>>>=20
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/input/touchscreen/Kconfig   |   1 +
>>>> drivers/input/touchscreen/tsc2007.c | 137 =
+++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>>>=20
>>>> diff --git a/drivers/input/touchscreen/Kconfig =
b/drivers/input/touchscreen/Kconfig
>>>> index 2fb1f43..84c28e8 100644
>>>> --- a/drivers/input/touchscreen/Kconfig
>>>> +++ b/drivers/input/touchscreen/Kconfig
>>>> @@ -1019,6 +1019,7 @@ config TOUCHSCREEN_TSC2005
>>>> config TOUCHSCREEN_TSC2007
>>>> 	tristate "TSC2007 based touchscreens"
>>>> 	depends on I2C
>>>> +	select IIO
>>> This is pulling quite a bit of core IIO code in for a feature a lot
>>> of users of this chip won't care about...
>>=20
>> Ah, yes.
>>=20
>> We didn't notice because we use many other IIO devices on the same =
board (GTA04).
>>=20
>> So I will add some #ifdef CONFIG_IIO where necessary.
> Cool

I have already prepared for PATCH V4 and tested w/o CONFIG_IIO and with.

>>=20
>>=20
>>>> 	help
>>>> 	  Say Y here if you have a TSC2007 based touchscreen.
>>>>=20
>>>> diff --git a/drivers/input/touchscreen/tsc2007.c =
b/drivers/input/touchscreen/tsc2007.c
>>>> index e9d5086..559cbc6 100644
>>>> --- a/drivers/input/touchscreen/tsc2007.c
>>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>>> @@ -30,6 +30,9 @@
>>>> #include <linux/of.h>
>>>> #include <linux/of_gpio.h>
>>>> #include <linux/input/touchscreen.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/machine.h>
>>> Why machine.h? (or driver.h for that reason).
>>=20
>> Good question. Header files sometimes get in during
>> development and are forgotten to take out... Or had
>> been necessary when development started a while ago.
>> Indeed I don't know why they are there.
>>=20
>>>=20
>>>> +#include <linux/iio/driver.h>
>>=20
>> I have checked and we really neither need machine.h nor driver.h.
>>=20
>>>>=20
>>>> #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>>>> #define TSC2007_MEASURE_AUX		(0x2 << 4)
>>>> @@ -61,6 +64,16 @@
>>>> #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>>>> #define PWRDOWN		(TSC2007_12BIT | =
TSC2007_POWER_OFF_IRQ_EN)
>>>>=20
>>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>>> +{ \
>>>> +	.datasheet_name =3D _name, \
>>>> +	.type =3D _type, \
>>>> +	.info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW) |	\
>>>> +			BIT(_chan_info), \
>>>> +	.indexed =3D 1, \
>>>> +	.channel =3D _chan, \
>>>> +}
>>>> +
>>>> struct ts_event {
>>>> 	u16	x;
>>>> 	u16	y;
>>>> @@ -69,9 +82,11 @@ struct ts_event {
>>>>=20
>>>> struct tsc2007 {
>>>> 	struct input_dev	*input;
>>>> +	struct iio_dev		*indio;
>>>> 	char			phys[32];
>>>>=20
>>>> 	struct i2c_client	*client;
>>>> +	struct mutex		mlock;
>>>>=20
>>>> 	u16			model;
>>>> 	u16			x_plate_ohms;
>>>> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, =
void *handle)
>>>> 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>>=20
>>>> 		/* pen is down, continue with the measurement */
>>>> +
>>>> +		mutex_lock(&ts->mlock);
>>>> 		tsc2007_read_values(ts, &tc);
>>>> +		mutex_unlock(&ts->mlock);
>>>>=20
>>>> 		rt =3D tsc2007_calculate_resistance(ts, &tc);
>>>>=20
>>>> @@ -319,6 +337,86 @@ static void tsc2007_close(struct input_dev =
*input_dev)
>>>> 	tsc2007_stop(ts);
>>>> }
>>>>=20
>>>> +static const struct iio_chan_spec tsc2007_iio_channel[] =3D {
>>>> +	TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* =
Ohms? */
>>> This one needs some explanation.  What is it measuring?
>>=20
>> Well, it is reporting the raw conductivity of the touch under certain
>> conditions. But basically it is also a voltage measured by the ADC.
>>=20
>> The real pressure value reported to user space is calculated
>> using this and the z1 and z2 values.
>>=20
>> If not pressed, conductivity is 0 and if lightly touched it jumps
>> to some base value. When more pressed, conductivity increases
>> while resistance goes down to some 300 Ohms or so.
>>=20
>> But I have not found an IIO_OHM or IIO_MHO constant. Which would also
>> not be the best description because it is not scaled to real ohms.
>>=20
>>>=20
>>>> +	TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>>> At the moment this is defined as barometric pressure.
>>=20
>> Ok, I see.
>>=20
>>> I suppose there
>>> is no reason it can't extend to touch pressure though, even if it's =
a boolean
>>> like this...
>>=20
>> So let's keep it this way?
> Except I'm saying we should drop it entirely.
>>=20
>>>=20
>>>> +	TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>>> +	TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> I would be tempted to only expose those channels that have a meaning
>>> outside of touch screen usage (e.g. adc, temp0 and temp1). =20
>>=20

The following is my main argument why we should not drop it (it also is
neither complex nor harmful if not used):

>> Well, for debugging and checking for non-responding or defective =
touch
>> it is very handy to have them in /sys/bus/iio for direct inspection.
>> For example we run a device test tool reading such values.

>=20
>> You might argue that we should run our own fork for this purpose to =
provide
>> a patched driver. But we want people to use the kernel from =
kernel.org
>> and still be able to do such debugging without asking them to replace =
the
>> kernel first.

In other words: we do neither want to run our own kernel fork (which =
might not
be the standard case) but also not loose (practically required) features =
we
need...


>>=20
>>>> +};
>>>> +
>>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>>> +	struct iio_chan_spec const *chan, int *val, int *val2, long =
mask)
>>>> +{
>>>> +	struct  tsc2007 *tsc =3D iio_priv(indio_dev);
>>>> +	int adc_chan =3D chan->channel;
>>>> +	int ret =3D 0;
>>>> +
>>>> +	if (adc_chan >=3D ARRAY_SIZE(tsc2007_iio_channel))
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (mask !=3D IIO_CHAN_INFO_RAW)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&tsc->mlock);
>>>> +
>>>> +	switch (chan->channel) {
>>>> +	case 0:
>>>> +		*val =3D tsc2007_xfer(tsc, READ_X);
>>>> +		break;
>>>> +	case 1:
>>>> +		*val =3D tsc2007_xfer(tsc, READ_Y);
>>>> +		break;
>>>> +	case 2:
>>>> +		*val =3D tsc2007_xfer(tsc, READ_Z1);
>>>> +		break;
>>>> +	case 3:
>>>> +		*val =3D tsc2007_xfer(tsc, READ_Z2);
>>>> +		break;
>>>> +	case 4:
>>>> +		*val =3D tsc2007_xfer(tsc, (ADC_ON_12BIT | =
TSC2007_MEASURE_AUX));
>>>> +		break;
>>>> +	case 5: {
>>>> +		struct ts_event tc;
>>>> +
>>>> +		tc.x =3D tsc2007_xfer(tsc, READ_X);
>>>> +		tc.z1 =3D tsc2007_xfer(tsc, READ_Z1);
>>>> +		tc.z2 =3D tsc2007_xfer(tsc, READ_Z2);
>>>> +		*val =3D tsc2007_calculate_resistance(tsc, &tc);
>>>> +		break;
>>>> +	}
>>>> +	case 6:
>>>> +		*val =3D tsc2007_is_pen_down(tsc);
>>>> +		break;
>>>> +	case 7:
>>>> +		*val =3D tsc2007_xfer(tsc,
>>>> +				    (ADC_ON_12BIT | =
TSC2007_MEASURE_TEMP0));
>>>> +		break;
>>>> +	case 8:
>>>> +		*val =3D tsc2007_xfer(tsc,
>>>> +				    (ADC_ON_12BIT | =
TSC2007_MEASURE_TEMP1));
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	/* Prepare for next touch reading - power down ADC, enable =
PENIRQ */
>>>> +	tsc2007_xfer(tsc, PWRDOWN);
>>>> +
>>>> +	mutex_unlock(&tsc->mlock);
>>>> +
>>>> +	ret =3D IIO_VAL_INT;
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static const struct iio_info tsc2007_iio_info =3D {
>>>> +	.read_raw =3D tsc2007_read_raw,
>>>> +	.driver_module =3D THIS_MODULE,
>>>> +};
>>>> +
>>>> #ifdef CONFIG_OF
>>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>>> {
>>>> @@ -453,15 +551,20 @@ static int tsc2007_probe(struct i2c_client =
*client,
>>>> 	const struct tsc2007_platform_data *pdata =3D =
dev_get_platdata(&client->dev);
>>>> 	struct tsc2007 *ts;
>>>> 	struct input_dev *input_dev;
>>>> +	struct iio_dev *indio_dev;
>>>> 	int err;
>>>>=20
>>>> 	if (!i2c_check_functionality(client->adapter,
>>>> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>> 		return -EIO;
>>>>=20
>>>> -	ts =3D devm_kzalloc(&client->dev, sizeof(struct tsc2007), =
GFP_KERNEL);
>>>> -	if (!ts)
>>>> +	indio_dev =3D devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>>> +	if (!indio_dev) {
>>>> +		dev_err(&client->dev, "iio_device_alloc failed\n");
>>>> 		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	ts =3D iio_priv(indio_dev);
>>>>=20
>>>> 	input_dev =3D devm_input_allocate_device(&client->dev);
>>>> 	if (!input_dev)
>>>> @@ -469,10 +572,26 @@ static int tsc2007_probe(struct i2c_client =
*client,
>>>>=20
>>>> 	i2c_set_clientdata(client, ts);
>>>>=20
>>>> +	indio_dev->name =3D "tsc2007";
>>>> +	indio_dev->dev.parent =3D &client->dev;
>>>> +	indio_dev->info =3D &tsc2007_iio_info;
>>>> +	indio_dev->modes =3D INDIO_DIRECT_MODE;
>>>> +	indio_dev->channels =3D tsc2007_iio_channel;
>>>> +	indio_dev->num_channels =3D ARRAY_SIZE(tsc2007_iio_channel);
>>>> +
>>>> +	err =3D iio_device_register(indio_dev);
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "iio_device_register() failed: =
%d\n",
>>>> +			err);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> 	ts->client =3D client;
>>>> 	ts->irq =3D client->irq;
>>>> 	ts->input =3D input_dev;
>>>> +	ts->indio =3D indio_dev;
>>>> 	init_waitqueue_head(&ts->wait);
>>>> +	mutex_init(&ts->mlock);
>>>>=20
>>>> 	snprintf(ts->phys, sizeof(ts->phys),
>>>> 		 "%s/input0", dev_name(&client->dev));
>>>> @@ -548,6 +667,19 @@ static int tsc2007_probe(struct i2c_client =
*client,
>>>> 	return 0;
>>>> }
>>>>=20
>>>> +static int tsc2007_remove(struct i2c_client *client)
>>>> +{
>>>> +	struct tsc2007 *ts =3D i2c_get_clientdata(client);
>>>> +	struct input_dev *input_dev =3D ts->input;
>>>> +	struct iio_dev *indio_dev =3D ts->indio;
>>>> +
>>>> +	input_unregister_device(input_dev);
>>>> +
>>>> +	iio_device_unregister(indio_dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> static const struct i2c_device_id tsc2007_idtable[] =3D {
>>>> 	{ "tsc2007", 0 },
>>>> 	{ }
>>>> @@ -570,6 +702,7 @@ static struct i2c_driver tsc2007_driver =3D {
>>>> 	},
>>>> 	.id_table	=3D tsc2007_idtable,
>>>> 	.probe		=3D tsc2007_probe,
>>>> +	.remove		=3D tsc2007_remove,
>>>> };
>>>>=20
>>>> module_i2c_driver(tsc2007_driver);
>>>>=20
>>>=20
>>=20
>> BR and thanks,
>> Nikolaus

BR and thanks,
Nikolaus

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 10:07 AM, H. Nikolaus Schaller
<hns@goldelico.com> wrote:
> Hi Jonathan,
>
>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels
>>> in addition to the raw x, y, z values and the estimated touch
>>> screen resistance. This can be used for debugging or special
>>> applications.
>> I'm unconvinced that exposing the touch related channels is terribly
>> useful.
>
> Mostly for debugging of if someone wants to do a (better) touch detection
> in user space.

For that we should probably export data as a heat map via V4L2
interface, similarly to what Nick Dyer did for Synaptics and Atmel
drivers (albeit their heatmap functions are too slow for real
processing and only suitable for debug/validation).

Thanks.

-- 
Dmitry

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 10:07 AM, H. Nikolaus Schaller
<hns@goldelico.com> wrote:
> Hi Jonathan,
>
>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels
>>> in addition to the raw x, y, z values and the estimated touch
>>> screen resistance. This can be used for debugging or special
>>> applications.
>> I'm unconvinced that exposing the touch related channels is terribly
>> useful.
>
> Mostly for debugging of if someone wants to do a (better) touch detection
> in user space.

For that we should probably export data as a heat map via V4L2
interface, similarly to what Nick Dyer did for Synaptics and Atmel
drivers (albeit their heatmap functions are too slow for real
processing and only suitable for debug/validation).

Thanks.

-- 
Dmitry

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Dmitry,

> Am 24.09.2016 um 19:44 schrieb Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> 
> On Sat, Sep 24, 2016 at 10:07 AM, H. Nikolaus Schaller
> <hns@goldelico.com> wrote:
>> Hi Jonathan,
>> 
>>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23@kernel.org>:
>>> 
>>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>>> The tsc2007 chip not only has a resistive touch screen controller but
>>>> also an external AUX adc imput which can be used for an ambient
>>>> light sensor, battery voltage monitoring or any general purpose.
>>>> 
>>>> Additionally it can measure the chip temperature.
>>>> 
>>>> This extension provides an iio interface for these adc channels
>>>> in addition to the raw x, y, z values and the estimated touch
>>>> screen resistance. This can be used for debugging or special
>>>> applications.
>>> I'm unconvinced that exposing the touch related channels is terribly
>>> useful.
>> 
>> Mostly for debugging of if someone wants to do a (better) touch detection

s/of/or/

>> in user space.
> 
> For that we should probably export data as a heat map via V4L2
> interface, similarly to what Nick Dyer did for Synaptics and Atmel
> drivers (albeit their heatmap functions are too slow for real
> processing and only suitable for debug/validation).

Sorry for my ignorance, but what is a heat map?
Does it allow to cat /sys/bus/something the raw values?

And why introduce another interface if we already get the iio that
easily reports such values by just adding more channels and some
lines of code?

BR,
Nikolaus

Re: [PATCH v3 3/8] drivers:input:tsc2007: add iio interface to read external ADC input, temperature and raw conversion values

[H. Nikolaus Schaller]

Hi Dmitry,

> Am 24.09.2016 um 19:44 schrieb Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 
> On Sat, Sep 24, 2016 at 10:07 AM, H. Nikolaus Schaller
> <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> wrote:
>> Hi Jonathan,
>> 
>>> Am 24.09.2016 um 18:07 schrieb Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>> 
>>> On 23/09/16 13:41, H. Nikolaus Schaller wrote:
>>>> The tsc2007 chip not only has a resistive touch screen controller but
>>>> also an external AUX adc imput which can be used for an ambient
>>>> light sensor, battery voltage monitoring or any general purpose.
>>>> 
>>>> Additionally it can measure the chip temperature.
>>>> 
>>>> This extension provides an iio interface for these adc channels
>>>> in addition to the raw x, y, z values and the estimated touch
>>>> screen resistance. This can be used for debugging or special
>>>> applications.
>>> I'm unconvinced that exposing the touch related channels is terribly
>>> useful.
>> 
>> Mostly for debugging of if someone wants to do a (better) touch detection

s/of/or/

>> in user space.
> 
> For that we should probably export data as a heat map via V4L2
> interface, similarly to what Nick Dyer did for Synaptics and Atmel
> drivers (albeit their heatmap functions are too slow for real
> processing and only suitable for debug/validation).

Sorry for my ignorance, but what is a heat map?
Does it allow to cat /sys/bus/something the raw values?

And why introduce another interface if we already get the iio that
easily reports such values by just adding more channels and some
lines of code?

BR,
Nikolaus

[PATCH v3 4/8] drivers:input:tsc2007: check for presence and power down tsc2007 during probe

[H. Nikolaus Schaller]

1. check if chip is really present and don't succeed if it isn't.
2. if it succeeds, power down the chip until accessed

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/touchscreen/tsc2007.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 559cbc6..3ddc31e 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -657,6 +657,15 @@ static int tsc2007_probe(struct i2c_client *client,
 
 	tsc2007_stop(ts);
 
+	/* power down the chip (TSC2007_SETUP does not ACK on I2C) */
+	err = tsc2007_xfer(ts, PWRDOWN);
+	if (err < 0) {
+		dev_err(&client->dev,
+			"Failed to setup chip: %d\n", err);
+		iio_device_unregister(indio_dev);
+		return err;	/* usually, chip does not respond */
+	}
+
 	err = input_register_device(input_dev);
 	if (err) {
 		dev_err(&client->dev,
-- 
2.7.3

[PATCH v3 5/8] DT:omap3+tsc2007: use new common touchscreen bindings

[H. Nikolaus Schaller]

While we fix the GTA04 we add proper pinmux for the
penirq gpio.

Tested on: GTA04A4 and Pyra-Handheld

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index c09a057..40d1459 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -273,6 +273,13 @@
 			OMAP3_CORE1_IOPAD(0x2134, PIN_INPUT_PULLUP | MUX_MODE4) /* gpio112 */
 		>;
 	};
+
+	penirq_pins: pinmux_penirq_pins {
+		pinctrl-single,pins = <
+			/* here we could enable to wakeup the cpu from suspend by a pen touch */
+			OMAP3_CORE1_IOPAD(0x2194, PIN_INPUT_PULLUP | MUX_MODE4) /* gpio160 */
+		>;
+	};
 };
 
 &omap3_pmx_core2 {
@@ -410,10 +417,24 @@
 	tsc2007@48 {
 		compatible = "ti,tsc2007";
 		reg = <0x48>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&penirq_pins>;
 		interrupt-parent = <&gpio6>;
 		interrupts = <0 IRQ_TYPE_EDGE_FALLING>; /* GPIO_160 */
-		gpios = <&gpio6 0 GPIO_ACTIVE_LOW>;
-		ti,x-plate-ohms = <600>;
+		gpios = <&gpio6 0 GPIO_ACTIVE_LOW>;	/* GPIO_160 */
+		touchscreen-size-x = <480>;
+		touchscreen-size-y = <640>;
+		touchscreen-max-pressure = <1000>;
+		touchscreen-fuzz-x = <3>;
+		touchscreen-fuzz-y = <8>;
+		touchscreen-fuzz-pressure = <10>;
+		touchscreen-inverted-y;
+		ti,min-x = <0x100>;
+		ti,max-x = <0xf00>;
+		ti,min-y = <0x100>;
+		ti,max-y = <0xf00>;
+		ti,max-rt = <4096>;
+		ti,x-plate-ohms = <550>;
 	};
 
 	/* RFID EEPROM */
-- 
2.7.3

[PATCH v3 5/8] DT:omap3+tsc2007: use new common touchscreen bindings

[H. Nikolaus Schaller]

While we fix the GTA04 we add proper pinmux for the
penirq gpio.

Tested on: GTA04A4 and Pyra-Handheld

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index c09a057..40d1459 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -273,6 +273,13 @@
 			OMAP3_CORE1_IOPAD(0x2134, PIN_INPUT_PULLUP | MUX_MODE4) /* gpio112 */
 		>;
 	};
+
+	penirq_pins: pinmux_penirq_pins {
+		pinctrl-single,pins = <
+			/* here we could enable to wakeup the cpu from suspend by a pen touch */
+			OMAP3_CORE1_IOPAD(0x2194, PIN_INPUT_PULLUP | MUX_MODE4) /* gpio160 */
+		>;
+	};
 };
 
 &omap3_pmx_core2 {
@@ -410,10 +417,24 @@
 	tsc2007@48 {
 		compatible = "ti,tsc2007";
 		reg = <0x48>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&penirq_pins>;
 		interrupt-parent = <&gpio6>;
 		interrupts = <0 IRQ_TYPE_EDGE_FALLING>; /* GPIO_160 */
-		gpios = <&gpio6 0 GPIO_ACTIVE_LOW>;
-		ti,x-plate-ohms = <600>;
+		gpios = <&gpio6 0 GPIO_ACTIVE_LOW>;	/* GPIO_160 */
+		touchscreen-size-x = <480>;
+		touchscreen-size-y = <640>;
+		touchscreen-max-pressure = <1000>;
+		touchscreen-fuzz-x = <3>;
+		touchscreen-fuzz-y = <8>;
+		touchscreen-fuzz-pressure = <10>;
+		touchscreen-inverted-y;
+		ti,min-x = <0x100>;
+		ti,max-x = <0xf00>;
+		ti,min-y = <0x100>;
+		ti,max-y = <0xf00>;
+		ti,max-rt = <4096>;
+		ti,x-plate-ohms = <550>;
 	};
 
 	/* RFID EEPROM */
-- 
2.7.3

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

[PATCH v3 6/8] drivers:input:ads7846(+tsc2046): add new common binding names, pre-calibration and flipping

[H. Nikolaus Schaller]

commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
introduced common DT bindings for touchscreens [1] and a helper function to
parse the DT.

commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
added another helper for parsing axis inversion and swapping
and applying them to x and y coordinates.

Both helpers have been integrated to accommodate any orientation of the
touch panel in relation to the LCD.

A new feature is to introduce scaling the min/max ADC values to the screen
size.

This makes it possible to pre-calibrate the touch so that is (almost)
exactly matches the LCD pixel coordinates it is glued onto. This allows to
well enough operate the touch before a user space calibration step can
improve the precision.

[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/input/ads7846.txt          |  9 +++-
 drivers/input/touchscreen/ads7846.c                | 60 ++++++++++++++++++----
 2 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/ads7846.txt b/Documentation/devicetree/bindings/input/ads7846.txt
index 9fc47b0..29f91ed 100644
--- a/Documentation/devicetree/bindings/input/ads7846.txt
+++ b/Documentation/devicetree/bindings/input/ads7846.txt
@@ -26,6 +26,12 @@ Additional required properties:
 
 Optional properties:
 
+You can optionally specify any of the touchscreen parameters described in
+
+	Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
+
+This allows to scale, invert or swap coordinates and define the fuzz factors.
+
 	ti,vref-delay-usecs		vref supply delay in usecs, 0 for
 					external vref (u16).
 	ti,vref-mv			The VREF voltage, in millivolts (u16).
@@ -33,7 +39,7 @@ Optional properties:
 					(ADS7846).
 	ti,keep-vref-on			set to keep vref on for differential
 					measurements as well
-	ti,swap-xy			swap x and y axis
+	ti,swap-xy			deprecated name for touchscreen-swapped-x-y
 	ti,settle-delay-usec		Settling time of the analog signals;
 					a function of Vcc and the capacitance
 					on the X/Y drivers.  If set to non-zero,
@@ -82,6 +88,7 @@ Example for a TSC2046 chip connected to an McSPI controller of an OMAP SoC::
 			pendown-gpio = <&gpio1 8 0>;
 			vcc-supply = <&reg_vcc3>;
 
+			touchscreen-swapped-x-y;
 			ti,x-min = /bits/ 16 <0>;
 			ti,x-max = /bits/ 16 <8000>;
 			ti,y-min = /bits/ 16 <0>;
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 1ce3ecb..400e421 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -34,6 +34,7 @@
 #include <linux/spi/ads7846.h>
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
+#include <linux/input/touchscreen.h>
 #include <asm/irq.h>
 
 /*
@@ -109,8 +110,13 @@ struct ads7846 {
 	u16			vref_delay_usecs;
 	u16			x_plate_ohms;
 	u16			pressure_max;
+	u16			x_min;
+	u16			x_max;
+	u16			y_min;
+	u16			y_max;
+
+	struct touchscreen_properties prop;
 
-	bool			swap_xy;
 	bool			use_internal;
 
 	struct ads7846_packet	*packet;
@@ -825,22 +831,36 @@ static void ads7846_report_state(struct ads7846 *ts)
 	 */
 	if (Rt) {
 		struct input_dev *input = ts->input;
+		int sx, sy;
+
+		dev_dbg(&ts->spi->dev,
+			"Raw point(%4d,%4d), pressure (%4u)\n",
+				x, y, Rt);
+
+		/* scale ADC values to desired output range */
+		sx = (ts->prop.max_x * (x - ts->x_min))
+			/ (ts->x_max - ts->x_min);
+		sy = (ts->prop.max_y * (y - ts->y_min))
+			/ (ts->y_max - ts->y_min);
 
-		if (ts->swap_xy)
-			swap(x, y);
+		dev_dbg(&ts->spi->dev,
+			"Scaled point(%4d,%4d), pressure (%4u)\n",
+				sx, sy, Rt);
 
+		/* report event */
 		if (!ts->pendown) {
 			input_report_key(input, BTN_TOUCH, 1);
 			ts->pendown = true;
 			dev_vdbg(&ts->spi->dev, "DOWN\n");
 		}
 
-		input_report_abs(input, ABS_X, x);
-		input_report_abs(input, ABS_Y, y);
+		touchscreen_report_pos(ts->input, &ts->prop,
+				       (unsigned int) sx, (unsigned int) sy,
+				       false);
 		input_report_abs(input, ABS_PRESSURE, ts->pressure_max - Rt);
 
 		input_sync(input);
-		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", x, y, Rt);
+		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", sx, sy, Rt);
 	}
 }
 
@@ -1212,6 +1232,8 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
 	pdata->keep_vref_on = of_property_read_bool(node, "ti,keep-vref-on");
 
 	pdata->swap_xy = of_property_read_bool(node, "ti,swap-xy");
+	if (pdata->swap_xy)
+		dev_notice(dev, "please update device tree to use touchscreen-swapped-x-y");
 
 	of_property_read_u16(node, "ti,settle-delay-usec",
 			     &pdata->settle_delay_usecs);
@@ -1315,7 +1337,6 @@ static int ads7846_probe(struct spi_device *spi)
 	ts->pressure_max = pdata->pressure_max ? : ~0;
 
 	ts->vref_mv = pdata->vref_mv;
-	ts->swap_xy = pdata->swap_xy;
 
 	if (pdata->filter != NULL) {
 		if (pdata->filter_init != NULL) {
@@ -1355,18 +1376,35 @@ static int ads7846_probe(struct spi_device *spi)
 	input_dev->dev.parent = &spi->dev;
 
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input_dev->absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) |
+				BIT_MASK(ABS_PRESSURE);
 	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	ts->x_min = pdata->x_min ? : 0;
+	ts->x_max = pdata->x_max ? : MAX_12BIT;
+	ts->y_min = pdata->y_min ? : 0;
+	ts->y_max = pdata->y_max ? : MAX_12BIT;
+
 	input_set_abs_params(input_dev, ABS_X,
-			pdata->x_min ? : 0,
-			pdata->x_max ? : MAX_12BIT,
+			ts->x_min,
+			ts->x_max,
 			0, 0);
 	input_set_abs_params(input_dev, ABS_Y,
-			pdata->y_min ? : 0,
-			pdata->y_max ? : MAX_12BIT,
+			ts->y_min,
+			ts->y_max,
 			0, 0);
 	input_set_abs_params(input_dev, ABS_PRESSURE,
 			pdata->pressure_min, pdata->pressure_max, 0, 0);
 
+	if (spi->dev.of_node) {
+		input_abs_set_min(input_dev, ABS_X, 0);
+		input_abs_set_min(input_dev, ABS_Y, 0);
+
+		touchscreen_parse_properties(ts->input, false, &ts->prop);
+	}
+
+	ts->prop.swap_x_y |= pdata->swap_xy;
+
 	ads7846_setup_spi_msg(ts, pdata);
 
 	ts->reg = regulator_get(&spi->dev, "vcc");
-- 
2.7.3

Re: [PATCH v3 6/8] drivers:input:ads7846(+tsc2046): add new common binding names, pre-calibration and flipping

[Rob Herring]

On Fri, Sep 23, 2016 at 02:41:14PM +0200, H. Nikolaus Schaller wrote:
> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
> introduced common DT bindings for touchscreens [1] and a helper function to
> parse the DT.
> 
> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
> added another helper for parsing axis inversion and swapping
> and applying them to x and y coordinates.
> 
> Both helpers have been integrated to accommodate any orientation of the
> touch panel in relation to the LCD.
> 
> A new feature is to introduce scaling the min/max ADC values to the screen
> size.
> 
> This makes it possible to pre-calibrate the touch so that is (almost)
> exactly matches the LCD pixel coordinates it is glued onto. This allows to
> well enough operate the touch before a user space calibration step can
> improve the precision.

This appears to be the prior patch's commit msg, or you don't need ADC 
values in DT for this one?

> 
> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/input/ads7846.txt          |  9 +++-
>  drivers/input/touchscreen/ads7846.c                | 60 ++++++++++++++++++----
>  2 files changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/ads7846.txt b/Documentation/devicetree/bindings/input/ads7846.txt
> index 9fc47b0..29f91ed 100644
> --- a/Documentation/devicetree/bindings/input/ads7846.txt
> +++ b/Documentation/devicetree/bindings/input/ads7846.txt
> @@ -26,6 +26,12 @@ Additional required properties:
>  
>  Optional properties:
>  
> +You can optionally specify any of the touchscreen parameters described in
> +
> +	Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> +
> +This allows to scale, invert or swap coordinates and define the fuzz factors.
> +
>  	ti,vref-delay-usecs		vref supply delay in usecs, 0 for
>  					external vref (u16).
>  	ti,vref-mv			The VREF voltage, in millivolts (u16).
> @@ -33,7 +39,7 @@ Optional properties:
>  					(ADS7846).
>  	ti,keep-vref-on			set to keep vref on for differential
>  					measurements as well
> -	ti,swap-xy			swap x and y axis
> +	ti,swap-xy			deprecated name for touchscreen-swapped-x-y
>  	ti,settle-delay-usec		Settling time of the analog signals;
>  					a function of Vcc and the capacitance
>  					on the X/Y drivers.  If set to non-zero,
> @@ -82,6 +88,7 @@ Example for a TSC2046 chip connected to an McSPI controller of an OMAP SoC::
>  			pendown-gpio = <&gpio1 8 0>;
>  			vcc-supply = <&reg_vcc3>;
>  
> +			touchscreen-swapped-x-y;
>  			ti,x-min = /bits/ 16 <0>;
>  			ti,x-max = /bits/ 16 <8000>;
>  			ti,y-min = /bits/ 16 <0>;
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 1ce3ecb..400e421 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -34,6 +34,7 @@
>  #include <linux/spi/ads7846.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/module.h>
> +#include <linux/input/touchscreen.h>
>  #include <asm/irq.h>
>  
>  /*
> @@ -109,8 +110,13 @@ struct ads7846 {
>  	u16			vref_delay_usecs;
>  	u16			x_plate_ohms;
>  	u16			pressure_max;
> +	u16			x_min;
> +	u16			x_max;
> +	u16			y_min;
> +	u16			y_max;
> +
> +	struct touchscreen_properties prop;
>  
> -	bool			swap_xy;
>  	bool			use_internal;
>  
>  	struct ads7846_packet	*packet;
> @@ -825,22 +831,36 @@ static void ads7846_report_state(struct ads7846 *ts)
>  	 */
>  	if (Rt) {
>  		struct input_dev *input = ts->input;
> +		int sx, sy;
> +
> +		dev_dbg(&ts->spi->dev,
> +			"Raw point(%4d,%4d), pressure (%4u)\n",
> +				x, y, Rt);
> +
> +		/* scale ADC values to desired output range */
> +		sx = (ts->prop.max_x * (x - ts->x_min))
> +			/ (ts->x_max - ts->x_min);
> +		sy = (ts->prop.max_y * (y - ts->y_min))
> +			/ (ts->y_max - ts->y_min);
>  
> -		if (ts->swap_xy)
> -			swap(x, y);
> +		dev_dbg(&ts->spi->dev,
> +			"Scaled point(%4d,%4d), pressure (%4u)\n",
> +				sx, sy, Rt);
>  
> +		/* report event */
>  		if (!ts->pendown) {
>  			input_report_key(input, BTN_TOUCH, 1);
>  			ts->pendown = true;
>  			dev_vdbg(&ts->spi->dev, "DOWN\n");
>  		}
>  
> -		input_report_abs(input, ABS_X, x);
> -		input_report_abs(input, ABS_Y, y);
> +		touchscreen_report_pos(ts->input, &ts->prop,
> +				       (unsigned int) sx, (unsigned int) sy,
> +				       false);
>  		input_report_abs(input, ABS_PRESSURE, ts->pressure_max - Rt);
>  
>  		input_sync(input);
> -		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", x, y, Rt);
> +		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", sx, sy, Rt);
>  	}
>  }
>  
> @@ -1212,6 +1232,8 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
>  	pdata->keep_vref_on = of_property_read_bool(node, "ti,keep-vref-on");
>  
>  	pdata->swap_xy = of_property_read_bool(node, "ti,swap-xy");
> +	if (pdata->swap_xy)
> +		dev_notice(dev, "please update device tree to use touchscreen-swapped-x-y");
>  
>  	of_property_read_u16(node, "ti,settle-delay-usec",
>  			     &pdata->settle_delay_usecs);
> @@ -1315,7 +1337,6 @@ static int ads7846_probe(struct spi_device *spi)
>  	ts->pressure_max = pdata->pressure_max ? : ~0;
>  
>  	ts->vref_mv = pdata->vref_mv;
> -	ts->swap_xy = pdata->swap_xy;
>  
>  	if (pdata->filter != NULL) {
>  		if (pdata->filter_init != NULL) {
> @@ -1355,18 +1376,35 @@ static int ads7846_probe(struct spi_device *spi)
>  	input_dev->dev.parent = &spi->dev;
>  
>  	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	input_dev->absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) |
> +				BIT_MASK(ABS_PRESSURE);
>  	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	ts->x_min = pdata->x_min ? : 0;
> +	ts->x_max = pdata->x_max ? : MAX_12BIT;
> +	ts->y_min = pdata->y_min ? : 0;
> +	ts->y_max = pdata->y_max ? : MAX_12BIT;
> +
>  	input_set_abs_params(input_dev, ABS_X,
> -			pdata->x_min ? : 0,
> -			pdata->x_max ? : MAX_12BIT,
> +			ts->x_min,
> +			ts->x_max,
>  			0, 0);
>  	input_set_abs_params(input_dev, ABS_Y,
> -			pdata->y_min ? : 0,
> -			pdata->y_max ? : MAX_12BIT,
> +			ts->y_min,
> +			ts->y_max,
>  			0, 0);
>  	input_set_abs_params(input_dev, ABS_PRESSURE,
>  			pdata->pressure_min, pdata->pressure_max, 0, 0);
>  
> +	if (spi->dev.of_node) {
> +		input_abs_set_min(input_dev, ABS_X, 0);
> +		input_abs_set_min(input_dev, ABS_Y, 0);
> +
> +		touchscreen_parse_properties(ts->input, false, &ts->prop);
> +	}
> +
> +	ts->prop.swap_x_y |= pdata->swap_xy;
> +
>  	ads7846_setup_spi_msg(ts, pdata);
>  
>  	ts->reg = regulator_get(&spi->dev, "vcc");
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 7/8] drivers:input:ads7846(+tsc2046): fix spi module table

[H. Nikolaus Schaller]

Fix module table so that the driver is loaded if compiled
as module and requested by DT.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/input/touchscreen/ads7846.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 400e421..9a5d84d 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -1532,6 +1532,17 @@ static int ads7846_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct spi_device_id ads7846_idtable[] = {
+	{ "tsc2046", 0 },
+	{ "ads7843", 0 },
+	{ "ads7845", 0 },
+	{ "ads7846", 0 },
+	{ "ads7873", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(spi, ads7846_idtable);
+
 static struct spi_driver ads7846_driver = {
 	.driver = {
 		.name	= "ads7846",
-- 
2.7.3

[PATCH v3 8/8] DT:omap3+ads7846: use new common touchscreen bindings

[H. Nikolaus Schaller]

The standard touch screen bindings [1] replace the private ti,swap-xy
with touchscreen-swaped-x-y. And for the Openpandora we use
touchscreen-size etc. to match the LCD screen size.

[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Tested with OpenPandora.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap3-lilly-a83x.dtsi              |  2 +-
 arch/arm/boot/dts/omap3-pandora-common.dtsi          | 17 +++++++++++++----
 arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi |  3 ++-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-lilly-a83x.dtsi b/arch/arm/boot/dts/omap3-lilly-a83x.dtsi
index eff816e..af6b66c 100644
--- a/arch/arm/boot/dts/omap3-lilly-a83x.dtsi
+++ b/arch/arm/boot/dts/omap3-lilly-a83x.dtsi
@@ -325,7 +325,7 @@
 		ti,y-max = /bits/ 16 <3600>;
 		ti,x-plate-ohms = /bits/ 16 <80>;
 		ti,pressure-max = /bits/ 16 <255>;
-		ti,swap-xy;
+		touchscreen-swapped-x-y;
 
 		wakeup-source;
 	};
diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi b/arch/arm/boot/dts/omap3-pandora-common.dtsi
index cbc47af..f66a2b4 100644
--- a/arch/arm/boot/dts/omap3-pandora-common.dtsi
+++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi
@@ -700,10 +700,19 @@
 		pendown-gpio = <&gpio3 30 GPIO_ACTIVE_HIGH>;
 		vcc-supply = <&vaux4>;
 
-		ti,x-min = /bits/ 16 <0>;
-		ti,x-max = /bits/ 16 <8000>;
-		ti,y-min = /bits/ 16 <0>;
-		ti,y-max = /bits/ 16 <4800>;
+		touchscreen-size-x = <800>;
+		touchscreen-size-y = <480>;
+		touchscreen-max-pressure = <1000>;
+		touchscreen-fuzz-x = <16>;
+		touchscreen-fuzz-y = <16>;
+		touchscreen-fuzz-pressure = <10>;
+		touchscreen-inverted-x;
+		touchscreen-inverted-y;
+
+		ti,x-min = /bits/ 16 <160>;
+		ti,x-max = /bits/ 16 <3900>;
+		ti,y-min = /bits/ 16 <220>;
+		ti,y-max = /bits/ 16 <3750>;
 		ti,x-plate-ohms = /bits/ 16 <40>;
 		ti,pressure-max = /bits/ 16 <255>;
 
diff --git a/arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi b/arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi
index 157345b..3627a63 100644
--- a/arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi
+++ b/arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi
@@ -66,6 +66,7 @@
 		ti,x-plate-ohms = /bits/ 16 <40>;
 		ti,pressure-max = /bits/ 16 <255>;
 		ti,swap-xy;
-		wakeup-source;
+		touchscreen-swapped-x-y;
+		linux,wakeup;
 	};
 };
-- 
2.7.3