[PATCH v3 00/11] Add support for SAMA5D2 touchscreen

[PATCH v3 00/11] Add support for SAMA5D2 touchscreen

[Eugen Hristev]

Hello,

This patch series is a rework of my previous series named:
[PATCH 00/14] iio: triggers: add consumer support

This is the version 3 of the series, and addresses the received feedback
on the v2 series named:
[PATCH v2 00/10]  Add support for SAMA5D2 touchscreen
Changes in v3 are presented at the end of the cover letter below.
Thanks everyone for the feedback. Below is the original v2 cover letter:

In few words, this is the implementation of splitting the functionality
of the IP block ADC device in SAMA5D2 SoC from ADC with touchscreen
support. In order to avoid having a MFD device, two separate
drivers that would work on same register base and split the IRQ,etc,
as advised on the mailing list, I created a consumer driver for the
channels, that will connect to the ADC as described in the device tree.

I have collected feedback from everyone and here is the result:
I have added a new generic resistive touchscreen driver, which acts
as a iio consumer for the given channels and will create an input
device and report the events. It uses a callback buffer to register
to the IIO device and waits for data to be pushed.
Inside the IIO device, I have kept a similar approach with the first version
of the series, except that now the driver can take multiple buffers, and
will configure the touchscreen part of the hardware device if the specific
channels are requested.

The SAMA5D2 ADC driver registers three new channels: two for the
position on the X and Y axis, and one for the touch pressure.
When channels are requested, it will check if the touchscreen channel mask
includes the requested channels (it is possible that the consumer driver
will not request pressure for example). If it's the case, it will work
in touchscreen mode, and will refuse to do usual analog-digital conversion,
because we have a single trigger and the touchscreen needs it.
When the scan mask will include only old channels, the driver will function
in the same way as before. If the scan mask somehow is a mix of the two (the
masks intersect), the driver will refuse to work whatsoever (cannot have both
in the same time).
The driver allows reading raw data for the new channels, if claim direct
mode works: no touchscreen driver requested anything. The new channels can
act like the old ones. However, when requesting these channels, the usual
trigger will not work and will not be enabled. The touchscreen channels
require special trigger and irq configuration: pen detect, no pen detect
and a periodic trigger to sample the touchscreen position and pressure.
If the user attempts to use another trigger while there is a buffer
that already requested the touchscreen channels (thus the trigger), the
driver will refuse to comply.

In order to have defines for the channel numbers, I added a bindings include
file that goes on a separate commit :
dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer info
This should go in the same tree with the following commits :
  ARM: dts: at91: sama5d2: add channel cells for ADC device
  ARM: dts: at91: sama5d2: Add resistive touch device

as build will break because these commits depend on the binding one
which creates the included header file.

Changes in v3:
 - changed input driver name according to feedback and reworked in commits
to adapt to binding changes and new name.
 - moved channel index fix in at91-sama5d2_adc at the beginning of the series
(PATCH 01/11)
 - created a new optional binding for the touchscreen as a separate commit
and added it to the series :
 [PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure
 threshold touchscreen property
 - changed at91-sama5d2_adc driver patch to address the comments. Exact changes
are in the patch file for the driver source file.


Eugen Hristev (11):
  iio: adc: at91-sama5d2_adc: fix channel configuration for differential
    channels
  MAINTAINERS: add generic resistive touchscreen adc
  iio: Add channel for Position Relative
  dt-bindings: input: touchscreen: add pressure threshold touchscreen
    property
  dt-bindings: input: touchscreen: resistive-adc-touch: create bindings
  iio: inkern: add module put/get on iio dev module when requesting
    channels
  iio: adc: at91-sama5d2_adc: add support for position and pressure
    channels
  input: touchscreen: resistive-adc-touch: add generic resistive ADC
    touchscreen
  dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer
    info
  ARM: dts: at91: sama5d2: add channel cells for ADC device
  ARM: dts: at91: sama5d2: Add resistive touch device

 Documentation/ABI/testing/sysfs-bus-iio            |  12 +
 .../bindings/iio/adc/at91-sama5d2_adc.txt          |   9 +
 .../input/touchscreen/resistive-adc-touch.txt      |  30 +
 .../bindings/input/touchscreen/touchscreen.txt     |   3 +
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/sama5d2.dtsi                     |  12 +
 drivers/iio/adc/at91-sama5d2_adc.c                 | 646 ++++++++++++++++++---
 drivers/iio/industrialio-core.c                    |   1 +
 drivers/iio/inkern.c                               |   8 +-
 drivers/input/touchscreen/Kconfig                  |  13 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/resistive-adc-touch.c    | 200 +++++++
 include/dt-bindings/iio/adc/at91-sama5d2_adc.h     |  16 +
 include/uapi/linux/iio/types.h                     |   1 +
 tools/iio/iio_event_monitor.c                      |   2 +
 15 files changed, 891 insertions(+), 69 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt
 create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
 create mode 100644 include/dt-bindings/iio/adc/at91-sama5d2_adc.h

-- 
2.7.4

[PATCH v3 01/11] iio: adc: at91-sama5d2_adc: fix channel configuration for differential channels

[Eugen Hristev]

When iterating through the channels, the index in the array is not the
scan index. Added an xlate function to translate to the proper index.
The result of the bug is that the channel array is indexed with a wrong index,
thus instead of the proper channel, we access invalid memory, which may
lead to invalid results and/or corruption.
This will be used also for devicetree channel xlate.

Fixes: 5e1a1da0f ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
Fixes: 073c66201 ("iio: adc: at91-sama5d2_adc: add support for DMA")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 41 ++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4eff835..8729d65 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -333,6 +333,27 @@ static const struct iio_chan_spec at91_adc_channels[] = {
 				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
 };
 
+static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
+{
+	int i;
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		if (indio_dev->channels[i].scan_index == chan)
+			return i;
+	}
+	return -EINVAL;
+}
+
+static inline struct iio_chan_spec const *
+at91_adc_chan_get(struct iio_dev *indio_dev, int chan)
+{
+	int index = at91_adc_chan_xlate(indio_dev, chan);
+
+	if (index < 0)
+		return NULL;
+	return indio_dev->channels + index;
+}
+
 static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
@@ -350,8 +371,10 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
 
 	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
-		struct iio_chan_spec const *chan = indio->channels + bit;
+		struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
 
+		if (!chan)
+			continue;
 		if (state) {
 			at91_adc_writel(st, AT91_SAMA5D2_CHER,
 					BIT(chan->channel));
@@ -448,7 +471,11 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
 
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
 			 indio_dev->num_channels) {
-		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+		struct iio_chan_spec const *chan =
+					 at91_adc_chan_get(indio_dev, bit);
+
+		if (!chan)
+			continue;
 
 		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
 	}
@@ -526,8 +553,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
 	 */
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
 			 indio_dev->num_channels) {
-		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+		struct iio_chan_spec const *chan =
+					at91_adc_chan_get(indio_dev, bit);
 
+		if (!chan)
+			continue;
 		if (st->dma_st.dma_chan)
 			at91_adc_readl(st, chan->address);
 	}
@@ -587,8 +617,11 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
 
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
 			 indio_dev->num_channels) {
-		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+		struct iio_chan_spec const *chan =
+					at91_adc_chan_get(indio_dev, bit);
 
+		if (!chan)
+			continue;
 		st->buffer[i] = at91_adc_readl(st, chan->address);
 		i++;
 	}
-- 
2.7.4

[PATCH v3 02/11] MAINTAINERS: add generic resistive touchscreen adc

[Eugen Hristev]

Add MAINTAINERS entry for generic resistive touchscreen adc

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v3:
 - Changed source file name

 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 36f1442..b406e7f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5943,6 +5943,12 @@ F:	drivers/base/power/domain*.c
 F:	include/linux/pm_domain.h
 F:	Documentation/devicetree/bindings/power/power_domain.txt
 
+GENERIC RESISTIVE TOUCHSCREEN ADC DRIVER
+M:	Eugen Hristev <eugen.hristev@microchip.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/input/touchscreen/resistive-adc-touch.c
+
 GENERIC UIO DRIVER FOR PCI DEVICES
 M:	"Michael S. Tsirkin" <mst@redhat.com>
 L:	kvm@vger.kernel.org
-- 
2.7.4

[PATCH v3 03/11] iio: Add channel for Position Relative

[Eugen Hristev]

Add new channel type for relative position on a pad.

These type of analog sensor offers 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 will hand the relative position on the pad in both directions.

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>
---
Changes in v2:
 - modified channel name to relative position as suggested.
 - modified kernel version to 4.18 (presumable)

 Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
 drivers/iio/industrialio-core.c         |  1 +
 include/uapi/linux/iio/types.h          |  1 +
 tools/iio/iio_event_monitor.c           |  2 ++
 4 files changed, 16 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 6a5f34b..42a9287 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -190,6 +190,18 @@ 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_positionrelative_x_raw
+What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_y_raw
+KernelVersion:	4.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Relative 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 milli percents
+		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 19bdf3d..14bf3d24 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_POSITIONRELATIVE]  = "positionrelative",
 };
 
 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..033c7d2 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_POSITIONRELATIVE,
 };
 
 enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e..148f69d 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_POSITIONRELATIVE] = "positionrelative",
 };
 
 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_POSITIONRELATIVE:
 		break;
 	default:
 		return false;
-- 
2.7.4

[PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure threshold touchscreen property

[Eugen Hristev]

Add a common touchscreen optional property that will specify
the minimum pressure applied to the screen that is needed
such that the driver will report the touch event.

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

diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
index 537643e..c84047a 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
@@ -7,6 +7,9 @@ Optional properties for Touchscreens:
 				  (in pixels)
  - touchscreen-max-pressure	: maximum reported pressure (arbitrary range
 				  dependent on the controller)
+ - touchscreen-threshold-pressure: minimum pressure on the touchscreen to be
+				  achieved in order for the touchscreen
+				  driver to report a touch event.
  - touchscreen-fuzz-x		: horizontal noise value of the absolute input
 				  device (in pixels)
  - touchscreen-fuzz-y		: vertical noise value of the absolute input
-- 
2.7.4

[PATCH v3 05/11] dt-bindings: input: touchscreen: resistive-adc-touch: create bindings

[Eugen Hristev]

Added bindings for generic resistive touchscreen ADC.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v3:
 - renamed file and compatible to exclude "generic" keyword
 - removed the pressure threshold property, added it as a common
touchscreen property in the touchscreen common bindings in a separate
commit.

Changes in v2:
 - modified bindings to have a generic resistive touchscreen adc driver
instead of specific architecture one.

 .../input/touchscreen/resistive-adc-touch.txt      | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt b/Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt
new file mode 100644
index 0000000..1c4e4a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt
@@ -0,0 +1,30 @@
+Generic resistive touchscreen ADC
+
+Required properties:
+
+ - compatible: must be "resistive-adc-touch"
+The device must be connected to an ADC device that provides channels for
+position measurement and optional pressure.
+Refer to ../iio/iio-bindings.txt for details
+ - iio-channels: must have at least two channels connected to an ADC device.
+These should correspond to the channels exposed by the ADC device and should
+have the right index as the ADC device registers them. These channels
+represent the relative position on the "x" and "y" axes.
+ - iio-channel-names: must have all the channels' names. Mandatory channels
+are "x" and "y".
+
+Optional properties:
+ - iio-channels: The third channel named "pressure" is optional and can be
+used if the ADC device also measures pressure besides position.
+If this channel is missing, pressure will be ignored and the touchscreen
+will only report position.
+ - iio-channel-names: optional channel named "pressure".
+
+Example:
+
+	resistive_touch: resistive_touch {
+		compatible = "resistive-adc-touch";
+		touchscreen-threshold-pressure = <50000>;
+		io-channels = <&adc 24>, <&adc 25>, <&adc 26>;
+		io-channel-names = "x", "y", "pressure";
+	};
-- 
2.7.4

[PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Eugen Hristev]

When requesting channels for a particular consumer device,
besides requesting the device (incrementing the reference counter), also
do it for the driver module of the iio dev. This will avoid the situation
where the producer IIO device can be removed and the consumer is still
present in the kernel.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/inkern.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index ec98790..68d9b87 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/module.h>
 
 #include <linux/iio/iio.h>
 #include "iio_core.h"
@@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
 	if (index < 0)
 		goto err_put;
 	channel->channel = &indio_dev->channels[index];
+	try_module_get(channel->indio_dev->driver_module);
 
 	return 0;
 
@@ -256,8 +258,10 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
 	return chans;
 
 error_free_chans:
-	for (i = 0; i < mapind; i++)
+	for (i = 0; i < mapind; i++) {
+		module_put(chans[i].indio_dev->driver_module);
 		iio_device_put(chans[i].indio_dev);
+	}
 	kfree(chans);
 	return ERR_PTR(ret);
 }
@@ -351,6 +355,7 @@ void iio_channel_release(struct iio_channel *channel)
 {
 	if (!channel)
 		return;
+	module_put(channel->indio_dev->driver_module);
 	iio_device_put(channel->indio_dev);
 	kfree(channel);
 }
@@ -482,6 +487,7 @@ void iio_channel_release_all(struct iio_channel *channels)
 	struct iio_channel *chan = &channels[0];
 
 	while (chan->indio_dev) {
+		module_put(chan->indio_dev->driver_module);
 		iio_device_put(chan->indio_dev);
 		chan++;
 	}
-- 
2.7.4

[PATCH v3 07/11] iio: adc: at91-sama5d2_adc: add support for position and pressure channels

[Eugen Hristev]

