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@xxxxxxxxxx> wrote:
> >
> > On Aug 02 2023, Doug Anderson wrote:
> > > Benjamin,
> > >
> > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> > > <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx
> > > >
> > > > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > Signed-off-by: Cong Yang <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxx>
> 
> 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