diff mbox series

[V3] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

Message ID 1660186712-2598-1-git-send-email-marge.yang@synaptics.corp-partner.google.com (mailing list archive)
State Superseded
Headers show
Series [V3] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes. | expand

Commit Message

margeyang Aug. 11, 2022, 2:58 a.m. UTC 
From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>

The interrupt GPIO will be pulled down once
after RMI driver reads this command(Report ID:0x0A).
It will cause "Dark resume test fail" for chromebook device.
Hence, TP driver will ignore rmi_hid_read_block function once
after system resumes.

Signed-off-by: Marge Yang<marge.yang@synaptics.corp-partner.google.com>
---
 drivers/hid/hid-rmi.c | 14 ++++++++++++--
 include/linux/rmi.h   |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Dmitry Torokhov Aug. 11, 2022, 4:20 a.m. UTC | #1 
Hi Marge,

On Thu, Aug 11, 2022 at 10:58:32AM +0800, margeyang wrote:
> From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>

I recommend using your official @synaptics.com email address for
upstream submissions.

> 
> The interrupt GPIO will be pulled down once
> after RMI driver reads this command(Report ID:0x0A).
> It will cause "Dark resume test fail" for chromebook device.

What exactly is the failure mode? Please describe what exactly happens.
Maybe you can also add dtor@google.com to the relevant internal bugs.

> Hence, TP driver will ignore rmi_hid_read_block function once
> after system resumes.

Do you really want to avoid reads, or we happen to signal wakeup from
attention handler somewhere? Is it still valid to skip read if the
touchpad or touchscreen is truly the wakeup source?

> 
> Signed-off-by: Marge Yang<marge.yang@synaptics.corp-partner.google.com>
> ---
>  drivers/hid/hid-rmi.c | 14 ++++++++++++--
>  include/linux/rmi.h   |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 311eee599ce9..fdaf2c4ce181 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -203,7 +203,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  		if (ret < 0)
>  			goto exit;
>  	}
> -
> +	if (xport->ignoreonce == 1) {
> +		dev_err(&hdev->dev,
> +			"ignoreonce (%d)\n",
> +			xport->ignoreonce);
> +		xport->ignoreonce = 0;
> +		goto exit;
> +	}

Do we need to have it after updating the page register?

