[PATCH 00/14] iio: triggers: add consumer support

[PATCH 00/14] iio: triggers: add consumer support

[Eugen Hristev]

Hello,

I would like to introduce his patch series which implements:
- new iio channel IIO_POSITION
- new inkern API to expose iio triggers to consumer drivers
- new bindings for trigger consumer drivers in device tree
- new input touchscreen driver for SAMA5D2
- implementation of POSITION and PRESSURE channels in sama5d2_adc driver
- devicetree changes for sama5d2 soc w.r.t. resistive touchscreen.

The SAMA5D2 SOC contains the ADC IP which has support
for a resistive touchscreen inside, using channels 0,1,2,3 from the ADC.
As discussed earlier on the mailing list, here is an implementation based
on the fact that the IIO device can expose the trigger and the required
channels to another driver (in our case the input touchscreen driver).
This touchscreen driver will consume the trigger and the values ( by
feeding them to the input subsystem).

Design decisions: The touchscreen driver will do a successful probe at
all times. Only on touch open and close, it will connect to the ADC device.
This is done to avoid a hard dependency between loading the ADC module and
the touchscreen one. So probe will always succeed and the dependency
is only soft.

Below it's an explanation of each patch in the series. Some changes were
required in the inkern API and the implementation was done in the ADC driver
and the touchscreen driver.

1: Added maintainers entry for new driver. This is done in commit:
MAINTAINERS: add sama5d2 resistive touchscreen

2: First we need a new set of channels named Position to report from the IIO
device the position on the touchpad. This is done in commit :
iio: Add channel for Position

3: we need device tree connection between the trigger producer and the
trigger consumer. I made new bindings very similar with the ones for the IIO
channels. This is done in the commit:
dt-bindings: iio: add binding support for iio trigger
    provider/consumer

4: we need bindings for the input touchscreen driver, named sama5d2_rts
(resistive touch screen). This driver will be the consumer for the IIO
trigger and channels. This is done in the commit:
dt-bindings: input: touchscreen: sama5d2_rts: create bindings

5: we need some helper functions to get the trigger from the iio device
from inkern API. This is done in the commit:
iio: triggers: add helper function to retrieve trigger

6: we need helper functions to attach and detach a pollfunction to and
from the trigger itself. The trigger is registered by an IIO device, and
is still connected to it, but we need to be able to register a different
pollfunc from another driver. Thus I exposed this API to be inkern API.
This is done in commit:
iio: triggers: expose attach and detach helpers for pollfuncs

7: we need to be able to attach a pollfunc to a trigger, even if the
pollfunc is not coming from an iio device. Thus, register the pollfunc using
the iio_dev of the trigger (in case it's coming as NULL from the pollfunc).
This is done in the commit:
iio: triggers: on pollfunc attach, complete iio_dev if NULL

8: we need a private data on the pollfunc, such that the consumer
driver has a pointer to it's private data when the handler is called.
This is done in the commit:
iio: triggers: add private data to pollfuncs

9: we need to have inkern API to be able to look inside the devicetree
and find from the properties, which trigger(s) need to be connected.
Created some API that will look through the properties and return found
triggers. This is done in the commit:
iio: inkern: triggers: create helpers for OF trigger retrieval

10: in at91-sama5d2_adc we need to remove trigger on module remove in
order to not have stray triggers that are unusable after the module is
removed and reinserted. If such triggers stay registered, the code that
looks through the list of triggers will not work as intended.
This is done in commit:
iio: adc: at91-sama5d2_adc: force trigger removal on module remove

11: we need consecutive indexes for the channels provided by at91-sama5d2_adc
so that we optimize the index values and easier to connect the
touchscreen driver to the channels as a consumer. This is done in the commit:
iio: adc: at91-sama5d2_adc: optimize scan index for diff channels

12: the implementation of the channels and the support for position
conversion and pressure is done in the at91-sama5d2_adc driver.
The implementation provides a trigger named "touch" which can be
connected to a consumer driver.
Once a driver connects and attaches a pollfunc to this trigger, the
configure trigger callback is called, and then the ADC driver will
initialize pad measurement.
First step is to enable touchscreen 4wire support and enable
pen detect IRQ.
Once a pen is detected, a periodic trigger is setup to trigger every 2 ms
(e.g.) and sample the resistive touchscreen values. The trigger poll
is called, and the consumer driver is then woke up, and it can read the
respective channels for the values : X, and Y for position and pressure
channel.
Because only one trigger can be active in hardware in the same time,
while touching the pad, the ADC will block any attempt to use the
triggered buffer. Same, conversions using the software trigger are also
impossible (since the periodic trigger is setup).
If some driver wants to attach while the trigger is in use, it will also fail.
Once the pen is not detected anymore, the trigger is free for use (hardware
or software trigger, with or without DMA).
Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
This is done in the commit:
iio: adc: at91-sama5d2_adc: support for position and pressure channels

13: the implementation of the basic touchscreen driver, which is an IIO
consumer.
The driver registers an input device and connects to the given IIO device from
devicetree. It requires an IIO trigger (acting as a consumer) and three IIO
channels : one for X position, one for Y position and one for pressure.
It the reports the values to the input subsystem.
This is done in the commit:
input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver

14: Finally, we need to have the node in the sama5d2.dtsi,
as it's part of the SoC. This is done in the commit:
ARM: dts: at91: sama5d2: Add resistive touch device

This series is based on the discussion in mailing list and probably requires
some patching up, but hopefully the idea and the implementation of the inkern
trigger consumers and the splitting between the ADC driver and the input
driver is ok like this.

The implementation avoids using a MFD driver, which means to make three
drivers in fact, one for ADC, one for touch, with a shared IRQ, and a general
MFD driver that will be a placeholder for the two.

This implementation is based on patches initially written by
Swamy Bandaru Venkateshwara and Mohamed Jamsheeth Hajanajubudeen

Eugen Hristev (14):
  MAINTAINERS: add sama5d2 resistive touchscreen
  iio: Add channel for Position
  dt-bindings: iio: add binding support for iio trigger
    provider/consumer
  dt-bindings: input: touchscreen: sama5d2_rts: create bindings
  iio: triggers: add helper function to retrieve trigger
  iio: triggers: expose attach and detach helpers for pollfuncs
  iio: triggers: on pollfunc attach, complete iio_dev if NULL
  iio: triggers: add private data to pollfuncs
  iio: inkern: triggers: create helpers for OF trigger retrieval
  iio: adc: at91-sama5d2_adc: force trigger removal on module remove
  iio: adc: at91-sama5d2_adc: optimize scan index for diff channels
  iio: adc: at91-sama5d2_adc: support for position and pressure channels
  input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver
  ARM: dts: at91: sama5d2: Add resistive touch device

 Documentation/ABI/testing/sysfs-bus-iio            |  11 +
 .../devicetree/bindings/iio/iio-bindings.txt       |  52 ++-
 .../bindings/input/touchscreen/sama5d2_rts.txt     |  31 ++
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/sama5d2.dtsi                     |  12 +-
 drivers/iio/adc/at91-sama5d2_adc.c                 | 460 ++++++++++++++++++++-
 drivers/iio/iio_core_trigger.h                     |  21 +
 drivers/iio/industrialio-core.c                    |   1 +
 drivers/iio/industrialio-trigger.c                 |  42 +-
 drivers/iio/inkern.c                               |  91 ++++
 drivers/input/touchscreen/Kconfig                  |  13 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/sama5d2_rts.c            | 287 +++++++++++++
 include/linux/iio/trigger_consumer.h               |  21 +
 include/uapi/linux/iio/types.h                     |   1 +
 tools/iio/iio_event_monitor.c                      |   2 +
 16 files changed, 1036 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
 create mode 100644 drivers/input/touchscreen/sama5d2_rts.c

-- 
2.7.4

[PATCH 01/14] MAINTAINERS: add sama5d2 resistive touchscreen

[Eugen Hristev]

Add MAINTAINERS entry for sama5d2 resistive touchscreen driver

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 650aa0e..8beec28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8957,6 +8957,12 @@ F:	drivers/media/platform/atmel/atmel-isc.c
 F:	drivers/media/platform/atmel/atmel-isc-regs.h
 F:	devicetree/bindings/media/atmel-isc.txt
 
+MICROCHIP / ATMEL SAMA5D2 RESISTIVE TOUCHSCREEN DRIVER
+M:	Eugen Hristev <eugen.hristev@microchip.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/input/touchscreen/sama5d2_rts.c
+
 MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
 M:	Woojung Huh <Woojung.Huh@microchip.com>
 M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
-- 
2.7.4

[PATCH 02/14] iio: Add channel for Position

[Eugen Hristev]

Add new channel type for position on a pad.

These type of analog sensor represents the position of a pen
on a touchpad, and is represented as a voltage, which can be
converted to a position on X and Y axis on the pad.

The channel can then be consumed by a touchscreen driver or
read as-is for a raw indication of the touchpen on a touchpad.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
 drivers/iio/industrialio-core.c         |  1 +
 include/uapi/linux/iio/types.h          |  1 +
 tools/iio/iio_event_monitor.c           |  2 ++
 4 files changed, 15 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a478740..d2b9e2f 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -190,6 +190,17 @@ Description:
 		but should match other such assignments on device).
 		Units after application of scale and offset are m/s^2.
 
+What:		/sys/bus/iio/devices/iio:deviceX/in_position_x_raw
+What:		/sys/bus/iio/devices/iio:deviceX/in_position_y_raw
+KernelVersion:	4.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Position in direction x or y on a pad (may be arbitrarily
+		assigned but should match other such assignments on device).
+		Units after application of scale and offset are millipercents
+		from the pad's size in both directions. Should be calibrated by
+		the consumer.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2e8e36f..a4fa49b 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_COUNT] = "count",
 	[IIO_INDEX] = "index",
 	[IIO_GRAVITY]  = "gravity",
+	[IIO_POSITION]  = "position",
 };
 
 static const char * const iio_modifier_names[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf..35e17da 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -44,6 +44,7 @@ enum iio_chan_type {
 	IIO_COUNT,
 	IIO_INDEX,
 	IIO_GRAVITY,
+	IIO_POSITION,
 };
 
 enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e..0c2b317 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_PH] = "ph",
 	[IIO_UVINDEX] = "uvindex",
 	[IIO_GRAVITY] = "gravity",
+	[IIO_POSITION] = "position",
 };
 
 static const char * const iio_ev_type_text[] = {
@@ -151,6 +152,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_PH:
 	case IIO_UVINDEX:
 	case IIO_GRAVITY:
+	case IIO_POSITION:
 		break;
 	default:
 		return false;
-- 
2.7.4

[PATCH 03/14] dt-bindings: iio: add binding support for iio trigger provider/consumer

[Eugen Hristev]

Add bindings for producer/consumer for iio triggers.

Similar with iio channels, the iio triggers can be connected between drivers:
one driver will be a producer by registering iio triggers, and another driver
will connect as a consumer.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 .../devicetree/bindings/iio/iio-bindings.txt       | 52 +++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
index 68d6f8c..d861f0df 100644
--- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
@@ -11,6 +11,10 @@ value of a #io-channel-cells property in the IIO provider node.
 
 [1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
 
+Moreover, the provider can have a set of triggers that can be attached to
+from the consumer drivers.
+
+
 ==IIO providers==
 
 Required properties:
@@ -18,6 +22,11 @@ Required properties:
 		   with a single IIO output and 1 for nodes with multiple
 		   IIO outputs.
 
+Optional properties:
+#io-trigger-cells: Number of cells for the IIO trigger specifier. Typically 0
+		   for nodes with a single IIO trigger and 1 for nodes with
+		   multiple IIO triggers.
+
 Example for a simple configuration with no trigger:
 
 	adc: voltage-sensor@35 {
@@ -26,7 +35,7 @@ Example for a simple configuration with no trigger:
 		#io-channel-cells = <1>;
 	};
 
-Example for a configuration with trigger:
+Example for a configuration with channels provided by trigger:
 
 	adc@35 {
 		compatible = "some-vendor,some-adc";
@@ -42,6 +51,17 @@ Example for a configuration with trigger:
 		};
 	};
 
+Example for a configuration for a trigger provider:
+
+	adc: sensor-with-trigger@35 {
+		compatible = "some-vendor,some-adc";
+		reg = <0x35>;
+		#io-channel-cells = <1>;
+		#io-trigger-cells = <1>;
+		/* other properties */
+	};
+
+
 ==IIO consumers==
 
 Required properties:
@@ -61,16 +81,38 @@ io-channel-ranges:
 		IIO channels from this node. Useful for bus nodes to provide
 		and IIO channel to their children.
 
+io-triggers:	List of phandle and IIO specifier pairs, one pair
+		for each trigger input to the device. Note: if the
+		IIO trigger provider specifies '0' for #io-trigger-cells,
+		then only the phandle portion of the pair will appear.
+
+io-trigger-names:
+		List of IIO trigger input name strings sorted in the same
+		order as the io-triggers property. Consumers drivers
+		will use io-trigger-names to match IIO trigger input names
+		with IIO specifiers.
+
+io-trigger-ranges:
+		Empty property indicating that child nodes can inherit named
+		IIO triggers from this node. Useful for bus nodes to provide
+		IIO triggers to their children.
+
 For example:
 
 	device {
 		io-channels = <&adc 1>, <&ref 0>;
 		io-channel-names = "vcc", "vdd";
+		io-triggers = <&adc 0>, <&adc 1>;
+		io-trigger-names = "continuous", "external";
 	};
 
 This represents a device with two IIO inputs, named "vcc" and "vdd".
 The vcc channel is connected to output 1 of the &adc device, and the
 vdd channel is connected to output 0 of the &ref device.
+The device also connects to two IIO trigger inputs, named "continuous" and
+"external". The continuous trigger is connected to the trigger output 0 of the
+&adc device, and the external trigger is connected to the trigger output 1 of
+the same &adc device.
 
 ==Example==
 
@@ -78,6 +120,7 @@ vdd channel is connected to output 0 of the &ref device.
 		compatible = "maxim,max1139";
 		reg = <0x35>;
 		#io-channel-cells = <1>;
+		#io-trigger-cells = <1>;
 	};
 
 	...
@@ -94,4 +137,11 @@ vdd channel is connected to output 0 of the &ref device.
 		compatible = "some-consumer";
 		io-channels = <&adc 10>, <&adc 11>;
 		io-channel-names = "adc1", "adc2";
+		io-triggers = <&adc 0>;
+	};
+
+	some_other_consumer {
+		compatible = "some-other-consumer";
+		io-triggers = <&adc 1>, <&adc 2>;
+		io-trigger-names = "edge-triggered", "pwm-generated";
 	};
-- 
2.7.4

[PATCH 04/14] dt-bindings: input: touchscreen: sama5d2_rts: create bindings

[Eugen Hristev]

Added bindings for Microchip SAMA5D2 resistive touchscreen.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 .../bindings/input/touchscreen/sama5d2_rts.txt     | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt b/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
new file mode 100644
index 0000000..ea52a5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
@@ -0,0 +1,31 @@
+Microchip SAMA5D2 resistive touchscreen
+
+Required properties:
+
+ - compatible: microchip,sama5d2-resistive-touch
+
+Optional properties:
+
+The device must be connected to an IIO device that provides channels for
+position and pressure measurement, and a trigger output that will periodically
+trigger when the touch is pressed.
+Refer to ../iio/iio-bindings.txt for details
+
+ - io-channels: can have two or three channels connected to an IIO device.
+These should correspond to the channels exposed by the IIO device and should
+have the right index as the ADC registers them.
+ - io-channel-names: must have the two or three channels' names :
+"x", "y", or "x", "y" and "pressure"
+ - io-triggers: can have a trigger input connected to an IIO device.
+ - microchip,pressure-threshold: a pressure threshold for the touchscreen.
+   Represented by an integer value.
+
+Example:
+
+	resistive_touch: resistive-touch {
+		compatible = "microchip,sama5d2-resistive-touch";
+		microchip,pressure-threshold = <3000>;
+		io-channels = <&adc 19>, <&adc 20>, <&adc 21>;
+		io-channel-names = "x", "y", "pressure";
+		io-triggers = <&adc 1>;
+	};
-- 
2.7.4

[PATCH 05/14] iio: triggers: add helper function to retrieve trigger

[Eugen Hristev]

Add a helper function that will retrieve a trigger by index
from a device. This is intended to be used in trigger consumer drivers.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/iio_core_trigger.h     | 21 +++++++++++++++++++++
 drivers/iio/industrialio-trigger.c | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/iio/iio_core_trigger.h b/drivers/iio/iio_core_trigger.h
index 1fdb1e4..14c4253 100644
--- a/drivers/iio/iio_core_trigger.h
+++ b/drivers/iio/iio_core_trigger.h
@@ -21,6 +21,15 @@ void iio_device_register_trigger_consumer(struct iio_dev *indio_dev);
  **/
 void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev);
 
+/**
+ * iio_trigger_find_from_device() - get the trigger from the device having the
+ * index given.
+ * @indio_dev: iio_dev where to get the trigger from
+ * @index: the index of the trigger to retrieve
+ **/
+__maybe_unused struct iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index);
+
 #else
 
 /**
@@ -40,4 +49,16 @@ static void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
 {
 }
 
+/**
+ * iio_trigger_find_from_device() - get the trigger from the device having the
+ * index given.
+ * @indio_dev: iio_dev where to get the trigger from
+ * @index: the index of the trigger to retrieve
+ **/
+static __maybe_unused struct iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_TRIGGER_CONSUMER */
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ce66699..cbaa079 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -134,6 +134,29 @@ int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri
 }
 EXPORT_SYMBOL(iio_trigger_set_immutable);
 
+/* Find the trigger from the given device corresponding to given index */
+struct __maybe_unused iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index)
+{
+	struct iio_trigger *iter, *trig = NULL;
+	u32 i = 0;
+
+	mutex_lock(&iio_trigger_list_lock);
+
+	list_for_each_entry(iter, &iio_trigger_list, list) {
+		if (!iio_trigger_validate_own_device(iter, indio_dev)) {
+			if (i == index) {
+				trig = iter;
+				break;
+			}
+			i++;
+		}
+	}
+	mutex_unlock(&iio_trigger_list_lock);
+
+	return trig;
+}
+
 /* Search for trigger by name, assuming iio_trigger_list_lock held */
 static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
 {
-- 
2.7.4

[PATCH 06/14] iio: triggers: expose attach and detach helpers for pollfuncs

[Eugen Hristev]

Make attach and detach functions for pollfuncs available as symbols.
These functions are required in the trigger consumer drivers in order
to register their own pollfunctions to iio devices.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/industrialio-trigger.c   | 10 ++++++----
 include/linux/iio/trigger_consumer.h |  9 +++++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index cbaa079..8565c92 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -265,8 +265,8 @@ static void iio_trigger_put_irq(struct iio_trigger *trig, int irq)
  * the relevant function is in there may be the best option.
  */
 /* Worth protecting against double additions? */
-static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
-					struct iio_poll_func *pf)
+int iio_trigger_attach_poll_func(struct iio_trigger *trig,
+				 struct iio_poll_func *pf)
 {
 	int ret = 0;
 	bool notinuse
@@ -312,9 +312,10 @@ static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	module_put(pf->indio_dev->driver_module);
 	return ret;
 }
+EXPORT_SYMBOL(iio_trigger_attach_poll_func);
 
-static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
-					 struct iio_poll_func *pf)
+int iio_trigger_detach_poll_func(struct iio_trigger *trig,
+				 struct iio_poll_func *pf)
 {
 	int ret = 0;
 	bool no_other_users
@@ -334,6 +335,7 @@ static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
 
 	return ret;
 }
+EXPORT_SYMBOL(iio_trigger_detach_poll_func);
 
 irqreturn_t iio_pollfunc_store_time(int irq, void *p)
 {
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index c4f8c74..aeefcdb 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -60,4 +60,13 @@ void iio_trigger_notify_done(struct iio_trigger *trig);
 int iio_triggered_buffer_postenable(struct iio_dev *indio_dev);
 int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
 
+/*
+ * Two functions for the uncommon case when we need to attach or detach
+ * a specific pollfunc to and from a trigger
+ */
+int iio_trigger_attach_poll_func(struct iio_trigger *trig,
+				 struct iio_poll_func *pf);
+
+int iio_trigger_detach_poll_func(struct iio_trigger *trig,
+				 struct iio_poll_func *pf);
 #endif
-- 
2.7.4

[PATCH 07/14] iio: triggers: on pollfunc attach, complete iio_dev if NULL

[Eugen Hristev]

When attaching a pollfunc to a trigger, if the pollfunc does not
have an associated iio_dev pointer, just use the private data
iio_dev pointer from the trigger to fill in the poll func required
iio_dev reference.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/industrialio-trigger.c   | 9 +++++++++
 include/linux/iio/trigger_consumer.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 8565c92..ab180bd 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -272,6 +272,15 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	bool notinuse
 		= bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
 
+	/*
+	 * If we did not get a iio_dev in the poll func, attempt to
+	 * obtain the trigger's owner's device struct
+	 */
+	if (!pf->indio_dev)
+		pf->indio_dev = iio_trigger_get_drvdata(trig);
+	if (!pf->indio_dev)
+		return -EINVAL;
+
 	/* Prevent the module from being removed whilst attached to a trigger */
 	__module_get(pf->indio_dev->driver_module);
 
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index aeefcdb..36e2a02 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -63,6 +63,8 @@ int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
 /*
  * Two functions for the uncommon case when we need to attach or detach
  * a specific pollfunc to and from a trigger
+ * If the pollfunc has a NULL iio_dev pointer, it will be filled from the
+ * trigger struct.
  */
 int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 				 struct iio_poll_func *pf);
