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

[PATCH v4 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]

Changes V4:
* fix a merge/squash issue resulting in a non-bisectable patch set (suggested by kbuid test robot)
* remove some unnecessary #include (suggested by Jonathan Cameron <jic23@kernel.org>)
* make the iio extension depend on CONFIG_IIO rather than selecting it (suggested by Jonathan Cameron <jic23@kernel.org>)
* swapped patch 3/n and patch 4/n to remove internal dependency

2016-09-23 14:41:23: 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: check for presence and power down tsc2007
    during probe
  drivers:input:tsc2007: add iio interface to read external ADC input
    and temperature
  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/ads7846.c                |  71 ++++-
 drivers/input/touchscreen/tsc2007.c                | 297 +++++++++++++++++++--
 include/linux/i2c/tsc2007.h                        |   8 +
 9 files changed, 397 insertions(+), 55 deletions(-)

-- 
2.7.3

[PATCH v4 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                | 120 +++++++++++++++++----
 include/linux/i2c/tsc2007.h                        |   8 ++
 3 files changed, 118 insertions(+), 30 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..c1d9593 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,41 @@ 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);
 
+			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 */
 			input_report_key(input, BTN_TOUCH, 1);
-			input_report_abs(input, ABS_X, tc.x);
-			input_report_abs(input, ABS_Y, tc.y);
+			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 +264,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 +335,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 +374,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 +407,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 +454,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 +478,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 +515,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

[PATCH v4 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index c1d9593..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);
@@ -227,7 +228,11 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 				sx, sy, rt);
 
 			/* report event */
-			input_report_key(input, BTN_TOUCH, 1);
+			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,
@@ -250,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 v4 3/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 e9d5086..c314331 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -538,6 +538,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 v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

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

Since it is not wasting much resources and is very straightforward,
we simply provide all other adc channels as optional iio interfaces
as weel. This can be used for debugging or special applications.

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

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index c314331..b4a9504 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
@@ -61,6 +62,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 +80,13 @@ struct ts_event {
 
 struct tsc2007 {
 	struct input_dev	*input;
+#ifdef CONFIG_IIO
+	struct iio_dev		*indio;
+#endif
 	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,89 @@ static void tsc2007_close(struct input_dev *input_dev)
 	tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+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,
+};
+#endif
+
 #ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
@@ -453,26 +554,56 @@ 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;
+#ifdef CONFIG_IIO
+	struct iio_dev *indio_dev;
+#endif
 	int err;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -EIO;
 
+#ifdef CONFIG_IIO
+	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);
+#else
 	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
 	if (!ts)
 		return -ENOMEM;
-
+#endif
 	input_dev = devm_input_allocate_device(&client->dev);
 	if (!input_dev)
 		return -ENOMEM;
 
 	i2c_set_clientdata(client, ts);
 
+#ifdef CONFIG_IIO
+	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->indio = indio_dev;
+#endif
+
 	ts->client = client;
 	ts->irq = client->irq;
 	ts->input = input_dev;
 	init_waitqueue_head(&ts->wait);
+	mutex_init(&ts->mlock);
 
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));
@@ -543,7 +674,9 @@ static int tsc2007_probe(struct i2c_client *client,
 	if (err < 0) {
 		dev_err(&client->dev,
 			"Failed to setup chip: %d\n", err);
+#ifdef CONFIG_IIO
 		iio_device_unregister(indio_dev);
+#endif
 		return err;	/* usually, chip does not respond */
 	}
 
@@ -557,6 +690,21 @@ 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;
+#ifdef CONFIG_IIO
+	struct iio_dev *indio_dev = ts->indio;
+#endif
+	input_unregister_device(input_dev);
+
+#ifdef CONFIG_IIO
+	iio_device_unregister(indio_dev);
+#endif
+	return 0;
+}
+
 static const struct i2c_device_id tsc2007_idtable[] = {
 	{ "tsc2007", 0 },
 	{ }
@@ -579,6 +727,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 v4 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 b3a8b1f..64d6ee3 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 v4 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

[PATCH v4 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 v4 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 fa611a5..b8b3864 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 b0d1551..d12008a 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

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

[Andrew F. Davis]

On 10/17/2016 08:57 AM, H. Nikolaus Schaller wrote:
> 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 },
> +	{ }
> +};
> +

no space needed

> +MODULE_DEVICE_TABLE(spi, ads7846_idtable);
> +
>  static struct spi_driver ads7846_driver = {
>  	.driver = {
>  		.name	= "ads7846",
> 

the MODULE_ALIAS below can now also be removed with this patch.

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

[H. Nikolaus Schaller]

> Am 17.10.2016 um 17:54 schrieb Andrew F. Davis <afd@ti.com>:
> 
> On 10/17/2016 08:57 AM, H. Nikolaus Schaller wrote:
>> 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 },
>> +	{ }
>> +};
>> +
> 
> no space needed
> 
>> +MODULE_DEVICE_TABLE(spi, ads7846_idtable);
>> +
>> static struct spi_driver ads7846_driver = {
>> 	.driver = {
>> 		.name	= "ads7846",
>> 
> 
> the MODULE_ALIAS below can now also be removed with this patch.

Ok! Noted for V5.

BR and thanks,
Nikolaus

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

[Rob Herring]

On Mon, Oct 17, 2016 at 8:57 AM, H. Nikolaus Schaller <hns@goldelico.com> 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.

Please add the explanation of why this is a compatible change here.

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

Sounds like a separate change.

> 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                | 120 +++++++++++++++++----
>  include/linux/i2c/tsc2007.h                        |   8 ++
>  3 files changed, 118 insertions(+), 30 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).

I thought these were going to be common? I think they should be.

Rob

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

[H. Nikolaus Schaller]

Hi Rob,

> Am 18.10.2016 um 18:22 schrieb Rob Herring <robh+dt@kernel.org>:
> 
> On Mon, Oct 17, 2016 at 8:57 AM, H. Nikolaus Schaller <hns@goldelico.com> 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.
> 
> Please add the explanation of why this is a compatible change here.

Can you please describe in more detail what you are missing here?

The patch simply makes use of the generic helper function introduced by the
two patches cited above and is therefore automatically compatible if none of
the new properties is defined.

The tsc2007 didn't have these features before. So what should we say
about compatibility?

> 
>> A new feature is to introduce scaling the min/max ADC values to the screen
>> size.
> 
> Sounds like a separate change.

I have thought about that for a while, but did not find a way to separate it
into two independent patches. If you can propose something, please help.

> 
>> 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                | 120 +++++++++++++++++----
>> include/linux/i2c/tsc2007.h                        |   8 ++
>> 3 files changed, 118 insertions(+), 30 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).
> 
> I thought these were going to be common? I think they should be.

Yes, we had discussed that before.

In a second thought, I found that they are not even existing for the different
touch chip drivers that are around. And some chip might not need them (because
e.g. min/max are constants or defined my means outside the DT or the chip itself
can store them in NV memory).

And, since they define ADC raw-values, they are very closely related to the
individual chip, so that I am not sure if it is really necessary to make
them common. At least if they are properly documented in the bindings for
the specific chip they can have different names without disturbing each
other.

So I think it is perfectly reasonable to define common bindings towards
the input event layer (e.g. size, fuzz, swapping, inversion) and support
them by common code which bakes raw coordinates into input events. The latest
helper functions already do most of that fully automatic.

Next, I had simply copied them from the ads7846 driver where they
exist for a long time. So this is sort of "common", at least for two different
chips now. See also patch 6/8 of this series which adds the common properties
to the ads7846 as well.

For more reference about the existing bindings:

Documentation/devicetree/bindings/input/ads7846.txt

(btw I think someone should move them to bindings/input/touchscreen).

It appears as if there is some overlap of the new generic properties (for x/y
swapping) with the old ads7846 properties, but that is something you had
already proposed a while ago to make me the driver recognize both properties to
stay compatible with older DT files.

So if we now rename the min/max-x/y properties for the tsc2007 we have to rename
them for the ads7846 (and maybe others) as well, which might break out-of-tree
ads7846 devices or leads to more complex code for handling of property aliases.

Does this sound enough reasons to keep it as proposed originally?

BR and thanks,
Nikolaus