This implements the support for position and pressure for the included
touchscreen support in the SAMA5D2 SOC ADC block.
Two position channels are added and one for pressure.
They can be read in raw format, or through a buffer.
A normal use case is for a consumer driver to register a callback buffer
for these channels.
When the touchscreen channels are in the active scan mask,
the driver will start the touchscreen sampling and push the data to the
buffer.

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>
---
Changes in v3:
 - prefix macros with AT91_SAMA5D2
 - reworked the x_pos and y_pos functions into a single one with two
additional wrappers
 - reworked pressure report to have it grow naturally and not top down
 - fixed some checks regarding IIO_VOLTAGE as suggested
 - added a comment explaining some code in trigger handling
 - reworked the frequency get handler to use the saved value instead of
reading it from the hardware.
 - added comment on deffered work queueing
 - pulled out INFO_RAW function into a separate utility function as suggested
 - added iio_dev ops structure at all times . The functions are needed in
case we do not have a hardware trigger attached, but we want to use the
consumer touchscreen driver, thus a callback buffer is attached. Then we still
need to have buffer preenable and postdisable to configure the touch IRQs (etc.)

Changes in v2:
 - the support is now based on callback buffer.
 drivers/iio/adc/at91-sama5d2_adc.c | 601 +++++++++++++++++++++++++++++++++----
 1 file changed, 539 insertions(+), 62 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 8729d65..5368464 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,38 @@
 #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(1, 0)
+
 /* 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             GENMASK(5, 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           GENMASK(11, 8)
+/* Touchscreen Mode Register - Touch/trigger frequency ratio */
+#define AT91_SAMA5D2_TSMR_TSFREQ(x)             ((x) << 8)
+/* Touchscreen Mode Register - Pen Debounce Time mask */
+#define AT91_SAMA5D2_TSMR_PENDBC_MASK           GENMASK(31, 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,6 +193,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           GENMASK(31, 16)
+/* Trigger Mode - trigger period */
+#define AT91_SAMA5D2_TRGR_TRGPER(x)             ((x) << 16)
 
 /* Correction Select Register */
 #define AT91_SAMA5D2_COSR	0xd0
@@ -169,6 +217,22 @@
 #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_SINGLE_CHAN_CNT + \
