| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <xvrraazybsoqzdnmuk6y75lypm75gsll6zygd6jdl7zarbqsfo@tfqfnovhgoh5>
Date: Mon, 21 Aug 2023 18:57:00 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Doug Anderson <dianders@...gle.com>
Cc: Cong Yang <yangcong5@...qin.corp-partner.google.com>,
benjamin.tissoires@...hat.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
dmitry.torokhov@...il.com, jikos@...nel.org, hsinyi@...gle.com,
linux-input@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] HID: i2c-hid: elan: Add ili9882t timing
On Aug 21 2023, Doug Anderson wrote:
> Hi,
>
> On Mon, Aug 21, 2023 at 7:14 AM Benjamin Tissoires <bentiss@...nel.org> wrote:
> >
> > On Aug 21 2023, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@...nel.org> wrote:
> > > >
> > > > On Aug 02 2023, Doug Anderson wrote:
> > > > > Benjamin,
> > > > >
> > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > > > > <yangcong5@...qin.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@...omium.org>
> > > > > > Signed-off-by: Cong Yang <yangcong5@...qin.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@...nel.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/
Powered by blists - more mailing lists