> 
> Rob
> 
>> - 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 v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 17/10/16 14:57, 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.
> 
> Since it is not wasting much resources and is very straightforward,
> we simply provide all other adc channels as optional iio interfaces
> as weel. This can be used for debugging or special applications.
well
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
This could be cleaner done perhaps by factoring out the IIO stuff into a separate
file and using a header with stubs to deal with the no available case.

There will only be a handful of stubs and it'll give you a lot cleaner code
in here.

If def fun in .c files is always harder to deal with than in a header
where stubs are really obvious.

J
> ---
>  drivers/input/touchscreen/tsc2007.c | 151 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index c314331..b4a9504 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/iio/iio.h>
>  
>  #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>  #define TSC2007_MEASURE_AUX		(0x2 << 4)
> @@ -61,6 +62,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 +80,13 @@ struct ts_event {
>  
>  struct tsc2007 {
>  	struct input_dev	*input;
> +#ifdef CONFIG_IIO
> +	struct iio_dev		*indio;
> +#endif
>  	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,89 @@ static void tsc2007_close(struct input_dev *input_dev)
>  	tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +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,
> +};
> +#endif
> +
>  #ifdef CONFIG_OF
>  static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  {
> @@ -453,26 +554,56 @@ 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;
> +#ifdef CONFIG_IIO
> +	struct iio_dev *indio_dev;
> +#endif
>  	int err;
>  
>  	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>  		return -EIO;
>  
> +#ifdef CONFIG_IIO
> +	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);
> +#else
>  	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>  	if (!ts)
>  		return -ENOMEM;
> -
> +#endif
>  	input_dev = devm_input_allocate_device(&client->dev);
>  	if (!input_dev)
>  		return -ENOMEM;
>  
>  	i2c_set_clientdata(client, ts);
>  
> +#ifdef CONFIG_IIO
> +	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->indio = indio_dev;
> +#endif
> +
>  	ts->client = client;
>  	ts->irq = client->irq;
>  	ts->input = input_dev;
>  	init_waitqueue_head(&ts->wait);
> +	mutex_init(&ts->mlock);
>  
>  	snprintf(ts->phys, sizeof(ts->phys),
>  		 "%s/input0", dev_name(&client->dev));
> @@ -543,7 +674,9 @@ static int tsc2007_probe(struct i2c_client *client,
>  	if (err < 0) {
>  		dev_err(&client->dev,
>  			"Failed to setup chip: %d\n", err);
> +#ifdef CONFIG_IIO
>  		iio_device_unregister(indio_dev);
> +#endif
>  		return err;	/* usually, chip does not respond */
>  	}
>  
> @@ -557,6 +690,21 @@ 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;
> +#ifdef CONFIG_IIO
> +	struct iio_dev *indio_dev = ts->indio;
> +#endif
> +	input_unregister_device(input_dev);
> +
> +#ifdef CONFIG_IIO
> +	iio_device_unregister(indio_dev);
> +#endif
> +	return 0;
> +}
> +
>  static const struct i2c_device_id tsc2007_idtable[] = {
>  	{ "tsc2007", 0 },
>  	{ }
> @@ -579,6 +727,7 @@ static struct i2c_driver tsc2007_driver = {
>  	},
>  	.id_table	= tsc2007_idtable,
>  	.probe		= tsc2007_probe,
> +	.remove		= tsc2007_remove,
>  };
>  
>  module_i2c_driver(tsc2007_driver);
> 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 17/10/16 14:57, 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.
>> 
>> Since it is not wasting much resources and is very straightforward,
>> we simply provide all other adc channels as optional iio interfaces
>> as weel. This can be used for debugging or special applications.
> well
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> This could be cleaner done perhaps by factoring out the IIO stuff into a separate
> file and using a header with stubs to deal with the no available case.
> 
> There will only be a handful of stubs and it'll give you a lot cleaner code
> in here.
> 
> If def fun in .c files is always harder to deal with than in a header
> where stubs are really obvious.

Yes, it became a lot of #ifdefs spread over the source file.

The easiest thing would be to require IIO to be enabled :)

With your proposal to consider refactoring, I think the crucial part
is the conditional allocation either through devm_iio_device_alloc()
or devm_kzalloc(). This can be refactored into some conditional
tsc2007_alloc().

I have tried some draft (not tested and not tidied up) to check if the
direction is good.

This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
seem to be very complex now in comparison. And the patch itself has only a
handful of hunks (8).

Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to understand
because it is not really iio specific and one has to switch between two source
files. And I would have to touch the Makefile as well.

What do you think?

If generally ok, I can include that in [PATCH v5].

BR and thanks,
Nikolaus


diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..691e79f 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>
 
 #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
 #define TSC2007_MEASURE_AUX            (0x2 << 4)
@@ -69,9 +70,13 @@ struct ts_event {
 
 struct tsc2007 {
        struct input_dev        *input;
+#ifdef CONFIG_IIO
+       struct iio_dev          *indio;
+#endif
        char                    phys[32];
 
        struct i2c_client       *client;
+       struct mutex            mlock;
 
        u16                     model;
        u16                     x_plate_ohms;
@@ -192,7 +197,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 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
        tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+#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, \
+}
+
+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,
+};
+
+static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
+                          struct input_dev **input_dev)
+{
+       int err;
+       struct iio_dev *indio_dev;
+
+       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)
+               return -ENOMEM;
+
+       i2c_set_clientdata(client, *ts);
+       (*ts)->indio = indio_dev;
+
+       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;
+       }
+
+       return 0;
+}
+
+#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
+
+#else /* CONFIG_IIO */
+
+static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
+                          struct input_dev **input_dev)
+{
+       int err;
+
+       *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
+       if (!*ts)
+               return -ENOMEM;
+
+       *input_dev = devm_input_allocate_device(&client->dev);
+       if (!*input_dev)
+               return -ENOMEM;
+
+       i2c_set_clientdata(client, *ts);
+
+       return 0;
+}
+
+#define tsc2007_iio_device_unregister(ts) /* not needed */
+
+#endif /* CONFIG_IIO */
+
 #ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
@@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
                                     I2C_FUNC_SMBUS_READ_WORD_DATA))
                return -EIO;
 
-       ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
-       if (!ts)
-               return -ENOMEM;
-
-       input_dev = devm_input_allocate_device(&client->dev);
-       if (!input_dev)
-               return -ENOMEM;
-
-       i2c_set_clientdata(client, ts);
+       err = tsc2007_alloc(client, &ts, &input_dev);
+       if (err < 0)
+               return err;
 
        ts->client = client;
        ts->irq = client->irq;
        ts->input = input_dev;
        init_waitqueue_head(&ts->wait);
+       mutex_init(&ts->mlock);
 
        snprintf(ts->phys, sizeof(ts->phys),
                 "%s/input0", dev_name(&client->dev));