+					 AT91_SAMA5D2_DIFF_CHAN_CNT * 2)
+#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 AT91_SAMA5D2_MAX_CHAN_IDX	AT91_SAMA5D2_TOUCH_P_CHAN_IDX
+
+#define AT91_SAMA5D2_TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */
+#define AT91_SAMA5D2_TOUCH_PEN_DETECT_DEBOUNCE_US    200
+
+#define AT91_SAMA5D2_XYZ_MASK		GENMASK(11, 0)
+
+#define AT91_SAMA5D2_MAX_POS_BITS			12
+
 /*
  * Maximum number of bytes to hold conversion from all channels
  * without the timestamp.
@@ -222,6 +286,37 @@
 		.indexed = 1,						\
 	}
 
+#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)				\
+	{								\
+		.type = IIO_POSITIONRELATIVE,				\
+		.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)
 
@@ -260,6 +355,22 @@ struct at91_adc_dma {
 	s64				dma_ts;
 };
 
+/**
+ * at91_adc_touch - at91-sama5d2 touchscreen information struct
+ * @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
+ * @channels_bitmask:		bitmask with the touchscreen channels enabled
+ * @workq:			workqueue for buffer data pushing
+ */
+struct at91_adc_touch {
+	u16				sample_period_val;
+	bool				touching;
+	u16				x_pos;
+	unsigned long			channels_bitmask;
+	struct work_struct		workq;
+};
+
 struct at91_adc_state {
 	void __iomem			*base;
 	int				irq;
@@ -267,6 +378,7 @@ 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;
@@ -275,6 +387,7 @@ struct at91_adc_state {
 	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
@@ -329,8 +442,10 @@ 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_chan_xlate(struct iio_dev *indio_dev, int chan)
@@ -354,6 +469,160 @@ at91_adc_chan_get(struct iio_dev *indio_dev, int chan)
 	return indio_dev->channels + index;
 }
 
+static inline int at91_adc_of_xlate(struct iio_dev *indio_dev,
+				    const struct of_phandle_args *iiospec)
+{
+	return at91_adc_chan_xlate(indio_dev, iiospec->args[0]);
+}
+
+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(AT91_SAMA5D2_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(2) & 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(2) & 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((AT91_SAMA5D2_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 u16 at91_adc_touch_pos(struct at91_adc_state *st, int reg)
+{
+	u32 val;
+	u32 scale, result, pos;
+
+	/*
+	 * to obtain the actual position we must divide by scale
+	 * and multiply with max, where
+	 * max = 2^AT91_SAMA5D2_MAX_POS_BITS - 1
+	 */
+	/* first half of register is the x or y, second half is the scale */
+	val = at91_adc_readl(st, reg);
+	if (!val)
+		dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
+
+	pos = val & AT91_SAMA5D2_XYZ_MASK;
+	result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
+	scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
+	if (scale == 0) {
+		dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
+		return 0;
+	}
+	result /= scale;
+
+	return result;
+}
+
+static u16 at91_adc_touch_x_pos(struct at91_adc_state *st)
+{
+	st->touch_st.x_pos = at91_adc_touch_pos(st, AT91_SAMA5D2_XPOSR);
+	return st->touch_st.x_pos;
+}
+
+static u16 at91_adc_touch_y_pos(struct at91_adc_state *st)
+{
+	return at91_adc_touch_pos(st, AT91_SAMA5D2_YPOSR);
+}
+
+static u16 at91_adc_touch_pressure(struct at91_adc_state *st)
+{
+	u32 val;
+	u32 z1, z2;
+	u32 pres;
+	u32 rxp = 1;
+	u32 factor = 1000;
+
+	/* calculate the pressure */
+	val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
+	z1 = val & AT91_SAMA5D2_XYZ_MASK;
+	z2 = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
+
+	if (z1 != 0)
+		pres = rxp * (st->touch_st.x_pos * factor / 1024) *
+			(z2 * factor / z1 - factor) /
+			factor;
+	else
+		pres = 0xFFFF;       /* no pen contact */
+
+	/*
+	 * The pressure from device grows down, minimum is 0xFFFF, maximum 0x0.
+	 * We compute it this way, but let's return it in the expected way,
+	 * growing from 0 to 0xFFFF.
+	 */
+	return 0xFFFF - pres;
+}
+
+static int at91_adc_read_position(struct at91_adc_state *st, int chan, u16 *val)
+{
+	*val = 0;
+	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, u16 *val)
+{
+	*val = 0;
+	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_configure_trigger(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
@@ -375,6 +644,10 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 
 		if (!chan)
 			continue;
+		/* these channel types cannot be handled by this trigger */
+		if (chan->type == IIO_POSITIONRELATIVE ||
+		    chan->type == IIO_PRESSURE)
+			continue;
 		if (state) {
 			at91_adc_writel(st, AT91_SAMA5D2_CHER,
 					BIT(chan->channel));
@@ -520,7 +793,21 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
 static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
 {
 	int ret;
+	struct at91_adc_state *st = iio_priv(indio_dev);
 
+	/* check if we are enabling triggered buffer or the touchscreen */
+	if (bitmap_subset(indio_dev->active_scan_mask,
+			  &st->touch_st.channels_bitmask,
+			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
+		/* touchscreen enabling */
+		at91_adc_configure_touch(st, true);
+		return 0;
+	}
+	/* if trigger is not hardware, nothing to do here */
+	if (!st->selected_trig->hw_trig)
+		return 0;
+
+	/* we continue with the triggered buffer */
 	ret = at91_adc_dma_start(indio_dev);
 	if (ret) {
 		dev_err(&indio_dev->dev, "buffer postenable failed\n");
@@ -536,6 +823,19 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
 	int ret;
 	u8 bit;
 
+	/* check if we are disabling triggered buffer or the touchscreen */
+	if (bitmap_subset(indio_dev->active_scan_mask,
+			  &st->touch_st.channels_bitmask,
+			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
+		/* touchscreen disable */
+		at91_adc_configure_touch(st, false);
+		return 0;
+	}
+	/* if trigger is not hardware, nothing to do here */
+	if (!st->selected_trig->hw_trig)
+		return 0;
+
+	/* continue with the triggered buffer */
 	ret = iio_triggered_buffer_predisable(indio_dev);
 	if (ret < 0)
 		dev_err(&indio_dev->dev, "buffer predisable failed\n");
@@ -558,6 +858,10 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
 
 		if (!chan)
 			continue;
+		/* these channel types are virtual, no need to do anything */
+		if (chan->type == IIO_POSITIONRELATIVE ||
+		    chan->type == IIO_PRESSURE)
+			continue;
 		if (st->dma_st.dma_chan)
 			at91_adc_readl(st, chan->address);
 	}
@@ -622,7 +926,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
 
 		if (!chan)
 			continue;
-		st->buffer[i] = at91_adc_readl(st, chan->address);
+		/*
+		 * Our external trigger only supports the voltage channels.
+		 * In case someone requested a different type of channel
+		 * just put zeroes to buffer.
+		 * This should not happen because we check the scan mode
+		 * and scan mask when we enable the buffer, and we don't allow
+		 * the buffer to start with a mixed mask (voltage and something
+		 * else).
+		 * Thus, emit a warning.
+		 */
+		if (chan->type == IIO_VOLTAGE) {
+			st->buffer[i] = at91_adc_readl(st, chan->address);
+		} else {
+			st->buffer[i] = 0;
+			WARN(true, "This trigger cannot handle this type of channel");
+		}
 		i++;
 	}
 	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
@@ -688,9 +1007,20 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
 
 static int at91_adc_buffer_init(struct iio_dev *indio)
 {
-	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
+	struct at91_adc_state *st = iio_priv(indio);
+
+	if (st->selected_trig->hw_trig) {
+		return devm_iio_triggered_buffer_setup(&indio->dev, indio,
 			&iio_pollfunc_store_time,
 			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
+	}
+	/*
+	 * we need to prepare the buffer ops in case we will get
+	 * another buffer attached (like a callback buffer for the touchscreen)
+	 */
+	indio->setup_ops = &at91_buffer_setup_ops;
+
+	return 0;
 }
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -736,19 +1066,83 @@ 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)
+static inline unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
 {
-	unsigned f_adc, f_per = clk_get_rate(st->per_clk);
-	unsigned mr, prescal;
+	return st->current_sample_rate;
+}
 
-	mr = at91_adc_readl(st, AT91_SAMA5D2_MR);
-	prescal = (mr >> AT91_SAMA5D2_MR_PRESCAL_OFFSET)
-		  & AT91_SAMA5D2_MR_PRESCAL_MAX;
-	f_adc = f_per / (2 * (prescal + 1));
+static void at91_adc_touch_data_handler(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	u8 bit;
+	u16 val;
+	int i = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 AT91_SAMA5D2_MAX_CHAN_IDX + 1) {
+		struct iio_chan_spec const *chan =
+					 at91_adc_chan_get(indio_dev, bit);
 
-	return f_adc;
+		if (chan->type == IIO_POSITIONRELATIVE)
+			at91_adc_read_position(st, chan->channel, &val);
+		else if (chan->type == IIO_PRESSURE)
+			at91_adc_read_pressure(st, chan->channel, &val);
+		else
+			continue;
+		st->buffer[i] = val;
+		i++;
+	}
+	/*
+	 * Schedule work to push to buffers.
+	 * This is intended to push to the callback buffer that another driver
+	 * registered. We are still in a handler from our IRQ. If we push
+	 * directly, it means the other driver has it's callback called
+	 * from our IRQ context. Which is something we better avoid.
+	 * Let's schedule it after our IRQ is completed.
+	 */
+	schedule_work(&st->touch_st.workq);
+}
+
+static void 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;
+}
+
+static void at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
+{
+	struct iio_dev *indio_dev = iio_priv_to_dev(st);
+
+	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
+			AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
+	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;
+
+	at91_adc_touch_data_handler(indio_dev);
+
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
+}
+
+static void at91_adc_workq_handler(struct work_struct *workq)
+{
+	struct at91_adc_touch *touch_st = container_of(workq,
+					struct at91_adc_touch, workq);
+	struct at91_adc_state *st = container_of(touch_st,
+					struct at91_adc_state, touch_st);
+	struct iio_dev *indio_dev = iio_priv_to_dev(st);
+
+	iio_push_to_buffers(indio_dev, st->buffer);
 }
 
 static irqreturn_t at91_adc_interrupt(int irq, void *private)
@@ -757,17 +1151,39 @@ static irqreturn_t at91_adc_interrupt(int irq, void *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 (status & AT91_SAMA5D2_IER_PEN) {
+		/* pen detected IRQ */
+		at91_adc_pen_detect_interrupt(st);
+	} else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
+		/* nopen detected IRQ */
+		at91_adc_no_pen_detect_interrupt(st);
+	} else if ((status & AT91_SAMA5D2_ISR_PENS) &&
+		   ((status & rdy_mask) == rdy_mask)) {
+		/* periodic trigger IRQ - during pen sense */
+		at91_adc_touch_data_handler(indio);
+	} else if (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) {
+		/* triggered buffer without DMA */
 		disable_irq_nosync(irq);
 		iio_trigger_poll(indio->trig);
 	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
+		/* triggered buffer 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);
@@ -775,58 +1191,81 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-static int at91_adc_read_raw(struct iio_dev *indio_dev,
-			     struct iio_chan_spec const *chan,
-			     int *val, int *val2, long mask)
+static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan, int *val)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	u32 cor = 0;
 	int ret;
 
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		/* we cannot use software trigger if hw trigger enabled */
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
-		mutex_lock(&st->lock);
+	/*
+	 * we cannot use software trigger or touchscreen
+	 * if external trigger is enabled
+	 */
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+	mutex_lock(&st->lock);
 
-		st->chan = chan;
+	if (chan->type == IIO_POSITIONRELATIVE) {
+		ret = at91_adc_read_position(st, chan->channel,
+					     (u16 *)val);
+		goto at91_adc_read_info_raw_exit;
+	}
+	if (chan->type == IIO_PRESSURE) {
+		ret = at91_adc_read_pressure(st, chan->channel,
+					     (u16 *)val);
+		goto at91_adc_read_info_raw_exit;
+	}
 
-		if (chan->differential)
-			cor = (BIT(chan->channel) | BIT(chan->channel2)) <<
-			      AT91_SAMA5D2_COR_DIFF_OFFSET;
-
-		at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
-		at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
-
-		ret = wait_event_interruptible_timeout(st->wq_data_available,
-						       st->conversion_done,
-						       msecs_to_jiffies(1000));
-		if (ret == 0)
-			ret = -ETIMEDOUT;
-
-		if (ret > 0) {
-			*val = st->conversion_value;
-			if (chan->scan_type.sign == 's')
-				*val = sign_extend32(*val, 11);
-			ret = IIO_VAL_INT;
-			st->conversion_done = false;
-		}
+	st->chan = chan;
+
+	if (chan->differential)
+		cor = (BIT(chan->channel) | BIT(chan->channel2)) <<
+		      AT91_SAMA5D2_COR_DIFF_OFFSET;
+
+	at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
+	at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
+
+	ret = wait_event_interruptible_timeout(st->wq_data_available,
+					       st->conversion_done,
+					       msecs_to_jiffies(1000));
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+
+	if (ret > 0) {
+		*val = st->conversion_value;
+		if (chan->scan_type.sign == 's')
+			*val = sign_extend32(*val, 11);
+		ret = IIO_VAL_INT;
+		st->conversion_done = false;
+	}
+
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
 
-		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
+	/* Needed to ACK the DRDY interruption */
+	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
 
-		/* Needed to ACK the DRDY interruption */
-		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+at91_adc_read_info_raw_exit:
 
-		mutex_unlock(&st->lock);
+	mutex_unlock(&st->lock);
 
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
+}
 
+static int at91_adc_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return at91_adc_read_info_raw(indio_dev, chan, val);
 	case IIO_CHAN_INFO_SCALE:
 		*val = st->vref_uv / 1000;
 		if (chan->differential)
@@ -974,9 +1413,29 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
 	return 0;
 }
 
+static int at91_adc_update_scan_mode(struct iio_dev *indio_dev,
+				     const unsigned long *scan_mask)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	if (bitmap_subset(scan_mask, &st->touch_st.channels_bitmask,
+			  AT91_SAMA5D2_MAX_CHAN_IDX + 1))
+		return 0;
+	/*
+	 * if the new bitmap is a combination of touchscreen and regular
+	 * channels, then we are not fine
+	 */
+	if (bitmap_intersects(&st->touch_st.channels_bitmask, scan_mask,
+			      AT91_SAMA5D2_MAX_CHAN_IDX + 1))
+		return -EBUSY;
+	return 0;
+}
+
 static const struct iio_info at91_adc_info = {
 	.read_raw = &at91_adc_read_raw,
 	.write_raw = &at91_adc_write_raw,
+	.update_scan_mode = &at91_adc_update_scan_mode,
+	.of_xlate = &at91_adc_of_xlate,
 	.hwfifo_set_watermark = &at91_adc_set_watermark,
 };
 
@@ -1044,13 +1503,20 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
 	indio_dev->info = &at91_adc_info;
 	indio_dev->channels = at91_adc_channels;
 	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
 
 	st = iio_priv(indio_dev);
 
+	bitmap_set(&st->touch_st.channels_bitmask,
+		   AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
+	bitmap_set(&st->touch_st.channels_bitmask,
+		   AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, 1);
+	bitmap_set(&st->touch_st.channels_bitmask,
+		   AT91_SAMA5D2_TOUCH_P_CHAN_IDX, 1);
+
 	ret = of_property_read_u32(pdev->dev.of_node,
 				   "atmel,min-sample-rate-hz",
 				   &st->soc_info.min_sample_rate);
@@ -1100,6 +1566,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	init_waitqueue_head(&st->wq_data_available);
 	mutex_init(&st->lock);
+	INIT_WORK(&st->touch_st.workq, at91_adc_workq_handler);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
@@ -1159,13 +1626,13 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, indio_dev);
 
-	if (st->selected_trig->hw_trig) {
-		ret = at91_adc_buffer_init(indio_dev);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
-			goto per_clk_disable_unprepare;
-		}
+	ret = at91_adc_buffer_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
+		goto per_clk_disable_unprepare;
+	}
 
+	if (st->selected_trig->hw_trig) {
 		ret = at91_adc_trigger_init(indio_dev);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "couldn't setup the triggers.\n");
@@ -1272,8 +1739,18 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
 	at91_adc_hw_init(st);
 
 	/* reconfiguring trigger hardware state */
-	if (iio_buffer_enabled(indio_dev))
+	if (!iio_buffer_enabled(indio_dev))
+		return 0;
+
+	/* check if we are enabling triggered buffer or the touchscreen */
+	if (bitmap_subset(indio_dev->active_scan_mask,
+			  &st->touch_st.channels_bitmask,
+			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
+		/* touchscreen enabling */
+		at91_adc_configure_touch(st, true);
+	} else {
 		at91_adc_configure_trigger(st->trig, true);
+	}
 
 	return 0;
 
-- 
2.7.4

[PATCH v3 08/11] input: touchscreen: resistive-adc-touch: add generic resistive ADC touchscreen

[Eugen Hristev]

This adds a generic resistive touchscreen (GRTS) driver, which is based
on an IIO device (an ADC). It must be connected to the channels of an ADC
to receive touch data. Then it will feed the data into the input subsystem
where it registers an input device.
It uses an IIO callback buffer to register to the IIO device

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>
---
Changes in v3:
 - pressure now reported naturally growing down-up
 - using helpers from touchscreen.h to parse DT properties
 - added devm_add_action_or_reset to handle callback buffer clean on exit
 - changed compatible and threshold property to adapt to changed bindings

Changes in v2:
 - this is now a generic resistive adc touchscreen driver

 drivers/input/touchscreen/Kconfig               |  13 ++
 drivers/input/touchscreen/Makefile              |   1 +
 drivers/input/touchscreen/resistive-adc-touch.c | 200 ++++++++++++++++++++++++
 3 files changed, 214 insertions(+)
 create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 4f15496..8f85d3a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -92,6 +92,19 @@ config TOUCHSCREEN_AD7879_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7879-spi.
 
+config TOUCHSCREEN_ADC
+	tristate "Generic ADC based resistive touchscreen"
+	depends on IIO
+	select IIO_BUFFER_CB
+	help
+	  Say Y here if you want to use the generic ADC
+	  resistive touchscreen driver.
+
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called resistive-adc-touch.ko.
+
 config TOUCHSCREEN_AR1021_I2C
 	tristate "Microchip AR1020/1021 i2c touchscreen"
 	depends on I2C && OF
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index dddae79..843c7f9 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
 obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
 obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
 obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
+obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
diff --git a/drivers/input/touchscreen/resistive-adc-touch.c b/drivers/input/touchscreen/resistive-adc-touch.c
new file mode 100644
index 0000000..7e201ee
--- /dev/null
+++ b/drivers/input/touchscreen/resistive-adc-touch.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADC generic resistive touchscreen (GRTS)
+ *
+ * Copyright (C) 2017,2018 Microchip Technology,
+ * Author: Eugen Hristev <eugen.hristev@microchip.com>
+ *
+ */
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define DRIVER_NAME					"resistive-adc-touch"
+#define GRTS_DEFAULT_PRESSURE_THRESHOLD			50000
+#define MAX_POS_MASK					GENMASK(11, 0)
+
+/**
+ * grts_state - generic resistive touch screen information struct
+ * @pressure_threshold:	number representing the threshold for the pressure
+ * @pressure:		are we getting pressure info or not
+ * @iio_chans:		list of channels acquired
+ * @iio_cb:		iio_callback buffer for the data
+ * @input:		the input device structure that we register
+ * @prop:		touchscreen properties struct
+ */
+struct grts_state {
+	u32				pressure_threshold;
+	bool				pressure;
+	struct iio_channel		*iio_chans;
+	struct iio_cb_buffer		*iio_cb;
+	struct input_dev		*input;
+	struct touchscreen_properties	prop;
+};
+
+static int grts_cb(const void *data, void *private)
+{
+	const u16 *touch_info = data;
+	struct grts_state *st = private;
+
+	unsigned int x, y, press = 0xFFFF;
+
+	/* channel data coming in buffer in the order below */
+	x = touch_info[0];
+	y = touch_info[1];
+	if (st->pressure)
+		press = touch_info[2];
+
+	if ((!x && !y) || (st->pressure && (press < st->pressure_threshold))) {
+		/* report end of touch */
+		input_report_key(st->input, BTN_TOUCH, 0);
+		input_sync(st->input);
+		return 0;
+	}
+
+	/* report proper touch to subsystem*/
+	touchscreen_report_pos(st->input, &st->prop, x, y, false);
+	if (st->pressure)
+		input_report_abs(st->input, ABS_PRESSURE, press);
+	input_report_key(st->input, BTN_TOUCH, 1);
+	input_sync(st->input);
+
+	return 0;
+}
+
+static int grts_open(struct input_dev *dev)
+{
+	int error;
+	struct grts_state *st = input_get_drvdata(dev);
+
+	error = iio_channel_start_all_cb(st->iio_cb);
+	if (error) {
+		dev_err(dev->dev.parent, "failed to start callback buffer.\n");
+		return error;
+	}
+	return 0;
+}
+
+static void grts_close(struct input_dev *dev)
+{
+	struct grts_state *st = input_get_drvdata(dev);
+
+	iio_channel_stop_all_cb(st->iio_cb);
+}
+
+static void grts_disable(void *data)
+{
+	iio_channel_release_all_cb(data);
+}
+
+static int grts_probe(struct platform_device *pdev)
+{
+	struct grts_state *st;
+	struct input_dev *input;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct iio_channel *chan;
+	int ret = 0;
+
+	st = devm_kzalloc(dev, sizeof(struct grts_state), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(node, "touchscreen-threshold-pressure",
+				   &st->pressure_threshold);
+	if (ret < 0) {
+		dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
+		st->pressure_threshold = GRTS_DEFAULT_PRESSURE_THRESHOLD;
+	}
+
+	/* get the channels from IIO device */
+	st->iio_chans = devm_iio_channel_get_all(dev);
+
+	if (IS_ERR(st->iio_chans)) {
+		if (PTR_ERR(st->iio_chans) != -EPROBE_DEFER)
+			dev_err(dev, "can't get iio channels.\n");
+		return PTR_ERR(st->iio_chans);
+	}
+
+	chan = &st->iio_chans[0];
+	st->pressure = false;
+	while (chan && chan->indio_dev) {
+		if (!strcmp(chan->channel->datasheet_name, "pressure"))
+			st->pressure = true;
+		chan++;
+	}
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "failed to allocate input device.\n");
+		return -ENOMEM;
+	}
+
+	input->name = DRIVER_NAME;
+	input->id.bustype = BUS_HOST;
+	input->open = grts_open;
+	input->close = grts_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 - 1, 0, 0);
+	if (st->pressure)
+		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);
+
+	/* parse optional device tree properties */
+	touchscreen_parse_properties(input, false, &st->prop);
+
+	st->input = input;
+	input_set_drvdata(input, st);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "failed to register input device.");
+		return ret;
+	}
+
+	st->iio_cb = iio_channel_get_all_cb(dev, grts_cb, st);
+
+	if (IS_ERR(st->iio_cb)) {
+		dev_err(dev, "failed to allocate callback buffer.\n");
+		ret =  PTR_ERR(st->iio_cb);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, grts_disable, st->iio_cb);
+
+	platform_set_drvdata(pdev, st);
+
+	return ret;
+}
+
+static const struct of_device_id grts_of_match[] = {
+	{
+		.compatible = "resistive-adc-touch",
+	}, {
+		/* sentinel */
+	},
+};
+
+MODULE_DEVICE_TABLE(of, grts_of_match);
+
+static struct platform_driver grts_driver = {
+	.probe = grts_probe,
+	.driver = {
+		   .name = DRIVER_NAME,
+		   .of_match_table = of_match_ptr(grts_of_match),
+	},
+};
+
+module_platform_driver(grts_driver);
+
+MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
+MODULE_DESCRIPTION("Generic ADC Resistive Touch Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v3 09/11] dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer info

[Eugen Hristev]

Added defines for channel consumer device-tree binding

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/iio/adc/at91-sama5d2_adc.txt     |  9 +++++++++
 include/dt-bindings/iio/adc/at91-sama5d2_adc.h           | 16 ++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 include/dt-bindings/iio/adc/at91-sama5d2_adc.h

diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
index 6469a4c..4a3c1d4 100644
--- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
@@ -21,6 +21,14 @@ Optional properties:
   - dmas: Phandle to dma channel for the ADC.
   - dma-names: Must be "rx" when dmas property is being used.
   See ../../dma/dma.txt for details.
+  - #io-channel-cells: in case consumer drivers are attached, this must be 1.
+  See <Documentation/devicetree/bindings/iio/iio-bindings.txt> for details.
+
+Properties for consumer drivers:
+  - Consumer drivers can be connected to this producer device, as specified
+  in <Documentation/devicetree/bindings/iio/iio-bindings.txt>
+  - Channels exposed are specified in:
+  <dt-bindings/iio/adc/at91-sama5d2_adc.txt>
 
 Example:
 
@@ -38,4 +46,5 @@ adc: adc@fc030000 {
 	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
 	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
 	dma-names = "rx";
+	#io-channel-cells = <1>;
 }
diff --git a/include/dt-bindings/iio/adc/at91-sama5d2_adc.h b/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
new file mode 100644
index 0000000..70f99db
--- /dev/null
+++ b/include/dt-bindings/iio/adc/at91-sama5d2_adc.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides constants for configuring the AT91 SAMA5D2 ADC
+ */
+
+#ifndef _DT_BINDINGS_IIO_ADC_AT91_SAMA5D2_ADC_H
+#define _DT_BINDINGS_IIO_ADC_AT91_SAMA5D2_ADC_H
+
+/* X relative position channel index */
+#define AT91_SAMA5D2_ADC_X_CHANNEL		24
+/* Y relative position channel index */
+#define AT91_SAMA5D2_ADC_Y_CHANNEL		25
+/* pressure channel index */
+#define AT91_SAMA5D2_ADC_P_CHANNEL		26
+
+#endif
-- 
2.7.4

[PATCH v3 10/11] ARM: dts: at91: sama5d2: add channel cells for ADC device

[Eugen Hristev]

Preparing the ADC device to connect channel consumer drivers

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

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index 61f68e5..f06ba99 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -47,6 +47,7 @@
 #include <dt-bindings/dma/at91.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/at91.h>
+#include <dt-bindings/iio/adc/at91-sama5d2_adc.h>
 
 / {
 	model = "Atmel SAMA5D2 family SoC";
@@ -1437,6 +1438,7 @@
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
 				atmel,trigger-edge-type = <IRQ_TYPE_EDGE_RISING>;
+				#io-channel-cells = <1>;
 				status = "disabled";
 			};
 
-- 
2.7.4

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

[Eugen Hristev]

Add generic resistive touch device which is connected to ADC block
inside the SAMA5D2 SoC

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v3:
 - modified compatible and threshold to adapt to changed bindings

Changes in v2:
 - now added a generic resistive touchscreen node instead of architecture
specific
 arch/arm/boot/dts/sama5d2.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index f06ba99..ce301dc 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1442,6 +1442,16 @@
 				status = "disabled";
 			};
 
