[PATCH v6 0/2] Add ili9882t bindings and timing

[PATCH v6 0/2] Add ili9882t bindings and timing

[Cong Yang]

Add bindings for Ilitek. The ili9882t touch screen chip same as
Elan eKTH6915 controller has a reset gpio. The difference is that
ilitek9882 needs to use vccio-supply instead of vcc33-supply. 
From Dmitry suggestion, it would make more sense to distinguish the
binging of ili9882 and eKTH6915.

From The datasheet specifies there should be 60ms between touch SDA
sleep and panel RESX. so we can add the 65 ms delay in i2c_hid_core_suspend.

As Doug said, only the panel follower patch series[1] land makes sense, and now the panel follower
patch series[1] has land.
[1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org

Changes in v6:
- PATCH 1/2: Modify subject.
- Link to v5: https://lore.kernel.org/all/20230609063615.758676-1-yangcong5@huaqin.corp-partner.google.com/

Changes in v5:
- PATCH 1/2: Add panel as a required in property and examples.
- PATCH 2/2: Set a NULL to ili9882t_chip_data for vcc33-supply, then not use vcc33 regulator.
- Link to v4: https://lore.kernel.org/all/20230608130147.2835818-1-yangcong5@huaqin.corp-partner.google.com/

Changes in v4:
- PATCH 1/2: Remove compatible items and add reset maxItems.
- PATCH 1/2: Refer to the panel description in Doug serias[1].
  [1] https://lore.kernel.org/all/20230607144931.v2.1.Id68e30343bb1e11470582a9078b086176cfec46b@changeid/ 
- PATCH 2/2: Set a "null" to ili9882t_chip_data for vcc33-supply, then using dummy regulator.
- Link to v3: https://lore.kernel.org/all/20230607133458.4075667-1-yangcong5@huaqin.corp-partner.google.com/

Changes in v3:
- PATCH 1/2: Introduce bindings for Ilitek.
- Link to v2: https://lore.kernel.org/all/20230605060524.1178164-1-yangcong5@huaqin.corp-partner.google.com/

Changes in v2:
- PATCH 1/2: fix ran make dt_binding_check warnings/errors.
- PATCH 1/2: remove oneOf,just enum.
- Link to v1: https://lore.kernel.org/all/20230602140948.2138668-1-yangcong5@huaqin.corp-partner.google.com/

Cong Yang (2):
  dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t
  HID: i2c-hid: elan: Add ili9882t timing

 .../bindings/input/ilitek,ili9882t.yaml       | 67 +++++++++++++++++++
 drivers/hid/i2c-hid/i2c-hid-of-elan.c         | 50 ++++++++++----
 2 files changed, 105 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml

-- 
2.25.1

[PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t

[Cong Yang]

The ili9882t touch screen chip same as Elan eKTH6915 controller
has a reset gpio. The difference is that ili9882t needs to use
vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
and touchscreens to power on/off together, let's add a phandle for this.

[1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 .../bindings/input/ilitek,ili9882t.yaml       | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml

diff --git a/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml b/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
new file mode 100644
index 000000000000..c5d9e0e919f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ilitek,ili9882t.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ilitek ili9882t touchscreen controller
+
+maintainers:
+  - Cong Yang <yangcong5@huaqin.corp-partner.google.com>
+
+description:
+  Supports the Ilitek ili9882t touchscreen controller.
+  This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+
+allOf:
+  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
+properties:
+  compatible:
+    const: ilitek,ili9882t
+
+  reg:
+    const: 0x41
+
+  interrupts:
+    maxItems: 1
+
+  panel: true
+
+  reset-gpios:
+    maxItems: 1
+    description: Reset GPIO.
+
+  vccio-supply:
+    description: The 1.8V supply to the touchscreen.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - panel
+  - vccio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen: touchscreen@41 {
+        compatible = "ilitek,ili9882t";
+        reg = <0x41>;
+
+        interrupt-parent = <&pio>;
+        interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+
+        panel = <&panel>;
+        reset-gpios = <&pio 60 GPIO_ACTIVE_LOW>;
+        vccio-supply = <&mt6366_vio18_reg>;
+      };
+    };
-- 
2.25.1

[PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Cong Yang]

The ili9882t is a TDDI IC (Touch with Display Driver). The
datasheet specifies there should be 60ms between touch SDA
sleep and panel RESX. Doug's series[1] allows panels and
touchscreens to power on/off together, so we can add the 65 ms
delay in i2c_hid_core_suspend before panel_unprepare.

Because ili9882t touchscrgeen is a panel follower, and
needs to use vccio-supply instead of vcc33-supply, so set
it NULL to ili9882t_chip_data, then not use vcc33 regulator.

[1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 029045d9661c..31abab57ad44 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -18,9 +18,11 @@
 #include "i2c-hid.h"
 
 struct elan_i2c_hid_chip_data {
-	unsigned int post_gpio_reset_delay_ms;
+	unsigned int post_gpio_reset_on_delay_ms;
+	unsigned int post_gpio_reset_off_delay_ms;
 	unsigned int post_power_delay_ms;
 	u16 hid_descriptor_address;
+	const char *main_supply_name;
 };
 
 struct i2c_hid_of_elan {
@@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
 		container_of(ops, struct i2c_hid_of_elan, ops);
 	int ret;
 
-	ret = regulator_enable(ihid_elan->vcc33);
-	if (ret)
-		return ret;
+	if (ihid_elan->vcc33) {
+		ret = regulator_enable(ihid_elan->vcc33);
+		if (ret)
+			return ret;
+	}
 
 	ret = regulator_enable(ihid_elan->vccio);
 	if (ret) {
@@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
 		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);
+	if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
+		msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
 
 	return 0;
 }
@@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
 		container_of(ops, struct i2c_hid_of_elan, ops);
 
 	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+	if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
+		msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
+
 	regulator_disable(ihid_elan->vccio);
-	regulator_disable(ihid_elan->vcc33);
+	if (ihid_elan->vcc33)
+		regulator_disable(ihid_elan->vcc33);
 }
 
 static int i2c_hid_of_elan_probe(struct i2c_client *client)
@@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
 	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);
 
+	if (ihid_elan->chip_data->main_supply_name) {
+		ihid_elan->vcc33 = devm_regulator_get(&client->dev,
+						      ihid_elan->chip_data->main_supply_name);
+		if (IS_ERR(ihid_elan->vcc33))
+			return PTR_ERR(ihid_elan->vcc33);
+	}
+
 	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,
+	.post_gpio_reset_on_delay_ms = 300,
+	.hid_descriptor_address = 0x0001,
+	.main_supply_name = "vcc33",
+};
+
+static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
+	.post_power_delay_ms = 1,
+	.post_gpio_reset_on_delay_ms = 200,
+	.post_gpio_reset_off_delay_ms = 65,
 	.hid_descriptor_address = 0x0001,