@@ -543,6 +702,7 @@ static int tsc2007_probe(struct i2c_client *client,
        if (err < 0) {
                dev_err(&client->dev,
                        "Failed to setup chip: %d\n", err);
+               tsc2007_iio_device_unregister(ts);
                return err;     /* usually, chip does not respond */
        }
 
@@ -556,6 +716,14 @@ 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);
+       input_unregister_device(ts->input);
+       tsc2007_iio_device_unregister(ts);
+       return 0;
+}
+
 static const struct i2c_device_id tsc2007_idtable[] = {
        { "tsc2007", 0 },
        { }
@@ -578,6 +746,7 @@ static struct i2c_driver tsc2007_driver = {
        },
        .id_table       = tsc2007_idtable,
        .probe          = tsc2007_probe,
+       .remove         = tsc2007_remove,
 };
 
 module_i2c_driver(tsc2007_driver);

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 22/10/16 21:46, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 17/10/16 14:57, 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.
>>>
>>> Since it is not wasting much resources and is very straightforward,
>>> we simply provide all other adc channels as optional iio interfaces
>>> as weel. This can be used for debugging or special applications.
>> well
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> This could be cleaner done perhaps by factoring out the IIO stuff into a separate
>> file and using a header with stubs to deal with the no available case.
>>
>> There will only be a handful of stubs and it'll give you a lot cleaner code
>> in here.
>>
>> If def fun in .c files is always harder to deal with than in a header
>> where stubs are really obvious.
> 
> Yes, it became a lot of #ifdefs spread over the source file.
> 
> The easiest thing would be to require IIO to be enabled :)
> 
> With your proposal to consider refactoring, I think the crucial part
> is the conditional allocation either through devm_iio_device_alloc()
> or devm_kzalloc(). This can be refactored into some conditional
> tsc2007_alloc().
> 
> I have tried some draft (not tested and not tidied up) to check if the
> direction is good.
> 
> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
> seem to be very complex now in comparison. And the patch itself has only a
> handful of hunks (8).
> 
> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to understand
> because it is not really iio specific and one has to switch between two source
> files. And I would have to touch the Makefile as well.
> 
> What do you think?
I'd still split it.  The only bit of the IIO block that isn't specific is
a tiny chunk of the allocation code (as you've highlighted).

Even that can be avoided by adding a tiny bit more indirection than would
otherwise be needed (it's not pretty but it would give a clean separation).

It's pretty much the way this sort of optional functionality should always
be done - means that if you don't care (i.e. it's not enabled) you don't
even have to see the code.

Jonathan
> 
> If generally ok, I can include that in [PATCH v5].
> 
> BR and thanks,
> Nikolaus
> 
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..691e79f 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/iio/iio.h>
>  
>  #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>  #define TSC2007_MEASURE_AUX            (0x2 << 4)
> @@ -69,9 +70,13 @@ struct ts_event {
>  
>  struct tsc2007 {
>         struct input_dev        *input;
> +#ifdef CONFIG_IIO
> +       struct iio_dev          *indio;
> +#endif
I wouldn't bother with this one.  Just have 
struct iio_dev; before this and it'll waste a whole
one pointer (+ you shouldn't need to have iio.h included
in here once you have spit the files).
>         char                    phys[32];
>  
>         struct i2c_client       *client;
> +       struct mutex            mlock;
>  
>         u16                     model;
>         u16                     x_plate_ohms;
> @@ -192,7 +197,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 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
>         tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +#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, \
> +}
> +
> +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,
> +};
> +
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +                          struct input_dev **input_dev)
> +{
> +       int err;
> +       struct iio_dev *indio_dev;
> +
> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
Instead of doing this to reduce the delta between versions make 
iio_priv a struct tsc2007 **

That is have a single pointer in there and do your allocation of struct
tsc2007 separately.

Having doing that, you can have this CONFIG_IIO block as just
doing the iio stuff with the input elements pulled back into the main
probe function.

Then define something like

iio_configure (stubbed to nothing if no IIO)
and
iio_unconfigure (also stubbed to nothing if no IIO).

A couple of additions in the header to make it all work
(the struct tsc2007 and tsc2007_xfer() + a few of the
register defines..

Nothing big and gets all the CONFIG_IIO into some really
obvious stubbing out in the header.


> +       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)
> +               return -ENOMEM;
> +
> +       i2c_set_clientdata(client, *ts);
> +       (*ts)->indio = indio_dev;
> +
> +       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;
> +       }
> +
> +       return 0;
> +}
> +
> +#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
> +
> +#else /* CONFIG_IIO */
> +
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +                          struct input_dev **input_dev)
> +{
> +       int err;
> +
> +       *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> +       if (!*ts)
> +               return -ENOMEM;
> +
> +       *input_dev = devm_input_allocate_device(&client->dev);
> +       if (!*input_dev)
> +               return -ENOMEM;
> +
> +       i2c_set_clientdata(client, *ts);
> +
> +       return 0;
> +}
> +
> +#define tsc2007_iio_device_unregister(ts) /* not needed */
That's rather ugly and fragile.  I'd stub it out as an actual function
with no content and let the compiler drop it.
> +
> +#endif /* CONFIG_IIO */
> +
>  #ifdef CONFIG_OF
>  static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  {
> @@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
>                                      I2C_FUNC_SMBUS_READ_WORD_DATA))
>                 return -EIO;
>  
> -       ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> -       if (!ts)
> -               return -ENOMEM;
> -
> -       input_dev = devm_input_allocate_device(&client->dev);
> -       if (!input_dev)
> -               return -ENOMEM;
> -
> -       i2c_set_clientdata(client, ts);
> +       err = tsc2007_alloc(client, &ts, &input_dev);
> +       if (err < 0)
> +               return err;
>  
>         ts->client = client;
>         ts->irq = client->irq;
>         ts->input = input_dev;
>         init_waitqueue_head(&ts->wait);
> +       mutex_init(&ts->mlock);
>  
>         snprintf(ts->phys, sizeof(ts->phys),
>                  "%s/input0", dev_name(&client->dev));
> @@ -543,6 +702,7 @@ static int tsc2007_probe(struct i2c_client *client,
>         if (err < 0) {
>                 dev_err(&client->dev,
>                         "Failed to setup chip: %d\n", err);
> +               tsc2007_iio_device_unregister(ts);
>                 return err;     /* usually, chip does not respond */
>         }
>  
> @@ -556,6 +716,14 @@ 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);
> +       input_unregister_device(ts->input);
> +       tsc2007_iio_device_unregister(ts);
> +       return 0;
> +}
> +
>  static const struct i2c_device_id tsc2007_idtable[] = {
>         { "tsc2007", 0 },
>         { }
> @@ -578,6 +746,7 @@ static struct i2c_driver tsc2007_driver = {
>         },
>         .id_table       = tsc2007_idtable,
>         .probe          = tsc2007_probe,
> +       .remove         = tsc2007_remove,
>  };
>  
>  module_i2c_driver(tsc2007_driver);
> 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23@kernel.org>:
>>> 
>>> On 17/10/16 14:57, 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.
>>>> 
>>>> Since it is not wasting much resources and is very straightforward,
>>>> we simply provide all other adc channels as optional iio interfaces
>>>> as weel. This can be used for debugging or special applications.
>>> well
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> This could be cleaner done perhaps by factoring out the IIO stuff into a separate
>>> file and using a header with stubs to deal with the no available case.
>>> 
>>> There will only be a handful of stubs and it'll give you a lot cleaner code
>>> in here.
>>> 
>>> If def fun in .c files is always harder to deal with than in a header
>>> where stubs are really obvious.
>> 
>> Yes, it became a lot of #ifdefs spread over the source file.
>> 
>> The easiest thing would be to require IIO to be enabled :)
>> 
>> With your proposal to consider refactoring, I think the crucial part
>> is the conditional allocation either through devm_iio_device_alloc()
>> or devm_kzalloc(). This can be refactored into some conditional
>> tsc2007_alloc().
>> 
>> I have tried some draft (not tested and not tidied up) to check if the
>> direction is good.
>> 
>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>> seem to be very complex now in comparison. And the patch itself has only a
>> handful of hunks (8).
>> 
>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to understand
>> because it is not really iio specific and one has to switch between two source
>> files. And I would have to touch the Makefile as well.
>> 
>> What do you think?
> I'd still split it.  The only bit of the IIO block that isn't specific is
> a tiny chunk of the allocation code (as you've highlighted).
> 
> Even that can be avoided by adding a tiny bit more indirection than would
> otherwise be needed (it's not pretty but it would give a clean separation).

I hope I understand what you mean (which is an indication that the result
may be much easier to read for you but not me...).

> It's pretty much the way this sort of optional functionality should always
> be done - means that if you don't care (i.e. it's not enabled) you don't
> even have to see the code.

> 
> Jonathan
>> 
>> If generally ok, I can include that in [PATCH v5].
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..691e79f 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/input/touchscreen.h>
>> +#include <linux/iio/iio.h>
>> 
>> #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>> #define TSC2007_MEASURE_AUX            (0x2 << 4)
>> @@ -69,9 +70,13 @@ struct ts_event {
>> 
>> struct tsc2007 {
>>        struct input_dev        *input;
>> +#ifdef CONFIG_IIO
>> +       struct iio_dev          *indio;
>> +#endif
> I wouldn't bother with this one.  Just have 
> struct iio_dev; before this and it'll waste a whole
> one pointer (+ you shouldn't need to have iio.h included
> in here once you have spit the files).

Looks as if I have to make a knot in my brain before I start to understand...

How can I use struct iio_dev here w/o including iio.h?

>>        char                    phys[32];
>> 
>>        struct i2c_client       *client;
>> +       struct mutex            mlock;
>> 
>>        u16                     model;
>>        u16                     x_plate_ohms;
>> @@ -192,7 +197,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 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
>>        tsc2007_stop(ts);
>> }
>> 
>> +#ifdef CONFIG_IIO
>> +
>> +#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, \
>> +}
>> +
>> +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,
>> +};
>> +
>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>> +                          struct input_dev **input_dev)
>> +{
>> +       int err;
>> +       struct iio_dev *indio_dev;
>> +
>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
> Instead of doing this to reduce the delta between versions make 
> iio_priv a struct tsc2007 **
> 
> That is have a single pointer in there and do your allocation of struct
> tsc2007 separately.

Sorry, but I think I do not completely understand what you mean here.

The problem is that we need to allocate some struct tsc2007 in both cases.
But in one case managed directly by &client->dev and in the other managed
indirectly. This is why I use the private area of struct iio_dev to store
the full struct tsc2007 and not just a pointer.

What I mean is:

>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> 	*ts = iio_priv(indio_dev);

vs. 

>> 	 *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);