-- 
2.7.4

[PATCH 08/14] iio: triggers: add private data to pollfuncs

[Eugen Hristev]

Add a private data pointer field to pollfunc struct.
This is useful in the trigger handler to get specific data
for the driver registering the pollfunc.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 include/linux/iio/trigger_consumer.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 36e2a02..13be595 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -29,6 +29,7 @@ struct iio_trigger;
  * @timestamp:			some devices need a timestamp grabbed as soon
  *				as possible after the trigger - hence handler
  *				passes it via here.
+ * @p:				private data for the poll func owner.
  **/
 struct iio_poll_func {
 	struct iio_dev *indio_dev;
@@ -38,6 +39,7 @@ struct iio_poll_func {
 	char *name;
 	int irq;
 	s64 timestamp;
+	void *p;
 };
 
 
@@ -49,6 +51,13 @@ struct iio_poll_func
 		    const char *fmt,
 		    ...);
 void iio_dealloc_pollfunc(struct iio_poll_func *pf);
+
+static inline void
+iio_pollfunc_set_private_data(struct iio_poll_func *pf, void *p)
+{
+	pf->p = p;
+}
+
 irqreturn_t iio_pollfunc_store_time(int irq, void *p);
 
 void iio_trigger_notify_done(struct iio_trigger *trig);
-- 
2.7.4

[PATCH 09/14] iio: inkern: triggers: create helpers for OF trigger retrieval

[Eugen Hristev]

Create helper API to get trigger information from OF regarding
trigger producer/consumer for iio triggers.
The functions will search for matching trigger by name, similar
with channel retrieval

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/inkern.c                 | 91 ++++++++++++++++++++++++++++++++++++
 include/linux/iio/trigger_consumer.h |  1 +
 2 files changed, 92 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 069defc..58bd18d 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -14,9 +14,13 @@
 
 #include <linux/iio/iio.h>
 #include "iio_core.h"
+#include "iio_core_trigger.h"
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
 #include <linux/iio/consumer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+
 
 struct iio_map_internal {
 	struct iio_dev *indio_dev;
@@ -262,6 +266,39 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
 	return ERR_PTR(ret);
 }
 
+#ifdef CONFIG_IIO_TRIGGER
+
+static struct iio_trigger *of_iio_trigger_get(struct device_node *np, int index)
+{
+	struct device *idev;
+	struct iio_dev *indio_dev;
+	int err;
+	struct of_phandle_args iiospec;
+	struct iio_trigger *trig;
+
+	err = of_parse_phandle_with_args(np, "io-triggers",
+					 "#io-trigger-cells",
+					 index, &iiospec);
+	if (err)
+		return ERR_PTR(err);
+
+	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
+			       iio_dev_node_match);
+	of_node_put(iiospec.np);
+	if (!idev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	indio_dev = dev_to_iio_dev(idev);
+
+	trig = iio_trigger_find_from_device(indio_dev, iiospec.args[0]);
+
+	if (!trig)
+		return ERR_PTR(-ENODEV);
+
+	return trig;
+}
+#endif /* CONFIG_IIO_TRIGGER */
+
 #else /* CONFIG_OF */
 
 static inline struct iio_channel *
@@ -275,6 +312,12 @@ static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
 	return NULL;
 }
 
+static inline struct iio_trigger *of_iio_trigger_get(struct device_node *np,
+						     int index)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_OF */
 
 static struct iio_channel *iio_channel_get_sys(const char *name,
@@ -927,3 +970,51 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
 			       chan->channel, buf, len);
 }
 EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
+
+#ifdef CONFIG_IIO_TRIGGER
+struct iio_trigger *iio_trigger_find(struct device *dev, char *name)
+{
+	struct device_node *np = dev->of_node;
+	struct iio_trigger *trig = NULL;
+
+	/* Walk up the tree of devices looking for a matching iio trigger */
+	while (np) {
+		int index = 0;
+
+		/*
+		 * For named iio triggers, first look up the name in the
+		 * "io-trigger-names" property.  If it cannot be found, the
+		 * index will be an error code, and of_iio_trigger_get()
+		 * will fail.
+		 */
+		if (name)
+			index = of_property_match_string(np, "io-trigger-names",
+							 name);
+		trig = of_iio_trigger_get(np, index);
+		if (!IS_ERR(trig) || PTR_ERR(trig) == -EPROBE_DEFER) {
+			break;
+		} else if (name && index >= 0) {
+			pr_err("ERROR: could not get IIO trigger %pOF:%s(%i)\n",
+			       np, name ? name : "", index);
+			return trig;
+		}
+
+		/*
+		 * No matching IIO trigger found on this node.
+		 * If the parent node has a "io-trigger-ranges" property,
+		 * then we can try one of its channels.
+		 */
+		np = np->parent;
+		if (np && !of_get_property(np, "io-trigger-ranges", NULL))
+			return trig;
+	}
+
+	if (!trig || (IS_ERR(trig) && PTR_ERR(trig) != -EPROBE_DEFER))
+		dev_dbg(dev, "error retrieving trigger information\n");
+	else
+		dev_dbg(dev, "trigger found: %s\n", name);
+
+	return trig;
+}
+EXPORT_SYMBOL_GPL(iio_trigger_find);
+#endif /* CONFIG_IIO_TRIGGER */
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 13be595..b2fc485 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -62,6 +62,7 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p);
 
 void iio_trigger_notify_done(struct iio_trigger *trig);
 
+struct iio_trigger *iio_trigger_find(struct device *dev, char *name);
 /*
  * Two functions for common case where all that happens is a pollfunc
  * is attached and detached from a trigger
-- 
2.7.4

[PATCH 10/14] iio: adc: at91-sama5d2_adc: force trigger removal on module remove

[Eugen Hristev]

On module remove, if we do not call trigger remove, the trigger
stays in the subsystem, and on further module insert, we will have
multiple triggers, and the old one is not usable.
Have to call the remove function on module remove to solve this.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4eff835..7b9febc 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1180,6 +1180,9 @@ static int at91_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
+	if (st->selected_trig->hw_trig)
+		devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
+
 	iio_device_unregister(indio_dev);
 
 	at91_adc_dma_disable(pdev);
-- 
2.7.4

[PATCH 11/14] iio: adc: at91-sama5d2_adc: optimize scan index for diff channels

[Eugen Hristev]

Optimize the scan index for the differential channels. Before, it
was single channel count + index of the first single channel
number of the differential pair. (e.g. 11+0, +2, +4, etc.)
Divide that number by two (since it's always even), and add it up
as a scan index to have consecutive numbered channels in the
index.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7b9febc..9610393 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -209,7 +209,7 @@
 		.channel = num,						\
 		.channel2 = num2,					\
 		.address = addr,					\
-		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
+		.scan_index = (num >> 1) + AT91_SAMA5D2_SINGLE_CHAN_CNT,\
 		.scan_type = {						\
 			.sign = 's',					\
 			.realbits = 12,					\
-- 
2.7.4

[PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Eugen Hristev]

The ADC IP supports position and pressure measurements for a touchpad
connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
measurement support.
Using the inkern API, a driver can request a trigger and read the
channel values from the ADC.
The implementation provides a trigger named "touch" which can be
connected to a consumer driver.
Once a driver connects and attaches a pollfunc to this trigger, the
configure trigger callback is called, and then the ADC driver will
initialize pad measurement.
First step is to enable touchscreen 4wire support and enable
pen detect IRQ.
Once a pen is detected, a periodic trigger is setup to trigger every
2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
is called, and the consumer driver is then woke up, and it can read the
respective channels for the values : X, and Y for position and pressure
channel.
Because only one trigger can be active in hardware in the same time,
while touching the pad, the ADC will block any attempt to use the
triggered buffer. Same, conversions using the software trigger are also
impossible (since the periodic trigger is setup).
If some driver wants to attach while the trigger is in use, it will
also fail.
Once the pen is not detected anymore, the trigger is free for use (hardware
or software trigger, with or without DMA).
Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.

Some parts of this patch are based on initial original work by
Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 455 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 446 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 9610393..79eb197 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -102,14 +102,26 @@
 #define AT91_SAMA5D2_LCDR	0x20
 /* Interrupt Enable Register */
 #define AT91_SAMA5D2_IER	0x24
+/* Interrupt Enable Register - TS X measurement ready */
+#define AT91_SAMA5D2_IER_XRDY   BIT(20)
+/* Interrupt Enable Register - TS Y measurement ready */
+#define AT91_SAMA5D2_IER_YRDY   BIT(21)
+/* Interrupt Enable Register - TS pressure measurement ready */
+#define AT91_SAMA5D2_IER_PRDY   BIT(22)
 /* Interrupt Enable Register - general overrun error */
 #define AT91_SAMA5D2_IER_GOVRE BIT(25)
+/* Interrupt Enable Register - Pen detect */
+#define AT91_SAMA5D2_IER_PEN    BIT(29)
+/* Interrupt Enable Register - No pen detect */
+#define AT91_SAMA5D2_IER_NOPEN  BIT(30)
 /* Interrupt Disable Register */
 #define AT91_SAMA5D2_IDR	0x28
 /* Interrupt Mask Register */
 #define AT91_SAMA5D2_IMR	0x2c
 /* Interrupt Status Register */
 #define AT91_SAMA5D2_ISR	0x30
+/* Interrupt Status Register - Pen touching sense status */
+#define AT91_SAMA5D2_ISR_PENS   BIT(31)
 /* Last Channel Trigger Mode Register */
 #define AT91_SAMA5D2_LCTMR	0x34
 /* Last Channel Compare Window Register */
@@ -131,8 +143,37 @@
 #define AT91_SAMA5D2_CDR0	0x50
 /* Analog Control Register */
 #define AT91_SAMA5D2_ACR	0x94
+/* Analog Control Register - Pen detect sensitivity mask */
+#define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(0, 1)
 /* Touchscreen Mode Register */
 #define AT91_SAMA5D2_TSMR	0xb0
+/* Touchscreen Mode Register - No touch mode */
+#define AT91_SAMA5D2_TSMR_TSMODE_NONE           0
+/* Touchscreen Mode Register - 4 wire screen, no pressure measurement */
+#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_NO_PRESS 1
+/* Touchscreen Mode Register - 4 wire screen, pressure measurement */
+#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS    2
+/* Touchscreen Mode Register - 5 wire screen */
+#define AT91_SAMA5D2_TSMR_TSMODE_5WIRE          3
+/* Touchscreen Mode Register - Average samples mask */
+#define AT91_SAMA5D2_TSMR_TSAV_MASK		(3 << 4)
+/* Touchscreen Mode Register - Average samples */
+#define AT91_SAMA5D2_TSMR_TSAV(x)		((x) << 4)
+/* Touchscreen Mode Register - Touch/trigger frequency ratio mask */
+#define AT91_SAMA5D2_TSMR_TSFREQ_MASK		(0xf << 8)
+/* Touchscreen Mode Register - Touch/trigger freqency ratio */
+#define AT91_SAMA5D2_TSMR_TSFREQ(x)		((x) << 8)
+/* Touchscreen Mode Register - Pen Debounce Time mask */
+#define AT91_SAMA5D2_TSMR_PENDBC_MASK		(0xf << 28)
+/* Touchscreen Mode Register - Pen Debounce Time */
+#define AT91_SAMA5D2_TSMR_PENDBC(x)            ((x) << 28)
+/* Touchscreen Mode Register - No DMA for touch measurements */
+#define AT91_SAMA5D2_TSMR_NOTSDMA               BIT(22)
+/* Touchscreen Mode Register - Disable pen detection */
+#define AT91_SAMA5D2_TSMR_PENDET_DIS            (0 << 24)
+/* Touchscreen Mode Register - Enable pen detection */
+#define AT91_SAMA5D2_TSMR_PENDET_ENA            BIT(24)
+
 /* Touchscreen X Position Register */
 #define AT91_SAMA5D2_XPOSR	0xb4
 /* Touchscreen Y Position Register */
@@ -151,7 +192,12 @@
 #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
 /* Trigger Mode external trigger any edge */
 #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
-
+/* Trigger Mode internal periodic */
+#define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
+/* Trigger Mode - trigger period mask */
+#define AT91_SAMA5D2_TRGR_TRGPER_MASK		(0xffff << 16)
+/* Trigger Mode - trigger period */
+#define AT91_SAMA5D2_TRGR_TRGPER(x)		((x) << 16)
 /* Correction Select Register */
 #define AT91_SAMA5D2_COSR	0xd0
 /* Correction Value Register */
@@ -169,6 +215,21 @@
 #define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
 #define AT91_SAMA5D2_DIFF_CHAN_CNT 6
 
+#define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX	(AT91_SAMA5D2_SINGLE_CHAN_CNT + \
+					AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
+
+#define AT91_SAMA5D2_TOUCH_X_CHAN_IDX	(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX + 1)
+#define AT91_SAMA5D2_TOUCH_Y_CHAN_IDX	(AT91_SAMA5D2_TOUCH_X_CHAN_IDX + 1)
+#define AT91_SAMA5D2_TOUCH_P_CHAN_IDX	(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX + 1)
+
+#define TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */
+#define TOUCH_PEN_DETECT_DEBOUNCE_US    200
+
+#define XYZ_MASK			GENMASK(11, 0)
+
+#define MAX_POS_BITS			12
+
+#define AT91_ADC_TOUCH_TRIG_SHORTNAME	"touch"
 /*
  * Maximum number of bytes to hold conversion from all channels
  * without the timestamp.
@@ -222,6 +283,37 @@
 		.indexed = 1,						\
 	}
 
+#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)				\
+	{								\
+		.type = IIO_POSITION,					\
+		.modified = 1,						\
+		.channel = num,						\
+		.channel2 = mod,					\
+		.scan_index = num,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 12,					\
+			.storagebits = 16,				\
+		},							\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+		.datasheet_name = name,					\
+	}
+#define AT91_SAMA5D2_CHAN_PRESSURE(num, name)				\
+	{								\
+		.type = IIO_PRESSURE,					\
+		.channel = num,						\
+		.scan_index = num,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 12,					\
+			.storagebits = 16,				\
+		},							\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+		.datasheet_name = name,					\
+	}
+
 #define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
 #define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
 
@@ -239,6 +331,20 @@ struct at91_adc_trigger {
 };
 
 /**
+ * at91_adc_touch - at91-sama5d2 touchscreen information struct
+ * @trig:			hold the start timestamp of dma operation
+ * @sample_period_val:		the value for periodic trigger interval
+ * @touching:			is the pen touching the screen or not
+ * @x_pos:			temporary placeholder for pressure computation
+ */
+struct at91_adc_touch {
+	struct iio_trigger		*trig;
+	u16				sample_period_val;
+	bool				touching;
+	u32				x_pos;
+};
+
+/**
  * at91_adc_dma - at91-sama5d2 dma information struct
  * @dma_chan:		the dma channel acquired
  * @rx_buf:		dma coherent allocated area
@@ -267,18 +373,22 @@ struct at91_adc_state {
 	struct regulator		*reg;
 	struct regulator		*vref;
 	int				vref_uv;
+	unsigned int			current_sample_rate;
 	struct iio_trigger		*trig;
 	const struct at91_adc_trigger	*selected_trig;
 	const struct iio_chan_spec	*chan;
 	bool				conversion_done;
 	u32				conversion_value;
+	bool				touch_requested;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
 	struct at91_adc_dma		dma_st;
+	struct at91_adc_touch		touch_st;
 	u16				buffer[AT91_BUFFER_MAX_HWORDS];
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
-	 * sysfs.
+	 * sysfs. Also protects when enabling or disabling touchscreen
+	 * producer mode and checking if this mode is enabled or not.
 	 */
 	struct mutex			lock;
 };
@@ -310,6 +420,7 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
 	},
 };
 
+/* channel order is not subject to change. inkern consumers rely on this */
 static const struct iio_chan_spec at91_adc_channels[] = {
 	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
 	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
@@ -329,10 +440,103 @@ static const struct iio_chan_spec at91_adc_channels[] = {
 	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
 	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
 	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
-	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
-				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
+	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX),
+	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_X_CHAN_IDX, "x", IIO_MOD_X),
+	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, "y", IIO_MOD_Y),
+	AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
 };
 
+static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
+{
+	u32 clk_khz = st->current_sample_rate / 1000;
+	int i = 0;
+	u16 pendbc;
+	u32 tsmr, acr;
+
+	if (!state) {
+		/* disabling touch IRQs and setting mode to no touch enabled */
+		at91_adc_writel(st, AT91_SAMA5D2_IDR,
+				AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
+		at91_adc_writel(st, AT91_SAMA5D2_TSMR, 0);
+		return 0;
+	}
+	/*
+	 * debounce time is in microseconds, we need it in milliseconds to
+	 * multiply with kilohertz, so, divide by 1000, but after the multiply.
+	 * round up to make sure pendbc is at least 1
+	 */
+	pendbc = round_up(TOUCH_PEN_DETECT_DEBOUNCE_US * clk_khz / 1000, 1);
+
+	/* get the required exponent */
+	while (pendbc >> i++)
+		;
+
+	pendbc = i;
+
+	tsmr = AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS;
+
+	tsmr |= AT91_SAMA5D2_TSMR_TSAV(1) & AT91_SAMA5D2_TSMR_TSAV_MASK;
+	tsmr |= AT91_SAMA5D2_TSMR_PENDBC(pendbc) &
+		AT91_SAMA5D2_TSMR_PENDBC_MASK;
+	tsmr |= AT91_SAMA5D2_TSMR_NOTSDMA;
+	tsmr |= AT91_SAMA5D2_TSMR_PENDET_ENA;
+	tsmr |= AT91_SAMA5D2_TSMR_TSFREQ(1) & AT91_SAMA5D2_TSMR_TSFREQ_MASK;
+
+	at91_adc_writel(st, AT91_SAMA5D2_TSMR, tsmr);
+
+	acr =  at91_adc_readl(st, AT91_SAMA5D2_ACR);
+	acr &= ~AT91_SAMA5D2_ACR_PENDETSENS_MASK;
+	acr |= 0x02 & AT91_SAMA5D2_ACR_PENDETSENS_MASK;
+	at91_adc_writel(st, AT91_SAMA5D2_ACR, acr);
+
+	/* Sample Period Time = (TRGPER + 1) / ADCClock */
+	st->touch_st.sample_period_val = round_up((TOUCH_SAMPLE_PERIOD_US *
+					 clk_khz / 1000) - 1, 1);
+	/* enable pen detect IRQ */
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
+
+	return 0;
+}
+
+static int at91_adc_touch_trigger_validate_device(struct iio_trigger *trig,
+						  struct iio_dev *indio_dev)
+{
+	/* the touch trigger cannot be used with a buffer */
+	return -EBUSY;
+}
+
+static int at91_adc_configure_touch_trigger(struct iio_trigger *trig,
+					    bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	/*
+	 * If we configure this with the IRQ enabled, the pen detected IRQ
+	 * might fire before we finish setting all up, and the IRQ handler
+	 * might misbehave. Better to reenable the IRQ after we are done
+	 */
+	disable_irq_nosync(st->irq);
+
+	mutex_lock(&st->lock);
+	if (state) {
+		ret = iio_buffer_enabled(indio_dev);
+		if (ret) {
+			dev_dbg(&indio_dev->dev, "trigger is currently in use\n");
+			ret = -EBUSY;
+			goto configure_touch_unlock_exit;
+		}
+	}
+	at91_adc_configure_touch(st, state);
+	st->touch_requested = state;
+
+configure_touch_unlock_exit:
+	enable_irq(st->irq);
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
 static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
@@ -390,12 +594,27 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
 	return 0;
 }
 
+static int at91_adc_reenable_touch_trigger(struct iio_trigger *trig)
+{
+	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
+	struct at91_adc_state *st = iio_priv(indio);
+
+	enable_irq(st->irq);
+
+	return 0;
+}
 static const struct iio_trigger_ops at91_adc_trigger_ops = {
 	.set_trigger_state = &at91_adc_configure_trigger,
 	.try_reenable = &at91_adc_reenable_trigger,
 	.validate_device = iio_trigger_validate_own_device,
 };
 
+static const struct iio_trigger_ops at91_adc_touch_trigger_ops = {
+	.set_trigger_state = &at91_adc_configure_touch_trigger,
+	.try_reenable = &at91_adc_reenable_touch_trigger,
+	.validate_device = &at91_adc_touch_trigger_validate_device,
+};
+
 static int at91_adc_dma_size_done(struct at91_adc_state *st)
 {
 	struct dma_tx_state state;
@@ -490,6 +709,23 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+
+	/* have to make sure nobody is requesting the trigger right now */
+	mutex_lock(&st->lock);
+	ret = st->touch_requested;
+	mutex_unlock(&st->lock);
+
+	/*
+	 * if the trigger is used by the touchscreen,
+	 * we must return an error
+	 */
+	return ret ? -EBUSY : 0;
+}
+
 static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
 {
 	int ret;
@@ -538,6 +774,7 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
 }
 
 static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
+	.preenable = &at91_adc_buffer_preenable,
 	.postenable = &at91_adc_buffer_postenable,
 	.predisable = &at91_adc_buffer_predisable,
 };