+	/*
+	 * this touchscreen is tightly integrated with the panel and assumes
+	 * that the relevant power rails (other than the IO rail) have already
+	 * been turned on by the panel driver because we're a panel follower.
+	 */
+	.main_supply_name = NULL,
 };
 
 static const struct of_device_id elan_i2c_hid_of_match[] = {
 	{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
+	{ .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match);
-- 
2.25.1

Re: [PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t

[Doug Anderson]

Hi,

On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The ili9882t touch screen chip same as Elan eKTH6915 controller
> has a reset gpio. The difference is that ili9882t needs to use
> vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
> and touchscreens to power on/off together, let's add a phandle for this.
>
> [1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  .../bindings/input/ilitek,ili9882t.yaml       | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)

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

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Doug Anderson]

Benjamin,

On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The ili9882t is a TDDI IC (Touch with Display Driver). The
> datasheet specifies there should be 60ms between touch SDA
> sleep and panel RESX. Doug's series[1] allows panels and
> touchscreens to power on/off together, so we can add the 65 ms
> delay in i2c_hid_core_suspend before panel_unprepare.
>
> Because ili9882t touchscrgeen is a panel follower, and
> needs to use vccio-supply instead of vcc33-supply, so set
> it NULL to ili9882t_chip_data, then not use vcc33 regulator.
>
> [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 12 deletions(-)


>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> index 029045d9661c..31abab57ad44 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> @@ -18,9 +18,11 @@
>  #include "i2c-hid.h"
>
>  struct elan_i2c_hid_chip_data {
> -       unsigned int post_gpio_reset_delay_ms;
> +       unsigned int post_gpio_reset_on_delay_ms;
> +       unsigned int post_gpio_reset_off_delay_ms;
>         unsigned int post_power_delay_ms;
>         u16 hid_descriptor_address;
> +       const char *main_supply_name;
>  };
>
>  struct i2c_hid_of_elan {
> @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
>                 container_of(ops, struct i2c_hid_of_elan, ops);
>         int ret;
>
> -       ret = regulator_enable(ihid_elan->vcc33);
> -       if (ret)
> -               return ret;
> +       if (ihid_elan->vcc33) {
> +               ret = regulator_enable(ihid_elan->vcc33);
> +               if (ret)
> +                       return ret;
> +       }
>
>         ret = regulator_enable(ihid_elan->vccio);
>         if (ret) {
> @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
>                 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);
> +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
>
>         return 0;
>  }
> @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
>                 container_of(ops, struct i2c_hid_of_elan, ops);
>
>         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> +
>         regulator_disable(ihid_elan->vccio);
> -       regulator_disable(ihid_elan->vcc33);
> +       if (ihid_elan->vcc33)
> +               regulator_disable(ihid_elan->vcc33);
>  }
>
>  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
>         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);
>
> +       if (ihid_elan->chip_data->main_supply_name) {
> +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> +                                                     ihid_elan->chip_data->main_supply_name);
> +               if (IS_ERR(ihid_elan->vcc33))
> +                       return PTR_ERR(ihid_elan->vcc33);
> +       }
> +
>         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,
> +       .post_gpio_reset_on_delay_ms = 300,
> +       .hid_descriptor_address = 0x0001,
> +       .main_supply_name = "vcc33",
> +};
> +
> +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> +       .post_power_delay_ms = 1,
> +       .post_gpio_reset_on_delay_ms = 200,
> +       .post_gpio_reset_off_delay_ms = 65,
>         .hid_descriptor_address = 0x0001,
> +       /*
> +        * this touchscreen is tightly integrated with the panel and assumes
> +        * that the relevant power rails (other than the IO rail) have already
> +        * been turned on by the panel driver because we're a panel follower.
> +        */
> +       .main_supply_name = NULL,
>  };
>
>  static const struct of_device_id elan_i2c_hid_of_match[] = {
>         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },

Logically, this patch depends on the panel-follower series that's now
landed in drm-misc-next. With your Ack, I'm willing to land these two
patches into drm-misc-next too. Other options:

a) We could land the two patches in the i2c-hid tree since they don't
appear to conflict. The touchscreen won't actually function until the
patches meetup in linux-next but I don't think they'll give any
compile errors (I haven't double-checked that, but I can). ...though
it's possible that the dt bindings might generate errors? Again, I can
investigate if we want to go this way.

b) We can snooze this for a few months and you can pick it to i2c-hid
when my series reaches mainline.

Let me know how you'd like to proceed.

-Doug

Re: [PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t

[Krzysztof Kozlowski]

On 02/08/2023 09:19, Cong Yang wrote:
> The ili9882t touch screen chip same as Elan eKTH6915 controller
> has a reset gpio. The difference is that ili9882t needs to use
> vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
> and touchscreens to power on/off together, let's add a phandle for this.
> 
> [1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  .../bindings/input/ilitek,ili9882t.yaml       | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml

It's v6 but this still misses the changelog.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t

[cong yang]

Hi,Krzysztof:

The changelog is on the cover letter. Thank you.





On Thu, Aug 3, 2023 at 7:12 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/08/2023 09:19, Cong Yang wrote:
> > The ili9882t touch screen chip same as Elan eKTH6915 controller
> > has a reset gpio. The difference is that ili9882t needs to use
> > vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
> > and touchscreens to power on/off together, let's add a phandle for this.
> >
> > [1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org
> >
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  .../bindings/input/ilitek,ili9882t.yaml       | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
>
> It's v6 but this still misses the changelog.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Benjamin Tissoires]

On Aug 02 2023, Doug Anderson wrote:
> Benjamin,
> 
> On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > The ili9882t is a TDDI IC (Touch with Display Driver). The
> > datasheet specifies there should be 60ms between touch SDA
> > sleep and panel RESX. Doug's series[1] allows panels and
> > touchscreens to power on/off together, so we can add the 65 ms
> > delay in i2c_hid_core_suspend before panel_unprepare.
> >
> > Because ili9882t touchscrgeen is a panel follower, and
> > needs to use vccio-supply instead of vcc33-supply, so set
> > it NULL to ili9882t_chip_data, then not use vcc33 regulator.
> >
> > [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> 
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > index 029045d9661c..31abab57ad44 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > @@ -18,9 +18,11 @@
> >  #include "i2c-hid.h"
> >
> >  struct elan_i2c_hid_chip_data {
> > -       unsigned int post_gpio_reset_delay_ms;
> > +       unsigned int post_gpio_reset_on_delay_ms;
> > +       unsigned int post_gpio_reset_off_delay_ms;
> >         unsigned int post_power_delay_ms;
> >         u16 hid_descriptor_address;
> > +       const char *main_supply_name;
> >  };
> >
> >  struct i2c_hid_of_elan {
> > @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> >                 container_of(ops, struct i2c_hid_of_elan, ops);
> >         int ret;
> >
> > -       ret = regulator_enable(ihid_elan->vcc33);
> > -       if (ret)
> > -               return ret;
> > +       if (ihid_elan->vcc33) {
> > +               ret = regulator_enable(ihid_elan->vcc33);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> >         ret = regulator_enable(ihid_elan->vccio);
> >         if (ret) {
> > @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> >                 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);
> > +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> > +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
> >
> >         return 0;
> >  }
> > @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> >                 container_of(ops, struct i2c_hid_of_elan, ops);
> >
> >         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> > +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> > +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> > +
> >         regulator_disable(ihid_elan->vccio);
> > -       regulator_disable(ihid_elan->vcc33);
> > +       if (ihid_elan->vcc33)
> > +               regulator_disable(ihid_elan->vcc33);
> >  }
> >
> >  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> >         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);
> >
> > +       if (ihid_elan->chip_data->main_supply_name) {
> > +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> > +                                                     ihid_elan->chip_data->main_supply_name);
> > +               if (IS_ERR(ihid_elan->vcc33))
> > +                       return PTR_ERR(ihid_elan->vcc33);
> > +       }
> > +
> >         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,
> > +       .post_gpio_reset_on_delay_ms = 300,
> > +       .hid_descriptor_address = 0x0001,
> > +       .main_supply_name = "vcc33",
> > +};
> > +
> > +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> > +       .post_power_delay_ms = 1,
> > +       .post_gpio_reset_on_delay_ms = 200,
> > +       .post_gpio_reset_off_delay_ms = 65,
> >         .hid_descriptor_address = 0x0001,
> > +       /*
> > +        * this touchscreen is tightly integrated with the panel and assumes
> > +        * that the relevant power rails (other than the IO rail) have already
> > +        * been turned on by the panel driver because we're a panel follower.
> > +        */
> > +       .main_supply_name = NULL,
> >  };
> >
> >  static const struct of_device_id elan_i2c_hid_of_match[] = {
> >         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> > +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
> 
> Logically, this patch depends on the panel-follower series that's now
> landed in drm-misc-next. With your Ack, I'm willing to land these two
> patches into drm-misc-next too. Other options:

If you are fine with the code, I think it could go with the drm tree
given that it depends on the panel-follower.

Unless it's too late for you to take 6.6 material (sorry I was off in
August and just came back).

