[PATCH 0/3] Input: st1232 - fixes for resolution reading

[PATCH 0/3] Input: st1232 - fixes for resolution reading

[Geert Uytterhoeven]

	Hi Dmitry, Andrej,

The recent commit 3a54a215410b1650 ("Input: st1232 - add support
resolution reading") broke probing of the st1232 touchscreen on
r8a7740/armadillo:

    st1232-ts 1-0055: Failed to read resolution: -6

Upon investigation, this turned out to be a little bit more complicated
than expected, hence I wrote not one but three fixes.

Thanks for your comments!

Geert Uytterhoeven (3):
  Input: st1232 - fix off-by-one error in resolution handling
  Input: st1232 - do not read more bytes than needed
  Input: st1232 - wait until device is ready before reading resolution

 drivers/input/touchscreen/st1232.c | 48 ++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 6 deletions(-)

-- 
2.25.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] Input: st1232 - fix off-by-one error in resolution handling

[Geert Uytterhoeven]

Before, the maximum coordinates were fixed to (799, 479) or (319, 479),
depending on touchscreen controller type.  The driver was changed to
read the actual values from the touchscreen controller, but did not take
into account the returned values are not the maximum coordinates, but
the touchscreen resolution (e.g. 800 and 480).

Fix this by subtracting 1.

Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/input/touchscreen/st1232.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index bda96762744e6da3..f18d4c7e03da17c6 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -85,8 +85,8 @@ static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
 
 	buf = ts->read_buf;
 
-	*max_x = ((buf[0] & 0x0070) << 4) | buf[1];
-	*max_y = ((buf[0] & 0x0007) << 8) | buf[2];
+	*max_x = (((buf[0] & 0x0070) << 4) | buf[1]) - 1;
+	*max_y = (((buf[0] & 0x0007) << 8) | buf[2]) - 1;
 
 	return 0;
 }
-- 
2.25.1

[PATCH 2/3] Input: st1232 - do not read more bytes than needed

[Geert Uytterhoeven]

st1232_ts_read_data() already reads ts->read_buf_len bytes (8 or 20
bytes) from the touchscreen controller.  This was fine when it was used
to read touch point coordinates only, but is overkill for reading the
touchscreen resolution, which just needs 3 bytes.

Optimize transfers by passing the wanted number of bytes.

Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/input/touchscreen/st1232.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index f18d4c7e03da17c6..459701056f2bda96 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -47,7 +47,8 @@ struct st1232_ts_data {
 	u8 *read_buf;
 };
 
-static int st1232_ts_read_data(struct st1232_ts_data *ts, u8 reg)
+static int st1232_ts_read_data(struct st1232_ts_data *ts, u8 reg,
+			       unsigned int n)
 {
 	struct i2c_client *client = ts->client;
 	struct i2c_msg msg[] = {
@@ -59,7 +60,7 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts, u8 reg)
 		{
 			.addr	= client->addr,
 			.flags	= I2C_M_RD | I2C_M_DMA_SAFE,
-			.len	= ts->read_buf_len,
+			.len	= n,
 			.buf	= ts->read_buf,
 		}
 	};
@@ -79,7 +80,7 @@ static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
 	int error;
 
 	/* select resolution register */
-	error = st1232_ts_read_data(ts, REG_XY_RESOLUTION);
+	error = st1232_ts_read_data(ts, REG_XY_RESOLUTION, 3);
 	if (error)
 		return error;
 
@@ -140,7 +141,7 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
 	int count;
 	int error;
 
-	error = st1232_ts_read_data(ts, REG_XY_COORDINATES);
+	error = st1232_ts_read_data(ts, REG_XY_COORDINATES, ts->read_buf_len);
 	if (error)
 		goto out;
 
-- 
2.25.1

[PATCH 3/3] Input: st1232 - wait until device is ready before reading resolution

[Geert Uytterhoeven]

According to the st1232 datasheet, the host has to wait for the device
to change into Normal state before accessing registers other than the
Status Register.

If the reset GPIO is wired, the device is powered on during driver
probe, just before reading the resolution.  However, the latter may
happen before the device is ready, leading to a probe failure:

    st1232-ts 1-0055: Failed to read resolution: -6

Fix this by waiting until the device is ready, by trying to read the
Status Register until it indicates so, or until timeout.

