[PATCH 00/10] input: iqs5xx: Minor enhancements and optimizations

[PATCH 00/10] input: iqs5xx: Minor enhancements and optimizations

[Jeff LaBundy]

This series includes a variety of minor enhancements and optimizations to
the Azoteq IQS550/572/525 trackpad/touchscreen controller driver, some of
which are based on learnings during recent work with other Azoteq devices.

As a result, the driver has shrunk a bit despite having gained additional
functionality.

Jeff LaBundy (10):
  input: iqs5xx: Minor cosmetic improvements
  input: iqs5xx: Preserve bootloader errors
  input: iqs5xx: Accommodate bootloader latency
  input: iqs5xx: Expose firmware revision to user space
  input: iqs5xx: Re-initialize device upon warm reset
  input: iqs5xx: Simplify axis setup logic
  input: iqs5xx: Eliminate unnecessary register read
  input: iqs5xx: Allow more time for ATI to complete
  input: iqs5xx: Make reset GPIO optional
  input: iqs5xx: Allow device to be a wake-up source

 drivers/input/touchscreen/iqs5xx.c | 231 ++++++++++++++++++-------------------
 1 file changed, 110 insertions(+), 121 deletions(-)

--
2.7.4

[PATCH 01/10] input: iqs5xx: Minor cosmetic improvements

[Jeff LaBundy]

Copyrights are generally followed by the name of a person or a
company (i.e. the copyright holder) but that was not done here.
Fix this by squashing the 'copyright' and 'author' lines.

Also, trim some leading whitespace ahead of the parameters for
the fw_file_store() function and re-align them for readability.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 4fd21bc..08e79d6 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -2,8 +2,7 @@
 /*
  * Azoteq IQS550/572/525 Trackpad/Touchscreen Controller
  *
- * Copyright (C) 2018
- * Author: Jeff LaBundy <jeff@labundy.com>
+ * Copyright (C) 2018 Jeff LaBundy <jeff@labundy.com>
  *
  * These devices require firmware exported from a PC-based configuration tool
  * made available by the vendor. Firmware files may be pushed to the device's
@@ -952,8 +951,9 @@ static int iqs5xx_fw_file_write(struct i2c_client *client, const char *fw_file)
 	return error;
 }
 
-static ssize_t fw_file_store(struct device *dev, struct device_attribute *attr,
-				const char *buf, size_t count)
+static ssize_t fw_file_store(struct device *dev,
+			     struct device_attribute *attr, const char *buf,
+			     size_t count)
 {
 	struct iqs5xx_private *iqs5xx = dev_get_drvdata(dev);
 	struct i2c_client *client = iqs5xx->client;
-- 
2.7.4

Re: [PATCH 01/10] input: iqs5xx: Minor cosmetic improvements

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:37PM -0600, Jeff LaBundy wrote:
> Copyrights are generally followed by the name of a person or a
> company (i.e. the copyright holder) but that was not done here.
> Fix this by squashing the 'copyright' and 'author' lines.
> 
> Also, trim some leading whitespace ahead of the parameters for
> the fw_file_store() function and re-align them for readability.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 02/10] input: iqs5xx: Preserve bootloader errors

[Jeff LaBundy]

After user space writes the fw_file attribute to push new firmware
to the device, the driver calls iqs5xx_dev_init() to re-initialize
the device with the updated firmware or recover the device in case
the update failed.

In the case of the latter, however, iqs5xx_fw_file_write() returns
zero (success) so long as iqs5xx_dev_init() does not fail, and any
error encountered during the update process is lost. Solve this by
saving the error before calling iqs5xx_dev_init().

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 08e79d6..ff0a0e9 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -883,7 +883,7 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
 static int iqs5xx_fw_file_write(struct i2c_client *client, const char *fw_file)
 {
 	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
-	int error;
+	int error, error_bl;
 	u8 *pmap;
 
 	if (iqs5xx->bl_status == IQS5XX_BL_STATUS_NONE)
@@ -937,6 +937,7 @@ static int iqs5xx_fw_file_write(struct i2c_client *client, const char *fw_file)
 		usleep_range(10000, 10100);
 	}
 
+	error_bl = error;
 	error = iqs5xx_dev_init(client);
 	if (!error && iqs5xx->bl_status == IQS5XX_BL_STATUS_RESET)
 		error = -EINVAL;
@@ -948,6 +949,9 @@ static int iqs5xx_fw_file_write(struct i2c_client *client, const char *fw_file)
 err_kfree:
 	kfree(pmap);
 
+	if (error_bl)
+		return error_bl;
+
 	return error;
 }
 
-- 
2.7.4

Re: [PATCH 02/10] input: iqs5xx: Preserve bootloader errors

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:38PM -0600, Jeff LaBundy wrote:
> After user space writes the fw_file attribute to push new firmware
> to the device, the driver calls iqs5xx_dev_init() to re-initialize
> the device with the updated firmware or recover the device in case
> the update failed.
> 
> In the case of the latter, however, iqs5xx_fw_file_write() returns
> zero (success) so long as iqs5xx_dev_init() does not fail, and any
> error encountered during the update process is lost. Solve this by
> saving the error before calling iqs5xx_dev_init().
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 03/10] input: iqs5xx: Accommodate bootloader latency

[Jeff LaBundy]

The bootloader NAK's all I2C communication after the first 64-byte
bulk write if the bus frequency is equal to 400 kHz. This prevents
the platform from pushing updated firmware to the device.

The vendor's USB bootloader programming dongle appears to insert a
delay between the "open" command and the first 64-byte bulk write.
Adding a similar delay to the driver seems to eliminate the issue.

Furthermore, the dongle does not access the bootloader immediately
after powering up the device. Follow suit by adding a delay before
the "open" command to avoid wasted retries at 400 kHz.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index ff0a0e9..b2de8c67 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -336,11 +336,16 @@ static int iqs5xx_bl_open(struct i2c_client *client)
 	 */
 	for (i = 0; i < IQS5XX_BL_ATTEMPTS; i++) {
 		iqs5xx_reset(client);
+		usleep_range(350, 400);
 
 		for (j = 0; j < IQS5XX_NUM_RETRIES; j++) {
 			error = iqs5xx_bl_cmd(client, IQS5XX_BL_CMD_VER, 0);
-			if (!error || error == -EINVAL)
-				return error;
+			if (!error)
+				usleep_range(10000, 10100);
+			else if (error != -EINVAL)
+				continue;
+
+			return error;
 		}
 	}
 
