Date:   Thu, 5 Nov 2020 15:40:10 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Mark Brown <broonie@...nel.org>
Cc:     kernel@...gutronix.de, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, linux-spi@...r.kernel.org,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v1 2/2] Input: ads7846: convert to one message

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@...gutronix.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 |

Powered by blists - more mailing lists