So how can the IIO case just extend/wrap devm_kzalloc(&client->dev...) and still
be managed as well?

> 
> Having doing that, you can have this CONFIG_IIO block as just
> doing the iio stuff with the input elements pulled back into the main
> probe function.
> 
> Then define something like
> 
> iio_configure (stubbed to nothing if no IIO)
> and
> iio_unconfigure (also stubbed to nothing if no IIO).
> 
> A couple of additions in the header to make it all work
> (the struct tsc2007 and tsc2007_xfer() + a few of the
> register defines..
> 
> Nothing big and gets all the CONFIG_IIO into some really
> obvious stubbing out in the header.


Is there some example driver which is doing it that way to be optionally IIO
compatible? That might be easier to understand and copy than a description.

> 
>> +       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)
>> +               return -ENOMEM;
>> +
>> +       i2c_set_clientdata(client, *ts);
>> +       (*ts)->indio = indio_dev;
>> +
>> +       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;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
>> +
>> +#else /* CONFIG_IIO */
>> +
>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>> +                          struct input_dev **input_dev)
>> +{
>> +       int err;
>> +
>> +       *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>> +       if (!*ts)
>> +               return -ENOMEM;
>> +
>> +       *input_dev = devm_input_allocate_device(&client->dev);
>> +       if (!*input_dev)
>> +               return -ENOMEM;
>> +
>> +       i2c_set_clientdata(client, *ts);
>> +
>> +       return 0;
>> +}
>> +
>> +#define tsc2007_iio_device_unregister(ts) /* not needed */
> That's rather ugly and fragile.  I'd stub it out as an actual function
> with no content and let the compiler drop it.

Well, it is a quick and dirty draft.
Should indeed better be a static (inline) function with empty body.

>> +
>> +#endif /* CONFIG_IIO */
>> +
>> #ifdef CONFIG_OF
>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>> {
>> @@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
>>                                     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>                return -EIO;
>> 
>> -       ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>> -       if (!ts)
>> -               return -ENOMEM;
>> -
>> -       input_dev = devm_input_allocate_device(&client->dev);
>> -       if (!input_dev)
>> -               return -ENOMEM;
>> -
>> -       i2c_set_clientdata(client, ts);
>> +       err = tsc2007_alloc(client, &ts, &input_dev);
>> +       if (err < 0)
>> +               return err;
>> 
>>        ts->client = client;
>>        ts->irq = client->irq;
>>        ts->input = input_dev;
>>        init_waitqueue_head(&ts->wait);
>> +       mutex_init(&ts->mlock);
>> 
>>        snprintf(ts->phys, sizeof(ts->phys),
>>                 "%s/input0", dev_name(&client->dev));
>> @@ -543,6 +702,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>        if (err < 0) {
>>                dev_err(&client->dev,
>>                        "Failed to setup chip: %d\n", err);
>> +               tsc2007_iio_device_unregister(ts);
>>                return err;     /* usually, chip does not respond */
>>        }
>> 
>> @@ -556,6 +716,14 @@ 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);
>> +       input_unregister_device(ts->input);
>> +       tsc2007_iio_device_unregister(ts);
>> +       return 0;
>> +}
>> +
>> static const struct i2c_device_id tsc2007_idtable[] = {
>>        { "tsc2007", 0 },
>>        { }
>> @@ -578,6 +746,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 v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi,
> 
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> +                          struct input_dev **input_dev)
>>> +{
>>> +       int err;
>>> +       struct iio_dev *indio_dev;
>>> +
>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> Instead of doing this to reduce the delta between versions make 
>> iio_priv a struct tsc2007 **
>> 
>> That is have a single pointer in there and do your allocation of struct
>> tsc2007 separately.
> 
> Sorry, but I think I do not completely understand what you mean here.
> 
> The problem is that we need to allocate some struct tsc2007 in both cases.
> But in one case managed directly by &client->dev and in the other managed
> indirectly. This is why I use the private area of struct iio_dev to store
> the full struct tsc2007 and not just a pointer.

Ok, I think I finally did understand how you mean this and have started to
implement something.

The idea is to have one alloc function to return a struct tsc2007. This
can be part of the probe function, like it is in the unpatched driver.

In case of iio this struct tsc2007 is also allocated explicitly so that
a pointer can be stored in iio_priv.

This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.

I have added that approach to my inlined patch and it seems to work (attached).

Sorry if I do not use the wording you would use and sometimes overlook
something you have said. I feel here like moving on thin ice and doing
guesswork about unspoken assumptions...

> 
>> 
>> Having doing that, you can have this CONFIG_IIO block as just
>> doing the iio stuff with the input elements pulled back into the main
>> probe function.
>> 
>> Then define something like
>> 
>> iio_configure (stubbed to nothing if no IIO)
>> and
>> iio_unconfigure (also stubbed to nothing if no IIO).

This seems to work (draft attached).

>> 
>> A couple of additions in the header

I think you mean tsc2007.h?

This currently contains only platform data and could IMHO be eliminated
if everything becomes DT.

>> to make it all work
>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>> register defines..

Here it appears to me that I have to make a lot of so far private static
and even static inline functions public so that I can make them stubs and
call them from tsc2007_iio.c.

And for having proper parameter types I have to make most private structs
also public.

I really like the idea to have the optional iio feature in a separate source
file, but when really starting to write code, I get the impression that
it introduces more problems than it solves.

And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
as well. There are also two static function in some #ifdef #else # endif
and not going through stubs.

So is this intended to give up some static definitions?

BR and thanks,
Nikolaus

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..92da8f6 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>
 
 #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
 #define TSC2007_MEASURE_AUX            (0x2 << 4)
@@ -98,6 +99,9 @@ struct tsc2007 {
 
        int                     (*get_pendown_state)(struct device *);
        void                    (*clear_penirq)(void);
+
+       struct mutex            mlock;
+       void                    *private;
 };
 
 static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
@@ -192,7 +196,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 +326,157 @@ static void tsc2007_close(struct input_dev *input_dev)
        tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+struct tsc2007_iio {
+       struct tsc2007 *ts;
+};
+
+#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, \
+}
+
+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_iio *iio = iio_priv(indio_dev);
+       struct tsc2007 *tsc = iio->ts;
+       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,
+};
+
+static inline int tsc2007_iio_configure(struct tsc2007 *ts)
+{
+       int err;
+       struct iio_dev *indio_dev;
+       struct tsc2007_iio *iio;
+
+       indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio));
+       if (!indio_dev) {
+               dev_err(&ts->client->dev, "iio_device_alloc failed\n");
+               return -ENOMEM;
+       }
+
+       iio = iio_priv(indio_dev);
+       iio->ts = ts;
+       ts->private = (void *) indio_dev;
+
+       indio_dev->name = "tsc2007";
+       indio_dev->dev.parent = &ts->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(&ts->client->dev, "iio_device_register() failed: %d\n",
+                       err);
+               return err;
+       }
+
+       return 0;
+}
+
+static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
+{
+       struct iio_dev *indio_dev = ts->private;
+
+       iio_device_unregister(indio_dev);
+}
+
+#else /* CONFIG_IIO */
+
+static inline int tsc2007_iio_configure(struct tsc2007 *ts)
+{
+       /* not needed */
+}
+
+static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
+{
+       /* not needed */
+}
+
+#endif /* CONFIG_IIO */
+
 #ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
@@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client,
        ts->client = client;
        ts->irq = client->irq;
        ts->input = input_dev;
+
+       err = tsc2007_iio_configure(ts);
+       if (err < 0)
+               return err;
+
        init_waitqueue_head(&ts->wait);
