[PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Hans de Goede]

Hi All,

This series consist of 2 parts:

1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
   devices which are not enumerated by ACPI to work on ACPI platforms
   (by manual i2c_client instantiation using i2c_client_id matching).

2. Patches 4-6. Remove the special i2c-hid-of-elan and i2c-hid-of-goodix
   driver, folding the functionality into the generic i2c-hid-of driver.
   Since 1. requires adding reset-gpio support to i2c-hid-of there was
   very little difference left between the generic i2c-hid-of code and
   the specialized drivers. So I decided to merge them into the generic
   driver instead of having duplicate code.

Note patches 4-6 have not been actually tested with an "elan,ekth6915"
touchscreen nor with a "goodix,gt7375p" touchscreen.

Douglas, can you perhaps test this patch-set with an "elan,ekth6915"
touchscreen and with a "goodix,gt7375p" touchscreen ?

Regards,

Hans


Hans de Goede (6):
  HID: i2c-hid-of: Consistenly use dev local variable in probe()
  HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
  HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
  HID: i2c-hid-of: Add chip_data struct
  HID: i2c-hid-of: Consolidate Elan support into generic i2c-hid-of
    driver
  HID: i2c-hid-of: Consolidate Goodix support into generic i2c-hid-of
    driver

 drivers/hid/i2c-hid/Kconfig             |  36 +------
 drivers/hid/i2c-hid/Makefile            |   2 -
 drivers/hid/i2c-hid/i2c-hid-of-elan.c   | 129 ------------------------
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 125 -----------------------
 drivers/hid/i2c-hid/i2c-hid-of.c        | 124 +++++++++++++++++++----
 5 files changed, 106 insertions(+), 310 deletions(-)
 delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-elan.c
 delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c

-- 
2.39.1

[PATCH 1/6] HID: i2c-hid-of: Consistenly use dev local variable in probe()

[Hans de Goede]

i2c_hid_of_probe() has a dev local variable pointing to &i2c_client->dev,
consistently use this everywhere in i2c_hid_of_probe().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 10176568133a..c82a5a54c3e6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -75,7 +75,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	int ret;
 	u32 val;
 
-	ihid_of = devm_kzalloc(&client->dev, sizeof(*ihid_of), GFP_KERNEL);
+	ihid_of = devm_kzalloc(dev, sizeof(*ihid_of), GFP_KERNEL);
 	if (!ihid_of)
 		return -ENOMEM;
 
@@ -84,24 +84,21 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 
 	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
 	if (ret) {
-		dev_err(&client->dev, "HID register address not provided\n");
+		dev_err(dev, "HID register address not provided\n");
 		return -ENODEV;
 	}
 	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-			val);
+		dev_err(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))
+	if (!device_property_read_u32(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),
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
 				      ihid_of->supplies);
 	if (ret)
 		return ret;
-- 
2.39.1

[PATCH 2/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Hans de Goede]

There are some x86 tablets / 2-in-1s which ship with Android as their
factory OS image. These have pretty broken ACPI tables, relying on
everything being hardcoded in the factory kernel image.

platform/x86/x86-android-tablets.c manually instantiates i2c-clients for
i2c devices on these tablets to make them work with the mainline kernel.

The Lenovo Yoga Book 1 (yb1-x90f/l) is such a 2-in-1. It has 2 I2C-HID
devices its main touchscreen and a Wacom digitizer. Its main touchscreen
can alternatively also be used in HiDeep's native protocol mode but
for the Wacom digitizer we really need I2C-HID.

This patch allows using i2c-hid-of on non OF platforms so that it can
bind to a non ACPI instantiated i2c_client on x86 for the Wacom digitizer.
Note the driver already has an "i2c-over-hid" i2c_device_id (rather then
an of_device_id).

Besides enabling building on non-OF platforms this also replaces
the only of_property_read_u32() call with device_property_read_u32() note
that other properties where already read using device_property_read_...().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/Kconfig      | 6 ++++--
 drivers/hid/i2c-hid/i2c-hid-of.c | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 4439be7fa74d..3be17109301a 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -23,12 +23,14 @@ config I2C_HID_ACPI
 
 config I2C_HID_OF
 	tristate "HID over I2C transport layer Open Firmware driver"
-	depends on OF
+	# No "depends on OF" because this can also be used for manually
+	# (board-file) instantiated "hid-over-i2c" type i2c-clients.
 	select I2C_HID_CORE
 	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.
+	  This driver supports Open Firmware (Device Tree)-based systems as
+	  well as binding to manually (board-file) instantiated i2c-hid-clients.
 
 	  If unsure, say N.
 
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index c82a5a54c3e6..385f7460e03c 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -82,7 +82,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	ihid_of->ops.power_up = i2c_hid_of_power_up;
 	ihid_of->ops.power_down = i2c_hid_of_power_down;
 
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
+	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
 	if (ret) {
 		dev_err(dev, "HID register address not provided\n");
 		return -ENODEV;
@@ -113,11 +113,13 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 				  hid_descriptor_address, quirks);
 }
 
+#ifdef CONFIG_OF
 static const struct of_device_id i2c_hid_of_match[] = {
 	{ .compatible = "hid-over-i2c" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
+#endif
 
 static const struct i2c_device_id i2c_hid_of_id_table[] = {
 	{ "hid", 0 },
-- 
2.39.1

[PATCH 3/6] HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of

[Hans de Goede]

Add reset GPIO support to the generic i2c-hid-of driver

This is necessary to make the Wacom digitizer on the Lenovo Yoga Book 1
(yb1-x90f/l) work and this will also allow consolidating the 2 specialized
i2c-hid-of-elan.c and i2c-hid-of-goodix.c drivers into the generic
i2c-hid-of driver.

For now the new "post-reset-deassert-delay-ms" property is only used on
x86/ACPI (non devicetree) devs. IOW it is not used in actual devicetree
files and the same goes for the reset GPIO. The devicetree-bindings
maintainers have requested properties like these to not be added to
the devicetree-bindings, so the new property + GPIO are deliberately
not added to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 385f7460e03c..d0289afa57dc 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -21,6 +21,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/hid.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
@@ -35,8 +36,10 @@ struct i2c_hid_of {
 	struct i2chid_ops ops;
 
 	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
 	struct regulator_bulk_data supplies[2];
 	int post_power_delay_ms;
+	int post_reset_delay_ms;
 };
 
 static int i2c_hid_of_power_up(struct i2chid_ops *ops)
@@ -55,6 +58,10 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
 	if (ihid_of->post_power_delay_ms)
 		msleep(ihid_of->post_power_delay_ms);
 
+	gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
+	if (ihid_of->post_reset_delay_ms)
+		msleep(ihid_of->post_reset_delay_ms);
+
 	return 0;
 }
 
@@ -62,6 +69,7 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
 {
 	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
 
+	gpiod_set_value_cansleep(ihid_of->reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(ihid_of->supplies),
 			       ihid_of->supplies);
 }
@@ -96,6 +104,14 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
 		ihid_of->post_power_delay_ms = val;
 
+	if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
+		ihid_of->post_reset_delay_ms = val;
+
+	/* Start out with reset asserted */
+	ihid_of->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid_of->reset_gpio))
+		return PTR_ERR(ihid_of->reset_gpio);
+
 	ihid_of->supplies[0].supply = "vdd";
 	ihid_of->supplies[1].supply = "vddl";
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
-- 
2.39.1

[PATCH 4/6] HID: i2c-hid-of: Add chip_data struct

[Hans de Goede]

Add a chip_data struct which can be used to set different supply-names
and default power-on-delay, reset-deassert-delay and hid_descriptor_address
values.

This is a preparation patch for consolidating the 2 specialized
i2c-hid-of-elan.c and i2c-hid-of-goodix.c drivers into the generic
i2c-hid-of driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 67 +++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index d0289afa57dc..4fafef1e36b9 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -32,24 +32,44 @@
 
 #include "i2c-hid.h"
 
+#define MAX_BULK_SUPPLIES	2
+
+struct i2c_hid_of_chip_data {
+	const char * const *supply_names;
+	int num_supplies;
+	int post_power_delay_ms;
+	int post_reset_delay_ms;
+	u16 hid_descriptor_address;
+};
+
 struct i2c_hid_of {
 	struct i2chid_ops ops;
 
 	struct i2c_client *client;
 	struct gpio_desc *reset_gpio;
-	struct regulator_bulk_data supplies[2];
+	struct regulator_bulk_data supplies[MAX_BULK_SUPPLIES];
+	int num_supplies;
 	int post_power_delay_ms;
 	int post_reset_delay_ms;
 };
 
