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

From: Oleksij Rempel
Date: Thu Nov 05 2020 - 09:40:17 EST

Next message: Boris Brezillon: "Re: [PATCH v5] mtd: spinand: micron: add support for MT29F2G01AAAED"
Previous message: Rob Herring: "Re: [PATCH v8 1/4] powerpc: Refactor kexec functions to move arch independent code to drivers/of"
In reply to: Oleksij Rempel: "[PATCH v1 2/2] Input: ads7846: convert to one message"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxx>
> ---
> 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 |

Next message: Boris Brezillon: "Re: [PATCH v5] mtd: spinand: micron: add support for MT29F2G01AAAED"
Previous message: Rob Herring: "Re: [PATCH v8 1/4] powerpc: Refactor kexec functions to move arch independent code to drivers/of"
In reply to: Oleksij Rempel: "[PATCH v1 2/2] Input: ads7846: convert to one message"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]