1. Patchew
  2. linux
  3. View series

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

Cong Yang posted 1 patch 2 years, 8 months ago 
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH] HID: i2c-hid: goodix: Add ili9882t timing
Posted by Cong Yang 2 years, 8 months ago 
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
Posted by Doug Anderson 2 years, 8 months ago 
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
Posted by Cong Yang 2 years, 8 months ago 
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
Posted by Cong Yang 2 years, 8 months ago 
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
Posted by Doug Anderson 2 years, 8 months ago 
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
Posted by Dmitry Torokhov 2 years, 8 months ago 
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
Posted by Cong Yang 2 years, 8 months ago 
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
Posted by Doug Anderson 2 years, 8 months ago 
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
Posted by Krzysztof Kozlowski 2 years, 8 months ago 
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
Posted by Doug Anderson 2 years, 8 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.