[PATCH v3 1/3] dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P

[PATCH v3 1/3] dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P

[Douglas Anderson]

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 <dianders@chromium.org>
---

Changes in v3:
- Removed Benjamin as a maintainer.
- Fixed compatible in example.
- Updated description.

Changes in v2:
- ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2.

 .../bindings/input/goodix,gt7375p.yaml        | 63 +++++++++++++++++++
 1 file changed, 63 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..15a38516e594
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -0,0 +1,63 @@
+# 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 <dianders@chromium.org>
+
+description:
+  Supports the Goodix GT7375P touchscreen.
+
+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.1.341.ge80a0c044ae-goog

[PATCH v3 2/3] HID: i2c-hid: Allow subclasses of i2c-hid for power sequencing

[Douglas Anderson]

This exports some things from i2c-hid so that we can have a driver
that's effectively a subclass of it and that can do its own power
sequencing.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Rework to use subclassing.

Changes in v2:
- Use a separate compatible string for this new touchscreen.
- Get timings based on the compatible string.

 drivers/hid/i2c-hid/i2c-hid-core.c    | 78 +++++++++++++++++----------
 include/linux/input/i2c-hid-core.h    | 19 +++++++
 include/linux/platform_data/i2c-hid.h |  9 ++++
 3 files changed, 79 insertions(+), 27 deletions(-)
 create mode 100644 include/linux/input/i2c-hid-core.h

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 786e3e9af1c9..910e9089fcf8 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -22,6 +22,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/input.h>
+#include <linux/input/i2c-hid-core.h>
 #include <linux/irq.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
@@ -1007,8 +1008,33 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
 		pdata->post_power_delay_ms = val;
 }
 
-static int i2c_hid_probe(struct i2c_client *client,
-			 const struct i2c_device_id *dev_id)
+static int i2c_hid_power_up_device(struct i2c_hid_platform_data *pdata)
+{
+	struct i2c_hid *ihid = container_of(pdata, struct i2c_hid, pdata);
+	struct hid_device *hid = ihid->hid;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
+				    pdata->supplies);
+	if (ret) {
+		if (hid)
+			hid_warn(hid, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	if (pdata->post_power_delay_ms)
+		msleep(pdata->post_power_delay_ms);
+
+	return 0;
+}
+
+static void i2c_hid_power_down_device(struct i2c_hid_platform_data *pdata)
+{
+	regulator_bulk_disable(ARRAY_SIZE(pdata->supplies), pdata->supplies);
+}
+
+int i2c_hid_probe(struct i2c_client *client,
+		  const struct i2c_device_id *dev_id)
 {
 	int ret;
 	struct i2c_hid *ihid;
@@ -1035,6 +1061,9 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
+	if (platform_data)
+		ihid->pdata = *platform_data;
+
 	if (client->dev.of_node) {
 		ret = i2c_hid_of_probe(client, &ihid->pdata);
 		if (ret)
@@ -1043,13 +1072,16 @@ static int i2c_hid_probe(struct i2c_client *client,
 		ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
 		if (ret)
 			return ret;
-	} else {
-		ihid->pdata = *platform_data;
 	}
 
 	/* Parse platform agnostic common properties from ACPI / device tree */
 	i2c_hid_fwnode_probe(client, &ihid->pdata);
 
+	if (!ihid->pdata.power_up_device)
+		ihid->pdata.power_up_device = i2c_hid_power_up_device;
+	if (!ihid->pdata.power_down_device)
+		ihid->pdata.power_down_device = i2c_hid_power_down_device;
+
 	ihid->pdata.supplies[0].supply = "vdd";
 	ihid->pdata.supplies[1].supply = "vddl";
 
@@ -1059,14 +1091,10 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
-				    ihid->pdata.supplies);
-	if (ret < 0)
+	ret = ihid->pdata.power_up_device(&ihid->pdata);
+	if (ret)
 		return ret;
 
-	if (ihid->pdata.post_power_delay_ms)
-		msleep(ihid->pdata.post_power_delay_ms);
-
 	i2c_set_clientdata(client, ihid);
 
 	ihid->client = client;
@@ -1144,13 +1172,13 @@ static int i2c_hid_probe(struct i2c_client *client,
 	free_irq(client->irq, ihid);
 
 err_regulator:
-	regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
-			       ihid->pdata.supplies);
+	ihid->pdata.power_down_device(&ihid->pdata);
 	i2c_hid_free_buffers(ihid);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_probe);
 
-static int i2c_hid_remove(struct i2c_client *client)
+int i2c_hid_remove(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid;
@@ -1163,22 +1191,23 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->bufsize)
 		i2c_hid_free_buffers(ihid);
 
-	regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
-			       ihid->pdata.supplies);
+	ihid->pdata.power_down_device(&ihid->pdata);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_remove);
 
