[PATCH v4 0/4] HID: i2c-hid: Reorganize to allow supporting goodix,gt7375p

[PATCH v4 0/4] HID: i2c-hid: Reorganize to allow supporting goodix,gt7375p

[Douglas Anderson]

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 pretended to have a Goodix i2c-hid device on
  it.  I don't have a device with GT7375P setup yet and the person who
  has been testing remotely for me hasn't tested this exact series.  I
  think it should still work, though.
- NOT tested on any ACPI devices (just compile tested).

There are probably still small nits, but hopefully we're getting
closer to something people like.

Changes in v4:
- Fully rejigger so ACPI and OF are full subclasses.
- ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4.
- Totally redid based on the new subclass system.

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

Changes in v2:
- Use a separate compatible string for this new touchscreen.
- Get timings based on the compatible string.
- ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2.

Douglas Anderson (4):
  HID: i2c-hid: Reorganize so ACPI and OF are subclasses
  arm64: defconfig: Update config names for i2c-hid rejigger
  dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P
  HID: i2c-hid: Introduce goodix-i2c-hid as a subclass of i2c-hid

 .../bindings/input/goodix,gt7375p.yaml        |  63 +++++
 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            | 167 ++++++++++++
 drivers/hid/i2c-hid/i2c-hid-core.c            | 253 +++---------------
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c       | 120 +++++++++
 drivers/hid/i2c-hid/i2c-hid-of.c              | 149 +++++++++++
 drivers/hid/i2c-hid/i2c-hid.h                 |  24 ++
 include/linux/platform_data/i2c-hid.h         |  41 ---
 11 files changed, 607 insertions(+), 268 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.1.341.ge80a0c044ae-goog

[PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Douglas Anderson]

This patch rejiggers the i2c-hid code so that the OF (Device Tree) and
ACPI support is separated out a bit.  The OF and ACPI drivers are now
effectively "subclasses" of the generic code.

Essentially, what we're doing here:
* Make "power up" and "power down" a virtual function that can be
  (optionally) implemented by subclasses.
* Each subclass is a drive on its own, so it implements probe / remove
  / suspend / resume / shutdown.  The core code provides
  implementations that the subclasses can call into, or fully use for
  their implementation if they don't have special needs.

We'll organize this so that we now have 3 modules: the old i2c-hid
module becomes the "core" module and two new modules will depend on
it, handling probing the specific device.

As part of this work, we'll remove the i2c-hid "platform data"
concept since it's not needed.

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

Changes in v4:
- Fully rejigger so ACPI and OF are full subclasses.

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/Makefile                  |   2 +-
 drivers/hid/i2c-hid/Kconfig           |  32 +++-
 drivers/hid/i2c-hid/Makefile          |   5 +-
 drivers/hid/i2c-hid/i2c-hid-acpi.c    | 167 +++++++++++++++++
 drivers/hid/i2c-hid/i2c-hid-core.c    | 253 ++++----------------------
 drivers/hid/i2c-hid/i2c-hid-of.c      | 149 +++++++++++++++
 drivers/hid/i2c-hid/i2c-hid.h         |  24 +++
 include/linux/platform_data/i2c-hid.h |  41 -----
 8 files changed, 406 insertions(+), 267 deletions(-)
 create mode 100644 drivers/hid/i2c-hid/i2c-hid-acpi.c
 create mode 100644 drivers/hid/i2c-hid/i2c-hid-of.c
 delete mode 100644 include/linux/platform_data/i2c-hid.h

diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 4acb583c92a6..8d0c15a0d652 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -138,7 +138,7 @@ obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
 obj-$(CONFIG_USB_KBD)		+= usbhid/
 
-obj-$(CONFIG_I2C_HID)		+= i2c-hid/
+obj-$(CONFIG_I2C_HID_CORE)	+= i2c-hid/
 
 obj-$(CONFIG_INTEL_ISH_HID)	+= intel-ish-hid/
 obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER)	+= intel-ish-hid/
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index c4e5dfeab2bd..819b7521c182 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -2,18 +2,40 @@
 menu "I2C HID support"
 	depends on I2C
 
-config I2C_HID
-	tristate "HID over I2C transport layer"
+config I2C_HID_ACPI
+	tristate "HID over I2C transport layer ACPI driver"
 	default n
-	depends on I2C && INPUT
-	select HID
+	depends on I2C && INPUT && ACPI
+	help
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which is connected to your computer via I2C.
+	  This driver supports ACPI-based systems.
+
+	  If unsure, say N.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i2c-hid-acpi.  It will also build/depend on the
+	  module i2c-hid.
+
+config I2C_HID_OF
+	tristate "HID over I2C transport layer Open Firmware driver"
+	default n
+	depends on I2C && INPUT && OF
 	help
 	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
 	  other HID based devices which is connected to your computer via I2C.
+	  This driver supports 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.
+	  will be called i2c-hid-of.  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
+	select HID
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 681b3896898e..9b4a73446841 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -3,7 +3,10 @@
 # Makefile for the I2C input drivers
 #
 
