[PATCH v1 0/2] Input: ads7846: reduce SPI related CPU load

[PATCH v1 0/2] Input: ads7846: reduce SPI related CPU load

[Oleksij Rempel]

This series is optimizing SPI transfer related CPU load.

Oleksij Rempel (2):
  Input: ads7846: convert to full duplex
  Input: ads7846: convert to one message

 drivers/input/touchscreen/ads7846.c | 395 +++++++++++-----------------
 1 file changed, 151 insertions(+), 244 deletions(-)

-- 
2.28.0

[PATCH v1 1/2] Input: ads7846: convert to full duplex

[Oleksij Rempel]

Starting with following patch, the ads7845 was partially converted to
full duplex mode:
3eac5c7e44f3 Input: ads7846 - extend the driver for ads7845 controller support

Since it is not touchscreen controller specific, it is better to make
this conversion consequent for complete driver. This will reduce CPU
load and make driver more readable.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/input/touchscreen/ads7846.c | 193 +++++++++-------------------
 1 file changed, 62 insertions(+), 131 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 393ab337a9c4..e9a520c9ad69 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -62,19 +62,25 @@
 /* this driver doesn't aim at the peak continuous sample rate */
 #define	SAMPLE_BITS	(8 /*cmd*/ + 16 /*sample*/ + 2 /* before, after */)
 
-struct ts_event {
-	/*
-	 * For portability, we can't read 12 bit values using SPI (which
-	 * would make the controller deliver them as native byte order u16
-	 * with msbs zeroed).  Instead, we read them as two 8-bit values,
-	 * *** WHICH NEED BYTESWAPPING *** and range adjustment.
+struct ads7846_buf {
+	u8 cmd;
+	/* This union is a temporary hack. The driver does an in-place
+	 * endianness conversion. This will be cleaned up in the next
+	 * patch.
 	 */
-	u16	x;
-	u16	y;
-	u16	z1, z2;
-	bool	ignore;
-	u8	x_buf[3];
-	u8	y_buf[3];
+	union {
+		__be16 data_be16;
+		u16 data;
+	};
+} __attribute__((__packed__));
+
+
+struct ts_event {
+	bool ignore;
+	struct ads7846_buf x;
+	struct ads7846_buf y;
+	struct ads7846_buf z1;
+	struct ads7846_buf z2;
 };
 
 /*
@@ -83,11 +89,12 @@ struct ts_event {
  * systems where main memory is not DMA-coherent (most non-x86 boards).
  */
 struct ads7846_packet {
-	u8			read_x, read_y, read_z1, read_z2, pwrdown;
-	u16			dummy;		/* for the pwrdown read */
-	struct ts_event		tc;
-	/* for ads7845 with mpc5121 psc spi we use 3-byte buffers */
-	u8			read_x_cmd[3], read_y_cmd[3], pwrdown_cmd[3];
+	struct ts_event tc;
+	struct ads7846_buf read_x_cmd;
+	struct ads7846_buf read_y_cmd;
+	struct ads7846_buf read_z1_cmd;
+	struct ads7846_buf read_z2_cmd;
+	struct ads7846_buf pwrdown_cmd;
 };
 
 struct ads7846 {
@@ -694,16 +701,9 @@ static int ads7846_get_value(struct ads7846 *ts, struct spi_message *m)
 	int value;
 	struct spi_transfer *t =
 		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+	struct ads7846_buf *buf = t->rx_buf;
 
-	if (ts->model == 7845) {
-		value = be16_to_cpup((__be16 *)&(((char *)t->rx_buf)[1]));
-	} else {
-		/*
-		 * adjust:  on-wire is a must-ignore bit, a BE12 value, then
-		 * padding; built from two 8 bit values written msb-first.
-		 */
-		value = be16_to_cpup((__be16 *)t->rx_buf);
-	}
+	value = be16_to_cpup(&buf->data_be16);
 
 	/* enforce ADC output is 12 bits width */
 	return (value >> 3) & 0xfff;
@@ -713,8 +713,9 @@ static void ads7846_update_value(struct spi_message *m, int val)
 {
 	struct spi_transfer *t =
 		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+	struct ads7846_buf *buf = t->rx_buf;
 
-	*(u16 *)t->rx_buf = val;
+	buf->data = val;
 }
 
 static void ads7846_read_state(struct ads7846 *ts)
@@ -782,16 +783,14 @@ static void ads7846_report_state(struct ads7846 *ts)
 	 * from on-the-wire format as part of debouncing to get stable
 	 * readings.
 	 */
+	x = packet->tc.x.data;
+	y = packet->tc.y.data;
 	if (ts->model == 7845) {
-		x = *(u16 *)packet->tc.x_buf;
-		y = *(u16 *)packet->tc.y_buf;
 		z1 = 0;
 		z2 = 0;
 	} else {
-		x = packet->tc.x;
-		y = packet->tc.y;
-		z1 = packet->tc.z1;
-		z2 = packet->tc.z2;
+		z1 = packet->tc.z1.data;
+		z2 = packet->tc.z2.data;
 	}
 
 	/* range filtering */
@@ -1008,26 +1007,11 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 	spi_message_init(m);
 	m->context = ts;
 
-	if (ts->model == 7845) {
-		packet->read_y_cmd[0] = READ_Y(vref);
-		packet->read_y_cmd[1] = 0;
-		packet->read_y_cmd[2] = 0;
-		x->tx_buf = &packet->read_y_cmd[0];
-		x->rx_buf = &packet->tc.y_buf[0];
-		x->len = 3;
-		spi_message_add_tail(x, m);
-	} else {
-		/* y- still on; turn on only y+ (and ADC) */
-		packet->read_y = READ_Y(vref);
-		x->tx_buf = &packet->read_y;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
-		x->rx_buf = &packet->tc.y;
-		x->len = 2;
-		spi_message_add_tail(x, m);
-	}
+	packet->read_y_cmd.cmd = READ_Y(vref);
+	x->tx_buf = &packet->read_y_cmd;
+	x->rx_buf = &packet->tc.y;
+	x->len = 3;
+	spi_message_add_tail(x, m);
 
 	/*
 	 * The first sample after switching drivers can be low quality;
@@ -1037,15 +1021,11 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 	if (pdata->settle_delay_usecs) {
 		x->delay.value = pdata->settle_delay_usecs;
 		x->delay.unit = SPI_DELAY_UNIT_USECS;
-
 		x++;
-		x->tx_buf = &packet->read_y;
-		x->len = 1;
-		spi_message_add_tail(x, m);
 
-		x++;
+		x->tx_buf = &packet->read_y_cmd;
 		x->rx_buf = &packet->tc.y;
-		x->len = 2;
+		x->len = 3;
 		spi_message_add_tail(x, m);
 	}
 
@@ -1054,28 +1034,13 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 	spi_message_init(m);
 	m->context = ts;
 
-	if (ts->model == 7845) {
-		x++;
-		packet->read_x_cmd[0] = READ_X(vref);
-		packet->read_x_cmd[1] = 0;
-		packet->read_x_cmd[2] = 0;
-		x->tx_buf = &packet->read_x_cmd[0];
-		x->rx_buf = &packet->tc.x_buf[0];
-		x->len = 3;
-		spi_message_add_tail(x, m);
-	} else {
-		/* turn y- off, x+ on, then leave in lowpower */
-		x++;
-		packet->read_x = READ_X(vref);
-		x->tx_buf = &packet->read_x;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
-		x->rx_buf = &packet->tc.x;
-		x->len = 2;
-		spi_message_add_tail(x, m);
-	}
+	/* turn y- off, x+ on, then leave in lowpower */
+	x++;
+	packet->read_x_cmd.cmd = READ_X(vref);
+	x->tx_buf = &packet->read_x_cmd;
+	x->rx_buf = &packet->tc.x;
+	x->len = 3;
+	spi_message_add_tail(x, m);
 
 	/* ... maybe discard first sample ... */
 	if (pdata->settle_delay_usecs) {
@@ -1083,13 +1048,9 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 		x->delay.unit = SPI_DELAY_UNIT_USECS;
 
 		x++;
-		x->tx_buf = &packet->read_x;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
+		x->tx_buf = &packet->read_x_cmd;
 		x->rx_buf = &packet->tc.x;
-		x->len = 2;
+		x->len = 3;
 		spi_message_add_tail(x, m);
 	}
 