-static void i2c_hid_shutdown(struct i2c_client *client)
+void i2c_hid_shutdown(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
 	free_irq(client->irq, ihid);
 }
+EXPORT_SYMBOL_GPL(i2c_hid_shutdown);
 
 #ifdef CONFIG_PM_SLEEP
-static int i2c_hid_suspend(struct device *dev)
+int i2c_hid_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -1205,14 +1234,14 @@ static int i2c_hid_suspend(struct device *dev)
 			hid_warn(hid, "Failed to enable irq wake: %d\n",
 				wake_status);
 	} else {
-		regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
-				       ihid->pdata.supplies);
+		ihid->pdata.power_down_device(&ihid->pdata);
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_suspend);
 
-static int i2c_hid_resume(struct device *dev)
+int i2c_hid_resume(struct device *dev)
 {
 	int ret;
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1221,13 +1250,7 @@ static int i2c_hid_resume(struct device *dev)
 	int wake_status;
 
 	if (!device_may_wakeup(&client->dev)) {
-		ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
-					    ihid->pdata.supplies);
-		if (ret)
-			hid_warn(hid, "Failed to enable supplies: %d\n", ret);
-
-		if (ihid->pdata.post_power_delay_ms)
-			msleep(ihid->pdata.post_power_delay_ms);
+		ihid->pdata.power_up_device(&ihid->pdata);
 	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
@@ -1262,6 +1285,7 @@ static int i2c_hid_resume(struct device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_resume);
 #endif
 
 static const struct dev_pm_ops i2c_hid_pm = {
diff --git a/include/linux/input/i2c-hid-core.h b/include/linux/input/i2c-hid-core.h
new file mode 100644
index 000000000000..da7b0475f6f4
--- /dev/null
+++ b/include/linux/input/i2c-hid-core.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef I2C_HID_CORE_H
+#define I2C_HID_CORE_H
+
+#include <linux/i2c.h>
+
+int i2c_hid_probe(struct i2c_client *client,
+		  const struct i2c_device_id *dev_id);
+int i2c_hid_remove(struct i2c_client *client);
+
+void i2c_hid_shutdown(struct i2c_client *client);
+
+#ifdef CONFIG_PM_SLEEP
+int i2c_hid_suspend(struct device *dev);
+int i2c_hid_resume(struct device *dev);
+#endif
+
+#endif
diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
index c628bb5e1061..db567463d43e 100644
--- a/include/linux/platform_data/i2c-hid.h
+++ b/include/linux/platform_data/i2c-hid.h
@@ -21,6 +21,11 @@
  * @supplies: regulators for powering on the device.
  * @post_power_delay_ms: delay after powering on before device is usable.
  *
+ * @power_up_device: do sequencing to power up the device; may use the above
+ *                   supplies / post_power_delay_ms or ignore.
+ * @power_down_device: do sequencing to power down the device.
+ * @power_data: opaque pointer that power_up and power_down can use.
+ *
  * Note that it is the responsibility of the platform driver (or the acpi 5.0
  * driver, or the flattened device tree) to setup the irq related to the gpio in
  * the struct i2c_board_info.
@@ -36,6 +41,10 @@ struct i2c_hid_platform_data {
 	u16 hid_descriptor_address;
 	struct regulator_bulk_data supplies[2];
 	int post_power_delay_ms;
+
+	int (*power_up_device)(struct i2c_hid_platform_data *pdata);
+	void (*power_down_device)(struct i2c_hid_platform_data *pdata);
+	void *power_data;
 };
 
 #endif /* __LINUX_I2C_HID_H */
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v3 3/3] Input: Introduce goodix-i2c-hid as a subclass of i2c-hid

[Douglas Anderson]

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 ability of i2c-hid to have subclasses for power
sequencing to support the first Goodix i2c-hid touchscreen: GT7375P

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Rework to use subclassing.

 drivers/input/touchscreen/Kconfig          |  12 +++
 drivers/input/touchscreen/Makefile         |   1 +
 drivers/input/touchscreen/goodix-i2c-hid.c | 115 +++++++++++++++++++++
 3 files changed, 128 insertions(+)
 create mode 100644 drivers/input/touchscreen/goodix-i2c-hid.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index f012fe746df0..64d481101917 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -403,6 +403,18 @@ config TOUCHSCREEN_GOODIX
 	  To compile this driver as a module, choose M here: the
 	  module will be called goodix.
 
+config TOUCHSCREEN_GOODIX_I2C_HID
+	tristate "Goodix I2C-HID touchscreen"
+	depends on I2C_HID
+	help
+	  Say Y here if you have a Goodix touchscreen that uses i2c-hid
+	  to communicate.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called goodix-i2c-hid.
+
 config TOUCHSCREEN_HIDEEP
 	tristate "HiDeep Touch IC"
 	depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 6233541e9173..32b1a768aa06 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
 obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
 obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
 obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_I2C_HID)	+= goodix-i2c-hid.o
 obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
 obj-$(CONFIG_TOUCHSCREEN_ILI210X)	+= ili210x.o
 obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC)	+= imx6ul_tsc.o
