On Mon, Nov 28, 2022 at 21:02, Jeff LaBundy <jeff@xxxxxxxxxxx> 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@xxxxxxxxxxx>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@xxxxxxxxxxxx>
> ---
> 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