Acked-By: Benjamin Tissoires <bentiss@kernel.org>

Cheers,
Benjamin

> 
> a) We could land the two patches in the i2c-hid tree since they don't
> appear to conflict. The touchscreen won't actually function until the
> patches meetup in linux-next but I don't think they'll give any
> compile errors (I haven't double-checked that, but I can). ...though
> it's possible that the dt bindings might generate errors? Again, I can
> investigate if we want to go this way.
> 
> b) We can snooze this for a few months and you can pick it to i2c-hid
> when my series reaches mainline.
> 
> Let me know how you'd like to proceed.
> 
> -Doug

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Doug Anderson]

Hi,

On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Aug 02 2023, Doug Anderson wrote:
> > Benjamin,
> >
> > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > >
> > > The ili9882t is a TDDI IC (Touch with Display Driver). The
> > > datasheet specifies there should be 60ms between touch SDA
> > > sleep and panel RESX. Doug's series[1] allows panels and
> > > touchscreens to power on/off together, so we can add the 65 ms
> > > delay in i2c_hid_core_suspend before panel_unprepare.
> > >
> > > Because ili9882t touchscrgeen is a panel follower, and
> > > needs to use vccio-supply instead of vcc33-supply, so set
> > > it NULL to ili9882t_chip_data, then not use vcc33 regulator.
> > >
> > > [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
> > >
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > > ---
> > >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
> > >  1 file changed, 38 insertions(+), 12 deletions(-)
> >
> >
> > >
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > index 029045d9661c..31abab57ad44 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > @@ -18,9 +18,11 @@
> > >  #include "i2c-hid.h"
> > >
> > >  struct elan_i2c_hid_chip_data {
> > > -       unsigned int post_gpio_reset_delay_ms;
> > > +       unsigned int post_gpio_reset_on_delay_ms;
> > > +       unsigned int post_gpio_reset_off_delay_ms;
> > >         unsigned int post_power_delay_ms;
> > >         u16 hid_descriptor_address;
> > > +       const char *main_supply_name;
> > >  };
> > >
> > >  struct i2c_hid_of_elan {
> > > @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > >         int ret;
> > >
> > > -       ret = regulator_enable(ihid_elan->vcc33);
> > > -       if (ret)
> > > -               return ret;
> > > +       if (ihid_elan->vcc33) {
> > > +               ret = regulator_enable(ihid_elan->vcc33);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > >
> > >         ret = regulator_enable(ihid_elan->vccio);
> > >         if (ret) {
> > > @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > >                 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);
> > > +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> > > +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
> > >
> > >         return 0;
> > >  }
> > > @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > >
> > >         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> > > +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> > > +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> > > +
> > >         regulator_disable(ihid_elan->vccio);
> > > -       regulator_disable(ihid_elan->vcc33);
> > > +       if (ihid_elan->vcc33)
> > > +               regulator_disable(ihid_elan->vcc33);
> > >  }
> > >
> > >  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > >         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);
> > >
> > > +       if (ihid_elan->chip_data->main_supply_name) {
> > > +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> > > +                                                     ihid_elan->chip_data->main_supply_name);
> > > +               if (IS_ERR(ihid_elan->vcc33))
> > > +                       return PTR_ERR(ihid_elan->vcc33);
> > > +       }
> > > +
> > >         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,
> > > +       .post_gpio_reset_on_delay_ms = 300,
> > > +       .hid_descriptor_address = 0x0001,
> > > +       .main_supply_name = "vcc33",
> > > +};
> > > +
> > > +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> > > +       .post_power_delay_ms = 1,
> > > +       .post_gpio_reset_on_delay_ms = 200,
> > > +       .post_gpio_reset_off_delay_ms = 65,
> > >         .hid_descriptor_address = 0x0001,
> > > +       /*
> > > +        * this touchscreen is tightly integrated with the panel and assumes
> > > +        * that the relevant power rails (other than the IO rail) have already
> > > +        * been turned on by the panel driver because we're a panel follower.
> > > +        */
> > > +       .main_supply_name = NULL,
> > >  };
> > >
> > >  static const struct of_device_id elan_i2c_hid_of_match[] = {
> > >         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> > > +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
> >
> > Logically, this patch depends on the panel-follower series that's now
> > landed in drm-misc-next. With your Ack, I'm willing to land these two
> > patches into drm-misc-next too. Other options:
>
> If you are fine with the code, I think it could go with the drm tree
> given that it depends on the panel-follower.
>
> Unless it's too late for you to take 6.6 material (sorry I was off in
> August and just came back).
>
> Acked-By: Benjamin Tissoires <bentiss@kernel.org>