diff --git a/drivers/input/touchscreen/goodix-i2c-hid.c b/drivers/input/touchscreen/goodix-i2c-hid.c
new file mode 100644
index 000000000000..a2cc4f923d8a
--- /dev/null
+++ b/drivers/input/touchscreen/goodix-i2c-hid.c
@@ -0,0 +1,115 @@
+// 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/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input/i2c-hid-core.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/i2c-hid.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct goodix_i2c_hid_timing_data {
+	unsigned int post_gpio_reset_delay_ms;
+	unsigned int post_power_delay_ms;
+};
+
+static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
+	.post_power_delay_ms = 10,
+	.post_gpio_reset_delay_ms = 120,
+};
+
+struct goodix_i2c_hid_plat_data {
+	struct gpio_desc *reset_gpio;
+	const struct goodix_i2c_hid_timing_data *timings;
+
+	struct i2c_hid_platform_data ihid_pdata;
+};
+
+static int goodix_i2c_hid_power_up_device(struct i2c_hid_platform_data *ihid_pdata)
+{
+	struct goodix_i2c_hid_plat_data *pdata = ihid_pdata->power_data;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ihid_pdata->supplies),
+				    ihid_pdata->supplies);
+	if (ret)
+		return ret;
+
+	if (pdata->timings->post_power_delay_ms)
+		msleep(pdata->timings->post_power_delay_ms);
+
+	gpiod_set_value_cansleep(pdata->reset_gpio, 0);
+	if (pdata->timings->post_gpio_reset_delay_ms)
+		msleep(pdata->timings->post_gpio_reset_delay_ms);
+
+	return 0;
+}
+
+static void goodix_i2c_hid_power_down_device(struct i2c_hid_platform_data *ihid_pdata)
+{
+	struct goodix_i2c_hid_plat_data *pdata = ihid_pdata->power_data;
+
+	gpiod_set_value_cansleep(pdata->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(ihid_pdata->supplies),
+			       ihid_pdata->supplies);
+}
+
+static int goodix_i2c_hid_ts_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
+{
+	struct goodix_i2c_hid_plat_data *pdata;
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+
+	/* Start out with reset asserted */
+	pdata->reset_gpio =
+		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->reset_gpio))
+		return PTR_ERR(pdata->reset_gpio);
+
+	pdata->timings = device_get_match_data(&client->dev);
+
+	pdata->ihid_pdata.hid_descriptor_address = 0x0001;
+	pdata->ihid_pdata.power_data = pdata;
+	pdata->ihid_pdata.power_up_device = goodix_i2c_hid_power_up_device;
+	pdata->ihid_pdata.power_down_device = goodix_i2c_hid_power_down_device;
+
+	client->dev.platform_data = &pdata->ihid_pdata;
+
+	return i2c_hid_probe(client, id);
+}
+
+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 const struct dev_pm_ops goodix_i2c_hid_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_suspend, i2c_hid_resume)
+};
+
+static struct i2c_driver goodix_i2c_hid_ts_driver = {
+	.driver = {
+		.name	= "goodix-i2c-hid",
+		.pm	= &goodix_i2c_hid_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
+	},
+	.probe		= goodix_i2c_hid_ts_probe,
+	.remove		= i2c_hid_remove,
+	.shutdown	= i2c_hid_shutdown,
+};
+module_i2c_driver(goodix_i2c_hid_ts_driver);
+
+MODULE_AUTHOR("Douglas Anderson <dianders@chromium.org>");
+MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v3 2/3] HID: i2c-hid: Allow subclasses of i2c-hid for power sequencing