+			resistive_touch: resistive-touch {
+				compatible = "resistive-adc-touch";
+				io-channels = <&adc AT91_SAMA5D2_ADC_X_CHANNEL>,
+					      <&adc AT91_SAMA5D2_ADC_Y_CHANNEL>,
+					      <&adc AT91_SAMA5D2_ADC_P_CHANNEL>;
+				io-channel-names = "x", "y", "pressure";
+				touchscreen-threshold-pressure = <50000>;
+				status = "disabled";
+			};
+
 			pioA: pinctrl@fc038000 {
 				compatible = "atmel,sama5d2-pinctrl";
 				reg = <0xfc038000 0x600>;
-- 
2.7.4

Re: [PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure threshold touchscreen property

[Rob Herring]

On Tue, Apr 10, 2018 at 11:57:50AM +0300, Eugen Hristev wrote:
> Add a common touchscreen optional property that will specify
> the minimum pressure applied to the screen that is needed
> such that the driver will report the touch event.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 05/11] dt-bindings: input: touchscreen: resistive-adc-touch: create bindings

[Rob Herring]

On Tue, Apr 10, 2018 at 11:57:51AM +0300, Eugen Hristev wrote:
> Added bindings for generic resistive touchscreen ADC.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> Changes in v3:
>  - renamed file and compatible to exclude "generic" keyword
>  - removed the pressure threshold property, added it as a common
> touchscreen property in the touchscreen common bindings in a separate
> commit.
> 
> Changes in v2:
>  - modified bindings to have a generic resistive touchscreen adc driver
> instead of specific architecture one.
> 
>  .../input/touchscreen/resistive-adc-touch.txt      | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 01/11] iio: adc: at91-sama5d2_adc: fix channel configuration for differential channels

[Jonathan Cameron]

On Tue, 10 Apr 2018 11:57:47 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> When iterating through the channels, the index in the array is not the
> scan index. Added an xlate function to translate to the proper index.
> The result of the bug is that the channel array is indexed with a wrong index,
> thus instead of the proper channel, we access invalid memory, which may
> lead to invalid results and/or corruption.
> This will be used also for devicetree channel xlate.
> 
> Fixes: 5e1a1da0f ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
> Fixes: 073c66201 ("iio: adc: at91-sama5d2_adc: add support for DMA")
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
This fix obviously stands on it's own so I have applied it to the
fixes-togreg branch of iio.git and will send out a pull request with it in
after the merge window is closed and rc1 is out.

Marked for stable.

Note this will delay the rest of the series being applied but that is no
bad thing as I would like a little more time for people to consider it anyway
and we are obviously in no rush given the time in the cycle!

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 41 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4eff835..8729d65 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -333,6 +333,27 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>  				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
>  };
>  
> +static int at91_adc_chan_xlate(struct iio_dev *indio_dev, int chan)
> +{
> +	int i;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		if (indio_dev->channels[i].scan_index == chan)
> +			return i;
> +	}
> +	return -EINVAL;
> +}
> +
> +static inline struct iio_chan_spec const *
> +at91_adc_chan_get(struct iio_dev *indio_dev, int chan)
> +{
> +	int index = at91_adc_chan_xlate(indio_dev, chan);
> +
> +	if (index < 0)
> +		return NULL;
> +	return indio_dev->channels + index;
> +}
> +
>  static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> @@ -350,8 +371,10 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>  
>  	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> -		struct iio_chan_spec const *chan = indio->channels + bit;
> +		struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
>  
> +		if (!chan)
> +			continue;
>  		if (state) {
>  			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>  					BIT(chan->channel));
> @@ -448,7 +471,11 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>  
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
>  			 indio_dev->num_channels) {
> -		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +		struct iio_chan_spec const *chan =
> +					 at91_adc_chan_get(indio_dev, bit);
> +
> +		if (!chan)
> +			continue;
>  
>  		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
>  	}
> @@ -526,8 +553,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>  	 */
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
>  			 indio_dev->num_channels) {
> -		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +		struct iio_chan_spec const *chan =
> +					at91_adc_chan_get(indio_dev, bit);
>  
> +		if (!chan)
> +			continue;
>  		if (st->dma_st.dma_chan)
>  			at91_adc_readl(st, chan->address);
>  	}
> @@ -587,8 +617,11 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
>  
>  	for_each_set_bit(bit, indio_dev->active_scan_mask,
>  			 indio_dev->num_channels) {
> -		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +		struct iio_chan_spec const *chan =
> +					at91_adc_chan_get(indio_dev, bit);
>  
> +		if (!chan)
> +			continue;
>  		st->buffer[i] = at91_adc_readl(st, chan->address);
>  		i++;
>  	}

Re: [PATCH v3 03/11] iio: Add channel for Position Relative

[Jonathan Cameron]

On Tue, 10 Apr 2018 11:57:49 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Add new channel type for relative position on a pad.
> 
> These type of analog sensor offers 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 will hand the relative position on the pad in both directions.
> 
> 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>
> ---
> Changes in v2:
>  - modified channel name to relative position as suggested.
>  - modified kernel version to 4.18 (presumable)
> 
>  Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
>  drivers/iio/industrialio-core.c         |  1 +
>  include/uapi/linux/iio/types.h          |  1 +
>  tools/iio/iio_event_monitor.c           |  2 ++
>  4 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 6a5f34b..42a9287 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -190,6 +190,18 @@ 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_positionrelative_x_raw
> +What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_y_raw
> +KernelVersion:	4.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Relative 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 milli percents
> +		from the pad's size in both directions. Should be calibrated by
> +		the consumer.
I know the milli percent comes form the humidity equivalent, but I wonder
if we are right to follow that.  10^-5 is a pretty random base unit (though
I got argued into it being a standard choice for humidity sensors IIRC...

What do people think?  We could go with 1 for full range or just percent perhaps?

I'm not that fussed about staying consistent with humidity - we are unlikely
to end up with sensors doing both anytime soon so there shouldn't be
any confusion...

Jonathan
> +
>  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 19bdf3d..14bf3d24 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_POSITIONRELATIVE]  = "positionrelative",
>  };
>  
>  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..033c7d2 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_POSITIONRELATIVE,
>  };
>  
>  enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index b61245e..148f69d 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_POSITIONRELATIVE] = "positionrelative",
>  };
>  
>  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_POSITIONRELATIVE:
>  		break;
>  	default:
>  		return false;

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Jonathan Cameron]

On Tue, 10 Apr 2018 11:57:52 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> When requesting channels for a particular consumer device,
> besides requesting the device (incrementing the reference counter), also
> do it for the driver module of the iio dev. This will avoid the situation
> where the producer IIO device can be removed and the consumer is still
> present in the kernel.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/iio/inkern.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index ec98790..68d9b87 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> +#include <linux/module.h>
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>  	if (index < 0)
>  		goto err_put;
>  	channel->channel = &indio_dev->channels[index];
> +	try_module_get(channel->indio_dev->driver_module);

And if it fails? (the module we are trying to get is going away...)

We should try and handle it I think. Be it by just erroring out of here.


>  
>  	return 0;
>  
> @@ -256,8 +258,10 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  	return chans;
>  
>  error_free_chans:
> -	for (i = 0; i < mapind; i++)
> +	for (i = 0; i < mapind; i++) {
> +		module_put(chans[i].indio_dev->driver_module);
>  		iio_device_put(chans[i].indio_dev);
> +	}
>  	kfree(chans);
>  	return ERR_PTR(ret);
>  }
> @@ -351,6 +355,7 @@ void iio_channel_release(struct iio_channel *channel)
>  {
>  	if (!channel)
>  		return;
> +	module_put(channel->indio_dev->driver_module);
>  	iio_device_put(channel->indio_dev);
>  	kfree(channel);
>  }
> @@ -482,6 +487,7 @@ void iio_channel_release_all(struct iio_channel *channels)
>  	struct iio_channel *chan = &channels[0];
>  
>  	while (chan->indio_dev) {
> +		module_put(chan->indio_dev->driver_module);
>  		iio_device_put(chan->indio_dev);
>  		chan++;
>  	}

Re: [PATCH v3 07/11] iio: adc: at91-sama5d2_adc: add support for position and pressure channels

[Jonathan Cameron]

On Tue, 10 Apr 2018 11:57:53 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> This implements the support for position and pressure for the included
> touchscreen support in the SAMA5D2 SOC ADC block.
> Two position channels are added and one for pressure.
> They can be read in raw format, or through a buffer.
> A normal use case is for a consumer driver to register a callback buffer
> for these channels.
> When the touchscreen channels are in the active scan mask,
> the driver will start the touchscreen sampling and push the data to the
> buffer.
> 
> 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>

A few additional minor things I picked up on in this read.
Just trivial tidy ups or things I think could make the code
a little easier to read.

Looks good in general.

Jonathan

