[PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Douglas Anderson]

As pointed out by Rob Herring [1], we should have a device-specific
compatible string.  This means people shouldn't be using the
"i2c-over-hid" compatible string anymore, or at least not without a
more specific compatible string before it.  Specifically:

1. For newly added devices we should just have the device-specific
   device string (no "hid-over-i2c" fallback) and infer the timings
   and hid-descr-addr from there.

2. If there's a need for a device tree to be backward compatible, we
   should list the device-specific compatible string and add the
   "hid-over-i2c" fallback and the various timings.

[1] https://lore.kernel.org/r/20201019211036.GA3595039@bogus

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

Changes in v2:
- ("dt-bindings: HID: i2c-hid: Label this binding as deprecated") new in v2.

 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index c76bafaf98d2..733a5f053280 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -1,5 +1,8 @@
 * HID over I2C Device-Tree bindings
 
+WARNING: this binding is deprecated.  Instead of using this, create specific
+bindings for each hid-over-i2c device.
+
 HID over I2C provides support for various Human Interface Devices over the
 I2C bus. These devices can be for example touchpads, keyboards, touch screens
 or sensors.
-- 
2.29.0.rc1.297.gfa9743e501-goog

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

[Douglas Anderson]

This adds new bindings for the Goodix GT7375P touchscreen.  While this
touchscreen works with generic "i2c-over-hid", the current advice is
to give it its own compatible string.  The cleanest way to do this is
to give it its own bindings.

Among other things, this has the advantage that we can list the two
possible i2c addresses for this device, which gives extra checking.

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

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

 .../bindings/input/goodix,gt7375p.yaml        | 64 +++++++++++++++++++
 1 file changed, 64 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..b5008f89e26c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -0,0 +1,64 @@
+# 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:
+  - Benjamin Tissoires <benjamin.tissoires@redhat.com>
+  - 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 = "hid-over-i2c";
+        reg = <0x5d>;
+
+        interrupt-parent = <&tlmm>;
+        interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+
+        reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
+        vdd-supply = <&pp3300_ts>;
+      };
+    };
-- 
2.29.0.rc1.297.gfa9743e501-goog

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

[Doug Anderson]

Hi,

On Fri, Oct 23, 2020 at 4:23 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> +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 = "hid-over-i2c";

You always find one more problem right after you hit send, don't you?
Or is that just me?

Obviously the above should say

compatible = "goodix,gt7375p";

Luckily when I change that "dt_binding_check" still passes.  Whew!

I won't send a v3 yet and I'll wait for feedback.  On the off chance
that there are no other problems with this binding and the maintainer
wants to land this, I don't have any objections to the maintainer
fixing this when the patch lands.

-Doug

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

[Benjamin Tissoires]

Hi Doug,

On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> This adds new bindings for the Goodix GT7375P touchscreen.  While this
> touchscreen works with generic "i2c-over-hid", the current advice is
> to give it its own compatible string.  The cleanest way to do this is
> to give it its own bindings.
>
> Among other things, this has the advantage that we can list the two
> possible i2c addresses for this device, which gives extra checking.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2.
>
>  .../bindings/input/goodix,gt7375p.yaml        | 64 +++++++++++++++++++
>  1 file changed, 64 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..b5008f89e26c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -0,0 +1,64 @@
> +# 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:
> +  - Benjamin Tissoires <benjamin.tissoires@redhat.com>

Given my answer in patch 1, I am not very happy being added as a
maintainer here.

Cheers,
Benjamin


> +  - 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 = "hid-over-i2c";
> +        reg = <0x5d>;
> +
> +        interrupt-parent = <&tlmm>;
> +        interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> +
> +        reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
> +        vdd-supply = <&pp3300_ts>;
> +      };
> +    };
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

[PATCH v2 3/3] HID: i2c-hid: Support the Goodix GT7375P touchscreen

[Douglas Anderson]

The Goodix GT7375P touchscreen uses i2c-hid so we can support it with
just a few changes to the i2c-hid driver.  Specifically this
touchscreen needs to control a reset GPIO during its power sequencing.

The Goodix timing diagram shows this:

         +----------------------------------
         |
AVDD ----+
               +------------------------------
         | (a) |
RESET ---------+
                     +-------------
               | (b) |
I2C comm OK ---------+

Where (a) is 10 ms and (b) is 120 ms.