+       mutex_init(&ts->mlock);
 
        snprintf(ts->phys, sizeof(ts->phys),
                 "%s/input0", dev_name(&client->dev));
@@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client,
                                                          ts->fuzzz, 0);
        } else {
                err = tsc2007_probe_dt(client, ts);
-               if (err)
+               if (err) {
+                       tsc2007_iio_unconfigure(ts);
                        return err;
+               }
        }
 
        if (pdata) {
@@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client,
                                dev_err(&client->dev,
                                        "Failed to register exit_platform_hw action, %d\n",
                                        err);
+                               tsc2007_iio_unconfigure(ts);
                                return err;
                        }
                }
@@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client,
        if (err) {
                dev_err(&client->dev, "Failed to request irq %d: %d\n",
                        ts->irq, err);
+               tsc2007_iio_unconfigure(ts);
                return err;
        }
 
@@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client,
        if (err < 0) {
                dev_err(&client->dev,
                        "Failed to setup chip: %d\n", err);
+               tsc2007_iio_unconfigure(ts);
                return err;     /* usually, chip does not respond */
        }
 
@@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client,
        if (err) {
                dev_err(&client->dev,
                        "Failed to register input device: %d\n", err);
+               tsc2007_iio_unconfigure(ts);
                return err;
        }
 
        return 0;
 }
 