-- 
2.7.4

Re: [PATCH 03/10] input: iqs5xx: Accommodate bootloader latency

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:39PM -0600, Jeff LaBundy wrote:
> The bootloader NAK's all I2C communication after the first 64-byte
> bulk write if the bus frequency is equal to 400 kHz. This prevents
> the platform from pushing updated firmware to the device.
> 
> The vendor's USB bootloader programming dongle appears to insert a
> delay between the "open" command and the first 64-byte bulk write.
> Adding a similar delay to the driver seems to eliminate the issue.
> 
> Furthermore, the dongle does not access the bootloader immediately
> after powering up the device. Follow suit by adding a delay before
> the "open" command to avoid wasted retries at 400 kHz.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 04/10] input: iqs5xx: Expose firmware revision to user space

[Jeff LaBundy]

The device's firmware accommodates a revision field that customers
can assign when firmware is exported from the vendor's development
tool. Having the ability to read this field from user space can be
useful during development.

As such, promote the fw_file attribute from W/O to R/W. Writing to
the attribute pushes firmware to the device as normal, but reading
from it will now return the customer-assigned revision field as an
unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index b2de8c67..5ee22ab 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -64,6 +64,7 @@
 #define IQS5XX_XY_CFG0		0x0669
 #define IQS5XX_X_RES		0x066E
 #define IQS5XX_Y_RES		0x0670
+#define IQS5XX_EXP_FILE		0x0677
 #define IQS5XX_CHKSM		0x83C0
 #define IQS5XX_APP		0x8400
 #define IQS5XX_CSTM		0xBE00
@@ -99,6 +100,7 @@ struct iqs5xx_private {
 	struct input_dev *input;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock;
+	u16 exp_file;
 	u8 bl_status;
 };
 
@@ -666,6 +668,10 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 		return -EINVAL;
 	}
 
+	error = iqs5xx_read_word(client, IQS5XX_EXP_FILE, &iqs5xx->exp_file);
+	if (error)
+		return error;
+
 	error = iqs5xx_axis_init(client);
 	if (error)
 		return error;
