| Message ID | 20201211222448.2115188-1-dianders@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: i2c-hid: Reorganize to allow supporting goodix, gt7375p | expand |
Benjamin, On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <dianders@chromium.org> wrote: > > The goal of this series is to support the Goodix GT7375P touchscreen. > This touchscreen is special because it has power sequencing > requirements that necessitate driving a reset GPIO. > > To do this, we totally rejigger the way i2c-hid is organized so that > it's easier to jam the Goodix support in there. > > This series was: > - Tested on a device that uses normal i2c-hid. > - Tested on a device that has a Goodix i2c-hid device. > - Tested on an ACPI device, but an earlier version of the series. > > I believe the plan is for Benjamin to land the whole series. Will > said this about the arm64 defconfig change (and provided his Ack): > > ...there are a few things I really care about > > in defconfig (e.g. things like page size!), generally speaking we don't > > need to Ack everything that changes in there. > > > > That said, might be worth checking whether arm-soc have any defconfig > > changes queued in -next so you don't end up with conflicts. > > Changes in v8: > - Mark suspend/resume as static as per patches robot. > > Changes in v7: > - Rebase atop commit afdd34c5fa40 ("HID: i2c-hid: show the error ...") > > Changes in v6: > - ACPI probe function should have been "static" > - Don't export suspend/resume, just export dev_pm_ops from core. > - Fixed crash in ACPI module (missing init of "client") > - No need for regulator include in the core. > - Removed i2c_device_id table from ACPI module. > - Suspend/resume are no longer exported from the core. > > Changes in v5: > - Add shutdown_tail op and use it in ACPI. > - Added mention of i2c-hid in the yaml itself as per Rob. > - Adjusted subject as per Rob. > - i2chid_subclass_data => i2chid_ops. > - power_up_device => power_up (same with power_down). > - subclass => ops. > > Changes in v4: > - ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4. > - Fully rejigger so ACPI and OF are full subclasses. > - Totally redid based on the new subclass system. > > Changes in v3: > - Fixed compatible in example. > - Removed Benjamin as a maintainer. > - Rework to use subclassing. > - Updated description. > > Changes in v2: > - ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2. > - Get timings based on the compatible string. > - Use a separate compatible string for this new touchscreen. > > Douglas Anderson (4): > HID: i2c-hid: Reorganize so ACPI and OF are separate modules > arm64: defconfig: Update config names for i2c-hid rejigger > dt-bindings: input: HID: i2c-hid: Introduce bindings for the Goodix > GT7375P > HID: i2c-hid: Introduce goodix-i2c-hid using i2c-hid core I think this series is ready to land. The "defconfig" has a trivial conflict with commit 74b87103b3d0 ("arm64: defconfig: Enable HID multitouch") against linuxnext, but it's so simple that hopefully folks will be OK with that when it lands. Please let me know if there's anything else you need me to do. :-) -Doug
Hi Doug, On Wed, Jan 6, 2021 at 2:35 AM Doug Anderson <dianders@chromium.org> wrote: > > Benjamin, > > On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > The goal of this series is to support the Goodix GT7375P touchscreen. > > This touchscreen is special because it has power sequencing > > requirements that necessitate driving a reset GPIO. > > > > To do this, we totally rejigger the way i2c-hid is organized so that > > it's easier to jam the Goodix support in there. > > > > This series was: > > - Tested on a device that uses normal i2c-hid. > > - Tested on a device that has a Goodix i2c-hid device. > > - Tested on an ACPI device, but an earlier version of the series. > > > > I believe the plan is for Benjamin to land the whole series. Will > > said this about the arm64 defconfig change (and provided his Ack): > > > ...there are a few things I really care about > > > in defconfig (e.g. things like page size!), generally speaking we don't > > > need to Ack everything that changes in there. > > > > > > That said, might be worth checking whether arm-soc have any defconfig > > > changes queued in -next so you don't end up with conflicts. > > > > Changes in v8: > > - Mark suspend/resume as static as per patches robot. > > > > Changes in v7: > > - Rebase atop commit afdd34c5fa40 ("HID: i2c-hid: show the error ...") > > > > Changes in v6: > > - ACPI probe function should have been "static" > > - Don't export suspend/resume, just export dev_pm_ops from core. > > - Fixed crash in ACPI module (missing init of "client") > > - No need for regulator include in the core. > > - Removed i2c_device_id table from ACPI module. > > - Suspend/resume are no longer exported from the core. > > > > Changes in v5: > > - Add shutdown_tail op and use it in ACPI. > > - Added mention of i2c-hid in the yaml itself as per Rob. > > - Adjusted subject as per Rob. > > - i2chid_subclass_data => i2chid_ops. > > - power_up_device => power_up (same with power_down). > > - subclass => ops. > > > > Changes in v4: > > - ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4. > > - Fully rejigger so ACPI and OF are full subclasses. > > - Totally redid based on the new subclass system. > > > > Changes in v3: > > - Fixed compatible in example. > > - Removed Benjamin as a maintainer. > > - Rework to use subclassing. > > - Updated description. > > > > Changes in v2: > > - ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2. > > - Get timings based on the compatible string. > > - Use a separate compatible string for this new touchscreen. > > > > Douglas Anderson (4): > > HID: i2c-hid: Reorganize so ACPI and OF are separate modules > > arm64: defconfig: Update config names for i2c-hid rejigger > > dt-bindings: input: HID: i2c-hid: Introduce bindings for the Goodix > > GT7375P > > HID: i2c-hid: Introduce goodix-i2c-hid using i2c-hid core > > I think this series is ready to land. The "defconfig" has a trivial > conflict with commit 74b87103b3d0 ("arm64: defconfig: Enable HID > multitouch") against linuxnext, but it's so simple that hopefully > folks will be OK with that when it lands. > > Please let me know if there's anything else you need me to do. :-) > I wanted to apply the series yesterday, but for these kinds of changes I like giving it a spin on actual hardware. Turns out that my XPS-13 can not boot to v5.11-rc2, which makes testing the new branch slightly more difficult. I'll give it a spin next week, but I think I should be able to land it for 5.12. Regarding the defconfig conflict, no worries, we can handle it with Stephen and Linus. Cheers, Benjamin
On Fri, Jan 8, 2021 at 6:52 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Doug, > > On Wed, Jan 6, 2021 at 2:35 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Benjamin, > > > > On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > The goal of this series is to support the Goodix GT7375P touchscreen. > > > This touchscreen is special because it has power sequencing > > > requirements that necessitate driving a reset GPIO. > > > > > > To do this, we totally rejigger the way i2c-hid is organized so that > > > it's easier to jam the Goodix support in there. > > > > > > This series was: > > > - Tested on a device that uses normal i2c-hid. > > > - Tested on a device that has a Goodix i2c-hid device. > > > - Tested on an ACPI device, but an earlier version of the series. > > > > > > I believe the plan is for Benjamin to land the whole series. Will > > > said this about the arm64 defconfig change (and provided his Ack): > > > > ...there are a few things I really care about > > > > in defconfig (e.g. things like page size!), generally speaking we don't > > > > need to Ack everything that changes in there. > > > > > > > > That said, might be worth checking whether arm-soc have any defconfig > > > > changes queued in -next so you don't end up with conflicts. > > > > > > Changes in v8: > > > - Mark suspend/resume as static as per patches robot. > > > > > > Changes in v7: > > > - Rebase atop commit afdd34c5fa40 ("HID: i2c-hid: show the error ...") > > > > > > Changes in v6: > > > - ACPI probe function should have been "static" > > > - Don't export suspend/resume, just export dev_pm_ops from core. > > > - Fixed crash in ACPI module (missing init of "client") > > > - No need for regulator include in the core. > > > - Removed i2c_device_id table from ACPI module. > > > - Suspend/resume are no longer exported from the core. > > > > > > Changes in v5: > > > - Add shutdown_tail op and use it in ACPI. > > > - Added mention of i2c-hid in the yaml itself as per Rob. > > > - Adjusted subject as per Rob. > > > - i2chid_subclass_data => i2chid_ops. > > > - power_up_device => power_up (same with power_down). > > > - subclass => ops. > > > > > > Changes in v4: > > > - ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4. > > > - Fully rejigger so ACPI and OF are full subclasses. > > > - Totally redid based on the new subclass system. > > > > > > Changes in v3: > > > - Fixed compatible in example. > > > - Removed Benjamin as a maintainer. > > > - Rework to use subclassing. > > > - Updated description. > > > > > > Changes in v2: > > > - ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2. > > > - Get timings based on the compatible string. > > > - Use a separate compatible string for this new touchscreen. > > > > > > Douglas Anderson (4): > > > HID: i2c-hid: Reorganize so ACPI and OF are separate modules > > > arm64: defconfig: Update config names for i2c-hid rejigger > > > dt-bindings: input: HID: i2c-hid: Introduce bindings for the Goodix > > > GT7375P > > > HID: i2c-hid: Introduce goodix-i2c-hid using i2c-hid core > > > > I think this series is ready to land. The "defconfig" has a trivial > > conflict with commit 74b87103b3d0 ("arm64: defconfig: Enable HID > > multitouch") against linuxnext, but it's so simple that hopefully > > folks will be OK with that when it lands. > > > > Please let me know if there's anything else you need me to do. :-) > > > > I wanted to apply the series yesterday, but for these kinds of changes > I like giving it a spin on actual hardware. Turns out that my XPS-13 > can not boot to v5.11-rc2, which makes testing the new branch slightly > more difficult. > > I'll give it a spin next week, but I think I should be able to land it for 5.12. > > Regarding the defconfig conflict, no worries, we can handle it with > Stephen and Linus. > After 2 full kernel bisects (I messed up the first because I am an idiot and inverted good and bad after the first reboot), I found my culprit, and I was able to test the series today. The series works fine regarding enumeration and removing of devices, but it prevents my system from being suspended. If I rmmod i2c-hid-acpi, suspend works fine, but if it is present, it immediately comes back, which makes me think that something must be wrong. I also just reverted the series and confirmed that suspend/resume now works, meaning that patch 1/4 needs to be checked. Cheers, Benjamin
Hi, On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > I wanted to apply the series yesterday, but for these kinds of changes > > I like giving it a spin on actual hardware. Turns out that my XPS-13 > > can not boot to v5.11-rc2, which makes testing the new branch slightly > > more difficult. > > > > I'll give it a spin next week, but I think I should be able to land it for 5.12. > > > > Regarding the defconfig conflict, no worries, we can handle it with > > Stephen and Linus. > > > > After 2 full kernel bisects (I messed up the first because I am an > idiot and inverted good and bad after the first reboot), I found my > culprit, and I was able to test the series today. > > The series works fine regarding enumeration and removing of devices, > but it prevents my system from being suspended. If I rmmod > i2c-hid-acpi, suspend works fine, but if it is present, it immediately > comes back, which makes me think that something must be wrong. > > I also just reverted the series and confirmed that suspend/resume now > works, meaning that patch 1/4 needs to be checked. Can you give me any hints about what type of failure you're seeing? Any logs? I don't have an ACPI system to test with... Is there any chance that some type of userspace / udev rule is getting tripped up by the driver being renamed? We ran into something like this recently on Chrome OS where we had a tool that was hardcoded to look for "i2c-hid" and needed to be adapted to account for the new driver name. Often userspace tweaks with wakeup rules based on driver name... I'll go stare at the code now and see if anything jumps out. -Doug
On Wed, Jan 13, 2021 at 5:05 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > > I wanted to apply the series yesterday, but for these kinds of changes > > > I like giving it a spin on actual hardware. Turns out that my XPS-13 > > > can not boot to v5.11-rc2, which makes testing the new branch slightly > > > more difficult. > > > > > > I'll give it a spin next week, but I think I should be able to land it for 5.12. > > > > > > Regarding the defconfig conflict, no worries, we can handle it with > > > Stephen and Linus. > > > > > > > After 2 full kernel bisects (I messed up the first because I am an > > idiot and inverted good and bad after the first reboot), I found my > > culprit, and I was able to test the series today. > > > > The series works fine regarding enumeration and removing of devices, > > but it prevents my system from being suspended. If I rmmod > > i2c-hid-acpi, suspend works fine, but if it is present, it immediately > > comes back, which makes me think that something must be wrong. > > > > I also just reverted the series and confirmed that suspend/resume now > > works, meaning that patch 1/4 needs to be checked. > > Can you give me any hints about what type of failure you're seeing? > Any logs? I don't have an ACPI system to test with... I don't have any logs, just that the system comes back up. There is a chance we are not powering the device down correctly, which triggers an IRQ and which puts the system back on. > > Is there any chance that some type of userspace / udev rule is getting > tripped up by the driver being renamed? We ran into something like > this recently on Chrome OS where we had a tool that was hardcoded to > look for "i2c-hid" and needed to be adapted to account for the new > driver name. Often userspace tweaks with wakeup rules based on driver > name... I don't think there is anything like that on a regular desktop. > > I'll go stare at the code now and see if anything jumps out. > Thanks, but don't spend too much time on it, unless something really jumps out. I'll debug that tomorrow. It's much easier with an actual device than by just looking at the code. Cheers, Benjamin > -Doug >
Hi, On Wed, Jan 13, 2021 at 8:35 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Jan 13, 2021 at 5:05 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > I wanted to apply the series yesterday, but for these kinds of changes > > > > I like giving it a spin on actual hardware. Turns out that my XPS-13 > > > > can not boot to v5.11-rc2, which makes testing the new branch slightly > > > > more difficult. > > > > > > > > I'll give it a spin next week, but I think I should be able to land it for 5.12. > > > > > > > > Regarding the defconfig conflict, no worries, we can handle it with > > > > Stephen and Linus. > > > > > > > > > > After 2 full kernel bisects (I messed up the first because I am an > > > idiot and inverted good and bad after the first reboot), I found my > > > culprit, and I was able to test the series today. > > > > > > The series works fine regarding enumeration and removing of devices, > > > but it prevents my system from being suspended. If I rmmod > > > i2c-hid-acpi, suspend works fine, but if it is present, it immediately > > > comes back, which makes me think that something must be wrong. > > > > > > I also just reverted the series and confirmed that suspend/resume now > > > works, meaning that patch 1/4 needs to be checked. > > > > Can you give me any hints about what type of failure you're seeing? > > Any logs? I don't have an ACPI system to test with... > > I don't have any logs, just that the system comes back up. There is a > chance we are not powering the device down correctly, which triggers > an IRQ and which puts the system back on. > > > > > Is there any chance that some type of userspace / udev rule is getting > > tripped up by the driver being renamed? We ran into something like > > this recently on Chrome OS where we had a tool that was hardcoded to > > look for "i2c-hid" and needed to be adapted to account for the new > > driver name. Often userspace tweaks with wakeup rules based on driver > > name... > > I don't think there is anything like that on a regular desktop. > > > > > I'll go stare at the code now and see if anything jumps out. > > > > Thanks, but don't spend too much time on it, unless something really > jumps out. I'll debug that tomorrow. It's much easier with an actual > device than by just looking at the code. > Well, that's weird. Now suspend resume works reliably even with your series. It could just have been that the lid sensor was too close to a magnet or something like that. Though while testing the old version of i2c-hid, it was working... Such a mystery :) Anyway, while trying to dig up that now-non-issue, I got the following patch that you likely want to squash into 1/4: --- diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c index 0f86060f01b4..dd6d9f74e7e7 100644 --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c @@ -31,7 +31,6 @@ struct i2c_hid_acpi { struct i2chid_ops ops; struct i2c_client *client; - bool power_fixed; }; static const struct acpi_device_id i2c_hid_acpi_blacklist[] = { @@ -75,25 +74,6 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_client *client) return hid_descriptor_address; } -static int i2c_hid_acpi_power_up(struct i2chid_ops *ops) -{ - struct i2c_hid_acpi *ihid_of = - container_of(ops, struct i2c_hid_acpi, ops); - struct device *dev = &ihid_of->client->dev; - struct acpi_device *adev; - - /* Only need to call acpi_device_fix_up_power() the first time */ - if (ihid_of->power_fixed) - return 0; - ihid_of->power_fixed = true; - - adev = ACPI_COMPANION(dev); - if (adev) - acpi_device_fix_up_power(adev); - - return 0; -} - static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops) { struct i2c_hid_acpi *ihid_of = @@ -107,6 +87,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct i2c_hid_acpi *ihid_acpi; + struct acpi_device *adev; u16 hid_descriptor_address; int ret; @@ -115,7 +96,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, return -ENOMEM; ihid_acpi->client = client; - ihid_acpi->ops.power_up = i2c_hid_acpi_power_up; ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail; ret = i2c_hid_acpi_get_descriptor(client); @@ -123,6 +103,10 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, return ret; hid_descriptor_address = ret; + adev = ACPI_COMPANION(dev); + if (adev) + acpi_device_fix_up_power(adev); + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { device_set_wakeup_capable(dev, true); device_set_wakeup_enable(dev, false); --- This allows to keep the powering ordering of the old i2c-hid module (power up before setting device wakeup capable), and simplify the not so obvious power_fixed field of struct i2c_hid_acpi. (I can also send it as a followup on the series if you prefer). Cheers, Benjamin
Hi, On Fri, Jan 15, 2021 at 6:58 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > Thanks, but don't spend too much time on it, unless something really > > jumps out. I'll debug that tomorrow. It's much easier with an actual > > device than by just looking at the code. > > > > Well, that's weird. Now suspend resume works reliably even with your > series. It could just have been that the lid sensor was too close to a > magnet or something like that. Though while testing the old version of > i2c-hid, it was working... Such a mystery :) Friggin magnets, how do those work? ;-) I also managed to obtain remote access to a device with an ACPI i2c-hid device and confirmed that suspend/resume was working and that I saw no errors, though obviously I couldn't physically interact with the device remotely. Hopefully that gives a tiny bit of extra confidence that the series is OK... > This allows to keep the powering ordering of the old i2c-hid module > (power up before setting device wakeup capable), and simplify the > not so obvious power_fixed field of struct i2c_hid_acpi. > > (I can also send it as a followup on the series if you prefer). Squashed it into a v9 as well as a local variable rename that I noticed while looking at the code with fresh eyes. My v9 also incorporates the new Goodix timing that I self-commented about on v8. Crossing fingers that it's all good now. :-) -Doug