@@ -555,7 +792,11 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
 
 	trig->dev.parent = indio->dev.parent;
 	iio_trigger_set_drvdata(trig, indio);
-	trig->ops = &at91_adc_trigger_ops;
+
+	if (strcmp(trigger_name, AT91_ADC_TOUCH_TRIG_SHORTNAME))
+		trig->ops = &at91_adc_trigger_ops;
+	else
+		trig->ops = &at91_adc_touch_trigger_ops;
 
 	ret = devm_iio_trigger_register(&indio->dev, trig);
 	if (ret)
@@ -571,7 +812,16 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
 	st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
 	if (IS_ERR(st->trig)) {
 		dev_err(&indio->dev,
-			"could not allocate trigger\n");
+			"could not allocate trigger %s\n",
+			 st->selected_trig->name);
+		return PTR_ERR(st->trig);
+	}
+
+	st->touch_st.trig = at91_adc_allocate_trigger(indio,
+						AT91_ADC_TOUCH_TRIG_SHORTNAME);
+	if (IS_ERR(st->trig)) {
+		dev_err(&indio->dev, "could not allocate trigger"
+			AT91_ADC_TOUCH_TRIG_SHORTNAME "\n");
 		return PTR_ERR(st->trig);
 	}
 
@@ -703,6 +953,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
 
 	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
 		freq, startup, prescal);
+
+	st->current_sample_rate = freq;
 }
 
 static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
@@ -718,23 +970,77 @@ static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
 	return f_adc;
 }
 
+static irqreturn_t at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
+{
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_PEN);
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_NOPEN |
+			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
+			AT91_SAMA5D2_IER_PRDY);
+	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
+			AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC |
+			AT91_SAMA5D2_TRGR_TRGPER(st->touch_st.sample_period_val));
+	st->touch_st.touching = true;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
+{
+	at91_adc_writel(st, AT91_SAMA5D2_TRGR, 0);
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
+			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
+			AT91_SAMA5D2_IER_PRDY);
+	st->touch_st.touching = false;
+
+	disable_irq_nosync(st->irq);
+	iio_trigger_poll(st->touch_st.trig);
+
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t at91_adc_interrupt(int irq, void *private)
 {
 	struct iio_dev *indio = private;
 	struct at91_adc_state *st = iio_priv(indio);
 	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
 	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
+	u32 rdy_mask = AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
+			AT91_SAMA5D2_IER_PRDY;
 
 	if (!(status & imr))
 		return IRQ_NONE;
 
-	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
+	if (st->touch_requested && (status & AT91_SAMA5D2_IER_PEN)) {
+		/* pen detected IRQ */
+		return at91_adc_pen_detect_interrupt(st);
+	} else if (st->touch_requested && (status & AT91_SAMA5D2_IER_NOPEN)) {
+		/* nopen detected IRQ */
+		return at91_adc_no_pen_detect_interrupt(st);
+	} else if (st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS) &&
+		   ((status & rdy_mask) == rdy_mask)) {
+		/* periodic trigger IRQ - during pen sense */
+		disable_irq_nosync(irq);
+		iio_trigger_poll(st->touch_st.trig);
+	} else if ((st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS))) {
+		/*
+		 * touching, but the measurements are not ready yet.
+		 * read and ignore.
+		 */
+		status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
+		status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
+		status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
+	} else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
+		/* buffered trigger without DMA */
 		disable_irq_nosync(irq);
 		iio_trigger_poll(indio->trig);
 	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
+		/* buffered trigger with DMA - should not happen */
 		disable_irq_nosync(irq);
 		WARN(true, "Unexpected irq occurred\n");
 	} else if (!iio_buffer_enabled(indio)) {
+		/* software requested conversion */
 		st->conversion_value = at91_adc_readl(st, st->chan->address);
 		st->conversion_done = true;
 		wake_up_interruptible(&st->wq_data_available);
@@ -742,6 +1048,96 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static u32 at91_adc_touch_x_pos(struct at91_adc_state *st)
+{
+	u32 xscale, val;
+	u32 x, xpos;
+
+	/* x position = (x / xscale) * max, max = 2^MAX_POS_BITS - 1 */
+	val = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
+	if (!val)
+		dev_dbg(&iio_priv_to_dev(st)->dev, "x_pos is 0\n");
+
+	xpos = val & XYZ_MASK;
+	x = (xpos << MAX_POS_BITS) - xpos;
+	xscale = (val >> 16) & XYZ_MASK;
+	if (xscale == 0) {
+		dev_err(&iio_priv_to_dev(st)->dev, "xscale is 0\n");
+		return 0;
+	}
+	x /= xscale;
+	st->touch_st.x_pos = x;
+
+	return x;
+}
+
+static u32 at91_adc_touch_y_pos(struct at91_adc_state *st)
+{
+	u32 yscale, val;
+	u32 y, ypos;
+
+	/* y position = (y / yscale) * max, max = 2^MAX_POS_BITS - 1 */
+	val = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
+	ypos = val & XYZ_MASK;
+	y = (ypos << MAX_POS_BITS) - ypos;
+	yscale = (val >> 16) & XYZ_MASK;
+
+	if (yscale == 0)
+		return 0;
+
+	y /= yscale;
+
+	return y;
+}
+
+static u32 at91_adc_touch_pressure(struct at91_adc_state *st)
+{
+	u32 val, z1, z2;
+	u32 pres;
+	u32 rxp = 1;
+	u32 factor = 1000;
+
+	/* calculate the pressure */
+	val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
+	z1 = val & XYZ_MASK;
+	z2 = (val >> 16) & XYZ_MASK;
+
+	if (z1 != 0)
+		pres = rxp * (st->touch_st.x_pos * factor / 1024) *
+			(z2 * factor / z1 - factor) /
+			factor;
+	else
+		pres = 0xFFFFFFFF;       /* no pen contact */
+
+	return pres;
+}
+
+static int at91_adc_read_position(struct at91_adc_state *st, int chan, int *val)
+{
+	if (!st->touch_st.touching)
+		return -ENODATA;
+	if (chan == AT91_SAMA5D2_TOUCH_X_CHAN_IDX)
+		*val = at91_adc_touch_x_pos(st);
+	else if (chan == AT91_SAMA5D2_TOUCH_Y_CHAN_IDX)
+		*val = at91_adc_touch_y_pos(st);
+	else
+		return -ENODATA;
+
+	return IIO_VAL_INT;
+}
+
+static int at91_adc_read_pressure(struct at91_adc_state *st, int chan, int *val)
+{
+	if (!st->touch_st.touching)
+		return -ENODATA;
+	if (chan == AT91_SAMA5D2_TOUCH_P_CHAN_IDX)
+		*val = at91_adc_touch_pressure(st);
+	else
+		return -ENODATA;
+
+	return IIO_VAL_INT;
+}
+
 static int at91_adc_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
@@ -752,11 +1148,38 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+
+		if (chan->type == IIO_POSITION) {
+			ret = at91_adc_read_position(st, chan->channel, val);
+			mutex_unlock(&st->lock);
+			return ret;
+		}
+		if (chan->type == IIO_PRESSURE) {
+			ret = at91_adc_read_pressure(st, chan->channel, val);
+			mutex_unlock(&st->lock);
+			return ret;
+		}
+		/* if we using touch, channels 0, 1, 2, 3 are unavailable */
+		if (st->touch_requested && chan->channel <= 3) {
+			mutex_unlock(&st->lock);
+			return -EBUSY;
+		}
+		/*
+		 * if we have the periodic trigger set up, we can't use
+		 * software trigger either.
+		 */
+		if (st->touch_st.touching) {
+			mutex_unlock(&st->lock);
+			return -ENODATA;
+		}
+
 		/* we cannot use software trigger if hw trigger enabled */
 		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&st->lock);
 			return ret;
-		mutex_lock(&st->lock);
+		}
 
 		st->chan = chan;
 
@@ -785,6 +1208,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 
 		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
 		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
+		/*
+		 * It is possible that after this conversion, we reuse these
+		 * channels for the touchscreen. So, reset the COR now.
+		 */
+		at91_adc_writel(st, AT91_SAMA5D2_COR, 0);
 
 		/* Needed to ACK the DRDY interruption */
 		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
@@ -1180,6 +1608,10 @@ static int at91_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
+	mutex_lock(&st->lock);
+	devm_iio_trigger_unregister(&indio_dev->dev, st->touch_st.trig);
+	mutex_unlock(&st->lock);
+
 	if (st->selected_trig->hw_trig)
 		devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
 
@@ -1245,6 +1677,11 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
 	if (iio_buffer_enabled(indio_dev))
 		at91_adc_configure_trigger(st->trig, true);
 
+	mutex_lock(&st->lock);
+	if (st->touch_requested)
+		at91_adc_configure_touch_trigger(st->touch_st.trig, true);
+	mutex_unlock(&st->lock);
+
 	return 0;
 
 vref_disable_resume:
-- 
2.7.4

[PATCH 13/14] input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver

[Eugen Hristev]

This is the implementation of the Microchip SAMA5D2 SOC resistive
touchscreen driver.
The driver registers an input device and connects to the give IIO device
from devicetree. It requires an IIO trigger (acting as a consumer) and
three IIO channels : one for X position, one for Y position and one
for pressure.
It the reports the values to the input subsystem.

Some parts of this driver are based on the initial original work by
Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/input/touchscreen/Kconfig       |  13 ++
 drivers/input/touchscreen/Makefile      |   1 +
 drivers/input/touchscreen/sama5d2_rts.c | 287 ++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/input/touchscreen/sama5d2_rts.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 64b30fe..db8f541 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -126,6 +126,19 @@ config TOUCHSCREEN_ATMEL_MXT_T37
 	  Say Y here if you want support to output data from the T37
 	  Diagnostic Data object using a V4L device.
 
+config TOUCHSCREEN_SAMA5D2
+	tristate "Microchip SAMA5D2 resistive touchscreen support"
+	depends on ARCH_AT91
+	depends on AT91_SAMA5D2_ADC
+	help
+	  Say Y here if you have 4-wire touchscreen connected
+          to ADC Controller on your SAMA5D2 Microchip SoC.
+
+          If unsure, say N.
+
+          To compile this driver as a module, choose M here: the
+          module will be called sama5d2_rts.
+
 config TOUCHSCREEN_AUO_PIXCIR
 	tristate "AUO in-cell touchscreen using Pixcir ICs"
 	depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 850c156..9a2772e 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
+obj-$(CONFIG_TOUCHSCREEN_SAMA5D2)	+= sama5d2_rts.o
 obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR)	+= auo-pixcir-ts.o
 obj-$(CONFIG_TOUCHSCREEN_BU21013)	+= bu21013_ts.o
 obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318)	+= chipone_icn8318.o