-obj-$(CONFIG_I2C_HID)				+= i2c-hid.o
+obj-$(CONFIG_I2C_HID_CORE)			+= i2c-hid.o
 
 i2c-hid-objs					=  i2c-hid-core.o
 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
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
new file mode 100644
index 000000000000..d4c29dc62297
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -0,0 +1,167 @@
+/*
+ * HID over I2C ACPI Subclass
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * This code was forked out of the core code, which was partly based on
+ * "USB HID support for Linux":
+ *
+ *  Copyright (c) 1999 Andreas Gal
+ *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ *  Copyright (c) 2007-2008 Oliver Neukum
+ *  Copyright (c) 2006-2010 Jiri Kosina
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+
+#include "i2c-hid.h"
+
+struct i2c_hid_acpi {
+	struct i2chid_subclass_data subclass;
+	struct i2c_client *client;
+	bool power_fixed;
+};
+
+static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
+	/*
+	 * The CHPN0001 ACPI device, which is used to describe the Chipone
+	 * ICN8505 controller, has a _CID of PNP0C50 but is not HID compatible.
+	 */
+	{"CHPN0001", 0 },
+	{ },
+};
+
+static int i2c_hid_acpi_get_descriptor(struct i2c_client *client)
+{
+	static guid_t i2c_hid_guid =
+		GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
+			  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
+	union acpi_object *obj;
+	struct acpi_device *adev;
+	acpi_handle handle;
+	u16 hid_descriptor_address;
+
+	handle = ACPI_HANDLE(&client->dev);
+	if (!handle || acpi_bus_get_device(handle, &adev)) {
+		dev_err(&client->dev, "Error could not get ACPI device\n");
+		return -ENODEV;
+	}
+
+	if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
+		return -ENODEV;
+
+	obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
+				      ACPI_TYPE_INTEGER);
+	if (!obj) {
+		dev_err(&client->dev, "Error _DSM call to get HID descriptor address failed\n");
+		return -ENODEV;
+	}
+
+	hid_descriptor_address = obj->integer.value;
+	ACPI_FREE(obj);
+
+	return hid_descriptor_address;
+}
+
+static int i2c_hid_acpi_power_up_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_acpi *ihid_of =
+		container_of(subclass, struct i2c_hid_acpi, subclass);
+	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;
+}
+
+int i2c_hid_acpi_probe(struct i2c_client *client,
+		       const struct i2c_device_id *dev_id)
+{
+	struct device *dev = &client->dev;
+	struct i2c_hid_acpi *ihid_acpi;
+	u16 hid_descriptor_address;
+	int ret;
+
+	ihid_acpi = devm_kzalloc(&client->dev, sizeof(*ihid_acpi), GFP_KERNEL);
+	if (!ihid_acpi)
+		return -ENOMEM;
+
+	ihid_acpi->subclass.power_up_device = i2c_hid_acpi_power_up_device;
+
+	ret = i2c_hid_acpi_get_descriptor(client);
+	if (ret < 0)
+		return ret;
+	hid_descriptor_address = ret;
+
+	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
+		device_set_wakeup_capable(dev, true);
+		device_set_wakeup_enable(dev, false);
+	}
+
+	return i2c_hid_core_probe(client, &ihid_acpi->subclass,
+				  hid_descriptor_address);
+}
+
+static void i2c_hid_acpi_shutdown(struct i2c_client *client)
+{
+	i2c_hid_core_shutdown(client);
+	acpi_device_set_power(ACPI_COMPANION(&client->dev), ACPI_STATE_D3_COLD);
+}
+
+static const struct dev_pm_ops i2c_hid_acpi_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+};
+
+static const struct acpi_device_id i2c_hid_acpi_match[] = {
+	{"ACPI0C50", 0 },
+	{"PNP0C50", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
+
+static const struct i2c_device_id i2c_hid_acpi_id_table[] = {
+	{ "hid", 0 },
+	{ "hid-over-i2c", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_id_table);
+
+static struct i2c_driver i2c_hid_acpi_driver = {
+	.driver = {
+		.name	= "i2c_hid_acpi",
+		.pm	= &i2c_hid_acpi_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
+	},
+
+	.probe		= i2c_hid_acpi_probe,
+	.remove		= i2c_hid_core_remove,
+	.shutdown	= i2c_hid_acpi_shutdown,
+	.id_table	= i2c_hid_acpi_id_table,
+};
+
+module_i2c_driver(i2c_hid_acpi_driver);
+
+MODULE_DESCRIPTION("HID over I2C ACPI driver");
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index aeff1ffb0c8b..7e7303c2a45e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -35,12 +35,8 @@
 #include <linux/kernel.h>
 #include <linux/hid.h>
 #include <linux/mutex.h>
-#include <linux/acpi.h>
-#include <linux/of.h>
 #include <linux/regulator/consumer.h>
 
-#include <linux/platform_data/i2c-hid.h>
-
 #include "../hid-ids.h"
 #include "i2c-hid.h"
 
@@ -156,10 +152,10 @@ struct i2c_hid {
 
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 
-	struct i2c_hid_platform_data pdata;
-
 	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
+
+	struct i2chid_subclass_data *subclass;
 };
 
 static const struct i2c_hid_quirks {
@@ -884,144 +880,29 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	return 0;
 }
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
-	/*
-	 * The CHPN0001 ACPI device, which is used to describe the Chipone
-	 * ICN8505 controller, has a _CID of PNP0C50 but is not HID compatible.
-	 */
-	{"CHPN0001", 0 },
-	{ },
-};
-
-static int i2c_hid_acpi_pdata(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
-{
-	static guid_t i2c_hid_guid =
-		GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
-			  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
-	union acpi_object *obj;
-	struct acpi_device *adev;
-	acpi_handle handle;
-
-	handle = ACPI_HANDLE(&client->dev);
-	if (!handle || acpi_bus_get_device(handle, &adev)) {
-		dev_err(&client->dev, "Error could not get ACPI device\n");
-		return -ENODEV;
-	}
-
-	if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
-		return -ENODEV;
-
-	obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
-				      ACPI_TYPE_INTEGER);
-	if (!obj) {
-		dev_err(&client->dev, "Error _DSM call to get HID descriptor address failed\n");
-		return -ENODEV;
-	}
-
-	pdata->hid_descriptor_address = obj->integer.value;
-	ACPI_FREE(obj);
-
-	return 0;
-}
-
-static void i2c_hid_acpi_fix_up_power(struct device *dev)
-{
-	struct acpi_device *adev;
-
-	adev = ACPI_COMPANION(dev);
-	if (adev)
-		acpi_device_fix_up_power(adev);
-}
-
-static void i2c_hid_acpi_enable_wakeup(struct device *dev)
-{
-	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
-		device_set_wakeup_capable(dev, true);
-		device_set_wakeup_enable(dev, false);
-	}
-}
-
-static void i2c_hid_acpi_shutdown(struct device *dev)
-{
-	acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D3_COLD);
-}
-
-static const struct acpi_device_id i2c_hid_acpi_match[] = {
-	{"ACPI0C50", 0 },
-	{"PNP0C50", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
-#else
-static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
-{
-	return -ENODEV;
-}
-
-static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
-
-static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {}
-
-static inline void i2c_hid_acpi_shutdown(struct device *dev) {}
-#endif
-
-#ifdef CONFIG_OF
-static int i2c_hid_of_probe(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
+static int i2c_hid_core_power_up(struct i2c_hid *ihid)
 {
-	struct device *dev = &client->dev;
-	u32 val;
-	int ret;
-
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
-	if (ret) {
-		dev_err(&client->dev, "HID register address not provided\n");
-		return -ENODEV;
-	}
-	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-			val);
-		return -EINVAL;
-	}
-	pdata->hid_descriptor_address = val;
-
-	return 0;
-}
+	if (!ihid->subclass->power_up_device)
+		return 0;
 
-static const struct of_device_id i2c_hid_of_match[] = {
-	{ .compatible = "hid-over-i2c" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
-#else
-static inline int i2c_hid_of_probe(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
-{
-	return -ENODEV;
+	return ihid->subclass->power_up_device(ihid->subclass);
 }
-#endif
 
-static void i2c_hid_fwnode_probe(struct i2c_client *client,
-				 struct i2c_hid_platform_data *pdata)
+static void i2c_hid_core_power_down(struct i2c_hid *ihid)
 {
-	u32 val;
+	if (!ihid->subclass->power_down_device)
+		return;
 
-	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
-				      &val))
-		pdata->post_power_delay_ms = val;
+	ihid->subclass->power_down_device(ihid->subclass);
 }
 
-static int i2c_hid_probe(struct i2c_client *client,
-			 const struct i2c_device_id *dev_id)
+int i2c_hid_core_probe(struct i2c_client *client,
+		       struct i2chid_subclass_data *subclass,
+		       u16 hid_descriptor_address)
 {
 	int ret;
 	struct i2c_hid *ihid;
 	struct hid_device *hid;
-	__u16 hidRegister;
-	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
 
 	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
 
@@ -1042,44 +923,17 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
-	if (client->dev.of_node) {
-		ret = i2c_hid_of_probe(client, &ihid->pdata);
-		if (ret)
-			return ret;
-	} else if (!platform_data) {
-		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);
+	ihid->subclass = subclass;
 
-	ihid->pdata.supplies[0].supply = "vdd";
-	ihid->pdata.supplies[1].supply = "vddl";
-
-	ret = devm_regulator_bulk_get(&client->dev,
-				      ARRAY_SIZE(ihid->pdata.supplies),
-				      ihid->pdata.supplies);
+	ret = i2c_hid_core_power_up(ihid);
 	if (ret)
 		return ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
-				    ihid->pdata.supplies);
-	if (ret < 0)
-		return ret;
-
-	if (ihid->pdata.post_power_delay_ms)
-		msleep(ihid->pdata.post_power_delay_ms);
-
 	i2c_set_clientdata(client, ihid);
 
 	ihid->client = client;
 
-	hidRegister = ihid->pdata.hid_descriptor_address;
-	ihid->wHIDDescRegister = cpu_to_le16(hidRegister);
+	ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address);
 
 	init_waitqueue_head(&ihid->wait);
 	mutex_init(&ihid->reset_lock);
@@ -1089,11 +943,7 @@ static int i2c_hid_probe(struct i2c_client *client,
 	 * real computation later. */
 	ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
 	if (ret < 0)
-		goto err_regulator;
-
-	i2c_hid_acpi_fix_up_power(&client->dev);
-
-	i2c_hid_acpi_enable_wakeup(&client->dev);
+		goto err_powered;
 
 	device_enable_async_suspend(&client->dev);
 
@@ -1102,16 +952,16 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (ret < 0) {
 		dev_dbg(&client->dev, "nothing at this address: %d\n", ret);
 		ret = -ENXIO;
-		goto err_regulator;
+		goto err_powered;
 	}
 
 	ret = i2c_hid_fetch_hid_descriptor(ihid);
 	if (ret < 0)
-		goto err_regulator;
+		goto err_powered;
 
 	ret = i2c_hid_init_irq(client);
 	if (ret < 0)
-		goto err_regulator;
+		goto err_powered;
 
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
@@ -1150,14 +1000,14 @@ static int i2c_hid_probe(struct i2c_client *client,
 err_irq:
 	free_irq(client->irq, ihid);
 
-err_regulator:
-	regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
-			       ihid->pdata.supplies);
+err_powered:
+	i2c_hid_core_power_down(ihid);
 	i2c_hid_free_buffers(ihid);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_core_probe);
 
-static int i2c_hid_remove(struct i2c_client *client)
+int i2c_hid_core_remove(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid;
@@ -1170,24 +1020,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);
+	i2c_hid_core_power_down(ihid);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_core_remove);
 
-static void i2c_hid_shutdown(struct i2c_client *client)
+void i2c_hid_core_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);
-
-	i2c_hid_acpi_shutdown(&client->dev);
 }
+EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown);
 
 #ifdef CONFIG_PM_SLEEP
-static int i2c_hid_suspend(struct device *dev)
+int i2c_hid_core_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -1214,14 +1063,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);
+		i2c_hid_core_power_down(ihid);
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_core_suspend);
 
