diff mbox series

[4/5] Input: iqs269a - do not poll during suspend or resume

Message ID Y4V2XUtLVkOHvi6C@nixie71 (mailing list archive)
State Superseded
Headers show
Series Miscellaneous fixes for Azoteq IQS269A | expand

Commit Message

Jeff LaBundy Nov. 29, 2022, 3:02 a.m. UTC 
Polling the device while it transitions from automatic to manual
power mode switching may keep the device from actually finishing
the transition. The process appears to time out depending on the
polling rate and the device's core clock frequency.

This is ultimately unnecessary in the first place; instead it is
sufficient to write the desired mode during initialization, then
disable automatic switching at suspend. This eliminates the need
to ensure the device is prepared for a manual change and removes
the 'suspend_mode' variable.

Similarly, polling the device while it transitions from one mode
to another under manual control may time out as well. This added
step does not appear to be necessary either, so drop it.

Fixes: 04e49867fad1 ("Input: add support for Azoteq IQS269A")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/misc/iqs269a.c | 118 +++++++++--------------------------
 1 file changed, 31 insertions(+), 87 deletions(-)

Comments

Mattijs Korpershoek Dec. 15, 2022, 10:07 a.m. UTC | #1 
On Mon, Nov 28, 2022 at 21:02, Jeff LaBundy <jeff@labundy.com> wrote:

> Polling the device while it transitions from automatic to manual
> power mode switching may keep the device from actually finishing
> the transition. The process appears to time out depending on the
> polling rate and the device's core clock frequency.
>
> This is ultimately unnecessary in the first place; instead it is
> sufficient to write the desired mode during initialization, then
> disable automatic switching at suspend. This eliminates the need
> to ensure the device is prepared for a manual change and removes
> the 'suspend_mode' variable.
>
> Similarly, polling the device while it transitions from one mode
> to another under manual control may time out as well. This added
> step does not appear to be necessary either, so drop it.
>
> Fixes: 04e49867fad1 ("Input: add support for Azoteq IQS269A")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/input/misc/iqs269a.c | 118 +++++++++--------------------------
>  1 file changed, 31 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index 0eb3cff177e5..eca680bf8c20 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -148,9 +148,6 @@
>  #define IQS269_ATI_POLL_TIMEOUT_US		(iqs269->delay_mult * 500000)
>  #define IQS269_ATI_STABLE_DELAY_MS		(iqs269->delay_mult * 150)
>  
> -#define IQS269_PWR_MODE_POLL_SLEEP_US		IQS269_ATI_POLL_SLEEP_US
> -#define IQS269_PWR_MODE_POLL_TIMEOUT_US		IQS269_ATI_POLL_TIMEOUT_US
> -
>  #define iqs269_irq_wait()			usleep_range(200, 250)
>  
>  enum iqs269_local_cap_size {
> @@ -295,7 +292,6 @@ struct iqs269_private {
>  	struct input_dev *keypad;
>  	struct input_dev *slider[IQS269_NUM_SL];
>  	unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
> -	unsigned int suspend_mode;
>  	unsigned int delay_mult;
>  	unsigned int ch_num;
>  	bool hall_enable;
> @@ -773,17 +769,6 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
>  	iqs269->hall_enable = device_property_present(&client->dev,
>  						      "azoteq,hall-enable");
>  
> -	if (!device_property_read_u32(&client->dev, "azoteq,suspend-mode",
> -				      &val)) {
> -		if (val > IQS269_SYS_SETTINGS_PWR_MODE_MAX) {
> -			dev_err(&client->dev, "Invalid suspend mode: %u\n",
> -				val);
> -			return -EINVAL;
> -		}
> -
> -		iqs269->suspend_mode = val;
> -	}
> -
>  	error = regmap_raw_read(iqs269->regmap, IQS269_SYS_SETTINGS, sys_reg,
>  				sizeof(*sys_reg));
>  	if (error)
> @@ -1011,6 +996,17 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
>  	general &= ~IQS269_SYS_SETTINGS_DIS_AUTO;
>  	general &= ~IQS269_SYS_SETTINGS_PWR_MODE_MASK;
>  
> +	if (!device_property_read_u32(&client->dev, "azoteq,suspend-mode",
> +				      &val)) {
> +		if (val > IQS269_SYS_SETTINGS_PWR_MODE_MAX) {
> +			dev_err(&client->dev, "Invalid suspend mode: %u\n",
> +				val);
> +			return -EINVAL;
> +		}
> +
> +		general |= (val << IQS269_SYS_SETTINGS_PWR_MODE_SHIFT);
> +	}
> +
>  	if (!device_property_read_u32(&client->dev, "azoteq,ulp-update",
>  				      &val)) {
>  		if (val > IQS269_SYS_SETTINGS_ULP_UPDATE_MAX) {
> @@ -1693,59 +1689,30 @@ static int iqs269_probe(struct i2c_client *client)
>  	return error;
>  }
>  
> +static u16 iqs269_general_get(struct iqs269_private *iqs269)
> +{
> +	u16 general = be16_to_cpu(iqs269->sys_reg.general);
> +
> +	general &= ~IQS269_SYS_SETTINGS_REDO_ATI;
> +	general &= ~IQS269_SYS_SETTINGS_ACK_RESET;
> +
> +	return general | IQS269_SYS_SETTINGS_DIS_AUTO;
> +}
> +
>  static int __maybe_unused iqs269_suspend(struct device *dev)
>  {
>  	struct iqs269_private *iqs269 = dev_get_drvdata(dev);
>  	struct i2c_client *client = iqs269->client;
> -	unsigned int val;
>  	int error;
> +	u16 general = iqs269_general_get(iqs269);
>  
> -	if (!iqs269->suspend_mode)
> +	if (!(general & IQS269_SYS_SETTINGS_PWR_MODE_MASK))
>  		return 0;
>  
>  	disable_irq(client->irq);
>  
> -	/*
> -	 * Automatic power mode switching must be disabled before the device is
> -	 * forced into any particular power mode. In this case, the device will
> -	 * transition into normal-power mode.
> -	 */
> -	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
> -				   IQS269_SYS_SETTINGS_DIS_AUTO, ~0);
> -	if (error)
> -		goto err_irq;
> -
> -	/*
> -	 * The following check ensures the device has completed its transition
> -	 * into normal-power mode before a manual mode switch is performed.
> -	 */
> -	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
> -					!(val & IQS269_SYS_FLAGS_PWR_MODE_MASK),
> -					 IQS269_PWR_MODE_POLL_SLEEP_US,
> -					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
> -	if (error)
> -		goto err_irq;
> -
> -	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
> -				   IQS269_SYS_SETTINGS_PWR_MODE_MASK,
> -				   iqs269->suspend_mode <<
> -				   IQS269_SYS_SETTINGS_PWR_MODE_SHIFT);
> -	if (error)
> -		goto err_irq;
> -
> -	/*
> -	 * This last check ensures the device has completed its transition into
> -	 * the desired power mode to prevent any spurious interrupts from being
> -	 * triggered after iqs269_suspend has already returned.
> -	 */
> -	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
> -					 (val & IQS269_SYS_FLAGS_PWR_MODE_MASK)
> -					 == (iqs269->suspend_mode <<
> -					     IQS269_SYS_FLAGS_PWR_MODE_SHIFT),
> -					 IQS269_PWR_MODE_POLL_SLEEP_US,
> -					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
> +	error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS, general);
>  
> -err_irq:
>  	iqs269_irq_wait();
>  	enable_irq(client->irq);
>  
> @@ -1756,43 +1723,20 @@ static int __maybe_unused iqs269_resume(struct device *dev)
>  {
>  	struct iqs269_private *iqs269 = dev_get_drvdata(dev);
>  	struct i2c_client *client = iqs269->client;
> -	unsigned int val;
>  	int error;
> +	u16 general = iqs269_general_get(iqs269);
>  
> -	if (!iqs269->suspend_mode)
> +	if (!(general & IQS269_SYS_SETTINGS_PWR_MODE_MASK))
>  		return 0;
>  
>  	disable_irq(client->irq);
>  
> -	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
> -				   IQS269_SYS_SETTINGS_PWR_MODE_MASK, 0);
> -	if (error)
> -		goto err_irq;
> -
> -	/*
> -	 * This check ensures the device has returned to normal-power mode
> -	 * before automatic power mode switching is re-enabled.
> -	 */
> -	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
> -					!(val & IQS269_SYS_FLAGS_PWR_MODE_MASK),
> -					 IQS269_PWR_MODE_POLL_SLEEP_US,
> -					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
> -	if (error)
> -		goto err_irq;
> -
> -	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
> -				   IQS269_SYS_SETTINGS_DIS_AUTO, 0);
> -	if (error)
> -		goto err_irq;
> -
> -	/*
> -	 * This step reports any events that may have been "swallowed" as a
> -	 * result of polling PWR_MODE (which automatically acknowledges any
> -	 * pending interrupts).
> -	 */
> -	error = iqs269_report(iqs269);
> +	error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS,
> +			     general & ~IQS269_SYS_SETTINGS_PWR_MODE_MASK);
> +	if (!error)
> +		error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS,
> +				     general & ~IQS269_SYS_SETTINGS_DIS_AUTO);
>  
> -err_irq:
>  	iqs269_irq_wait();
>  	enable_irq(client->irq);
>  
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index 0eb3cff177e5..eca680bf8c20 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -148,9 +148,6 @@ 
 #define IQS269_ATI_POLL_TIMEOUT_US		(iqs269->delay_mult * 500000)
 #define IQS269_ATI_STABLE_DELAY_MS		(iqs269->delay_mult * 150)
 