+static int tsc2007_remove(struct i2c_client *client)
+{
+       struct tsc2007 *ts = i2c_get_clientdata(client);
+       tsc2007_iio_unconfigure(ts);
+       input_unregister_device(ts->input);
+       return 0;
+}
+
 static const struct i2c_device_id tsc2007_idtable[] = {
        { "tsc2007", 0 },
        { }
@@ -578,6 +756,7 @@ static struct i2c_driver tsc2007_driver = {
        },
        .id_table       = tsc2007_idtable,
        .probe          = tsc2007_probe,
+       .remove         = tsc2007_remove,
 };
 
 module_i2c_driver(tsc2007_driver);

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 10:57, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
>>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23@kernel.org>:
>>>>
>>>> On 17/10/16 14:57, 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.
>>>>>
>>>>> Since it is not wasting much resources and is very straightforward,
>>>>> we simply provide all other adc channels as optional iio interfaces
>>>>> as weel. This can be used for debugging or special applications.
>>>> well
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> This could be cleaner done perhaps by factoring out the IIO stuff into a separate
>>>> file and using a header with stubs to deal with the no available case.
>>>>
>>>> There will only be a handful of stubs and it'll give you a lot cleaner code
>>>> in here.
>>>>
>>>> If def fun in .c files is always harder to deal with than in a header
>>>> where stubs are really obvious.
>>>
>>> Yes, it became a lot of #ifdefs spread over the source file.
>>>
>>> The easiest thing would be to require IIO to be enabled :)
>>>
>>> With your proposal to consider refactoring, I think the crucial part
>>> is the conditional allocation either through devm_iio_device_alloc()
>>> or devm_kzalloc(). This can be refactored into some conditional
>>> tsc2007_alloc().
>>>
>>> I have tried some draft (not tested and not tidied up) to check if the
>>> direction is good.
>>>
>>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>>> seem to be very complex now in comparison. And the patch itself has only a
>>> handful of hunks (8).
>>>
>>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
>>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to understand
>>> because it is not really iio specific and one has to switch between two source
>>> files. And I would have to touch the Makefile as well.
>>>
>>> What do you think?
>> I'd still split it.  The only bit of the IIO block that isn't specific is
>> a tiny chunk of the allocation code (as you've highlighted).
>>
>> Even that can be avoided by adding a tiny bit more indirection than would
>> otherwise be needed (it's not pretty but it would give a clean separation).
> 
> I hope I understand what you mean (which is an indication that the result
> may be much easier to read for you but not me...).
> 
>> It's pretty much the way this sort of optional functionality should always
>> be done - means that if you don't care (i.e. it's not enabled) you don't
>> even have to see the code.
> 
>>
>> Jonathan
>>>
>>> If generally ok, I can include that in [PATCH v5].
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..691e79f 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> #include <linux/input/touchscreen.h>
>>> +#include <linux/iio/iio.h>
>>>
>>> #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX            (0x2 << 4)
>>> @@ -69,9 +70,13 @@ struct ts_event {
>>>
>>> struct tsc2007 {
>>>        struct input_dev        *input;
>>> +#ifdef CONFIG_IIO
>>> +       struct iio_dev          *indio;
>>> +#endif
>> I wouldn't bother with this one.  Just have 
>> struct iio_dev; before this and it'll waste a whole
>> one pointer (+ you shouldn't need to have iio.h included
>> in here once you have spit the files).
> 
> Looks as if I have to make a knot in my brain before I start to understand...
> 
> How can I use struct iio_dev here w/o including iio.h?
you aren't using it.  You have a pointer to it.

So it (before this definition) you have a line that says
struct iio_dev;  you let the compiler know such a structure exists.
At that point you don't actually have to provide a definition of
what is in it as long as all you use is a pointer (they are always
the same size).
> 
>>>        char                    phys[32];
>>>
>>>        struct i2c_client       *client;
>>> +       struct mutex            mlock;
>>>
>>>        u16                     model;
>>>        u16                     x_plate_ohms;
>>> @@ -192,7 +197,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 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
>>>        tsc2007_stop(ts);
>>> }
>>>
>>> +#ifdef CONFIG_IIO
>>> +
>>> +#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, \
>>> +}
>>> +
>>> +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,
>>> +};
>>> +
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> +                          struct input_dev **input_dev)
>>> +{
>>> +       int err;
>>> +       struct iio_dev *indio_dev;
>>> +
>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> Instead of doing this to reduce the delta between versions make 
>> iio_priv a struct tsc2007 **
>>
>> That is have a single pointer in there and do your allocation of struct
>> tsc2007 separately.
> 
> Sorry, but I think I do not completely understand what you mean here.
> 
> The problem is that we need to allocate some struct tsc2007 in both cases.
> But in one case managed directly by &client->dev and in the other managed
> indirectly. This is why I use the private area of struct iio_dev to store
> the full struct tsc2007 and not just a pointer.
> 
No you don't need to do what you are currently doing.

You need to have some means to navigate from struct iio_dev to the 
struct tsc2007 - that doesn't have to be because it actually is
in iio_priv.

You can instead put a point to it in iio_priv (and only that) and allocate
it the same way in both paths (stashing a copy of the address in the
pointer in iio_priv).


> What I mean is:
> 
>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>> 	*ts = iio_priv(indio_dev);
> 
> vs. 
> 
>>> 	 *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> 
> 
> So how can the IIO case just extend/wrap devm_kzalloc(&client->dev...) and still
> be managed as well?
Not relevant if you just allocate it the same way both times.
> 
>>
>> Having doing that, you can have this CONFIG_IIO block as just
>> doing the iio stuff with the input elements pulled back into the main
>> probe function.
>>
>> Then define something like
>>
>> iio_configure (stubbed to nothing if no IIO)
>> and
>> iio_unconfigure (also stubbed to nothing if no IIO).
>>
>> A couple of additions in the header to make it all work
>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>> register defines..
>>
>> Nothing big and gets all the CONFIG_IIO into some really
>> obvious stubbing out in the header.
> 
> 
> Is there some example driver which is doing it that way to be optionally IIO
> compatible? That might be easier to understand and copy than a description.
This particular combination is unusual - but it similar to how we
do optional buffer or trigger support in various iio drivers.

Perhaps see include/linux/iio/common/st_sensors.h and look for CONFIG_IIO_TRIGGER
> 
>>
>>> +       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)
>>> +               return -ENOMEM;
>>> +
>>> +       i2c_set_clientdata(client, *ts);
>>> +       (*ts)->indio = indio_dev;
>>> +
>>> +       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;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
>>> +
>>> +#else /* CONFIG_IIO */
>>> +
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> +                          struct input_dev **input_dev)
>>> +{
>>> +       int err;
>>> +
>>> +       *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> +       if (!*ts)
>>> +               return -ENOMEM;
>>> +
>>> +       *input_dev = devm_input_allocate_device(&client->dev);
>>> +       if (!*input_dev)
>>> +               return -ENOMEM;
>>> +
>>> +       i2c_set_clientdata(client, *ts);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +#define tsc2007_iio_device_unregister(ts) /* not needed */
>> That's rather ugly and fragile.  I'd stub it out as an actual function
>> with no content and let the compiler drop it.
> 
> Well, it is a quick and dirty draft.
> Should indeed better be a static (inline) function with empty body.
> 
>>> +
>>> +#endif /* CONFIG_IIO */
>>> +
>>> #ifdef CONFIG_OF
>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>> {
>>> @@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
>>>                                     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>                return -EIO;
>>>
>>> -       ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> -       if (!ts)
>>> -               return -ENOMEM;
>>> -
>>> -       input_dev = devm_input_allocate_device(&client->dev);
>>> -       if (!input_dev)
>>> -               return -ENOMEM;
>>> -
>>> -       i2c_set_clientdata(client, ts);
>>> +       err = tsc2007_alloc(client, &ts, &input_dev);
>>> +       if (err < 0)
>>> +               return err;
>>>
>>>        ts->client = client;
>>>        ts->irq = client->irq;
>>>        ts->input = input_dev;
>>>        init_waitqueue_head(&ts->wait);
>>> +       mutex_init(&ts->mlock);
>>>
>>>        snprintf(ts->phys, sizeof(ts->phys),
>>>                 "%s/input0", dev_name(&client->dev));
>>> @@ -543,6 +702,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>>        if (err < 0) {
>>>                dev_err(&client->dev,
>>>                        "Failed to setup chip: %d\n", err);
>>> +               tsc2007_iio_device_unregister(ts);
>>>                return err;     /* usually, chip does not respond */
>>>        }
>>>
>>> @@ -556,6 +716,14 @@ 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);
>>> +       input_unregister_device(ts->input);
>>> +       tsc2007_iio_device_unregister(ts);
>>> +       return 0;
>>> +}
>>> +
>>> static const struct i2c_device_id tsc2007_idtable[] = {
>>>        { "tsc2007", 0 },
>>>        { }
>>> @@ -578,6 +746,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 v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 19:34, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>
>> Hi,
>>
>>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>>> +                          struct input_dev **input_dev)
>>>> +{
>>>> +       int err;
>>>> +       struct iio_dev *indio_dev;
>>>> +
>>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>> Instead of doing this to reduce the delta between versions make 
>>> iio_priv a struct tsc2007 **
>>>
>>> That is have a single pointer in there and do your allocation of struct
>>> tsc2007 separately.
>>
>> Sorry, but I think I do not completely understand what you mean here.
>>
>> The problem is that we need to allocate some struct tsc2007 in both cases.
>> But in one case managed directly by &client->dev and in the other managed
>> indirectly. This is why I use the private area of struct iio_dev to store
>> the full struct tsc2007 and not just a pointer.
> 
> Ok, I think I finally did understand how you mean this and have started to
> implement something.
> 
oops. Didn't look on in my emails to get to this one!
> The idea is to have one alloc function to return a struct tsc2007. This
> can be part of the probe function, like it is in the unpatched driver.
> 
> In case of iio this struct tsc2007 is also allocated explicitly so that
> a pointer can be stored in iio_priv.
> 
> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
> 
> I have added that approach to my inlined patch and it seems to work (attached).
> 
> Sorry if I do not use the wording you would use and sometimes overlook
> something you have said. I feel here like moving on thin ice and doing
> guesswork about unspoken assumptions...
That's fine.  Stuff that can appear obvious to one person is not
necessarily obvious to another!
> 
>>
>>>
>>> Having doing that, you can have this CONFIG_IIO block as just
>>> doing the iio stuff with the input elements pulled back into the main
>>> probe function.
>>>
>>> Then define something like
>>>
>>> iio_configure (stubbed to nothing if no IIO)
>>> and
>>> iio_unconfigure (also stubbed to nothing if no IIO).
> 
> This seems to work (draft attached).
> 
>>>
>>> A couple of additions in the header
> 
> I think you mean tsc2007.h?
Nope. A local header alongside the driver is what you want for this stuff.
driver/input/tsc2007.h 
> 
> This currently contains only platform data and could IMHO be eliminated
> if everything becomes DT.
> 
>>> to make it all work
>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>> register defines..
> 
> Here it appears to me that I have to make a lot of so far private static
> and even static inline functions public so that I can make them stubs and
> call them from tsc2007_iio.c.
There will be a few.
> 
> And for having proper parameter types I have to make most private structs
> also public.
Yes a few of those as well.
> 
> I really like the idea to have the optional iio feature in a separate source
> file, but when really starting to write code, I get the impression that
> it introduces more problems than it solves.
> 
> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
> as well. There are also two static function in some #ifdef #else # endif
> and not going through stubs.
Usually it is only done once a certain volume of code exists.
> 
> So is this intended to give up some static definitions?
Yes, that happens the moment you have multiple source files.

Some losses but generally end up with clean code separation. Always a trade
off unfortunately.  Pity we can't just insist IIO is available! Rather large
to pull in for what is probable a niche use case.

Below is definitely heading in the right direction. I remember vaguely being
convinced of the worth of doing this when optional code is involved!
(was a good while ago now)

Jonathan
> 
> BR and thanks,
> Nikolaus
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..92da8f6 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/input/touchscreen.h>
> +#include <linux/iio/iio.h>
Should not need this after introducing the new file.  Will only be
needed in the iio specific .c file.
>  
>  #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>  #define TSC2007_MEASURE_AUX            (0x2 << 4)
> @@ -98,6 +99,9 @@ struct tsc2007 {
This will definitely need to go in the header though.
>  
>         int                     (*get_pendown_state)(struct device *);
>         void                    (*clear_penirq)(void);
> +
> +       struct mutex            mlock;
> +       void                    *private;
>  };
>  
>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
> @@ -192,7 +196,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 +326,157 @@ static void tsc2007_close(struct input_dev *input_dev)
>         tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +struct tsc2007_iio {
> +       struct tsc2007 *ts;
Spot on.
> +};
> +
> +#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, \
> +}
> +
> +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_iio *iio = iio_priv(indio_dev);
> +       struct tsc2007 *tsc = iio->ts;
> +       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,
> +};
> +
> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
> +{
> +       int err;
> +       struct iio_dev *indio_dev;
> +       struct tsc2007_iio *iio;
> +
> +       indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio));
> +       if (!indio_dev) {
> +               dev_err(&ts->client->dev, "iio_device_alloc failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       iio = iio_priv(indio_dev);
> +       iio->ts = ts;
> +       ts->private = (void *) indio_dev;
> +
> +       indio_dev->name = "tsc2007";
> +       indio_dev->dev.parent = &ts->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(&ts->client->dev, "iio_device_register() failed: %d\n",
> +                       err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
> +{
> +       struct iio_dev *indio_dev = ts->private;
> +
> +       iio_device_unregister(indio_dev);
> +}
> +
> +#else /* CONFIG_IIO */
> +
> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
> +{
> +       /* not needed */
> +}
> +
> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
> +{
> +       /* not needed */
> +}
> +
> +#endif /* CONFIG_IIO */
> +
>  #ifdef CONFIG_OF
>  static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  {
> @@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client,
>         ts->client = client;
>         ts->irq = client->irq;
>         ts->input = input_dev;
> +
> +       err = tsc2007_iio_configure(ts);
> +       if (err < 0)
> +               return err;
> +
>         init_waitqueue_head(&ts->wait);
> +       mutex_init(&ts->mlock);
>  
>         snprintf(ts->phys, sizeof(ts->phys),
>                  "%s/input0", dev_name(&client->dev));
> @@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client,
>                                                           ts->fuzzz, 0);
>         } else {
>                 err = tsc2007_probe_dt(client, ts);
> -               if (err)
> +               if (err) {
> +                       tsc2007_iio_unconfigure(ts);
>                         return err;
> +               }
>         }
>  
>         if (pdata) {
> @@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client,
>                                 dev_err(&client->dev,
>                                         "Failed to register exit_platform_hw action, %d\n",
>                                         err);
> +                               tsc2007_iio_unconfigure(ts);
>                                 return err;
>                         }
>                 }
> @@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client,
>         if (err) {
>                 dev_err(&client->dev, "Failed to request irq %d: %d\n",
>                         ts->irq, err);
> +               tsc2007_iio_unconfigure(ts);
Maybe need a common unwind and use a goto to get to it.
>                 return err;
>         }
>  
> @@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client,
>         if (err < 0) {
>                 dev_err(&client->dev,
>                         "Failed to setup chip: %d\n", err);
> +               tsc2007_iio_unconfigure(ts);
>                 return err;     /* usually, chip does not respond */
>         }
>  
> @@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client,
>         if (err) {
>                 dev_err(&client->dev,
>                         "Failed to register input device: %d\n", err);
> +               tsc2007_iio_unconfigure(ts);
>                 return err;
>         }
>  
>         return 0;
>  }
>  
> +static int tsc2007_remove(struct i2c_client *client)
> +{
> +       struct tsc2007 *ts = i2c_get_clientdata(client);
> +       tsc2007_iio_unconfigure(ts);
> +       input_unregister_device(ts->input);
> +       return 0;
> +}
> +
>  static const struct i2c_device_id tsc2007_idtable[] = {
>         { "tsc2007", 0 },
>         { }
> @@ -578,6 +756,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 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 v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>> 
>>> Hi,
>>> 
>>>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>>>> +                          struct input_dev **input_dev)
>>>>> +{
>>>>> +       int err;
>>>>> +       struct iio_dev *indio_dev;
>>>>> +
>>>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>>> Instead of doing this to reduce the delta between versions make 
>>>> iio_priv a struct tsc2007 **
>>>> 
>>>> That is have a single pointer in there and do your allocation of struct
>>>> tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by &client->dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
>>>> 
>>>> Having doing that, you can have this CONFIG_IIO block as just
>>>> doing the iio stuff with the input elements pulled back into the main
>>>> probe function.
>>>> 
>>>> Then define something like
>>>> 
>>>> iio_configure (stubbed to nothing if no IIO)
>>>> and
>>>> iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
>>>> 
>>>> A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 

Ah, ok. This makes sense.

>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
>>>> to make it all work
>>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>>> register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)