diff --git a/drivers/input/touchscreen/sama5d2_rts.c b/drivers/input/touchscreen/sama5d2_rts.c
new file mode 100644
index 0000000..e2ae413
--- /dev/null
+++ b/drivers/input/touchscreen/sama5d2_rts.c
@@ -0,0 +1,287 @@
+/*
+ * Microchip resistive touchscreen (RTS) driver for SAMA5D2.
+ *
+ * Copyright (C) 2017 Microchip Technology,
+ * Author: Eugen Hristev <eugen.hristev@microchip.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+#include <linux/input.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define DRIVER_NAME					"sama5d2_rts"
+#define MAX_POS_MASK					GENMASK(11, 0)
+#define AT91_RTS_DEFAULT_PRESSURE_THRESHOLD		10000
+
+/**
+ * at91_rts - at91 resistive touchscreen information struct
+ * @input:		the input device structure that we register
+ * @chan_x:		X channel to IIO device to get position on X axis
+ * @chan_y:		Y channel to IIO device to get position on Y axis
+ * @chan_pressure:	pressure channel to IIO device to get pressure
+ * @trig:		trigger to IIO device to register to for polling
+ * @rts_pf:		pollfunc for the trigger to be called by IIO dev
+ * @pressure_threshold:	number representing the threshold for the pressure
+ * @adc_connected:	to know if adc device is connected
+ * @workq:		to defer computations to this work queue for reporting
+ */
+struct at91_rts {
+	struct input_dev	*input;
+	struct iio_channel	*chan_x, *chan_y, *chan_pressure;
+	struct iio_trigger	*trig;
+	struct iio_poll_func	*rts_pf;
+	u32                     pressure_threshold;
+	bool			adc_connected;
+	struct work_struct	workq;
+};
+
+static irqreturn_t at91_rts_trigger_handler(int irq, void *p)
+{
+	struct at91_rts *st = ((struct iio_poll_func *)p)->p;
+
+	schedule_work(&st->workq);
+
+	return IRQ_HANDLED;
+}
+
+static void at91_rts_workq_handler(struct work_struct *workq)
+{
+	struct at91_rts *st = container_of(workq, struct at91_rts, workq);
+	unsigned int x, y, press;
+	int ret;
+
+	/* read the channels, if all good, report touch */
+	ret = iio_read_channel_raw(st->chan_x, &x);
+	if (ret < 0)
+		goto at91_rts_workq_handler_end_touch;
+
+	ret = iio_read_channel_raw(st->chan_y, &y);
+	if (ret < 0)
+		goto at91_rts_workq_handler_end_touch;
+
+	ret = iio_read_channel_raw(st->chan_pressure, &press);
+	if (ret < 0)
+		goto at91_rts_workq_handler_end_touch;
+
+	/* if pressure too low, don't report */
+	if (press > st->pressure_threshold)
+		goto at91_rts_workq_handler_exit;
+
+	input_report_abs(st->input, ABS_X, x);
+	input_report_abs(st->input, ABS_Y, y);
+	input_report_abs(st->input, ABS_PRESSURE, press);
+	input_report_key(st->input, BTN_TOUCH, 1);
+	input_sync(st->input);
+
+	iio_trigger_notify_done(st->trig);
+	return;
+
+at91_rts_workq_handler_end_touch:
+	/* report end of touch */
+	input_report_key(st->input, BTN_TOUCH, 0);
+	input_sync(st->input);
+at91_rts_workq_handler_exit:
+	iio_trigger_notify_done(st->trig);
+}
+
+static int at91_rts_open(struct input_dev *dev)
+{
+	int ret;
+	struct at91_rts *st = input_get_drvdata(dev);
+
+	/* avoid multiple initialization in case touchscreen is opened again */
+	if (st->adc_connected)
+		return 0;
+
+	/*
+	 * First, look for the channels. It is possible that the ADC device
+	 * did not probe yet, but we already probed, so we returning probe defer
+	 * doesn't make much sense.
+	 */
+	st->chan_x = iio_channel_get(dev->dev.parent, "x");
+	if (IS_ERR_OR_NULL(st->chan_x)) {
+		dev_err(dev->dev.parent, "cannot get X channel from ADC");
+		ret = PTR_ERR(st->chan_x);
+		goto at91_rts_open_free_chan;
+	}
+
+	st->chan_y = iio_channel_get(dev->dev.parent, "y");
+	if (IS_ERR_OR_NULL(st->chan_y)) {
+		dev_err(dev->dev.parent, "cannot get Y channel from ADC");
+		ret = PTR_ERR(st->chan_y);
+		goto at91_rts_open_free_chan;
+	}
+
+	st->chan_pressure = iio_channel_get(dev->dev.parent, "pressure");
+	if (IS_ERR_OR_NULL(st->chan_pressure)) {
+		dev_err(dev->dev.parent, "cannot get pressure channel from ADC");
+		ret = PTR_ERR(st->chan_pressure);
+		goto at91_rts_open_free_chan;
+	}
+
+	/* look for the trigger in device tree */
+	st->trig = iio_trigger_find(dev->dev.parent, NULL);
+	if (IS_ERR_OR_NULL(st->trig)) {
+		dev_err(dev->dev.parent, "cannot get trigger from ADC");
+		ret = PTR_ERR(st->trig);
+		goto at91_rts_open_free_chan;
+	}
+
+	/* allocate a pollfunc for the trigger */
+	st->rts_pf = iio_alloc_pollfunc(at91_rts_trigger_handler, NULL,
+					IRQF_ONESHOT, NULL,
+					dev->dev.parent->of_node->name);
+	if (!st->rts_pf) {
+		ret = -ENOMEM;
+		dev_err(dev->dev.parent, "cannot allocate trigger pollfunc");
+		goto at91_rts_open_free_chan;
+	}
+
+	iio_pollfunc_set_private_data(st->rts_pf, st);
+
+	/*
+	 * Attach the pollfunc to the trigger. This will also call the
+	 * configure function to enable the trigger
+	 */
+	ret = iio_trigger_attach_poll_func(st->trig, st->rts_pf);
+	if (ret)
+		goto at91_rts_open_dealloc_pf;
+
+	dev_dbg(dev->dev.parent, "channels found, attached to trigger");
+
+	st->adc_connected = true;
+	return 0;
+
+at91_rts_open_dealloc_pf:
+	iio_dealloc_pollfunc(st->rts_pf);
+at91_rts_open_free_chan:
+	if (!IS_ERR_OR_NULL(st->chan_x))
+		iio_channel_release(st->chan_x);
+	if (!IS_ERR_OR_NULL(st->chan_y))
+		iio_channel_release(st->chan_y);
+	if (!IS_ERR_OR_NULL(st->chan_pressure))
+		iio_channel_release(st->chan_pressure);
+	/*
+	 * Avoid keeping old values in channel pointers. in case some channel
+	 * failed and we reopen them, and now fail, we will have invalid values
+	 * to release. So write them as NULL now.
+	 */
+	st->chan_x = NULL;
+	st->chan_y = NULL;
+	st->chan_pressure = NULL;
+	return ret;
+}
+
+static void at91_rts_close(struct input_dev *dev)
+{
+	struct at91_rts *st = input_get_drvdata(dev);
+
+	if (!st->adc_connected)
+		return;
+
+	iio_trigger_detach_poll_func(st->trig, st->rts_pf);
+	iio_dealloc_pollfunc(st->rts_pf);
+
+	if (!IS_ERR_OR_NULL(st->chan_x))
+		iio_channel_release(st->chan_x);
+	if (!IS_ERR_OR_NULL(st->chan_y))
+		iio_channel_release(st->chan_y);
+	if (!IS_ERR_OR_NULL(st->chan_pressure))
+		iio_channel_release(st->chan_pressure);
+
+	st->adc_connected = false;
+}
+
+static int at91_rts_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct at91_rts *st;
+	struct input_dev *input;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+
+	st = devm_kzalloc(dev, sizeof(struct at91_rts), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+	st->adc_connected = false;
+
+	INIT_WORK(&st->workq, at91_rts_workq_handler);
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	ret = of_property_read_u32(node, "microchip,pressure-threshold",
+				   &st->pressure_threshold);
+	if (ret < 0) {
+		dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
+		st->pressure_threshold = AT91_RTS_DEFAULT_PRESSURE_THRESHOLD;
+	}
+
+	input->name = DRIVER_NAME;
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = &pdev->dev;
+	input->open = at91_rts_open;
+	input->close = at91_rts_close;
+
+	input_set_abs_params(input, ABS_X, 0, MAX_POS_MASK - 1, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, MAX_POS_MASK, 0, 0);
+	input_set_abs_params(input, ABS_PRESSURE, 0, 0xffffff, 0, 0);
+
+	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	st->input = input;
+	input_set_drvdata(input, st);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "failed to register input device: %d", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, st);
+
+	dev_info(dev, "probed successfully\n");
+	return 0;
+}
+
+static int at91_rts_remove(struct platform_device *pdev)
+{
+	struct at91_rts *st = platform_get_drvdata(pdev);
+
+	input_unregister_device(st->input);
+
+	return 0;
+}
+
+static const struct of_device_id at91_rts_of_match[] = {
+	{
+		.compatible = "microchip,sama5d2-resistive-touch",
+	}, {
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, at91_rts_of_match);
+
+static struct platform_driver atmel_rts_driver = {
+	.probe = at91_rts_probe,
+	.remove = at91_rts_remove,
+	.driver = {
+		   .name = DRIVER_NAME,
+		   .of_match_table = of_match_ptr(at91_rts_of_match),
+	},
+};
+
+module_platform_driver(atmel_rts_driver);
+
+MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
+MODULE_DESCRIPTION("Microchip SAMA5D2 Resistive Touch Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 14/14] ARM: dts: at91: sama5d2: Add resistive touch device

[Eugen Hristev]

Add the resistive touchscreen device, and the cell numbers to the
ADC device.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/boot/dts/sama5d2.dtsi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index b1a26b4..30b3797 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -402,7 +402,6 @@
 					compatible = "atmel,hlcdc-display-controller";
 					#address-cells = <1>;
 					#size-cells = <0>;
-
 					port@0 {
 						#address-cells = <1>;
 						#size-cells = <0>;
@@ -1431,6 +1430,17 @@
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
 				atmel,trigger-edge-type = <IRQ_TYPE_EDGE_RISING>;
+				#io-channel-cells = <1>;
+				#io-trigger-cells = <1>;
+				status = "disabled";
+			};
+
+			resistive_touch: resistive-touch {
+				compatible = "microchip,sama5d2-resistive-touch";
+				io-channels = <&adc 19>, <&adc 20>, <&adc 21>;
+				io-channel-names = "x", "y", "pressure";
+				io-triggers = <&adc 1>;
+				microchip,pressure-threshold = <10000>;
 				status = "disabled";
 			};
 
-- 
2.7.4

Re: [PATCH 14/14] ARM: dts: at91: sama5d2: Add resistive touch device

[Alexandre Belloni]

Hi,

On 22/12/2017 at 17:07:21 +0200, Eugen Hristev wrote:
> Add the resistive touchscreen device, and the cell numbers to the
> ADC device.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  arch/arm/boot/dts/sama5d2.dtsi | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index b1a26b4..30b3797 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -402,7 +402,6 @@
>  					compatible = "atmel,hlcdc-display-controller";
>  					#address-cells = <1>;
>  					#size-cells = <0>;
> -

This is an unrelated change.

>  					port@0 {
>  						#address-cells = <1>;
>  						#size-cells = <0>;

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 13/14] input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver

[Philippe Ombredanne]

Eugen,

On Fri, Dec 22, 2017 at 4:07 PM, Eugen Hristev
<eugen.hristev@microchip.com> wrote:
> This is the implementation of the Microchip SAMA5D2 SOC resistive
> touchscreen driver.

<snip>

> --- /dev/null
> +++ b/drivers/input/touchscreen/sama5d2_rts.c
> @@ -0,0 +1,287 @@
> +/*
> + * Microchip resistive touchscreen (RTS) driver for SAMA5D2.
> + *
> + * Copyright (C) 2017 Microchip Technology,
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */


IMHO this should be this as a the first line instead as documented in
Thomas patch set [0] :

> +// SPDX-License-Identifier: GPL-2.0

I am just a lowly messenger and my weightless voice is as light as the
down feather of a new born goose. But if you have concerns with using
C++ comment styles as you should, please read Linus [1][2][3],
Thomas[4] and Greg[5] comments on the topic.


[0] https://lkml.org/lkml/2017/12/4/934
[1] https://lkml.org/lkml/2017/11/2/715
[2] https://lkml.org/lkml/2017/11/25/125
[3] https://lkml.org/lkml/2017/11/25/133
[4] https://lkml.org/lkml/2017/11/2/805
[5] https://lkml.org/lkml/2017/10/19/165

-- 
Cordially
Philippe Ombredanne, a kernel licensing scruffy

Re: [PATCH 03/14] dt-bindings: iio: add binding support for iio trigger provider/consumer

[Rob Herring]

On Fri, Dec 22, 2017 at 05:07:10PM +0200, Eugen Hristev wrote:
> Add bindings for producer/consumer for iio triggers.
> 
> Similar with iio channels, the iio triggers can be connected between drivers:
> one driver will be a producer by registering iio triggers, and another driver
> will connect as a consumer.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  .../devicetree/bindings/iio/iio-bindings.txt       | 52 +++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> index 68d6f8c..d861f0df 100644
> --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -11,6 +11,10 @@ value of a #io-channel-cells property in the IIO provider node.
>  
>  [1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>  
> +Moreover, the provider can have a set of triggers that can be attached to
> +from the consumer drivers.
> +
> +
>  ==IIO providers==
>  
>  Required properties:
> @@ -18,6 +22,11 @@ Required properties:
>  		   with a single IIO output and 1 for nodes with multiple
>  		   IIO outputs.
>  
> +Optional properties:
> +#io-trigger-cells: Number of cells for the IIO trigger specifier. Typically 0
> +		   for nodes with a single IIO trigger and 1 for nodes with
> +		   multiple IIO triggers.
> +
>  Example for a simple configuration with no trigger:
>  
>  	adc: voltage-sensor@35 {
> @@ -26,7 +35,7 @@ Example for a simple configuration with no trigger:
>  		#io-channel-cells = <1>;
>  	};
>  
> -Example for a configuration with trigger:
> +Example for a configuration with channels provided by trigger:
>  
>  	adc@35 {
>  		compatible = "some-vendor,some-adc";
> @@ -42,6 +51,17 @@ Example for a configuration with trigger:
>  		};
>  	};
>  
> +Example for a configuration for a trigger provider:
> +
> +	adc: sensor-with-trigger@35 {
> +		compatible = "some-vendor,some-adc";
> +		reg = <0x35>;
> +		#io-channel-cells = <1>;
> +		#io-trigger-cells = <1>;
> +		/* other properties */
> +	};
> +
> +
>  ==IIO consumers==
>  
>  Required properties:
> @@ -61,16 +81,38 @@ io-channel-ranges:
>  		IIO channels from this node. Useful for bus nodes to provide
>  		and IIO channel to their children.
>  
> +io-triggers:	List of phandle and IIO specifier pairs, one pair
> +		for each trigger input to the device. Note: if the
> +		IIO trigger provider specifies '0' for #io-trigger-cells,
> +		then only the phandle portion of the pair will appear.
> +
> +io-trigger-names:
> +		List of IIO trigger input name strings sorted in the same
> +		order as the io-triggers property. Consumers drivers
> +		will use io-trigger-names to match IIO trigger input names
> +		with IIO specifiers.
> +
> +io-trigger-ranges:
> +		Empty property indicating that child nodes can inherit named
> +		IIO triggers from this node. Useful for bus nodes to provide
> +		IIO triggers to their children.

I think it would be better to be explicit in the child nodes. What's the 
use you had in mind?

Rob

Re: [PATCH 13/14] input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver

[Rob Herring]

On Fri, Dec 22, 2017 at 05:07:20PM +0200, Eugen Hristev wrote:
> This is the implementation of the Microchip SAMA5D2 SOC resistive
> touchscreen driver.
> The driver registers an input device and connects to the give IIO device
> from devicetree. It requires an IIO trigger (acting as a consumer) and
> three IIO channels : one for X position, one for Y position and one
> for pressure.
> It the reports the values to the input subsystem.
> 
> Some parts of this driver are based on the initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy

This doesn't appear to have anything specific to SAMA5D2 SoC, but is 
rather just a generic ADC (IIO based) resistive touchscreen driver.

Perhaps the binding can also be just an "adc-resistive-touchscreen".

Rob

Re: [PATCH 02/14] iio: Add channel for Position

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:09 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Add new channel type for position on a pad.
> 
> These type of analog sensor represents the position of a pen
> on a touchpad, and is represented as a voltage, which can be
> converted to a position on X and Y axis on the pad.
> 
> The channel can then be consumed by a touchscreen driver or
> read as-is for a raw indication of the touchpen on a touchpad.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>  drivers/iio/industrialio-core.c         |  1 +
>  include/uapi/linux/iio/types.h          |  1 +
>  tools/iio/iio_event_monitor.c           |  2 ++
>  4 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a478740..d2b9e2f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -190,6 +190,17 @@ Description:
>  		but should match other such assignments on device).
>  		Units after application of scale and offset are m/s^2.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/in_position_x_raw
> +What:		/sys/bus/iio/devices/iio:deviceX/in_position_y_raw
> +KernelVersion:	4.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Position in direction x or y on a pad (may be arbitrarily
> +		assigned but should match other such assignments on device).
> +		Units after application of scale and offset are millipercents
> +		from the pad's size in both directions. Should be calibrated by
> +		the consumer.

Hmm. The units are an issues as to be consistent with the existing ABI position
should be in meters.  Perhaps the trick is to do similar to we have done for
relative humidity and call this in_positionrelative_x_raw etc.

That leaves position open for absolute position devices (who knows what)
in the future.

> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2e8e36f..a4fa49b 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_COUNT] = "count",
>  	[IIO_INDEX] = "index",
>  	[IIO_GRAVITY]  = "gravity",
> +	[IIO_POSITION]  = "position",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 4213cdf..35e17da 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -44,6 +44,7 @@ enum iio_chan_type {
>  	IIO_COUNT,
>  	IIO_INDEX,
>  	IIO_GRAVITY,
> +	IIO_POSITION,
>  };
>  
>  enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index b61245e..0c2b317 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_PH] = "ph",
>  	[IIO_UVINDEX] = "uvindex",
>  	[IIO_GRAVITY] = "gravity",
> +	[IIO_POSITION] = "position",
>  };
>  
>  static const char * const iio_ev_type_text[] = {
> @@ -151,6 +152,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_PH:
>  	case IIO_UVINDEX:
>  	case IIO_GRAVITY:
> +	case IIO_POSITION:
>  		break;
>  	default:
>  		return false;

Re: [PATCH 10/14] iio: adc: at91-sama5d2_adc: force trigger removal on module remove

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:17 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On module remove, if we do not call trigger remove, the trigger
> stays in the subsystem, and on further module insert, we will have
> multiple triggers, and the old one is not usable.
> Have to call the remove function on module remove to solve this.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

This needs more explanation.  I can't see why the managed removal
isn't sufficient.  On removal the dev should have gone away taking
the trigger with it.

If it isn't then it looks like a straight forward bug that needs fixing.

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4eff835..7b9febc 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1180,6 +1180,9 @@ static int at91_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  
> +	if (st->selected_trig->hw_trig)
> +		devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
> +
>  	iio_device_unregister(indio_dev);
>  
>  	at91_adc_dma_disable(pdev);

Re: [PATCH 11/14] iio: adc: at91-sama5d2_adc: optimize scan index for diff channels

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:18 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Optimize the scan index for the differential channels. Before, it
> was single channel count + index of the first single channel
> number of the differential pair. (e.g. 11+0, +2, +4, etc.)
> Divide that number by two (since it's always even), and add it up
> as a scan index to have consecutive numbered channels in the
> index.
Why?  This is odd as it stands, but that isn't a strong enough reason
to fix it.

This is making a userspace ABI change.  We need a very strong
argument for why it is necessary and also why existing userspace
won't care.

> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7b9febc..9610393 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -209,7 +209,7 @@
>  		.channel = num,						\
>  		.channel2 = num2,					\
>  		.address = addr,					\
> -		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
> +		.scan_index = (num >> 1) + AT91_SAMA5D2_SINGLE_CHAN_CNT,\
>  		.scan_type = {						\
>  			.sign = 's',					\
>  			.realbits = 12,					\

Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:19 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> The ADC IP supports position and pressure measurements for a touchpad
> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> measurement support.
> Using the inkern API, a driver can request a trigger and read the
> channel values from the ADC.
> The implementation provides a trigger named "touch" which can be
> connected to a consumer driver.
> Once a driver connects and attaches a pollfunc to this trigger, the
> configure trigger callback is called, and then the ADC driver will
> initialize pad measurement.
> First step is to enable touchscreen 4wire support and enable
> pen detect IRQ.
> Once a pen is detected, a periodic trigger is setup to trigger every
> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> is called, and the consumer driver is then woke up, and it can read the
> respective channels for the values : X, and Y for position and pressure
> channel.
> Because only one trigger can be active in hardware in the same time,
> while touching the pad, the ADC will block any attempt to use the
> triggered buffer. Same, conversions using the software trigger are also
> impossible (since the periodic trigger is setup).
> If some driver wants to attach while the trigger is in use, it will
> also fail.
> Once the pen is not detected anymore, the trigger is free for use (hardware
> or software trigger, with or without DMA).
> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> 
> Some parts of this patch are based on initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> 
OK, so comments inline.

What I'm missing currently though is an explanation of why the slightly
more standard arrangement of using a callback buffer doesn't work here.
The only addition I think you need to do that is to allow a consumer to
request a particular trigger.  I also think some of the other provisions
could be handled using standard features and slightly reducing the flexibility.
I don't know for example if it's useful to allow other channels to be
read when touch is not in progress or not.

So restrictions:

1. Touch screen channels can only be read when touch is enabled.
 - use the available_scan_masks to control this. Or the callback that lets
   you do the same dynamically.
2. You need to push these channels to your consumer driver.
 - register a callback buffer rather than jumping through the hoops to
   insert your own pollfunc.  That will call a function in your
   consumer, providing the data from the 3 channels directly.
3. You need to make sure it is using the right driver.  For that you
   will I think need a new interface.

Various other comments inline. I may well be missing something as this is
a fair bit of complex code to read - if so then next version should have
a clear cover letter describing why this more standard approach can't be
used.

> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 455 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 446 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9610393..79eb197 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -102,14 +102,26 @@
>  #define AT91_SAMA5D2_LCDR	0x20
>  /* Interrupt Enable Register */
>  #define AT91_SAMA5D2_IER	0x24
> +/* Interrupt Enable Register - TS X measurement ready */
> +#define AT91_SAMA5D2_IER_XRDY   BIT(20)
> +/* Interrupt Enable Register - TS Y measurement ready */
> +#define AT91_SAMA5D2_IER_YRDY   BIT(21)
> +/* Interrupt Enable Register - TS pressure measurement ready */
> +#define AT91_SAMA5D2_IER_PRDY   BIT(22)
>  /* Interrupt Enable Register - general overrun error */
>  #define AT91_SAMA5D2_IER_GOVRE BIT(25)
> +/* Interrupt Enable Register - Pen detect */
> +#define AT91_SAMA5D2_IER_PEN    BIT(29)
> +/* Interrupt Enable Register - No pen detect */
> +#define AT91_SAMA5D2_IER_NOPEN  BIT(30)
>  /* Interrupt Disable Register */
>  #define AT91_SAMA5D2_IDR	0x28
>  /* Interrupt Mask Register */
>  #define AT91_SAMA5D2_IMR	0x2c
>  /* Interrupt Status Register */
>  #define AT91_SAMA5D2_ISR	0x30
> +/* Interrupt Status Register - Pen touching sense status */
> +#define AT91_SAMA5D2_ISR_PENS   BIT(31)
>  /* Last Channel Trigger Mode Register */
>  #define AT91_SAMA5D2_LCTMR	0x34
>  /* Last Channel Compare Window Register */
> @@ -131,8 +143,37 @@
>  #define AT91_SAMA5D2_CDR0	0x50
>  /* Analog Control Register */
>  #define AT91_SAMA5D2_ACR	0x94
> +/* Analog Control Register - Pen detect sensitivity mask */
> +#define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(0, 1)
>  /* Touchscreen Mode Register */
>  #define AT91_SAMA5D2_TSMR	0xb0
> +/* Touchscreen Mode Register - No touch mode */
> +#define AT91_SAMA5D2_TSMR_TSMODE_NONE           0
> +/* Touchscreen Mode Register - 4 wire screen, no pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_NO_PRESS 1
> +/* Touchscreen Mode Register - 4 wire screen, pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS    2
> +/* Touchscreen Mode Register - 5 wire screen */
> +#define AT91_SAMA5D2_TSMR_TSMODE_5WIRE          3
> +/* Touchscreen Mode Register - Average samples mask */
> +#define AT91_SAMA5D2_TSMR_TSAV_MASK		(3 << 4)
> +/* Touchscreen Mode Register - Average samples */
> +#define AT91_SAMA5D2_TSMR_TSAV(x)		((x) << 4)
> +/* Touchscreen Mode Register - Touch/trigger frequency ratio mask */
> +#define AT91_SAMA5D2_TSMR_TSFREQ_MASK		(0xf << 8)
> +/* Touchscreen Mode Register - Touch/trigger freqency ratio */
> +#define AT91_SAMA5D2_TSMR_TSFREQ(x)		((x) << 8)
> +/* Touchscreen Mode Register - Pen Debounce Time mask */
> +#define AT91_SAMA5D2_TSMR_PENDBC_MASK		(0xf << 28)
> +/* Touchscreen Mode Register - Pen Debounce Time */
> +#define AT91_SAMA5D2_TSMR_PENDBC(x)            ((x) << 28)
> +/* Touchscreen Mode Register - No DMA for touch measurements */
> +#define AT91_SAMA5D2_TSMR_NOTSDMA               BIT(22)
> +/* Touchscreen Mode Register - Disable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_DIS            (0 << 24)
> +/* Touchscreen Mode Register - Enable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_ENA            BIT(24)
> +
>  /* Touchscreen X Position Register */
>  #define AT91_SAMA5D2_XPOSR	0xb4
>  /* Touchscreen Y Position Register */
> @@ -151,7 +192,12 @@
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>  /* Trigger Mode external trigger any edge */
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> -
> +/* Trigger Mode internal periodic */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
> +/* Trigger Mode - trigger period mask */
> +#define AT91_SAMA5D2_TRGR_TRGPER_MASK		(0xffff << 16)
> +/* Trigger Mode - trigger period */
> +#define AT91_SAMA5D2_TRGR_TRGPER(x)		((x) << 16)
>  /* Correction Select Register */
>  #define AT91_SAMA5D2_COSR	0xd0
>  /* Correction Value Register */
> @@ -169,6 +215,21 @@
>  #define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>  #define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>  
> +#define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX	(AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> +					AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
> +
> +#define AT91_SAMA5D2_TOUCH_X_CHAN_IDX	(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_Y_CHAN_IDX	(AT91_SAMA5D2_TOUCH_X_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_P_CHAN_IDX	(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX + 1)
> +
> +#define TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */

These all need the AT91_SAMA5D2 prefix.

> +#define TOUCH_PEN_DETECT_DEBOUNCE_US    200
> +
> +#define XYZ_MASK			GENMASK(11, 0)
> +
> +#define MAX_POS_BITS			12
> +
> +#define AT91_ADC_TOUCH_TRIG_SHORTNAME	"touch"
>  /*
>   * Maximum number of bytes to hold conversion from all channels
>   * without the timestamp.
> @@ -222,6 +283,37 @@
>  		.indexed = 1,						\
>  	}
>  
> +#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)				\
> +	{								\
> +		.type = IIO_POSITION,					\
> +		.modified = 1,						\
> +		.channel = num,						\
> +		.channel2 = mod,					\
> +		.scan_index = num,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +		.datasheet_name = name,					\
> +	}
> +#define AT91_SAMA5D2_CHAN_PRESSURE(num, name)				\
> +	{								\
> +		.type = IIO_PRESSURE,					\
> +		.channel = num,						\
> +		.scan_index = num,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +		.datasheet_name = name,					\
> +	}
> +
>  #define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
>  #define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
>  
> @@ -239,6 +331,20 @@ struct at91_adc_trigger {
>  };
>  
>  /**
> + * at91_adc_touch - at91-sama5d2 touchscreen information struct
> + * @trig:			hold the start timestamp of dma operation
> + * @sample_period_val:		the value for periodic trigger interval
> + * @touching:			is the pen touching the screen or not
> + * @x_pos:			temporary placeholder for pressure computation
> + */
> +struct at91_adc_touch {
> +	struct iio_trigger		*trig;
> +	u16				sample_period_val;
> +	bool				touching;
> +	u32				x_pos;
> +};
> +
> +/**
>   * at91_adc_dma - at91-sama5d2 dma information struct
>   * @dma_chan:		the dma channel acquired
>   * @rx_buf:		dma coherent allocated area
> @@ -267,18 +373,22 @@ struct at91_adc_state {
>  	struct regulator		*reg;
>  	struct regulator		*vref;
>  	int				vref_uv;
> +	unsigned int			current_sample_rate;
>  	struct iio_trigger		*trig;
>  	const struct at91_adc_trigger	*selected_trig;
>  	const struct iio_chan_spec	*chan;
>  	bool				conversion_done;
>  	u32				conversion_value;
> +	bool				touch_requested;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
>  	struct at91_adc_dma		dma_st;
> +	struct at91_adc_touch		touch_st;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> -	 * sysfs.
> +	 * sysfs. Also protects when enabling or disabling touchscreen
> +	 * producer mode and checking if this mode is enabled or not.
>  	 */
>  	struct mutex			lock;
>  };
> @@ -310,6 +420,7 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>  	},
>  };
>  
> +/* channel order is not subject to change. inkern consumers rely on this */
>  static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> @@ -329,10 +440,103 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> -	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> -				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
> +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX),
> +	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_X_CHAN_IDX, "x", IIO_MOD_X),
> +	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, "y", IIO_MOD_Y),
> +	AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
>  };
>  
> +static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
> +{
> +	u32 clk_khz = st->current_sample_rate / 1000;
> +	int i = 0;
> +	u16 pendbc;
> +	u32 tsmr, acr;
> +
> +	if (!state) {
> +		/* disabling touch IRQs and setting mode to no touch enabled */
> +		at91_adc_writel(st, AT91_SAMA5D2_IDR,
> +				AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
> +		at91_adc_writel(st, AT91_SAMA5D2_TSMR, 0);
> +		return 0;
> +	}
> +	/*
> +	 * debounce time is in microseconds, we need it in milliseconds to
> +	 * multiply with kilohertz, so, divide by 1000, but after the multiply.
> +	 * round up to make sure pendbc is at least 1
> +	 */
> +	pendbc = round_up(TOUCH_PEN_DETECT_DEBOUNCE_US * clk_khz / 1000, 1);
> +
> +	/* get the required exponent */
> +	while (pendbc >> i++)
> +		;
This is related to the first 0?  There are cleaner ways of doing this
with ffs and friends.
> +
> +	pendbc = i;
> +
> +	tsmr = AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS;
> +
> +	tsmr |= AT91_SAMA5D2_TSMR_TSAV(1) & AT91_SAMA5D2_TSMR_TSAV_MASK;
> +	tsmr |= AT91_SAMA5D2_TSMR_PENDBC(pendbc) &
> +		AT91_SAMA5D2_TSMR_PENDBC_MASK;
> +	tsmr |= AT91_SAMA5D2_TSMR_NOTSDMA;
> +	tsmr |= AT91_SAMA5D2_TSMR_PENDET_ENA;
> +	tsmr |= AT91_SAMA5D2_TSMR_TSFREQ(1) & AT91_SAMA5D2_TSMR_TSFREQ_MASK;
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_TSMR, tsmr);
> +
> +	acr =  at91_adc_readl(st, AT91_SAMA5D2_ACR);
> +	acr &= ~AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> +	acr |= 0x02 & AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> +	at91_adc_writel(st, AT91_SAMA5D2_ACR, acr);
> +
> +	/* Sample Period Time = (TRGPER + 1) / ADCClock */
> +	st->touch_st.sample_period_val = round_up((TOUCH_SAMPLE_PERIOD_US *
> +					 clk_khz / 1000) - 1, 1);
> +	/* enable pen detect IRQ */
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_touch_trigger_validate_device(struct iio_trigger *trig,
> +						  struct iio_dev *indio_dev)
> +{
> +	/* the touch trigger cannot be used with a buffer */
> +	return -EBUSY;
> +}
> +
> +static int at91_adc_configure_touch_trigger(struct iio_trigger *trig,
> +					    bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	/*
> +	 * If we configure this with the IRQ enabled, the pen detected IRQ
> +	 * might fire before we finish setting all up, and the IRQ handler
> +	 * might misbehave. Better to reenable the IRQ after we are done
> +	 */
> +	disable_irq_nosync(st->irq);
> +
> +	mutex_lock(&st->lock);
> +	if (state) {
> +		ret = iio_buffer_enabled(indio_dev);
> +		if (ret) {
> +			dev_dbg(&indio_dev->dev, "trigger is currently in use\n");
> +			ret = -EBUSY;
> +			goto configure_touch_unlock_exit;
> +		}
> +	}
> +	at91_adc_configure_touch(st, state);
> +	st->touch_requested = state;
> +
> +configure_touch_unlock_exit:
> +	enable_irq(st->irq);
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
>  static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> @@ -390,12 +594,27 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>  	return 0;
>  }
>  
> +static int at91_adc_reenable_touch_trigger(struct iio_trigger *trig)
> +{
> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio);
> +
> +	enable_irq(st->irq);
> +
> +	return 0;
> +}
>  static const struct iio_trigger_ops at91_adc_trigger_ops = {
>  	.set_trigger_state = &at91_adc_configure_trigger,
>  	.try_reenable = &at91_adc_reenable_trigger,
>  	.validate_device = iio_trigger_validate_own_device,
>  };
>  
> +static const struct iio_trigger_ops at91_adc_touch_trigger_ops = {
> +	.set_trigger_state = &at91_adc_configure_touch_trigger,
> +	.try_reenable = &at91_adc_reenable_touch_trigger,
> +	.validate_device = &at91_adc_touch_trigger_validate_device,
> +};
> +
>  static int at91_adc_dma_size_done(struct at91_adc_state *st)
>  {
>  	struct dma_tx_state state;
> @@ -490,6 +709,23 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* have to make sure nobody is requesting the trigger right now */

This needs some more explanation as I don't totally follow what this
is designed to protect against.

Realistically a device is only useful if it has one trigger at a time
feeding a valid set of channels to however many consumers (whether
in the driver or not).

> +	mutex_lock(&st->lock);
> +	ret = st->touch_requested;
> +	mutex_unlock(&st->lock);
> +
> +	/*
> +	 * if the trigger is used by the touchscreen,
> +	 * we must return an error
> +	 */
> +	return ret ? -EBUSY : 0;
> +}
> +
>  static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	int ret;
> @@ -538,6 +774,7 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>  }
>  
>  static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.preenable = &at91_adc_buffer_preenable,
>  	.postenable = &at91_adc_buffer_postenable,
>  	.predisable = &at91_adc_buffer_predisable,
>  };
> @@ -555,7 +792,11 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>  
>  	trig->dev.parent = indio->dev.parent;
>  	iio_trigger_set_drvdata(trig, indio);
> -	trig->ops = &at91_adc_trigger_ops;
> +
> +	if (strcmp(trigger_name, AT91_ADC_TOUCH_TRIG_SHORTNAME))

