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

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

[margeyang]

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, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 311eee599ce9..45eacb0b8dae 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -190,7 +190,7 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
 {
 	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
 	struct hid_device *hdev = data->hdev;
-	int ret;
+	int ret = 0;
 	int bytes_read;
 	int bytes_needed;
 	int retries;
@@ -204,6 +204,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
 			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 */
@@ -469,7 +476,12 @@ static int rmi_post_resume(struct hid_device *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

Re: [PATCH V4] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

[Hans de Goede]

Hi,

On 8/15/22 12:26, 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, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 311eee599ce9..45eacb0b8dae 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -190,7 +190,7 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  {
>  	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
>  	struct hid_device *hdev = data->hdev;
> -	int ret;
> +	int ret = 0;
>  	int bytes_read;
>  	int bytes_needed;
>  	int retries;
> @@ -204,6 +204,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  			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 */
> @@ -469,7 +476,12 @@ static int rmi_post_resume(struct hid_device *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;
>  };
>  
>  /**

Re: [PATCH V4] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

[Benjamin Tissoires]

On Aug 15 2022, 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>
> ---

I have fixed your signed-off-by line by adding a space between your name
and address, and converted the C++ style comments into proper multiline
comments, and applied to for-6.1/rmi in hid.git

Sorry for the delay, this one went through the cracks.

Cheers,
Benjamin

>  drivers/hid/hid-rmi.c | 14 +++++++++++++-
>  include/linux/rmi.h   |  2 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 311eee599ce9..45eacb0b8dae 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -190,7 +190,7 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  {
>  	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
>  	struct hid_device *hdev = data->hdev;
> -	int ret;
> +	int ret = 0;
>  	int bytes_read;
>  	int bytes_needed;
>  	int retries;
> @@ -204,6 +204,13 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  			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 */
> @@ -469,7 +476,12 @@ static int rmi_post_resume(struct hid_device *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
> 

Re: [PATCH V4] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

[Dmitry Torokhov]

Hi Benjamin,

On Wed, Sep 21, 2022 at 05:11:43PM +0200, Benjamin Tissoires wrote:
> On Aug 15 2022, 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>
> > ---
> 
> I have fixed your signed-off-by line by adding a space between your name
> and address, and converted the C++ style comments into proper multiline
> comments, and applied to for-6.1/rmi in hid.git
> 
> Sorry for the delay, this one went through the cracks.

I think we are rushing with this. There are questions whether the ACPI
data for the device is generated properly and also whether we should be
smarted when counting wakeup events in case interrupt that is
potentially wakeup-capable  happens in the middle of the resume process.

The patch is not a fix for behavior that affects users, but rather a
band-aid to appease a Chrome OS test, which is IMO is a weak reason for
accepting the patch.

Thanks.

-- 
Dmitry

Re: [PATCH V4] HID: HID-rmi - ignore to rmi_hid_read_block after system resumes.

[Benjamin Tissoires]

On Thu, Sep 22, 2022 at 12:51 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin,
>
> On Wed, Sep 21, 2022 at 05:11:43PM +0200, Benjamin Tissoires wrote:
> > On Aug 15 2022, 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>
> > > ---
> >
> > I have fixed your signed-off-by line by adding a space between your name
> > and address, and converted the C++ style comments into proper multiline
> > comments, and applied to for-6.1/rmi in hid.git
> >
> > Sorry for the delay, this one went through the cracks.
>
> I think we are rushing with this. There are questions whether the ACPI
> data for the device is generated properly and also whether we should be
> smarted when counting wakeup events in case interrupt that is
> potentially wakeup-capable  happens in the middle of the resume process.
>
> The patch is not a fix for behavior that affects users, but rather a
> band-aid to appease a Chrome OS test, which is IMO is a weak reason for
> accepting the patch.

All right, fair enough.

I'll drop it from the for-6.1/rmi branch and for-next then. I thought
Marge's explanations in v3 were convincing enough but I don't have
visibility on the ChromeOS bugs.

Cheers,
Benjamin