Thanks for the Ack, but yeah, it's probably too late for drm-misc.
Hopefully this can go through the normal tree after the next -rc1
then. Thanks!

-Doug

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Benjamin Tissoires]

On Aug 21 2023, Doug Anderson wrote:
> Hi,
> 
> On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > On Aug 02 2023, Doug Anderson wrote:
> > > Benjamin,
> > >
> > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > > >
> > > > The ili9882t is a TDDI IC (Touch with Display Driver). The
> > > > datasheet specifies there should be 60ms between touch SDA
> > > > sleep and panel RESX. Doug's series[1] allows panels and
> > > > touchscreens to power on/off together, so we can add the 65 ms
> > > > delay in i2c_hid_core_suspend before panel_unprepare.
> > > >
> > > > Because ili9882t touchscrgeen is a panel follower, and
> > > > needs to use vccio-supply instead of vcc33-supply, so set
> > > > it NULL to ili9882t_chip_data, then not use vcc33 regulator.
> > > >
> > > > [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
> > > >
> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > > > ---
> > > >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
> > > >  1 file changed, 38 insertions(+), 12 deletions(-)
> > >
> > >
> > > >
> > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > index 029045d9661c..31abab57ad44 100644
> > > > --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > @@ -18,9 +18,11 @@
> > > >  #include "i2c-hid.h"
> > > >
> > > >  struct elan_i2c_hid_chip_data {
> > > > -       unsigned int post_gpio_reset_delay_ms;
> > > > +       unsigned int post_gpio_reset_on_delay_ms;
> > > > +       unsigned int post_gpio_reset_off_delay_ms;
> > > >         unsigned int post_power_delay_ms;
> > > >         u16 hid_descriptor_address;
> > > > +       const char *main_supply_name;
> > > >  };
> > > >
> > > >  struct i2c_hid_of_elan {
> > > > @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > >         int ret;
> > > >
> > > > -       ret = regulator_enable(ihid_elan->vcc33);
> > > > -       if (ret)
> > > > -               return ret;
> > > > +       if (ihid_elan->vcc33) {
> > > > +               ret = regulator_enable(ihid_elan->vcc33);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > >
> > > >         ret = regulator_enable(ihid_elan->vccio);
> > > >         if (ret) {
> > > > @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > >                 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);
> > > > +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
> > > >
> > > >         return 0;
> > > >  }
> > > > @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > >
> > > >         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> > > > +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> > > > +
> > > >         regulator_disable(ihid_elan->vccio);
> > > > -       regulator_disable(ihid_elan->vcc33);
> > > > +       if (ihid_elan->vcc33)
> > > > +               regulator_disable(ihid_elan->vcc33);
> > > >  }
> > > >
> > > >  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > > @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > >         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);
> > > >
> > > > +       if (ihid_elan->chip_data->main_supply_name) {
> > > > +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> > > > +                                                     ihid_elan->chip_data->main_supply_name);
> > > > +               if (IS_ERR(ihid_elan->vcc33))
> > > > +                       return PTR_ERR(ihid_elan->vcc33);
> > > > +       }
> > > > +
> > > >         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,
> > > > +       .post_gpio_reset_on_delay_ms = 300,
> > > > +       .hid_descriptor_address = 0x0001,
> > > > +       .main_supply_name = "vcc33",
> > > > +};
> > > > +
> > > > +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> > > > +       .post_power_delay_ms = 1,
> > > > +       .post_gpio_reset_on_delay_ms = 200,
> > > > +       .post_gpio_reset_off_delay_ms = 65,
> > > >         .hid_descriptor_address = 0x0001,
> > > > +       /*
> > > > +        * this touchscreen is tightly integrated with the panel and assumes
> > > > +        * that the relevant power rails (other than the IO rail) have already
> > > > +        * been turned on by the panel driver because we're a panel follower.
> > > > +        */
> > > > +       .main_supply_name = NULL,
> > > >  };
> > > >
> > > >  static const struct of_device_id elan_i2c_hid_of_match[] = {
> > > >         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> > > > +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
> > >
> > > Logically, this patch depends on the panel-follower series that's now
> > > landed in drm-misc-next. With your Ack, I'm willing to land these two
> > > patches into drm-misc-next too. Other options:
> >
> > If you are fine with the code, I think it could go with the drm tree
> > given that it depends on the panel-follower.
> >
> > Unless it's too late for you to take 6.6 material (sorry I was off in
> > August and just came back).
> >
> > Acked-By: Benjamin Tissoires <bentiss@kernel.org>
> 
> Thanks for the Ack, but yeah, it's probably too late for drm-misc.
> Hopefully this can go through the normal tree after the next -rc1
> then. Thanks!