While we could just add some properties and specify this generically
in the device tree, the guidance from the device tree maintainer is
that it's better to list the specific model and infer everything from
there.  Thus that's what this patch implements.

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

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

 drivers/hid/i2c-hid/i2c-hid-core.c    | 50 ++++++++++++++++++++++++++-
 include/linux/platform_data/i2c-hid.h |  5 +++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 786e3e9af1c9..0b2cd78b05e1 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -71,6 +71,12 @@ do {									  \
 		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
 } while (0)
 
+struct i2c_hid_match_data {
+	u16 hid_descriptor_address;
+	int post_power_delay_ms;
+	int post_gpio_reset_delay_ms;
+};
+
 struct i2c_hid_desc {
 	__le16 wHIDDescLength;
 	__le16 bcdVersion;
@@ -962,6 +968,21 @@ static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {}
 #endif
 
 #ifdef CONFIG_OF
+static bool i2c_hid_init_from_of_match(struct device *dev,
+				       struct i2c_hid_platform_data *pdata)
+{
+	const struct i2c_hid_match_data *match_data = device_get_match_data(dev);
+
+	if (!match_data)
+		return false;
+
+	pdata->hid_descriptor_address = match_data->hid_descriptor_address;
+	pdata->post_power_delay_ms = match_data->post_power_delay_ms;
+	pdata->post_gpio_reset_delay_ms = match_data->post_gpio_reset_delay_ms;
+
+	return true;
+}
+
 static int i2c_hid_of_probe(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
@@ -969,6 +990,11 @@ static int i2c_hid_of_probe(struct i2c_client *client,
 	u32 val;
 	int ret;
 
+	/* Try getting everything based on the compatible string first */
+	if (i2c_hid_init_from_of_match(&client->dev, pdata))
+		return 0;
+
+	/* Fallback to getting hid-descr-addr from a property */
 	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
 	if (ret) {
 		dev_err(&client->dev, "HID register address not provided\n");
@@ -984,8 +1010,15 @@ static int i2c_hid_of_probe(struct i2c_client *client,
 	return 0;
 }
 
+static const struct i2c_hid_match_data goodix_gt7375p_match_data = {
+	.hid_descriptor_address = 0x0001,
+	.post_power_delay_ms = 10,
+	.post_gpio_reset_delay_ms = 120,
+};
+
 static const struct of_device_id i2c_hid_of_match[] = {
-	{ .compatible = "hid-over-i2c" },
+	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_match_data },
+	{ .compatible = "hid-over-i2c" }, /* Deprecated */
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
@@ -1053,6 +1086,12 @@ static int i2c_hid_probe(struct i2c_client *client,
 	ihid->pdata.supplies[0].supply = "vdd";
 	ihid->pdata.supplies[1].supply = "vddl";
 
+	/* Start out with reset asserted */
+	ihid->pdata.reset_gpio =
+		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid->pdata.reset_gpio))
+		return PTR_ERR(ihid->pdata.reset_gpio);
+
 	ret = devm_regulator_bulk_get(&client->dev,
 				      ARRAY_SIZE(ihid->pdata.supplies),
 				      ihid->pdata.supplies);
@@ -1067,6 +1106,10 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (ihid->pdata.post_power_delay_ms)
 		msleep(ihid->pdata.post_power_delay_ms);
 
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
+	if (ihid->pdata.post_gpio_reset_delay_ms)
+		msleep(ihid->pdata.post_gpio_reset_delay_ms);
+
 	i2c_set_clientdata(client, ihid);
 
 	ihid->client = client;
@@ -1163,6 +1206,7 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->bufsize)
 		i2c_hid_free_buffers(ihid);
 
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
 			       ihid->pdata.supplies);
 
@@ -1228,6 +1272,10 @@ static int i2c_hid_resume(struct device *dev)
 
 		if (ihid->pdata.post_power_delay_ms)
 			msleep(ihid->pdata.post_power_delay_ms);
+
+		gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
+		if (ihid->pdata.post_gpio_reset_delay_ms)
+			msleep(ihid->pdata.post_gpio_reset_delay_ms);
 	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
index c628bb5e1061..b2150223ffa6 100644
--- a/include/linux/platform_data/i2c-hid.h
+++ b/include/linux/platform_data/i2c-hid.h
@@ -12,6 +12,7 @@
 #ifndef __LINUX_I2C_HID_H
 #define __LINUX_I2C_HID_H
 
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/types.h>
 
@@ -20,6 +21,8 @@
  * @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.
+ * @post_gpio_reset_delay_ms: delay after reset via GPIO.
+ * @reset_gpio: optional gpio to de-assert after post_power_delay_ms.
  *
  * 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
@@ -36,6 +39,8 @@ struct i2c_hid_platform_data {
 	u16 hid_descriptor_address;
 	struct regulator_bulk_data supplies[2];
 	int post_power_delay_ms;
+	int post_gpio_reset_delay_ms;
+	struct gpio_desc *reset_gpio;
 };
 
 #endif /* __LINUX_I2C_HID_H */
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH v2 3/3] HID: i2c-hid: Support the Goodix GT7375P touchscreen

