On Monday 06 July 2015 16:47:57 Andrew Duggan wrote:
> Hi Gabriele,
>
> On 07/06/2015 03:20 AM, Gabriele Mazzotta wrote:
> > Hi,
> >
> > I recently noticed that there's a minor issue with hid-rmi.c. After a
> > suspend/resume cycle the f11 control register is set to the default
> > configuration, thus undoing the changes performed on init.
>
> This is because i2c_hid does a reset of the touchpad during resume.
> Power cycles or resets will clear the changes to the control registers.
> There isn't a way to make these changes persistent without changing the
> firmware.
OK, I suspected this was what was happening.
> > I made some changes to the driver to prevent this from happening: the
> > configuration is saved on suspend and restored upon resume. This seemed
> > the simplest thing to do, but I encountered a small problem.
>
> I haven't been able to successfully complete reads which I perform in
> the suspend callback. They end up timing out waiting for the response.
> Maybe this is only an issue on certain platforms if this is working for you.
I have the same problem and I solved it with the following patch.
Please see if it works for you as well:
>From 654156ae5ed496daba792b4231858c788712df15 Mon Sep 17 00:00:00 2001
From: Gabriele Mazzotta <gabriele....@gmail.com>
Date: Sat, 27 Jun 2015 16:29:45 +0200
Subject: [PATCH] hid: i2c-hid - call suspend callback before disabling irq
This will make possible to perform operations from the device suspend
callback that needs the irq to be enabled.
---
drivers/hid/i2c-hid/i2c-hid.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index f77469d..9ed69b5 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -1092,13 +1092,13 @@ static int i2c_hid_suspend(struct device *dev)
struct hid_device *hid = ihid->hid;
int ret = 0;
+ if (hid->driver && hid->driver->suspend)
+ ret = hid->driver->suspend(hid, PMSG_SUSPEND);
+
disable_irq(ihid->irq);
if (device_may_wakeup(&client->dev))
enable_irq_wake(ihid->irq);
- if (hid->driver && hid->driver->suspend)
- ret = hid->driver->suspend(hid, PMSG_SUSPEND);
-
/* Save some power */
i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
--
> >
> > I'm saving and writing the whole register since the kernel can't know
> > what userspace tools might have done. According to a comment in the
> > sources, some firmwares split the control register, so blindly copying
> > and writing 20 sequential bytes as I'm doing could be a problem.
>
> Since reading from the suspend callback doesn't seem to be reliable on
> all platforms, I think it would be better to store the values of the
> control registers on init. The driver can update the stored values and
> write that back to the device on init and after coming out of resume.
> This will overwrite any changes done by userspace tools. But, if it is
> important enough to have a F11 control register change persist over
> suspend / resume then it should probably be implemented in the hid-rmi
> anyway.
>
> > Is there a way to recognize those firmwares? Or even better, is there a
> > way to prevent the firmware from restoring the default configuration?
>
> This bug can be worked around by only reading a max of 16 bytes at a
> time. So to read all 20 you can just read 16, then add 16 to the address
> and read the remaining 4. Also, the size of the control registers
> depends on the configuration so it could be more or less then 20. Did
> you have a particular register that you want to change which isn't
> currently in hid-rmi?
No, only what's currently in hid-rmi.
> > PS: I didn't check if the same happens with other registers, but I
> > suspenct it does.
> >
> > Thanks,
> > Gabriele
>
> In the meantime I have another patch to post related to suspend /
> resume. I'm going to go ahead and post that now to avoid conflicts.
>
> Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