We don't have those strict rules in hid.git. And given that I was in
PTO, I think it's fine if we take the patch now if it's compiling fine
on its own and doesn't break on existing hardware. What are the
consequences of using this patch without the panel-follower series?

Also, does it has enough reviews from the DT folks?

Cheers,
Benjamin

> 
> -Doug

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Doug Anderson]

Hi,

On Mon, Aug 21, 2023 at 7:14 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Aug 21 2023, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> > >
> > > On Aug 02 2023, Doug Anderson wrote:
> > > > Benjamin,
> > > >
> > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > > > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > > > >
> > > > > The ili9882t is a TDDI IC (Touch with Display Driver). The
> > > > > datasheet specifies there should be 60ms between touch SDA
> > > > > sleep and panel RESX. Doug's series[1] allows panels and
> > > > > touchscreens to power on/off together, so we can add the 65 ms
> > > > > delay in i2c_hid_core_suspend before panel_unprepare.
> > > > >
> > > > > Because ili9882t touchscrgeen is a panel follower, and
> > > > > needs to use vccio-supply instead of vcc33-supply, so set
> > > > > it NULL to ili9882t_chip_data, then not use vcc33 regulator.
> > > > >
> > > > > [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
> > > > >
> > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > > > > ---
> > > > >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
> > > > >  1 file changed, 38 insertions(+), 12 deletions(-)
> > > >
> > > >
> > > > >
> > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > index 029045d9661c..31abab57ad44 100644
> > > > > --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > @@ -18,9 +18,11 @@
> > > > >  #include "i2c-hid.h"
> > > > >
> > > > >  struct elan_i2c_hid_chip_data {
> > > > > -       unsigned int post_gpio_reset_delay_ms;
> > > > > +       unsigned int post_gpio_reset_on_delay_ms;
> > > > > +       unsigned int post_gpio_reset_off_delay_ms;
> > > > >         unsigned int post_power_delay_ms;
> > > > >         u16 hid_descriptor_address;
> > > > > +       const char *main_supply_name;
> > > > >  };
> > > > >
> > > > >  struct i2c_hid_of_elan {
> > > > > @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > > >         int ret;
> > > > >
> > > > > -       ret = regulator_enable(ihid_elan->vcc33);
> > > > > -       if (ret)
> > > > > -               return ret;
> > > > > +       if (ihid_elan->vcc33) {
> > > > > +               ret = regulator_enable(ihid_elan->vcc33);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +       }
> > > > >
> > > > >         ret = regulator_enable(ihid_elan->vccio);
> > > > >         if (ret) {
> > > > > @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > > >                 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);
> > > > > +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> > > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
> > > > >
> > > > >         return 0;
> > > > >  }
> > > > > @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> > > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > > >
> > > > >         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> > > > > +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> > > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> > > > > +
> > > > >         regulator_disable(ihid_elan->vccio);
> > > > > -       regulator_disable(ihid_elan->vcc33);
> > > > > +       if (ihid_elan->vcc33)
> > > > > +               regulator_disable(ihid_elan->vcc33);
> > > > >  }
> > > > >
> > > > >  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > > > @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > > >         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);
> > > > >
> > > > > +       if (ihid_elan->chip_data->main_supply_name) {
> > > > > +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> > > > > +                                                     ihid_elan->chip_data->main_supply_name);
> > > > > +               if (IS_ERR(ihid_elan->vcc33))
> > > > > +                       return PTR_ERR(ihid_elan->vcc33);
> > > > > +       }
> > > > > +
> > > > >         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,
> > > > > +       .post_gpio_reset_on_delay_ms = 300,
> > > > > +       .hid_descriptor_address = 0x0001,
> > > > > +       .main_supply_name = "vcc33",
> > > > > +};
> > > > > +
> > > > > +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> > > > > +       .post_power_delay_ms = 1,
> > > > > +       .post_gpio_reset_on_delay_ms = 200,
> > > > > +       .post_gpio_reset_off_delay_ms = 65,
> > > > >         .hid_descriptor_address = 0x0001,
> > > > > +       /*
> > > > > +        * this touchscreen is tightly integrated with the panel and assumes
> > > > > +        * that the relevant power rails (other than the IO rail) have already
> > > > > +        * been turned on by the panel driver because we're a panel follower.
> > > > > +        */
> > > > > +       .main_supply_name = NULL,
> > > > >  };
> > > > >
> > > > >  static const struct of_device_id elan_i2c_hid_of_match[] = {
> > > > >         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> > > > > +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
> > > >
> > > > Logically, this patch depends on the panel-follower series that's now
> > > > landed in drm-misc-next. With your Ack, I'm willing to land these two
> > > > patches into drm-misc-next too. Other options:
> > >
> > > If you are fine with the code, I think it could go with the drm tree
> > > given that it depends on the panel-follower.
> > >
> > > Unless it's too late for you to take 6.6 material (sorry I was off in
> > > August and just came back).
> > >
> > > Acked-By: Benjamin Tissoires <bentiss@kernel.org>
> >
> > Thanks for the Ack, but yeah, it's probably too late for drm-misc.
> > Hopefully this can go through the normal tree after the next -rc1
> > then. Thanks!
>
> We don't have those strict rules in hid.git. And given that I was in
> PTO, I think it's fine if we take the patch now if it's compiling fine
> on its own and doesn't break on existing hardware. What are the
> consequences of using this patch without the panel-follower series?