@@ -1101,14 +1062,10 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 		m->context = ts;
 
 		x++;
-		packet->read_z1 = READ_Z1(vref);
-		x->tx_buf = &packet->read_z1;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
+		packet->read_z1_cmd.cmd = READ_Z1(vref);
+		x->tx_buf = &packet->read_z1_cmd;
 		x->rx_buf = &packet->tc.z1;
-		x->len = 2;
+		x->len = 3;
 		spi_message_add_tail(x, m);
 
 		/* ... maybe discard first sample ... */
@@ -1117,13 +1074,9 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 			x->delay.unit = SPI_DELAY_UNIT_USECS;
 
 			x++;
-			x->tx_buf = &packet->read_z1;
-			x->len = 1;
-			spi_message_add_tail(x, m);
-
-			x++;
+			x->tx_buf = &packet->read_z1_cmd;
 			x->rx_buf = &packet->tc.z1;
-			x->len = 2;
+			x->len = 3;
 			spi_message_add_tail(x, m);
 		}
 
@@ -1133,14 +1086,10 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 		m->context = ts;
 
 		x++;
-		packet->read_z2 = READ_Z2(vref);
-		x->tx_buf = &packet->read_z2;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
+		packet->read_z2_cmd.cmd = READ_Z2(vref);
+		x->tx_buf = &packet->read_z2_cmd;
 		x->rx_buf = &packet->tc.z2;
-		x->len = 2;
+		x->len = 3;
 		spi_message_add_tail(x, m);
 
 		/* ... maybe discard first sample ... */
@@ -1149,13 +1098,9 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 			x->delay.unit = SPI_DELAY_UNIT_USECS;
 
 			x++;
-			x->tx_buf = &packet->read_z2;
-			x->len = 1;
-			spi_message_add_tail(x, m);
-
-			x++;
+			x->tx_buf = &packet->read_z2_cmd;
 			x->rx_buf = &packet->tc.z2;
-			x->len = 2;
+			x->len = 3;
 			spi_message_add_tail(x, m);
 		}
 	}
@@ -1166,24 +1111,10 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 	spi_message_init(m);
 	m->context = ts;
 