Ok. I think I can now integrate it and then post as PATCH v5 (together with
some other fixes for the patch set).

> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/input/touchscreen.h>
>> +#include <linux/iio/iio.h>
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>> #define TSC2007_MEASURE_AUX            (0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.
>> 
>>        int                     (*get_pendown_state)(struct device *);
>>        void                    (*clear_penirq)(void);
>> +
>> +       struct mutex            mlock;
>> +       void                    *private;
>> };
>> 
>> static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>> @@ -192,7 +196,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 +326,157 @@ static void tsc2007_close(struct input_dev *input_dev)
>>        tsc2007_stop(ts);
>> }
>> 
>> +#ifdef CONFIG_IIO
>> +
>> +struct tsc2007_iio {
>> +       struct tsc2007 *ts;
> Spot on.
>> +};
>> +
>> +#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, \
>> +}
>> +
>> +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_iio *iio = iio_priv(indio_dev);
>> +       struct tsc2007 *tsc = iio->ts;
>> +       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,
>> +};
>> +
>> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
>> +{
>> +       int err;
>> +       struct iio_dev *indio_dev;
>> +       struct tsc2007_iio *iio;
>> +
>> +       indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio));
>> +       if (!indio_dev) {
>> +               dev_err(&ts->client->dev, "iio_device_alloc failed\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       iio = iio_priv(indio_dev);
>> +       iio->ts = ts;
>> +       ts->private = (void *) indio_dev;
>> +
>> +       indio_dev->name = "tsc2007";
>> +       indio_dev->dev.parent = &ts->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(&ts->client->dev, "iio_device_register() failed: %d\n",
>> +                       err);
>> +               return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>> +{
>> +       struct iio_dev *indio_dev = ts->private;
>> +
>> +       iio_device_unregister(indio_dev);
>> +}
>> +
>> +#else /* CONFIG_IIO */
>> +
>> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
>> +{
>> +       /* not needed */
>> +}
>> +
>> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>> +{
>> +       /* not needed */
>> +}
>> +
>> +#endif /* CONFIG_IIO */
>> +
>> #ifdef CONFIG_OF
>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>> {
>> @@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client,
>>        ts->client = client;
>>        ts->irq = client->irq;
>>        ts->input = input_dev;
>> +
>> +       err = tsc2007_iio_configure(ts);
>> +       if (err < 0)
>> +               return err;
>> +
>>        init_waitqueue_head(&ts->wait);
>> +       mutex_init(&ts->mlock);
>> 
>>        snprintf(ts->phys, sizeof(ts->phys),
>>                 "%s/input0", dev_name(&client->dev));
>> @@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client,
>>                                                          ts->fuzzz, 0);
>>        } else {
>>                err = tsc2007_probe_dt(client, ts);
>> -               if (err)
>> +               if (err) {
>> +                       tsc2007_iio_unconfigure(ts);
>>                        return err;
>> +               }
>>        }
>> 
>>        if (pdata) {
>> @@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>                                dev_err(&client->dev,
>>                                        "Failed to register exit_platform_hw action, %d\n",
>>                                        err);
>> +                               tsc2007_iio_unconfigure(ts);
>>                                return err;
>>                        }
>>                }
>> @@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>        if (err) {
>>                dev_err(&client->dev, "Failed to request irq %d: %d\n",
>>                        ts->irq, err);
>> +               tsc2007_iio_unconfigure(ts);
> Maybe need a common unwind and use a goto to get to it.

Yes, makes sense.

>>                return err;
>>        }
>> 
>> @@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>        if (err < 0) {
>>                dev_err(&client->dev,
>>                        "Failed to setup chip: %d\n", err);
>> +               tsc2007_iio_unconfigure(ts);
>>                return err;     /* usually, chip does not respond */
>>        }
>> 
>> @@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client,
>>        if (err) {
>>                dev_err(&client->dev,
>>                        "Failed to register input device: %d\n", err);
>> +               tsc2007_iio_unconfigure(ts);
>>                return err;
>>        }
>> 
>>        return 0;
>> }
>> 
>> +static int tsc2007_remove(struct i2c_client *client)
>> +{
>> +       struct tsc2007 *ts = i2c_get_clientdata(client);
>> +       tsc2007_iio_unconfigure(ts);
>> +       input_unregister_device(ts->input);
>> +       return 0;
>> +}
>> +
>> static const struct i2c_device_id tsc2007_idtable[] = {
>>        { "tsc2007", 0 },
>>        { }
>> @@ -578,6 +756,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 linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

BR and thanks,
Nikolaus

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>> 
>>> Hi,
>>> 
>>>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>>>> +                          struct input_dev **input_dev)
>>>>> +{
>>>>> +       int err;
>>>>> +       struct iio_dev *indio_dev;
>>>>> +
>>>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>>> Instead of doing this to reduce the delta between versions make 
>>>> iio_priv a struct tsc2007 **
>>>> 
>>>> That is have a single pointer in there and do your allocation of struct
>>>> tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by &client->dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
>>>> 
>>>> Having doing that, you can have this CONFIG_IIO block as just
>>>> doing the iio stuff with the input elements pulled back into the main
>>>> probe function.
>>>> 
>>>> Then define something like
>>>> 
>>>> iio_configure (stubbed to nothing if no IIO)
>>>> and
>>>> iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
>>>> 
>>>> A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 
>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
>>>> to make it all work
>>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>>> register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)
> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/input/touchscreen.h>
>> +#include <linux/iio/iio.h>
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>> #define TSC2007_MEASURE_AUX            (0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.

Now I have split the code into:

tsc2007.h (constants, structs and stubs)
tsc2007_iio.c (the iio stuff)
tsc2007.c (most parts of the original driver)

but I have a problem of correctly modifying the Makefile.

It currently looks like:

obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o
obj-$(CONFIG_IIO)			+= tsc2007_iio.o

We have configured CONFIG_TOUCHSCREEN_TSC2007=m and CONFIG_IIO=y.

This obviously compiles tsc2007_iio.o into the kernel.

This means that tsc2007_iio.o references tsc2007_xfer which is part of
the module.

I would like to get both linked into the module, but the iio part
obviously only if CONFIG_IIO is defined (either -y or -m).

How can I define this?

Or can I define

obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o tsc2007_iio.o

and embrace all code in tsc2007_iio with a big #ifdef CONFIG_IIO
so that it is compiled into an empty object file in the non-iio case?