-static int i2c_hid_resume(struct device *dev)
+int i2c_hid_core_resume(struct device *dev)
 {
 	int ret;
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1230,13 +1079,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);
+		i2c_hid_core_power_up(ihid);
 	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
@@ -1271,37 +1114,9 @@ static int i2c_hid_resume(struct device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_hid_core_resume);
 #endif
 
-static const struct dev_pm_ops i2c_hid_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_suspend, i2c_hid_resume)
-};
-
-static const struct i2c_device_id i2c_hid_id_table[] = {
-	{ "hid", 0 },
-	{ "hid-over-i2c", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
-
-
-static struct i2c_driver i2c_hid_driver = {
-	.driver = {
-		.name	= "i2c_hid",
-		.pm	= &i2c_hid_pm,
-		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
-		.acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
-		.of_match_table = of_match_ptr(i2c_hid_of_match),
-	},
-
-	.probe		= i2c_hid_probe,
-	.remove		= i2c_hid_remove,
-	.shutdown	= i2c_hid_shutdown,
-	.id_table	= i2c_hid_id_table,
-};
-
-module_i2c_driver(i2c_hid_driver);
-
 MODULE_DESCRIPTION("HID over I2C core driver");
 MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
 MODULE_LICENSE("GPL");
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
new file mode 100644
index 000000000000..e1838cdef0aa
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -0,0 +1,149 @@
+/*
+ * HID over I2C Open Firmware Subclass
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * This code was forked out of the core code, which was partly based on
+ * "USB HID support for Linux":
+ *
+ *  Copyright (c) 1999 Andreas Gal
+ *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ *  Copyright (c) 2007-2008 Oliver Neukum
+ *  Copyright (c) 2006-2010 Jiri Kosina
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.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 i2c_hid_of {
+	struct i2chid_subclass_data subclass;
+
+	struct i2c_client *client;
+	struct regulator_bulk_data supplies[2];
+	int post_power_delay_ms;
+};
+
+static int i2c_hid_of_power_up_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of *ihid_of =
+		container_of(subclass, struct i2c_hid_of, subclass);
+	struct device *dev = &ihid_of->client->dev;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
+				    ihid_of->supplies);
+	if (ret) {
+		dev_warn(dev, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	if (ihid_of->post_power_delay_ms)
+		msleep(ihid_of->post_power_delay_ms);
+
+	return 0;
+}
+
+static void i2c_hid_of_power_down_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of *ihid_of = container_of(subclass, struct i2c_hid_of,
+						  subclass);
+
+	regulator_bulk_disable(ARRAY_SIZE(ihid_of->supplies),
+			       ihid_of->supplies);
+}
+
+static int i2c_hid_of_probe(struct i2c_client *client,
+			    const struct i2c_device_id *dev_id)
+{
+	struct device *dev = &client->dev;
+	struct i2c_hid_of *ihid_of;
+	u16 hid_descriptor_address;
+	int ret;
+	u32 val;
+
+	ihid_of = devm_kzalloc(&client->dev, sizeof(*ihid_of), GFP_KERNEL);
+	if (!ihid_of)
+		return -ENOMEM;
+
+	ihid_of->subclass.power_up_device = i2c_hid_of_power_up_device;
+	ihid_of->subclass.power_down_device = i2c_hid_of_power_down_device;
+
+	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
+	if (ret) {
+		dev_err(&client->dev, "HID register address not provided\n");
+		return -ENODEV;
+	}
+	if (val >> 16) {
+		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
+			val);
+		return -EINVAL;
+	}
+	hid_descriptor_address = val;
+
+	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
+				      &val))
+		ihid_of->post_power_delay_ms = val;
+
+	ihid_of->supplies[0].supply = "vdd";
+	ihid_of->supplies[1].supply = "vddl";
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(ihid_of->supplies),
+				      ihid_of->supplies);
+	if (ret)
+		return ret;
+
+	return i2c_hid_core_probe(client, &ihid_of->subclass,
+				  hid_descriptor_address);
+}
+
+static const struct dev_pm_ops i2c_hid_of_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+};
+
+static const struct of_device_id i2c_hid_of_match[] = {
+	{ .compatible = "hid-over-i2c" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
+
+static const struct i2c_device_id i2c_hid_of_id_table[] = {
+	{ "hid", 0 },
+	{ "hid-over-i2c", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, i2c_hid_of_id_table);
+
+static struct i2c_driver i2c_hid_of_driver = {
+	.driver = {
+		.name	= "i2c_hid_of",
+		.pm	= &i2c_hid_of_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = of_match_ptr(i2c_hid_of_match),
+	},
+
+	.probe		= i2c_hid_of_probe,
+	.remove		= i2c_hid_core_remove,
+	.shutdown	= i2c_hid_core_shutdown,
+	.id_table	= i2c_hid_of_id_table,
+};
+
+module_i2c_driver(i2c_hid_of_driver);
+
+MODULE_DESCRIPTION("HID over I2C OF driver");
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index a8c19aef5824..e5665224fe2b 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -3,6 +3,7 @@
 #ifndef I2C_HID_H
 #define I2C_HID_H
 
+#include <linux/i2c.h>
 
 #ifdef CONFIG_DMI
 struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
@@ -17,4 +18,27 @@ static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
 { return NULL; }
 #endif
 
+/**
+ * struct i2chid_subclass_data - Data passed from subclass to the core.
+ *
+ * @power_up_device: do sequencing to power up the device.
+ * @power_down_device: do sequencing to power down the device.
+ */
+struct i2chid_subclass_data {
+	int (*power_up_device)(struct i2chid_subclass_data *subclass);
+	void (*power_down_device)(struct i2chid_subclass_data *subclass);
+};
+
+int i2c_hid_core_probe(struct i2c_client *client,
+		       struct i2chid_subclass_data *subclass,
+		       u16 hid_descriptor_address);
+int i2c_hid_core_remove(struct i2c_client *client);
+
+void i2c_hid_core_shutdown(struct i2c_client *client);
+
+#ifdef CONFIG_PM_SLEEP
+int i2c_hid_core_suspend(struct device *dev);
+int i2c_hid_core_resume(struct device *dev);
+#endif
+
 #endif
diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
deleted file mode 100644
index c628bb5e1061..000000000000
--- a/include/linux/platform_data/i2c-hid.h
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * HID over I2C protocol implementation
- *
- * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
- * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file COPYING in the main directory of this archive for
- * more details.
- */
-
-#ifndef __LINUX_I2C_HID_H
-#define __LINUX_I2C_HID_H
-
-#include <linux/regulator/consumer.h>
-#include <linux/types.h>
-
-/**
- * struct i2chid_platform_data - used by hid over i2c implementation.
- * @hid_descriptor_address: i2c register where the HID descriptor is stored.
- * @supplies: regulators for powering on the device.
- * @post_power_delay_ms: delay after powering on before device is usable.
- *
- * 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.
- * The platform driver should also setup the gpio according to the device:
- *
- * A typical example is the following:
- *	irq = gpio_to_irq(intr_gpio);
- *	hkdk4412_i2c_devs5[0].irq = irq; // store the irq in i2c_board_info
- *	gpio_request(intr_gpio, "elan-irq");
- *	s3c_gpio_setpull(intr_gpio, S3C_GPIO_PULL_UP);
- */
-struct i2c_hid_platform_data {
-	u16 hid_descriptor_address;
-	struct regulator_bulk_data supplies[2];
-	int post_power_delay_ms;
-};
-
-#endif /* __LINUX_I2C_HID_H */
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v4 2/4] arm64: defconfig: Update config names for i2c-hid rejigger

[Douglas Anderson]

The i2c-hid driver has been split in two.  Let's enable both halves.

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

Changes in v4:
- ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4.

 arch/arm64/configs/defconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5cfe3cf6f2ac..9258d623331e 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -725,7 +725,8 @@ CONFIG_SND_SOC_WM8904=m
 CONFIG_SND_SOC_WSA881X=m
 CONFIG_SND_SIMPLE_CARD=m
 CONFIG_SND_AUDIO_GRAPH_CARD=m
-CONFIG_I2C_HID=m
+CONFIG_I2C_HID_ACPI=m
+CONFIG_I2C_HID_OF=m
 CONFIG_USB_CONN_GPIO=m
 CONFIG_USB=y
 CONFIG_USB_OTG=y
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v4 3/4] 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>
---