-	if (ts->model == 7845) {
-		x++;
-		packet->pwrdown_cmd[0] = PWRDOWN;
-		packet->pwrdown_cmd[1] = 0;
-		packet->pwrdown_cmd[2] = 0;
-		x->tx_buf = &packet->pwrdown_cmd[0];
-		x->len = 3;
-	} else {
-		x++;
-		packet->pwrdown = PWRDOWN;
-		x->tx_buf = &packet->pwrdown;
-		x->len = 1;
-		spi_message_add_tail(x, m);
-
-		x++;
-		x->rx_buf = &packet->dummy;
-		x->len = 2;
-	}
+	x++;
+	packet->pwrdown_cmd.cmd = PWRDOWN;
+	x->tx_buf = &packet->pwrdown_cmd;
+	x->len = 3;
 
 	CS_CHANGE(*x);
 	spi_message_add_tail(x, m);
-- 
2.28.0

[PATCH v1 2/2] Input: ads7846: convert to one message

[Oleksij Rempel]

Convert multiple full duplex transfers in to a single transfer.

Current driver version support two modes:
- not filtered
- driver specific debounce filter
- platform specific debounce filter (do any platform provides such
filter?)

Without filter this HW is not really usable, since the physic of
resistive touchscreen can provide some bounce effects. With filter, we
have constant amount of retries + debounce retries if some anomaly was
detected.

This patch create one SPI transfer with all fields and not optional retires. If
bounce anomaly was detected, we will make more transfer if needed.

Without this patch, we will get about 10% CPU load on iMX6S if some thing
is pressing on the screen (holding finger, etc.)

With this patch, depending in the amount of retries, the CPU load will
be 1.5-2% with "ti,debounce-rep = <3>" and 1% or less with
"ti,debounce-rep = <10>". Depending on the buffer size, different SPI
controllers use different optimizations. On iMX, the buffer below
64 Byte will be transfered in the PIO mode and beyond this threshold,
it will be transfered in the DMA mode.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/input/touchscreen/ads7846.c | 310 +++++++++++++---------------
 1 file changed, 143 insertions(+), 167 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index e9a520c9ad69..a68b9e5aa505 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -64,37 +64,25 @@
 
 struct ads7846_buf {
 	u8 cmd;
-	/* This union is a temporary hack. The driver does an in-place
-	 * endianness conversion. This will be cleaned up in the next
-	 * patch.
-	 */
-	union {
-		__be16 data_be16;
-		u16 data;
-	};
+	__be16 data;
 } __attribute__((__packed__));
 