+static const char * const i2c_hid_of_default_supply_names[] = {
+	"vdd",
+	"vddl",
+};
+
+static const struct i2c_hid_of_chip_data i2c_hid_of_default_chip_data = {
+	.supply_names = i2c_hid_of_default_supply_names,
+	.num_supplies = ARRAY_SIZE(i2c_hid_of_default_supply_names),
+};
+
 static int i2c_hid_of_power_up(struct i2chid_ops *ops)
 {
 	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
 	struct device *dev = &ihid_of->client->dev;
 	int ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
-				    ihid_of->supplies);
+	ret = regulator_bulk_enable(ihid_of->num_supplies, ihid_of->supplies);
 	if (ret) {
 		dev_warn(dev, "Failed to enable supplies: %d\n", ret);
 		return ret;
@@ -70,18 +90,28 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
 	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
 
 	gpiod_set_value_cansleep(ihid_of->reset_gpio, 1);
-	regulator_bulk_disable(ARRAY_SIZE(ihid_of->supplies),
-			       ihid_of->supplies);
+	regulator_bulk_disable(ihid_of->num_supplies, ihid_of->supplies);
 }
 
 static int i2c_hid_of_probe(struct i2c_client *client)
 {
+	const struct i2c_hid_of_chip_data *chip_data;
 	struct device *dev = &client->dev;
 	struct i2c_hid_of *ihid_of;
 	u16 hid_descriptor_address;
 	u32 quirks = 0;
 	int ret;
 	u32 val;
+	int i;
+
+	chip_data = device_get_match_data(dev);
+	if (!chip_data)
+		chip_data = &i2c_hid_of_default_chip_data;
+
+	if (chip_data->num_supplies > MAX_BULK_SUPPLIES) {
+		dev_err(dev, "Error chip_data->num_supplies > MAX_BULK_SUPPLIES (internal kernel error)\n");
+		return -EINVAL;
+	}
 
 	ihid_of = devm_kzalloc(dev, sizeof(*ihid_of), GFP_KERNEL);
 	if (!ihid_of)
@@ -90,16 +120,22 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	ihid_of->ops.power_up = i2c_hid_of_power_up;
 	ihid_of->ops.power_down = i2c_hid_of_power_down;
 
+	/* chip_data as defaults, allow device-properties to override things */
+	ihid_of->post_power_delay_ms = chip_data->post_power_delay_ms;
+	ihid_of->post_reset_delay_ms = chip_data->post_reset_delay_ms;
+	hid_descriptor_address = chip_data->hid_descriptor_address;
+
 	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
-	if (ret) {
+	if (ret == 0) {
+		if (val >> 16) {
+			dev_err(dev, "Bad HID register address: 0x%08x\n", val);
+			return -EINVAL;
+		}
+		hid_descriptor_address = val;
+	} else if (!hid_descriptor_address) {
 		dev_err(dev, "HID register address not provided\n");
 		return -ENODEV;
 	}
-	if (val >> 16) {
-		dev_err(dev, "Bad HID register address: 0x%08x\n", val);
-		return -EINVAL;
-	}
-	hid_descriptor_address = val;
 
 	if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
 		ihid_of->post_power_delay_ms = val;
@@ -112,10 +148,11 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	if (IS_ERR(ihid_of->reset_gpio))
 		return PTR_ERR(ihid_of->reset_gpio);
 
-	ihid_of->supplies[0].supply = "vdd";
-	ihid_of->supplies[1].supply = "vddl";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
-				      ihid_of->supplies);
+	ihid_of->num_supplies = chip_data->num_supplies;
+	for (i = 0; i < ihid_of->num_supplies; i++)
+		ihid_of->supplies[i].supply = chip_data->supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ihid_of->num_supplies, ihid_of->supplies);
 	if (ret)
 		return ret;
 
-- 
2.39.1

[PATCH 5/6] HID: i2c-hid-of: Consolidate Elan support into generic i2c-hid-of driver

[Hans de Goede]

The generic i2c-hid-of driver nowsupports a reset GPIO and allows
setting the supply-names, hid_descriptor_address and delays through
match_data / chip_data.

This means that i2c-hid-of can be easily made to support
the elan,ekth6915 compatible directly and the i2c-hid-of-elan driver
can be dropped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/Kconfig           |  15 ---
 drivers/hid/i2c-hid/Makefile          |   1 -
 drivers/hid/i2c-hid/i2c-hid-of-elan.c | 129 --------------------------
 drivers/hid/i2c-hid/i2c-hid-of.c      |  14 +++
 4 files changed, 14 insertions(+), 145 deletions(-)
 delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-elan.c

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 3be17109301a..6aa7cf18fd2d 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -38,21 +38,6 @@ config I2C_HID_OF
 	  will be called i2c-hid-of.  It will also build/depend on the
 	  module i2c-hid.
 
-config I2C_HID_OF_ELAN
-	tristate "Driver for Elan hid-i2c based devices on OF systems"
-	depends on OF
-	select I2C_HID_CORE
-	help
-	  Say Y here if you want support for Elan 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-elan.  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"
 	depends on OF
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 55bd5e0f35af..302545a771f3 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -10,5 +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_ELAN)			+= i2c-hid-of-elan.o
 obj-$(CONFIG_I2C_HID_OF_GOODIX)			+= i2c-hid-of-goodix.o
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
deleted file mode 100644
index 76ddc8be1cbb..000000000000
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ /dev/null
@@ -1,129 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Driver for Elan 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 elan_i2c_hid_chip_data {
-	unsigned int post_gpio_reset_delay_ms;
-	unsigned int post_power_delay_ms;
-	u16 hid_descriptor_address;
-};
-
-struct i2c_hid_of_elan {
-	struct i2chid_ops ops;
-
-	struct regulator *vcc33;
-	struct regulator *vccio;
-	struct gpio_desc *reset_gpio;
-	const struct elan_i2c_hid_chip_data *chip_data;
-};
-
-static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
-{
-	struct i2c_hid_of_elan *ihid_elan =
-		container_of(ops, struct i2c_hid_of_elan, ops);
-	int ret;
-
-	ret = regulator_enable(ihid_elan->vcc33);
-	if (ret)
-		return ret;
-
-	ret = regulator_enable(ihid_elan->vccio);
-	if (ret) {
-		regulator_disable(ihid_elan->vcc33);
-		return ret;
-	}
-
-	if (ihid_elan->chip_data->post_power_delay_ms)
-		msleep(ihid_elan->chip_data->post_power_delay_ms);
-
-	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
-	if (ihid_elan->chip_data->post_gpio_reset_delay_ms)
-		msleep(ihid_elan->chip_data->post_gpio_reset_delay_ms);
-
-	return 0;
-}
-
-static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
-{
-	struct i2c_hid_of_elan *ihid_elan =
-		container_of(ops, struct i2c_hid_of_elan, ops);
-
-	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
-	regulator_disable(ihid_elan->vccio);
-	regulator_disable(ihid_elan->vcc33);
-}
-
-static int i2c_hid_of_elan_probe(struct i2c_client *client)
-{
-	struct i2c_hid_of_elan *ihid_elan;
-
-	ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL);
-	if (!ihid_elan)
-		return -ENOMEM;
-
-	ihid_elan->ops.power_up = elan_i2c_hid_power_up;
-	ihid_elan->ops.power_down = elan_i2c_hid_power_down;
-
-	/* Start out with reset asserted */
-	ihid_elan->reset_gpio =
-		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ihid_elan->reset_gpio))
-		return PTR_ERR(ihid_elan->reset_gpio);
-
-	ihid_elan->vccio = devm_regulator_get(&client->dev, "vccio");
-	if (IS_ERR(ihid_elan->vccio))
-		return PTR_ERR(ihid_elan->vccio);
-
-	ihid_elan->vcc33 = devm_regulator_get(&client->dev, "vcc33");
-	if (IS_ERR(ihid_elan->vcc33))
-		return PTR_ERR(ihid_elan->vcc33);
-
-	ihid_elan->chip_data = device_get_match_data(&client->dev);
-
-	return i2c_hid_core_probe(client, &ihid_elan->ops,
-				  ihid_elan->chip_data->hid_descriptor_address, 0);
-}
-
-static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
-	.post_power_delay_ms = 1,
-	.post_gpio_reset_delay_ms = 300,
-	.hid_descriptor_address = 0x0001,
-};
-
-static const struct of_device_id elan_i2c_hid_of_match[] = {
-	{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match);
-
-static struct i2c_driver elan_i2c_hid_ts_driver = {
-	.driver = {
-		.name	= "i2c_hid_of_elan",
-		.pm	= &i2c_hid_core_pm,
-		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
-		.of_match_table = of_match_ptr(elan_i2c_hid_of_match),
-	},
-	.probe_new	= i2c_hid_of_elan_probe,
-	.remove		= i2c_hid_core_remove,
-	.shutdown	= i2c_hid_core_shutdown,
-};
-module_i2c_driver(elan_i2c_hid_ts_driver);
-
-MODULE_AUTHOR("Douglas Anderson <dianders@chromium.org>");
-MODULE_DESCRIPTION("Elan i2c-hid touchscreen driver");
-MODULE_LICENSE("GPL");
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 4fafef1e36b9..313eb198d840 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -167,8 +167,22 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 }
 
 #ifdef CONFIG_OF
