| Message ID | 20230802071947.1683318-3-yangcong5@huaqin.corp-partner.google.com |
|---|---|
| State | Accepted |
| Commit | f2f43bf15d7aa3286eced18d5199ee579e2c2614 |
| Headers | show |
| Series | Add ili9882t bindings and timing | expand |
On Aug 02 2023, Doug Anderson wrote: > Benjamin, > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang > <yangcong5@huaqin.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@chromium.org> > > Signed-off-by: Cong Yang <yangcong5@huaqin.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@kernel.org> 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
Hi, On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > On Aug 02 2023, Doug Anderson wrote: > > Benjamin, > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang > > <yangcong5@huaqin.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@chromium.org> > > > Signed-off-by: Cong Yang <yangcong5@huaqin.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@kernel.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! -Doug
On Aug 21 2023, Doug Anderson wrote: > Hi, > > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > On Aug 02 2023, Doug Anderson wrote: > > > Benjamin, > > > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang > > > <yangcong5@huaqin.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@chromium.org> > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.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@kernel.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? Also, does it has enough reviews from the DT folks? Cheers, Benjamin > > -Doug
Hi, On Mon, Aug 21, 2023 at 7:14 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > On Aug 21 2023, Doug Anderson wrote: > > Hi, > > > > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > On Aug 02 2023, Doug Anderson wrote: > > > > Benjamin, > > > > > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang > > > > <yangcong5@huaqin.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@chromium.org> > > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.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@kernel.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] [1] https://lore.kernel.org/all/949a2d21-eb14-3ef8-a7be-9c12152cd15a@linaro.org/
On Aug 21 2023, Doug Anderson wrote: > Hi, > > On Mon, Aug 21, 2023 at 7:14 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > On Aug 21 2023, Doug Anderson wrote: > > > Hi, > > > > > > On Mon, Aug 21, 2023 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > > > On Aug 02 2023, Doug Anderson wrote: > > > > > Benjamin, > > > > > > > > > > On Wed, Aug 2, 2023 at 12:20 AM Cong Yang > > > > > <yangcong5@huaqin.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@chromium.org> > > > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.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@kernel.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/
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 }, { } }; MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match);