Benjamin,
On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
<[email protected]> 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/[email protected]
>
> Reviewed-by: Douglas Anderson <[email protected]>
> Signed-off-by: Cong Yang <[email protected]>
> ---
> 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
On Aug 02 2023, Doug Anderson wrote:
> Benjamin,
>
> On Wed, Aug 2, 2023 at 12:20 AM Cong Yang
> <[email protected]> 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/[email protected]
> >
> > Reviewed-by: Douglas Anderson <[email protected]>
> > Signed-off-by: Cong Yang <[email protected]>
> > ---
> > 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 <[email protected]>
Cheers,
Benjamin
>
> 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