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: 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