@@ -1004,7 +1010,15 @@ static ssize_t fw_file_store(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR_WO(fw_file);
+static ssize_t fw_file_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct iqs5xx_private *iqs5xx = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", iqs5xx->exp_file);
+}
+
+static DEVICE_ATTR_RW(fw_file);
 
 static struct attribute *iqs5xx_attrs[] = {
 	&dev_attr_fw_file.attr,
-- 
2.7.4

Re: [PATCH 04/10] input: iqs5xx: Expose firmware revision to user space

[Dmitry Torokhov]

Hi Jeff,

On Mon, Jan 18, 2021 at 02:43:40PM -0600, Jeff LaBundy wrote:
> The device's firmware accommodates a revision field that customers
> can assign when firmware is exported from the vendor's development
> tool. Having the ability to read this field from user space can be
> useful during development.
> 
> As such, promote the fw_file attribute from W/O to R/W. Writing to
> the attribute pushes firmware to the device as normal, but reading
> from it will now return the customer-assigned revision field as an
> unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).

No, let's not overload this attribute and instead create a dedicated
fw_version or similar read-only attribute to expose active firmware
version to userspace.

Thanks.

-- 
Dmitry

Re: [PATCH 04/10] input: iqs5xx: Expose firmware revision to user space

[Jeff LaBundy]

Hi Dmitry,

Thank you for taking a look at this series and I do apologize for the
complaint from the bot this morning.

On Sun, Jan 24, 2021 at 08:22:01PM -0800, Dmitry Torokhov wrote:
> Hi Jeff,
> 
> On Mon, Jan 18, 2021 at 02:43:40PM -0600, Jeff LaBundy wrote:
> > The device's firmware accommodates a revision field that customers
> > can assign when firmware is exported from the vendor's development
> > tool. Having the ability to read this field from user space can be
> > useful during development.
> > 
> > As such, promote the fw_file attribute from W/O to R/W. Writing to
> > the attribute pushes firmware to the device as normal, but reading
> > from it will now return the customer-assigned revision field as an
> > unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).
> 
> No, let's not overload this attribute and instead create a dedicated
> fw_version or similar read-only attribute to expose active firmware
> version to userspace.

Not a problem; I'll create a new R/O attribute for this purpose.

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy

[PATCH 05/10] input: iqs5xx: Re-initialize device upon warm reset

[Jeff LaBundy]

The device may be inadvertently reset during runtime in the event
of ESD strike, etc. To protect against this case, acknowledge the
SHOW_RESET interrupt and re-initialize the device.

To facilitate this change, expand the range of registers that are
read in the interrupt handler to include the system status fields.

Also, update the unrelated (but nearby) SUSPEND register field to
use the BIT() macro. The remaining register fields are cleaned up
in another patch.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 51 ++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 5ee22ab..9412fb8 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -40,8 +40,11 @@
 #define IQS5XX_PROJ_NUM_B000	15
 #define IQS5XX_MAJOR_VER_MIN	2
 
-#define IQS5XX_RESUME		0x00
-#define IQS5XX_SUSPEND		0x01
+#define IQS5XX_SHOW_RESET	BIT(7)
+#define IQS5XX_ACK_RESET	BIT(7)
+
+#define IQS5XX_SUSPEND		BIT(0)
+#define IQS5XX_RESUME		0
 
 #define IQS5XX_SW_INPUT_EVENT	0x10
 #define IQS5XX_SETUP_COMPLETE	0x40
@@ -53,8 +56,8 @@
 #define IQS5XX_SWITCH_XY_AXIS	0x04
 
 #define IQS5XX_PROD_NUM		0x0000
-#define IQS5XX_ABS_X		0x0016
-#define IQS5XX_ABS_Y		0x0018
+#define IQS5XX_SYS_INFO0	0x000F
+#define IQS5XX_SYS_INFO1	0x0010
 #define IQS5XX_SYS_CTRL0	0x0431
 #define IQS5XX_SYS_CTRL1	0x0432
 #define IQS5XX_SYS_CFG0		0x058E
@@ -127,6 +130,14 @@ struct iqs5xx_touch_data {
 	u8 area;
 } __packed;
 