(no changes since v3)

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 v4 4/4] HID: i2c-hid: 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 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 | 120 ++++++++++++++++++++++++
 3 files changed, 138 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..7b27fd8b7401
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -0,0 +1,120 @@
+// 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_subclass_data subclass;
+
+	struct regulator *vdd;
+	struct gpio_desc *reset_gpio;
+	const struct goodix_i2c_hid_timing_data *timings;
+};
+
+static int goodix_i2c_hid_power_up_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of_goodix *ihid_goodix =
+		container_of(subclass, struct i2c_hid_of_goodix, subclass);
+	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_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of_goodix *ihid_goodix =
+		container_of(subclass, struct i2c_hid_of_goodix, subclass);
+
+	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->subclass.power_up_device = goodix_i2c_hid_power_up_device;
+	ihid_goodix->subclass.power_down_device = goodix_i2c_hid_power_down_device;
+
+	/* 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->subclass, 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 const struct dev_pm_ops goodix_i2c_hid_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+};
+
+static struct i2c_driver goodix_i2c_hid_ts_driver = {
+	.driver = {
+		.name	= "i2c_hid_of_goodix",
+		.pm	= &goodix_i2c_hid_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 <dianders@chromium.org>");
+MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.1.341.ge80a0c044ae-goog

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Hans de Goede]

Hi,

On 11/4/20 2:29 AM, Douglas Anderson wrote:
> This patch rejiggers the i2c-hid code so that the OF (Device Tree) and
> ACPI support is separated out a bit.  The OF and ACPI drivers are now
> effectively "subclasses" of the generic code.
> 
> Essentially, what we're doing here:
> * Make "power up" and "power down" a virtual function that can be
>   (optionally) implemented by subclasses.
> * Each subclass is a drive on its own, so it implements probe / remove
>   / suspend / resume / shutdown.  The core code provides
>   implementations that the subclasses can call into, or fully use for
>   their implementation if they don't have special needs.
> 
> We'll organize this so that we now have 3 modules: the old i2c-hid
> module becomes the "core" module and two new modules will depend on
> it, handling probing the specific device.
> 
> As part of this work, we'll remove the i2c-hid "platform data"
> concept since it's not needed.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thank you for doing this, overall this looks pretty good to me.

Some small comments inline, not a full review.

<snip>

> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> new file mode 100644
> index 000000000000..d4c29dc62297
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -0,0 +1,167 @@
> +/*
> + * HID over I2C ACPI Subclass
> + *
> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
> + * Copyright (c) 2012 Red Hat, Inc
> + *
> + * This code was forked out of the core code, which was partly based on
> + * "USB HID support for Linux":
> + *
> + *  Copyright (c) 1999 Andreas Gal
> + *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
> + *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
> + *  Copyright (c) 2007-2008 Oliver Neukum
> + *  Copyright (c) 2006-2010 Jiri Kosina
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file COPYING in the main directory of this archive for
> + * more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +
> +#include "i2c-hid.h"
> +
> +struct i2c_hid_acpi {
> +	struct i2chid_subclass_data subclass;

This feels a bit weird, we are the subclass so typically we would
be embedding a base_class data struct here ...

(more remarks below, note just my 2 cents you may want to wait
for feedback from others).

> +	struct i2c_client *client;

You pass this to i2c_hid_core_probe which then stores it own
copy, why not just store it in the subclass (or even better
baseclass) data struct ?

> +	bool power_fixed;
> +};
> +
> +static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
> +	/*
> +	 * The CHPN0001 ACPI device, which is used to describe the Chipone
> +	 * ICN8505 controller, has a _CID of PNP0C50 but is not HID compatible.
> +	 */
> +	{"CHPN0001", 0 },
> +	{ },
> +};
> +
> +static int i2c_hid_acpi_get_descriptor(struct i2c_client *client)
> +{
> +	static guid_t i2c_hid_guid =
> +		GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> +			  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> +	union acpi_object *obj;
> +	struct acpi_device *adev;
> +	acpi_handle handle;
> +	u16 hid_descriptor_address;
> +
> +	handle = ACPI_HANDLE(&client->dev);
> +	if (!handle || acpi_bus_get_device(handle, &adev)) {
> +		dev_err(&client->dev, "Error could not get ACPI device\n");
> +		return -ENODEV;
> +	}
> +
> +	if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
> +		return -ENODEV;
> +
> +	obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
> +				      ACPI_TYPE_INTEGER);
> +	if (!obj) {
> +		dev_err(&client->dev, "Error _DSM call to get HID descriptor address failed\n");
> +		return -ENODEV;
> +	}
> +
> +	hid_descriptor_address = obj->integer.value;
> +	ACPI_FREE(obj);
> +
> +	return hid_descriptor_address;
> +}
> +
> +static int i2c_hid_acpi_power_up_device(struct i2chid_subclass_data *subclass)
> +{
> +	struct i2c_hid_acpi *ihid_of =
> +		container_of(subclass, struct i2c_hid_acpi, subclass);
> +	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;
> +}
> +
> +int i2c_hid_acpi_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *dev_id)
> +{
> +	struct device *dev = &client->dev;
> +	struct i2c_hid_acpi *ihid_acpi;
> +	u16 hid_descriptor_address;
> +	int ret;
> +
> +	ihid_acpi = devm_kzalloc(&client->dev, sizeof(*ihid_acpi), GFP_KERNEL);
> +	if (!ihid_acpi)
> +		return -ENOMEM;
> +
> +	ihid_acpi->subclass.power_up_device = i2c_hid_acpi_power_up_device;
> +
> +	ret = i2c_hid_acpi_get_descriptor(client);
> +	if (ret < 0)
> +		return ret;
> +	hid_descriptor_address = ret;
> +
> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> +		device_set_wakeup_capable(dev, true);
> +		device_set_wakeup_enable(dev, false);
> +	}
> +
> +	return i2c_hid_core_probe(client, &ihid_acpi->subclass,
> +				  hid_descriptor_address);
> +}
> +
> +static void i2c_hid_acpi_shutdown(struct i2c_client *client)
> +{
> +	i2c_hid_core_shutdown(client);
> +	acpi_device_set_power(ACPI_COMPANION(&client->dev), ACPI_STATE_D3_COLD);
> +}
> +
> +static const struct dev_pm_ops i2c_hid_acpi_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
> +};
> +
> +static const struct acpi_device_id i2c_hid_acpi_match[] = {
> +	{"ACPI0C50", 0 },
> +	{"PNP0C50", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
> +
> +static const struct i2c_device_id i2c_hid_acpi_id_table[] = {
> +	{ "hid", 0 },
> +	{ "hid-over-i2c", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_id_table);
> +
> +static struct i2c_driver i2c_hid_acpi_driver = {
> +	.driver = {
> +		.name	= "i2c_hid_acpi",
> +		.pm	= &i2c_hid_acpi_pm,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +		.acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
> +	},
> +
> +	.probe		= i2c_hid_acpi_probe,
> +	.remove		= i2c_hid_core_remove,
> +	.shutdown	= i2c_hid_acpi_shutdown,
> +	.id_table	= i2c_hid_acpi_id_table,
> +};
> +
> +module_i2c_driver(i2c_hid_acpi_driver);
> +
> +MODULE_DESCRIPTION("HID over I2C ACPI driver");
> +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index aeff1ffb0c8b..7e7303c2a45e 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -35,12 +35,8 @@
>  #include <linux/kernel.h>
>  #include <linux/hid.h>
>  #include <linux/mutex.h>
> -#include <linux/acpi.h>
> -#include <linux/of.h>
>  #include <linux/regulator/consumer.h>
>  
> -#include <linux/platform_data/i2c-hid.h>
> -
>  #include "../hid-ids.h"
>  #include "i2c-hid.h"
>  
> @@ -156,10 +152,10 @@ struct i2c_hid {
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
>  
> -	struct i2c_hid_platform_data pdata;
> -
>  	bool			irq_wake_enabled;
>  	struct mutex		reset_lock;
> +
> +	struct i2chid_subclass_data *subclass;
>  };

Personally, I would do things a bit differently here:

1. Just add the

	int (*power_up_device)(struct i2chid_subclass_data *subclass);
	void (*power_down_device)(struct i2chid_subclass_data *subclass);

members which you put in the subclass struct here.

2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
and use this as the base-class which I described before (and store the client
pointer here).

3. And then kzalloc both this baseclass struct + the subclass-data
(only the bool "power_fixed" in the ACPI case) in one go in the subclass code
replacing 2 kzallocs (+ error checking with one, simplifying the code and
reducing memory fragmentation (by a tiny sliver).

###

About the power_*_device callbacks, I wonder if it would not be more consistent
to also have a shutdown callback and make i2c_driver.shutdown point to
a (modified) i2c_hid_core_shutdown() function. 

You may also want to consider pointing that shutdown callback to the power_off
function in the of case (in a separate commit as that is a behavioral change).

<snip>

> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> new file mode 100644
> index 000000000000..e1838cdef0aa
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -0,0 +1,149 @@
> +/*
> + * HID over I2C Open Firmware Subclass
> + *
> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
> + * Copyright (c) 2012 Red Hat, Inc

<snip>

> +MODULE_DESCRIPTION("HID over I2C OF driver");
> +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");

In case Benjamin misses this during his own review: I'm not sure if he
will want to be set as AUTHOR of this, given that part of the plan is
for someone else to be the primary point of contact for the of bits.

> +MODULE_LICENSE("GPL");

<snip>

> diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
> deleted file mode 100644
> index c628bb5e1061..000000000000
> --- a/include/linux/platform_data/i2c-hid.h
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/*
> - * HID over I2C protocol implementation
> - *
> - * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> - * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
> - *
> - * This file is subject to the terms and conditions of the GNU General Public
> - * License.  See the file COPYING in the main directory of this archive for
> - * more details.
> - */
> -
> -#ifndef __LINUX_I2C_HID_H
> -#define __LINUX_I2C_HID_H
> -
> -#include <linux/regulator/consumer.h>
> -#include <linux/types.h>
> -
> -/**
> - * struct i2chid_platform_data - used by hid over i2c implementation.
> - * @hid_descriptor_address: i2c register where the HID descriptor is stored.
> - * @supplies: regulators for powering on the device.
> - * @post_power_delay_ms: delay after powering on before device is usable.
> - *
> - * 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.
> - * The platform driver should also setup the gpio according to the device:
> - *
> - * A typical example is the following:
> - *	irq = gpio_to_irq(intr_gpio);
> - *	hkdk4412_i2c_devs5[0].irq = irq; // store the irq in i2c_board_info
> - *	gpio_request(intr_gpio, "elan-irq");
> - *	s3c_gpio_setpull(intr_gpio, S3C_GPIO_PULL_UP);
> - */
> -struct i2c_hid_platform_data {
> -	u16 hid_descriptor_address;
> -	struct regulator_bulk_data supplies[2];
> -	int post_power_delay_ms;
> -};
> -
> -#endif /* __LINUX_I2C_HID_H */

Nice, deleting platform_data files is always good :)

Regards,

Hans

Re: [PATCH v4 4/4] HID: i2c-hid: Introduce goodix-i2c-hid as a subclass of i2c-hid

[Hans de Goede]

Hi,

On 11/4/20 2:29 AM, Douglas Anderson wrote:
> 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 v4:
> - Totally redid based on the new subclass system.

Again just my 2 cents, but I'm not sure if this should be an
entirely separate driver, or just something added to the
generic drivers/hid/i2c-hid/i2c-hid-of.c code.

I have no real preference either way, just asking the
question to make sure both options are considered.

Regards,

Hans




> 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 | 120 ++++++++++++++++++++++++
>  3 files changed, 138 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..7b27fd8b7401
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -0,0 +1,120 @@
> +// 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_subclass_data subclass;
> +
> +	struct regulator *vdd;
> +	struct gpio_desc *reset_gpio;
> +	const struct goodix_i2c_hid_timing_data *timings;
> +};
> +
> +static int goodix_i2c_hid_power_up_device(struct i2chid_subclass_data *subclass)
> +{
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(subclass, struct i2c_hid_of_goodix, subclass);
> +	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_device(struct i2chid_subclass_data *subclass)
> +{
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(subclass, struct i2c_hid_of_goodix, subclass);
> +
> +	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->subclass.power_up_device = goodix_i2c_hid_power_up_device;
> +	ihid_goodix->subclass.power_down_device = goodix_i2c_hid_power_down_device;
> +
> +	/* 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->subclass, 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 const struct dev_pm_ops goodix_i2c_hid_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
> +};
> +
> +static struct i2c_driver goodix_i2c_hid_ts_driver = {
> +	.driver = {
> +		.name	= "i2c_hid_of_goodix",
> +		.pm	= &goodix_i2c_hid_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 <dianders@chromium.org>");
> +MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Doug Anderson]

Hi,

On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> > +#include "i2c-hid.h"
> > +
> > +struct i2c_hid_acpi {
> > +     struct i2chid_subclass_data subclass;
>
> This feels a bit weird, we are the subclass so typically we would
> be embedding a base_class data struct here ...
>
> (more remarks below, note just my 2 cents you may want to wait
> for feedback from others).
>
> > +     struct i2c_client *client;
>
> You pass this to i2c_hid_core_probe which then stores it own
> copy, why not just store it in the subclass (or even better
> baseclass) data struct ?

My goal was to avoid moving the big structure to the header file.
Without doing that, I think you need something more like the setup I
have.  I'll wait for Benjamin to comment on whether he'd prefer
something like what I have here or if I should move the structure.


> > @@ -156,10 +152,10 @@ struct i2c_hid {
> >
> >       wait_queue_head_t       wait;           /* For waiting the interrupt */
> >
> > -     struct i2c_hid_platform_data pdata;
> > -
> >       bool                    irq_wake_enabled;
> >       struct mutex            reset_lock;
> > +
> > +     struct i2chid_subclass_data *subclass;
> >  };
>
> Personally, I would do things a bit differently here:
>
> 1. Just add the
>
>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
>
> members which you put in the subclass struct here.
>
> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
> and use this as the base-class which I described before (and store the client
> pointer here).
>
> 3. And then kzalloc both this baseclass struct + the subclass-data
> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
> replacing 2 kzallocs (+ error checking with one, simplifying the code and
> reducing memory fragmentation (by a tiny sliver).

Sure, I'll do that if Benjamin likes moving the structure to the header.


> About the power_*_device callbacks, I wonder if it would not be more consistent
> to also have a shutdown callback and make i2c_driver.shutdown point to
> a (modified) i2c_hid_core_shutdown() function.

Personally this doesn't seem cleaner to me, but I'm happy to do it if
folks like it better.  Coming up with a name for the callback would be
a bit awkward, which is a sign that this isn't quite ideal?  For the
power_up()/power_down() those are sane concepts to abstract out.  Here
we'd be abstracting out "subclass_shutdown_tail()" or something?
...and if a subclass needs something at the head of shutdown, we'd
need to add a "subclass_shutdown_head()"?


> You may also want to consider pointing that shutdown callback to the power_off
> function in the of case (in a separate commit as that is a behavioral change).

I don't think this is the point of shutdown, but I could be corrected.
Shutdown isn't really supposed to be the same as driver remove or
anything.  IIUC the main point of shutdown is to support kexec and the
goal is to quiesce DMA transactions.  Turning off power has never been
a requirement that I was aware of.  We don't want to jam too much
stuff in shutdown or else "shutdown" becomes as slow as boot for no
good reason, right?


> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> > new file mode 100644
> > index 000000000000..e1838cdef0aa
> > --- /dev/null
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> > @@ -0,0 +1,149 @@
> > +/*
> > + * HID over I2C Open Firmware Subclass
> > + *
> > + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
> > + * Copyright (c) 2012 Red Hat, Inc
>
> <snip>
>
> > +MODULE_DESCRIPTION("HID over I2C OF driver");
> > +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
>
> In case Benjamin misses this during his own review: I'm not sure if he
> will want to be set as AUTHOR of this, given that part of the plan is
> for someone else to be the primary point of contact for the of bits.

I can stick myself in as the author if needed.  I'll wait for
Benjamin's feedback here.


-Doug

Re: [PATCH v4 4/4] HID: i2c-hid: Introduce goodix-i2c-hid as a subclass of i2c-hid

[Doug Anderson]

Hi,

On Wed, Nov 4, 2020 at 4:09 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/4/20 2:29 AM, Douglas Anderson wrote:
> > 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 v4:
> > - Totally redid based on the new subclass system.
>
> Again just my 2 cents, but I'm not sure if this should be an
> entirely separate driver, or just something added to the
> generic drivers/hid/i2c-hid/i2c-hid-of.c code.
>
> I have no real preference either way, just asking the
> question to make sure both options are considered.

Yeah, I thought about it.  ...but at the moment I'm not convinced it's
really any cleaner and I think there's very little shared code.  In
the goodix case, I don't want to specify the extra regulator or the
timings or even the hid descriptor address.  In the non-goodix case I
don't want the goodix properties.  It also sounded as if Benjamin's
preferred solutions involved having two separate files.  I'll wait for
Benjamin's feedback here, though, and if he wants me to combine them
then I'll give it a shot for v5.

-Doug

-Doug

Re: [PATCH v4 3/4] dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P

[Rob Herring]

On Tue, Nov 03, 2020 at 05:29:28PM -0800, Douglas Anderson wrote:
> 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.

'dt-bindings: input: ...' for the subject.

> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v3)
> 
> 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.

Perhaps mention hid over i2c here given that's only captured in the 
commit message.

With those nits addressed.

Reviewed-by: Rob Herring <robh@kernel.org>

> +
> +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
> 

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Hans de Goede]

Hi,

On 11/4/20 5:06 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> +#include "i2c-hid.h"
>>> +
>>> +struct i2c_hid_acpi {
>>> +     struct i2chid_subclass_data subclass;
>>
>> This feels a bit weird, we are the subclass so typically we would
>> be embedding a base_class data struct here ...
>>
>> (more remarks below, note just my 2 cents you may want to wait
>> for feedback from others).
>>
>>> +     struct i2c_client *client;
>>
>> You pass this to i2c_hid_core_probe which then stores it own
>> copy, why not just store it in the subclass (or even better
>> baseclass) data struct ?
> 
> My goal was to avoid moving the big structure to the header file.
> Without doing that, I think you need something more like the setup I
> have.  I'll wait for Benjamin to comment on whether he'd prefer
> something like what I have here or if I should move the structure.

Ok, if Benjamin decides to keep things this way, can you consider
renaming i2chid_subclass_data to i2chid_ops ?

It just feels weird to have a struct with subclass in the name
embedded inside as a member in another struct, usualy the kobject model
works by having the the parent/base-class struct embedded inside
the subclass data struct.

This also avoids the need for a callback_priv_data pointer to the ops,
as the ops get a pointer to the baseclass data struct as argument and
you can then use container_of to get your own subclassdata struct
since that encapsulates (contains) the baseclass struct.

Note the dropping of the callback_priv_data pointer only works if you
do move the entire struct to the header.



> 
> 
>>> @@ -156,10 +152,10 @@ struct i2c_hid {
>>>
>>>       wait_queue_head_t       wait;           /* For waiting the interrupt */
>>>
>>> -     struct i2c_hid_platform_data pdata;
>>> -
>>>       bool                    irq_wake_enabled;
>>>       struct mutex            reset_lock;
>>> +
>>> +     struct i2chid_subclass_data *subclass;
>>>  };
>>
>> Personally, I would do things a bit differently here:
>>
>> 1. Just add the
>>
>>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
>>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
>>
>> members which you put in the subclass struct here.
>>
>> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
>> and use this as the base-class which I described before (and store the client
>> pointer here).
>>
>> 3. And then kzalloc both this baseclass struct + the subclass-data
>> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
>> replacing 2 kzallocs (+ error checking with one, simplifying the code and
>> reducing memory fragmentation (by a tiny sliver).
> 
> Sure, I'll do that if Benjamin likes moving the structure to the header.
> 
> 
>> About the power_*_device callbacks, I wonder if it would not be more consistent
>> to also have a shutdown callback and make i2c_driver.shutdown point to
>> a (modified) i2c_hid_core_shutdown() function.
> 
> Personally this doesn't seem cleaner to me, but I'm happy to do it if
> folks like it better.  Coming up with a name for the callback would be
> a bit awkward, which is a sign that this isn't quite ideal?  For the
> power_up()/power_down() those are sane concepts to abstract out.  Here
> we'd be abstracting out "subclass_shutdown_tail()" or something?
> ...and if a subclass needs something at the head of shutdown, we'd
> need to add a "subclass_shutdown_head()"?

I have no real preference here either way.

>> You may also want to consider pointing that shutdown callback to the power_off
>> function in the of case (in a separate commit as that is a behavioral change).
> 
> I don't think this is the point of shutdown, but I could be corrected.
> Shutdown isn't really supposed to be the same as driver remove or
> anything.  IIUC the main point of shutdown is to support kexec and the
> goal is to quiesce DMA transactions.  Turning off power has never been
> a requirement that I was aware of.  We don't want to jam too much
> stuff in shutdown or else "shutdown" becomes as slow as boot for no
> good reason, right?

This sorta depends on if the regulators for the HID device are part of the
PMIC or not. If they are part of the PMIC then on shutdown they will
typically be turned off by the PMIC. But if they are separate they may
stay enabled on shutdown.

Anyways I again have no real preference here...

Regards,

Hans






> 
> 
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
>>> new file mode 100644
>>> index 000000000000..e1838cdef0aa
>>> --- /dev/null
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
>>> @@ -0,0 +1,149 @@
>>> +/*
>>> + * HID over I2C Open Firmware Subclass
>>> + *
>>> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
>>> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
>>> + * Copyright (c) 2012 Red Hat, Inc
>>
>> <snip>
>>
>>> +MODULE_DESCRIPTION("HID over I2C OF driver");
>>> +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
>>
>> In case Benjamin misses this during his own review: I'm not sure if he
>> will want to be set as AUTHOR of this, given that part of the plan is
>> for someone else to be the primary point of contact for the of bits.
> 
> I can stick myself in as the author if needed.  I'll wait for
> Benjamin's feedback here.
> 
> 
> -Doug
> 

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Benjamin Tissoires]

Hi,

sorry for the delay. I have been heavily sidetracked and have a bunch
of internal deadlines coming in :/

On Mon, Nov 9, 2020 at 12:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/4/20 5:06 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >>> +#include "i2c-hid.h"
> >>> +
> >>> +struct i2c_hid_acpi {
> >>> +     struct i2chid_subclass_data subclass;
> >>
> >> This feels a bit weird, we are the subclass so typically we would
> >> be embedding a base_class data struct here ...
> >>
> >> (more remarks below, note just my 2 cents you may want to wait
> >> for feedback from others).
> >>
> >>> +     struct i2c_client *client;
> >>
> >> You pass this to i2c_hid_core_probe which then stores it own
> >> copy, why not just store it in the subclass (or even better
> >> baseclass) data struct ?
> >
> > My goal was to avoid moving the big structure to the header file.
> > Without doing that, I think you need something more like the setup I
> > have.  I'll wait for Benjamin to comment on whether he'd prefer
> > something like what I have here or if I should move the structure.
>
> Ok, if Benjamin decides to keep things this way, can you consider
> renaming i2chid_subclass_data to i2chid_ops ?
>
> It just feels weird to have a struct with subclass in the name
> embedded inside as a member in another struct, usualy the kobject model
> works by having the the parent/base-class struct embedded inside
> the subclass data struct.
>
> This also avoids the need for a callback_priv_data pointer to the ops,
> as the ops get a pointer to the baseclass data struct as argument and
> you can then use container_of to get your own subclassdata struct
> since that encapsulates (contains) the baseclass struct.
>
> Note the dropping of the callback_priv_data pointer only works if you
> do move the entire struct to the header.

I am not sure my opinion is the best in this case. However, the one
thing I'd like us to do is knowing which use cases we are solving, and
this should hopefully help us finding the best approach:

- use case 1: fully upstream driver (like this one)
   -> the OEM sets up the DT associated with the embedded devices
   -> the kernel is compiled with the proper flags/configs
  -> the device works out of the box (yay!)

- use case 2: tinkerer in a garage
  -> assembly of a generic SoC + Goodix v-next panel (that needs
modifications in the driver)
  -> use of a generic (arm?) distribution
  -> the user compiles the new (changed) goodix driver
  -> the DT is populated (with overloads)
  -> the device works
  -> do we want to keep compatibility across kernel versions (not
recompile the custom module)

- use case 3: Google fixed kernel
  -> the kernel is built once for all platforms
  -> OEMs can recompile a few drivers if they need, but can not touch
the core system
  -> DT/goodix specific drivers are embedded
  -> device works
  -> do we want compatibility across major versions, and how "nice" we
want to be with OEM?

I understand that use case 2 should in the end become use case 1, but
having a possibility for casual/enthusiasts developers to fix their
hardware is always nice.

So to me, having the base struct in an external header means we are
adding a lot of ABI and putting a lot more weight to case 1.

Personally, I am not that much in favour of being too strict and I
think we also want to help these external drivers. It is true that
i2c-hid should be relatively stable from now on, but we can never
predict the future, so maybe the external header is not so much a good
thing (for me).

Anyway, if we were to extract the base struct, we would need to
provide allocators to be able to keep forward compatibility (I think).

Does that help a bit?

[mode bikeshedding on]
And to go back to Hans' suggestion, I really prefer i2chid_ops. This
whole architecture makes me think of a bus, not a subclass hierarchy.
In the same way we have the hid bus, we could have the i2c-hid bus,
with separate drivers in it (acpi, of, goodix).

Note that I don't want the i2c-hid to be converted into an actual bus,
but just rely on the concepts.
[bikeshedding off]

>
>
>
> >
> >
> >>> @@ -156,10 +152,10 @@ struct i2c_hid {
> >>>
> >>>       wait_queue_head_t       wait;           /* For waiting the interrupt */
> >>>
> >>> -     struct i2c_hid_platform_data pdata;
> >>> -
> >>>       bool                    irq_wake_enabled;
> >>>       struct mutex            reset_lock;
> >>> +
> >>> +     struct i2chid_subclass_data *subclass;
> >>>  };
> >>
> >> Personally, I would do things a bit differently here:
> >>
> >> 1. Just add the
> >>
> >>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
> >>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
> >>
> >> members which you put in the subclass struct here.
> >>
> >> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
> >> and use this as the base-class which I described before (and store the client
> >> pointer here).
> >>
> >> 3. And then kzalloc both this baseclass struct + the subclass-data
> >> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
> >> replacing 2 kzallocs (+ error checking with one, simplifying the code and
> >> reducing memory fragmentation (by a tiny sliver).
> >
> > Sure, I'll do that if Benjamin likes moving the structure to the header.
> >
> >
> >> About the power_*_device callbacks, I wonder if it would not be more consistent
> >> to also have a shutdown callback and make i2c_driver.shutdown point to
> >> a (modified) i2c_hid_core_shutdown() function.
> >
> > Personally this doesn't seem cleaner to me, but I'm happy to do it if
> > folks like it better.  Coming up with a name for the callback would be
> > a bit awkward, which is a sign that this isn't quite ideal?  For the
> > power_up()/power_down() those are sane concepts to abstract out.  Here
> > we'd be abstracting out "subclass_shutdown_tail()" or something?
> > ...and if a subclass needs something at the head of shutdown, we'd
> > need to add a "subclass_shutdown_head()"?
>
> I have no real preference here either way.

If we are using i2chid_ops, we could just have `shutdown_tail()`.
Basically drop any "device" or "subclass" in the op name.
This would lead to better code IMO: "ihid->dev_ops->shutdown()" for example

Cheers,
Benjamin

>
> >> You may also want to consider pointing that shutdown callback to the power_off
> >> function in the of case (in a separate commit as that is a behavioral change).
> >
> > I don't think this is the point of shutdown, but I could be corrected.
> > Shutdown isn't really supposed to be the same as driver remove or
> > anything.  IIUC the main point of shutdown is to support kexec and the
> > goal is to quiesce DMA transactions.  Turning off power has never been
> > a requirement that I was aware of.  We don't want to jam too much
> > stuff in shutdown or else "shutdown" becomes as slow as boot for no
> > good reason, right?
>
> This sorta depends on if the regulators for the HID device are part of the
> PMIC or not. If they are part of the PMIC then on shutdown they will
> typically be turned off by the PMIC. But if they are separate they may
> stay enabled on shutdown.
>
> Anyways I again have no real preference here...
>
> Regards,
>
> Hans
>
>
>
>
>
>
> >
> >
> >>> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> >>> new file mode 100644
> >>> index 000000000000..e1838cdef0aa
> >>> --- /dev/null
> >>> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> >>> @@ -0,0 +1,149 @@
> >>> +/*
> >>> + * HID over I2C Open Firmware Subclass
> >>> + *
> >>> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >>> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
> >>> + * Copyright (c) 2012 Red Hat, Inc
> >>
> >> <snip>
> >>
> >>> +MODULE_DESCRIPTION("HID over I2C OF driver");
> >>> +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
> >>
> >> In case Benjamin misses this during his own review: I'm not sure if he
> >> will want to be set as AUTHOR of this, given that part of the plan is
> >> for someone else to be the primary point of contact for the of bits.
> >
> > I can stick myself in as the author if needed.  I'll wait for
> > Benjamin's feedback here.
> >
> >
> > -Doug
> >
>

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Hans de Goede]

Hi,

On 11/9/20 3:29 PM, Benjamin Tissoires wrote:
> Hi,
> 
> sorry for the delay. I have been heavily sidetracked and have a bunch
> of internal deadlines coming in :/
> 
> On Mon, Nov 9, 2020 at 12:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/4/20 5:06 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>>> +#include "i2c-hid.h"
>>>>> +
>>>>> +struct i2c_hid_acpi {
>>>>> +     struct i2chid_subclass_data subclass;
>>>>
>>>> This feels a bit weird, we are the subclass so typically we would
>>>> be embedding a base_class data struct here ...
>>>>
>>>> (more remarks below, note just my 2 cents you may want to wait
>>>> for feedback from others).
>>>>
>>>>> +     struct i2c_client *client;
>>>>
>>>> You pass this to i2c_hid_core_probe which then stores it own
>>>> copy, why not just store it in the subclass (or even better
>>>> baseclass) data struct ?
>>>
>>> My goal was to avoid moving the big structure to the header file.
>>> Without doing that, I think you need something more like the setup I
>>> have.  I'll wait for Benjamin to comment on whether he'd prefer
>>> something like what I have here or if I should move the structure.
>>
>> Ok, if Benjamin decides to keep things this way, can you consider
>> renaming i2chid_subclass_data to i2chid_ops ?
>>
>> It just feels weird to have a struct with subclass in the name
>> embedded inside as a member in another struct, usualy the kobject model
>> works by having the the parent/base-class struct embedded inside
>> the subclass data struct.
>>
>> This also avoids the need for a callback_priv_data pointer to the ops,
>> as the ops get a pointer to the baseclass data struct as argument and
>> you can then use container_of to get your own subclassdata struct
>> since that encapsulates (contains) the baseclass struct.
>>
>> Note the dropping of the callback_priv_data pointer only works if you
>> do move the entire struct to the header.
> 
> I am not sure my opinion is the best in this case. However, the one
> thing I'd like us to do is knowing which use cases we are solving, and
> this should hopefully help us finding the best approach:
> 
> - use case 1: fully upstream driver (like this one)
>    -> the OEM sets up the DT associated with the embedded devices
>    -> the kernel is compiled with the proper flags/configs
>   -> the device works out of the box (yay!)
> 
> - use case 2: tinkerer in a garage
>   -> assembly of a generic SoC + Goodix v-next panel (that needs
> modifications in the driver)
>   -> use of a generic (arm?) distribution
>   -> the user compiles the new (changed) goodix driver
>   -> the DT is populated (with overloads)
>   -> the device works
>   -> do we want to keep compatibility across kernel versions (not
> recompile the custom module)
> 
> - use case 3: Google fixed kernel
>   -> the kernel is built once for all platforms
>   -> OEMs can recompile a few drivers if they need, but can not touch
> the core system
>   -> DT/goodix specific drivers are embedded
>   -> device works
>   -> do we want compatibility across major versions, and how "nice" we
> want to be with OEM?
> 
> I understand that use case 2 should in the end become use case 1, but
> having a possibility for casual/enthusiasts developers to fix their
> hardware is always nice.
> 
> So to me, having the base struct in an external header means we are
> adding a lot of ABI and putting a lot more weight to case 1.
> 
> Personally, I am not that much in favour of being too strict and I
> think we also want to help these external drivers. It is true that
> i2c-hid should be relatively stable from now on, but we can never
> predict the future, so maybe the external header is not so much a good
> thing (for me).
> 
> Anyway, if we were to extract the base struct, we would need to
> provide allocators to be able to keep forward compatibility (I think).
> 
> Does that help a bit?
> 
> [mode bikeshedding on]
> And to go back to Hans' suggestion, I really prefer i2chid_ops. This
> whole architecture makes me think of a bus, not a subclass hierarchy.
> In the same way we have the hid bus, we could have the i2c-hid bus,
> with separate drivers in it (acpi, of, goodix).
> 
> Note that I don't want the i2c-hid to be converted into an actual bus,
> but just rely on the concepts.
> [bikeshedding off]

Ok, so TL;DR: keep as is but rename subclass to i2chid_ops. That works
for me.

>>>>> @@ -156,10 +152,10 @@ struct i2c_hid {
>>>>>
>>>>>       wait_queue_head_t       wait;           /* For waiting the interrupt */
>>>>>
>>>>> -     struct i2c_hid_platform_data pdata;
>>>>> -
>>>>>       bool                    irq_wake_enabled;
>>>>>       struct mutex            reset_lock;
>>>>> +
>>>>> +     struct i2chid_subclass_data *subclass;
>>>>>  };
>>>>
>>>> Personally, I would do things a bit differently here:
>>>>
>>>> 1. Just add the
>>>>
>>>>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
>>>>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
>>>>
>>>> members which you put in the subclass struct here.
>>>>
>>>> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
>>>> and use this as the base-class which I described before (and store the client
>>>> pointer here).
>>>>
>>>> 3. And then kzalloc both this baseclass struct + the subclass-data
>>>> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
>>>> replacing 2 kzallocs (+ error checking with one, simplifying the code and
>>>> reducing memory fragmentation (by a tiny sliver).
>>>
>>> Sure, I'll do that if Benjamin likes moving the structure to the header.
>>>
>>>
>>>> About the power_*_device callbacks, I wonder if it would not be more consistent
>>>> to also have a shutdown callback and make i2c_driver.shutdown point to
>>>> a (modified) i2c_hid_core_shutdown() function.
>>>
>>> Personally this doesn't seem cleaner to me, but I'm happy to do it if
>>> folks like it better.  Coming up with a name for the callback would be
>>> a bit awkward, which is a sign that this isn't quite ideal?  For the
>>> power_up()/power_down() those are sane concepts to abstract out.  Here
>>> we'd be abstracting out "subclass_shutdown_tail()" or something?
>>> ...and if a subclass needs something at the head of shutdown, we'd
>>> need to add a "subclass_shutdown_head()"?
>>
>> I have no real preference here either way.
> 
> If we are using i2chid_ops, we could just have `shutdown_tail()`.
> Basically drop any "device" or "subclass" in the op name.
> This would lead to better code IMO: "ihid->dev_ops->shutdown()" for example


This also works for me.

Regards,

Hans

Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses

[Doug Anderson]

Hi,

On Mon, Nov 9, 2020 at 6:44 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/9/20 3:29 PM, Benjamin Tissoires wrote:
> > Hi,
> >
> > sorry for the delay. I have been heavily sidetracked and have a bunch
> > of internal deadlines coming in :/
> >
> > On Mon, Nov 9, 2020 at 12:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 11/4/20 5:06 PM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Wed, Nov 4, 2020 at 4:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>>> +#include "i2c-hid.h"
> >>>>> +
> >>>>> +struct i2c_hid_acpi {
> >>>>> +     struct i2chid_subclass_data subclass;
> >>>>
> >>>> This feels a bit weird, we are the subclass so typically we would
> >>>> be embedding a base_class data struct here ...
> >>>>
> >>>> (more remarks below, note just my 2 cents you may want to wait
> >>>> for feedback from others).
> >>>>
> >>>>> +     struct i2c_client *client;
> >>>>
> >>>> You pass this to i2c_hid_core_probe which then stores it own
> >>>> copy, why not just store it in the subclass (or even better
> >>>> baseclass) data struct ?
> >>>
> >>> My goal was to avoid moving the big structure to the header file.
> >>> Without doing that, I think you need something more like the setup I
> >>> have.  I'll wait for Benjamin to comment on whether he'd prefer
> >>> something like what I have here or if I should move the structure.
> >>
> >> Ok, if Benjamin decides to keep things this way, can you consider
> >> renaming i2chid_subclass_data to i2chid_ops ?
> >>
> >> It just feels weird to have a struct with subclass in the name
> >> embedded inside as a member in another struct, usualy the kobject model
> >> works by having the the parent/base-class struct embedded inside
> >> the subclass data struct.
> >>
> >> This also avoids the need for a callback_priv_data pointer to the ops,
> >> as the ops get a pointer to the baseclass data struct as argument and
> >> you can then use container_of to get your own subclassdata struct
> >> since that encapsulates (contains) the baseclass struct.
> >>
> >> Note the dropping of the callback_priv_data pointer only works if you
> >> do move the entire struct to the header.
> >
> > I am not sure my opinion is the best in this case. However, the one
> > thing I'd like us to do is knowing which use cases we are solving, and
> > this should hopefully help us finding the best approach:
> >
> > - use case 1: fully upstream driver (like this one)
> >    -> the OEM sets up the DT associated with the embedded devices
> >    -> the kernel is compiled with the proper flags/configs
> >   -> the device works out of the box (yay!)
> >
> > - use case 2: tinkerer in a garage
> >   -> assembly of a generic SoC + Goodix v-next panel (that needs
> > modifications in the driver)
> >   -> use of a generic (arm?) distribution
> >   -> the user compiles the new (changed) goodix driver
> >   -> the DT is populated (with overloads)
> >   -> the device works
> >   -> do we want to keep compatibility across kernel versions (not
> > recompile the custom module)
> >
> > - use case 3: Google fixed kernel
> >   -> the kernel is built once for all platforms
> >   -> OEMs can recompile a few drivers if they need, but can not touch
> > the core system
> >   -> DT/goodix specific drivers are embedded
> >   -> device works
> >   -> do we want compatibility across major versions, and how "nice" we
> > want to be with OEM?
> >
> > I understand that use case 2 should in the end become use case 1, but
> > having a possibility for casual/enthusiasts developers to fix their
> > hardware is always nice.
> >
> > So to me, having the base struct in an external header means we are
> > adding a lot of ABI and putting a lot more weight to case 1.
> >
> > Personally, I am not that much in favour of being too strict and I
> > think we also want to help these external drivers. It is true that
> > i2c-hid should be relatively stable from now on, but we can never
> > predict the future, so maybe the external header is not so much a good
> > thing (for me).
> >
> > Anyway, if we were to extract the base struct, we would need to
> > provide allocators to be able to keep forward compatibility (I think).
> >
> > Does that help a bit?
> >
> > [mode bikeshedding on]
> > And to go back to Hans' suggestion, I really prefer i2chid_ops. This
> > whole architecture makes me think of a bus, not a subclass hierarchy.
> > In the same way we have the hid bus, we could have the i2c-hid bus,
> > with separate drivers in it (acpi, of, goodix).
> >
> > Note that I don't want the i2c-hid to be converted into an actual bus,
> > but just rely on the concepts.
> > [bikeshedding off]
>
> Ok, so TL;DR: keep as is but rename subclass to i2chid_ops. That works
> for me.

Done in v5.


> >>>>> @@ -156,10 +152,10 @@ struct i2c_hid {
> >>>>>
> >>>>>       wait_queue_head_t       wait;           /* For waiting the interrupt */
> >>>>>
> >>>>> -     struct i2c_hid_platform_data pdata;
> >>>>> -
> >>>>>       bool                    irq_wake_enabled;
> >>>>>       struct mutex            reset_lock;
> >>>>> +
> >>>>> +     struct i2chid_subclass_data *subclass;
> >>>>>  };
> >>>>
> >>>> Personally, I would do things a bit differently here:
> >>>>
> >>>> 1. Just add the
> >>>>
> >>>>         int (*power_up_device)(struct i2chid_subclass_data *subclass);
> >>>>         void (*power_down_device)(struct i2chid_subclass_data *subclass);
> >>>>
> >>>> members which you put in the subclass struct here.
> >>>>
> >>>> 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h
> >>>> and use this as the base-class which I described before (and store the client
> >>>> pointer here).
> >>>>
> >>>> 3. And then kzalloc both this baseclass struct + the subclass-data
> >>>> (only the bool "power_fixed" in the ACPI case) in one go in the subclass code
> >>>> replacing 2 kzallocs (+ error checking with one, simplifying the code and
> >>>> reducing memory fragmentation (by a tiny sliver).
> >>>
> >>> Sure, I'll do that if Benjamin likes moving the structure to the header.
> >>>
> >>>
> >>>> About the power_*_device callbacks, I wonder if it would not be more consistent
> >>>> to also have a shutdown callback and make i2c_driver.shutdown point to
> >>>> a (modified) i2c_hid_core_shutdown() function.
> >>>
> >>> Personally this doesn't seem cleaner to me, but I'm happy to do it if
> >>> folks like it better.  Coming up with a name for the callback would be
> >>> a bit awkward, which is a sign that this isn't quite ideal?  For the
> >>> power_up()/power_down() those are sane concepts to abstract out.  Here
> >>> we'd be abstracting out "subclass_shutdown_tail()" or something?
> >>> ...and if a subclass needs something at the head of shutdown, we'd
> >>> need to add a "subclass_shutdown_head()"?
> >>
> >> I have no real preference here either way.
> >
> > If we are using i2chid_ops, we could just have `shutdown_tail()`.
> > Basically drop any "device" or "subclass" in the op name.
> > This would lead to better code IMO: "ihid->dev_ops->shutdown()" for example
>
>
> This also works for me.

I've done this part and called the callback "shutdown_tail()", which I
think was what was agreed upon above.  If you want me to rename it to
"shutdown()" I can always send a v6.

NOTE: I haven't added a patch that makes shutdown do a "power off" by
default.  That seems like it should wait until there's a use case
showing that it helps with something.

-Doug