> ---
> Changes in v3:
>  - prefix macros with AT91_SAMA5D2
>  - reworked the x_pos and y_pos functions into a single one with two
> additional wrappers
>  - reworked pressure report to have it grow naturally and not top down
>  - fixed some checks regarding IIO_VOLTAGE as suggested
>  - added a comment explaining some code in trigger handling
>  - reworked the frequency get handler to use the saved value instead of
> reading it from the hardware.
>  - added comment on deffered work queueing
>  - pulled out INFO_RAW function into a separate utility function as suggested
>  - added iio_dev ops structure at all times . The functions are needed in
> case we do not have a hardware trigger attached, but we want to use the
> consumer touchscreen driver, thus a callback buffer is attached. Then we still
> need to have buffer preenable and postdisable to configure the touch IRQs (etc.)
> 
> Changes in v2:
>  - the support is now based on callback buffer.
>  drivers/iio/adc/at91-sama5d2_adc.c | 601 +++++++++++++++++++++++++++++++++----
>  1 file changed, 539 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 8729d65..5368464 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,38 @@
>  #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(1, 0)
> +
>  /* 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             GENMASK(5, 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           GENMASK(11, 8)
> +/* Touchscreen Mode Register - Touch/trigger frequency ratio */
> +#define AT91_SAMA5D2_TSMR_TSFREQ(x)             ((x) << 8)
> +/* Touchscreen Mode Register - Pen Debounce Time mask */
> +#define AT91_SAMA5D2_TSMR_PENDBC_MASK           GENMASK(31, 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,6 +193,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           GENMASK(31, 16)
> +/* Trigger Mode - trigger period */
> +#define AT91_SAMA5D2_TRGR_TRGPER(x)             ((x) << 16)
>  
>  /* Correction Select Register */
>  #define AT91_SAMA5D2_COSR	0xd0
> @@ -169,6 +217,22 @@
>  #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_SINGLE_CHAN_CNT + \
> +					 AT91_SAMA5D2_DIFF_CHAN_CNT * 2)
> +#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 AT91_SAMA5D2_MAX_CHAN_IDX	AT91_SAMA5D2_TOUCH_P_CHAN_IDX
> +
> +#define AT91_SAMA5D2_TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */
> +#define AT91_SAMA5D2_TOUCH_PEN_DETECT_DEBOUNCE_US    200
> +
> +#define AT91_SAMA5D2_XYZ_MASK		GENMASK(11, 0)
> +
> +#define AT91_SAMA5D2_MAX_POS_BITS			12
> +
>  /*
>   * Maximum number of bytes to hold conversion from all channels
>   * without the timestamp.
> @@ -222,6 +286,37 @@
>  		.indexed = 1,						\
>  	}
>  
> +#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)				\
> +	{								\
> +		.type = IIO_POSITIONRELATIVE,				\
> +		.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)
>  
> @@ -260,6 +355,22 @@ struct at91_adc_dma {
>  	s64				dma_ts;
>  };
>  
> +/**
> + * at91_adc_touch - at91-sama5d2 touchscreen information struct
> + * @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
> + * @channels_bitmask:		bitmask with the touchscreen channels enabled
> + * @workq:			workqueue for buffer data pushing
> + */
> +struct at91_adc_touch {
> +	u16				sample_period_val;
> +	bool				touching;
> +	u16				x_pos;
> +	unsigned long			channels_bitmask;
> +	struct work_struct		workq;
> +};
> +
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
> @@ -267,6 +378,7 @@ 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;
> @@ -275,6 +387,7 @@ struct at91_adc_state {
>  	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
> @@ -329,8 +442,10 @@ 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_chan_xlate(struct iio_dev *indio_dev, int chan)
> @@ -354,6 +469,160 @@ at91_adc_chan_get(struct iio_dev *indio_dev, int chan)
>  	return indio_dev->channels + index;
>  }
>  
> +static inline int at91_adc_of_xlate(struct iio_dev *indio_dev,
> +				    const struct of_phandle_args *iiospec)
> +{
> +	return at91_adc_chan_xlate(indio_dev, iiospec->args[0]);
> +}
> +
> +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(AT91_SAMA5D2_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(2) & 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(2) & 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((AT91_SAMA5D2_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 u16 at91_adc_touch_pos(struct at91_adc_state *st, int reg)
> +{
> +	u32 val;
> +	u32 scale, result, pos;
> +
> +	/*
> +	 * to obtain the actual position we must divide by scale
> +	 * and multiply with max, where
> +	 * max = 2^AT91_SAMA5D2_MAX_POS_BITS - 1
> +	 */
> +	/* first half of register is the x or y, second half is the scale */
> +	val = at91_adc_readl(st, reg);
> +	if (!val)
> +		dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
> +
> +	pos = val & AT91_SAMA5D2_XYZ_MASK;
> +	result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
> +	scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
> +	if (scale == 0) {
> +		dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
> +		return 0;
> +	}
> +	result /= scale;
> +
> +	return result;
> +}
> +
> +static u16 at91_adc_touch_x_pos(struct at91_adc_state *st)
> +{
> +	st->touch_st.x_pos = at91_adc_touch_pos(st, AT91_SAMA5D2_XPOSR);
> +	return st->touch_st.x_pos;
> +}
> +
> +static u16 at91_adc_touch_y_pos(struct at91_adc_state *st)
> +{
> +	return at91_adc_touch_pos(st, AT91_SAMA5D2_YPOSR);
> +}
> +
> +static u16 at91_adc_touch_pressure(struct at91_adc_state *st)
> +{
> +	u32 val;
> +	u32 z1, z2;
> +	u32 pres;
> +	u32 rxp = 1;
> +	u32 factor = 1000;
> +
> +	/* calculate the pressure */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> +	z1 = val & AT91_SAMA5D2_XYZ_MASK;
> +	z2 = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
> +
> +	if (z1 != 0)
> +		pres = rxp * (st->touch_st.x_pos * factor / 1024) *
> +			(z2 * factor / z1 - factor) /
> +			factor;
> +	else
> +		pres = 0xFFFF;       /* no pen contact */
> +
> +	/*
> +	 * The pressure from device grows down, minimum is 0xFFFF, maximum 0x0.
> +	 * We compute it this way, but let's return it in the expected way,
> +	 * growing from 0 to 0xFFFF.
> +	 */
> +	return 0xFFFF - pres;
> +}
> +
> +static int at91_adc_read_position(struct at91_adc_state *st, int chan, u16 *val)
> +{
> +	*val = 0;
> +	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, u16 *val)
> +{
> +	*val = 0;
> +	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_configure_trigger(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> @@ -375,6 +644,10 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  
>  		if (!chan)
>  			continue;
> +		/* these channel types cannot be handled by this trigger */
> +		if (chan->type == IIO_POSITIONRELATIVE ||
> +		    chan->type == IIO_PRESSURE)
> +			continue;

A blank line here might help readability.

>  		if (state) {
>  			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>  					BIT(chan->channel));
> @@ -520,7 +793,21 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>  static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	int ret;
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  
> +	/* check if we are enabling triggered buffer or the touchscreen */
> +	if (bitmap_subset(indio_dev->active_scan_mask,
> +			  &st->touch_st.channels_bitmask,
> +			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
> +		/* touchscreen enabling */
> +		at91_adc_configure_touch(st, true);
> +		return 0;
> +	}
> +	/* if trigger is not hardware, nothing to do here */
> +	if (!st->selected_trig->hw_trig)
> +		return 0;
> +
> +	/* we continue with the triggered buffer */
>  	ret = at91_adc_dma_start(indio_dev);
>  	if (ret) {
>  		dev_err(&indio_dev->dev, "buffer postenable failed\n");
> @@ -536,6 +823,19 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>  	int ret;
>  	u8 bit;
>  
> +	/* check if we are disabling triggered buffer or the touchscreen */
> +	if (bitmap_subset(indio_dev->active_scan_mask,
> +			  &st->touch_st.channels_bitmask,
> +			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
> +		/* touchscreen disable */
> +		at91_adc_configure_touch(st, false);
> +		return 0;
> +	}
> +	/* if trigger is not hardware, nothing to do here */
> +	if (!st->selected_trig->hw_trig)
> +		return 0;
> +
> +	/* continue with the triggered buffer */
>  	ret = iio_triggered_buffer_predisable(indio_dev);
>  	if (ret < 0)
>  		dev_err(&indio_dev->dev, "buffer predisable failed\n");
> @@ -558,6 +858,10 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>  
>  		if (!chan)
>  			continue;
> +		/* these channel types are virtual, no need to do anything */
> +		if (chan->type == IIO_POSITIONRELATIVE ||
> +		    chan->type == IIO_PRESSURE)
> +			continue;
>  		if (st->dma_st.dma_chan)
>  			at91_adc_readl(st, chan->address);
>  	}
> @@ -622,7 +926,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
>  
>  		if (!chan)
>  			continue;
> -		st->buffer[i] = at91_adc_readl(st, chan->address);
> +		/*
> +		 * Our external trigger only supports the voltage channels.
> +		 * In case someone requested a different type of channel
> +		 * just put zeroes to buffer.
> +		 * This should not happen because we check the scan mode
> +		 * and scan mask when we enable the buffer, and we don't allow
> +		 * the buffer to start with a mixed mask (voltage and something
> +		 * else).
> +		 * Thus, emit a warning.
> +		 */
> +		if (chan->type == IIO_VOLTAGE) {
> +			st->buffer[i] = at91_adc_readl(st, chan->address);
> +		} else {
> +			st->buffer[i] = 0;
> +			WARN(true, "This trigger cannot handle this type of channel");
> +		}
>  		i++;
>  	}
>  	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
> @@ -688,9 +1007,20 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>  
>  static int at91_adc_buffer_init(struct iio_dev *indio)
>  {
> -	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> +	struct at91_adc_state *st = iio_priv(indio);
> +
> +	if (st->selected_trig->hw_trig) {
> +		return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>  			&iio_pollfunc_store_time,
>  			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> +	}
> +	/*
> +	 * we need to prepare the buffer ops in case we will get
> +	 * another buffer attached (like a callback buffer for the touchscreen)
> +	 */
> +	indio->setup_ops = &at91_buffer_setup_ops;
> +
> +	return 0;
>  }
>  
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> @@ -736,19 +1066,83 @@ 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)
> +static inline unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
>  {
> -	unsigned f_adc, f_per = clk_get_rate(st->per_clk);
> -	unsigned mr, prescal;
> +	return st->current_sample_rate;
> +}
>  
> -	mr = at91_adc_readl(st, AT91_SAMA5D2_MR);
> -	prescal = (mr >> AT91_SAMA5D2_MR_PRESCAL_OFFSET)
> -		  & AT91_SAMA5D2_MR_PRESCAL_MAX;
> -	f_adc = f_per / (2 * (prescal + 1));
> +static void at91_adc_touch_data_handler(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	u8 bit;
> +	u16 val;
> +	int i = 0;
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 AT91_SAMA5D2_MAX_CHAN_IDX + 1) {
> +		struct iio_chan_spec const *chan =
> +					 at91_adc_chan_get(indio_dev, bit);
>  
> -	return f_adc;
> +		if (chan->type == IIO_POSITIONRELATIVE)
> +			at91_adc_read_position(st, chan->channel, &val);
> +		else if (chan->type == IIO_PRESSURE)
> +			at91_adc_read_pressure(st, chan->channel, &val);
> +		else
> +			continue;
> +		st->buffer[i] = val;
> +		i++;
> +	}
> +	/*
> +	 * Schedule work to push to buffers.
> +	 * This is intended to push to the callback buffer that another driver
> +	 * registered. We are still in a handler from our IRQ. If we push
> +	 * directly, it means the other driver has it's callback called
> +	 * from our IRQ context. Which is something we better avoid.
> +	 * Let's schedule it after our IRQ is completed.
> +	 */
> +	schedule_work(&st->touch_st.workq);
> +}
> +
> +static void 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;
> +}
> +
> +static void at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> +			AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
> +	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;
> +
> +	at91_adc_touch_data_handler(indio_dev);
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +}
> +
> +static void at91_adc_workq_handler(struct work_struct *workq)
> +{
> +	struct at91_adc_touch *touch_st = container_of(workq,
> +					struct at91_adc_touch, workq);
> +	struct at91_adc_state *st = container_of(touch_st,
> +					struct at91_adc_state, touch_st);
> +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +
> +	iio_push_to_buffers(indio_dev, st->buffer);
>  }
>  
>  static irqreturn_t at91_adc_interrupt(int irq, void *private)
> @@ -757,17 +1151,39 @@ static irqreturn_t at91_adc_interrupt(int irq, void *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 (status & AT91_SAMA5D2_IER_PEN) {
> +		/* pen detected IRQ */
> +		at91_adc_pen_detect_interrupt(st);
> +	} else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
> +		/* nopen detected IRQ */
> +		at91_adc_no_pen_detect_interrupt(st);
> +	} else if ((status & AT91_SAMA5D2_ISR_PENS) &&
> +		   ((status & rdy_mask) == rdy_mask)) {
> +		/* periodic trigger IRQ - during pen sense */
> +		at91_adc_touch_data_handler(indio);
> +	} else if (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) {
> +		/* triggered buffer without DMA */
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
>  	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		/* triggered buffer 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);
> @@ -775,58 +1191,81 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> -static int at91_adc_read_raw(struct iio_dev *indio_dev,
> -			     struct iio_chan_spec const *chan,
> -			     int *val, int *val2, long mask)
> +static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  	u32 cor = 0;
>  	int ret;
>  
> -	switch (mask) {
> -	case IIO_CHAN_INFO_RAW:
> -		/* we cannot use software trigger if hw trigger enabled */
> -		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> -			return ret;
> -		mutex_lock(&st->lock);
> +	/*
> +	 * we cannot use software trigger or touchscreen
> +	 * if external trigger is enabled
> +	 */
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +	mutex_lock(&st->lock);
>  
> -		st->chan = chan;
> +	if (chan->type == IIO_POSITIONRELATIVE) {
> +		ret = at91_adc_read_position(st, chan->channel,
> +					     (u16 *)val);
> +		goto at91_adc_read_info_raw_exit;
I know it is more lines of code, but I think I'd prefer to
get rid of this long goto as it's not an error case.

I would suggest moving the mutex inside this if statement
(and the next one) and do the lock / unlock / return all
within the if.  For the other path, take the mutex after
these if statements.

> +	}
> +	if (chan->type == IIO_PRESSURE) {
> +		ret = at91_adc_read_pressure(st, chan->channel,
> +					     (u16 *)val);
> +		goto at91_adc_read_info_raw_exit;
> +	}
>  
> -		if (chan->differential)
> -			cor = (BIT(chan->channel) | BIT(chan->channel2)) <<
> -			      AT91_SAMA5D2_COR_DIFF_OFFSET;
> -
> -		at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> -		at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> -		at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
> -		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> -
> -		ret = wait_event_interruptible_timeout(st->wq_data_available,
> -						       st->conversion_done,
> -						       msecs_to_jiffies(1000));
> -		if (ret == 0)
> -			ret = -ETIMEDOUT;
> -
> -		if (ret > 0) {
> -			*val = st->conversion_value;
> -			if (chan->scan_type.sign == 's')
> -				*val = sign_extend32(*val, 11);
> -			ret = IIO_VAL_INT;
> -			st->conversion_done = false;
> -		}
> +	st->chan = chan;
> +
> +	if (chan->differential)
> +		cor = (BIT(chan->channel) | BIT(chan->channel2)) <<
> +		      AT91_SAMA5D2_COR_DIFF_OFFSET;
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> +	at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
> +	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> +
> +	ret = wait_event_interruptible_timeout(st->wq_data_available,
> +					       st->conversion_done,
> +					       msecs_to_jiffies(1000));
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +
> +	if (ret > 0) {
> +		*val = st->conversion_value;
> +		if (chan->scan_type.sign == 's')
> +			*val = sign_extend32(*val, 11);
> +		ret = IIO_VAL_INT;
> +		st->conversion_done = false;
> +	}
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
> +	at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
>  
> -		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
> -		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> +	/* Needed to ACK the DRDY interruption */
> +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>  
> -		/* Needed to ACK the DRDY interruption */
> -		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> +at91_adc_read_info_raw_exit:
>  
> -		mutex_unlock(&st->lock);
> +	mutex_unlock(&st->lock);
>  
> -		iio_device_release_direct_mode(indio_dev);
> -		return ret;
> +	iio_device_release_direct_mode(indio_dev);
> +	return ret;
> +}
>  
> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return at91_adc_read_info_raw(indio_dev, chan, val);
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = st->vref_uv / 1000;
>  		if (chan->differential)
> @@ -974,9 +1413,29 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>  	return 0;
>  }
>  
> +static int at91_adc_update_scan_mode(struct iio_dev *indio_dev,
> +				     const unsigned long *scan_mask)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	if (bitmap_subset(scan_mask, &st->touch_st.channels_bitmask,
> +			  AT91_SAMA5D2_MAX_CHAN_IDX + 1))
> +		return 0;
> +	/*
> +	 * if the new bitmap is a combination of touchscreen and regular
> +	 * channels, then we are not fine
> +	 */
> +	if (bitmap_intersects(&st->touch_st.channels_bitmask, scan_mask,
> +			      AT91_SAMA5D2_MAX_CHAN_IDX + 1))
> +		return -EBUSY;
> +	return 0;
> +}
> +
>  static const struct iio_info at91_adc_info = {
>  	.read_raw = &at91_adc_read_raw,
>  	.write_raw = &at91_adc_write_raw,
> +	.update_scan_mode = &at91_adc_update_scan_mode,
> +	.of_xlate = &at91_adc_of_xlate,
>  	.hwfifo_set_watermark = &at91_adc_set_watermark,
>  };
>  
> @@ -1044,13 +1503,20 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>  	indio_dev->info = &at91_adc_info;
>  	indio_dev->channels = at91_adc_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
>  
>  	st = iio_priv(indio_dev);
>  
> +	bitmap_set(&st->touch_st.channels_bitmask,
> +		   AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> +	bitmap_set(&st->touch_st.channels_bitmask,
> +		   AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, 1);
> +	bitmap_set(&st->touch_st.channels_bitmask,
> +		   AT91_SAMA5D2_TOUCH_P_CHAN_IDX, 1);
> +
>  	ret = of_property_read_u32(pdev->dev.of_node,
>  				   "atmel,min-sample-rate-hz",
>  				   &st->soc_info.min_sample_rate);
> @@ -1100,6 +1566,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	init_waitqueue_head(&st->wq_data_available);
>  	mutex_init(&st->lock);
> +	INIT_WORK(&st->touch_st.workq, at91_adc_workq_handler);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res)
> @@ -1159,13 +1626,13 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> -	if (st->selected_trig->hw_trig) {
> -		ret = at91_adc_buffer_init(indio_dev);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> -			goto per_clk_disable_unprepare;
> -		}
> +	ret = at91_adc_buffer_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> +		goto per_clk_disable_unprepare;
> +	}
>  
> +	if (st->selected_trig->hw_trig) {
>  		ret = at91_adc_trigger_init(indio_dev);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "couldn't setup the triggers.\n");
> @@ -1272,8 +1739,18 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	at91_adc_hw_init(st);
>  
>  	/* reconfiguring trigger hardware state */
> -	if (iio_buffer_enabled(indio_dev))
> +	if (!iio_buffer_enabled(indio_dev))
> +		return 0;
> +
> +	/* check if we are enabling triggered buffer or the touchscreen */
> +	if (bitmap_subset(indio_dev->active_scan_mask,
> +			  &st->touch_st.channels_bitmask,
> +			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
> +		/* touchscreen enabling */
> +		at91_adc_configure_touch(st, true);
> +	} else {
>  		at91_adc_configure_trigger(st->trig, true);
> +	}
>  
>  	return 0;
>  

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Dmitry Torokhov]