I think it should be fine.

I actually tried running `make dt_binding_check
DT_SCHEMA_FILES=ilitek,ili9882t.yaml` with just this bindings file and
I actually _didn't_ get an error, so that's good. I guess it still
verifies OK even without commit 2ca376ef18f6 ("dt-bindings: HID:
i2c-hid: Add "panel" property to i2c-hid backed touchscreens"). I
guess the "panel: true" is enough for it to at least not complain...
;-)

So I think there's no downside to landing this in the i2c-hid tree. As
I mentioned before, this panel won't actually be functional without
the panel follower code, but once the two meetup in linuxnext we'll
end up with something that works. :-)


> Also, does it has enough reviews from the DT folks?

The bindings have Krzysztof's review and that's the important one. I
believe Krzysztof was unhappy that Cong Yang hasn't been including
version history in each individual patch, but he did provide a
reviewed by on v5 [1]

[1] https://lore.kernel.org/all/949a2d21-eb14-3ef8-a7be-9c12152cd15a@linaro.org/

Re: [PATCH v6 0/2] Add ili9882t bindings and timing

[Benjamin Tissoires]

On Wed, 02 Aug 2023 15:19:45 +0800, Cong Yang wrote:
> Add bindings for Ilitek. The ili9882t touch screen chip same as
> Elan eKTH6915 controller has a reset gpio. The difference is that
> ilitek9882 needs to use vccio-supply instead of vcc33-supply.
> From Dmitry suggestion, it would make more sense to distinguish the
> binging of ili9882 and eKTH6915.
> 
> From The datasheet specifies there should be 60ms between touch SDA
> sleep and panel RESX. so we can add the 65 ms delay in i2c_hid_core_suspend.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.6/elan), thanks!