+struct iqs5xx_status {
+	u8 sys_info[2];
+	u8 num_active;
+	__be16 rel_x;
+	__be16 rel_y;
+	struct iqs5xx_touch_data touch_data[IQS5XX_NUM_CONTACTS];
+} __packed;
+
 static int iqs5xx_read_burst(struct i2c_client *client,
 			     u16 reg, void *val, u16 len)
 {
@@ -676,6 +687,10 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 	if (error)
 		return error;
 
+	error = iqs5xx_write_byte(client, IQS5XX_SYS_CTRL0, IQS5XX_ACK_RESET);
+	if (error)
+		return error;
+
 	error = iqs5xx_read_byte(client, IQS5XX_SYS_CFG0, &val);
 	if (error)
 		return error;
@@ -712,7 +727,7 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 static irqreturn_t iqs5xx_irq(int irq, void *data)
 {
 	struct iqs5xx_private *iqs5xx = data;
-	struct iqs5xx_touch_data touch_data[IQS5XX_NUM_CONTACTS];
+	struct iqs5xx_status status;
 	struct i2c_client *client = iqs5xx->client;
 	struct input_dev *input = iqs5xx->input;
 	int error, i;
@@ -725,21 +740,35 @@ static irqreturn_t iqs5xx_irq(int irq, void *data)
 	if (iqs5xx->bl_status == IQS5XX_BL_STATUS_RESET)
 		return IRQ_NONE;
 
-	error = iqs5xx_read_burst(client, IQS5XX_ABS_X,
-				  touch_data, sizeof(touch_data));
+	error = iqs5xx_read_burst(client, IQS5XX_SYS_INFO0,
+				  &status, sizeof(status));
 	if (error)
 		return IRQ_NONE;
 
-	for (i = 0; i < ARRAY_SIZE(touch_data); i++) {
-		u16 pressure = be16_to_cpu(touch_data[i].strength);
+	if (status.sys_info[0] & IQS5XX_SHOW_RESET) {
+		dev_err(&client->dev, "Unexpected device reset\n");
+
+		error = iqs5xx_dev_init(client);
+		if (error) {
+			dev_err(&client->dev,
+				"Failed to re-initialize device: %d\n", error);
+			return IRQ_NONE;
+		}
+
+		return IRQ_HANDLED;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(status.touch_data); i++) {
+		struct iqs5xx_touch_data *touch_data = &status.touch_data[i];
+		u16 pressure = be16_to_cpu(touch_data->strength);
 
 		input_mt_slot(input, i);
 		if (input_mt_report_slot_state(input, MT_TOOL_FINGER,
 					       pressure != 0)) {
 			input_report_abs(input, ABS_MT_POSITION_X,
-					 be16_to_cpu(touch_data[i].abs_x));
+					 be16_to_cpu(touch_data->abs_x));
 			input_report_abs(input, ABS_MT_POSITION_Y,
-					 be16_to_cpu(touch_data[i].abs_y));
+					 be16_to_cpu(touch_data->abs_y));
 			input_report_abs(input, ABS_MT_PRESSURE, pressure);
 		}
 	}
-- 
2.7.4

Re: [PATCH 05/10] input: iqs5xx: Re-initialize device upon warm reset

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:41PM -0600, Jeff LaBundy wrote:
> The device may be inadvertently reset during runtime in the event
> of ESD strike, etc. To protect against this case, acknowledge the
> SHOW_RESET interrupt and re-initialize the device.
> 
> To facilitate this change, expand the range of registers that are
> read in the interrupt handler to include the system status fields.
> 
> Also, update the unrelated (but nearby) SUSPEND register field to
> use the BIT() macro. The remaining register fields are cleaned up
> in another patch.

Added linux/bits.h include and applied, thank you.

-- 
Dmitry

[PATCH 06/10] input: iqs5xx: Simplify axis setup logic

[Jeff LaBundy]

The present implementation manipulates axis swap and inversion fields
in the device to more or less duplicate what touchscreen_report_pos()
does. The resulting logic is convoluted and difficult to follow.

Instead report the maximum X and Y coordinates in earnest as they are
read from the device, then let touchscreen_parse_properties() fix the
axes up as necessary. Finally, use touchscreen_report_pos() to report
the transformed coordinates.

Last but not least, the maximum X and Y coordinates are not functions
of the number of rows/columns that comprise the touch surface. Either
coordinate is simply limited to 1 below what is reported for absolute
X or Y coordinates when no fingers are present (0xFFFF).

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 100 ++++++++-----------------------------
 1 file changed, 21 insertions(+), 79 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 9412fb8..d802cee 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -29,9 +29,9 @@
 
 #define IQS5XX_FW_FILE_LEN	64
 #define IQS5XX_NUM_RETRIES	10
-#define IQS5XX_NUM_POINTS	256
 #define IQS5XX_NUM_CONTACTS	5
 #define IQS5XX_WR_BYTES_MAX	2
+#define IQS5XX_XY_RES_MAX	0xFFFE
 
 #define IQS5XX_PROD_NUM_IQS550	40
 #define IQS5XX_PROD_NUM_IQS572	58
@@ -51,10 +51,6 @@
 #define IQS5XX_EVENT_MODE	0x01
 #define IQS5XX_TP_EVENT		0x04
 
-#define IQS5XX_FLIP_X		0x01
-#define IQS5XX_FLIP_Y		0x02
-#define IQS5XX_SWITCH_XY_AXIS	0x04
-
 #define IQS5XX_PROD_NUM		0x0000
 #define IQS5XX_SYS_INFO0	0x000F
 #define IQS5XX_SYS_INFO1	0x0010
@@ -62,9 +58,6 @@
 #define IQS5XX_SYS_CTRL1	0x0432
 #define IQS5XX_SYS_CFG0		0x058E
 #define IQS5XX_SYS_CFG1		0x058F
-#define IQS5XX_TOTAL_RX		0x063D
-#define IQS5XX_TOTAL_TX		0x063E
-#define IQS5XX_XY_CFG0		0x0669
 #define IQS5XX_X_RES		0x066E
 #define IQS5XX_Y_RES		0x0670
 #define IQS5XX_EXP_FILE		0x0677
@@ -102,6 +95,7 @@ struct iqs5xx_private {
 	struct i2c_client *client;
 	struct input_dev *input;
 	struct gpio_desc *reset_gpio;
+	struct touchscreen_properties prop;
 	struct mutex lock;
 	u16 exp_file;
 	u8 bl_status;
@@ -498,12 +492,10 @@ static void iqs5xx_close(struct input_dev *input)
 static int iqs5xx_axis_init(struct i2c_client *client)
 {
 	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
-	struct touchscreen_properties prop;
+	struct touchscreen_properties *prop = &iqs5xx->prop;
 	struct input_dev *input;
+	u16 max_x, max_y;
 	int error;
-	u16 max_x, max_x_hw;
-	u16 max_y, max_y_hw;
-	u8 val;
 
 	if (!iqs5xx->input) {
 		input = devm_input_allocate_device(&client->dev);
@@ -523,89 +515,39 @@ static int iqs5xx_axis_init(struct i2c_client *client)
 		iqs5xx->input = input;
 	}
 
-	touchscreen_parse_properties(iqs5xx->input, true, &prop);
-
-	error = iqs5xx_read_byte(client, IQS5XX_TOTAL_RX, &val);
-	if (error)
-		return error;
-	max_x_hw = (val - 1) * IQS5XX_NUM_POINTS;
-
-	error = iqs5xx_read_byte(client, IQS5XX_TOTAL_TX, &val);
+	error = iqs5xx_read_word(client, IQS5XX_X_RES, &max_x);
 	if (error)
 		return error;
-	max_y_hw = (val - 1) * IQS5XX_NUM_POINTS;
 
-	error = iqs5xx_read_byte(client, IQS5XX_XY_CFG0, &val);
+	error = iqs5xx_read_word(client, IQS5XX_Y_RES, &max_y);
 	if (error)
 		return error;
 
-	if (val & IQS5XX_SWITCH_XY_AXIS)
-		swap(max_x_hw, max_y_hw);
-
-	if (prop.swap_x_y)
-		val ^= IQS5XX_SWITCH_XY_AXIS;
+	input_abs_set_max(iqs5xx->input, ABS_MT_POSITION_X, max_x);
+	input_abs_set_max(iqs5xx->input, ABS_MT_POSITION_Y, max_y);
 
-	if (prop.invert_x)
-		val ^= prop.swap_x_y ? IQS5XX_FLIP_Y : IQS5XX_FLIP_X;
+	touchscreen_parse_properties(iqs5xx->input, true, prop);
 
-	if (prop.invert_y)
-		val ^= prop.swap_x_y ? IQS5XX_FLIP_X : IQS5XX_FLIP_Y;
-
-	error = iqs5xx_write_byte(client, IQS5XX_XY_CFG0, val);
-	if (error)
-		return error;
-
-	if (prop.max_x > max_x_hw) {
+	if (prop->max_x > IQS5XX_XY_RES_MAX) {
 		dev_err(&client->dev, "Invalid maximum x-coordinate: %u > %u\n",
-			prop.max_x, max_x_hw);
+			prop->max_x, IQS5XX_XY_RES_MAX);
 		return -EINVAL;
-	} else if (prop.max_x == 0) {
-		error = iqs5xx_read_word(client, IQS5XX_X_RES, &max_x);
+	} else if (prop->max_x != max_x) {
+		error = iqs5xx_write_word(client, IQS5XX_X_RES, prop->max_x);
 		if (error)
 			return error;
-
-		input_abs_set_max(iqs5xx->input,
-				  prop.swap_x_y ? ABS_MT_POSITION_Y :
-						  ABS_MT_POSITION_X,
-				  max_x);
-	} else {
-		max_x = (u16)prop.max_x;
 	}
 
-	if (prop.max_y > max_y_hw) {
+	if (prop->max_y > IQS5XX_XY_RES_MAX) {
 		dev_err(&client->dev, "Invalid maximum y-coordinate: %u > %u\n",
-			prop.max_y, max_y_hw);
+			prop->max_y, IQS5XX_XY_RES_MAX);
 		return -EINVAL;
-	} else if (prop.max_y == 0) {
-		error = iqs5xx_read_word(client, IQS5XX_Y_RES, &max_y);
+	} else if (prop->max_y != max_y) {
+		error = iqs5xx_write_word(client, IQS5XX_Y_RES, prop->max_y);
 		if (error)
 			return error;
-
-		input_abs_set_max(iqs5xx->input,
-				  prop.swap_x_y ? ABS_MT_POSITION_X :
-						  ABS_MT_POSITION_Y,
-				  max_y);
-	} else {
-		max_y = (u16)prop.max_y;
 	}
 
-	/*
-	 * Write horizontal and vertical resolution to the device in case its
-	 * original defaults were overridden or swapped as per the properties
-	 * specified in the device tree.
-	 */
-	error = iqs5xx_write_word(client,
-				  prop.swap_x_y ? IQS5XX_Y_RES : IQS5XX_X_RES,
-				  max_x);
-	if (error)
-		return error;
-
-	error = iqs5xx_write_word(client,
-				  prop.swap_x_y ? IQS5XX_X_RES : IQS5XX_Y_RES,
-				  max_y);
-	if (error)
-		return error;
-
 	error = input_mt_init_slots(iqs5xx->input, IQS5XX_NUM_CONTACTS,
 				    INPUT_MT_DIRECT);
 	if (error)
@@ -765,10 +707,10 @@ static irqreturn_t iqs5xx_irq(int irq, void *data)
 		input_mt_slot(input, i);
 		if (input_mt_report_slot_state(input, MT_TOOL_FINGER,
 					       pressure != 0)) {
-			input_report_abs(input, ABS_MT_POSITION_X,
-					 be16_to_cpu(touch_data->abs_x));
-			input_report_abs(input, ABS_MT_POSITION_Y,
-					 be16_to_cpu(touch_data->abs_y));
+			touchscreen_report_pos(iqs5xx->input, &iqs5xx->prop,
+					       be16_to_cpu(touch_data->abs_x),
+					       be16_to_cpu(touch_data->abs_y),
+					       true);
 			input_report_abs(input, ABS_MT_PRESSURE, pressure);
 		}
 	}
-- 
2.7.4

Re: [PATCH 06/10] input: iqs5xx: Simplify axis setup logic

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:42PM -0600, Jeff LaBundy wrote:
> The present implementation manipulates axis swap and inversion fields
> in the device to more or less duplicate what touchscreen_report_pos()
> does. The resulting logic is convoluted and difficult to follow.
> 
> Instead report the maximum X and Y coordinates in earnest as they are
> read from the device, then let touchscreen_parse_properties() fix the
> axes up as necessary. Finally, use touchscreen_report_pos() to report
> the transformed coordinates.
> 
> Last but not least, the maximum X and Y coordinates are not functions
> of the number of rows/columns that comprise the touch surface. Either
> coordinate is simply limited to 1 below what is reported for absolute
> X or Y coordinates when no fingers are present (0xFFFF).
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 07/10] input: iqs5xx: Eliminate unnecessary register read

[Jeff LaBundy]

Instead of relying on firmware to enable important register fields
and performing read-modify-write operations to additionally enable
the fields the driver cares about, it's much simpler just to write
all of the pertinent fields explicitly.

This avoids an unnecessary register read operation at start-up and
makes way for the iqs5xx_read_byte() helper to be dropped.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index d802cee..50b9344 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -46,10 +46,13 @@
 #define IQS5XX_SUSPEND		BIT(0)
 #define IQS5XX_RESUME		0
 
-#define IQS5XX_SW_INPUT_EVENT	0x10
-#define IQS5XX_SETUP_COMPLETE	0x40
-#define IQS5XX_EVENT_MODE	0x01
-#define IQS5XX_TP_EVENT		0x04
+#define IQS5XX_SETUP_COMPLETE	BIT(6)
+#define IQS5XX_WDT		BIT(5)
+#define IQS5XX_ALP_REATI	BIT(3)
+#define IQS5XX_REATI		BIT(2)
+
+#define IQS5XX_TP_EVENT		BIT(2)
+#define IQS5XX_EVENT_MODE	BIT(0)
 
 #define IQS5XX_PROD_NUM		0x0000
 #define IQS5XX_SYS_INFO0	0x000F
@@ -188,11 +191,6 @@ static int iqs5xx_read_word(struct i2c_client *client, u16 reg, u16 *val)
 	return 0;
 }
 
-static int iqs5xx_read_byte(struct i2c_client *client, u16 reg, u8 *val)
-{
-	return iqs5xx_read_burst(client, reg, val, sizeof(*val));
-}
-
 static int iqs5xx_write_burst(struct i2c_client *client,
 			      u16 reg, const void *val, u16 len)
 {
@@ -562,7 +560,6 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
 	struct iqs5xx_dev_id_info *dev_id_info;
 	int error;
-	u8 val;
 	u8 buf[sizeof(*dev_id_info) + 1];
 
 	error = iqs5xx_read_burst(client, IQS5XX_PROD_NUM,
@@ -633,18 +630,14 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 	if (error)
 		return error;
 
-	error = iqs5xx_read_byte(client, IQS5XX_SYS_CFG0, &val);
-	if (error)
-		return error;
-
-	val |= IQS5XX_SETUP_COMPLETE;
-	val &= ~IQS5XX_SW_INPUT_EVENT;
-	error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG0, val);
+	error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG0,
+				  IQS5XX_SETUP_COMPLETE | IQS5XX_WDT |
+				  IQS5XX_ALP_REATI | IQS5XX_REATI);
 	if (error)
 		return error;
 
-	val = IQS5XX_TP_EVENT | IQS5XX_EVENT_MODE;
-	error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG1, val);
+	error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG1,
+				  IQS5XX_TP_EVENT | IQS5XX_EVENT_MODE);
 	if (error)
 		return error;
 
-- 
2.7.4

Re: [PATCH 07/10] input: iqs5xx: Eliminate unnecessary register read

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:43PM -0600, Jeff LaBundy wrote:
> Instead of relying on firmware to enable important register fields
> and performing read-modify-write operations to additionally enable
> the fields the driver cares about, it's much simpler just to write
> all of the pertinent fields explicitly.
> 
> This avoids an unnecessary register read operation at start-up and
> makes way for the iqs5xx_read_byte() helper to be dropped.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 08/10] input: iqs5xx: Allow more time for ATI to complete

[Jeff LaBundy]

After the device is initialized, it runs ATI (calibration) during
which it cannot readily respond to I2C communication. To keep the
open and close callbacks from writing to the device too soon, the
driver waits 100 ms before returning from probe.

The vendor reports that ATI may actually take up to 250 ms to run
(including margin), so increase the delay accordingly. Update the
comments to clarify the reason for the delay as well.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 50b9344..babd1f1 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -648,13 +648,12 @@ static int iqs5xx_dev_init(struct i2c_client *client)
 	iqs5xx->bl_status = dev_id_info->bl_status;
 
 	/*
-	 * Closure of the first communication window that appears following the
-	 * release of reset appears to kick off an initialization period during
-	 * which further communication is met with clock stretching. The return
-	 * from this function is delayed so that further communication attempts
-	 * avoid this period.
+	 * The following delay allows ATI to complete before the open and close
+	 * callbacks are free to elicit I2C communication. Any attempts to read
+	 * from or write to the device during this time may face extended clock
+	 * stretching and prompt the I2C controller to report an error.
 	 */
-	msleep(100);
+	msleep(250);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 08/10] input: iqs5xx: Allow more time for ATI to complete

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:44PM -0600, Jeff LaBundy wrote:
> After the device is initialized, it runs ATI (calibration) during
> which it cannot readily respond to I2C communication. To keep the
> open and close callbacks from writing to the device too soon, the
> driver waits 100 ms before returning from probe.
> 
> The vendor reports that ATI may actually take up to 250 ms to run
> (including margin), so increase the delay accordingly. Update the
> comments to clarify the reason for the delay as well.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH 09/10] input: iqs5xx: Make reset GPIO optional

[Jeff LaBundy]

The device's hardware reset pin is only required if the platform
must be able to update the device's firmware on the fly.

As such, demote the reset GPIO to optional in support of devices
that ship with pre-programmed firmware and don't route the reset
pin back to the SOC.

If user space attempts to push updated firmware which would rely
upon the reset pin to wake the bootloader, attempts to reach the
bootloader are simply NAK'd and the device resumes normally.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index babd1f1..dac132b 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -242,6 +242,9 @@ static void iqs5xx_reset(struct i2c_client *client)
 {
 	struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
 
+	if (!iqs5xx->reset_gpio)
+		return;
+
 	gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
 	usleep_range(200, 300);
 
@@ -1045,8 +1048,8 @@ static int iqs5xx_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, iqs5xx);
 	iqs5xx->client = client;
 
-	iqs5xx->reset_gpio = devm_gpiod_get(&client->dev,
-					    "reset", GPIOD_OUT_LOW);
+	iqs5xx->reset_gpio = devm_gpiod_get_optional(&client->dev,
+						     "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(iqs5xx->reset_gpio)) {
 		error = PTR_ERR(iqs5xx->reset_gpio);
 		dev_err(&client->dev, "Failed to request GPIO: %d\n", error);
-- 
2.7.4

Re: [PATCH 09/10] input: iqs5xx: Make reset GPIO optional

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:45PM -0600, Jeff LaBundy wrote:
> The device's hardware reset pin is only required if the platform
> must be able to update the device's firmware on the fly.
> 
> As such, demote the reset GPIO to optional in support of devices
> that ship with pre-programmed firmware and don't route the reset
> pin back to the SOC.
> 
> If user space attempts to push updated firmware which would rely
> upon the reset pin to wake the bootloader, attempts to reach the
> bootloader are simply NAK'd and the device resumes normally.

Can we maybe make the firmware attribute invisible in this case? Or
return early instead of failing to enter bootloader mode?

Thanks.

-- 
Dmitry

Re: [PATCH 09/10] input: iqs5xx: Make reset GPIO optional

[Jeff LaBundy]

Hi Dmitry,

On Sun, Jan 24, 2021 at 08:43:46PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 18, 2021 at 02:43:45PM -0600, Jeff LaBundy wrote:
> > The device's hardware reset pin is only required if the platform
> > must be able to update the device's firmware on the fly.
> > 
> > As such, demote the reset GPIO to optional in support of devices
> > that ship with pre-programmed firmware and don't route the reset
> > pin back to the SOC.
> > 
> > If user space attempts to push updated firmware which would rely
> > upon the reset pin to wake the bootloader, attempts to reach the
> > bootloader are simply NAK'd and the device resumes normally.
> 
> Can we maybe make the firmware attribute invisible in this case? Or
> return early instead of failing to enter bootloader mode?

I almost sent the second alternative, but instead I liked the idea of
restricting the check for reset_gpio to the only method that actually
uses it. That way, nothing outside of iqs5xx_reset() has to check for
the GPIO and can just fail gracefully in its absence.

That being said, either of your suggestions work just as well; let me
take another stab at it.

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy

[PATCH 10/10] input: iqs5xx: Allow device to be a wake-up source

[Jeff LaBundy]

Avoid placing the device in suspend mode (from which it cannot
generate interrupts) if it is defined as a wake-up source. The
device is still permitted to enter a low-power sensing mode on
its own.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/touchscreen/iqs5xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index dac132b..6c1bc6b 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -1001,7 +1001,7 @@ static int __maybe_unused iqs5xx_suspend(struct device *dev)
 	struct input_dev *input = iqs5xx->input;
 	int error = 0;
 
-	if (!input)
+	if (!input || device_may_wakeup(dev))
 		return error;
 
 	mutex_lock(&input->mutex);
@@ -1020,7 +1020,7 @@ static int __maybe_unused iqs5xx_resume(struct device *dev)
 	struct input_dev *input = iqs5xx->input;
 	int error = 0;
 
-	if (!input)
+	if (!input || device_may_wakeup(dev))
 		return error;
 
 	mutex_lock(&input->mutex);
-- 
2.7.4

Re: [PATCH 10/10] input: iqs5xx: Allow device to be a wake-up source

[Dmitry Torokhov]

On Mon, Jan 18, 2021 at 02:43:46PM -0600, Jeff LaBundy wrote:
> Avoid placing the device in suspend mode (from which it cannot
> generate interrupts) if it is defined as a wake-up source. The
> device is still permitted to enter a low-power sensing mode on
> its own.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry