[PATCH] HID: i2c-hid: goodix: Add ili9882t timing

[PATCH] HID: i2c-hid: goodix: Add ili9882t timing

[Cong Yang]

The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
panel reset gpio to be high before i2c commands. Use a longer delay in
post_power_delay_ms to ensure the poweron sequence.

Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..c5870b683a26 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -101,8 +101,14 @@ static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
 	.post_gpio_reset_delay_ms = 180,
 };
 
+static const struct goodix_i2c_hid_timing_data ilitek_ili9882t_timing_data = {
+	.post_power_delay_ms = 200,
+	.post_gpio_reset_delay_ms = 180,
+};
+
 static const struct of_device_id goodix_i2c_hid_of_match[] = {
 	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
+	{ .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_timing_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
-- 
2.25.1

Re: [PATCH] HID: i2c-hid: goodix: Add ili9882t timing

[Doug Anderson]

Hi,

On Fri, May 19, 2023 at 2:02 AM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
> panel reset gpio to be high before i2c commands. Use a longer delay in
> post_power_delay_ms to ensure the poweron sequence.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Two comments:

1. You need to submit a bindings patch to document your
"ilitek,ili9882t" compatible string.

2. I would tend to add the support to the "i2c-hid-of-elan.c" driver
instead of the goodix one. Probably the drivers need to combined again
(I'll see if I can post a patch for that before too long), but if I
were picking one I'd pick the elan one, I think.

-Doug

[v2 0/2] Add ili9882t timing

[Cong Yang]

Compare V1,move ili9882t timing to the "i2c-hid-of-elan.c" driver.
Add "ilitek,ili9882t" compatible string to bindings document.

Cong Yang (2):
  HID: i2c-hid: elan: Add ili9882t timing
  dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

 .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
 drivers/hid/i2c-hid/i2c-hid-of-elan.c                    | 7 +++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.25.1

[v2 1/2] HID: i2c-hid: elan: Add ili9882t timing

[Cong Yang]

The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
panel reset gpio to be high before i2c commands. Use a longer delay in
post_power_delay_ms to ensure the poweron sequence.

Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 drivers/hid/i2c-hid/i2c-hid-of-elan.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 76ddc8be1cbb..dd2435270e73 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -105,8 +105,15 @@ static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
 	.hid_descriptor_address = 0x0001,
 };
 
+static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
+	.post_power_delay_ms = 200,
+	.post_gpio_reset_delay_ms = 180,
+	.hid_descriptor_address = 0x0001,
+};
+
 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: [v2 1/2] HID: i2c-hid: elan: Add ili9882t timing

[Doug Anderson]

Hi,

On Fri, May 19, 2023 at 10:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
> panel reset gpio to be high before i2c commands. Use a longer delay in
> post_power_delay_ms to ensure the poweron sequence.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 7 +++++++
>  1 file changed, 7 insertions(+)

This seems OK to me. The one thing I'd also do is to update the
Kconfig description to say that this driver is also used for Ilitek. I
think it's fine to keep the symbol name as I2C_HID_OF_ELAN but just
change the description.

-Doug

Re: [v2 1/2] HID: i2c-hid: elan: Add ili9882t timing

[Dmitry Torokhov]

On Mon, May 22, 2023 at 08:27:38AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 19, 2023 at 10:07 PM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
> > panel reset gpio to be high before i2c commands. Use a longer delay in
> > post_power_delay_ms to ensure the poweron sequence.
> >
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> 
> This seems OK to me. The one thing I'd also do is to update the
> Kconfig description to say that this driver is also used for Ilitek. I
> think it's fine to keep the symbol name as I2C_HID_OF_ELAN but just
> change the description.

Does ilitek have the same set of regulators, etc, or is it only the
timing? I'd probably make it a separate mini-driver...

Thanks.

-- 
Dmitry

[v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

[Cong Yang]

Add an ilitek touch screen chip ili9882t.

Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index 05e6f2df604c..29eb63b2360b 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -15,11 +15,16 @@ description:
 
 properties:
   compatible:
-    items:
+    oneOf:
       - const: elan,ekth6915
+      - items:
+          - const: elan,ekth6915
+          - const: ilitek,ili9882t
 
   reg:
-    const: 0x10
+    enum:
+      - 0x10
+      - 0x41
 
   interrupts:
     maxItems: 1
-- 
2.25.1

Re: [v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

[Doug Anderson]

Hi,

On Fri, May 19, 2023 at 10:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> Add an ilitek touch screen chip ili9882t.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

I'm curious about the DT maintainers opinion here. Should this be a
new bindings file, or should it be together in the elan file. If
nothing else, I think the secondary voltage rail name is wrong. I took
a quick peek at a datasheet I found and I don't even see a 3.3V rail
going to the ili9882t. That makes it weird to reuse "vcc33-supply" for
a second supply...

-Doug

Re: [v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

[Krzysztof Kozlowski]

On 22/05/2023 17:33, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 19, 2023 at 10:07 PM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
>>
>> Add an ilitek touch screen chip ili9882t.
>>
>> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
>> ---
>>  .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> I'm curious about the DT maintainers opinion here. Should this be a
> new bindings file, or should it be together in the elan file. If
> nothing else, I think the secondary voltage rail name is wrong. I took
> a quick peek at a datasheet I found and I don't even see a 3.3V rail
> going to the ili9882t. That makes it weird to reuse "vcc33-supply" for
> a second supply...

It's easier if they are CCed...

Best regards,
Krzysztof

Re: [v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

[Doug Anderson]

Hi,

On Tue, May 30, 2023 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 22/05/2023 17:33, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 19, 2023 at 10:07 PM Cong Yang
> > <yangcong5@huaqin.corp-partner.google.com> wrote:
> >>
> >> Add an ilitek touch screen chip ili9882t.
> >>
> >> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> >> ---
> >>  .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > I'm curious about the DT maintainers opinion here. Should this be a
> > new bindings file, or should it be together in the elan file. If
> > nothing else, I think the secondary voltage rail name is wrong. I took
> > a quick peek at a datasheet I found and I don't even see a 3.3V rail
> > going to the ili9882t. That makes it weird to reuse "vcc33-supply" for
> > a second supply...
>
> It's easier if they are CCed...

Crud. I just assumed and didn't check the CC list. Cong: can you
resend and make sure you're CCing the people that get_maintainers
points at. One way to find that would be:

./scripts/get_maintainer.pl -f
Documentation/devicetree/bindings/input/elan,ekth6915.yaml

Re: [v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

[Doug Anderson]

Hi,

On Wed, May 31, 2023 at 8:06 PM cong yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> Thanks, I'll keep an eye on that next time. This patch can be discarded,.After adding this series https://lore.kernel.org/r/20230523193017.4109557-1-dianders@chromium.org，

Thanks! I'll see if I can give that series a spin soon and then see
how we can make progress to getting it landed.


> using ekth6915  also can meet my needs.

Even if using ekth6915 can meet your needs, it's still better to
actually add the right compatible string. Putting in the device tree
that you have an "elan6915" and that you're providing "vcc33" isn't
the best when you actually have a different touchscreen and are
providing a very different voltage. Adding the proper bindings is
definitely preferred.


> On Wed, May 31, 2023 at 12:58 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Tue, May 30, 2023 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> >
>> > On 22/05/2023 17:33, Doug Anderson wrote:
>> > > Hi,
>> > >
>> > > On Fri, May 19, 2023 at 10:07 PM Cong Yang
>> > > <yangcong5@huaqin.corp-partner.google.com> wrote:
>> > >>
>> > >> Add an ilitek touch screen chip ili9882t.
>> > >>
>> > >> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
>> > >> ---
>> > >>  .../devicetree/bindings/input/elan,ekth6915.yaml         | 9 +++++++--
>> > >>  1 file changed, 7 insertions(+), 2 deletions(-)
>> > >
>> > > I'm curious about the DT maintainers opinion here. Should this be a
>> > > new bindings file, or should it be together in the elan file. If
>> > > nothing else, I think the secondary voltage rail name is wrong. I took
>> > > a quick peek at a datasheet I found and I don't even see a 3.3V rail
>> > > going to the ili9882t. That makes it weird to reuse "vcc33-supply" for
>> > > a second supply...
>> >
>> > It's easier if they are CCed...
>>
>> Crud. I just assumed and didn't check the CC list. Cong: can you
>> resend and make sure you're CCing the people that get_maintainers
>> points at. One way to find that would be:
>>
>> ./scripts/get_maintainer.pl -f
>> Documentation/devicetree/bindings/input/elan,ekth6915.yaml