On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:
> On Tue, 10 Apr 2018 11:57:52 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
> > When requesting channels for a particular consumer device,
> > besides requesting the device (incrementing the reference counter), also
> > do it for the driver module of the iio dev. This will avoid the situation
> > where the producer IIO device can be removed and the consumer is still
> > present in the kernel.
> > 
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
> >  drivers/iio/inkern.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index ec98790..68d9b87 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/mutex.h>
> >  #include <linux/of.h>
> > +#include <linux/module.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include "iio_core.h"
> > @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> >  	if (index < 0)
> >  		goto err_put;
> >  	channel->channel = &indio_dev->channels[index];
> > +	try_module_get(channel->indio_dev->driver_module);
> 
> And if it fails? (the module we are trying to get is going away...)
> 
> We should try and handle it I think. Be it by just erroring out of here.

Even more, this has nothing to do with modules. A device can go away for
any number of reasons (we unbind it manually via sysfs, we pull the USB
plug from the host in case it is USB-connected device, we unload I2C
adapter for the bus device resides on, we kick underlying PCI device)
and we should be able to handle this in some fashion. Handling errors
from reads and ignoring garbage is one of methods.

FWIW this is a NACK from me.

Thanks.

-- 
Dmitry

Re: [PATCH v3 03/11] iio: Add channel for Position Relative

[Eugen Hristev]

On 15.04.2018 22:29, Jonathan Cameron wrote:
> On Tue, 10 Apr 2018 11:57:49 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> Add new channel type for relative position on a pad.
>>
>> These type of analog sensor offers 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 will hand the relative position on the pad in both directions.
>>
>> 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>
>> ---
>> Changes in v2:
>>   - modified channel name to relative position as suggested.
>>   - modified kernel version to 4.18 (presumable)
>>
>>   Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
>>   drivers/iio/industrialio-core.c         |  1 +
>>   include/uapi/linux/iio/types.h          |  1 +
>>   tools/iio/iio_event_monitor.c           |  2 ++
>>   4 files changed, 16 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 6a5f34b..42a9287 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -190,6 +190,18 @@ 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_positionrelative_x_raw
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_y_raw
>> +KernelVersion:	4.18
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Relative 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 milli percents
>> +		from the pad's size in both directions. Should be calibrated by
>> +		the consumer.
> I know the milli percent comes form the humidity equivalent, but I wonder
> if we are right to follow that.  10^-5 is a pretty random base unit (though
> I got argued into it being a standard choice for humidity sensors IIRC...
> 
> What do people think?  We could go with 1 for full range or just percent perhaps?
> 
> I'm not that fussed about staying consistent with humidity - we are unlikely
> to end up with sensors doing both anytime soon so there shouldn't be
> any confusion...
> 
> Jonathan
Hello Jonathan,

For my specific use case, percents are not enough, as this would mean 
the resolution will be pretty low. If we have a touchpanel of a higher 
resolution, having the position only ranged 0 to 100 is pretty bad.
Having millipercents means we can have a resolution up to 100,000 points 
in range, which is fine for the hardware as of this moment.
Of course, specific drivers reporting these channels can define a 
specific range if they desire (report just in the first percent if you 
wish (0-1000) or so, which would mean a lower resolution.
Centi-percent would work fine as well I believe (0-10,000 range), but to 
keep consistent with your suggestion regarding relative humidity, I 
picked this larger scale.
In my specific case, since we have a 12 bit ADC, range is 0-4096 for 
this value.
Calibrating these values to the actual size of the touchpanel is left 
for the consumer to do (map 4096x4096 to the actual resolution)

If you have a better idea than milli-percents I can change it, no problem.

Eugen
>> +
>>   [...]

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Eugen Hristev]

On 17.04.2018 02:58, Dmitry Torokhov wrote:
> On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:
>> On Tue, 10 Apr 2018 11:57:52 +0300
>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>
>>> When requesting channels for a particular consumer device,
>>> besides requesting the device (incrementing the reference counter), also
>>> do it for the driver module of the iio dev. This will avoid the situation
>>> where the producer IIO device can be removed and the consumer is still
>>> present in the kernel.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>> ---
>>>   drivers/iio/inkern.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index ec98790..68d9b87 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -11,6 +11,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/mutex.h>
>>>   #include <linux/of.h>
>>> +#include <linux/module.h>
>>>   
>>>   #include <linux/iio/iio.h>
>>>   #include "iio_core.h"
>>> @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>>>   	if (index < 0)
>>>   		goto err_put;
>>>   	channel->channel = &indio_dev->channels[index];
>>> +	try_module_get(channel->indio_dev->driver_module);
>>
>> And if it fails? (the module we are trying to get is going away...)
>>
>> We should try and handle it I think. Be it by just erroring out of here.
> 
> Even more, this has nothing to do with modules. A device can go away for
> any number of reasons (we unbind it manually via sysfs, we pull the USB
> plug from the host in case it is USB-connected device, we unload I2C
> adapter for the bus device resides on, we kick underlying PCI device)
> and we should be able to handle this in some fashion. Handling errors
> from reads and ignoring garbage is one of methods.
> 
> FWIW this is a NACK from me.
> 
> Thanks.
Hello,

This patch is actually a "best effort attempt" for the consumer driver 
(touch driver) to get a reference to the producer of the data (the IIO 
device), when it requests the specific channels.
As of this moment, there is no attempt whatsoever for the consumer to 
have a reference on the producer driver. Thus, the producer can be 
removed at any time, and the consumer will fail ungraciously.
I can change the perspective from "best effort" to "mandatory" to get a 
reference to the producer, or you wish to stop trying to get any 
reference at all (remove this patch completely) ?

Thanks,
Eugen

> 

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Dmitry Torokhov]

Hi Eugen,

On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
> 
> 
> On 17.04.2018 02:58, Dmitry Torokhov wrote:
> > On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:
> > > On Tue, 10 Apr 2018 11:57:52 +0300
> > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > 
> > > > When requesting channels for a particular consumer device,
> > > > besides requesting the device (incrementing the reference counter), also
> > > > do it for the driver module of the iio dev. This will avoid the situation
> > > > where the producer IIO device can be removed and the consumer is still
> > > > present in the kernel.
> > > > 
> > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > ---
> > > >   drivers/iio/inkern.c | 8 +++++++-
> > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > > index ec98790..68d9b87 100644
> > > > --- a/drivers/iio/inkern.c
> > > > +++ b/drivers/iio/inkern.c
> > > > @@ -11,6 +11,7 @@
> > > >   #include <linux/slab.h>
> > > >   #include <linux/mutex.h>
> > > >   #include <linux/of.h>
> > > > +#include <linux/module.h>
> > > >   #include <linux/iio/iio.h>
> > > >   #include "iio_core.h"
> > > > @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> > > >   	if (index < 0)
> > > >   		goto err_put;
> > > >   	channel->channel = &indio_dev->channels[index];
> > > > +	try_module_get(channel->indio_dev->driver_module);
> > > 
> > > And if it fails? (the module we are trying to get is going away...)
> > > 
> > > We should try and handle it I think. Be it by just erroring out of here.
> > 
> > Even more, this has nothing to do with modules. A device can go away for
> > any number of reasons (we unbind it manually via sysfs, we pull the USB
> > plug from the host in case it is USB-connected device, we unload I2C
> > adapter for the bus device resides on, we kick underlying PCI device)
> > and we should be able to handle this in some fashion. Handling errors
> > from reads and ignoring garbage is one of methods.
> > 
> > FWIW this is a NACK from me.
> > 
> > Thanks.
> Hello,
> 
> This patch is actually a "best effort attempt" for the consumer driver
> (touch driver) to get a reference to the producer of the data (the IIO
> device), when it requests the specific channels.
> As of this moment, there is no attempt whatsoever for the consumer to have a
> reference on the producer driver. Thus, the producer can be removed at any
> time, and the consumer will fail ungraciously.

This is the root of the issue. The consumer should be prepared to handle
errors from producer.

> I can change the perspective from "best effort" to "mandatory" to get a
> reference to the producer, or you wish to stop trying to get any reference
> at all (remove this patch completely) ?

You should take reference to the device itself (if it is not taken
already), so it does not disappear completely and you can continue using
IIO API to access it, and IIO API should be prepared to deal with "dead"
devices, but as I pointed in my other email, trying to pin the driver
is quite pointless as there are myriad other ways of device stopping
working besides module unloading.

In any case, I think this problem is outside of the scope of this
patchset that adds a generic resistive touchscreen, so if you want to
continue working on this I'd recommend moving it into a separate series.

Thanks.

-- 
Dmitry

Re: [PATCH v3 03/11] iio: Add channel for Position Relative

[Jonathan Cameron]