-#define IQS269_PWR_MODE_POLL_SLEEP_US		IQS269_ATI_POLL_SLEEP_US
-#define IQS269_PWR_MODE_POLL_TIMEOUT_US		IQS269_ATI_POLL_TIMEOUT_US
-
 #define iqs269_irq_wait()			usleep_range(200, 250)
 
 enum iqs269_local_cap_size {
@@ -295,7 +292,6 @@  struct iqs269_private {
 	struct input_dev *keypad;
 	struct input_dev *slider[IQS269_NUM_SL];
 	unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
-	unsigned int suspend_mode;
 	unsigned int delay_mult;
 	unsigned int ch_num;
 	bool hall_enable;
@@ -773,17 +769,6 @@  static int iqs269_parse_prop(struct iqs269_private *iqs269)
 	iqs269->hall_enable = device_property_present(&client->dev,
 						      "azoteq,hall-enable");
 
-	if (!device_property_read_u32(&client->dev, "azoteq,suspend-mode",
-				      &val)) {
-		if (val > IQS269_SYS_SETTINGS_PWR_MODE_MAX) {
-			dev_err(&client->dev, "Invalid suspend mode: %u\n",
-				val);
-			return -EINVAL;
-		}
-
-		iqs269->suspend_mode = val;
-	}
-
 	error = regmap_raw_read(iqs269->regmap, IQS269_SYS_SETTINGS, sys_reg,
 				sizeof(*sys_reg));
 	if (error)
@@ -1011,6 +996,17 @@  static int iqs269_parse_prop(struct iqs269_private *iqs269)
 	general &= ~IQS269_SYS_SETTINGS_DIS_AUTO;
 	general &= ~IQS269_SYS_SETTINGS_PWR_MODE_MASK;
 
+	if (!device_property_read_u32(&client->dev, "azoteq,suspend-mode",
+				      &val)) {
+		if (val > IQS269_SYS_SETTINGS_PWR_MODE_MAX) {
+			dev_err(&client->dev, "Invalid suspend mode: %u\n",
+				val);
+			return -EINVAL;
+		}
+
+		general |= (val << IQS269_SYS_SETTINGS_PWR_MODE_SHIFT);
+	}
+
 	if (!device_property_read_u32(&client->dev, "azoteq,ulp-update",
 				      &val)) {
 		if (val > IQS269_SYS_SETTINGS_ULP_UPDATE_MAX) {
@@ -1693,59 +1689,30 @@  static int iqs269_probe(struct i2c_client *client)
 	return error;
 }
 
+static u16 iqs269_general_get(struct iqs269_private *iqs269)
+{
+	u16 general = be16_to_cpu(iqs269->sys_reg.general);
+
+	general &= ~IQS269_SYS_SETTINGS_REDO_ATI;
+	general &= ~IQS269_SYS_SETTINGS_ACK_RESET;
+
+	return general | IQS269_SYS_SETTINGS_DIS_AUTO;
+}
+
 static int __maybe_unused iqs269_suspend(struct device *dev)
 {
 	struct iqs269_private *iqs269 = dev_get_drvdata(dev);
 	struct i2c_client *client = iqs269->client;
-	unsigned int val;
 	int error;
+	u16 general = iqs269_general_get(iqs269);
 
-	if (!iqs269->suspend_mode)
+	if (!(general & IQS269_SYS_SETTINGS_PWR_MODE_MASK))
 		return 0;
 
 	disable_irq(client->irq);
 
-	/*
-	 * Automatic power mode switching must be disabled before the device is
-	 * forced into any particular power mode. In this case, the device will
-	 * transition into normal-power mode.
-	 */
-	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
-				   IQS269_SYS_SETTINGS_DIS_AUTO, ~0);
-	if (error)
-		goto err_irq;
-
-	/*
-	 * The following check ensures the device has completed its transition
-	 * into normal-power mode before a manual mode switch is performed.
-	 */
-	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
-					!(val & IQS269_SYS_FLAGS_PWR_MODE_MASK),
-					 IQS269_PWR_MODE_POLL_SLEEP_US,
-					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
-	if (error)
-		goto err_irq;
-
-	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
-				   IQS269_SYS_SETTINGS_PWR_MODE_MASK,
-				   iqs269->suspend_mode <<
-				   IQS269_SYS_SETTINGS_PWR_MODE_SHIFT);
-	if (error)
-		goto err_irq;
-
-	/*
-	 * This last check ensures the device has completed its transition into
-	 * the desired power mode to prevent any spurious interrupts from being
-	 * triggered after iqs269_suspend has already returned.
-	 */
-	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
-					 (val & IQS269_SYS_FLAGS_PWR_MODE_MASK)
-					 == (iqs269->suspend_mode <<
-					     IQS269_SYS_FLAGS_PWR_MODE_SHIFT),
-					 IQS269_PWR_MODE_POLL_SLEEP_US,
-					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
+	error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS, general);
 
-err_irq:
 	iqs269_irq_wait();
 	enable_irq(client->irq);
 
@@ -1756,43 +1723,20 @@  static int __maybe_unused iqs269_resume(struct device *dev)
 {
 	struct iqs269_private *iqs269 = dev_get_drvdata(dev);
 	struct i2c_client *client = iqs269->client;
-	unsigned int val;
 	int error;
+	u16 general = iqs269_general_get(iqs269);
 
-	if (!iqs269->suspend_mode)
+	if (!(general & IQS269_SYS_SETTINGS_PWR_MODE_MASK))
 		return 0;
 
 	disable_irq(client->irq);
 
-	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
-				   IQS269_SYS_SETTINGS_PWR_MODE_MASK, 0);
-	if (error)
-		goto err_irq;
-
-	/*
-	 * This check ensures the device has returned to normal-power mode
-	 * before automatic power mode switching is re-enabled.
-	 */
-	error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
-					!(val & IQS269_SYS_FLAGS_PWR_MODE_MASK),
-					 IQS269_PWR_MODE_POLL_SLEEP_US,
-					 IQS269_PWR_MODE_POLL_TIMEOUT_US);
-	if (error)
-		goto err_irq;
-
-	error = regmap_update_bits(iqs269->regmap, IQS269_SYS_SETTINGS,
-				   IQS269_SYS_SETTINGS_DIS_AUTO, 0);
-	if (error)
-		goto err_irq;
-
-	/*
-	 * This step reports any events that may have been "swallowed" as a
-	 * result of polling PWR_MODE (which automatically acknowledges any
-	 * pending interrupts).
-	 */
-	error = iqs269_report(iqs269);
+	error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS,
+			     general & ~IQS269_SYS_SETTINGS_PWR_MODE_MASK);
+	if (!error)
+		error = regmap_write(iqs269->regmap, IQS269_SYS_SETTINGS,
+				     general & ~IQS269_SYS_SETTINGS_DIS_AUTO);
 
-err_irq:
 	iqs269_irq_wait();
 	enable_irq(client->irq);