>  	for (retries = 5; retries > 0; retries--) {
>  		data->writeReport[0] = RMI_READ_ADDR_REPORT_ID;
>  		data->writeReport[1] = 0; /* old 1 byte read count */
> @@ -468,8 +474,12 @@ static int rmi_post_resume(struct hid_device *hdev)
>  	ret = hid_hw_open(hdev);
>  	if (ret)
>  		return ret;
> -
> +	// Avoid to read rmi_hid_read_block once after system resumes.
> +	// The interrupt will be pulled down
> +	// after RMI Read command(Report ID:0x0A).
> +	data->xport.ignoreonce = 1;
>  	ret = rmi_reset_attn_mode(hdev)
> +	data->xport.ignoreonce = 0;
>  	if (ret)
>  		goto out;
>  
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index ab7eea01ab42..24f63ad00970 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -270,6 +270,8 @@ struct rmi_transport_dev {
>  	struct rmi_device_platform_data pdata;
>  
>  	struct input_dev *input;
> +
> +	int ignoreonce;
>  };
>  
>  /**
> -- 
> 2.22.0.windows.1
> 

Thanks.
Marge Yang Aug. 11, 2022, 7:26 a.m. UTC | #2 
Hi Dmitry,
	Thanks for your questions.
I have updated the following questions

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Thursday, August 11, 2022 12:21 PM
To: margeyang <marge.yang@synaptics.corp-partner.google.com>
Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; hdegoede@redhat.com; benjamin.tissoires@redhat.com; Marge Yang <Marge.Yang@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com>
Subject: Re: [PATCH V3] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.


Hi Marge,

On Thu, Aug 11, 2022 at 10:58:32AM +0800, margeyang wrote:
> From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>

I recommend using your official @synaptics.com email address for upstream submissions.

[Marge 08/11] Because Synaptics IT's policy is changed, my Linux device is not  added into white list .
Hence, I can't use marge.yang@tw.synaptics.com and use "git send-email" command to send Linux patch file.
After Synaptics IT changes this setting, I will use marge.yang@tw.synaptics.com email address for upstream submissions
>
> The interrupt GPIO will be pulled down once after RMI driver reads 
> this command(Report ID:0x0A).
> It will cause "Dark resume test fail" for chromebook device.

What exactly is the failure mode? Please describe what exactly happens.
Maybe you can also add dtor@google.com to the relevant internal bugs.

[Marge 08/11] Add dtor@google.com into the list of email.
For Dark Resume (https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md) is enabled on this device/device family (this should be enabled per board / unibuild family)
It is a Chromebook auto test tool.
Check if the device driver is incrementing the wakeup counts correctly. 
    Note the wakeup count of the device before suspend.
    Suspend the device.
    Trigger the wake using the wake source.
    Resume the device.
    Check if the wakeup count has incremented.

The interrupt GPIO will be pulled down when RMI driver reads this command(Report ID: RMI_READ_ADDR_REPORT_ID).
It causes that the wakeup count has incremented and Dark Resume fails.
 

> Hence, TP driver will ignore rmi_hid_read_block function once after 
> system resumes.

Do you really want to avoid reads, or we happen to signal wakeup from attention handler somewhere? Is it still valid to skip read if the touchpad or touchscreen is truly the wakeup source?
[Marge 08/11]
Yes, we want to avoid reads (Report ID: RMI_READ_ADDR_REPORT_ID).
For Synaptics FW design, the interrupt GPIO will be pulled down after RMI driver reads this command(Report ID: RMI_READ_ADDR_REPORT_ID).
"Dark resume" test case on Chromebook device will detect one interrupt (not finger data) during the process of resume function.
"Dark resume" test case will fail because wakeup count has incremented between "Suspend" and "Resume".
Hence, this issue should happen on RMI mode for all Synaptics devices.
This change is used for Synaptics touchpad/touchscreen devices.

Yes, it is valid.
When the touchpad or touchscreen is truly the wakeup source, the interrupt GPIO will be pulled.
"Dark resume" test case on Chromebook device will detect one interrupt (finger data) during the process of resume function.
"Dark resume" test case will show "input" event.

>
> Signed-off-by: Marge 
> Yang<marge.yang@synaptics.corp-partner.google.com>
> ---
>  drivers/hid/hid-rmi.c | 14 ++++++++++++--
>  include/linux/rmi.h   |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c index 
> 311eee599ce9..fdaf2c4ce181 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -203,7 +203,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>               if (ret < 0)
>                       goto exit;
>       }
> -
> +     if (xport->ignoreonce == 1) {
> +             dev_err(&hdev->dev,
> +                     "ignoreonce (%d)\n",
> +                     xport->ignoreonce);
> +             xport->ignoreonce = 0;
> +             goto exit;
> +     }

Do we need to have it after updating the page register?

[Marge 8/11] Yes, we need it to avoid reads (Report ID: RMI_READ_ADDR_REPORT_ID).

>       for (retries = 5; retries > 0; retries--) {
>               data->writeReport[0] = RMI_READ_ADDR_REPORT_ID;
>               data->writeReport[1] = 0; /* old 1 byte read count */ @@ 
> -468,8 +474,12 @@ static int rmi_post_resume(struct hid_device *hdev)
>       ret = hid_hw_open(hdev);
>       if (ret)
>               return ret;
> -
> +     // Avoid to read rmi_hid_read_block once after system resumes.
> +     // The interrupt will be pulled down
> +     // after RMI Read command(Report ID:0x0A).
> +     data->xport.ignoreonce = 1;
>       ret = rmi_reset_attn_mode(hdev)
> +     data->xport.ignoreonce = 0;
>       if (ret)
>               goto out;
>
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h index 
> ab7eea01ab42..24f63ad00970 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -270,6 +270,8 @@ struct rmi_transport_dev {
>       struct rmi_device_platform_data pdata;
>
>       struct input_dev *input;
> +
> +     int ignoreonce;
>  };
>
>  /**
> --
> 2.22.0.windows.1
>

Thanks.

--
Dmitry
Hans de Goede Aug. 11, 2022, 7:51 a.m. UTC | #3 
Hi,

On 8/11/22 04:58, margeyang wrote:
> From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>
> 
> The interrupt GPIO will be pulled down once
> after RMI driver reads this command(Report ID:0x0A).
> It will cause "Dark resume test fail" for chromebook device.
> Hence, TP driver will ignore rmi_hid_read_block function once
> after system resumes.
> 
> Signed-off-by: Marge Yang<marge.yang@synaptics.corp-partner.google.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/hid/hid-rmi.c | 14 ++++++++++++--
>  include/linux/rmi.h   |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 311eee599ce9..fdaf2c4ce181 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -203,7 +203,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  		if (ret < 0)
>  			goto exit;
>  	}
> -
> +	if (xport->ignoreonce == 1) {
> +		dev_err(&hdev->dev,
> +			"ignoreonce (%d)\n",
> +			xport->ignoreonce);
> +		xport->ignoreonce = 0;
> +		goto exit;
> +	}
>  	for (retries = 5; retries > 0; retries--) {
>  		data->writeReport[0] = RMI_READ_ADDR_REPORT_ID;
>  		data->writeReport[1] = 0; /* old 1 byte read count */
> @@ -468,8 +474,12 @@ static int rmi_post_resume(struct hid_device *hdev)
>  	ret = hid_hw_open(hdev);
>  	if (ret)
>  		return ret;
> -
> +	// Avoid to read rmi_hid_read_block once after system resumes.
> +	// The interrupt will be pulled down
> +	// after RMI Read command(Report ID:0x0A).
> +	data->xport.ignoreonce = 1;
>  	ret = rmi_reset_attn_mode(hdev);
> +	data->xport.ignoreonce = 0;
>  	if (ret)
>  		goto out;
>  
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index ab7eea01ab42..24f63ad00970 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -270,6 +270,8 @@ struct rmi_transport_dev {
>  	struct rmi_device_platform_data pdata;
>  
>  	struct input_dev *input;
> +
> +	int ignoreonce;
>  };
>  
>  /**
diff mbox series

Patch

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 311eee599ce9..fdaf2c4ce181 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -203,7 +203,13 @@  static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
 		if (ret < 0)
 			goto exit;
 	}
-
+	if (xport->ignoreonce == 1) {
+		dev_err(&hdev->dev,
+			"ignoreonce (%d)\n",
+			xport->ignoreonce);
+		xport->ignoreonce = 0;
+		goto exit;
+	}
 	for (retries = 5; retries > 0; retries--) {
 		data->writeReport[0] = RMI_READ_ADDR_REPORT_ID;
 		data->writeReport[1] = 0; /* old 1 byte read count */
@@ -468,8 +474,12 @@  static int rmi_post_resume(struct hid_device *hdev)
 	ret = hid_hw_open(hdev);
 	if (ret)
 		return ret;
-
+	// Avoid to read rmi_hid_read_block once after system resumes.
+	// The interrupt will be pulled down
+	// after RMI Read command(Report ID:0x0A).
+	data->xport.ignoreonce = 1;
 	ret = rmi_reset_attn_mode(hdev);
+	data->xport.ignoreonce = 0;
 	if (ret)
 		goto out;
 
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index ab7eea01ab42..24f63ad00970 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -270,6 +270,8 @@  struct rmi_transport_dev {
 	struct rmi_device_platform_data pdata;
 
 	struct input_dev *input;
+
+	int ignoreonce;
 };
 
 /**