On Tue, 17 Apr 2018 10:30:46 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On 15.04.2018 22:29, Jonathan Cameron wrote:
> > On Tue, 10 Apr 2018 11:57:49 +0300
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >   
> >> Add new channel type for relative position on a pad.
> >>
> >> These type of analog sensor offers 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 will hand the relative position on the pad in both directions.
> >>
> >> 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>
> >> ---
> >> Changes in v2:
> >>   - modified channel name to relative position as suggested.
> >>   - modified kernel version to 4.18 (presumable)
> >>
> >>   Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
> >>   drivers/iio/industrialio-core.c         |  1 +
> >>   include/uapi/linux/iio/types.h          |  1 +
> >>   tools/iio/iio_event_monitor.c           |  2 ++
> >>   4 files changed, 16 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> >> index 6a5f34b..42a9287 100644
> >> --- a/Documentation/ABI/testing/sysfs-bus-iio
> >> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> >> @@ -190,6 +190,18 @@ 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_positionrelative_x_raw
> >> +What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_y_raw
> >> +KernelVersion:	4.18
> >> +Contact:	linux-iio@vger.kernel.org
> >> +Description:
> >> +		Relative 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 milli percents
> >> +		from the pad's size in both directions. Should be calibrated by
> >> +		the consumer.  
> > I know the milli percent comes form the humidity equivalent, but I wonder
> > if we are right to follow that.  10^-5 is a pretty random base unit (though
> > I got argued into it being a standard choice for humidity sensors IIRC...
> > 
> > What do people think?  We could go with 1 for full range or just percent perhaps?
> > 
> > I'm not that fussed about staying consistent with humidity - we are unlikely
> > to end up with sensors doing both anytime soon so there shouldn't be
> > any confusion...
> > 
> > Jonathan  
> Hello Jonathan,
> 
> For my specific use case, percents are not enough, as this would mean 
> the resolution will be pretty low. If we have a touchpanel of a higher 
> resolution, having the position only ranged 0 to 100 is pretty bad.
> Having millipercents means we can have a resolution up to 100,000 points 
> in range, which is fine for the hardware as of this moment.
> Of course, specific drivers reporting these channels can define a 
> specific range if they desire (report just in the first percent if you 
> wish (0-1000) or so, which would mean a lower resolution.
> Centi-percent would work fine as well I believe (0-10,000 range), but to 
> keep consistent with your suggestion regarding relative humidity, I 
> picked this larger scale.
> In my specific case, since we have a 12 bit ADC, range is 0-4096 for 
> this value.
> Calibrating these values to the actual size of the touchpanel is left 
> for the consumer to do (map 4096x4096 to the actual resolution)
> 
> If you have a better idea than milli-percents I can change it, no problem.
> 

It doesn't actually matter what the defined 'base units' are from a question
of what we can represent.  That is just handled by suppling an appropriate
SCALE for the channel.  All this is about is trying to provide something
consistent rather than having any effect on the actual range.  Will let
this sit a while longer...

Jonathan

> Eugen
> >> +
> >>   [...]  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Jonathan Cameron]

On Tue, 17 Apr 2018 12:19:06 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Eugen,
> 
> On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
> > 
> > 
> > On 17.04.2018 02:58, Dmitry Torokhov wrote:  
> > > On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 10 Apr 2018 11:57:52 +0300
> > > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > >   
> > > > > When requesting channels for a particular consumer device,
> > > > > besides requesting the device (incrementing the reference counter), also
> > > > > do it for the driver module of the iio dev. This will avoid the situation
> > > > > where the producer IIO device can be removed and the consumer is still
> > > > > present in the kernel.
> > > > > 
> > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > ---
> > > > >   drivers/iio/inkern.c | 8 +++++++-
> > > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > > > index ec98790..68d9b87 100644
> > > > > --- a/drivers/iio/inkern.c
> > > > > +++ b/drivers/iio/inkern.c
> > > > > @@ -11,6 +11,7 @@
> > > > >   #include <linux/slab.h>
> > > > >   #include <linux/mutex.h>
> > > > >   #include <linux/of.h>
> > > > > +#include <linux/module.h>
> > > > >   #include <linux/iio/iio.h>
> > > > >   #include "iio_core.h"
> > > > > @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> > > > >   	if (index < 0)
> > > > >   		goto err_put;
> > > > >   	channel->channel = &indio_dev->channels[index];
> > > > > +	try_module_get(channel->indio_dev->driver_module);  
> > > > 
> > > > And if it fails? (the module we are trying to get is going away...)
> > > > 
> > > > We should try and handle it I think. Be it by just erroring out of here.  
> > > 
> > > Even more, this has nothing to do with modules. A device can go away for
> > > any number of reasons (we unbind it manually via sysfs, we pull the USB
> > > plug from the host in case it is USB-connected device, we unload I2C
> > > adapter for the bus device resides on, we kick underlying PCI device)
> > > and we should be able to handle this in some fashion. Handling errors
> > > from reads and ignoring garbage is one of methods.
> > > 
> > > FWIW this is a NACK from me.
> > > 
> > > Thanks.  
> > Hello,
> > 
> > This patch is actually a "best effort attempt" for the consumer driver
> > (touch driver) to get a reference to the producer of the data (the IIO
> > device), when it requests the specific channels.
> > As of this moment, there is no attempt whatsoever for the consumer to have a
> > reference on the producer driver. Thus, the producer can be removed at any
> > time, and the consumer will fail ungraciously.  
> 
> This is the root of the issue. The consumer should be prepared to handle
> errors from producer.
> 
> > I can change the perspective from "best effort" to "mandatory" to get a
> > reference to the producer, or you wish to stop trying to get any reference
> > at all (remove this patch completely) ?  
> 
> You should take reference to the device itself (if it is not taken
> already), so it does not disappear completely and you can continue using
> IIO API to access it, and IIO API should be prepared to deal with "dead"
> devices, but as I pointed in my other email, trying to pin the driver
> is quite pointless as there are myriad other ways of device stopping
> working besides module unloading.
> 
> In any case, I think this problem is outside of the scope of this
> patchset that adds a generic resistive touchscreen, so if you want to
> continue working on this I'd recommend moving it into a separate series.
> 
> Thanks.
> 
Agreed, this one has come up a number of times before.  Quite a lot of
work got done by (IIRC) Lars Peter Clausen to stabilize things in various
unexpected 'going away' events.  Of course there may be paths we have
added since that (it was years ago) that can cause trouble...

Anyhow, separate issue as Dmitry says, let's deal with it separately.

Jonathan

Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels

[Lars-Peter Clausen]

On 04/18/2018 11:35 AM, Jonathan Cameron wrote:
> On Tue, 17 Apr 2018 12:19:06 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
>> Hi Eugen,
>>
>> On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
>>>
>>>
>>> On 17.04.2018 02:58, Dmitry Torokhov wrote:  
>>>> On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:  
>>>>> On Tue, 10 Apr 2018 11:57:52 +0300
>>>>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>>>>   
>>>>>> When requesting channels for a particular consumer device,
>>>>>> besides requesting the device (incrementing the reference counter), also
>>>>>> do it for the driver module of the iio dev. This will avoid the situation
>>>>>> where the producer IIO device can be removed and the consumer is still
>>>>>> present in the kernel.
>>>>>>
>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>> ---
>>>>>>   drivers/iio/inkern.c | 8 +++++++-
>>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>>>> index ec98790..68d9b87 100644
>>>>>> --- a/drivers/iio/inkern.c
>>>>>> +++ b/drivers/iio/inkern.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>   #include <linux/slab.h>
>>>>>>   #include <linux/mutex.h>
>>>>>>   #include <linux/of.h>
>>>>>> +#include <linux/module.h>
>>>>>>   #include <linux/iio/iio.h>
>>>>>>   #include "iio_core.h"
>>>>>> @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>>>>>>   	if (index < 0)
>>>>>>   		goto err_put;
>>>>>>   	channel->channel = &indio_dev->channels[index];
>>>>>> +	try_module_get(channel->indio_dev->driver_module);  
>>>>>
>>>>> And if it fails? (the module we are trying to get is going away...)
>>>>>
>>>>> We should try and handle it I think. Be it by just erroring out of here.  
>>>>
>>>> Even more, this has nothing to do with modules. A device can go away for
>>>> any number of reasons (we unbind it manually via sysfs, we pull the USB
>>>> plug from the host in case it is USB-connected device, we unload I2C
>>>> adapter for the bus device resides on, we kick underlying PCI device)
>>>> and we should be able to handle this in some fashion. Handling errors
>>>> from reads and ignoring garbage is one of methods.
>>>>
>>>> FWIW this is a NACK from me.
>>>>
>>>> Thanks.  
>>> Hello,
>>>
>>> This patch is actually a "best effort attempt" for the consumer driver
>>> (touch driver) to get a reference to the producer of the data (the IIO
>>> device), when it requests the specific channels.
>>> As of this moment, there is no attempt whatsoever for the consumer to have a
>>> reference on the producer driver. Thus, the producer can be removed at any
>>> time, and the consumer will fail ungraciously.  
>>
>> This is the root of the issue. The consumer should be prepared to handle
>> errors from producer.
>>
>>> I can change the perspective from "best effort" to "mandatory" to get a
>>> reference to the producer, or you wish to stop trying to get any reference
>>> at all (remove this patch completely) ?  
>>
>> You should take reference to the device itself (if it is not taken
>> already), so it does not disappear completely and you can continue using
>> IIO API to access it, and IIO API should be prepared to deal with "dead"
>> devices, but as I pointed in my other email, trying to pin the driver
>> is quite pointless as there are myriad other ways of device stopping
>> working besides module unloading.
>>
>> In any case, I think this problem is outside of the scope of this
>> patchset that adds a generic resistive touchscreen, so if you want to
>> continue working on this I'd recommend moving it into a separate series.
>>
>> Thanks.
>>
> Agreed, this one has come up a number of times before.  Quite a lot of
> work got done by (IIRC) Lars Peter Clausen to stabilize things in various
> unexpected 'going away' events.  Of course there may be paths we have
> added since that (it was years ago) that can cause trouble...
> 
> Anyhow, separate issue as Dmitry says, let's deal with it separately.

We do properly get a ref to the device. Not getting a ref to the module is
on purpose. Cause as Dmitry said it does nothing the device can disappear at
any point either way. I second the NACK.

Re: [PATCH v3 08/11] input: touchscreen: resistive-adc-touch: add generic resistive ADC touchscreen

[Dmitry Torokhov]

Hi Eugen,

On Tue, Apr 10, 2018 at 11:57:54AM +0300, Eugen Hristev wrote:
> This adds a generic resistive touchscreen (GRTS) driver, which is based
> on an IIO device (an ADC). It must be connected to the channels of an ADC
> to receive touch data. Then it will feed the data into the input subsystem
> where it registers an input device.
> It uses an IIO callback buffer to register to the IIO device
> 
> 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>
> ---
> Changes in v3:
>  - pressure now reported naturally growing down-up
>  - using helpers from touchscreen.h to parse DT properties
>  - added devm_add_action_or_reset to handle callback buffer clean on exit
>  - changed compatible and threshold property to adapt to changed bindings
> 
> Changes in v2:
>  - this is now a generic resistive adc touchscreen driver
> 
>  drivers/input/touchscreen/Kconfig               |  13 ++
>  drivers/input/touchscreen/Makefile              |   1 +
>  drivers/input/touchscreen/resistive-adc-touch.c | 200 ++++++++++++++++++++++++
>  3 files changed, 214 insertions(+)
>  create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 4f15496..8f85d3a 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -92,6 +92,19 @@ config TOUCHSCREEN_AD7879_SPI
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7879-spi.
>  
> +config TOUCHSCREEN_ADC
> +	tristate "Generic ADC based resistive touchscreen"
> +	depends on IIO
> +	select IIO_BUFFER_CB
> +	help
> +	  Say Y here if you want to use the generic ADC
> +	  resistive touchscreen driver.
> +
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called resistive-adc-touch.ko.
> +
>  config TOUCHSCREEN_AR1021_I2C
>  	tristate "Microchip AR1020/1021 i2c touchscreen"
>  	depends on I2C && OF
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index dddae79..843c7f9 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
> +obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
> diff --git a/drivers/input/touchscreen/resistive-adc-touch.c b/drivers/input/touchscreen/resistive-adc-touch.c
> new file mode 100644
> index 0000000..7e201ee
> --- /dev/null
> +++ b/drivers/input/touchscreen/resistive-adc-touch.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADC generic resistive touchscreen (GRTS)
> + *
> + * Copyright (C) 2017,2018 Microchip Technology,
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + */
> +#include <linux/input.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME					"resistive-adc-touch"
> +#define GRTS_DEFAULT_PRESSURE_THRESHOLD			50000
> +#define MAX_POS_MASK					GENMASK(11, 0)
> +
> +/**
> + * grts_state - generic resistive touch screen information struct
> + * @pressure_threshold:	number representing the threshold for the pressure
> + * @pressure:		are we getting pressure info or not
> + * @iio_chans:		list of channels acquired
> + * @iio_cb:		iio_callback buffer for the data
> + * @input:		the input device structure that we register
> + * @prop:		touchscreen properties struct
> + */
> +struct grts_state {
> +	u32				pressure_threshold;
> +	bool				pressure;
> +	struct iio_channel		*iio_chans;
> +	struct iio_cb_buffer		*iio_cb;
> +	struct input_dev		*input;
> +	struct touchscreen_properties	prop;
> +};
> +
> +static int grts_cb(const void *data, void *private)
> +{
> +	const u16 *touch_info = data;
> +	struct grts_state *st = private;
> +

Extra blank line.

> +	unsigned int x, y, press = 0xFFFF;

Why to you need to initialize "press"?

> +
> +	/* channel data coming in buffer in the order below */
> +	x = touch_info[0];
> +	y = touch_info[1];
> +	if (st->pressure)
> +		press = touch_info[2];
> +
> +	if ((!x && !y) || (st->pressure && (press < st->pressure_threshold))) {
> +		/* report end of touch */

Hm, so this makes "press" not pressure but resistance...

> +		input_report_key(st->input, BTN_TOUCH, 0);
> +		input_sync(st->input);
> +		return 0;
> +	}
> +
> +	/* report proper touch to subsystem*/
> +	touchscreen_report_pos(st->input, &st->prop, x, y, false);
> +	if (st->pressure)
> +		input_report_abs(st->input, ABS_PRESSURE, press);

and if this is resistance, then you want to have:

		input_report_abs(st->input, ABS_PRESSURE,
				 st->resistance_threshold - resistance);

> +	input_report_key(st->input, BTN_TOUCH, 1);
> +	input_sync(st->input);
> +
> +	return 0;
> +}
> +
> +static int grts_open(struct input_dev *dev)
> +{
> +	int error;
> +	struct grts_state *st = input_get_drvdata(dev);
> +
> +	error = iio_channel_start_all_cb(st->iio_cb);
> +	if (error) {
> +		dev_err(dev->dev.parent, "failed to start callback buffer.\n");
> +		return error;
> +	}
> +	return 0;
> +}
> +
> +static void grts_close(struct input_dev *dev)
> +{
> +	struct grts_state *st = input_get_drvdata(dev);
> +
> +	iio_channel_stop_all_cb(st->iio_cb);
> +}
> +
> +static void grts_disable(void *data)
> +{
> +	iio_channel_release_all_cb(data);
> +}
> +
> +static int grts_probe(struct platform_device *pdev)
> +{
> +	struct grts_state *st;
> +	struct input_dev *input;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct iio_channel *chan;
> +	int ret = 0;

Let's call this variable "error". There is also no need to initialize it
to 0.

> +
> +	st = devm_kzalloc(dev, sizeof(struct grts_state), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(node, "touchscreen-threshold-pressure",
> +				   &st->pressure_threshold);

Let's use generic device property API (device_property_read_u32).


> +	if (ret < 0) {

I do not think of_ or device_ property API returns positive on success.
So simply

	if (error) {
		...
	}

> +		dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
> +		st->pressure_threshold = GRTS_DEFAULT_PRESSURE_THRESHOLD;
> +	}
> +
> +	/* get the channels from IIO device */
> +	st->iio_chans = devm_iio_channel_get_all(dev);
> +

Extra blank line.

> +	if (IS_ERR(st->iio_chans)) {
> +		if (PTR_ERR(st->iio_chans) != -EPROBE_DEFER)
> +			dev_err(dev, "can't get iio channels.\n");
> +		return PTR_ERR(st->iio_chans);
> +	}
> +
> +	chan = &st->iio_chans[0];
> +	st->pressure = false;
> +	while (chan && chan->indio_dev) {
> +		if (!strcmp(chan->channel->datasheet_name, "pressure"))
> +			st->pressure = true;
> +		chan++;
> +	}
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "failed to allocate input device.\n");
> +		return -ENOMEM;
> +	}
> +
> +	input->name = DRIVER_NAME;
> +	input->id.bustype = BUS_HOST;
> +	input->open = grts_open;
> +	input->close = grts_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 - 1, 0, 0);
> +	if (st->pressure)
> +		input_set_abs_params(input, ABS_PRESSURE, 0, 0xffffff, 0, 0);