BR and thanks,
Nikolaus

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 24/10/16 20:14, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
>>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>>>
>>>> Hi,
>>>>
>>>>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>>>>> +                          struct input_dev **input_dev)
>>>>>> +{
>>>>>> +       int err;
>>>>>> +       struct iio_dev *indio_dev;
>>>>>> +
>>>>>> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>>>> Instead of doing this to reduce the delta between versions make 
>>>>> iio_priv a struct tsc2007 **
>>>>>
>>>>> That is have a single pointer in there and do your allocation of struct
>>>>> tsc2007 separately.
>>>>
>>>> Sorry, but I think I do not completely understand what you mean here.
>>>>
>>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>>> But in one case managed directly by &client->dev and in the other managed
>>>> indirectly. This is why I use the private area of struct iio_dev to store
>>>> the full struct tsc2007 and not just a pointer.
>>>
>>> Ok, I think I finally did understand how you mean this and have started to
>>> implement something.
>>>
>> oops. Didn't look on in my emails to get to this one!
>>> The idea is to have one alloc function to return a struct tsc2007. This
>>> can be part of the probe function, like it is in the unpatched driver.
>>>
>>> In case of iio this struct tsc2007 is also allocated explicitly so that
>>> a pointer can be stored in iio_priv.
>>>
>>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>>>
>>> I have added that approach to my inlined patch and it seems to work (attached).
>>>
>>> Sorry if I do not use the wording you would use and sometimes overlook
>>> something you have said. I feel here like moving on thin ice and doing
>>> guesswork about unspoken assumptions...
>> That's fine.  Stuff that can appear obvious to one person is not
>> necessarily obvious to another!
>>>
>>>>
>>>>>
>>>>> Having doing that, you can have this CONFIG_IIO block as just
>>>>> doing the iio stuff with the input elements pulled back into the main
>>>>> probe function.
>>>>>
>>>>> Then define something like
>>>>>
>>>>> iio_configure (stubbed to nothing if no IIO)
>>>>> and
>>>>> iio_unconfigure (also stubbed to nothing if no IIO).
>>>
>>> This seems to work (draft attached).
>>>
>>>>>
>>>>> A couple of additions in the header
>>>
>>> I think you mean tsc2007.h?
>> Nope. A local header alongside the driver is what you want for this stuff.
>> driver/input/tsc2007.h 
>>>
>>> This currently contains only platform data and could IMHO be eliminated
>>> if everything becomes DT.
>>>
>>>>> to make it all work
>>>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>>>> register defines..
>>>
>>> Here it appears to me that I have to make a lot of so far private static
>>> and even static inline functions public so that I can make them stubs and
>>> call them from tsc2007_iio.c.
>> There will be a few.
>>>
>>> And for having proper parameter types I have to make most private structs
>>> also public.
>> Yes a few of those as well.
>>>
>>> I really like the idea to have the optional iio feature in a separate source
>>> file, but when really starting to write code, I get the impression that
>>> it introduces more problems than it solves.
>>>
>>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>>> as well. There are also two static function in some #ifdef #else # endif
>>> and not going through stubs.
>> Usually it is only done once a certain volume of code exists.
>>>
>>> So is this intended to give up some static definitions?
>> Yes, that happens the moment you have multiple source files.
>>
>> Some losses but generally end up with clean code separation. Always a trade
>> off unfortunately.  Pity we can't just insist IIO is available! Rather large
>> to pull in for what is probable a niche use case.
>>
>> Below is definitely heading in the right direction. I remember vaguely being
>> convinced of the worth of doing this when optional code is involved!
>> (was a good while ago now)
>>
>> Jonathan
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..92da8f6 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> #include <linux/input/touchscreen.h>
>>> +#include <linux/iio/iio.h>
>> Should not need this after introducing the new file.  Will only be
>> needed in the iio specific .c file.
>>>
>>> #define TSC2007_MEASURE_TEMP0          (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX            (0x2 << 4)
>>> @@ -98,6 +99,9 @@ struct tsc2007 {
>> This will definitely need to go in the header though.
> 
> Now I have split the code into:
> 
> tsc2007.h (constants, structs and stubs)
> tsc2007_iio.c (the iio stuff)
> tsc2007.c (most parts of the original driver)
> 
> but I have a problem of correctly modifying the Makefile.
> 
> It currently looks like:
> 
> obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o
> obj-$(CONFIG_IIO)			+= tsc2007_iio.o
> 
> We have configured CONFIG_TOUCHSCREEN_TSC2007=m and CONFIG_IIO=y.
> 
> This obviously compiles tsc2007_iio.o into the kernel.
> 
> This means that tsc2007_iio.o references tsc2007_xfer which is part of
> the module.
> 
> I would like to get both linked into the module, but the iio part
> obviously only if CONFIG_IIO is defined (either -y or -m).
> 
> How can I define this?
> 
> Or can I define
> 
> obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o tsc2007_iio.o
This is a common problem with optional support. Various ways of 
handling it.

A simple one would be:
#Define the stuff that always forms part of the .o file
magic_tsc2007-y := tsc2007.o
#Define the optional bit to build the resulting magic_tsc2007.o file
magic_tsc2007-$(CONFIG_IIO) += tsc2007_iio.o
#Use this magic combined file
obj-$(CONFIG_TOUCHSCREEN_TSC2007) += magic_tsc2007.o

With sensible naming of course ;)

Jonathan
> 
> and embrace all code in tsc2007_iio with a big #ifdef CONFIG_IIO
> so that it is compiled into an empty object file in the non-iio case?
> 
> 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 v4 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Rob Herring]

On Tue, Oct 18, 2016 at 12:27 PM, H. Nikolaus Schaller
<hns@goldelico.com> wrote:
> Hi Rob,
>
>> Am 18.10.2016 um 18:22 schrieb Rob Herring <robh+dt@kernel.org>:
>>
>> On Mon, Oct 17, 2016 at 8:57 AM, H. Nikolaus Schaller <hns@goldelico.com> 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.
>>
>> Please add the explanation of why this is a compatible change here.
>
> Can you please describe in more detail what you are missing here?

You stop handling ti,fuzzx for example. So the default driver value
will be used and this is okay because...

Seems like you would have a regression in behavior if the fuzz values
specified in an existing DT are ignored.

> The patch simply makes use of the generic helper function introduced by the
> two patches cited above and is therefore automatically compatible if none of
> the new properties is defined.
>
> The tsc2007 didn't have these features before. So what should we say
> about compatibility?

It certainly had the fuzz features before.


>>> -- 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).
>>
>> I thought these were going to be common? I think they should be.
>
> Yes, we had discussed that before.
>
> In a second thought, I found that they are not even existing for the different
> touch chip drivers that are around. And some chip might not need them (because
> e.g. min/max are constants or defined my means outside the DT or the chip itself
> can store them in NV memory).
>
> And, since they define ADC raw-values, they are very closely related to the
> individual chip, so that I am not sure if it is really necessary to make
> them common. At least if they are properly documented in the bindings for
> the specific chip they can have different names without disturbing each
> other.

They wouldn't disturb each other, but then there is no chance to have
common parsing either.

> So I think it is perfectly reasonable to define common bindings towards
> the input event layer (e.g. size, fuzz, swapping, inversion) and support
> them by common code which bakes raw coordinates into input events. The latest
> helper functions already do most of that fully automatic.

I could argue that size in pixels has no business being in DT. That's
a property of the panel the touchscreen is glued to. I guess in some
cases, the controller is internally scaling the ADC values.

> Next, I had simply copied them from the ads7846 driver where they
> exist for a long time. So this is sort of "common", at least for two different
> chips now. See also patch 6/8 of this series which adds the common properties
> to the ads7846 as well.
>
> For more reference about the existing bindings:
>
> Documentation/devicetree/bindings/input/ads7846.txt
>
> (btw I think someone should move them to bindings/input/touchscreen).

Patches welcome.

> It appears as if there is some overlap of the new generic properties (for x/y
> swapping) with the old ads7846 properties, but that is something you had
> already proposed a while ago to make me the driver recognize both properties to
> stay compatible with older DT files.
>
> So if we now rename the min/max-x/y properties for the tsc2007 we have to rename
> them for the ads7846 (and maybe others) as well, which might break out-of-tree
> ads7846 devices or leads to more complex code for handling of property aliases.

We don't have to rename them for ads7846. That's already baked. But
there's a trend here. You're defining the same property. Use a common
name, so when the 3rd binding comes along needing the same thing, we
already have a common binding. If we're catching this on only the 2nd
binding, we're doing pretty good.

Rob