Pass this is as a parameter to the function and avoid the strcmp nastiness.

> +		trig->ops = &at91_adc_trigger_ops;
> +	else
> +		trig->ops = &at91_adc_touch_trigger_ops;
>  
>  	ret = devm_iio_trigger_register(&indio->dev, trig);
>  	if (ret)
> @@ -571,7 +812,16 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
>  	st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
>  	if (IS_ERR(st->trig)) {
>  		dev_err(&indio->dev,
> -			"could not allocate trigger\n");
> +			"could not allocate trigger %s\n",
> +			 st->selected_trig->name);
> +		return PTR_ERR(st->trig);
> +	}
> +
> +	st->touch_st.trig = at91_adc_allocate_trigger(indio,
> +						AT91_ADC_TOUCH_TRIG_SHORTNAME);
> +	if (IS_ERR(st->trig)) {
> +		dev_err(&indio->dev, "could not allocate trigger"
> +			AT91_ADC_TOUCH_TRIG_SHORTNAME "\n");
>  		return PTR_ERR(st->trig);
>  	}
>  
> @@ -703,6 +953,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>  
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>  		freq, startup, prescal);
> +
> +	st->current_sample_rate = freq;
>  }
>  
>  static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
> @@ -718,23 +970,77 @@ static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
>  	return f_adc;
>  }
>  
> +static irqreturn_t at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_PEN);
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_NOPEN |
> +			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY);
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> +			AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC |
> +			AT91_SAMA5D2_TRGR_TRGPER(st->touch_st.sample_period_val));
> +	st->touch_st.touching = true;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, 0);
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
> +			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY);
> +	st->touch_st.touching = false;
Hmm. I think we are unfortunately racing here.  There is nothing preventing
this running concurrently with the read_raw calls that check the same variable.