+static const char * const elan_ekth6915_supply_names[] = {
+	"vcc33",
+	"vccio",
+};
+
+static const struct i2c_hid_of_chip_data elan_ekth6915_chip_data = {
+	.supply_names = elan_ekth6915_supply_names,
+	.num_supplies = ARRAY_SIZE(elan_ekth6915_supply_names),
+	.post_power_delay_ms = 1,
+	.post_reset_delay_ms = 300,
+	.hid_descriptor_address = 0x0001,
+};
+
 static const struct of_device_id i2c_hid_of_match[] = {
 	{ .compatible = "hid-over-i2c" },
+	{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
-- 
2.39.1

[PATCH 6/6] HID: i2c-hid-of: Consolidate Goodix support into generic i2c-hid-of driver

[Hans de Goede]

The generic i2c-hid-of driver nowsupports a reset GPIO and allows
setting the supply-names, hid_descriptor_address and delays through
match_data / chip_data.

This means that i2c-hid-of can be easily made to support
the goodix,gt7375p compatible directly and the i2c-hid-of-goodix
driver can be dropped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/Kconfig             |  15 ---
 drivers/hid/i2c-hid/Makefile            |   1 -
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 125 ------------------------
 drivers/hid/i2c-hid/i2c-hid-of.c        |  14 +++
 4 files changed, 14 insertions(+), 141 deletions(-)
 delete 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 6aa7cf18fd2d..10e4b368b818 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -38,21 +38,6 @@ 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"
-	depends on OF
-	select I2C_HID_CORE
-	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.
-
 config I2C_HID_CORE
 	tristate
 endif
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 302545a771f3..9b4a73446841 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -10,4 +10,3 @@ 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
deleted file mode 100644
index 0060e3dcd775..000000000000
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ /dev/null
@@ -1,125 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Driver for Goodix touchscreens that use the i2c-hid protocol.
- *
- * Copyright 2020 Google LLC
- */
-
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/gpio/consumer.h>
-#include <linux/i2c.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/pm.h>
-#include <linux/regulator/consumer.h>
-
-#include "i2c-hid.h"
-
-struct goodix_i2c_hid_timing_data {
-	unsigned int post_gpio_reset_delay_ms;
-	unsigned int post_power_delay_ms;
-};
-
-struct i2c_hid_of_goodix {
-	struct i2chid_ops ops;
-
-	struct regulator *vdd;
-	struct regulator *vddio;
-	struct gpio_desc *reset_gpio;
-	const struct goodix_i2c_hid_timing_data *timings;
-};
-
-static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
-{
-	struct i2c_hid_of_goodix *ihid_goodix =
-		container_of(ops, struct i2c_hid_of_goodix, ops);
-	int ret;
-
-	ret = regulator_enable(ihid_goodix->vdd);
-	if (ret)
-		return ret;
-
-	ret = regulator_enable(ihid_goodix->vddio);
-	if (ret)
-		return ret;
-
-	if (ihid_goodix->timings->post_power_delay_ms)
-		msleep(ihid_goodix->timings->post_power_delay_ms);
-
-	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
-	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
-		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
-
-	return 0;
-}
-
-static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
-{
-	struct i2c_hid_of_goodix *ihid_goodix =
-		container_of(ops, struct i2c_hid_of_goodix, ops);
-
-	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
-	regulator_disable(ihid_goodix->vddio);
-	regulator_disable(ihid_goodix->vdd);
-}
-
-static int i2c_hid_of_goodix_probe(struct i2c_client *client)
-{
-	struct i2c_hid_of_goodix *ihid_goodix;
-
-	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
-				   GFP_KERNEL);
-	if (!ihid_goodix)
-		return -ENOMEM;
-
-	ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
-	ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
-
-	/* Start out with reset asserted */
-	ihid_goodix->reset_gpio =
-		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ihid_goodix->reset_gpio))
-		return PTR_ERR(ihid_goodix->reset_gpio);
-
-	ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR(ihid_goodix->vdd))
-		return PTR_ERR(ihid_goodix->vdd);
-
-	ihid_goodix->vddio = devm_regulator_get(&client->dev, "mainboard-vddio");
-	if (IS_ERR(ihid_goodix->vddio))
-		return PTR_ERR(ihid_goodix->vddio);
-
-	ihid_goodix->timings = device_get_match_data(&client->dev);
-
-	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
-}
-
-static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
-	.post_power_delay_ms = 10,
-	.post_gpio_reset_delay_ms = 180,
-};
-
-static const struct of_device_id goodix_i2c_hid_of_match[] = {
-	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
-
-static struct i2c_driver goodix_i2c_hid_ts_driver = {
-	.driver = {
-		.name	= "i2c_hid_of_goodix",
-		.pm	= &i2c_hid_core_pm,
-		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
-		.of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
-	},
-	.probe_new	= 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");
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 313eb198d840..7f298210447a 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -180,9 +180,23 @@ static const struct i2c_hid_of_chip_data elan_ekth6915_chip_data = {
 	.hid_descriptor_address = 0x0001,
 };
 
+static const char * const goodix_gt7375p_supply_names[] = {
+	"vdd",
+	"mainboard-vddio",
+};
+
+static const struct i2c_hid_of_chip_data goodix_gt7375p_chip_data = {
+	.supply_names = goodix_gt7375p_supply_names,
+	.num_supplies = ARRAY_SIZE(goodix_gt7375p_supply_names),
+	.post_power_delay_ms = 10,
+	.post_reset_delay_ms = 180,
+	.hid_descriptor_address = 0x0001,
+};
+
 static const struct of_device_id i2c_hid_of_match[] = {
 	{ .compatible = "hid-over-i2c" },
 	{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
+	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_chip_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
-- 
2.39.1

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Benjamin Tissoires]

Hi Hans,

On Apr 09 2023, Hans de Goede wrote:
> Hi All,
> 
> This series consist of 2 parts:
> 
> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
>    devices which are not enumerated by ACPI to work on ACPI platforms
>    (by manual i2c_client instantiation using i2c_client_id matching).

Patches 1 and 2 are looking good, but I wonder if you can not achieve the
same result by relying on an ACPI SSDT override. I got something similar
working on this thread[0].

I understand the "post-reset-deassert-delay-ms" might be something hard
to express with an SSDT, but we should already have all the bits in
place, no?

Also, the problem of "post-reset-deassert-delay-ms" is that you are not
documenting it, because the OF folks do not want this in device tree,
and I tend to agree with them. So this basically creates a brand new
undocumented property, which is less than ideal.

> 
> 2. Patches 4-6. Remove the special i2c-hid-of-elan and i2c-hid-of-goodix
>    driver, folding the functionality into the generic i2c-hid-of driver.
>    Since 1. requires adding reset-gpio support to i2c-hid-of there was
>    very little difference left between the generic i2c-hid-of code and
>    the specialized drivers. So I decided to merge them into the generic
>    driver instead of having duplicate code.

I understand the spirit, but I am not a big fan of this. The reason is
just detailed your following statements: getting tests on those is hard.

So there is code duplication, yes, but OTOH this guarantees that we do
not break those devices while working on something else.

I can always be convinced otherwise, but I still think the approach of
the devicetree-bindings maintainers works better: if you need a new
property that isn't available in the core of i2c-hid-of, and which is
device specific (even if it's just a msleep for a line to be ready),
make this a separate driver. Trying to parametrize everything with
properties will just end up in a situation where one "meaningless"
property will break another device, and it's going to be a pain to
trace, because those drivers are not tested every single kernel release.

> 
> Note patches 4-6 have not been actually tested with an "elan,ekth6915"
> touchscreen nor with a "goodix,gt7375p" touchscreen.
> 
> Douglas, can you perhaps test this patch-set with an "elan,ekth6915"
> touchscreen and with a "goodix,gt7375p" touchscreen ?
> 
> Regards,
> 
> Hans
> 

Cheers,
Benjamin


[0] https://lore.kernel.org/linux-input/20230308155527.jnrsowubvnk22ica@mail.corp.redhat.com/

> 
> Hans de Goede (6):
>   HID: i2c-hid-of: Consistenly use dev local variable in probe()
>   HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
>   HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
>   HID: i2c-hid-of: Add chip_data struct
>   HID: i2c-hid-of: Consolidate Elan support into generic i2c-hid-of
>     driver
>   HID: i2c-hid-of: Consolidate Goodix support into generic i2c-hid-of
>     driver
> 
>  drivers/hid/i2c-hid/Kconfig             |  36 +------
>  drivers/hid/i2c-hid/Makefile            |   2 -
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c   | 129 ------------------------
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 125 -----------------------
>  drivers/hid/i2c-hid/i2c-hid-of.c        | 124 +++++++++++++++++++----
>  5 files changed, 106 insertions(+), 310 deletions(-)
>  delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-elan.c
>  delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> 
> -- 
> 2.39.1
> 

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Hans de Goede]

Hi Benjamin,

On 4/11/23 11:02, Benjamin Tissoires wrote:
> Hi Hans,
> 
> On Apr 09 2023, Hans de Goede wrote:
>> Hi All,
>>
>> This series consist of 2 parts:
>>
>> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
>>    devices which are not enumerated by ACPI to work on ACPI platforms
>>    (by manual i2c_client instantiation using i2c_client_id matching).
> 
> Patches 1 and 2 are looking good, but I wonder if you can not achieve the
> same result by relying on an ACPI SSDT override. I got something similar
> working on this thread[0].

Yes this could be made to work with an ACPI override. But the goal is
to make things work OOTB for end users when they install Linux and
ACPI overrides are very far from something which works OOTB.

> I understand the "post-reset-deassert-delay-ms" might be something hard
> to express with an SSDT, but we should already have all the bits in
> place, no?

Actually if an ACPI override is used then the setting of the GPIO
can be done in _PS0 and _PS3 (power on / off) methods and those
can simply include a sleep after setting the GPIO.

> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> documenting it, because the OF folks do not want this in device tree,
> and I tend to agree with them. So this basically creates a brand new
> undocumented property, which is less than ideal.

I'm merely not documenting it because there are no devicetree users yet.

Between the 2 currently supported of devices with a reset GPIO +
the I2C-HID capable touchscreen + wacom digitizer on the x86 android
Yoga Book 1 I'm trying to get to work that is 4 I2C-HID devices which
all follow the pattern of: 1. They have a reset GPIO, 2. they need
some delay after reset is deasserted.

It seems silly to keep adding more and more device-ids + match-data
with just the delays in there when it seems that many many I2C-HID
capable controllers/chips follow this pattern.

Also note that there already is a very similar "post-power-on-delay-ms"
property. I really don't see what makes specifying a delay after
enabling regulators through a property ok, but specifying the delay
after reset-deassert not ok. Allowing one but not the other is not
very consistent.

The reason why I'm not documenting the property now is because of
lack of current devicetree users. It can be documented once
the first DT users show up and getting it accepted should really not
be an issue given that "post-power-on-delay-ms" already exists.

Note if just the existence of the property is the main stumbling
block I can go the match_data route for the wacom digitizer on
the Yoga Book 1 too and add an extra i2c_device_id with match-data
setting the delay. This could then either be its own specialized
driver, or we could still go with the current patch-set
(minus the property) and add an i2c_device_id with match-data
to i2c-hid-of.c .

The only question then is how to name the i2c_device_id for the wacom
digitizer. It has a vid:pid of 056A:0169 So maybe "wacom0169" ?


>> 2. Patches 4-6. Remove the special i2c-hid-of-elan and i2c-hid-of-goodix
>>    driver, folding the functionality into the generic i2c-hid-of driver.
>>    Since 1. requires adding reset-gpio support to i2c-hid-of there was
>>    very little difference left between the generic i2c-hid-of code and
>>    the specialized drivers. So I decided to merge them into the generic
>>    driver instead of having duplicate code.
> 
> I understand the spirit, but I am not a big fan of this. The reason is
> just detailed your following statements: getting tests on those is hard.

Actually AFAIK the chromeos folks have an automated test lab where
all supported models get tested and they regularly test the latest
mainline kernels. So even without me asking for it any regressions
here should have been caught in this case since support for both
special-case i2c-hid-of drivers was added for chromeos.

And the code is almost identical, the only difference is using
the bulk-regulator API vs enabling the regulators 1 by 1, which
should not make any difference.

> So there is code duplication, yes, but OTOH this guarantees that we do
> not break those devices while working on something else.
> 
> I can always be convinced otherwise, but I still think the approach of
> the devicetree-bindings maintainers works better: if you need a new
> property that isn't available in the core of i2c-hid-of, and which is
> device specific (even if it's just a msleep for a line to be ready),
> make this a separate driver. Trying to parametrize everything with
> properties will just end up in a situation where one "meaningless"
> property will break another device, and it's going to be a pain to
> trace, because those drivers are not tested every single kernel release.

This is not trying to parametrize everything, this is trying to
parametrize something which turns out to be necessary over 4 different
chips/controller models. And I'm pretty sure that if I start looking
into ACPI tables I will find many more controllers which use a reset
GPIO + a delay after de-assert like this.

IOW something which is clearly a very common pattern.

You have been advocating to make HID code more generic allowing
device-quirks in BPF format to avoid adding drivers for every
tiny descriptor fixup.

Do you really want to go the route of a tiny driver for every
i2c-hid chip variant used with devicetree, rather then having
a single extra property ?

Note that if patches 1-3 had been in place when Douglas
started adding support for the "elan,ekth6915" and
"goodix,gt7375p" devices that the devicetree on
the chromeos devices using those would then likely
have simply used the "hid-descr-addr", "post-power-on-delay-ms"
and "post-reset-deassert-delay-ms" properties and no
separate drivers would have been necessary at all.

(We need patches 4-6 now only to keep compatibility with
 existing devicetree files which don't set these)

So I really see patches 4-6 as a way to reduce future
work reviewing specialized drivers for you and Jiri.

Yes there may still be some special cases in the future
which need a specialized driver which we have now, but
I believe that covering the common reset-GPIO pattern
will drastically reduce the need for those drivers and
thus will lower the maintainer burden.

Regards,

Hans

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Benjamin Tissoires]

On Apr 11 2023, Hans de Goede wrote:
> Hi Benjamin,
> 
> On 4/11/23 11:02, Benjamin Tissoires wrote:
> > Hi Hans,
> > 
> > On Apr 09 2023, Hans de Goede wrote:
> >> Hi All,
> >>
> >> This series consist of 2 parts:
> >>
> >> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
> >>    devices which are not enumerated by ACPI to work on ACPI platforms
> >>    (by manual i2c_client instantiation using i2c_client_id matching).
> > 
> > Patches 1 and 2 are looking good, but I wonder if you can not achieve the
> > same result by relying on an ACPI SSDT override. I got something similar
> > working on this thread[0].
> 
> Yes this could be made to work with an ACPI override. But the goal is
> to make things work OOTB for end users when they install Linux and
> ACPI overrides are very far from something which works OOTB.

Fair enough.

> 
> > I understand the "post-reset-deassert-delay-ms" might be something hard
> > to express with an SSDT, but we should already have all the bits in
> > place, no?
> 
> Actually if an ACPI override is used then the setting of the GPIO
> can be done in _PS0 and _PS3 (power on / off) methods and those
> can simply include a sleep after setting the GPIO.

Though this is all conditional if we can make ACPI SSDT override
something that can be shipped while installing the device...

> 
> > Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> > documenting it, because the OF folks do not want this in device tree,
> > and I tend to agree with them. So this basically creates a brand new
> > undocumented property, which is less than ideal.
> 
> I'm merely not documenting it because there are no devicetree users yet.

AFAIU, the non devicetree properties should also be documented through
DT bindings, no? So not documenting feels very much like "I want to slip
this one in without having to deal with DT maintainers" (and before you
take it personaly, I know this is definitively not the intent). So I'd
rather much having a public API documented, even if there are no users.

> 
> Between the 2 currently supported of devices with a reset GPIO +
> the I2C-HID capable touchscreen + wacom digitizer on the x86 android
> Yoga Book 1 I'm trying to get to work that is 4 I2C-HID devices which
> all follow the pattern of: 1. They have a reset GPIO, 2. they need
> some delay after reset is deasserted.
> 
> It seems silly to keep adding more and more device-ids + match-data
> with just the delays in there when it seems that many many I2C-HID
> capable controllers/chips follow this pattern.

The problem, AFAICT, is that I2C-HID is only described through ACPI, and
a lot of the HW specific is let to ACPI. So i2c-hid-of, is basically OEM
bringing components together on a DT platform, and hoping for the best.
This works well in the way that we don't need to add a new driver for
it, but they can not easily describe what they need (or even fail like
your tablet which is supposed to be working thorugh ACPI).

So if we had one big competitor, like Google for Chromebooks, who just
said: "this is what you need for OF i2c-hid devices, and you can not
rely on anything else", life would be much simpler.

> 
> Also note that there already is a very similar "post-power-on-delay-ms"
> property. I really don't see what makes specifying a delay after
> enabling regulators through a property ok, but specifying the delay
> after reset-deassert not ok. Allowing one but not the other is not
> very consistent.

Agree :/

> 
> The reason why I'm not documenting the property now is because of
> lack of current devicetree users. It can be documented once
> the first DT users show up and getting it accepted should really not
> be an issue given that "post-power-on-delay-ms" already exists.

Honestly this is not a good way of doing thing IMO. This is basically
what I did with i2c-hid, and it pissed the DT maintainers. (I did it
because at the time the maintainers were not reactive, and/or the ML
were not correctly set IIRC).

> 
> Note if just the existence of the property is the main stumbling
> block I can go the match_data route for the wacom digitizer on
> the Yoga Book 1 too and add an extra i2c_device_id with match-data
> setting the delay. This could then either be its own specialized
> driver, or we could still go with the current patch-set
> (minus the property) and add an i2c_device_id with match-data
> to i2c-hid-of.c .

I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a
function call, a callback or a match-data (or a driver-data), this is
something we are in control and we can change. A blind undocumented
property is going to be a pain if we get to change it.

(unless of course you can get Rob's ack on the preperty itself).

> 
> The only question then is how to name the i2c_device_id for the wacom
> digitizer. It has a vid:pid of 056A:0169 So maybe "wacom0169" ?

Seems reasonable to me :)

[few minutes later]

Well, maybe we don't want the PID to be used here, because we will end
up having to quirk every single device. But OTOH I do not see a
different solution...


> 
> 
> >> 2. Patches 4-6. Remove the special i2c-hid-of-elan and i2c-hid-of-goodix
> >>    driver, folding the functionality into the generic i2c-hid-of driver.
> >>    Since 1. requires adding reset-gpio support to i2c-hid-of there was
> >>    very little difference left between the generic i2c-hid-of code and
> >>    the specialized drivers. So I decided to merge them into the generic
> >>    driver instead of having duplicate code.
> > 
> > I understand the spirit, but I am not a big fan of this. The reason is
> > just detailed your following statements: getting tests on those is hard.
> 
> Actually AFAIK the chromeos folks have an automated test lab where
> all supported models get tested and they regularly test the latest
> mainline kernels. So even without me asking for it any regressions
> here should have been caught in this case since support for both
> special-case i2c-hid-of drivers was added for chromeos.
> 
> And the code is almost identical, the only difference is using
> the bulk-regulator API vs enabling the regulators 1 by 1, which
> should not make any difference.

Well, it's nice to know regressions, but it's nicer to know them before
we introduce them in linux-next :)

My point is if you don't manage to get tests on those devices, and we
can "guarantee" that the changes in i2c-hid-of.c will be a noop for
them, why bother merging them together? If the files have dedicated
maintainers, we should probably rely on them instead :)

> 
> > So there is code duplication, yes, but OTOH this guarantees that we do
> > not break those devices while working on something else.
> > 
> > I can always be convinced otherwise, but I still think the approach of
> > the devicetree-bindings maintainers works better: if you need a new
> > property that isn't available in the core of i2c-hid-of, and which is
> > device specific (even if it's just a msleep for a line to be ready),
> > make this a separate driver. Trying to parametrize everything with
> > properties will just end up in a situation where one "meaningless"
> > property will break another device, and it's going to be a pain to
> > trace, because those drivers are not tested every single kernel release.
> 
> This is not trying to parametrize everything, this is trying to
> parametrize something which turns out to be necessary over 4 different
> chips/controller models. And I'm pretty sure that if I start looking
> into ACPI tables I will find many more controllers which use a reset
> GPIO + a delay after de-assert like this.
> 
> IOW something which is clearly a very common pattern.
> 
> You have been advocating to make HID code more generic allowing
> device-quirks in BPF format to avoid adding drivers for every
> tiny descriptor fixup.

Hehe, fair enough :) But my problem here is more who is responsible for
this code, and merging them together means more responsibility to the
i2c-hid-of.c maintainer. Having separate self-contained drivers for
handling device subtleties (when they are not generic) allows to not
break one device when fixing one other.

> 
> Do you really want to go the route of a tiny driver for every
> i2c-hid chip variant used with devicetree, rather then having
> a single extra property ?

I'd like that property to be validated by Rob first. You raised the
inconsistency above, and I'd rather have an ack from him first.

Having a "this is how every i2c-hid device works" kind of argument might
make it enough for him to change his opinion (because I think that was
the argument for the post power delay).

> 
> Note that if patches 1-3 had been in place when Douglas
> started adding support for the "elan,ekth6915" and
> "goodix,gt7375p" devices that the devicetree on
> the chromeos devices using those would then likely
> have simply used the "hid-descr-addr", "post-power-on-delay-ms"
> and "post-reset-deassert-delay-ms" properties and no
> separate drivers would have been necessary at all.

I think that's how the whole separate driver started. And the argument
of having separate drivers still stands, for devices that are not doing
the same things like others.

The compatible allows to store a set of device specific data that will
not change whatever board the device is placed in. So though technically
easier for the maintainers, having a dedicated property is putting the
burden on the user. While OTOH, if this property is internal API, we can
have a table with it, that says that Goodix needs X ms, when Elan Y and
Wacom Z ms.

But then, you're going to say that this requires a kernel bump, when a
device property is just on the board side, so much convenient from an
OEM point of view :(

> 
> (We need patches 4-6 now only to keep compatibility with
>  existing devicetree files which don't set these)
> 
> So I really see patches 4-6 as a way to reduce future
> work reviewing specialized drivers for you and Jiri.

And I thank you for that :)

> 
> Yes there may still be some special cases in the future
> which need a specialized driver which we have now, but
> I believe that covering the common reset-GPIO pattern
> will drastically reduce the need for those drivers and
> thus will lower the maintainer burden.

Just a small thinking here: if we keep the current compatible drivers
here, we have an example we can point people at if they need fancier
things. So maybe keeping them (or one at least) is a good thing, no?

Cheers,
Benjamin

> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Hans de Goede]

Hi Benjamin,

On 4/11/23 14:50, Benjamin Tissoires wrote:
> On Apr 11 2023, Hans de Goede wrote:
>> Hi Benjamin,
>>
>> On 4/11/23 11:02, Benjamin Tissoires wrote:
>>> Hi Hans,
>>>
>>> On Apr 09 2023, Hans de Goede wrote:
>>>> Hi All,
>>>>
>>>> This series consist of 2 parts:
>>>>
>>>> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
>>>>    devices which are not enumerated by ACPI to work on ACPI platforms
>>>>    (by manual i2c_client instantiation using i2c_client_id matching).
>>>
>>> Patches 1 and 2 are looking good, but I wonder if you can not achieve the
>>> same result by relying on an ACPI SSDT override. I got something similar
>>> working on this thread[0].
>>
>> Yes this could be made to work with an ACPI override. But the goal is
>> to make things work OOTB for end users when they install Linux and
>> ACPI overrides are very far from something which works OOTB.
> 
> Fair enough.
> 
>>
>>> I understand the "post-reset-deassert-delay-ms" might be something hard
>>> to express with an SSDT, but we should already have all the bits in
>>> place, no?
>>
>> Actually if an ACPI override is used then the setting of the GPIO
>> can be done in _PS0 and _PS3 (power on / off) methods and those
>> can simply include a sleep after setting the GPIO.
> 
> Though this is all conditional if we can make ACPI SSDT override
> something that can be shipped while installing the device...
> 
>>
>>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
>>> documenting it, because the OF folks do not want this in device tree,
>>> and I tend to agree with them. So this basically creates a brand new
>>> undocumented property, which is less than ideal.
>>
>> I'm merely not documenting it because there are no devicetree users yet.
> 
> AFAIU, the non devicetree properties should also be documented through
> DT bindings, no? So not documenting feels very much like "I want to slip
> this one in without having to deal with DT maintainers" (and before you
> take it personaly, I know this is definitively not the intent). So I'd
> rather much having a public API documented, even if there are no users.