On Armadillo 800 EVA, typically the first read fails with an I2C
transfer error, while the second read indicates the device is ready.

Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Adding an mdelay(1) before reading the resolution also seems to work.
But as the datasheet doesn't mention timing w.r.t. power on reset, I
think it's better to follow the documented procedure.
---
 drivers/input/touchscreen/st1232.c | 35 ++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 459701056f2bda96..47e5b3ac5bb053f6 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -26,6 +26,20 @@
 #define ST1232_TS_NAME	"st1232-ts"
 #define ST1633_TS_NAME	"st1633-ts"
 
+#define REG_STATUS		0x01	/* Device Status | Error Code */
+
+#define STATUS_NORMAL		0x00
+#define STATUS_INIT		0x01
+#define STATUS_ERROR		0x02
+#define STATUS_AUTO_TUNING	0x03
+#define STATUS_IDLE		0x04
+#define STATUS_POWER_DOWN	0x05
+
+#define ERROR_NONE		0x00
+#define ERROR_INVALID_ADDRESS	0x10
+#define ERROR_INVALID_VALUE	0x20
+#define ERROR_INVALID_PLATFORM	0x30
+
 #define REG_XY_RESOLUTION	0x04
 #define REG_XY_COORDINATES	0x12
 #define ST_TS_MAX_FINGERS	10
@@ -73,6 +87,22 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts, u8 reg,
 	return 0;
 }
 
+static int st1232_ts_wait_ready(struct st1232_ts_data *ts)
+{
+	unsigned int retries;
+	int ret;
+
+	for (retries = 10; retries; retries--) {
+		ret = st1232_ts_read_data(ts, REG_STATUS, 1);
+		if (!ret && ts->read_buf[0] == (STATUS_NORMAL | ERROR_NONE))
+			return 0;
+
+		usleep_range(1000, 2000);
+	}
+
+	return -ENXIO;
+}
+
 static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
 				     u16 *max_y)
 {
@@ -252,6 +282,11 @@ static int st1232_ts_probe(struct i2c_client *client,
 	input_dev->name = "st1232-touchscreen";
 	input_dev->id.bustype = BUS_I2C;
 
+	/* Wait until device is ready */
+	error = st1232_ts_wait_ready(ts);
+	if (error)
+		return error;
+
 	/* Read resolution from the chip */
 	error = st1232_ts_read_resolution(ts, &max_x, &max_y);
 	if (error) {
-- 
2.25.1

Re: [PATCH 1/3] Input: st1232 - fix off-by-one error in resolution handling

[Dmitry Torokhov]

On Tue, Dec 29, 2020 at 05:25:59PM +0100, Geert Uytterhoeven wrote:
> Before, the maximum coordinates were fixed to (799, 479) or (319, 479),
> depending on touchscreen controller type.  The driver was changed to
> read the actual values from the touchscreen controller, but did not take
> into account the returned values are not the maximum coordinates, but
> the touchscreen resolution (e.g. 800 and 480).
> 
> Fix this by subtracting 1.
> 
> Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied, thank you.

-- 
Dmitry

Re: [PATCH 2/3] Input: st1232 - do not read more bytes than needed

[Dmitry Torokhov]

On Tue, Dec 29, 2020 at 05:26:00PM +0100, Geert Uytterhoeven wrote:
> st1232_ts_read_data() already reads ts->read_buf_len bytes (8 or 20
> bytes) from the touchscreen controller.  This was fine when it was used
> to read touch point coordinates only, but is overkill for reading the
> touchscreen resolution, which just needs 3 bytes.
> 
> Optimize transfers by passing the wanted number of bytes.
> 
> Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied, thank you.

-- 
Dmitry

Re: [PATCH 3/3] Input: st1232 - wait until device is ready before reading resolution

[Dmitry Torokhov]

On Tue, Dec 29, 2020 at 05:26:01PM +0100, Geert Uytterhoeven wrote:
> According to the st1232 datasheet, the host has to wait for the device
> to change into Normal state before accessing registers other than the
> Status Register.
> 
> If the reset GPIO is wired, the device is powered on during driver
> probe, just before reading the resolution.  However, the latter may
> happen before the device is ready, leading to a probe failure:
> 
>     st1232-ts 1-0055: Failed to read resolution: -6
> 
> Fix this by waiting until the device is ready, by trying to read the
> Status Register until it indicates so, or until timeout.
> 
> On Armadillo 800 EVA, typically the first read fails with an I2C
> transfer error, while the second read indicates the device is ready.
> 
> Fixes: 3a54a215410b1650 ("Input: st1232 - add support resolution reading")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied, thank you.

-- 
Dmitry