[1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t
      https://git.kernel.org/hid/hid/c/7d3b0d9ebddd
[2/2] HID: i2c-hid: elan: Add ili9882t timing
      https://git.kernel.org/hid/hid/c/f2f43bf15d7a

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>

Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing

[Benjamin Tissoires]

On Aug 21 2023, Doug Anderson wrote:
> Hi,
> 
> On Mon, Aug 21, 2023 at 7:14 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > On Aug 21 2023, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> > > >
> > > > On Aug 02 2023, Doug Anderson wrote:
> > > > > Benjamin,
> > > > >
> > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > > > > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > > > > >
> > > > > > The ili9882t is a TDDI IC (Touch with Display Driver). The
> > > > > > datasheet specifies there should be 60ms between touch SDA
> > > > > > sleep and panel RESX. Doug's series[1] allows panels and
> > > > > > touchscreens to power on/off together, so we can add the 65 ms
> > > > > > delay in i2c_hid_core_suspend before panel_unprepare.
> > > > > >
> > > > > > Because ili9882t touchscrgeen is a panel follower, and
> > > > > > needs to use vccio-supply instead of vcc33-supply, so set
> > > > > > it NULL to ili9882t_chip_data, then not use vcc33 regulator.
> > > > > >
> > > > > > [1]: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org
> > > > > >
> > > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > > > > > ---
> > > > > >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 50 ++++++++++++++++++++-------
> > > > > >  1 file changed, 38 insertions(+), 12 deletions(-)
> > > > >
> > > > >
> > > > > >
> > > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > > index 029045d9661c..31abab57ad44 100644
> > > > > > --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > > +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> > > > > > @@ -18,9 +18,11 @@
> > > > > >  #include "i2c-hid.h"
> > > > > >
> > > > > >  struct elan_i2c_hid_chip_data {
> > > > > > -       unsigned int post_gpio_reset_delay_ms;
> > > > > > +       unsigned int post_gpio_reset_on_delay_ms;
> > > > > > +       unsigned int post_gpio_reset_off_delay_ms;
> > > > > >         unsigned int post_power_delay_ms;
> > > > > >         u16 hid_descriptor_address;
> > > > > > +       const char *main_supply_name;
> > > > > >  };
> > > > > >
> > > > > >  struct i2c_hid_of_elan {
> > > > > > @@ -38,9 +40,11 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > > > >         int ret;
> > > > > >
> > > > > > -       ret = regulator_enable(ihid_elan->vcc33);
> > > > > > -       if (ret)
> > > > > > -               return ret;
> > > > > > +       if (ihid_elan->vcc33) {
> > > > > > +               ret = regulator_enable(ihid_elan->vcc33);
> > > > > > +               if (ret)
> > > > > > +                       return ret;
> > > > > > +       }
> > > > > >
> > > > > >         ret = regulator_enable(ihid_elan->vccio);
> > > > > >         if (ret) {
> > > > > > @@ -52,8 +56,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> > > > > >                 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);
> > > > > > +       if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
> > > > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
> > > > > >
> > > > > >         return 0;
> > > > > >  }
> > > > > > @@ -64,8 +68,12 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> > > > > >                 container_of(ops, struct i2c_hid_of_elan, ops);
> > > > > >
> > > > > >         gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> > > > > > +       if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> > > > > > +               msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
> > > > > > +
> > > > > >         regulator_disable(ihid_elan->vccio);
> > > > > > -       regulator_disable(ihid_elan->vcc33);
> > > > > > +       if (ihid_elan->vcc33)
> > > > > > +               regulator_disable(ihid_elan->vcc33);
> > > > > >  }
> > > > > >
> > > > > >  static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > > > > @@ -89,24 +97,42 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > > > >         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);
> > > > > >
> > > > > > +       if (ihid_elan->chip_data->main_supply_name) {
> > > > > > +               ihid_elan->vcc33 = devm_regulator_get(&client->dev,
> > > > > > +                                                     ihid_elan->chip_data->main_supply_name);
> > > > > > +               if (IS_ERR(ihid_elan->vcc33))
> > > > > > +                       return PTR_ERR(ihid_elan->vcc33);
> > > > > > +       }
> > > > > > +
> > > > > >         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,
> > > > > > +       .post_gpio_reset_on_delay_ms = 300,
> > > > > > +       .hid_descriptor_address = 0x0001,
> > > > > > +       .main_supply_name = "vcc33",
> > > > > > +};
> > > > > > +
> > > > > > +static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
> > > > > > +       .post_power_delay_ms = 1,
> > > > > > +       .post_gpio_reset_on_delay_ms = 200,
> > > > > > +       .post_gpio_reset_off_delay_ms = 65,
> > > > > >         .hid_descriptor_address = 0x0001,
> > > > > > +       /*
> > > > > > +        * this touchscreen is tightly integrated with the panel and assumes
> > > > > > +        * that the relevant power rails (other than the IO rail) have already
> > > > > > +        * been turned on by the panel driver because we're a panel follower.
> > > > > > +        */
> > > > > > +       .main_supply_name = NULL,
> > > > > >  };
> > > > > >
> > > > > >  static const struct of_device_id elan_i2c_hid_of_match[] = {
> > > > > >         { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
> > > > > > +       { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
> > > > >
> > > > > Logically, this patch depends on the panel-follower series that's now
> > > > > landed in drm-misc-next. With your Ack, I'm willing to land these two
> > > > > patches into drm-misc-next too. Other options:
> > > >
> > > > If you are fine with the code, I think it could go with the drm tree
> > > > given that it depends on the panel-follower.
> > > >
> > > > Unless it's too late for you to take 6.6 material (sorry I was off in
> > > > August and just came back).
> > > >
> > > > Acked-By: Benjamin Tissoires <bentiss@kernel.org>
> > >
> > > Thanks for the Ack, but yeah, it's probably too late for drm-misc.
> > > Hopefully this can go through the normal tree after the next -rc1
> > > then. Thanks!
> >
> > We don't have those strict rules in hid.git. And given that I was in
> > PTO, I think it's fine if we take the patch now if it's compiling fine
> > on its own and doesn't break on existing hardware. What are the
> > consequences of using this patch without the panel-follower series?
> 
> I think it should be fine.
> 
> I actually tried running `make dt_binding_check
> DT_SCHEMA_FILES=ilitek,ili9882t.yaml` with just this bindings file and
> I actually _didn't_ get an error, so that's good. I guess it still
> verifies OK even without commit 2ca376ef18f6 ("dt-bindings: HID:
> i2c-hid: Add "panel" property to i2c-hid backed touchscreens"). I
> guess the "panel: true" is enough for it to at least not complain...
> ;-)
> 
> So I think there's no downside to landing this in the i2c-hid tree. As
> I mentioned before, this panel won't actually be functional without
> the panel follower code, but once the two meetup in linuxnext we'll
> end up with something that works. :-)
> 
> 
> > Also, does it has enough reviews from the DT folks?
> 
> The bindings have Krzysztof's review and that's the important one. I
> believe Krzysztof was unhappy that Cong Yang hasn't been including
> version history in each individual patch, but he did provide a
> reviewed by on v5 [1]

Great!

thanks for the confirmation of the tests. As you should have seen in my
reply in 0/2, the patches are now applied.

Cheers,
Benjamin

> 
> [1] https://lore.kernel.org/all/949a2d21-eb14-3ef8-a7be-9c12152cd15a@linaro.org/