If this is fine (because we will always get valid data anyway (if stale)
then a comment is needed to explain that.

> +
> +	disable_irq_nosync(st->irq);
> +	iio_trigger_poll(st->touch_st.trig);

Comment to explain why a poll here is fine, but not on the pen on would be
good (I can guess but better to state it!)

> +
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  {
>  	struct iio_dev *indio = private;
>  	struct at91_adc_state *st = iio_priv(indio);
>  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> +	u32 rdy_mask = AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY;
>  
>  	if (!(status & imr))
>  		return IRQ_NONE;
>  
> -	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> +	if (st->touch_requested && (status & AT91_SAMA5D2_IER_PEN)) {
> +		/* pen detected IRQ */
> +		return at91_adc_pen_detect_interrupt(st);
> +	} else if (st->touch_requested && (status & AT91_SAMA5D2_IER_NOPEN)) {
> +		/* nopen detected IRQ */
> +		return at91_adc_no_pen_detect_interrupt(st);
> +	} else if (st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS) &&
> +		   ((status & rdy_mask) == rdy_mask)) {
> +		/* periodic trigger IRQ - during pen sense */
> +		disable_irq_nosync(irq);
> +		iio_trigger_poll(st->touch_st.trig);
> +	} else if ((st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS))) {
> +		/*
> +		 * touching, but the measurements are not ready yet.
> +		 * read and ignore.
> +		 */
> +		status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> +		status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> +		status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> +	} else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> +		/* buffered trigger without DMA */
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
>  	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		/* buffered trigger with DMA - should not happen */
>  		disable_irq_nosync(irq);
>  		WARN(true, "Unexpected irq occurred\n");
>  	} else if (!iio_buffer_enabled(indio)) {
> +		/* software requested conversion */
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> @@ -742,6 +1048,96 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> +static u32 at91_adc_touch_x_pos(struct at91_adc_state *st)
> +{
> +	u32 xscale, val;
> +	u32 x, xpos;
> +
> +	/* x position = (x / xscale) * max, max = 2^MAX_POS_BITS - 1 */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> +	if (!val)
> +		dev_dbg(&iio_priv_to_dev(st)->dev, "x_pos is 0\n");
> +
> +	xpos = val & XYZ_MASK;
> +	x = (xpos << MAX_POS_BITS) - xpos;
> +	xscale = (val >> 16) & XYZ_MASK;
> +	if (xscale == 0) {
> +		dev_err(&iio_priv_to_dev(st)->dev, "xscale is 0\n");
> +		return 0;
> +	}
> +	x /= xscale;
> +	st->touch_st.x_pos = x;
> +
> +	return x;
> +}
> +
> +static u32 at91_adc_touch_y_pos(struct at91_adc_state *st)
> +{
> +	u32 yscale, val;
> +	u32 y, ypos;
> +
> +	/* y position = (y / yscale) * max, max = 2^MAX_POS_BITS - 1 */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> +	ypos = val & XYZ_MASK;
> +	y = (ypos << MAX_POS_BITS) - ypos;
> +	yscale = (val >> 16) & XYZ_MASK;
> +
> +	if (yscale == 0)
> +		return 0;
> +
> +	y /= yscale;
> +
> +	return y;
> +}
> +
> +static u32 at91_adc_touch_pressure(struct at91_adc_state *st)
> +{
> +	u32 val, z1, z2;
> +	u32 pres;
> +	u32 rxp = 1;
> +	u32 factor = 1000;
> +
> +	/* calculate the pressure */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> +	z1 = val & XYZ_MASK;

XYZ_MASK seems oddly named given what this seems to be doing...

> +	z2 = (val >> 16) & XYZ_MASK;
> +
> +	if (z1 != 0)
> +		pres = rxp * (st->touch_st.x_pos * factor / 1024) *
> +			(z2 * factor / z1 - factor) /
> +			factor;
> +	else
> +		pres = 0xFFFFFFFF;       /* no pen contact */
> +
> +	return pres;
> +}
> +
> +static int at91_adc_read_position(struct at91_adc_state *st, int chan, int *val)
> +{
> +	if (!st->touch_st.touching)
> +		return -ENODATA;
> +	if (chan == AT91_SAMA5D2_TOUCH_X_CHAN_IDX)
> +		*val = at91_adc_touch_x_pos(st);
> +	else if (chan == AT91_SAMA5D2_TOUCH_Y_CHAN_IDX)
> +		*val = at91_adc_touch_y_pos(st);
> +	else
> +		return -ENODATA;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int at91_adc_read_pressure(struct at91_adc_state *st, int chan, int *val)
> +{
> +	if (!st->touch_st.touching)
> +		return -ENODATA;
> +	if (chan == AT91_SAMA5D2_TOUCH_P_CHAN_IDX)

General code flow simpler if you check error first
if (chan != AT91_SAMA5D2_TOUCH_P_CHAN_IDX)
	return -ENODATA;

*val =...

> +		*val = at91_adc_touch_pressure(st);
> +	else
> +		return -ENODATA;
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
> @@ -752,11 +1148,38 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +
> +		if (chan->type == IIO_POSITION) {

Switch or else if as only one is true at a time.

Hmm. So you allow sysfs reads of these channels if touch in progress.
Do we actually have a use for this?  Seems we could have simpler code
by just not providing direct reads for them if not.

> +			ret = at91_adc_read_position(st, chan->channel, val);
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +		if (chan->type == IIO_PRESSURE) {
> +			ret = at91_adc_read_pressure(st, chan->channel, val);
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +		/* if we using touch, channels 0, 1, 2, 3 are unavailable */
> +		if (st->touch_requested && chan->channel <= 3) {
> +			mutex_unlock(&st->lock);
> +			return -EBUSY;
> +		}
> +		/*
> +		 * if we have the periodic trigger set up, we can't use
> +		 * software trigger either.
> +		 */
> +		if (st->touch_st.touching) {

> +			mutex_unlock(&st->lock);
> +			return -ENODATA;
> +		}
> +
>  		/* we cannot use software trigger if hw trigger enabled */
>  		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&st->lock);
>  			return ret;
> -		mutex_lock(&st->lock);
> +		}
>  
>  		st->chan = chan;
>  
> @@ -785,6 +1208,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  
>  		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
>  		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> +		/*
> +		 * It is possible that after this conversion, we reuse these
> +		 * channels for the touchscreen. So, reset the COR now.
> +		 */
> +		at91_adc_writel(st, AT91_SAMA5D2_COR, 0);
>  
>  		/* Needed to ACK the DRDY interruption */
>  		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> @@ -1180,6 +1608,10 @@ static int at91_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  
> +	mutex_lock(&st->lock);
> +	devm_iio_trigger_unregister(&indio_dev->dev, st->touch_st.trig);

As before this needs detailed explanation. It should not be necessary.

> +	mutex_unlock(&st->lock);
> +
>  	if (st->selected_trig->hw_trig)
>  		devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
>  
> @@ -1245,6 +1677,11 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	if (iio_buffer_enabled(indio_dev))
>  		at91_adc_configure_trigger(st->trig, true);
>  
> +	mutex_lock(&st->lock);
> +	if (st->touch_requested)
> +		at91_adc_configure_touch_trigger(st->touch_st.trig, true);
> +	mutex_unlock(&st->lock);
> +
>  	return 0;
>  
>  vref_disable_resume:

Re: [PATCH 13/14] input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:20 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> This is the implementation of the Microchip SAMA5D2 SOC resistive
> touchscreen driver.
> The driver registers an input device and connects to the give IIO device
> from devicetree. It requires an IIO trigger (acting as a consumer) and
> three IIO channels : one for X position, one for Y position and one
> for pressure.
> It the reports the values to the input subsystem.
> 
> Some parts of this driver are based on the initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

I've suggested some difference in implementation, but this is very nearly
a generic resistive touch screen driver which is rather nice.

It does use somewhat magic trigger which is effectively a filtered
periodic trigger (only fires when touch has occurred) which would not
be particularly hard to implement in other resistive touch screen ADCs.
I'm not totally sure that is a strong requirement though - any periodic
trigger would work.

Anyhow, the big stuff to my mind is whether we can use the buffer_cb
code to do this.  I originally wrote that for an accelerometer to input
bridge driver (which I never got around to finishing upstreaming). It's
existing usecases are rather esoteric but it should work here I think.

Jonathan

> ---
>  drivers/input/touchscreen/Kconfig       |  13 ++
>  drivers/input/touchscreen/Makefile      |   1 +
>  drivers/input/touchscreen/sama5d2_rts.c | 287 ++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/input/touchscreen/sama5d2_rts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 64b30fe..db8f541 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -126,6 +126,19 @@ config TOUCHSCREEN_ATMEL_MXT_T37
>  	  Say Y here if you want support to output data from the T37
>  	  Diagnostic Data object using a V4L device.
>  
> +config TOUCHSCREEN_SAMA5D2
> +	tristate "Microchip SAMA5D2 resistive touchscreen support"
> +	depends on ARCH_AT91
> +	depends on AT91_SAMA5D2_ADC
> +	help
> +	  Say Y here if you have 4-wire touchscreen connected
> +          to ADC Controller on your SAMA5D2 Microchip SoC.
> +
> +          If unsure, say N.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called sama5d2_rts.
> +
>  config TOUCHSCREEN_AUO_PIXCIR
>  	tristate "AUO in-cell touchscreen using Pixcir ICs"
>  	depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 850c156..9a2772e 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SAMA5D2)	+= sama5d2_rts.o
>  obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR)	+= auo-pixcir-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_BU21013)	+= bu21013_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318)	+= chipone_icn8318.o
> diff --git a/drivers/input/touchscreen/sama5d2_rts.c b/drivers/input/touchscreen/sama5d2_rts.c
> new file mode 100644
> index 0000000..e2ae413
> --- /dev/null
> +++ b/drivers/input/touchscreen/sama5d2_rts.c
> @@ -0,0 +1,287 @@
> +/*
> + * Microchip resistive touchscreen (RTS) driver for SAMA5D2.
> + *
> + * Copyright (C) 2017 Microchip Technology,
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +#include <linux/input.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define DRIVER_NAME					"sama5d2_rts"
> +#define MAX_POS_MASK					GENMASK(11, 0)
> +#define AT91_RTS_DEFAULT_PRESSURE_THRESHOLD		10000
> +
> +/**
> + * at91_rts - at91 resistive touchscreen information struct
> + * @input:		the input device structure that we register
> + * @chan_x:		X channel to IIO device to get position on X axis
> + * @chan_y:		Y channel to IIO device to get position on Y axis
> + * @chan_pressure:	pressure channel to IIO device to get pressure
> + * @trig:		trigger to IIO device to register to for polling
> + * @rts_pf:		pollfunc for the trigger to be called by IIO dev
> + * @pressure_threshold:	number representing the threshold for the pressure
> + * @adc_connected:	to know if adc device is connected
> + * @workq:		to defer computations to this work queue for reporting
> + */
> +struct at91_rts {
> +	struct input_dev	*input;
> +	struct iio_channel	*chan_x, *chan_y, *chan_pressure;
> +	struct iio_trigger	*trig;
> +	struct iio_poll_func	*rts_pf;
> +	u32                     pressure_threshold;
> +	bool			adc_connected;
> +	struct work_struct	workq;
> +};
> +
> +static irqreturn_t at91_rts_trigger_handler(int irq, void *p)
> +{
> +	struct at91_rts *st = ((struct iio_poll_func *)p)->p;
> +
> +	schedule_work(&st->workq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void at91_rts_workq_handler(struct work_struct *workq)
> +{
> +	struct at91_rts *st = container_of(workq, struct at91_rts, workq);
> +	unsigned int x, y, press;
> +	int ret;
> +

If we were instead to use a callback buffer this would all be
in the ADC driver as a simple 3 element channel scan.

> +	/* read the channels, if all good, report touch */
> +	ret = iio_read_channel_raw(st->chan_x, &x);
> +	if (ret < 0)
> +		goto at91_rts_workq_handler_end_touch;
> +
> +	ret = iio_read_channel_raw(st->chan_y, &y);
> +	if (ret < 0)
> +		goto at91_rts_workq_handler_end_touch;
> +
> +	ret = iio_read_channel_raw(st->chan_pressure, &press);
> +	if (ret < 0)
> +		goto at91_rts_workq_handler_end_touch;
> +
At this point our callback would be called and provided the above data.

> +	/* if pressure too low, don't report */
> +	if (press > st->pressure_threshold)
> +		goto at91_rts_workq_handler_exit;
> +
> +	input_report_abs(st->input, ABS_X, x);
> +	input_report_abs(st->input, ABS_Y, y);
> +	input_report_abs(st->input, ABS_PRESSURE, press);
> +	input_report_key(st->input, BTN_TOUCH, 1);
> +	input_sync(st->input);
> +

This would be back in the callback buffer code once the
callback has run.

Has the interesting side effect of making this code effectively generic.
It no longer has any ties to the at91 at all that I can spot.
Just a selection of channels and a request for a particular trigger -
both of which could come from device tree.

> +	iio_trigger_notify_done(st->trig);
> +	return;
> +
> +at91_rts_workq_handler_end_touch:
> +	/* report end of touch */
> +	input_report_key(st->input, BTN_TOUCH, 0);
> +	input_sync(st->input);
> +at91_rts_workq_handler_exit:
> +	iio_trigger_notify_done(st->trig);
> +}
> +
> +static int at91_rts_open(struct input_dev *dev)
> +{
> +	int ret;
> +	struct at91_rts *st = input_get_drvdata(dev);
> +
> +	/* avoid multiple initialization in case touchscreen is opened again */
> +	if (st->adc_connected)
> +		return 0;
> +
> +	/*
> +	 * First, look for the channels. It is possible that the ADC device
> +	 * did not probe yet, but we already probed, so we returning probe defer
> +	 * doesn't make much sense.

Quite - so why not do the obvious and request these in the probe and hold
them until remove?  Then if they aren't ready defer the probe. This driver
is useless until the ADC is there so why let it successfully probe before
that point?

> +	 */
> +	st->chan_x = iio_channel_get(dev->dev.parent, "x");
> +	if (IS_ERR_OR_NULL(st->chan_x)) {
> +		dev_err(dev->dev.parent, "cannot get X channel from ADC");
> +		ret = PTR_ERR(st->chan_x);
> +		goto at91_rts_open_free_chan;
> +	}
> +
> +	st->chan_y = iio_channel_get(dev->dev.parent, "y");
> +	if (IS_ERR_OR_NULL(st->chan_y)) {
> +		dev_err(dev->dev.parent, "cannot get Y channel from ADC");
> +		ret = PTR_ERR(st->chan_y);
> +		goto at91_rts_open_free_chan;
> +	}
> +
> +	st->chan_pressure = iio_channel_get(dev->dev.parent, "pressure");
> +	if (IS_ERR_OR_NULL(st->chan_pressure)) {
> +		dev_err(dev->dev.parent, "cannot get pressure channel from ADC");
> +		ret = PTR_ERR(st->chan_pressure);
> +		goto at91_rts_open_free_chan;
> +	}
> +
> +	/* look for the trigger in device tree */
> +	st->trig = iio_trigger_find(dev->dev.parent, NULL);
> +	if (IS_ERR_OR_NULL(st->trig)) {
> +		dev_err(dev->dev.parent, "cannot get trigger from ADC");
> +		ret = PTR_ERR(st->trig)
This also feels like it should be retrieved during the probe and we should
defer if that fails.  Can't do anything useful without it!
> +		goto at91_rts_open_free_chan;
> +	}
> +
> +	/* allocate a pollfunc for the trigger */
> +	st->rts_pf = iio_alloc_pollfunc(at91_rts_trigger_handler, NULL,
> +					IRQF_ONESHOT, NULL,
> +					dev->dev.parent->of_node->name);
> +	if (!st->rts_pf) {
> +		ret = -ENOMEM;
> +		dev_err(dev->dev.parent, "cannot allocate trigger pollfunc");
> +		goto at91_rts_open_free_chan;
> +	}
> +
> +	iio_pollfunc_set_private_data(st->rts_pf, st);
> +
> +	/*
> +	 * Attach the pollfunc to the trigger. This will also call the
> +	 * configure function to enable the trigger
> +	 */
> +	ret = iio_trigger_attach_poll_func(st->trig, st->rts_pf);
> +	if (ret)
> +		goto at91_rts_open_dealloc_pf;
Ah. Now I think I see why you needed the poll function rather than 
doing this with a callback buffer.

I'd rather see a consumer interface that requests the whole ADC runs on
a particular trigger.

> +
> +	dev_dbg(dev->dev.parent, "channels found, attached to trigger");
> +
> +	st->adc_connected = true;
> +	return 0;
> +
> +at91_rts_open_dealloc_pf:
> +	iio_dealloc_pollfunc(st->rts_pf);
> +at91_rts_open_free_chan:
> +	if (!IS_ERR_OR_NULL(st->chan_x))
> +		iio_channel_release(st->chan_x);
> +	if (!IS_ERR_OR_NULL(st->chan_y))
> +		iio_channel_release(st->chan_y);
> +	if (!IS_ERR_OR_NULL(st->chan_pressure))
> +		iio_channel_release(st->chan_pressure);
> +	/*
> +	 * Avoid keeping old values in channel pointers. in case some channel
> +	 * failed and we reopen them, and now fail, we will have invalid values
> +	 * to release. So write them as NULL now.
> +	 */
> +	st->chan_x = NULL;
> +	st->chan_y = NULL;
> +	st->chan_pressure = NULL;
> +	return ret;
> +}
> +
> +static void at91_rts_close(struct input_dev *dev)
> +{
> +	struct at91_rts *st = input_get_drvdata(dev);
> +
> +	if (!st->adc_connected)
> +		return;
> +
> +	iio_trigger_detach_poll_func(st->trig, st->rts_pf);
> +	iio_dealloc_pollfunc(st->rts_pf);
> +
> +	if (!IS_ERR_OR_NULL(st->chan_x))
> +		iio_channel_release(st->chan_x);
> +	if (!IS_ERR_OR_NULL(st->chan_y))
> +		iio_channel_release(st->chan_y);
> +	if (!IS_ERR_OR_NULL(st->chan_pressure))
> +		iio_channel_release(st->chan_pressure);
> +
> +	st->adc_connected = false;
> +}
> +
> +static int at91_rts_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct at91_rts *st;
> +	struct input_dev *input;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +
> +	st = devm_kzalloc(dev, sizeof(struct at91_rts), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +	st->adc_connected = false;
> +
> +	INIT_WORK(&st->workq, at91_rts_workq_handler);
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = of_property_read_u32(node, "microchip,pressure-threshold",
> +				   &st->pressure_threshold);
> +	if (ret < 0) {
> +		dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
> +		st->pressure_threshold = AT91_RTS_DEFAULT_PRESSURE_THRESHOLD;
> +	}
> +
> +	input->name = DRIVER_NAME;
> +	input->id.bustype = BUS_HOST;
> +	input->dev.parent = &pdev->dev;
> +	input->open = at91_rts_open;
> +	input->close = at91_rts_close;
> +
> +	input_set_abs_params(input, ABS_X, 0, MAX_POS_MASK - 1, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, MAX_POS_MASK, 0, 0);
> +	input_set_abs_params(input, ABS_PRESSURE, 0, 0xffffff, 0, 0);
> +
> +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	st->input = input;
> +	input_set_drvdata(input, st);
> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "failed to register input device: %d", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, st);
> +
> +	dev_info(dev, "probed successfully\n");

Not useful. There are many ways to find this out without looking at dmesg.
Please remove.

> +	return 0;
> +}
> +
> +static int at91_rts_remove(struct platform_device *pdev)
> +{
> +	struct at91_rts *st = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(st->input);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id at91_rts_of_match[] = {
> +	{
> +		.compatible = "microchip,sama5d2-resistive-touch",
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, at91_rts_of_match);
> +
> +static struct platform_driver atmel_rts_driver = {
> +	.probe = at91_rts_probe,
> +	.remove = at91_rts_remove,
> +	.driver = {
> +		   .name = DRIVER_NAME,
> +		   .of_match_table = of_match_ptr(at91_rts_of_match),
> +	},
> +};
> +
> +module_platform_driver(atmel_rts_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> +MODULE_DESCRIPTION("Microchip SAMA5D2 Resistive Touch Driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 09/14] iio: inkern: triggers: create helpers for OF trigger retrieval

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:16 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Create helper API to get trigger information from OF regarding
> trigger producer/consumer for iio triggers.
> The functions will search for matching trigger by name, similar
> with channel retrieval
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

This makes sense once we make the touchscreen driver truely generic.
If it wasn't generic this could be rolled up as data within that driver.

A few small comments inline.

> ---
>  drivers/iio/inkern.c                 | 91 ++++++++++++++++++++++++++++++++++++
>  include/linux/iio/trigger_consumer.h |  1 +
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 069defc..58bd18d 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -14,9 +14,13 @@
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> +#include "iio_core_trigger.h"
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
>  #include <linux/iio/consumer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +
Don't add white space here.
>  
>  struct iio_map_internal {
>  	struct iio_dev *indio_dev;
> @@ -262,6 +266,39 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  	return ERR_PTR(ret);
>  }
>  
> +#ifdef CONFIG_IIO_TRIGGER
Hmm. Generally frowned upon to have ifdef blocks inline within c files.
Perhaps worth pulling these out to inkern-trigger.c and using Kconfig magic
to build if relevant - not worth splitting the header though.

> +
> +static struct iio_trigger *of_iio_trigger_get(struct device_node *np, int index)
> +{
> +	struct device *idev;
> +	struct iio_dev *indio_dev;
> +	int err;
> +	struct of_phandle_args iiospec;
> +	struct iio_trigger *trig;
> +
> +	err = of_parse_phandle_with_args(np, "io-triggers",
> +					 "#io-trigger-cells",
> +					 index, &iiospec);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> +			       iio_dev_node_match);
> +	of_node_put(iiospec.np);
> +	if (!idev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	indio_dev = dev_to_iio_dev(idev);
> +
> +	trig = iio_trigger_find_from_device(indio_dev, iiospec.args[0]);
> +
> +	if (!trig)
> +		return ERR_PTR(-ENODEV);
> +
> +	return trig;
> +}
> +#endif /* CONFIG_IIO_TRIGGER */
> +
>  #else /* CONFIG_OF */
>  
>  static inline struct iio_channel *
> @@ -275,6 +312,12 @@ static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  	return NULL;
>  }
>  
> +static inline struct iio_trigger *of_iio_trigger_get(struct device_node *np,
> +						     int index)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_OF */
>  
>  static struct iio_channel *iio_channel_get_sys(const char *name,
> @@ -927,3 +970,51 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
>  			       chan->channel, buf, len);
>  }
>  EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
> +
> +#ifdef CONFIG_IIO_TRIGGER
> +struct iio_trigger *iio_trigger_find(struct device *dev, char *name)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct iio_trigger *trig = NULL;
> +
> +	/* Walk up the tree of devices looking for a matching iio trigger */
> +	while (np) {
> +		int index = 0;
> +
> +		/*
> +		 * For named iio triggers, first look up the name in the
> +		 * "io-trigger-names" property.  If it cannot be found, the
> +		 * index will be an error code, and of_iio_trigger_get()
> +		 * will fail.
> +		 */
> +		if (name)
> +			index = of_property_match_string(np, "io-trigger-names",
> +							 name);
> +		trig = of_iio_trigger_get(np, index);
> +		if (!IS_ERR(trig) || PTR_ERR(trig) == -EPROBE_DEFER) {
> +			break;
> +		} else if (name && index >= 0) {
> +			pr_err("ERROR: could not get IIO trigger %pOF:%s(%i)\n",
> +			       np, name ? name : "", index);
> +			return trig;
> +		}
> +
> +		/*
> +		 * No matching IIO trigger found on this node.
> +		 * If the parent node has a "io-trigger-ranges" property,
> +		 * then we can try one of its channels.
> +		 */
> +		np = np->parent;
> +		if (np && !of_get_property(np, "io-trigger-ranges", NULL))
> +			return trig;
> +	}
> +
> +	if (!trig || (IS_ERR(trig) && PTR_ERR(trig) != -EPROBE_DEFER))
> +		dev_dbg(dev, "error retrieving trigger information\n");
> +	else
> +		dev_dbg(dev, "trigger found: %s\n", name);
> +
> +	return trig;
> +}
> +EXPORT_SYMBOL_GPL(iio_trigger_find);
> +#endif /* CONFIG_IIO_TRIGGER */
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index 13be595..b2fc485 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -62,6 +62,7 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p);
>  
>  void iio_trigger_notify_done(struct iio_trigger *trig);
>  
> +struct iio_trigger *iio_trigger_find(struct device *dev, char *name);
>  /*
>   * Two functions for common case where all that happens is a pollfunc
>   * is attached and detached from a trigger

Re: [PATCH 07/14] iio: triggers: on pollfunc attach, complete iio_dev if NULL

[Jonathan Cameron]

On Fri, 22 Dec 2017 17:07:14 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> When attaching a pollfunc to a trigger, if the pollfunc does not
> have an associated iio_dev pointer, just use the private data
> iio_dev pointer from the trigger to fill in the poll func required
> iio_dev reference.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

I'm yet to be convinced this is necessary rather than using a callback
buffer. It's also decidedly unsafe as there is no particular reason
in general to assume the private data is an iio_dev.

> ---
>  drivers/iio/industrialio-trigger.c   | 9 +++++++++
>  include/linux/iio/trigger_consumer.h | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 8565c92..ab180bd 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -272,6 +272,15 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>  	bool notinuse
>  		= bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
>  
> +	/*
> +	 * If we did not get a iio_dev in the poll func, attempt to
> +	 * obtain the trigger's owner's device struct
> +	 */
> +	if (!pf->indio_dev)
> +		pf->indio_dev = iio_trigger_get_drvdata(trig);

That isn't always valid. Triggers don't always have a iio_dev associated
with them at all.
> +	if (!pf->indio_dev)
> +		return -EINVAL;
> +
>  	/* Prevent the module from being removed whilst attached to a trigger */
>  	__module_get(pf->indio_dev->driver_module);
>  
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index aeefcdb..36e2a02 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -63,6 +63,8 @@ int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
>  /*
>   * Two functions for the uncommon case when we need to attach or detach
>   * a specific pollfunc to and from a trigger
> + * If the pollfunc has a NULL iio_dev pointer, it will be filled from the
> + * trigger struct.
>   */
>  int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>  				 struct iio_poll_func *pf);

Re: [PATCH 03/14] dt-bindings: iio: add binding support for iio trigger provider/consumer

[Jonathan Cameron]

On Tue, 26 Dec 2017 16:35:00 -0600
Rob Herring <robh@kernel.org> wrote:

> On Fri, Dec 22, 2017 at 05:07:10PM +0200, Eugen Hristev wrote:
> > Add bindings for producer/consumer for iio triggers.
> > 
> > Similar with iio channels, the iio triggers can be connected between drivers:
> > one driver will be a producer by registering iio triggers, and another driver
> > will connect as a consumer.
> > 
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
In cases where the connectivity is entirely known to the various drivers
(battery chargers integrated in SoCs that use ADC channels for example) we
have always kept the map in driver.

I'm not yet entirely clear if we can do this here.  Might make sense if
we can...


> > ---
> >  .../devicetree/bindings/iio/iio-bindings.txt       | 52 +++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > index 68d6f8c..d861f0df 100644
> > --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -11,6 +11,10 @@ value of a #io-channel-cells property in the IIO provider node.
> >  
> >  [1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
> >  
> > +Moreover, the provider can have a set of triggers that can be attached to
> > +from the consumer drivers.
> > +
> > +
> >  ==IIO providers==
> >  
> >  Required properties:
> > @@ -18,6 +22,11 @@ Required properties:
> >  		   with a single IIO output and 1 for nodes with multiple
> >  		   IIO outputs.
> >  
> > +Optional properties:
> > +#io-trigger-cells: Number of cells for the IIO trigger specifier. Typically 0
> > +		   for nodes with a single IIO trigger and 1 for nodes with
> > +		   multiple IIO triggers.
> > +
> >  Example for a simple configuration with no trigger:
> >  
> >  	adc: voltage-sensor@35 {
> > @@ -26,7 +35,7 @@ Example for a simple configuration with no trigger:
> >  		#io-channel-cells = <1>;
> >  	};
> >  
> > -Example for a configuration with trigger:
> > +Example for a configuration with channels provided by trigger:
> >  
> >  	adc@35 {
> >  		compatible = "some-vendor,some-adc";
> > @@ -42,6 +51,17 @@ Example for a configuration with trigger:
> >  		};
> >  	};
> >  
> > +Example for a configuration for a trigger provider:
> > +
> > +	adc: sensor-with-trigger@35 {
> > +		compatible = "some-vendor,some-adc";
> > +		reg = <0x35>;
> > +		#io-channel-cells = <1>;
> > +		#io-trigger-cells = <1>;
> > +		/* other properties */
> > +	};
> > +
> > +
> >  ==IIO consumers==
> >  
> >  Required properties:
> > @@ -61,16 +81,38 @@ io-channel-ranges:
> >  		IIO channels from this node. Useful for bus nodes to provide
> >  		and IIO channel to their children.
> >  
> > +io-triggers:	List of phandle and IIO specifier pairs, one pair
> > +		for each trigger input to the device. Note: if the
> > +		IIO trigger provider specifies '0' for #io-trigger-cells,
> > +		then only the phandle portion of the pair will appear.
> > +
> > +io-trigger-names:
> > +		List of IIO trigger input name strings sorted in the same
> > +		order as the io-triggers property. Consumers drivers
> > +		will use io-trigger-names to match IIO trigger input names
> > +		with IIO specifiers.
> > +
> > +io-trigger-ranges:
> > +		Empty property indicating that child nodes can inherit named
> > +		IIO triggers from this node. Useful for bus nodes to provide
> > +		IIO triggers to their children.  
> 
> I think it would be better to be explicit in the child nodes. What's the 
> use you had in mind?
> 
> Rob
> --
> 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 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Eugen Hristev]

On 29.12.2017 19:02, Jonathan Cameron wrote:
> On Fri, 22 Dec 2017 17:07:19 +0200
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> The ADC IP supports position and pressure measurements for a touchpad
>> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
>> measurement support.
>> Using the inkern API, a driver can request a trigger and read the
>> channel values from the ADC.
>> The implementation provides a trigger named "touch" which can be
>> connected to a consumer driver.
>> Once a driver connects and attaches a pollfunc to this trigger, the
>> configure trigger callback is called, and then the ADC driver will
>> initialize pad measurement.
>> First step is to enable touchscreen 4wire support and enable
>> pen detect IRQ.
>> Once a pen is detected, a periodic trigger is setup to trigger every
>> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
>> is called, and the consumer driver is then woke up, and it can read the
>> respective channels for the values : X, and Y for position and pressure
>> channel.
>> Because only one trigger can be active in hardware in the same time,
>> while touching the pad, the ADC will block any attempt to use the
>> triggered buffer. Same, conversions using the software trigger are also
>> impossible (since the periodic trigger is setup).
>> If some driver wants to attach while the trigger is in use, it will
>> also fail.
>> Once the pen is not detected anymore, the trigger is free for use (hardware
>> or software trigger, with or without DMA).
>> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
>>
>> Some parts of this patch are based on initial original work by
>> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
>>
> OK, so comments inline.
> 
> What I'm missing currently though is an explanation of why the slightly
> more standard arrangement of using a callback buffer doesn't work here.
> The only addition I think you need to do that is to allow a consumer to
> request a particular trigger.  I also think some of the other provisions
> could be handled using standard features and slightly reducing the flexibility.
> I don't know for example if it's useful to allow other channels to be
> read when touch is not in progress or not.
> 
> So restrictions:
> 
> 1. Touch screen channels can only be read when touch is enabled.
>   - use the available_scan_masks to control this. Or the callback that lets
>     you do the same dynamically.
> 2. You need to push these channels to your consumer driver.
>   - register a callback buffer rather than jumping through the hoops to
>     insert your own pollfunc.  That will call a function in your
>     consumer, providing the data from the 3 channels directly.
> 3. You need to make sure it is using the right driver.  For that you
>     will I think need a new interface.
> 
> Various other comments inline. I may well be missing something as this is
> a fair bit of complex code to read - if so then next version should have
> a clear cover letter describing why this more standard approach can't be
> used.

Hello Jonathan and thanks for the review of my patch series,

before starting and working over the required modifications and 
suggestions that you sent me, I want to be a little more explicit about 
the design of my implementation.
Hope this will clarify some things, and maybe I can as well understand 
better what you have in mind to support this feature set.

Why have I picked a pollfunction: We discussed a while back on the 
mailing list that you do not have an inkern mechanism to expose the 
triggers to other drivers, and that it may be a good idea to have it for 
such kind of actually multi function device, instead of having a MFD 
driver, an ADC driver, and an Input driver, all sharing the same 
register map, the same IRQ , etc, with some kind of synchronization to 
avoid stepping on each other for the hardware resource.
So I considered to expose the trigger by attaching and detaching 
pollfunctions to it. Which is the main thing what we use a trigger for.