[Rob Herring]

On Mon, Nov 2, 2020 at 6:13 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> This exports some things from i2c-hid so that we can have a driver
> that's effectively a subclass of it and that can do its own power
> sequencing.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v3:
> - Rework to use subclassing.
>
> Changes in v2:
> - Use a separate compatible string for this new touchscreen.
> - Get timings based on the compatible string.
>
>  drivers/hid/i2c-hid/i2c-hid-core.c    | 78 +++++++++++++++++----------
>  include/linux/input/i2c-hid-core.h    | 19 +++++++
>  include/linux/platform_data/i2c-hid.h |  9 ++++
>  3 files changed, 79 insertions(+), 27 deletions(-)
>  create mode 100644 include/linux/input/i2c-hid-core.h
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 786e3e9af1c9..910e9089fcf8 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -22,6 +22,7 @@
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/input.h>
> +#include <linux/input/i2c-hid-core.h>
>  #include <linux/irq.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> @@ -1007,8 +1008,33 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
>                 pdata->post_power_delay_ms = val;
>  }
>
> -static int i2c_hid_probe(struct i2c_client *client,
> -                        const struct i2c_device_id *dev_id)
> +static int i2c_hid_power_up_device(struct i2c_hid_platform_data *pdata)
> +{
> +       struct i2c_hid *ihid = container_of(pdata, struct i2c_hid, pdata);
> +       struct hid_device *hid = ihid->hid;
> +       int ret;
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
> +                                   pdata->supplies);
> +       if (ret) {
> +               if (hid)
> +                       hid_warn(hid, "Failed to enable supplies: %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (pdata->post_power_delay_ms)
> +               msleep(pdata->post_power_delay_ms);
> +
> +       return 0;
> +}
> +
> +static void i2c_hid_power_down_device(struct i2c_hid_platform_data *pdata)
> +{
> +       regulator_bulk_disable(ARRAY_SIZE(pdata->supplies), pdata->supplies);
> +}
> +
> +int i2c_hid_probe(struct i2c_client *client,
> +                 const struct i2c_device_id *dev_id)
>  {
>         int ret;
>         struct i2c_hid *ihid;
> @@ -1035,6 +1061,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>         if (!ihid)
>                 return -ENOMEM;
>
> +       if (platform_data)
> +               ihid->pdata = *platform_data;
> +
>         if (client->dev.of_node) {
>                 ret = i2c_hid_of_probe(client, &ihid->pdata);
>                 if (ret)
> @@ -1043,13 +1072,16 @@ static int i2c_hid_probe(struct i2c_client *client,
>                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
>                 if (ret)
>                         return ret;
> -       } else {
> -               ihid->pdata = *platform_data;
>         }
>
>         /* Parse platform agnostic common properties from ACPI / device tree */
>         i2c_hid_fwnode_probe(client, &ihid->pdata);
>
> +       if (!ihid->pdata.power_up_device)
> +               ihid->pdata.power_up_device = i2c_hid_power_up_device;
> +       if (!ihid->pdata.power_down_device)
> +               ihid->pdata.power_down_device = i2c_hid_power_down_device;
> +
>         ihid->pdata.supplies[0].supply = "vdd";
>         ihid->pdata.supplies[1].supply = "vddl";
>
> @@ -1059,14 +1091,10 @@ static int i2c_hid_probe(struct i2c_client *client,
>         if (ret)
>                 return ret;
>
> -       ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
> -                                   ihid->pdata.supplies);
> -       if (ret < 0)
> +       ret = ihid->pdata.power_up_device(&ihid->pdata);
> +       if (ret)

This is an odd driver structure IMO. I guess platform data is already
there, but that's not what we'd use for any new driver.

Why not export i2c_hid_probe, i2c_hid_remove, etc. and then just call
them from the goodix driver and possibly make it handle all DT
platforms?

Who else needs regulators besides DT platforms? I thought with ACPI
it's all wonderfully abstracted away?

Rob

Re: [PATCH v3 2/3] HID: i2c-hid: Allow subclasses of i2c-hid for power sequencing

[Hans de Goede]

Hi,

On 11/3/20 2:46 AM, Rob Herring wrote:
> On Mon, Nov 2, 2020 at 6:13 PM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> This exports some things from i2c-hid so that we can have a driver
>> that's effectively a subclass of it and that can do its own power
>> sequencing.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Rework to use subclassing.
>>
>> Changes in v2:
>> - Use a separate compatible string for this new touchscreen.
>> - Get timings based on the compatible string.
>>
>>  drivers/hid/i2c-hid/i2c-hid-core.c    | 78 +++++++++++++++++----------
>>  include/linux/input/i2c-hid-core.h    | 19 +++++++
>>  include/linux/platform_data/i2c-hid.h |  9 ++++
>>  3 files changed, 79 insertions(+), 27 deletions(-)
>>  create mode 100644 include/linux/input/i2c-hid-core.h
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index 786e3e9af1c9..910e9089fcf8 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/i2c.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/input.h>
>> +#include <linux/input/i2c-hid-core.h>
>>  #include <linux/irq.h>
>>  #include <linux/delay.h>
>>  #include <linux/slab.h>
>> @@ -1007,8 +1008,33 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
>>                 pdata->post_power_delay_ms = val;
>>  }
>>
>> -static int i2c_hid_probe(struct i2c_client *client,
>> -                        const struct i2c_device_id *dev_id)
>> +static int i2c_hid_power_up_device(struct i2c_hid_platform_data *pdata)
>> +{
>> +       struct i2c_hid *ihid = container_of(pdata, struct i2c_hid, pdata);
>> +       struct hid_device *hid = ihid->hid;
>> +       int ret;
>> +
>> +       ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
>> +                                   pdata->supplies);
>> +       if (ret) {
>> +               if (hid)
>> +                       hid_warn(hid, "Failed to enable supplies: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       if (pdata->post_power_delay_ms)
>> +               msleep(pdata->post_power_delay_ms);
>> +
>> +       return 0;
>> +}
>> +
>> +static void i2c_hid_power_down_device(struct i2c_hid_platform_data *pdata)
>> +{
>> +       regulator_bulk_disable(ARRAY_SIZE(pdata->supplies), pdata->supplies);
>> +}
>> +
>> +int i2c_hid_probe(struct i2c_client *client,
>> +                 const struct i2c_device_id *dev_id)
>>  {
>>         int ret;
>>         struct i2c_hid *ihid;
>> @@ -1035,6 +1061,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>>         if (!ihid)
>>                 return -ENOMEM;
>>
>> +       if (platform_data)
>> +               ihid->pdata = *platform_data;
>> +
>>         if (client->dev.of_node) {
>>                 ret = i2c_hid_of_probe(client, &ihid->pdata);
>>                 if (ret)
>> @@ -1043,13 +1072,16 @@ static int i2c_hid_probe(struct i2c_client *client,
>>                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
>>                 if (ret)
>>                         return ret;
>> -       } else {
>> -               ihid->pdata = *platform_data;
>>         }
>>
>>         /* Parse platform agnostic common properties from ACPI / device tree */
>>         i2c_hid_fwnode_probe(client, &ihid->pdata);
>>
>> +       if (!ihid->pdata.power_up_device)
>> +               ihid->pdata.power_up_device = i2c_hid_power_up_device;
>> +       if (!ihid->pdata.power_down_device)
>> +               ihid->pdata.power_down_device = i2c_hid_power_down_device;
>> +
>>         ihid->pdata.supplies[0].supply = "vdd";
>>         ihid->pdata.supplies[1].supply = "vddl";
>>
>> @@ -1059,14 +1091,10 @@ static int i2c_hid_probe(struct i2c_client *client,
>>         if (ret)
>>                 return ret;
>>
>> -       ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
>> -                                   ihid->pdata.supplies);
>> -       if (ret < 0)
>> +       ret = ihid->pdata.power_up_device(&ihid->pdata);
>> +       if (ret)
> 
> This is an odd driver structure IMO. I guess platform data is already
> there, but that's not what we'd use for any new driver.
> 
> Why not export i2c_hid_probe, i2c_hid_remove, etc. and then just call
> them from the goodix driver and possibly make it handle all DT
> platforms?
> 
> Who else needs regulators besides DT platforms? I thought with ACPI
> it's all wonderfully abstracted away?

Right with ACPI we do not need the regulators, actually not checking
for them with ACPI would be preferable, if only to suppress kernel
messages like these:

[    3.515658] i2c_hid i2c-SYNA8007:00: supply vdd not found, using dummy regulator
[    3.515848] i2c_hid i2c-SYNA8007:00: supply vddl not found, using dummy regulator

To be fair the i2c-hid-core.c code does have some acpi specific handling too.

With the latest fixes from:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-5.10/upstream-fixes
taken into account we have the following acpi specific functions being called
from various places:

i2c_hid_acpi_fix_up_power   (called on probe)
i2c_hid_acpi_enable_wakeup  (called on probe)
i2c_hid_acpi_shutdown       (called on shutdown)

Not I'm not Benjamin / not the MAINTAINER of this code, but I think that
splitting out both the ACPI *and* the of/dt handling might make sense.

Maybe even turn drivers/hid/i2c-hid/i2c-hid-core.c into a library
and have 2 separate:

drivers/hid/i2c-hid/i2c-hid-acpi.c
drivers/hid/i2c-hid/i2c-hid-of.c

drivers using that library.

That would change the kernel-module name, but there only is the debug
module parameter which is affected by that from a userspace API point
of break, so I think that changing the kernel-module name is fine.

So you would have 2 i2c drivers, one with an acpi_match_table and one
with an of_match_table. And then either also have 2 separate probe
functions, or have a probe helper which gets passed some platform_data
given by the acpi/of probe function + some extra callbacks (either
as extra arguments or inside the pdata).

Having a separate drivers/hid/i2c-hid/i2c-hid-of.c file also allows
for a separate MAINTAINER entry where someone else then Benjamin
becomes responsible for reviewing DT related changes...

Anyways just my 2 cents, it is probably wise to wait what Benjamin
has to say before sinking time in implementing my suggestion :)

Regards,

Hans

Re: [PATCH v3 2/3] HID: i2c-hid: Allow subclasses of i2c-hid for power sequencing

[Benjamin Tissoires]

Hi,

On Tue, Nov 3, 2020 at 10:09 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/3/20 2:46 AM, Rob Herring wrote:
> > On Mon, Nov 2, 2020 at 6:13 PM Douglas Anderson <dianders@chromium.org> wrote:
> >>
> >> This exports some things from i2c-hid so that we can have a driver
> >> that's effectively a subclass of it and that can do its own power
> >> sequencing.
> >>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >>
> >> Changes in v3:
> >> - Rework to use subclassing.
> >>
> >> Changes in v2:
> >> - Use a separate compatible string for this new touchscreen.
> >> - Get timings based on the compatible string.
> >>
> >>  drivers/hid/i2c-hid/i2c-hid-core.c    | 78 +++++++++++++++++----------
> >>  include/linux/input/i2c-hid-core.h    | 19 +++++++
> >>  include/linux/platform_data/i2c-hid.h |  9 ++++
> >>  3 files changed, 79 insertions(+), 27 deletions(-)
> >>  create mode 100644 include/linux/input/i2c-hid-core.h
> >>
> >> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> >> index 786e3e9af1c9..910e9089fcf8 100644
> >> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> >> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> >> @@ -22,6 +22,7 @@
> >>  #include <linux/i2c.h>
> >>  #include <linux/interrupt.h>
> >>  #include <linux/input.h>
> >> +#include <linux/input/i2c-hid-core.h>
> >>  #include <linux/irq.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/slab.h>
> >> @@ -1007,8 +1008,33 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
> >>                 pdata->post_power_delay_ms = val;
> >>  }
> >>
> >> -static int i2c_hid_probe(struct i2c_client *client,
> >> -                        const struct i2c_device_id *dev_id)
> >> +static int i2c_hid_power_up_device(struct i2c_hid_platform_data *pdata)
> >> +{
> >> +       struct i2c_hid *ihid = container_of(pdata, struct i2c_hid, pdata);
> >> +       struct hid_device *hid = ihid->hid;
> >> +       int ret;
> >> +
> >> +       ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
> >> +                                   pdata->supplies);
> >> +       if (ret) {
> >> +               if (hid)
> >> +                       hid_warn(hid, "Failed to enable supplies: %d\n", ret);
> >> +               return ret;
> >> +       }
> >> +
> >> +       if (pdata->post_power_delay_ms)
> >> +               msleep(pdata->post_power_delay_ms);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static void i2c_hid_power_down_device(struct i2c_hid_platform_data *pdata)
> >> +{
> >> +       regulator_bulk_disable(ARRAY_SIZE(pdata->supplies), pdata->supplies);
> >> +}
> >> +
> >> +int i2c_hid_probe(struct i2c_client *client,
> >> +                 const struct i2c_device_id *dev_id)
> >>  {
> >>         int ret;
> >>         struct i2c_hid *ihid;
> >> @@ -1035,6 +1061,9 @@ static int i2c_hid_probe(struct i2c_client *client,
> >>         if (!ihid)
> >>                 return -ENOMEM;
> >>
> >> +       if (platform_data)
> >> +               ihid->pdata = *platform_data;
> >> +
> >>         if (client->dev.of_node) {
> >>                 ret = i2c_hid_of_probe(client, &ihid->pdata);
> >>                 if (ret)
> >> @@ -1043,13 +1072,16 @@ static int i2c_hid_probe(struct i2c_client *client,
> >>                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
> >>                 if (ret)
> >>                         return ret;
> >> -       } else {
> >> -               ihid->pdata = *platform_data;
> >>         }
> >>
> >>         /* Parse platform agnostic common properties from ACPI / device tree */
> >>         i2c_hid_fwnode_probe(client, &ihid->pdata);
> >>
> >> +       if (!ihid->pdata.power_up_device)
> >> +               ihid->pdata.power_up_device = i2c_hid_power_up_device;
> >> +       if (!ihid->pdata.power_down_device)
> >> +               ihid->pdata.power_down_device = i2c_hid_power_down_device;
> >> +
> >>         ihid->pdata.supplies[0].supply = "vdd";
> >>         ihid->pdata.supplies[1].supply = "vddl";
> >>
> >> @@ -1059,14 +1091,10 @@ static int i2c_hid_probe(struct i2c_client *client,
> >>         if (ret)
> >>                 return ret;
> >>
> >> -       ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
> >> -                                   ihid->pdata.supplies);
> >> -       if (ret < 0)
> >> +       ret = ihid->pdata.power_up_device(&ihid->pdata);
> >> +       if (ret)
> >
> > This is an odd driver structure IMO. I guess platform data is already
> > there, but that's not what we'd use for any new driver.
> >
> > Why not export i2c_hid_probe, i2c_hid_remove, etc. and then just call
> > them from the goodix driver and possibly make it handle all DT
> > platforms?
> >
> > Who else needs regulators besides DT platforms? I thought with ACPI
> > it's all wonderfully abstracted away?
>
> Right with ACPI we do not need the regulators, actually not checking
> for them with ACPI would be preferable, if only to suppress kernel
> messages like these:
>
> [    3.515658] i2c_hid i2c-SYNA8007:00: supply vdd not found, using dummy regulator
> [    3.515848] i2c_hid i2c-SYNA8007:00: supply vddl not found, using dummy regulator
>
> To be fair the i2c-hid-core.c code does have some acpi specific handling too.
>
> With the latest fixes from:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-5.10/upstream-fixes
> taken into account we have the following acpi specific functions being called
> from various places:
>
> i2c_hid_acpi_fix_up_power   (called on probe)
> i2c_hid_acpi_enable_wakeup  (called on probe)
> i2c_hid_acpi_shutdown       (called on shutdown)
>
> Not I'm not Benjamin / not the MAINTAINER of this code, but I think that
> splitting out both the ACPI *and* the of/dt handling might make sense.

Yep, fully agree.

>
> Maybe even turn drivers/hid/i2c-hid/i2c-hid-core.c into a library
> and have 2 separate:
>
> drivers/hid/i2c-hid/i2c-hid-acpi.c
> drivers/hid/i2c-hid/i2c-hid-of.c
>
> drivers using that library.
>
> That would change the kernel-module name, but there only is the debug
> module parameter which is affected by that from a userspace API point
> of break, so I think that changing the kernel-module name is fine.

Ack, this is a small downside compared to a better extensibility of the driver.

Also, we could then delete entirely the platform_data as the register
of the hid_descriptor_address could simply be added as an argument to
i2c_hid_probe. Though that would force me to rewrite my testing patch
with custom i2c-hid devices over an USB<->I2C adapter.

>
> So you would have 2 i2c drivers, one with an acpi_match_table and one
> with an of_match_table. And then either also have 2 separate probe
> functions, or have a probe helper which gets passed some platform_data
> given by the acpi/of probe function + some extra callbacks (either
> as extra arguments or inside the pdata).
>
> Having a separate drivers/hid/i2c-hid/i2c-hid-of.c file also allows
> for a separate MAINTAINER entry where someone else then Benjamin
> becomes responsible for reviewing DT related changes...

I like that :)

>
> Anyways just my 2 cents, it is probably wise to wait what Benjamin
> has to say before sinking time in implementing my suggestion :)

I also want to say that I like the general idea of Doug's patch.
Having a separate driver that handles the specific use case of goodix
is really nice, as it allows to just load this driver without touching
the core of i2c-hid. I believe this is in line with what Google tries
to do with their kernel that OEMs can not touch, but only add overlays
to it. The implementation is not polished (I don't think this new
driver belongs to the input subsystem), but I like the general idea of
having the "subclassing". Maybe we can make it prettier with Hans'
suggestion, given that this mainly means we are transforming
i2c-hid-core.c into a library.

As for where this new goodix driver goes, it can stay in
drivers/hid/i2c-hid IMO.

Cheers,
Benjamin

Re: [PATCH v3 2/3] HID: i2c-hid: Allow subclasses of i2c-hid for power sequencing

[Dmitry Torokhov]

On Tue, Nov 03, 2020 at 01:42:47PM +0100, Benjamin Tissoires wrote:
> 
> I also want to say that I like the general idea of Doug's patch.
> Having a separate driver that handles the specific use case of goodix
> is really nice, as it allows to just load this driver without touching
> the core of i2c-hid. I believe this is in line with what Google tries
> to do with their kernel that OEMs can not touch, but only add overlays
> to it. The implementation is not polished (I don't think this new
> driver belongs to the input subsystem), but I like the general idea of
> having the "subclassing". Maybe we can make it prettier with Hans'
> suggestion, given that this mainly means we are transforming
> i2c-hid-core.c into a library.
> 
> As for where this new goodix driver goes, it can stay in
> drivers/hid/i2c-hid IMO.

Yep, I agree, it has nothing to do with input (except the device being
physically a touchscreen ;) ), so driver/hid/i2c-hid makes most sense to
me too.

Thanks.

-- 
Dmitry

Re: [PATCH v3 3/3] Input: Introduce goodix-i2c-hid as a subclass of i2c-hid

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]

Hi Douglas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on input/next]
[also build test WARNING on jikos-trivial/for-next linus/master v5.10-rc3 next-20201110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Introduce-bindings-for-the-Goodix-GT7375P/20201103-081548
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-a016-20201110 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4d81c8adb6ed9840257f6cb6b93f60856d422a15)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/528ad0eceb01a39e733c337c44500abd49b7bc2a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Douglas-Anderson/dt-bindings-HID-i2c-hid-Introduce-bindings-for-the-Goodix-GT7375P/20201103-081548
        git checkout 528ad0eceb01a39e733c337c44500abd49b7bc2a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/input/touchscreen/goodix-i2c-hid.c:90:34: warning: unused variable 'goodix_i2c_hid_of_match' [-Wunused-const-variable]
   static const struct of_device_id goodix_i2c_hid_of_match[] = {
                                    ^
   1 warning generated.

vim +/goodix_i2c_hid_of_match +90 drivers/input/touchscreen/goodix-i2c-hid.c

    89	
  > 90	static const struct of_device_id goodix_i2c_hid_of_match[] = {
    91		{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
    92		{ }
    93	};
    94	MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
    95	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30949 bytes --]