Right, so as a hobby I have a tendency to work on these somewhat niche/weird
x86 devices, like x86 tablets which use Android as factory OS :)

As such I have encountered the need for device-properties to pass info
from drivers/platform/x86 code to more generic drivers a number of
times before.

Each time this happens, if I try to add them to bindings I get
asked for some example devicetree code, I then respond that these
are *device*-properties not of-properties and that there are no
current devicetree users after which the DT maintainers tell me
to then NOT document them in the DT bindings, at least not until
actually DT users show up. I fully expect any attempt do add
this to the DT bindings to go the same way.

And now I have you telling me you really want to see this
documented at the same time as it getting implemented. Which
I fully understand, but does leads to a bit of a catch 22.

Anyways lets just go with the alternative of treating this
similar as the existing specialized drivers, see below.

<snip>

>> Note if just the existence of the property is the main stumbling
>> block I can go the match_data route for the wacom digitizer on
>> the Yoga Book 1 too and add an extra i2c_device_id with match-data
>> setting the delay. This could then either be its own specialized
>> driver, or we could still go with the current patch-set
>> (minus the property) and add an i2c_device_id with match-data
>> to i2c-hid-of.c .
> 
> I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a
> function call, a callback or a match-data (or a driver-data), this is
> something we are in control and we can change.