So, what I had in mind, was to create a consumer driver that will 
request triggers from the IIO device just like other drivers request 
channels (part which is already done in IIO).
In order to do this I had to somehow wake up the consumer driver when 
new data was available from the touchscreen. So, having the IRQ only in 
the ADC device, and then on Pen detect and No pen detect just start or 
stop the periodic trigger, which needs to be polled. The magic part is 
that the consumer driver has a poll function already attached to this 
trigger, so the poll function is just called every time we have new 
data. The poll function is attached as an irq handler, and then we can 
reuse all the read_raw data by using a scheduled work from the consumer 
driver, to read the channels.
To do this, the ADC registers a special trigger named "touch trigger" 
which is never enabled by the ADC driver. Instead, when a pollfunc is 
attached to it, the attach function will also configure it with enabled 
state. In the ADC, this means to start the touchscreen functionality. If 
the touch is requested, it will standby and wait for pen detect IRQ.
Once we have pen detect, we can use a periodic trigger to sample the 
touch data, and poll the "touch" trigger. The consumer driver will wake 
up and schedule a work , that will use the standard read raw interface 
(inkern) that will read three virtual channels (position + pressure). 
They are not actual hardware channels, as the touch information is being 
received on channels 0,1,2,3, but reading these virtual channels will 
read from different registers inside the ADC IP ( x position, y 
position, pressure), do some computations on the data, and feed the 
consumer with the values , hiding the behind the scenes hardware 
specific calculations.
After trigger is polled , the ADC will resume normal functionality, and 
the consumer driver will continue to sleep.
We need to have a periodic trigger to sample the data because the actual 
analog to digital conversion inside the IP block needs to be triggered. 
The touchscreen data measurements cannot happen in hardware without 
being triggered. If I try with a hrtimer to get a periodic IRQ to just 
read the data, it will never be ready. The datasheet states that the 
touchscreen measurements "will be attached to the conversion sequence". 
So the periodic trigger is forcing a conversion sequence. This could be 
done with a software trigger as well, but why the hassle to start it 
every 2 milliseconds (or other time interval), if we can do it by 
periodic trigger ?
Once we get the No pen IRQ, we stop the periodic trigger and it can be 
used in another purpose (software or external as of now in the driver, 
in the future we can add PWM trigger and Timer trigger)

In short, the ADC in Sama5D2 also supports touchscreen, and in 
touchscreen mode , 4 of the channels are being used for this purpose. 
This however, doesn't stop the ADC to use the other channels . The 
hardware has 12 total single channels and they can be paired to have 6 
more differential channels. The only thing that is blocked is the 
trigger, but only if the pen is touching (when we start the periodic 
trigger to sample the touchscreen). If the pen is not touching, an 
external trigger or software trigger can be used without any issues (so 
why limit the functionality, if this is available from hardware ?). 
Because of the reason I discussed above (touchscreen sequence must be 
triggered), we cannot use another trigger in the same time.


I see your idea with the callback buffer and it's worth exploring. 
Mainly this series was to actually show you what I had in mind about 
supporting the resistive touchscreen, and to give you some actually 
working code/patch, so we can discuss based on real implementation, not 
just suppositions.

You are right in many of the other comments that you said, and I will 
come up with a v2 to this series. For now, I need to know if this is a 
good or right direction in which I am going, or I should try to change 
all the mechanism to callback buffer ? Or maybe I am totally in a bad 
direction ?
The requirements are that the consumer driver needs to be somehow woke 
up for every new touch data available, and report to the input 
subsystem. As it was done before, the at91 old driver, just creates and 
registers an input device by itself, and then reports the position and 
touches. I was thinking that with this trigger consumer implementation, 
things can be better in terms of subsystem separation and support.

Thanks again and let me know of your thoughts,

Eugen



[...]

Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Jonathan Cameron]

On Thu, 4 Jan 2018 17:17:54 +0200
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On 29.12.2017 19:02, Jonathan Cameron wrote:
> > On Fri, 22 Dec 2017 17:07:19 +0200
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >   
> >> The ADC IP supports position and pressure measurements for a touchpad
> >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> >> measurement support.
> >> Using the inkern API, a driver can request a trigger and read the
> >> channel values from the ADC.
> >> The implementation provides a trigger named "touch" which can be
> >> connected to a consumer driver.
> >> Once a driver connects and attaches a pollfunc to this trigger, the
> >> configure trigger callback is called, and then the ADC driver will
> >> initialize pad measurement.
> >> First step is to enable touchscreen 4wire support and enable
> >> pen detect IRQ.
> >> Once a pen is detected, a periodic trigger is setup to trigger every
> >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> >> is called, and the consumer driver is then woke up, and it can read the
> >> respective channels for the values : X, and Y for position and pressure
> >> channel.
> >> Because only one trigger can be active in hardware in the same time,
> >> while touching the pad, the ADC will block any attempt to use the
> >> triggered buffer. Same, conversions using the software trigger are also
> >> impossible (since the periodic trigger is setup).
> >> If some driver wants to attach while the trigger is in use, it will
> >> also fail.
> >> Once the pen is not detected anymore, the trigger is free for use (hardware
> >> or software trigger, with or without DMA).
> >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> >>
> >> Some parts of this patch are based on initial original work by
> >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> >>  
> > OK, so comments inline.
> > 
> > What I'm missing currently though is an explanation of why the slightly
> > more standard arrangement of using a callback buffer doesn't work here.
> > The only addition I think you need to do that is to allow a consumer to
> > request a particular trigger.  I also think some of the other provisions
> > could be handled using standard features and slightly reducing the flexibility.
> > I don't know for example if it's useful to allow other channels to be
> > read when touch is not in progress or not.
> > 
> > So restrictions:
> > 
> > 1. Touch screen channels can only be read when touch is enabled.
> >   - use the available_scan_masks to control this. Or the callback that lets
> >     you do the same dynamically.
> > 2. You need to push these channels to your consumer driver.
> >   - register a callback buffer rather than jumping through the hoops to
> >     insert your own pollfunc.  That will call a function in your
> >     consumer, providing the data from the 3 channels directly.
> > 3. You need to make sure it is using the right driver.  For that you
> >     will I think need a new interface.
> > 
> > Various other comments inline. I may well be missing something as this is
> > a fair bit of complex code to read - if so then next version should have
> > a clear cover letter describing why this more standard approach can't be
> > used.  
> 
> Hello Jonathan and thanks for the review of my patch series,
> 
> before starting and working over the required modifications and 
> suggestions that you sent me, I want to be a little more explicit about 
> the design of my implementation.
> Hope this will clarify some things, and maybe I can as well understand 
> better what you have in mind to support this feature set.
> 
> Why have I picked a pollfunction: We discussed a while back on the 
> mailing list that you do not have an inkern mechanism to expose the 
> triggers to other drivers, and that it may be a good idea to have it for 
> such kind of actually multi function device, instead of having a MFD 
> driver, an ADC driver, and an Input driver, all sharing the same 
> register map, the same IRQ , etc, with some kind of synchronization to 
> avoid stepping on each other for the hardware resource.

No disagreement with that principle.

> So I considered to expose the trigger by attaching and detaching 
> pollfunctions to it. Which is the main thing what we use a trigger for.

Hmm. It's definitely one approach. But we do already have other drivers
where the trigger is controlled by a consumer and to my mind that
is a cleaner approach as it doesn't short cut the equivalent of
doing it from userspace.

drivers/iio/potentiostat/lmp91000.c does something similar though
for a rather different use. You need your consumer interface
to get the handle to the trigger in this case
(the lmp91000 is actually providing the trigger rather than
consuming it).


> 
> So, what I had in mind, was to create a consumer driver that will 
> request triggers from the IIO device just like other drivers request 
> channels (part which is already done in IIO).
> In order to do this I had to somehow wake up the consumer driver when 
> new data was available from the touchscreen. So, having the IRQ only in 
> the ADC device, and then on Pen detect and No pen detect just start or 
> stop the periodic trigger, which needs to be polled. The magic part is 
> that the consumer driver has a poll function already attached to this 
> trigger, so the poll function is just called every time we have new 
> data. The poll function is attached as an irq handler, and then we can 
> reuse all the read_raw data by using a scheduled work from the consumer 
> driver, to read the channels.

If you had done this via a callback buffer the only difference is that
the pollfunc would have been a standard one pulling the relevant channels
and passing them on down to the buffer interface which could then decide
what to do with them.

> To do this, the ADC registers a special trigger named "touch trigger" 
> which is never enabled by the ADC driver. Instead, when a pollfunc is 
> attached to it, the attach function will also configure it with enabled 
> state.

Whilst it might not make sense to enable it in the touch screen driver
I'm not sure there is strictly any reason to prevent it being so used.

> In the ADC, this means to start the touchscreen functionality. If 
> the touch is requested, it will standby and wait for pen detect IRQ.
> Once we have pen detect, we can use a periodic trigger to sample the 
> touch data, and poll the "touch" trigger. The consumer driver will wake 
> up and schedule a work , that will use the standard read raw interface 
> (inkern) that will read three virtual channels (position + pressure). 
> They are not actual hardware channels, as the touch information is being 
> received on channels 0,1,2,3, but reading these virtual channels will 
> read from different registers inside the ADC IP ( x position, y 
> position, pressure), do some computations on the data, and feed the 
> consumer with the values , hiding the behind the scenes hardware 
> specific calculations.

I wouldn't worry about whether they are real channels or not. This
is really similar to a differential ADC (some of those do the differential
digitally). Light sensors often have a number of 'real' channels used
to derive (via hideous non linear calculations) the illuminance as
it's hard to build a light sensor with the same sensitivity as the human
eye.  We have worse 'non real' channels as well such as activity channels
on some the accelerometers that report if it thinks you are walking /
running etc.
 
> After trigger is polled , the ADC will resume normal functionality, and 
> the consumer driver will continue to sleep.

So this is where I'm unsure.  Do you actually have a usecase where it
makes the sense to read from the ADC only when there is no touch?  Any
system doing that has an obvious denial of service attack - touch the
screen.

> We need to have a periodic trigger to sample the data because the actual 
> analog to digital conversion inside the IP block needs to be triggered. 
> The touchscreen data measurements cannot happen in hardware without 
> being triggered. If I try with a hrtimer to get a periodic IRQ to just 
> read the data, it will never be ready. The datasheet states that the 
> touchscreen measurements "will be attached to the conversion sequence". 
> So the periodic trigger is forcing a conversion sequence. This could be 
> done with a software trigger as well, but why the hassle to start it 
> every 2 milliseconds (or other time interval), if we can do it by 
> periodic trigger ?

Ah, one reason here would be to allow separate consumers to use the
device. In that case you'd run with a periodic trigger all the time
and have two buffers attached, the buffer_cb that is feeding your
touchscreen and another buffer to deal with the other channels
(presumably the standard one an IIO device has when using buffered
interfaces).

The buffer demux would ensure the data from the right channels
ends up in the right place.  It makes it look to the buffer
consumer like it is the only thing using / controlling the data
flow.

> Once we get the No pen IRQ, we stop the periodic trigger and it can be 
> used in another purpose (software or external as of now in the driver, 
> in the future we can add PWM trigger and Timer trigger)

This case isn't really useful though as any other use is denied
access when touch occurs.

I'll summarise what I think would work for this below.

> 
> In short, the ADC in Sama5D2 also supports touchscreen, and in 
> touchscreen mode , 4 of the channels are being used for this purpose. 
> This however, doesn't stop the ADC to use the other channels . The 
> hardware has 12 total single channels and they can be paired to have 6 
> more differential channels. The only thing that is blocked is the 
> trigger, but only if the pen is touching (when we start the periodic 
> trigger to sample the touchscreen). If the pen is not touching, an 
> external trigger or software trigger can be used without any issues (so 
> why limit the functionality, if this is available from hardware ?). 
> Because of the reason I discussed above (touchscreen sequence must be 
> triggered), we cannot use another trigger in the same time.
> 
> 
> I see your idea with the callback buffer and it's worth exploring. 
> Mainly this series was to actually show you what I had in mind about 
> supporting the resistive touchscreen, and to give you some actually 
> working code/patch, so we can discuss based on real implementation, not 
> just suppositions.

That side of things is fine.

> 
> You are right in many of the other comments that you said, and I will 
> come up with a v2 to this series. For now, I need to know if this is a 
> good or right direction in which I am going, or I should try to change 
> all the mechanism to callback buffer ? Or maybe I am totally in a bad 
> direction ?
> The requirements are that the consumer driver needs to be somehow woke 
> up for every new touch data available, and report to the input 
> subsystem. As it was done before, the at91 old driver, just creates and 
> registers an input device by itself, and then reports the position and 
> touches. I was thinking that with this trigger consumer implementation, 
> things can be better in terms of subsystem separation and support.
> 
> Thanks again and let me know of your thoughts,
> 
> Eugen

So a couple of things come to mind on how I'd structure this.
So what we have (very briefly)

No touch screen case:
* Generic ADC using all sorts of different triggers

Touch screen only case:
* Interrupt to indicate pen on / off
* A need to do a periodic trigger of the device but only
useful when touch is in progress.

Touch screen and other users:
* Interrupt to indicate pen on / off
* Periodic trigger needed for touchscreen when touch in progress.
* Do not have denial of service on other channels.

First two cases are easy enough by having a magic trigger, third
case is harder.
If we have the touchscreen then I would drop support for direct access to
to ADC channels whilst it's in use (so no sysfs - or emulate it if you
really want it by stashing results from scans done when touch is in
progress).

Have your touch screen channels just as normal additional channels,
but only via the buffered interface (no _RAW attribute).
If someone sets up to read them via buffered interface with
a different trigger I think they'll get values - whether they
are right is dependent (if I understand correctly) on whether
there is a touch in progress.  So no harm done and it'll make
the logic simpler.

The moment touch is opened and acquires the IIO channels
fix the trigger (may need new interface) to the periodic one
that you were enabling and disabling on touch.
Things get dicey if there is an existing user so you may
have to do it on driver probe rather than open of the input
device if we effectively want touch to have the highest
priority use of the ADC.

If other channels are enabled for buffered mode then note
this in the driver and have the periodic trigger on all the
time (to ensure they keep getting read)  This will pass
garbage to your touch screen driver, but it'll remove it due
to the pressure value being too low so no harm there.

Normal path will work for non touch channels (and in theory
the touch ones if they are turned on) via IIO buffer
interface.  It'll be restricted in form due to the needs of
the touch driver, but better than nothing and should cover
most usecases.

Now the interrupt on / off on touch bit becomes an optimization
in the case of only the buffer_cb being attached.

I think that fits cleanly in the current IIO framework and
looks more similar to our existing provider consumer approaches.

Still needs the hooks to get hold of the trigger though so
as to be able to tell the ADC which one to use. So rather
than being a trigger consumer interface, it's more of a trigger
configuration interface..  Exact term doesn't matter though.

Jonathan

> 
> 
> 
> [...]
> --
> 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 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Ludovic Desroches]

Hi Jonathan, Eugen,

On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote:
> On Thu, 4 Jan 2018 17:17:54 +0200
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
> > On 29.12.2017 19:02, Jonathan Cameron wrote:
> > > On Fri, 22 Dec 2017 17:07:19 +0200
> > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > >   
> > >> The ADC IP supports position and pressure measurements for a touchpad
> > >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> > >> measurement support.
> > >> Using the inkern API, a driver can request a trigger and read the
> > >> channel values from the ADC.
> > >> The implementation provides a trigger named "touch" which can be
> > >> connected to a consumer driver.
> > >> Once a driver connects and attaches a pollfunc to this trigger, the
> > >> configure trigger callback is called, and then the ADC driver will
> > >> initialize pad measurement.
> > >> First step is to enable touchscreen 4wire support and enable
> > >> pen detect IRQ.
> > >> Once a pen is detected, a periodic trigger is setup to trigger every
> > >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> > >> is called, and the consumer driver is then woke up, and it can read the
> > >> respective channels for the values : X, and Y for position and pressure
> > >> channel.
> > >> Because only one trigger can be active in hardware in the same time,
> > >> while touching the pad, the ADC will block any attempt to use the
> > >> triggered buffer. Same, conversions using the software trigger are also
> > >> impossible (since the periodic trigger is setup).
> > >> If some driver wants to attach while the trigger is in use, it will
> > >> also fail.
> > >> Once the pen is not detected anymore, the trigger is free for use (hardware
> > >> or software trigger, with or without DMA).
> > >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> > >>
> > >> Some parts of this patch are based on initial original work by
> > >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> > >>  
> > > OK, so comments inline.
> > > 
> > > What I'm missing currently though is an explanation of why the slightly
> > > more standard arrangement of using a callback buffer doesn't work here.
> > > The only addition I think you need to do that is to allow a consumer to
> > > request a particular trigger.  I also think some of the other provisions
> > > could be handled using standard features and slightly reducing the flexibility.
> > > I don't know for example if it's useful to allow other channels to be
> > > read when touch is not in progress or not.
> > > 
> > > So restrictions:
> > > 
> > > 1. Touch screen channels can only be read when touch is enabled.
> > >   - use the available_scan_masks to control this. Or the callback that lets
> > >     you do the same dynamically.
> > > 2. You need to push these channels to your consumer driver.
> > >   - register a callback buffer rather than jumping through the hoops to
> > >     insert your own pollfunc.  That will call a function in your
> > >     consumer, providing the data from the 3 channels directly.
> > > 3. You need to make sure it is using the right driver.  For that you
> > >     will I think need a new interface.
> > > 
> > > Various other comments inline. I may well be missing something as this is
> > > a fair bit of complex code to read - if so then next version should have
> > > a clear cover letter describing why this more standard approach can't be
> > > used.  
> > 
> > Hello Jonathan and thanks for the review of my patch series,
> > 
> > before starting and working over the required modifications and 
> > suggestions that you sent me, I want to be a little more explicit about 
> > the design of my implementation.
> > Hope this will clarify some things, and maybe I can as well understand 
> > better what you have in mind to support this feature set.
> > 
> > Why have I picked a pollfunction: We discussed a while back on the 
> > mailing list that you do not have an inkern mechanism to expose the 
> > triggers to other drivers, and that it may be a good idea to have it for 
> > such kind of actually multi function device, instead of having a MFD 
> > driver, an ADC driver, and an Input driver, all sharing the same 
> > register map, the same IRQ , etc, with some kind of synchronization to 
> > avoid stepping on each other for the hardware resource.
> 
> No disagreement with that principle.
> 
> > So I considered to expose the trigger by attaching and detaching 
> > pollfunctions to it. Which is the main thing what we use a trigger for.
> 
> Hmm. It's definitely one approach. But we do already have other drivers
> where the trigger is controlled by a consumer and to my mind that
> is a cleaner approach as it doesn't short cut the equivalent of
> doing it from userspace.
> 
> drivers/iio/potentiostat/lmp91000.c does something similar though
> for a rather different use. You need your consumer interface
> to get the handle to the trigger in this case
> (the lmp91000 is actually providing the trigger rather than
> consuming it).
> 
> 
> > 
> > So, what I had in mind, was to create a consumer driver that will 
> > request triggers from the IIO device just like other drivers request 
> > channels (part which is already done in IIO).
> > In order to do this I had to somehow wake up the consumer driver when 
> > new data was available from the touchscreen. So, having the IRQ only in 
> > the ADC device, and then on Pen detect and No pen detect just start or 
> > stop the periodic trigger, which needs to be polled. The magic part is 
> > that the consumer driver has a poll function already attached to this 
> > trigger, so the poll function is just called every time we have new 
> > data. The poll function is attached as an irq handler, and then we can 
> > reuse all the read_raw data by using a scheduled work from the consumer 
> > driver, to read the channels.
> 
> If you had done this via a callback buffer the only difference is that
> the pollfunc would have been a standard one pulling the relevant channels
> and passing them on down to the buffer interface which could then decide
> what to do with them.
> 
> > To do this, the ADC registers a special trigger named "touch trigger" 
> > which is never enabled by the ADC driver. Instead, when a pollfunc is 
> > attached to it, the attach function will also configure it with enabled 
> > state.
> 
> Whilst it might not make sense to enable it in the touch screen driver
> I'm not sure there is strictly any reason to prevent it being so used.
> 
> > In the ADC, this means to start the touchscreen functionality. If 
> > the touch is requested, it will standby and wait for pen detect IRQ.
> > Once we have pen detect, we can use a periodic trigger to sample the 
> > touch data, and poll the "touch" trigger. The consumer driver will wake 
> > up and schedule a work , that will use the standard read raw interface 
> > (inkern) that will read three virtual channels (position + pressure). 
> > They are not actual hardware channels, as the touch information is being 
> > received on channels 0,1,2,3, but reading these virtual channels will 
> > read from different registers inside the ADC IP ( x position, y 
> > position, pressure), do some computations on the data, and feed the 
> > consumer with the values , hiding the behind the scenes hardware 
> > specific calculations.
> 
> I wouldn't worry about whether they are real channels or not. This
> is really similar to a differential ADC (some of those do the differential
> digitally). Light sensors often have a number of 'real' channels used
> to derive (via hideous non linear calculations) the illuminance as
> it's hard to build a light sensor with the same sensitivity as the human
> eye.  We have worse 'non real' channels as well such as activity channels
> on some the accelerometers that report if it thinks you are walking /
> running etc.
>  
> > After trigger is polled , the ADC will resume normal functionality, and 
> > the consumer driver will continue to sleep.
> 
> So this is where I'm unsure.  Do you actually have a usecase where it
> makes the sense to read from the ADC only when there is no touch?  Any
> system doing that has an obvious denial of service attack - touch the
> screen.
> 