I am sure we can figure the range given the resistance threshold.

> +
> +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);

Replace this 2 with

	input_set_capability(input, EV_KEY, BTN_TOUCH);

> +
> +	/* parse optional device tree properties */
> +	touchscreen_parse_properties(input, false, &st->prop);
> +
> +	st->input = input;
> +	input_set_drvdata(input, st);
> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "failed to register input device.");
> +		return ret;
> +	}
> +
> +	st->iio_cb = iio_channel_get_all_cb(dev, grts_cb, st);
> +
> +	if (IS_ERR(st->iio_cb)) {
> +		dev_err(dev, "failed to allocate callback buffer.\n");
> +		ret =  PTR_ERR(st->iio_cb);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, grts_disable, st->iio_cb);

	if (error)
		return error;

> +
> +	platform_set_drvdata(pdev, st);

I do not think you are using platform device driver data anywhere.

> +
> +	return ret;

Prefer explicit success returns, so

	return 0;

> +}
> +
> +static const struct of_device_id grts_of_match[] = {
> +	{
> +		.compatible = "resistive-adc-touch",
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +
> +MODULE_DEVICE_TABLE(of, grts_of_match);
> +
> +static struct platform_driver grts_driver = {
> +	.probe = grts_probe,
> +	.driver = {
> +		   .name = DRIVER_NAME,

Keep indenting with tabs please.

> +		   .of_match_table = of_match_ptr(grts_of_match),
> +	},
> +};
> +
> +module_platform_driver(grts_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> +MODULE_DESCRIPTION("Generic ADC Resistive Touch Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: [PATCH v3 08/11] input: touchscreen: resistive-adc-touch: add generic resistive ADC touchscreen

[Dmitry Torokhov]

On Mon, Apr 23, 2018 at 04:22:51PM -0700, Dmitry Torokhov wrote:
> Hi Eugen,
> 
> On Tue, Apr 10, 2018 at 11:57:54AM +0300, Eugen Hristev wrote:
> > This adds a generic resistive touchscreen (GRTS) driver, which is based
> > on an IIO device (an ADC). It must be connected to the channels of an ADC
> > to receive touch data. Then it will feed the data into the input subsystem
> > where it registers an input device.
> > It uses an IIO callback buffer to register to the IIO device
> > 
> > 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>
> > ---
> > Changes in v3:
> >  - pressure now reported naturally growing down-up
> >  - using helpers from touchscreen.h to parse DT properties
> >  - added devm_add_action_or_reset to handle callback buffer clean on exit
> >  - changed compatible and threshold property to adapt to changed bindings
> > 
> > Changes in v2:
> >  - this is now a generic resistive adc touchscreen driver
> > 
> >  drivers/input/touchscreen/Kconfig               |  13 ++
> >  drivers/input/touchscreen/Makefile              |   1 +
> >  drivers/input/touchscreen/resistive-adc-touch.c | 200 ++++++++++++++++++++++++
> >  3 files changed, 214 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
> > 
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 4f15496..8f85d3a 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -92,6 +92,19 @@ config TOUCHSCREEN_AD7879_SPI
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ad7879-spi.
> >  
> > +config TOUCHSCREEN_ADC
> > +	tristate "Generic ADC based resistive touchscreen"
> > +	depends on IIO
> > +	select IIO_BUFFER_CB
> > +	help
> > +	  Say Y here if you want to use the generic ADC
> > +	  resistive touchscreen driver.
> > +
> > +	  If unsure, say N (but it's safe to say "Y").
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called resistive-adc-touch.ko.
> > +
> >  config TOUCHSCREEN_AR1021_I2C
> >  	tristate "Microchip AR1020/1021 i2c touchscreen"
> >  	depends on I2C && OF
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index dddae79..843c7f9 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -13,6 +13,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
> >  obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
> >  obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
> >  obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
> > +obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
> >  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
> >  obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
> >  obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
> > diff --git a/drivers/input/touchscreen/resistive-adc-touch.c b/drivers/input/touchscreen/resistive-adc-touch.c
> > new file mode 100644
> > index 0000000..7e201ee
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/resistive-adc-touch.c
> > @@ -0,0 +1,200 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ADC generic resistive touchscreen (GRTS)
> > + *
> > + * Copyright (C) 2017,2018 Microchip Technology,
> > + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> > + *
> > + */
> > +#include <linux/input.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/iio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRIVER_NAME					"resistive-adc-touch"
> > +#define GRTS_DEFAULT_PRESSURE_THRESHOLD			50000
> > +#define MAX_POS_MASK					GENMASK(11, 0)
> > +
> > +/**
> > + * grts_state - generic resistive touch screen information struct
> > + * @pressure_threshold:	number representing the threshold for the pressure
> > + * @pressure:		are we getting pressure info or not
> > + * @iio_chans:		list of channels acquired
> > + * @iio_cb:		iio_callback buffer for the data
> > + * @input:		the input device structure that we register
> > + * @prop:		touchscreen properties struct
> > + */
> > +struct grts_state {
> > +	u32				pressure_threshold;
> > +	bool				pressure;
> > +	struct iio_channel		*iio_chans;
> > +	struct iio_cb_buffer		*iio_cb;
> > +	struct input_dev		*input;
> > +	struct touchscreen_properties	prop;
> > +};
> > +
> > +static int grts_cb(const void *data, void *private)
> > +{
> > +	const u16 *touch_info = data;
> > +	struct grts_state *st = private;
> > +
> 
> Extra blank line.
> 
> > +	unsigned int x, y, press = 0xFFFF;
> 
> Why to you need to initialize "press"?
> 
> > +
> > +	/* channel data coming in buffer in the order below */
> > +	x = touch_info[0];
> > +	y = touch_info[1];
> > +	if (st->pressure)
> > +		press = touch_info[2];
> > +
> > +	if ((!x && !y) || (st->pressure && (press < st->pressure_threshold))) {
> > +		/* report end of touch */
> 
> Hm, so this makes "press" not pressure but resistance...

Sorry, ignore this and below. I misread the code.

> 
> > +		input_report_key(st->input, BTN_TOUCH, 0);
> > +		input_sync(st->input);
> > +		return 0;
> > +	}
> > +
> > +	/* report proper touch to subsystem*/
> > +	touchscreen_report_pos(st->input, &st->prop, x, y, false);
> > +	if (st->pressure)
> > +		input_report_abs(st->input, ABS_PRESSURE, press);
> 
> and if this is resistance, then you want to have:
> 
> 		input_report_abs(st->input, ABS_PRESSURE,
> 				 st->resistance_threshold - resistance);
> 
> > +	input_report_key(st->input, BTN_TOUCH, 1);
> > +	input_sync(st->input);
> > +
> > +	return 0;
> > +}
> > +
> > +static int grts_open(struct input_dev *dev)
> > +{
> > +	int error;
> > +	struct grts_state *st = input_get_drvdata(dev);
> > +
> > +	error = iio_channel_start_all_cb(st->iio_cb);
> > +	if (error) {
> > +		dev_err(dev->dev.parent, "failed to start callback buffer.\n");
> > +		return error;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void grts_close(struct input_dev *dev)
> > +{
> > +	struct grts_state *st = input_get_drvdata(dev);
> > +
> > +	iio_channel_stop_all_cb(st->iio_cb);
> > +}
> > +
> > +static void grts_disable(void *data)
> > +{
> > +	iio_channel_release_all_cb(data);
> > +}
> > +
> > +static int grts_probe(struct platform_device *pdev)
> > +{
> > +	struct grts_state *st;
> > +	struct input_dev *input;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct iio_channel *chan;
> > +	int ret = 0;
> 
> Let's call this variable "error". There is also no need to initialize it
> to 0.
> 
> > +
> > +	st = devm_kzalloc(dev, sizeof(struct grts_state), GFP_KERNEL);
> > +	if (!st)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32(node, "touchscreen-threshold-pressure",
> > +				   &st->pressure_threshold);
> 
> Let's use generic device property API (device_property_read_u32).
> 
> 
> > +	if (ret < 0) {
> 
> I do not think of_ or device_ property API returns positive on success.
> So simply
> 
> 	if (error) {
> 		...
> 	}
> 
> > +		dev_dbg(dev, "can't get touchscreen pressure threshold property.\n");
> > +		st->pressure_threshold = GRTS_DEFAULT_PRESSURE_THRESHOLD;
> > +	}
> > +
> > +	/* get the channels from IIO device */
> > +	st->iio_chans = devm_iio_channel_get_all(dev);
> > +
> 
> Extra blank line.
> 
> > +	if (IS_ERR(st->iio_chans)) {
> > +		if (PTR_ERR(st->iio_chans) != -EPROBE_DEFER)
> > +			dev_err(dev, "can't get iio channels.\n");
> > +		return PTR_ERR(st->iio_chans);
> > +	}
> > +
> > +	chan = &st->iio_chans[0];
> > +	st->pressure = false;
> > +	while (chan && chan->indio_dev) {
> > +		if (!strcmp(chan->channel->datasheet_name, "pressure"))
> > +			st->pressure = true;
> > +		chan++;
> > +	}
> > +
> > +	input = devm_input_allocate_device(dev);
> > +	if (!input) {
> > +		dev_err(dev, "failed to allocate input device.\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	input->name = DRIVER_NAME;
> > +	input->id.bustype = BUS_HOST;
> > +	input->open = grts_open;
> > +	input->close = grts_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 - 1, 0, 0);
> > +	if (st->pressure)
> > +		input_set_abs_params(input, ABS_PRESSURE, 0, 0xffffff, 0, 0);
> 
> I am sure we can figure the range given the resistance threshold.
> 
> > +
> > +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> 
> Replace this 2 with
> 
> 	input_set_capability(input, EV_KEY, BTN_TOUCH);
> 
> > +
> > +	/* parse optional device tree properties */
> > +	touchscreen_parse_properties(input, false, &st->prop);
> > +
> > +	st->input = input;
> > +	input_set_drvdata(input, st);
> > +
> > +	ret = input_register_device(input);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register input device.");
> > +		return ret;
> > +	}
> > +
> > +	st->iio_cb = iio_channel_get_all_cb(dev, grts_cb, st);
> > +
> > +	if (IS_ERR(st->iio_cb)) {
> > +		dev_err(dev, "failed to allocate callback buffer.\n");
> > +		ret =  PTR_ERR(st->iio_cb);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(dev, grts_disable, st->iio_cb);
> 
> 	if (error)
> 		return error;
> 
> > +
> > +	platform_set_drvdata(pdev, st);
> 
> I do not think you are using platform device driver data anywhere.
> 
> > +
> > +	return ret;
> 
> Prefer explicit success returns, so
> 
> 	return 0;
> 
> > +}
> > +
> > +static const struct of_device_id grts_of_match[] = {
> > +	{
> > +		.compatible = "resistive-adc-touch",
> > +	}, {
> > +		/* sentinel */
> > +	},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, grts_of_match);
> > +
> > +static struct platform_driver grts_driver = {
> > +	.probe = grts_probe,
> > +	.driver = {
> > +		   .name = DRIVER_NAME,
> 
> Keep indenting with tabs please.
> 
> > +		   .of_match_table = of_match_ptr(grts_of_match),
> > +	},
> > +};
> > +
> > +module_platform_driver(grts_driver);
> > +
> > +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> > +MODULE_DESCRIPTION("Generic ADC Resistive Touch Driver");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Dmitry

-- 
Dmitry