Ok.

So I see 2 options here:

1. Take the approach from patches 1-4 here, but drop the property and
   use match data on a new "wacom0169" i2c_device_id instead.
   This would also pave the way to merging patches 5 + 6 once tested
   by google to reduce some code duplication. Although you write below
   you would prefer to keep these around as example code for other
   specialized drivers...

2. Add a new specialized i2c-hid-of-wacom driver for this.
   Question: Since this will be using i2c_device_id binding not
   DT/OF binding the -of- in the name is technically incorrect,
   but it would be consistent with the other specialized drivers
   and could be seen as preparation (avoiding a rename/confusion)
   for when any DT enumerated devices which need special handling
   show up (note only relevant if you prefer this approach).

Either way is fine with me really. So you get to chose. If you
let me know which route you prefer, I'll go and prepare either
a v2 of this series, or a whole new patch for the new specialized
driver.

Regards,

Hans

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Benjamin Tissoires]

On Apr 11 2023, Hans de Goede wrote:
> Hi Benjamin,
> 
> On 4/11/23 14:50, Benjamin Tissoires wrote:
> > On Apr 11 2023, Hans de Goede wrote:
> >> Hi Benjamin,
> >>
> >> On 4/11/23 11:02, Benjamin Tissoires wrote:
> >>> Hi Hans,
> >>>
> >>> On Apr 09 2023, Hans de Goede wrote:
> >>>> Hi All,
> >>>>
> >>>> This series consist of 2 parts:
> >>>>
> >>>> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
> >>>>    devices which are not enumerated by ACPI to work on ACPI platforms
> >>>>    (by manual i2c_client instantiation using i2c_client_id matching).
> >>>
> >>> Patches 1 and 2 are looking good, but I wonder if you can not achieve the
> >>> same result by relying on an ACPI SSDT override. I got something similar
> >>> working on this thread[0].
> >>
> >> Yes this could be made to work with an ACPI override. But the goal is
> >> to make things work OOTB for end users when they install Linux and
> >> ACPI overrides are very far from something which works OOTB.
> > 
> > Fair enough.
> > 
> >>
> >>> I understand the "post-reset-deassert-delay-ms" might be something hard
> >>> to express with an SSDT, but we should already have all the bits in
> >>> place, no?
> >>
> >> Actually if an ACPI override is used then the setting of the GPIO
> >> can be done in _PS0 and _PS3 (power on / off) methods and those
> >> can simply include a sleep after setting the GPIO.
> > 
> > Though this is all conditional if we can make ACPI SSDT override
> > something that can be shipped while installing the device...
> > 
> >>
> >>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> >>> documenting it, because the OF folks do not want this in device tree,
> >>> and I tend to agree with them. So this basically creates a brand new
> >>> undocumented property, which is less than ideal.
> >>
> >> I'm merely not documenting it because there are no devicetree users yet.
> > 
> > AFAIU, the non devicetree properties should also be documented through
> > DT bindings, no? So not documenting feels very much like "I want to slip
> > this one in without having to deal with DT maintainers" (and before you
> > take it personaly, I know this is definitively not the intent). So I'd
> > rather much having a public API documented, even if there are no users.
> 
> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
> x86 devices, like x86 tablets which use Android as factory OS :)
> 
> As such I have encountered the need for device-properties to pass info
> from drivers/platform/x86 code to more generic drivers a number of
> times before.
> 
> Each time this happens, if I try to add them to bindings I get
> asked for some example devicetree code, I then respond that these
> are *device*-properties not of-properties and that there are no
> current devicetree users after which the DT maintainers tell me
> to then NOT document them in the DT bindings, at least not until
> actually DT users show up. I fully expect any attempt do add
> this to the DT bindings to go the same way.
> 
> And now I have you telling me you really want to see this
> documented at the same time as it getting implemented. Which
> I fully understand, but does leads to a bit of a catch 22.