You're right. We have an issue in this case due to the hardware. Using
touchscreen has side effects on other channels. We can use only one
trigger for all the channels. The situation would have been better with
a trigger dedicated to the touchscreen.

At the moment, we have not really stated about the exclusive use or not
of the touchscreen. We suppose we can get some customers wanting to use
both touchscreen and ADC. Eugen tried to deal with this case but, as you
noticed, it can lead to DoS.

> > We need to have a periodic trigger to sample the data because the actual 
> > analog to digital conversion inside the IP block needs to be triggered. 
> > The touchscreen data measurements cannot happen in hardware without 
> > being triggered. If I try with a hrtimer to get a periodic IRQ to just 
> > read the data, it will never be ready. The datasheet states that the 
> > touchscreen measurements "will be attached to the conversion sequence". 
> > So the periodic trigger is forcing a conversion sequence. This could be 
> > done with a software trigger as well, but why the hassle to start it 
> > every 2 milliseconds (or other time interval), if we can do it by 
> > periodic trigger ?
> 
> Ah, one reason here would be to allow separate consumers to use the
> device. In that case you'd run with a periodic trigger all the time
> and have two buffers attached, the buffer_cb that is feeding your
> touchscreen and another buffer to deal with the other channels
> (presumably the standard one an IIO device has when using buffered
> interfaces).

The issue is that we are sharing the periodic trigger so we have to use
the same period for both usage.

Regards

Ludovic

> 
> The buffer demux would ensure the data from the right channels
> ends up in the right place.  It makes it look to the buffer
> consumer like it is the only thing using / controlling the data
> flow.
> 
> > Once we get the No pen IRQ, we stop the periodic trigger and it can be 
> > used in another purpose (software or external as of now in the driver, 
> > in the future we can add PWM trigger and Timer trigger)
> 
> This case isn't really useful though as any other use is denied
> access when touch occurs.
> 
> I'll summarise what I think would work for this below.
> 
> > 
> > In short, the ADC in Sama5D2 also supports touchscreen, and in 
> > touchscreen mode , 4 of the channels are being used for this purpose. 
> > This however, doesn't stop the ADC to use the other channels . The 
> > hardware has 12 total single channels and they can be paired to have 6 
> > more differential channels. The only thing that is blocked is the 
> > trigger, but only if the pen is touching (when we start the periodic 
> > trigger to sample the touchscreen). If the pen is not touching, an 
> > external trigger or software trigger can be used without any issues (so 
> > why limit the functionality, if this is available from hardware ?). 
> > Because of the reason I discussed above (touchscreen sequence must be 
> > triggered), we cannot use another trigger in the same time.
> > 
> > 
> > I see your idea with the callback buffer and it's worth exploring. 
> > Mainly this series was to actually show you what I had in mind about 
> > supporting the resistive touchscreen, and to give you some actually 
> > working code/patch, so we can discuss based on real implementation, not 
> > just suppositions.
> 
> That side of things is fine.
> 
> > 
> > You are right in many of the other comments that you said, and I will 
> > come up with a v2 to this series. For now, I need to know if this is a 
> > good or right direction in which I am going, or I should try to change 
> > all the mechanism to callback buffer ? Or maybe I am totally in a bad 
> > direction ?
> > The requirements are that the consumer driver needs to be somehow woke 
> > up for every new touch data available, and report to the input 
> > subsystem. As it was done before, the at91 old driver, just creates and 
> > registers an input device by itself, and then reports the position and 
> > touches. I was thinking that with this trigger consumer implementation, 
> > things can be better in terms of subsystem separation and support.
> > 
> > Thanks again and let me know of your thoughts,
> > 
> > Eugen
> 
> So a couple of things come to mind on how I'd structure this.
> So what we have (very briefly)
> 
> No touch screen case:
> * Generic ADC using all sorts of different triggers
> 
> Touch screen only case:
> * Interrupt to indicate pen on / off
> * A need to do a periodic trigger of the device but only
> useful when touch is in progress.
> 
> Touch screen and other users:
> * Interrupt to indicate pen on / off
> * Periodic trigger needed for touchscreen when touch in progress.
> * Do not have denial of service on other channels.
> 
> First two cases are easy enough by having a magic trigger, third
> case is harder.
> If we have the touchscreen then I would drop support for direct access to
> to ADC channels whilst it's in use (so no sysfs - or emulate it if you
> really want it by stashing results from scans done when touch is in
> progress).
> 
> Have your touch screen channels just as normal additional channels,
> but only via the buffered interface (no _RAW attribute).
> If someone sets up to read them via buffered interface with
> a different trigger I think they'll get values - whether they
> are right is dependent (if I understand correctly) on whether
> there is a touch in progress.  So no harm done and it'll make
> the logic simpler.
> 
> The moment touch is opened and acquires the IIO channels
> fix the trigger (may need new interface) to the periodic one
> that you were enabling and disabling on touch.
> Things get dicey if there is an existing user so you may
> have to do it on driver probe rather than open of the input
> device if we effectively want touch to have the highest
> priority use of the ADC.
> 
> If other channels are enabled for buffered mode then note
> this in the driver and have the periodic trigger on all the
> time (to ensure they keep getting read)  This will pass
> garbage to your touch screen driver, but it'll remove it due
> to the pressure value being too low so no harm there.
> 
> Normal path will work for non touch channels (and in theory
> the touch ones if they are turned on) via IIO buffer
> interface.  It'll be restricted in form due to the needs of
> the touch driver, but better than nothing and should cover
> most usecases.
> 
> Now the interrupt on / off on touch bit becomes an optimization
> in the case of only the buffer_cb being attached.
> 
> I think that fits cleanly in the current IIO framework and
> looks more similar to our existing provider consumer approaches.
> 
> Still needs the hooks to get hold of the trigger though so
> as to be able to tell the ADC which one to use. So rather
> than being a trigger consumer interface, it's more of a trigger
> configuration interface..  Exact term doesn't matter though.
> 
> Jonathan
> 
> > 
> > 
> > 
> > [...]
> > --
> > 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 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

[Jonathan Cameron]

On Mon, 8 Jan 2018 15:12:52 +0100
Ludovic Desroches <ludovic.desroches@microchip.com> wrote:

> Hi Jonathan, Eugen,
> 
> On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote:
> > On Thu, 4 Jan 2018 17:17:54 +0200
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >   
> > > On 29.12.2017 19:02, Jonathan Cameron wrote:  
> > > > On Fri, 22 Dec 2017 17:07:19 +0200
> > > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > >     
> > > >> The ADC IP supports position and pressure measurements for a touchpad
> > > >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> > > >> measurement support.
> > > >> Using the inkern API, a driver can request a trigger and read the
> > > >> channel values from the ADC.
> > > >> The implementation provides a trigger named "touch" which can be
> > > >> connected to a consumer driver.
> > > >> Once a driver connects and attaches a pollfunc to this trigger, the
> > > >> configure trigger callback is called, and then the ADC driver will
> > > >> initialize pad measurement.
> > > >> First step is to enable touchscreen 4wire support and enable
> > > >> pen detect IRQ.
> > > >> Once a pen is detected, a periodic trigger is setup to trigger every
> > > >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> > > >> is called, and the consumer driver is then woke up, and it can read the
> > > >> respective channels for the values : X, and Y for position and pressure
> > > >> channel.
> > > >> Because only one trigger can be active in hardware in the same time,
> > > >> while touching the pad, the ADC will block any attempt to use the
> > > >> triggered buffer. Same, conversions using the software trigger are also
> > > >> impossible (since the periodic trigger is setup).
> > > >> If some driver wants to attach while the trigger is in use, it will
> > > >> also fail.
> > > >> Once the pen is not detected anymore, the trigger is free for use (hardware
> > > >> or software trigger, with or without DMA).
> > > >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> > > >>
> > > >> Some parts of this patch are based on initial original work by
> > > >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> > > >>    
> > > > OK, so comments inline.
> > > > 
> > > > What I'm missing currently though is an explanation of why the slightly
> > > > more standard arrangement of using a callback buffer doesn't work here.
> > > > The only addition I think you need to do that is to allow a consumer to
> > > > request a particular trigger.  I also think some of the other provisions
> > > > could be handled using standard features and slightly reducing the flexibility.
> > > > I don't know for example if it's useful to allow other channels to be
> > > > read when touch is not in progress or not.
> > > > 
> > > > So restrictions:
> > > > 
> > > > 1. Touch screen channels can only be read when touch is enabled.
> > > >   - use the available_scan_masks to control this. Or the callback that lets
> > > >     you do the same dynamically.
> > > > 2. You need to push these channels to your consumer driver.
> > > >   - register a callback buffer rather than jumping through the hoops to
> > > >     insert your own pollfunc.  That will call a function in your
> > > >     consumer, providing the data from the 3 channels directly.
> > > > 3. You need to make sure it is using the right driver.  For that you
> > > >     will I think need a new interface.
> > > > 
> > > > Various other comments inline. I may well be missing something as this is
> > > > a fair bit of complex code to read - if so then next version should have
> > > > a clear cover letter describing why this more standard approach can't be
> > > > used.    
> > > 
> > > Hello Jonathan and thanks for the review of my patch series,
> > > 
> > > before starting and working over the required modifications and 
> > > suggestions that you sent me, I want to be a little more explicit about 
> > > the design of my implementation.
> > > Hope this will clarify some things, and maybe I can as well understand 
> > > better what you have in mind to support this feature set.
> > > 
> > > Why have I picked a pollfunction: We discussed a while back on the 
> > > mailing list that you do not have an inkern mechanism to expose the 
> > > triggers to other drivers, and that it may be a good idea to have it for 
> > > such kind of actually multi function device, instead of having a MFD 
> > > driver, an ADC driver, and an Input driver, all sharing the same 
> > > register map, the same IRQ , etc, with some kind of synchronization to 
> > > avoid stepping on each other for the hardware resource.  
> > 
> > No disagreement with that principle.
> >   
> > > So I considered to expose the trigger by attaching and detaching 
> > > pollfunctions to it. Which is the main thing what we use a trigger for.  
> > 
> > Hmm. It's definitely one approach. But we do already have other drivers
> > where the trigger is controlled by a consumer and to my mind that
> > is a cleaner approach as it doesn't short cut the equivalent of
> > doing it from userspace.
> > 
> > drivers/iio/potentiostat/lmp91000.c does something similar though
> > for a rather different use. You need your consumer interface
> > to get the handle to the trigger in this case
> > (the lmp91000 is actually providing the trigger rather than
> > consuming it).
> > 
> >   
> > > 
> > > So, what I had in mind, was to create a consumer driver that will 
> > > request triggers from the IIO device just like other drivers request 
> > > channels (part which is already done in IIO).
> > > In order to do this I had to somehow wake up the consumer driver when 
> > > new data was available from the touchscreen. So, having the IRQ only in 
> > > the ADC device, and then on Pen detect and No pen detect just start or 
> > > stop the periodic trigger, which needs to be polled. The magic part is 
> > > that the consumer driver has a poll function already attached to this 
> > > trigger, so the poll function is just called every time we have new 
> > > data. The poll function is attached as an irq handler, and then we can 
> > > reuse all the read_raw data by using a scheduled work from the consumer 
> > > driver, to read the channels.  
> > 
> > If you had done this via a callback buffer the only difference is that
> > the pollfunc would have been a standard one pulling the relevant channels
> > and passing them on down to the buffer interface which could then decide
> > what to do with them.
> >   
> > > To do this, the ADC registers a special trigger named "touch trigger" 
> > > which is never enabled by the ADC driver. Instead, when a pollfunc is 
> > > attached to it, the attach function will also configure it with enabled 
> > > state.  
> > 
> > Whilst it might not make sense to enable it in the touch screen driver
> > I'm not sure there is strictly any reason to prevent it being so used.
> >   
> > > In the ADC, this means to start the touchscreen functionality. If 
> > > the touch is requested, it will standby and wait for pen detect IRQ.
> > > Once we have pen detect, we can use a periodic trigger to sample the 
> > > touch data, and poll the "touch" trigger. The consumer driver will wake 
> > > up and schedule a work , that will use the standard read raw interface 
> > > (inkern) that will read three virtual channels (position + pressure). 
> > > They are not actual hardware channels, as the touch information is being 
> > > received on channels 0,1,2,3, but reading these virtual channels will 
> > > read from different registers inside the ADC IP ( x position, y 
> > > position, pressure), do some computations on the data, and feed the 
> > > consumer with the values , hiding the behind the scenes hardware 
> > > specific calculations.  
> > 
> > I wouldn't worry about whether they are real channels or not. This
> > is really similar to a differential ADC (some of those do the differential
> > digitally). Light sensors often have a number of 'real' channels used
> > to derive (via hideous non linear calculations) the illuminance as
> > it's hard to build a light sensor with the same sensitivity as the human
> > eye.  We have worse 'non real' channels as well such as activity channels
> > on some the accelerometers that report if it thinks you are walking /
> > running etc.
> >    
> > > After trigger is polled , the ADC will resume normal functionality, and 
> > > the consumer driver will continue to sleep.  
> > 
> > So this is where I'm unsure.  Do you actually have a usecase where it
> > makes the sense to read from the ADC only when there is no touch?  Any
> > system doing that has an obvious denial of service attack - touch the
> > screen.
> >   
> 
> You're right. We have an issue in this case due to the hardware. Using
> touchscreen has side effects on other channels. We can use only one
> trigger for all the channels. The situation would have been better with
> a trigger dedicated to the touchscreen.
> 
> At the moment, we have not really stated about the exclusive use or not
> of the touchscreen. We suppose we can get some customers wanting to use
> both touchscreen and ADC. Eugen tried to deal with this case but, as you
> noticed, it can lead to DoS.

It's a restriction people aren't going to expect unfortunately...

> 
> > > We need to have a periodic trigger to sample the data because the actual 
> > > analog to digital conversion inside the IP block needs to be triggered. 
> > > The touchscreen data measurements cannot happen in hardware without 
> > > being triggered. If I try with a hrtimer to get a periodic IRQ to just 
> > > read the data, it will never be ready. The datasheet states that the 
> > > touchscreen measurements "will be attached to the conversion sequence". 
> > > So the periodic trigger is forcing a conversion sequence. This could be 
> > > done with a software trigger as well, but why the hassle to start it 
> > > every 2 milliseconds (or other time interval), if we can do it by 
> > > periodic trigger ?  
> > 
> > Ah, one reason here would be to allow separate consumers to use the
> > device. In that case you'd run with a periodic trigger all the time
> > and have two buffers attached, the buffer_cb that is feeding your
> > touchscreen and another buffer to deal with the other channels
> > (presumably the standard one an IIO device has when using buffered
> > interfaces).  
> 
> The issue is that we are sharing the periodic trigger so we have to use
> the same period for both usage.

Whilst a somewhat irritating restriction, it's probably not disastrous for
most ADC uses.

Jonathan
> 
> Regards
> 
> Ludovic
> 
> > 
> > The buffer demux would ensure the data from the right channels
> > ends up in the right place.  It makes it look to the buffer
> > consumer like it is the only thing using / controlling the data
> > flow.
> >   
> > > Once we get the No pen IRQ, we stop the periodic trigger and it can be 
> > > used in another purpose (software or external as of now in the driver, 
> > > in the future we can add PWM trigger and Timer trigger)  
> > 
> > This case isn't really useful though as any other use is denied
> > access when touch occurs.
> > 
> > I'll summarise what I think would work for this below.
> >   
> > > 
> > > In short, the ADC in Sama5D2 also supports touchscreen, and in 
> > > touchscreen mode , 4 of the channels are being used for this purpose. 
> > > This however, doesn't stop the ADC to use the other channels . The 
> > > hardware has 12 total single channels and they can be paired to have 6 
> > > more differential channels. The only thing that is blocked is the 
> > > trigger, but only if the pen is touching (when we start the periodic 
> > > trigger to sample the touchscreen). If the pen is not touching, an 
> > > external trigger or software trigger can be used without any issues (so 
> > > why limit the functionality, if this is available from hardware ?). 
> > > Because of the reason I discussed above (touchscreen sequence must be 
> > > triggered), we cannot use another trigger in the same time.
> > > 
> > > 
> > > I see your idea with the callback buffer and it's worth exploring. 
> > > Mainly this series was to actually show you what I had in mind about 
> > > supporting the resistive touchscreen, and to give you some actually 
> > > working code/patch, so we can discuss based on real implementation, not 
> > > just suppositions.  
> > 
> > That side of things is fine.
> >   
> > > 
> > > You are right in many of the other comments that you said, and I will 
> > > come up with a v2 to this series. For now, I need to know if this is a 
> > > good or right direction in which I am going, or I should try to change 
> > > all the mechanism to callback buffer ? Or maybe I am totally in a bad 
> > > direction ?
> > > The requirements are that the consumer driver needs to be somehow woke 
> > > up for every new touch data available, and report to the input 
> > > subsystem. As it was done before, the at91 old driver, just creates and 
> > > registers an input device by itself, and then reports the position and 
> > > touches. I was thinking that with this trigger consumer implementation, 
> > > things can be better in terms of subsystem separation and support.
> > > 
> > > Thanks again and let me know of your thoughts,
> > > 
> > > Eugen  
> > 
> > So a couple of things come to mind on how I'd structure this.
> > So what we have (very briefly)
> > 
> > No touch screen case:
> > * Generic ADC using all sorts of different triggers
> > 
> > Touch screen only case:
> > * Interrupt to indicate pen on / off
> > * A need to do a periodic trigger of the device but only
> > useful when touch is in progress.
> > 
> > Touch screen and other users:
> > * Interrupt to indicate pen on / off
> > * Periodic trigger needed for touchscreen when touch in progress.
> > * Do not have denial of service on other channels.
> > 
> > First two cases are easy enough by having a magic trigger, third
> > case is harder.
> > If we have the touchscreen then I would drop support for direct access to
> > to ADC channels whilst it's in use (so no sysfs - or emulate it if you
> > really want it by stashing results from scans done when touch is in
> > progress).
> > 
> > Have your touch screen channels just as normal additional channels,
> > but only via the buffered interface (no _RAW attribute).
> > If someone sets up to read them via buffered interface with
> > a different trigger I think they'll get values - whether they
> > are right is dependent (if I understand correctly) on whether
> > there is a touch in progress.  So no harm done and it'll make
> > the logic simpler.
> > 
> > The moment touch is opened and acquires the IIO channels
> > fix the trigger (may need new interface) to the periodic one
> > that you were enabling and disabling on touch.
> > Things get dicey if there is an existing user so you may
> > have to do it on driver probe rather than open of the input
> > device if we effectively want touch to have the highest
> > priority use of the ADC.
> > 
> > If other channels are enabled for buffered mode then note
> > this in the driver and have the periodic trigger on all the
> > time (to ensure they keep getting read)  This will pass
> > garbage to your touch screen driver, but it'll remove it due
> > to the pressure value being too low so no harm there.
> > 
> > Normal path will work for non touch channels (and in theory
> > the touch ones if they are turned on) via IIO buffer
> > interface.  It'll be restricted in form due to the needs of
> > the touch driver, but better than nothing and should cover
> > most usecases.
> > 
> > Now the interrupt on / off on touch bit becomes an optimization
> > in the case of only the buffer_cb being attached.
> > 
> > I think that fits cleanly in the current IIO framework and
> > looks more similar to our existing provider consumer approaches.
> > 
> > Still needs the hooks to get hold of the trigger though so
> > as to be able to tell the ADC which one to use. So rather
> > than being a trigger consumer interface, it's more of a trigger
> > configuration interface..  Exact term doesn't matter though.
> > 
> > Jonathan
> >   
> > > 
> > > 
> > > 
> > > [...]
> > > --
> > > 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  
> >   
> --
> 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