[Benjamin Tissoires]

Hi Doug,

On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> The Goodix GT7375P touchscreen uses i2c-hid so we can support it with
> just a few changes to the i2c-hid driver.  Specifically this
> touchscreen needs to control a reset GPIO during its power sequencing.
>
> The Goodix timing diagram shows this:
>
>          +----------------------------------
>          |
> AVDD ----+
>                +------------------------------
>          | (a) |
> RESET ---------+
>                      +-------------
>                | (b) |
> I2C comm OK ---------+
>
> Where (a) is 10 ms and (b) is 120 ms.

These timing issues always bother me. Is there any hint that this
particular touchscreen model is "certified" for a Windows usage?
Because if so, then we might as well mimic the timings the Windows
driver is doing instead of adding parameters for every single device.

>
> While we could just add some properties and specify this generically
> in the device tree, the guidance from the device tree maintainer is
> that it's better to list the specific model and infer everything from
> there.  Thus that's what this patch implements.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Use a separate compatible string for this new touchscreen.
> - Get timings based on the compatible string.
>
>  drivers/hid/i2c-hid/i2c-hid-core.c    | 50 ++++++++++++++++++++++++++-
>  include/linux/platform_data/i2c-hid.h |  5 +++
>  2 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 786e3e9af1c9..0b2cd78b05e1 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -71,6 +71,12 @@ do {                                                                   \
>                 dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
>  } while (0)
>
> +struct i2c_hid_match_data {
> +       u16 hid_descriptor_address;
> +       int post_power_delay_ms;
> +       int post_gpio_reset_delay_ms;
> +};
> +
>  struct i2c_hid_desc {
>         __le16 wHIDDescLength;
>         __le16 bcdVersion;
> @@ -962,6 +968,21 @@ static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {}
>  #endif
>
>  #ifdef CONFIG_OF
> +static bool i2c_hid_init_from_of_match(struct device *dev,
> +                                      struct i2c_hid_platform_data *pdata)
> +{
> +       const struct i2c_hid_match_data *match_data = device_get_match_data(dev);
> +
> +       if (!match_data)
> +               return false;
> +
> +       pdata->hid_descriptor_address = match_data->hid_descriptor_address;
> +       pdata->post_power_delay_ms = match_data->post_power_delay_ms;
> +       pdata->post_gpio_reset_delay_ms = match_data->post_gpio_reset_delay_ms;
> +
> +       return true;
> +}
> +
>  static int i2c_hid_of_probe(struct i2c_client *client,
>                 struct i2c_hid_platform_data *pdata)
>  {
> @@ -969,6 +990,11 @@ static int i2c_hid_of_probe(struct i2c_client *client,
>         u32 val;
>         int ret;
>
> +       /* Try getting everything based on the compatible string first */
> +       if (i2c_hid_init_from_of_match(&client->dev, pdata))
> +               return 0;
> +
> +       /* Fallback to getting hid-descr-addr from a property */
>         ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
>         if (ret) {
>                 dev_err(&client->dev, "HID register address not provided\n");
> @@ -984,8 +1010,15 @@ static int i2c_hid_of_probe(struct i2c_client *client,
>         return 0;
>  }
>
> +static const struct i2c_hid_match_data goodix_gt7375p_match_data = {
> +       .hid_descriptor_address = 0x0001,

Nah, please don't. See 1/3 of this series.

> +       .post_power_delay_ms = 10,
> +       .post_gpio_reset_delay_ms = 120,
> +};
> +
>  static const struct of_device_id i2c_hid_of_match[] = {
> -       { .compatible = "hid-over-i2c" },
> +       { .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_match_data },
> +       { .compatible = "hid-over-i2c" }, /* Deprecated */
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
> @@ -1053,6 +1086,12 @@ static int i2c_hid_probe(struct i2c_client *client,
>         ihid->pdata.supplies[0].supply = "vdd";
>         ihid->pdata.supplies[1].supply = "vddl";
>
> +       /* Start out with reset asserted */
> +       ihid->pdata.reset_gpio =
> +               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(ihid->pdata.reset_gpio))
> +               return PTR_ERR(ihid->pdata.reset_gpio);
> +

That gpio change should go into its own commit. The commit briefly
mentioned this, but we could get more information on it, because this
is modifying the common probe path.


[... re-reading, thinking later ...]

Well, the whole point of this patch is for that specific GPIO. And I
am really not happy having that merged here:
what if suddenly we have another device that has a reset line that
requires a different "protocol" (i.e. low instead of high, or need to
switch it 3 times from high to low before the sleep)?
ACPI integrated devices don't need that reset line. Or if they do, it
is integrated at the ACPI level. So can we have something similar
here? Either force Goodix not to use a reset line, either have a
platform driver (well, platform might not be the correct place) that
would handle those compatible strings and reset lines before i2c-hid?

The more I think of it, the more I think we should have another piece
in front of i2c-hid, to emulate somehow what ACPI is capable of doing
behind our back. That might require a little bit of work in i2c-hid,
but if in the end we can remove the regulator specifics, reset
specifics and any DT specifics that were added in the past and have
i2c-hid being just the spec it follows, that will surely help us
moving forward.

Cheers,
Benjamin

>         ret = devm_regulator_bulk_get(&client->dev,
>                                       ARRAY_SIZE(ihid->pdata.supplies),
>                                       ihid->pdata.supplies);
> @@ -1067,6 +1106,10 @@ static int i2c_hid_probe(struct i2c_client *client,
>         if (ihid->pdata.post_power_delay_ms)
>                 msleep(ihid->pdata.post_power_delay_ms);
>
> +       gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
> +       if (ihid->pdata.post_gpio_reset_delay_ms)
> +               msleep(ihid->pdata.post_gpio_reset_delay_ms);
> +
>         i2c_set_clientdata(client, ihid);
>
>         ihid->client = client;
> @@ -1163,6 +1206,7 @@ static int i2c_hid_remove(struct i2c_client *client)
>         if (ihid->bufsize)
>                 i2c_hid_free_buffers(ihid);
>
> +       gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
>         regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
>                                ihid->pdata.supplies);
>
> @@ -1228,6 +1272,10 @@ static int i2c_hid_resume(struct device *dev)
>
>                 if (ihid->pdata.post_power_delay_ms)
>                         msleep(ihid->pdata.post_power_delay_ms);
> +
> +               gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
> +               if (ihid->pdata.post_gpio_reset_delay_ms)
> +                       msleep(ihid->pdata.post_gpio_reset_delay_ms);
>         } else if (ihid->irq_wake_enabled) {
>                 wake_status = disable_irq_wake(client->irq);
>                 if (!wake_status)
> diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
> index c628bb5e1061..b2150223ffa6 100644
> --- a/include/linux/platform_data/i2c-hid.h
> +++ b/include/linux/platform_data/i2c-hid.h
> @@ -12,6 +12,7 @@
>  #ifndef __LINUX_I2C_HID_H
>  #define __LINUX_I2C_HID_H
>
> +#include <linux/gpio/consumer.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/types.h>
>
> @@ -20,6 +21,8 @@
>   * @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.
> + * @post_gpio_reset_delay_ms: delay after reset via GPIO.
> + * @reset_gpio: optional gpio to de-assert after post_power_delay_ms.
>   *
>   * 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
> @@ -36,6 +39,8 @@ struct i2c_hid_platform_data {
>         u16 hid_descriptor_address;
>         struct regulator_bulk_data supplies[2];
>         int post_power_delay_ms;
> +       int post_gpio_reset_delay_ms;
> +       struct gpio_desc *reset_gpio;
>  };
>
>  #endif /* __LINUX_I2C_HID_H */
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Benjamin Tissoires]

Hi Doug,

Foreword: I was about to say "yeah, whatever" to please Rob for once.
But after re-reading this and more specifically patch 3 of the series,
that won't do. More comments inlined.

On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> As pointed out by Rob Herring [1], we should have a device-specific
> compatible string.  This means people shouldn't be using the
> "i2c-over-hid" compatible string anymore, or at least not without a
> more specific compatible string before it.  Specifically:
>
> 1. For newly added devices we should just have the device-specific
>    device string (no "hid-over-i2c" fallback) and infer the timings
>    and hid-descr-addr from there.

And that's a big NACK from a maintainer point of view. I know in the
device tree world these strings are important so that people can just
say "I have a device compatible with X", and go on, but in the HID
world that means we will have to implement one compatible struct per
vendor/device, which is not something I want to do.

You can think of it as if you are suddenly saying that because it
would be easier for a few particular USB devices that need a quirk,
you "just" need to add the list of *all* USB HID devices that are
around. i2c-hid should be a driver that doesn't change unless 2 things
happen:
- there is a change in the spec
- there is a specific quirk required for a device that doesn't follow the spec.

So if having device tree support for these means we suddenly need to
add every single device around in the compatible table, I would be
tempted to just drop the support for those new devices.

Again, you (or anyone else) have to understand that the descriptor
address is just a parameter which is known at the manufacturing time,
but that can vary with different vendors and or products. In the ACPI
world, this parameter is provided in the DSDT, and there is no reason
for it to not be provided in the DT.

The last thing I want to see is people using device tree having to
recompile i2c-hid to register their own device.

If this part of the Device Tree binding is so important for the DT
world, then we should split up the DT bindings from i2c-hid, and have
some platform driver that would handle a conversion between devicetree
and platform data. But this driver won't be maintained by me.

I agree adding the various sleep parameters in the platform data is
not good, but I prefer that over having to maintain an endless table
of parameters for every single i2c-hid device out there.

Cheers,
Benjamin


>
> 2. If there's a need for a device tree to be backward compatible, we
>    should list the device-specific compatible string and add the
>    "hid-over-i2c" fallback and the various timings.
>
> [1] https://lore.kernel.org/r/20201019211036.GA3595039@bogus
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - ("dt-bindings: HID: i2c-hid: Label this binding as deprecated") new in v2.
>
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index c76bafaf98d2..733a5f053280 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -1,5 +1,8 @@
>  * HID over I2C Device-Tree bindings
>
> +WARNING: this binding is deprecated.  Instead of using this, create specific
> +bindings for each hid-over-i2c device.
> +
>  HID over I2C provides support for various Human Interface Devices over the
>  I2C bus. These devices can be for example touchpads, keyboards, touch screens
>  or sensors.
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Rob Herring]

On Fri, Oct 30, 2020 at 11:51:53AM +0100, Benjamin Tissoires wrote:
> Hi Doug,
> 
> Foreword: I was about to say "yeah, whatever" to please Rob for once.

Read my other reply first... I think we mostly agree.

> But after re-reading this and more specifically patch 3 of the series,
> that won't do. More comments inlined.
> 
> On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > As pointed out by Rob Herring [1], we should have a device-specific
> > compatible string.  This means people shouldn't be using the
> > "i2c-over-hid" compatible string anymore, or at least not without a
> > more specific compatible string before it.  Specifically:
> >
> > 1. For newly added devices we should just have the device-specific
> >    device string (no "hid-over-i2c" fallback) and infer the timings
> >    and hid-descr-addr from there.
> 
> And that's a big NACK from a maintainer point of view. I know in the
> device tree world these strings are important so that people can just
> say "I have a device compatible with X", and go on, but in the HID
> world that means we will have to implement one compatible struct per
> vendor/device, which is not something I want to do.

It's not really any different than PCI and USB VID/PIDs.

> You can think of it as if you are suddenly saying that because it
> would be easier for a few particular USB devices that need a quirk,
> you "just" need to add the list of *all* USB HID devices that are
> around. i2c-hid should be a driver that doesn't change unless 2 things
> happen:
> - there is a change in the spec
> - there is a specific quirk required for a device that doesn't follow the spec.

Or does something outside of what the spec covers. 

> So if having device tree support for these means we suddenly need to
> add every single device around in the compatible table, I would be
> tempted to just drop the support for those new devices.
> 
> Again, you (or anyone else) have to understand that the descriptor
> address is just a parameter which is known at the manufacturing time,
> but that can vary with different vendors and or products. In the ACPI
> world, this parameter is provided in the DSDT, and there is no reason
> for it to not be provided in the DT.

Whether that makes sense as a standard 'hid-over-i2c' property is a 
separate discussion. Seems like it might be.

It's trying to parameterize power sequencing to be generic and a never 
ending stream of quirk property additions that I'm against. That's based 
on the mistake of accepting those to some point in the past. 
hid-over-i2c is not special here.

If we wanted to parameterize power control/sequences in DT, then we'd 
need to handle any number of controls (GPIO, regulators, clocks, power 
domains, register poking, firmware loading, etc.) in any order and 
amounts of time in between. What we'd end up needing is some programming 
language in DT (Forth anyone?).

> The last thing I want to see is people using device tree having to
> recompile i2c-hid to register their own device.

That's fine if they don't need extra things like power control...

> If this part of the Device Tree binding is so important for the DT
> world, then we should split up the DT bindings from i2c-hid, and have
> some platform driver that would handle a conversion between devicetree
> and platform data. But this driver won't be maintained by me.
> 
> I agree adding the various sleep parameters in the platform data is
> not good, but I prefer that over having to maintain an endless table
> of parameters for every single i2c-hid device out there.

How is match data any different from platform data? It not other than 
one is all in the same file and the other adds a bunch of boilerplate 
and a pointless driver. If it's really such a maintainer burden, then 
perhaps the driver model could learn how to add match entries 
dynamically or from multiple sources (like a 2nd file of ids and 
data). Just throwing out ideas here...

Rob

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Benjamin Tissoires]

On Fri, Oct 30, 2020 at 7:00 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 30, 2020 at 11:51:53AM +0100, Benjamin Tissoires wrote:
> > Hi Doug,
> >
> > Foreword: I was about to say "yeah, whatever" to please Rob for once.
>
> Read my other reply first... I think we mostly agree.
>
> > But after re-reading this and more specifically patch 3 of the series,
> > that won't do. More comments inlined.
> >
> > On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > As pointed out by Rob Herring [1], we should have a device-specific
> > > compatible string.  This means people shouldn't be using the
> > > "i2c-over-hid" compatible string anymore, or at least not without a
> > > more specific compatible string before it.  Specifically:
> > >
> > > 1. For newly added devices we should just have the device-specific
> > >    device string (no "hid-over-i2c" fallback) and infer the timings
> > >    and hid-descr-addr from there.
> >
> > And that's a big NACK from a maintainer point of view. I know in the
> > device tree world these strings are important so that people can just
> > say "I have a device compatible with X", and go on, but in the HID
> > world that means we will have to implement one compatible struct per
> > vendor/device, which is not something I want to do.
>
> It's not really any different than PCI and USB VID/PIDs.

Well, it is, because in the USB (HID) world, there is a specification
that provides all of the entry points a device needs. In the i2c-hid
case, the only entry point a device needs, in the ACPI world is one
register address, and this is provided by ACPI itself. So in the ACPI
world, for i2c-hid devices, we don't need to recompile the driver to
support any current or new devices.

>
> > You can think of it as if you are suddenly saying that because it
> > would be easier for a few particular USB devices that need a quirk,
> > you "just" need to add the list of *all* USB HID devices that are
> > around. i2c-hid should be a driver that doesn't change unless 2 things
> > happen:
> > - there is a change in the spec
> > - there is a specific quirk required for a device that doesn't follow the spec.
>
> Or does something outside of what the spec covers.

This is solved in the ACPI case by running ACPI callbacks, and I am
more and more thinking we should mimic that for DT devices.

>
> > So if having device tree support for these means we suddenly need to
> > add every single device around in the compatible table, I would be
> > tempted to just drop the support for those new devices.
> >
> > Again, you (or anyone else) have to understand that the descriptor
> > address is just a parameter which is known at the manufacturing time,
> > but that can vary with different vendors and or products. In the ACPI
> > world, this parameter is provided in the DSDT, and there is no reason
> > for it to not be provided in the DT.
>
> Whether that makes sense as a standard 'hid-over-i2c' property is a
> separate discussion. Seems like it might be.

Actually it is not TBH. The spec doesn't mention that sleep time (or
the reset line FWIW), so it shouldn't even be seen by i2c-hid. But I
accepted maybe too much parametrization on i2c-hid, and now is
probably the time we take a step back and rewrite the code that goes
out of spec.

>
> It's trying to parameterize power sequencing to be generic and a never
> ending stream of quirk property additions that I'm against. That's based
> on the mistake of accepting those to some point in the past.
> hid-over-i2c is not special here.

Ack

>
> If we wanted to parameterize power control/sequences in DT, then we'd
> need to handle any number of controls (GPIO, regulators, clocks, power
> domains, register poking, firmware loading, etc.) in any order and
> amounts of time in between. What we'd end up needing is some programming
> language in DT (Forth anyone?).

Understood, and we are hitting the exact same problem here. The only
difference is that i2c-hid is already generic for anything but
power/reset, and this is what we are vehemently agreeing here :P

>
> > The last thing I want to see is people using device tree having to
> > recompile i2c-hid to register their own device.
>
> That's fine if they don't need extra things like power control...
>
> > If this part of the Device Tree binding is so important for the DT
> > world, then we should split up the DT bindings from i2c-hid, and have
> > some platform driver that would handle a conversion between devicetree
> > and platform data. But this driver won't be maintained by me.
> >
> > I agree adding the various sleep parameters in the platform data is
> > not good, but I prefer that over having to maintain an endless table
> > of parameters for every single i2c-hid device out there.
>
> How is match data any different from platform data? It not other than

The platform data is filled by ACPI based on the DSDT, and only a few
options are used: HID descriptor register address and irq IIRC.

The other platform data options were added more specifically to work
around the fact that DT doesn't have the language mentioned above, and
that's where dragons are coming in.

> one is all in the same file and the other adds a bunch of boilerplate
> and a pointless driver. If it's really such a maintainer burden, then
> perhaps the driver model could learn how to add match entries
> dynamically or from multiple sources (like a 2nd file of ids and
> data). Just throwing out ideas here...

I couldn't agree more (see my comment in patch 3/3 at
https://patchwork.kernel.org/project/linux-input/patch/20201023162220.v2.3.Ied4ce10d229cd7c69abf13a0361ba0b8d82eb9c4@changeid/#23723561
in case you were not cc-ed).