Ouch. Sorry for that. Then I guess if the DT maintainers have a tendency
to accept those hidden properties, this is the simplest solution from a
i2c-hid/HID maintainer point of view, no? It's going to be a pain for the
platform driver because you still have to hardcode those properties
somewhere I guess.

> 
> Anyways lets just go with the alternative of treating this
> similar as the existing specialized drivers, see below.
> 
> <snip>
> 
> >> Note if just the existence of the property is the main stumbling
> >> block I can go the match_data route for the wacom digitizer on
> >> the Yoga Book 1 too and add an extra i2c_device_id with match-data
> >> setting the delay. This could then either be its own specialized
> >> driver, or we could still go with the current patch-set
> >> (minus the property) and add an i2c_device_id with match-data
> >> to i2c-hid-of.c .
> > 
> > I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a
> > function call, a callback or a match-data (or a driver-data), this is
> > something we are in control and we can change.
> 
> Ok.
> 
> So I see 2 options here:
> 
> 1. Take the approach from patches 1-4 here, but drop the property and
>    use match data on a new "wacom0169" i2c_device_id instead.
>    This would also pave the way to merging patches 5 + 6 once tested
>    by google to reduce some code duplication. Although you write below
>    you would prefer to keep these around as example code for other
>    specialized drivers...
> 
> 2. Add a new specialized i2c-hid-of-wacom driver for this.
>    Question: Since this will be using i2c_device_id binding not
>    DT/OF binding the -of- in the name is technically incorrect,
>    but it would be consistent with the other specialized drivers
>    and could be seen as preparation (avoiding a rename/confusion)
>    for when any DT enumerated devices which need special handling
>    show up (note only relevant if you prefer this approach).

Well, option 2 is probably too much work for little gain. So I would go
with option 1, but with the following questions:

- a device property is public, so it can be seen as public API, right?
  So should we document it some way (not through DT) so we "guarantee"
  some behavior for it?

If the above is correct, then that means that the device property can
be used, which makes little changes to your series.

But then, why aren't you using that property directly for those 2 other
drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
stub around adding the gpio names and the specific reset? (A plain "this
is completely dumb" answer is fine, just trying to get my head around it).

So, given the above, and your experience with the DT maintainers, I
would go with patches 1-3 + a documentation of the new property, likely
in the header or in kernel docs.

Patches 4-6 either dropped, reworked, or left as they are, and we would
merge them only if the maintainers of those files tested the changes.

And if you prefer storing the post-reset delay in the hid tree, that's
fine too, but I guess you would prefer having less friction by keeping
it in the platform tree.

> 
> Either way is fine with me really. So you get to chose. If you
> let me know which route you prefer, I'll go and prepare either
> a v2 of this series, or a whole new patch for the new specialized
> driver.

Sorry for being a PITA, but having those driver separated allowed to
move forward without having to have a spaghetti plate in i2c-hid, which
was the case before the split (because *everything* was entangled: ACPI,
DT, OF, properties). So that's why I'm trying to understand and
minimize the changes.

Also, before you sending v2 and involving too much, we could try to wait a
few days for Doug to answer, and hear if he has an opinion. But if you
rather send v2 right away, that's your choice obviously :)

Cheers,
Benjamin

> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Hans de Goede]

Hi Benjamin,

On 4/11/23 18:56, Benjamin Tissoires wrote:
> On Apr 11 2023, Hans de Goede wrote:
>> Hi Benjamin,
>>
>> On 4/11/23 14:50, Benjamin Tissoires wrote:
>>> On Apr 11 2023, Hans de Goede wrote:
>>>> Hi Benjamin,
>>>>
>>>> On 4/11/23 11:02, Benjamin Tissoires wrote:

<snip>

>>>>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
>>>>> documenting it, because the OF folks do not want this in device tree,
>>>>> and I tend to agree with them. So this basically creates a brand new
>>>>> undocumented property, which is less than ideal.
>>>>
>>>> I'm merely not documenting it because there are no devicetree users yet.
>>>
>>> AFAIU, the non devicetree properties should also be documented through
>>> DT bindings, no? So not documenting feels very much like "I want to slip
>>> this one in without having to deal with DT maintainers" (and before you
>>> take it personaly, I know this is definitively not the intent). So I'd
>>> rather much having a public API documented, even if there are no users.
>>
>> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
>> x86 devices, like x86 tablets which use Android as factory OS :)
>>
>> As such I have encountered the need for device-properties to pass info
>> from drivers/platform/x86 code to more generic drivers a number of
>> times before.
>>
>> Each time this happens, if I try to add them to bindings I get
>> asked for some example devicetree code, I then respond that these
>> are *device*-properties not of-properties and that there are no
>> current devicetree users after which the DT maintainers tell me
>> to then NOT document them in the DT bindings, at least not until
>> actually DT users show up. I fully expect any attempt do add
>> this to the DT bindings to go the same way.
>>
>> And now I have you telling me you really want to see this
>> documented at the same time as it getting implemented. Which
>> I fully understand, but does leads to a bit of a catch 22.
> 
> Ouch. Sorry for that.

No problem.

> Then I guess if the DT maintainers have a tendency
> to accept those hidden properties, this is the simplest solution from a
> i2c-hid/HID maintainer point of view, no?

Yes, I believe so, which is why I went this route in the first place.

> It's going to be a pain for the
> platform driver because you still have to hardcode those properties
> somewhere I guess.

Since the entire description is missing in ACPI for the digitizer (*)
the x86-android-tablets.ko module which contains glue code to support
these x86 android tablets already contains the i2c-busnumber,
i2c-address, GPIO lookups, IRQ and other necessary device-properties
like "hid-descr-addr", so adding one more device-property is very little
trouble.

*) and also for other devices both on this and other x86 android tablets

<snip>

>> So I see 2 options here:
>>
>> 1. Take the approach from patches 1-4 here, but drop the property and
>>    use match data on a new "wacom0169" i2c_device_id instead.
>>    This would also pave the way to merging patches 5 + 6 once tested
>>    by google to reduce some code duplication. Although you write below
>>    you would prefer to keep these around as example code for other
>>    specialized drivers...
>>
>> 2. Add a new specialized i2c-hid-of-wacom driver for this.
>>    Question: Since this will be using i2c_device_id binding not
>>    DT/OF binding the -of- in the name is technically incorrect,
>>    but it would be consistent with the other specialized drivers
>>    and could be seen as preparation (avoiding a rename/confusion)
>>    for when any DT enumerated devices which need special handling
>>    show up (note only relevant if you prefer this approach).
> 
> Well, option 2 is probably too much work for little gain. So I would go
> with option 1, but with the following questions:
> 
> - a device property is public, so it can be seen as public API, right?
>   So should we document it some way (not through DT) so we "guarantee"
>   some behavior for it?

I believe the whole idea from the DT maintainers behind not documenting
it as DT binding when not actually used in dts files is to keep it as
in kernel *private* API, in this case between the x86-android-tablets.ko
code instantiating the i2c_client and the i2c-hid code consuming it.

Take the hideep touchscreen on this same tablet for example. After
this patch series we could also use it in i2c-hid mode (I did test
that as an extra test for patches 1-3) but the stock Android uses
it in its native hideep protocol mode which gives some more info
(ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next
the touchscreen is driven in its native mode. This requires sending
a command to (re)set it to native mode since it comes up in i2c-hid
mode by default.

This command is only send if a device-property is set (to avoid
causing issues on other hideep models) and the code consuming
that property looks like this:

        /*
         * Reset touch report format to the native HiDeep 20 protocol if requested.
         * This is necessary to make touchscreens which come up in I2C-HID mode
         * work with this driver.
         *
         * Note this is a kernel internal device-property set by x86 platform code,
         * this MUST not be used in devicetree files without first adding it to
         * the DT bindings.
         */
        if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol"))
                regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);

So maybe copy that and just add a:

	/*
         * Note this is a kernel internal device-property set by x86 platform code,
         * this MUST not be used in devicetree files without first adding it to
         * the DT bindings.
         */

Comment to the code reading the "post-reset-deassert-delay-ms"
property (patch 3/6) for v2 of this patch-set and leave it
at that ?

(and in hindsight I should have added that comment for v1 already)

> If the above is correct, then that means that the device property can
> be used, which makes little changes to your series.

Sounds good to me.

> But then, why aren't you using that property directly for those 2 other
> drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
> stub around adding the gpio names and the specific reset? (A plain "this
> is completely dumb" answer is fine, just trying to get my head around it).

Only 1 driver can bind to an i2c_client, and if those stub drivers
bind to it, then they must deal with it, or they would need to
create some fake i2c_client and pass everything through, but that
would be rather ugly.

> So, given the above, and your experience with the DT maintainers, I
> would go with patches 1-3 + a documentation of the new property, likely
> in the header or in kernel docs.

I'm fine with going with just patches 1-3. With patch 3 updated to
add the "this is a kernel internal only property" comment.

> Patches 4-6 either dropped, reworked, or left as they are, and we would
> merge them only if the maintainers of those files tested the changes.

Patches 4-6 were meant to make adding support for more
i2c-hid-of devices in the future easier, nothing more nothing
less. So I'm fine with dropping them.

I agree that at a minimum they should get tested before
merging them.

Regards,

Hans

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Benjamin Tissoires]

On Apr 11 2023, Hans de Goede wrote:
> Hi Benjamin,
> 
> On 4/11/23 18:56, Benjamin Tissoires wrote:
> > On Apr 11 2023, Hans de Goede wrote:
> >> Hi Benjamin,
> >>
> >> On 4/11/23 14:50, Benjamin Tissoires wrote:
> >>> On Apr 11 2023, Hans de Goede wrote:
> >>>> Hi Benjamin,
> >>>>
> >>>> On 4/11/23 11:02, Benjamin Tissoires wrote:
> 
> <snip>
> 
> >>>>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> >>>>> documenting it, because the OF folks do not want this in device tree,
> >>>>> and I tend to agree with them. So this basically creates a brand new
> >>>>> undocumented property, which is less than ideal.
> >>>>
> >>>> I'm merely not documenting it because there are no devicetree users yet.
> >>>
> >>> AFAIU, the non devicetree properties should also be documented through
> >>> DT bindings, no? So not documenting feels very much like "I want to slip
> >>> this one in without having to deal with DT maintainers" (and before you
> >>> take it personaly, I know this is definitively not the intent). So I'd
> >>> rather much having a public API documented, even if there are no users.
> >>
> >> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
> >> x86 devices, like x86 tablets which use Android as factory OS :)
> >>
> >> As such I have encountered the need for device-properties to pass info
> >> from drivers/platform/x86 code to more generic drivers a number of
> >> times before.
> >>
> >> Each time this happens, if I try to add them to bindings I get
> >> asked for some example devicetree code, I then respond that these
> >> are *device*-properties not of-properties and that there are no
> >> current devicetree users after which the DT maintainers tell me
> >> to then NOT document them in the DT bindings, at least not until
> >> actually DT users show up. I fully expect any attempt do add
> >> this to the DT bindings to go the same way.
> >>
> >> And now I have you telling me you really want to see this
> >> documented at the same time as it getting implemented. Which
> >> I fully understand, but does leads to a bit of a catch 22.
> > 
> > Ouch. Sorry for that.
> 
> No problem.
> 
> > Then I guess if the DT maintainers have a tendency
> > to accept those hidden properties, this is the simplest solution from a
> > i2c-hid/HID maintainer point of view, no?
> 
> Yes, I believe so, which is why I went this route in the first place.
> 
> > It's going to be a pain for the
> > platform driver because you still have to hardcode those properties
> > somewhere I guess.
> 
> Since the entire description is missing in ACPI for the digitizer (*)
> the x86-android-tablets.ko module which contains glue code to support
> these x86 android tablets already contains the i2c-busnumber,
> i2c-address, GPIO lookups, IRQ and other necessary device-properties
> like "hid-descr-addr", so adding one more device-property is very little
> trouble.
> 
> *) and also for other devices both on this and other x86 android tablets
> 
> <snip>
> 
> >> So I see 2 options here:
> >>
> >> 1. Take the approach from patches 1-4 here, but drop the property and
> >>    use match data on a new "wacom0169" i2c_device_id instead.
> >>    This would also pave the way to merging patches 5 + 6 once tested
> >>    by google to reduce some code duplication. Although you write below
> >>    you would prefer to keep these around as example code for other
> >>    specialized drivers...
> >>
> >> 2. Add a new specialized i2c-hid-of-wacom driver for this.
> >>    Question: Since this will be using i2c_device_id binding not
> >>    DT/OF binding the -of- in the name is technically incorrect,
> >>    but it would be consistent with the other specialized drivers
> >>    and could be seen as preparation (avoiding a rename/confusion)
> >>    for when any DT enumerated devices which need special handling
> >>    show up (note only relevant if you prefer this approach).
> > 
> > Well, option 2 is probably too much work for little gain. So I would go
> > with option 1, but with the following questions:
> > 
> > - a device property is public, so it can be seen as public API, right?
> >   So should we document it some way (not through DT) so we "guarantee"
> >   some behavior for it?
> 
> I believe the whole idea from the DT maintainers behind not documenting
> it as DT binding when not actually used in dts files is to keep it as
> in kernel *private* API, in this case between the x86-android-tablets.ko
> code instantiating the i2c_client and the i2c-hid code consuming it.
> 
> Take the hideep touchscreen on this same tablet for example. After
> this patch series we could also use it in i2c-hid mode (I did test
> that as an extra test for patches 1-3) but the stock Android uses
> it in its native hideep protocol mode which gives some more info
> (ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next
> the touchscreen is driven in its native mode. This requires sending
> a command to (re)set it to native mode since it comes up in i2c-hid
> mode by default.
> 
> This command is only send if a device-property is set (to avoid
> causing issues on other hideep models) and the code consuming
> that property looks like this:
> 
>         /*
>          * Reset touch report format to the native HiDeep 20 protocol if requested.
>          * This is necessary to make touchscreens which come up in I2C-HID mode
>          * work with this driver.
>          *
>          * Note this is a kernel internal device-property set by x86 platform code,
>          * this MUST not be used in devicetree files without first adding it to
>          * the DT bindings.
>          */
>         if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol"))
>                 regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);
> 
> So maybe copy that and just add a:
> 
> 	/*
>          * Note this is a kernel internal device-property set by x86 platform code,
>          * this MUST not be used in devicetree files without first adding it to
>          * the DT bindings.
>          */
> 
> Comment to the code reading the "post-reset-deassert-delay-ms"
> property (patch 3/6) for v2 of this patch-set and leave it
> at that ?

This seems like a better compromised :)

> 
> (and in hindsight I should have added that comment for v1 already)
> 
> > If the above is correct, then that means that the device property can
> > be used, which makes little changes to your series.
> 
> Sounds good to me.
> 
> > But then, why aren't you using that property directly for those 2 other
> > drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
> > stub around adding the gpio names and the specific reset? (A plain "this
> > is completely dumb" answer is fine, just trying to get my head around it).
> 
> Only 1 driver can bind to an i2c_client, and if those stub drivers
> bind to it, then they must deal with it, or they would need to
> create some fake i2c_client and pass everything through, but that
> would be rather ugly.

I was thinking that the i2c-hid-elan driver would override the property,
but that is assuming a driver can change a property once the device is
created, which I am now unsure.

> 
> > So, given the above, and your experience with the DT maintainers, I
> > would go with patches 1-3 + a documentation of the new property, likely
> > in the header or in kernel docs.
> 
> I'm fine with going with just patches 1-3. With patch 3 updated to
> add the "this is a kernel internal only property" comment.

Sounds like a good plan :)

> 
> > Patches 4-6 either dropped, reworked, or left as they are, and we would
> > merge them only if the maintainers of those files tested the changes.
> 
> Patches 4-6 were meant to make adding support for more
> i2c-hid-of devices in the future easier, nothing more nothing
> less. So I'm fine with dropping them.
> 
> I agree that at a minimum they should get tested before
> merging them.

We can keep them in a separate series, and wiat until we get some tests
on them before merging them, yes.

Cheers,
Benjamin

Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Doug Anderson]

Hi,

On Tue, Apr 11, 2023 at 9:57 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> > Either way is fine with me really. So you get to chose. If you
> > let me know which route you prefer, I'll go and prepare either
> > a v2 of this series, or a whole new patch for the new specialized
> > driver.
>
> Sorry for being a PITA, but having those driver separated allowed to
> move forward without having to have a spaghetti plate in i2c-hid, which
> was the case before the split (because *everything* was entangled: ACPI,
> DT, OF, properties). So that's why I'm trying to understand and
> minimize the changes.
>
> Also, before you sending v2 and involving too much, we could try to wait a
> few days for Doug to answer, and hear if he has an opinion. But if you
> rather send v2 right away, that's your choice obviously :)

I can test things if need be, but I want to make sure we're on the
right approach before going too deep into testing...

I guess a few notes here:

In general, I think DT maintainers are pretty leery of anything in the
device tree that tries to be "generic" and then a whole pile of
"kitchen sink" properties added to actually describe the device. Even
if it starts with just a few properties, the worry is that it will end
up being more and more over time. They would much rather specify which
exact device is present on the board and imply all the properties
based on knowing the device. Then the only things that are in the
device tree as properties are things that are board-specific. For
instance, if there was a hardware strapping that let you choose two
different hid descriptor addresses then that would be something to put
in the device tree.

The "post-power-on-delay-ms" was something that the DT maintainers
weren't too happy with. They would have much rather inferred this from
the specific compatible. You can actually see that the bindings say
that "Just "hid-over-i2c" alone is allowed, but not recommended."

Now, that being said, it's not always a hard-and-fast rule. For
instance, after years of needing to list every eDP panel directly in
device tree (often lying about it when multiple sources were listed),
we finally did manage to get the generic "panel-edp" bindings accepted
that has "just a few" properties needed to power up a device. ...then
the rest of the things we need are inferred once we start talking to
the device and get it to self-identify.

Bringing it back to i2c-hid-of: even though today the "goodix" and
"elan" drivers are largely the same, it wasn't always the case. For a
little while we had a whole pile of special logic in the "goodix"
driver to deal with the fact that if the touchscreen is powered up
(because it's shared or always-on) but the reset line is held asserted
that it draws a bunch of extra power. I had to end up taking that
logic out because it was too hard to reconcile with the second voltage
rail that I needed to add for a different board. See commit
557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the
regulator") and eb16f59e8e58 ("HID: i2c-hid: goodix: Add
mainboard-vddio-supply"). The need for this special logic is, as far
as I know, Goodix specific. I'm not aware of other touchscreens
holding themselves in a high power state if they are powered while
their reset line is held low. I don't think upstream would have liked
a DT properly like "avoid-holding-reset-low-while-powered;"
Ironically, there is actually more work to be done here. It turns out
that a different Chromebook that I wasn't aware of (and that wasn't
upstream yet) actually was relying on behavior to not assert reset and
we still need to figure out how to reconcile all of this. :(

I guess in general the idea of combining the drivers vs. coming up
with generic bindings is actually two separate things. We could have
separate bindings and still have one driver. At the time I made
i2c-of-goodix I was specifically requested to make separate drivers
for it. If maintainers want to re-combine them now, I won't object.
...but at least at the time it was a conscious decision and a specific
request to make them separate.

Looking at i2c-of-goodix and i2c-of-elan, we could probably combine
_those_ two drivers at this point, unless we actually end up needing
to go back again and do something goodix-specific for the reset line
again.

-Doug