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
.../bindings/input/goodix,gt7375p.yaml | 65 +++++
arch/arm64/configs/defconfig | 3 +-
drivers/hid/Makefile | 2 +-
drivers/hid/i2c-hid/Kconfig | 47 +++-
drivers/hid/i2c-hid/Makefile | 6 +-
drivers/hid/i2c-hid/i2c-hid-acpi.c | 159 +++++++++++
drivers/hid/i2c-hid/i2c-hid-core.c | 252 +++---------------
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 116 ++++++++
drivers/hid/i2c-hid/i2c-hid-of.c | 143 ++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 22 ++
include/linux/platform_data/i2c-hid.h | 41 ---
11 files changed, 594 insertions(+), 262 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
create mode 100644 drivers/hid/i2c-hid/i2c-hid-acpi.c
create mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
create mode 100644 drivers/hid/i2c-hid/i2c-hid-of.c
delete mode 100644 include/linux/platform_data/i2c-hid.h
--
2.29.2.576.ga3fc446d84-goog
Goodix i2c-hid touchscreens are mostly i2c-hid compliant but have some
special power sequencing requirements, including the need to drive a
reset line during the sequencing.
Let's use the new rejiggering of i2c-hid to support this with a thin
wrapper driver to support the first Goodix i2c-hid touchscreen:
GT7375P
Signed-off-by: Douglas Anderson <[email protected]>
---
(no changes since v6)
Changes in v6:
- Suspend/resume are no longer exported from the core.
Changes in v5:
- i2chid_subclass_data => i2chid_ops.
- power_up_device => power_up (same with power_down).
- subclass => ops.
Changes in v4:
- Totally redid based on the new subclass system.
Changes in v3:
- Rework to use subclassing.
drivers/hid/i2c-hid/Kconfig | 19 +++-
drivers/hid/i2c-hid/Makefile | 1 +
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 116 ++++++++++++++++++++++++
3 files changed, 134 insertions(+), 2 deletions(-)
create mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 819b7521c182..a16c6a69680b 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -32,10 +32,25 @@ config I2C_HID_OF
will be called i2c-hid-of. It will also build/depend on the
module i2c-hid.
+config I2C_HID_OF_GOODIX
+ tristate "Driver for Goodix hid-i2c based devices on OF systems"
+ default n
+ depends on I2C && INPUT && OF
+ help
+ Say Y here if you want support for Goodix i2c devices that use
+ the i2c-hid protocol on Open Firmware (Device Tree)-based
+ systems.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the module
+ will be called i2c-hid-of-goodix. It will also build/depend on
+ the module i2c-hid.
+
endmenu
config I2C_HID_CORE
tristate
- default y if I2C_HID_ACPI=y || I2C_HID_OF=y
- default m if I2C_HID_ACPI=m || I2C_HID_OF=m
+ default y if I2C_HID_ACPI=y || I2C_HID_OF=y || I2C_HID_OF_GOODIX=y
+ default m if I2C_HID_ACPI=m || I2C_HID_OF=m || I2C_HID_OF_GOODIX=m
select HID
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 9b4a73446841..302545a771f3 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -10,3 +10,4 @@ i2c-hid-$(CONFIG_DMI) += i2c-hid-dmi-quirks.o
obj-$(CONFIG_I2C_HID_ACPI) += i2c-hid-acpi.o
obj-$(CONFIG_I2C_HID_OF) += i2c-hid-of.o
+obj-$(CONFIG_I2C_HID_OF_GOODIX) += i2c-hid-of-goodix.o
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
new file mode 100644
index 000000000000..7cc51c25c609
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Goodix touchscreens that use the i2c-hid protocol.
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+
+#include "i2c-hid.h"
+
+struct goodix_i2c_hid_timing_data {
+ unsigned int post_gpio_reset_delay_ms;
+ unsigned int post_power_delay_ms;
+};
+
+struct i2c_hid_of_goodix {
+ struct i2chid_ops ops;
+
+ struct regulator *vdd;
+ struct gpio_desc *reset_gpio;
+ const struct goodix_i2c_hid_timing_data *timings;
+};
+
+static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
+{
+ struct i2c_hid_of_goodix *ihid_goodix =
+ container_of(ops, struct i2c_hid_of_goodix, ops);
+ int ret;
+
+ ret = regulator_enable(ihid_goodix->vdd);
+ if (ret)
+ return ret;
+
+ if (ihid_goodix->timings->post_power_delay_ms)
+ msleep(ihid_goodix->timings->post_power_delay_ms);
+
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
+ if (ihid_goodix->timings->post_gpio_reset_delay_ms)
+ msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
+
+ return 0;
+}
+
+static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
+{
+ struct i2c_hid_of_goodix *ihid_goodix =
+ container_of(ops, struct i2c_hid_of_goodix, ops);
+
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ regulator_disable(ihid_goodix->vdd);
+}
+
+static int i2c_hid_of_goodix_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct i2c_hid_of_goodix *ihid_goodix;
+
+ ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
+ GFP_KERNEL);
+ if (!ihid_goodix)
+ return -ENOMEM;
+
+ ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
+ ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
+
+ /* Start out with reset asserted */
+ ihid_goodix->reset_gpio =
+ devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ihid_goodix->reset_gpio))
+ return PTR_ERR(ihid_goodix->reset_gpio);
+
+ ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
+ if (IS_ERR(ihid_goodix->vdd))
+ return PTR_ERR(ihid_goodix->vdd);
+
+ ihid_goodix->timings = device_get_match_data(&client->dev);
+
+ return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001);
+}
+
+static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
+ .post_power_delay_ms = 10,
+ .post_gpio_reset_delay_ms = 120,
+};
+
+static const struct of_device_id goodix_i2c_hid_of_match[] = {
+ { .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
+
+static struct i2c_driver goodix_i2c_hid_ts_driver = {
+ .driver = {
+ .name = "i2c_hid_of_goodix",
+ .pm = &i2c_hid_core_pm,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
+ },
+ .probe = i2c_hid_of_goodix_probe,
+ .remove = i2c_hid_core_remove,
+ .shutdown = i2c_hid_core_shutdown,
+};
+module_i2c_driver(goodix_i2c_hid_ts_driver);
+
+MODULE_AUTHOR("Douglas Anderson <[email protected]>");
+MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
+MODULE_LICENSE("GPL v2");
--
2.29.2.576.ga3fc446d84-goog
This adds new bindings for the Goodix GT7375P touchscreen. While this
touchscreen's communications are based on the generic "i2c-over-hid"
protocol, it needs special power sequencing and thus gets its own
compatible and bindings.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
(no changes since v5)
Changes in v5:
- Added mention of i2c-hid in the yaml itself as per Rob.
- Adjusted subject as per Rob.
Changes in v3:
- Fixed compatible in example.
- Removed Benjamin as a maintainer.
- Updated description.
Changes in v2:
- ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2.
.../bindings/input/goodix,gt7375p.yaml | 65 +++++++++++++++++++
1 file changed, 65 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
new file mode 100644
index 000000000000..fe1c5016f7f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/goodix,gt7375p.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix GT7375P touchscreen
+
+maintainers:
+ - Douglas Anderson <[email protected]>
+
+description:
+ Supports the Goodix GT7375P touchscreen.
+ This touchscreen uses the i2c-hid protocol but has some non-standard
+ power sequencing required.
+
+properties:
+ compatible:
+ items:
+ - const: goodix,gt7375p
+
+ reg:
+ enum:
+ - 0x5d
+ - 0x14
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ true
+
+ vdd-supply:
+ description: The 3.3V supply to the touchscreen.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,rpmh.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ap_ts: touchscreen@5d {
+ compatible = "goodix,gt7375p";
+ reg = <0x5d>;
+
+ interrupt-parent = <&tlmm>;
+ interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+
+ reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
+ vdd-supply = <&pp3300_ts>;
+ };
+ };
--
2.29.2.576.ga3fc446d84-goog
Benjamin,
On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <[email protected]> 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,
On Fri, Dec 11, 2020 at 2:25 PM Douglas Anderson <[email protected]> wrote:
>
> +static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
> + .post_power_delay_ms = 10,
> + .post_gpio_reset_delay_ms = 120,
As I've been going through my holiday email I found that Goodix has
released a new datasheet (datasheet rev 0.6 from 2020-12-11) where
this moves from 120 ms to 180 ms. Sigh.
I'm hoping that we can just land it with the 120 though and then I'll
send a follow-up patch to change to 180. This will avoid spamming
everyone with a v9 of the patch series. Please yell if there are any
objections to that plan. I'd also be fine if you wanted to just
change it to 180 when landing it.
Thanks much!
-Doug
Hi Doug,
On Wed, Jan 6, 2021 at 2:35 AM Doug Anderson <[email protected]> wrote:
>
> Benjamin,
>
> On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <[email protected]> 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
<[email protected]> wrote:
>
> Hi Doug,
>
> On Wed, Jan 6, 2021 at 2:35 AM Doug Anderson <[email protected]> wrote:
> >
> > Benjamin,
> >
> > On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <[email protected]> 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
<[email protected]> 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 <[email protected]> wrote:
>
> Hi,
>
> On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires
> <[email protected]> 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 <[email protected]> wrote:
>
> On Wed, Jan 13, 2021 at 5:05 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires
> > <[email protected]> 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
<[email protected]> 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