-
-struct ts_event {
-	bool ignore;
-	struct ads7846_buf x;
-	struct ads7846_buf y;
-	struct ads7846_buf z1;
-	struct ads7846_buf z2;
-};
-
 /*
  * We allocate this separately to avoid cache line sharing issues when
  * driver is used with DMA-based SPI controllers (like atmel_spi) on
  * systems where main memory is not DMA-coherent (most non-x86 boards).
  */
 struct ads7846_packet {
-	struct ts_event tc;
-	struct ads7846_buf read_x_cmd;
-	struct ads7846_buf read_y_cmd;
-	struct ads7846_buf read_z1_cmd;
-	struct ads7846_buf read_z2_cmd;
+	unsigned int count;
+	unsigned int fields;
+	unsigned int last_field;
+	struct ads7846_buf *rx;
+	struct ads7846_buf *tx;
+
 	struct ads7846_buf pwrdown_cmd;
+
+	bool ignore;
+	u16 x, y, z1, z2;
 };
 
 struct ads7846 {
@@ -206,6 +194,13 @@ struct ads7846 {
 #define	REF_ON	(READ_12BIT_DFR(x, 1, 1))
 #define	REF_OFF	(READ_12BIT_DFR(y, 0, 0))
 
+enum ads7846_cmds {
+	ADS7846_Y,
+	ADS7846_X,
+	ADS7846_Z1,
+	ADS7846_Z2,
+};
+
 static int get_pendown_state(struct ads7846 *ts)
 {
 	if (ts->get_pendown_state)
@@ -696,26 +691,68 @@ static int ads7846_no_filter(void *ads, int data_idx, int *val)
 	return ADS7846_FILTER_OK;
 }
 
-static int ads7846_get_value(struct ads7846 *ts, struct spi_message *m)
+static int ads7846_get_value(struct ads7846_buf *buf)
 {
 	int value;
-	struct spi_transfer *t =
-		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
-	struct ads7846_buf *buf = t->rx_buf;
 
-	value = be16_to_cpup(&buf->data_be16);
+	value = be16_to_cpup(&buf->data);
 
 	/* enforce ADC output is 12 bits width */
 	return (value >> 3) & 0xfff;
 }
 
-static void ads7846_update_value(struct spi_message *m, int val)
+static void ads7846_set_field_val(struct ads7846 *ts, enum ads7846_cmds filed,
+				  u16 val)
 {
-	struct spi_transfer *t =
-		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
-	struct ads7846_buf *buf = t->rx_buf;
+	struct ads7846_packet *packet = ts->packet;
+
+	switch (filed) {
+	case ADS7846_Y:
+		packet->y = val;
+		break;
+	case ADS7846_X:
+		packet->x = val;
+		break;
+	case ADS7846_Z1:
+		packet->z1 = val;
+		break;
+	case ADS7846_Z2:
+		packet->z2 = val;
+		break;
+	default:
+		pr_err("kappuuuut auch!!!\n");
+	}
+}
+
+static int ads7846_filter_state(struct ads7846 *ts)
+{
+	struct ads7846_packet *packet = ts->packet;
+	int msg_idx = 0;
+	int action;
+	int val;
+	unsigned int a, b;
 
-	buf->data = val;
+	packet->ignore = false;
+	for (a = packet->last_field; a < packet->fields; a++) {
+		packet->last_field = a;
+		for (b = 0; b < packet->count; b++) {
+			val = ads7846_get_value(&packet->rx[b * packet->fields + a]);
+
+			action = ts->filter(ts->filter_data, msg_idx, &val);
+			if (action == ADS7846_FILTER_REPEAT) {
+				if (b == packet->count - 1)
+					return -EAGAIN;
+			} else if (action == ADS7846_FILTER_OK) {
+				ads7846_set_field_val(ts, a, val);
+				break;
+			} else {
+				packet->ignore = true;
+				return 0;
+			}
+		}
+	}
+
+	return 0;
 }
 
 static void ads7846_read_state(struct ads7846 *ts)
@@ -723,52 +760,30 @@ static void ads7846_read_state(struct ads7846 *ts)
 	struct ads7846_packet *packet = ts->packet;
 	struct spi_message *m;
 	int msg_idx = 0;
-	int val;
-	int action;
 	int error;
 
-	while (msg_idx < ts->msg_count) {
+	packet->last_field = 0;
 
+	while (msg_idx < ts->msg_count) {
 		ts->wait_for_sync();
 
 		m = &ts->msg[msg_idx];
 		error = spi_sync(ts->spi, m);
 		if (error) {
 			dev_err(&ts->spi->dev, "spi_sync --> %d\n", error);
-			packet->tc.ignore = true;
+			packet->ignore = true;
 			return;
 		}
 
-		/*
-		 * Last message is power down request, no need to convert
-		 * or filter the value.
-		 */
-		if (msg_idx < ts->msg_count - 1) {
+		/* last message is power down request */
+		if (msg_idx == ts->msg_count - 1)
+			break;
 
-			val = ads7846_get_value(ts, m);
+		error = ads7846_filter_state(ts);
+		if (error)
+			continue;
 
-			action = ts->filter(ts->filter_data, msg_idx, &val);
-			switch (action) {
-			case ADS7846_FILTER_REPEAT:
-				continue;
-
-			case ADS7846_FILTER_IGNORE:
-				packet->tc.ignore = true;
-				msg_idx = ts->msg_count - 1;
-				continue;
-
-			case ADS7846_FILTER_OK:
-				ads7846_update_value(m, val);
-				packet->tc.ignore = false;
-				msg_idx++;
-				break;
-
-			default:
-				BUG();
-			}
-		} else {
-			msg_idx++;
-		}
+		msg_idx++;
 	}
 }
 
@@ -783,14 +798,14 @@ static void ads7846_report_state(struct ads7846 *ts)
 	 * from on-the-wire format as part of debouncing to get stable
 	 * readings.
 	 */
-	x = packet->tc.x.data;
-	y = packet->tc.y.data;
+	x = packet->x;
+	y = packet->y;
 	if (ts->model == 7845) {
 		z1 = 0;
 		z2 = 0;
 	} else {
-		z1 = packet->tc.z1.data;
-		z2 = packet->tc.z2.data;
+		z1 = packet->z1;
+		z2 = packet->z2;
 	}
 
 	/* range filtering */
@@ -822,9 +837,9 @@ static void ads7846_report_state(struct ads7846 *ts)
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement
 	 */
-	if (packet->tc.ignore || Rt > ts->pressure_max) {
+	if (packet->ignore || Rt > ts->pressure_max) {
 		dev_vdbg(&ts->spi->dev, "ignored %d pressure %d\n",
-			 packet->tc.ignore, Rt);
+			 packet->ignore, Rt);
 		return;
 	}
 
@@ -981,17 +996,62 @@ static int ads7846_setup_pendown(struct spi_device *spi,
 	return 0;
 }
 
+
+static u8 ads7846_get_cmd(enum ads7846_cmds filed, int vref)
+{
+	switch (filed) {
+	case ADS7846_Y:
+		return READ_Y(vref);
+	case ADS7846_X:
+		return READ_X(vref);
+
+	/* 7846 specific commands  */
+	case ADS7846_Z1:
+		return READ_Z1(vref);
+	case ADS7846_Z2:
+		return READ_Z2(vref);
+	default:
+		pr_err("kappuuuut!!!\n");
+	}
+
+	return 0;
+}
+
 /*
  * Set up the transfers to read touchscreen state; this assumes we
  * use formula #2 for pressure, not #3.
  */
-static void ads7846_setup_spi_msg(struct ads7846 *ts,
+static int ads7846_setup_spi_msg(struct ads7846 *ts,
 				  const struct ads7846_platform_data *pdata)
 {
 	struct spi_message *m = &ts->msg[0];
 	struct spi_transfer *x = ts->xfer;
 	struct ads7846_packet *packet = ts->packet;
 	int vref = pdata->keep_vref_on;
+	unsigned int a, b;
+	size_t size;
+
+	if (ts->debounce_max && ts->debounce_rep)
+		/* ads7846_debounce_filter() is making ts->debounce_rep + 2
+		 * reads. So we need to get all samples for normal case. */
+		packet->count = ts->debounce_rep + 2;
+	else
+		packet->count = 1;
+
+	if (ts->model == 7846)
+		packet->fields = 4; /* x, y, z1, z2 */
+	else
+		packet->fields = 2; /* x, y */
+
+	size = sizeof(*packet->tx) * packet->count * packet->fields;
+
+	packet->tx = devm_kzalloc(&ts->spi->dev, size, GFP_KERNEL);
+	if (!packet->tx)
+		return -ENOMEM;
+
+	packet->rx = devm_kzalloc(&ts->spi->dev, size, GFP_KERNEL);
+	if (!packet->rx)
+		return -ENOMEM;
 
 	if (ts->model == 7873) {
 		/*
@@ -1007,110 +1067,24 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 	spi_message_init(m);
 	m->context = ts;
 
-	packet->read_y_cmd.cmd = READ_Y(vref);
-	x->tx_buf = &packet->read_y_cmd;
-	x->rx_buf = &packet->tc.y;
-	x->len = 3;
-	spi_message_add_tail(x, m);
-
-	/*
-	 * The first sample after switching drivers can be low quality;
-	 * optionally discard it, using a second one after the signals
-	 * have had enough time to stabilize.
-	 */
-	if (pdata->settle_delay_usecs) {
-		x->delay.value = pdata->settle_delay_usecs;
-		x->delay.unit = SPI_DELAY_UNIT_USECS;
-		x++;
-
-		x->tx_buf = &packet->read_y_cmd;
-		x->rx_buf = &packet->tc.y;
-		x->len = 3;
-		spi_message_add_tail(x, m);
+	for (a = 0; a < packet->count; a++) {
+		for (b = 0; b < packet->fields; b++) {
+			u8 cmd = ads7846_get_cmd(b, vref);
+			packet->tx[a * packet->fields + b].cmd = cmd;
+		}
 	}
 
-	ts->msg_count++;
-	m++;
-	spi_message_init(m);
-	m->context = ts;
-
-	/* turn y- off, x+ on, then leave in lowpower */
-	x++;
-	packet->read_x_cmd.cmd = READ_X(vref);
-	x->tx_buf = &packet->read_x_cmd;
-	x->rx_buf = &packet->tc.x;
-	x->len = 3;
+	x->tx_buf = packet->tx;
+	x->rx_buf = packet->rx;
+	x->len = size;
+	x->cs_change = 1; /* do not set CS until we do the PWRDOWN */
 	spi_message_add_tail(x, m);
 
-	/* ... maybe discard first sample ... */
-	if (pdata->settle_delay_usecs) {
-		x->delay.value = pdata->settle_delay_usecs;
-		x->delay.unit = SPI_DELAY_UNIT_USECS;
-
-		x++;
-		x->tx_buf = &packet->read_x_cmd;
-		x->rx_buf = &packet->tc.x;
-		x->len = 3;
-		spi_message_add_tail(x, m);
-	}
-
-	/* turn y+ off, x- on; we'll use formula #2 */
-	if (ts->model == 7846) {
-		ts->msg_count++;
-		m++;
-		spi_message_init(m);
-		m->context = ts;
-
-		x++;
-		packet->read_z1_cmd.cmd = READ_Z1(vref);
-		x->tx_buf = &packet->read_z1_cmd;
-		x->rx_buf = &packet->tc.z1;
-		x->len = 3;
-		spi_message_add_tail(x, m);
-
-		/* ... maybe discard first sample ... */
-		if (pdata->settle_delay_usecs) {
-			x->delay.value = pdata->settle_delay_usecs;
-			x->delay.unit = SPI_DELAY_UNIT_USECS;
-
-			x++;
-			x->tx_buf = &packet->read_z1_cmd;
-			x->rx_buf = &packet->tc.z1;
-			x->len = 3;
-			spi_message_add_tail(x, m);
-		}
-
-		ts->msg_count++;
-		m++;
-		spi_message_init(m);
-		m->context = ts;
-
-		x++;
-		packet->read_z2_cmd.cmd = READ_Z2(vref);
-		x->tx_buf = &packet->read_z2_cmd;
-		x->rx_buf = &packet->tc.z2;
-		x->len = 3;
-		spi_message_add_tail(x, m);
-
-		/* ... maybe discard first sample ... */
-		if (pdata->settle_delay_usecs) {
-			x->delay.value = pdata->settle_delay_usecs;
-			x->delay.unit = SPI_DELAY_UNIT_USECS;
-
-			x++;
-			x->tx_buf = &packet->read_z2_cmd;
-			x->rx_buf = &packet->tc.z2;
-			x->len = 3;
-			spi_message_add_tail(x, m);
-		}
-	}
-
-	/* power down */
 	ts->msg_count++;
 	m++;
 	spi_message_init(m);
 	m->context = ts;
-
+	/* power down */
 	x++;
 	packet->pwrdown_cmd.cmd = PWRDOWN;
 	x->tx_buf = &packet->pwrdown_cmd;
@@ -1118,6 +1092,8 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
 
 	CS_CHANGE(*x);
 	spi_message_add_tail(x, m);
+
+	return 0;
 }
 
 #ifdef CONFIG_OF
-- 
2.28.0

Re: [PATCH v1 2/2] Input: ads7846: convert to one message

[Oleksij Rempel]

Hello all,

This patch will need some more work. Currently, it is ignoring
ti,settle-delay-usec property, which should be fixed in the next version.

Regards,
Oleksij

On Wed, Nov 04, 2020 at 04:35:28PM +0100, Oleksij Rempel wrote:
> Convert multiple full duplex transfers in to a single transfer.
> 
> Current driver version support two modes:
> - not filtered
> - driver specific debounce filter
> - platform specific debounce filter (do any platform provides such
> filter?)
> 
> Without filter this HW is not really usable, since the physic of
> resistive touchscreen can provide some bounce effects. With filter, we
> have constant amount of retries + debounce retries if some anomaly was
> detected.
> 
> This patch create one SPI transfer with all fields and not optional retires. If
> bounce anomaly was detected, we will make more transfer if needed.
> 
> Without this patch, we will get about 10% CPU load on iMX6S if some thing
> is pressing on the screen (holding finger, etc.)
> 
> With this patch, depending in the amount of retries, the CPU load will
> be 1.5-2% with "ti,debounce-rep = <3>" and 1% or less with
> "ti,debounce-rep = <10>". Depending on the buffer size, different SPI
> controllers use different optimizations. On iMX, the buffer below
> 64 Byte will be transfered in the PIO mode and beyond this threshold,
> it will be transfered in the DMA mode.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/input/touchscreen/ads7846.c | 310 +++++++++++++---------------
>  1 file changed, 143 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index e9a520c9ad69..a68b9e5aa505 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -64,37 +64,25 @@
>  
>  struct ads7846_buf {
>  	u8 cmd;
> -	/* This union is a temporary hack. The driver does an in-place
> -	 * endianness conversion. This will be cleaned up in the next
> -	 * patch.
> -	 */
> -	union {
> -		__be16 data_be16;
> -		u16 data;
> -	};
> +	__be16 data;
>  } __attribute__((__packed__));
>  
> -
> -struct ts_event {
> -	bool ignore;
> -	struct ads7846_buf x;
> -	struct ads7846_buf y;
> -	struct ads7846_buf z1;
> -	struct ads7846_buf z2;
> -};
> -
>  /*
>   * We allocate this separately to avoid cache line sharing issues when
>   * driver is used with DMA-based SPI controllers (like atmel_spi) on
>   * systems where main memory is not DMA-coherent (most non-x86 boards).
>   */
>  struct ads7846_packet {
> -	struct ts_event tc;
> -	struct ads7846_buf read_x_cmd;
> -	struct ads7846_buf read_y_cmd;
> -	struct ads7846_buf read_z1_cmd;
> -	struct ads7846_buf read_z2_cmd;
> +	unsigned int count;
> +	unsigned int fields;
> +	unsigned int last_field;
> +	struct ads7846_buf *rx;
> +	struct ads7846_buf *tx;
> +
>  	struct ads7846_buf pwrdown_cmd;
> +
> +	bool ignore;
> +	u16 x, y, z1, z2;
>  };
>  
>  struct ads7846 {
> @@ -206,6 +194,13 @@ struct ads7846 {
>  #define	REF_ON	(READ_12BIT_DFR(x, 1, 1))
>  #define	REF_OFF	(READ_12BIT_DFR(y, 0, 0))
>  
> +enum ads7846_cmds {
> +	ADS7846_Y,
> +	ADS7846_X,
> +	ADS7846_Z1,
> +	ADS7846_Z2,
> +};
> +
>  static int get_pendown_state(struct ads7846 *ts)
>  {
>  	if (ts->get_pendown_state)
> @@ -696,26 +691,68 @@ static int ads7846_no_filter(void *ads, int data_idx, int *val)
>  	return ADS7846_FILTER_OK;
>  }
>  
> -static int ads7846_get_value(struct ads7846 *ts, struct spi_message *m)
> +static int ads7846_get_value(struct ads7846_buf *buf)
>  {
>  	int value;
> -	struct spi_transfer *t =
> -		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
> -	struct ads7846_buf *buf = t->rx_buf;
>  
> -	value = be16_to_cpup(&buf->data_be16);
> +	value = be16_to_cpup(&buf->data);
>  
>  	/* enforce ADC output is 12 bits width */
>  	return (value >> 3) & 0xfff;
>  }
>  
> -static void ads7846_update_value(struct spi_message *m, int val)
> +static void ads7846_set_field_val(struct ads7846 *ts, enum ads7846_cmds filed,
> +				  u16 val)
>  {
> -	struct spi_transfer *t =
> -		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
> -	struct ads7846_buf *buf = t->rx_buf;
> +	struct ads7846_packet *packet = ts->packet;
> +
> +	switch (filed) {
> +	case ADS7846_Y:
> +		packet->y = val;
> +		break;
> +	case ADS7846_X:
> +		packet->x = val;
> +		break;
> +	case ADS7846_Z1:
> +		packet->z1 = val;
> +		break;
> +	case ADS7846_Z2:
> +		packet->z2 = val;
> +		break;
> +	default:
> +		pr_err("kappuuuut auch!!!\n");
> +	}
> +}
> +
> +static int ads7846_filter_state(struct ads7846 *ts)
> +{
> +	struct ads7846_packet *packet = ts->packet;
> +	int msg_idx = 0;
> +	int action;
> +	int val;
> +	unsigned int a, b;
>  
> -	buf->data = val;
> +	packet->ignore = false;
> +	for (a = packet->last_field; a < packet->fields; a++) {
> +		packet->last_field = a;
> +		for (b = 0; b < packet->count; b++) {
> +			val = ads7846_get_value(&packet->rx[b * packet->fields + a]);
> +
> +			action = ts->filter(ts->filter_data, msg_idx, &val);
> +			if (action == ADS7846_FILTER_REPEAT) {
> +				if (b == packet->count - 1)
> +					return -EAGAIN;
> +			} else if (action == ADS7846_FILTER_OK) {
> +				ads7846_set_field_val(ts, a, val);
> +				break;
> +			} else {
> +				packet->ignore = true;
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static void ads7846_read_state(struct ads7846 *ts)
> @@ -723,52 +760,30 @@ static void ads7846_read_state(struct ads7846 *ts)
>  	struct ads7846_packet *packet = ts->packet;
>  	struct spi_message *m;
>  	int msg_idx = 0;
> -	int val;
> -	int action;
>  	int error;
>  
> -	while (msg_idx < ts->msg_count) {
> +	packet->last_field = 0;
>  
> +	while (msg_idx < ts->msg_count) {
>  		ts->wait_for_sync();
>  
>  		m = &ts->msg[msg_idx];
>  		error = spi_sync(ts->spi, m);
>  		if (error) {
>  			dev_err(&ts->spi->dev, "spi_sync --> %d\n", error);
> -			packet->tc.ignore = true;
> +			packet->ignore = true;
>  			return;
>  		}
>  
> -		/*
> -		 * Last message is power down request, no need to convert
> -		 * or filter the value.
> -		 */
> -		if (msg_idx < ts->msg_count - 1) {
> +		/* last message is power down request */
> +		if (msg_idx == ts->msg_count - 1)
> +			break;
>  
> -			val = ads7846_get_value(ts, m);
> +		error = ads7846_filter_state(ts);
> +		if (error)
> +			continue;
>  
> -			action = ts->filter(ts->filter_data, msg_idx, &val);
> -			switch (action) {
> -			case ADS7846_FILTER_REPEAT:
> -				continue;
> -
> -			case ADS7846_FILTER_IGNORE:
> -				packet->tc.ignore = true;
> -				msg_idx = ts->msg_count - 1;
> -				continue;
> -
> -			case ADS7846_FILTER_OK:
> -				ads7846_update_value(m, val);
> -				packet->tc.ignore = false;
> -				msg_idx++;
> -				break;
> -
> -			default:
> -				BUG();
> -			}
> -		} else {
> -			msg_idx++;
> -		}
> +		msg_idx++;
>  	}
>  }
>  
> @@ -783,14 +798,14 @@ static void ads7846_report_state(struct ads7846 *ts)
>  	 * from on-the-wire format as part of debouncing to get stable
>  	 * readings.
>  	 */
> -	x = packet->tc.x.data;
> -	y = packet->tc.y.data;
> +	x = packet->x;
> +	y = packet->y;
>  	if (ts->model == 7845) {
>  		z1 = 0;
>  		z2 = 0;
>  	} else {
> -		z1 = packet->tc.z1.data;
> -		z2 = packet->tc.z2.data;
> +		z1 = packet->z1;
> +		z2 = packet->z2;
>  	}
>  
>  	/* range filtering */
> @@ -822,9 +837,9 @@ static void ads7846_report_state(struct ads7846 *ts)
>  	 * the maximum. Don't report it to user space, repeat at least
>  	 * once more the measurement
>  	 */
> -	if (packet->tc.ignore || Rt > ts->pressure_max) {
> +	if (packet->ignore || Rt > ts->pressure_max) {
>  		dev_vdbg(&ts->spi->dev, "ignored %d pressure %d\n",
> -			 packet->tc.ignore, Rt);
> +			 packet->ignore, Rt);
>  		return;
>  	}
>  
> @@ -981,17 +996,62 @@ static int ads7846_setup_pendown(struct spi_device *spi,
>  	return 0;
>  }
>  
> +
> +static u8 ads7846_get_cmd(enum ads7846_cmds filed, int vref)
> +{
> +	switch (filed) {
> +	case ADS7846_Y:
> +		return READ_Y(vref);
> +	case ADS7846_X:
> +		return READ_X(vref);
> +
> +	/* 7846 specific commands  */
> +	case ADS7846_Z1:
> +		return READ_Z1(vref);
> +	case ADS7846_Z2:
> +		return READ_Z2(vref);
> +	default:
> +		pr_err("kappuuuut!!!\n");
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Set up the transfers to read touchscreen state; this assumes we
>   * use formula #2 for pressure, not #3.
>   */
> -static void ads7846_setup_spi_msg(struct ads7846 *ts,
> +static int ads7846_setup_spi_msg(struct ads7846 *ts,
>  				  const struct ads7846_platform_data *pdata)
>  {
>  	struct spi_message *m = &ts->msg[0];
>  	struct spi_transfer *x = ts->xfer;
>  	struct ads7846_packet *packet = ts->packet;
>  	int vref = pdata->keep_vref_on;
> +	unsigned int a, b;
> +	size_t size;
> +
> +	if (ts->debounce_max && ts->debounce_rep)
> +		/* ads7846_debounce_filter() is making ts->debounce_rep + 2
> +		 * reads. So we need to get all samples for normal case. */
> +		packet->count = ts->debounce_rep + 2;
> +	else
> +		packet->count = 1;
> +
> +	if (ts->model == 7846)
> +		packet->fields = 4; /* x, y, z1, z2 */
> +	else
> +		packet->fields = 2; /* x, y */
> +
> +	size = sizeof(*packet->tx) * packet->count * packet->fields;
> +
> +	packet->tx = devm_kzalloc(&ts->spi->dev, size, GFP_KERNEL);
> +	if (!packet->tx)
> +		return -ENOMEM;
> +
> +	packet->rx = devm_kzalloc(&ts->spi->dev, size, GFP_KERNEL);
> +	if (!packet->rx)
> +		return -ENOMEM;
>  
>  	if (ts->model == 7873) {
>  		/*
> @@ -1007,110 +1067,24 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
>  	spi_message_init(m);
>  	m->context = ts;
>  
> -	packet->read_y_cmd.cmd = READ_Y(vref);
> -	x->tx_buf = &packet->read_y_cmd;
> -	x->rx_buf = &packet->tc.y;
> -	x->len = 3;
> -	spi_message_add_tail(x, m);
> -
> -	/*
> -	 * The first sample after switching drivers can be low quality;
> -	 * optionally discard it, using a second one after the signals
> -	 * have had enough time to stabilize.
> -	 */
> -	if (pdata->settle_delay_usecs) {
> -		x->delay.value = pdata->settle_delay_usecs;
> -		x->delay.unit = SPI_DELAY_UNIT_USECS;
> -		x++;
> -
> -		x->tx_buf = &packet->read_y_cmd;
> -		x->rx_buf = &packet->tc.y;
> -		x->len = 3;
> -		spi_message_add_tail(x, m);
> +	for (a = 0; a < packet->count; a++) {
> +		for (b = 0; b < packet->fields; b++) {
> +			u8 cmd = ads7846_get_cmd(b, vref);
> +			packet->tx[a * packet->fields + b].cmd = cmd;
> +		}
>  	}
>  
> -	ts->msg_count++;
> -	m++;
> -	spi_message_init(m);
> -	m->context = ts;
> -
> -	/* turn y- off, x+ on, then leave in lowpower */
> -	x++;
> -	packet->read_x_cmd.cmd = READ_X(vref);
> -	x->tx_buf = &packet->read_x_cmd;
> -	x->rx_buf = &packet->tc.x;
> -	x->len = 3;
> +	x->tx_buf = packet->tx;
> +	x->rx_buf = packet->rx;
> +	x->len = size;
> +	x->cs_change = 1; /* do not set CS until we do the PWRDOWN */
>  	spi_message_add_tail(x, m);
>  
> -	/* ... maybe discard first sample ... */
> -	if (pdata->settle_delay_usecs) {
> -		x->delay.value = pdata->settle_delay_usecs;
> -		x->delay.unit = SPI_DELAY_UNIT_USECS;
> -
> -		x++;
> -		x->tx_buf = &packet->read_x_cmd;
> -		x->rx_buf = &packet->tc.x;
> -		x->len = 3;
> -		spi_message_add_tail(x, m);
> -	}
> -
> -	/* turn y+ off, x- on; we'll use formula #2 */
> -	if (ts->model == 7846) {
> -		ts->msg_count++;
> -		m++;
> -		spi_message_init(m);
> -		m->context = ts;
> -
> -		x++;
> -		packet->read_z1_cmd.cmd = READ_Z1(vref);
> -		x->tx_buf = &packet->read_z1_cmd;
> -		x->rx_buf = &packet->tc.z1;
> -		x->len = 3;
> -		spi_message_add_tail(x, m);
> -
> -		/* ... maybe discard first sample ... */
> -		if (pdata->settle_delay_usecs) {
> -			x->delay.value = pdata->settle_delay_usecs;
> -			x->delay.unit = SPI_DELAY_UNIT_USECS;
> -
> -			x++;
> -			x->tx_buf = &packet->read_z1_cmd;
> -			x->rx_buf = &packet->tc.z1;
> -			x->len = 3;
> -			spi_message_add_tail(x, m);
> -		}
> -
> -		ts->msg_count++;
> -		m++;
> -		spi_message_init(m);
> -		m->context = ts;
> -
> -		x++;
> -		packet->read_z2_cmd.cmd = READ_Z2(vref);
> -		x->tx_buf = &packet->read_z2_cmd;
> -		x->rx_buf = &packet->tc.z2;
> -		x->len = 3;
> -		spi_message_add_tail(x, m);
> -
> -		/* ... maybe discard first sample ... */
> -		if (pdata->settle_delay_usecs) {
> -			x->delay.value = pdata->settle_delay_usecs;
> -			x->delay.unit = SPI_DELAY_UNIT_USECS;
> -
> -			x++;
> -			x->tx_buf = &packet->read_z2_cmd;
> -			x->rx_buf = &packet->tc.z2;
> -			x->len = 3;
> -			spi_message_add_tail(x, m);
> -		}
> -	}
> -
> -	/* power down */
>  	ts->msg_count++;
>  	m++;
>  	spi_message_init(m);
>  	m->context = ts;
> -
> +	/* power down */
>  	x++;
>  	packet->pwrdown_cmd.cmd = PWRDOWN;
>  	x->tx_buf = &packet->pwrdown_cmd;
> @@ -1118,6 +1092,8 @@ static void ads7846_setup_spi_msg(struct ads7846 *ts,
>  
>  	CS_CHANGE(*x);
>  	spi_message_add_tail(x, m);
> +
> +	return 0;
>  }
>  
>  #ifdef CONFIG_OF
> -- 
> 2.28.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |