[PATCH] hid: i2c-hid: Unify invariant post-power-on delay

[Dzianis Kahanovich]

This is result of debugging WCOM5020 on "Thinkpad 10 2nd" (2 HID devices -
finger & pen), have random failures on any delay. Quirk-style fixes
still wrong.

I found this delay was too invariant (different after
regulator_enable() & i2c command) and not even called in many
i2c power-on places (like PM & HID open/close, called multiple in my case).
All looks logically wrong.
Lets consolidate & serialize power-on code & delay, adding new module
parameter "post_power_delay_ms_default".

Side-effect:  delay will be sleep twice (on init) - after regulator_enable()
and cmd power-on. There are no way to separately found "what is true power-on"
for abstract unknown device. But, AFAIK, delay on power-on is by specification.
This is just unified replacement of 2 different sleep attempt.

Now for me post_power_delay_ms_default=1000 is safe.

Signed-off-by: Dzianis Kahanovich <mahatma@xxxxx>

--- a/drivers/hid/i2c-hid/i2c-hid.c	2017-12-17 23:34:28.067527024 +0300
+++ b/drivers/hid/i2c-hid/i2c-hid.c	2017-12-17 23:38:04.405071925 +0300
@@ -61,6 +61,12 @@ static bool debug;
 module_param(debug, bool, 0444);
 MODULE_PARM_DESC(debug, "print a lot of debug information");

+/* default post power on delay */
+/* for some of strange devices or i2c like WCOM5020 even 1000 is enough */
+static int post_power_delay_ms_default;
+module_param(post_power_delay_ms_default, int, 0444);
+MODULE_PARM_DESC(post_power_delay_ms_default, "default post power-on delay");
+
 #define i2c_hid_dbg(ihid, fmt, arg...)					  \
 do {									  \
 	if (debug)							  \
@@ -382,7 +388,24 @@ static int i2c_hid_set_or_send_report(st
 	return data_len;
 }

-static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+/* Agnostic calling twice: after regulator "power on" & hid cmd "power on" */
+static inline void i2c_hid_power_delay(struct i2c_hid *ihid)
+{
+	/*
+	 * The HID over I2C specification states that if a DEVICE needs time
+	 * after the PWR_ON request, it should utilise CLOCK stretching.
+	 * However, it has been observered that the Windows driver provides a
+	 * 1ms sleep between the PWR_ON and RESET requests and that some devices
+	 * rely on this.
+	 */
+	if (ihid->pdata.post_power_delay_ms)
+		msleep(ihid->pdata.post_power_delay_ms);
+	else
+		usleep_range(1000, 5000);
+
+}
+
+static int __i2c_hid_set_power(struct i2c_client *client, int power_state)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
@@ -410,6 +433,23 @@ static int i2c_hid_set_power(struct i2c_
 		dev_err(&client->dev, "failed to change power setting.\n");

 set_pwr_exit:
+	if (!ret && power_state == I2C_HID_PWR_ON)
+		i2c_hid_power_delay(ihid);
+	return ret;
+}
+
+static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	/*
+	 * Can be called from various places, including PM calls,
+	 * strict behaviour & delay.
+	 */
+	mutex_lock(&ihid->reset_lock);
+	ret=__i2c_hid_set_power(client,power_state);
+	mutex_unlock(&ihid->reset_lock);
 	return ret;
 }

@@ -427,25 +467,17 @@ static int i2c_hid_hwreset(struct i2c_cl
 	 */
 	mutex_lock(&ihid->reset_lock);

-	ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+	ret = __i2c_hid_set_power(client, I2C_HID_PWR_ON);
 	if (ret)
 		goto out_unlock;

-	/*
-	 * The HID over I2C specification states that if a DEVICE needs time
-	 * after the PWR_ON request, it should utilise CLOCK stretching.
-	 * However, it has been observered that the Windows driver provides a
-	 * 1ms sleep between the PWR_ON and RESET requests and that some devices
-	 * rely on this.
-	 */
-	usleep_range(1000, 5000);

 	i2c_hid_dbg(ihid, "resetting...\n");

 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
 	if (ret) {
 		dev_err(&client->dev, "failed to reset device.\n");
-		i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+		__i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
 	}

 out_unlock:
@@ -988,6 +1020,8 @@ static int i2c_hid_probe(struct i2c_clie
 	if (!ihid)
 		return -ENOMEM;

+	ihid->pdata.post_power_delay_ms = post_power_delay_ms_default;
+
 	if (client->dev.of_node) {
 		ret = i2c_hid_of_probe(client, &ihid->pdata);
 		if (ret)
@@ -1021,8 +1055,7 @@ static int i2c_hid_probe(struct i2c_clie
 			ret);
 		goto err;
 	}
-	if (ihid->pdata.post_power_delay_ms)
-		msleep(ihid->pdata.post_power_delay_ms);
+	i2c_hid_power_delay(ihid);

 	i2c_set_clientdata(client, ihid);

@@ -1198,8 +1231,7 @@ static int i2c_hid_resume(struct device
 		ret = regulator_enable(ihid->pdata.supply);
 		if (ret < 0)
 			hid_warn(hid, "Failed to enable supply: %d\n", ret);
-		if (ihid->pdata.post_power_delay_ms)
-			msleep(ihid->pdata.post_power_delay_ms);
+		i2c_hid_power_delay(ihid);
 	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
--
WBR, Dzianis Kahanovich AKA Denis Kaganovich, http://mahatma.bspu.by/