Ideally, we should split out anything DT related in the i2c-hid code,
and work around it in the same "hidden" way ACPI is: have a few
functions that could be overridden when entering into full power, or
sleep. This code could be then maintained separately from the generic
code, and we would be able to have some compatible definitions there,
without polluting the other devices. And as you mentioned, maybe we
could make this dynamic.

I honestly wish we could have this as a separate module that would be
in charge of the DT folks, but already having it as a separate file
would be a win.

Cheers,
Benjamin

>
> Rob
>

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Dmitry Torokhov]

On Fri, Oct 30, 2020 at 08:12:06PM +0100, Benjamin Tissoires wrote:
> On Fri, Oct 30, 2020 at 7:00 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 30, 2020 at 11:51:53AM +0100, Benjamin Tissoires wrote:
> > > Hi Doug,
> > >
> > > Foreword: I was about to say "yeah, whatever" to please Rob for once.
> >
> > Read my other reply first... I think we mostly agree.
> >
> > > But after re-reading this and more specifically patch 3 of the series,
> > > that won't do. More comments inlined.
> > >
> > > On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@chromium.org> wrote:
> > > >
> > > > As pointed out by Rob Herring [1], we should have a device-specific
> > > > compatible string.  This means people shouldn't be using the
> > > > "i2c-over-hid" compatible string anymore, or at least not without a
> > > > more specific compatible string before it.  Specifically:
> > > >
> > > > 1. For newly added devices we should just have the device-specific
> > > >    device string (no "hid-over-i2c" fallback) and infer the timings
> > > >    and hid-descr-addr from there.
> > >
> > > And that's a big NACK from a maintainer point of view. I know in the
> > > device tree world these strings are important so that people can just
> > > say "I have a device compatible with X", and go on, but in the HID
> > > world that means we will have to implement one compatible struct per
> > > vendor/device, which is not something I want to do.
> >
> > It's not really any different than PCI and USB VID/PIDs.
> 
> Well, it is, because in the USB (HID) world, there is a specification
> that provides all of the entry points a device needs. In the i2c-hid
> case, the only entry point a device needs, in the ACPI world is one
> register address, and this is provided by ACPI itself. So in the ACPI
> world, for i2c-hid devices, we don't need to recompile the driver to
> support any current or new devices.
> 
> >
> > > You can think of it as if you are suddenly saying that because it
> > > would be easier for a few particular USB devices that need a quirk,
> > > you "just" need to add the list of *all* USB HID devices that are
> > > around. i2c-hid should be a driver that doesn't change unless 2 things
> > > happen:
> > > - there is a change in the spec
> > > - there is a specific quirk required for a device that doesn't follow the spec.
> >
> > Or does something outside of what the spec covers.
> 
> This is solved in the ACPI case by running ACPI callbacks, and I am
> more and more thinking we should mimic that for DT devices.

So this is the root of the problem. I2CHID spec was done for ACPI-based
systems, with very limited interface between hardware and the kernel and
all "unplesantness" such as powering up and down devices properly tucked
safely away into firmware. So there is still a lot of custom code, we
just do not see it and can pretend it does not exist.

So even in case of "standard" I2C one can not say they do not need to
recompile to use a new device, they just need to recompile different
thing (driver vs firmware).

I am still unsure if we want a flexible way of describing power up
sequence, or simply hard-code based on a given model. Given that here
are many I2C-HID compatible devices a flexible scheme would be nice IMO.

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Doug Anderson]

Hi,

On Fri, Oct 30, 2020 at 12:12 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> I honestly wish we could have this as a separate module that would be
> in charge of the DT folks, but already having it as a separate file
> would be a win.

I've made my best effort at splitting it into a fully separate module
in my v3.  I'm sure there will be bikeshed-type issues, but maybe it
looks OK-ish now?

-Doug

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Rob Herring]

On Fri, Oct 23, 2020 at 04:22:52PM -0700, Douglas Anderson wrote:
> As pointed out by Rob Herring [1], we should have a device-specific
> compatible string.  This means people shouldn't be using the
> "i2c-over-hid" compatible string anymore, or at least not without a
> more specific compatible string before it.  Specifically:
> 
> 1. For newly added devices we should just have the device-specific
>    device string (no "hid-over-i2c" fallback) and infer the timings
>    and hid-descr-addr from there.

I wouldn't go that far. Having a fallback is perfectly acceptible. And 
hopefully there are at least some devices where that's good enough for 
drivers to use.

If we have cases of only 'i2c-over-hid' being used (in DT), then the 
solution is making this a schema so we can enforce that as not valid.

> 
> 2. If there's a need for a device tree to be backward compatible, we
>    should list the device-specific compatible string and add the
>    "hid-over-i2c" fallback and the various timings.
> 
> [1] https://lore.kernel.org/r/20201019211036.GA3595039@bogus
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - ("dt-bindings: HID: i2c-hid: Label this binding as deprecated") new in v2.
> 
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index c76bafaf98d2..733a5f053280 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -1,5 +1,8 @@
>  * HID over I2C Device-Tree bindings
>  
> +WARNING: this binding is deprecated.  Instead of using this, create specific
> +bindings for each hid-over-i2c device.
> +
>  HID over I2C provides support for various Human Interface Devices over the
>  I2C bus. These devices can be for example touchpads, keyboards, touch screens
>  or sensors.
> -- 
> 2.29.0.rc1.297.gfa9743e501-goog
> 

Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Doug Anderson]

Hi,

On Fri, Oct 30, 2020 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 23, 2020 at 04:22:52PM -0700, Douglas Anderson wrote:
> > As pointed out by Rob Herring [1], we should have a device-specific
> > compatible string.  This means people shouldn't be using the
> > "i2c-over-hid" compatible string anymore, or at least not without a
> > more specific compatible string before it.  Specifically:
> >
> > 1. For newly added devices we should just have the device-specific
> >    device string (no "hid-over-i2c" fallback) and infer the timings
> >    and hid-descr-addr from there.
>
> I wouldn't go that far. Having a fallback is perfectly acceptible. And
> hopefully there are at least some devices where that's good enough for
> drivers to use.
>
> If we have cases of only 'i2c-over-hid' being used (in DT), then the
> solution is making this a schema so we can enforce that as not valid.

OK, fair enough.  I think in the case of the Goodix touchscreen I'm
trying to support, though, it's not useful to have the fallback since
it really doesn't seem to work without all the delays.  :(  I sent my
v3 without touching anything about "i2c-over-hid" as far as bindings
goes.

For my edification, though, when do you believe "i2c-over-hid" should
be the fallback?  Presumably you would advocate for
"post-power-on-delay-ms" being marked as deprecated, right?  That
should have been inferred by the compatible string, right?  So, in
theory, anyone who needed this delay couldn't have ever taken
advantage of the fallback, right?  They wouldn't have worked without
the delay?

-Doug