Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[cong yang <yangcong5@huaqin.corp-partner.google.com>]

Hi,

On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > At present, we have found that there may be a problem of blurred
> > screen during fast sleep/resume. The direct cause of the blurred
> > screen is that the IC does not receive 0x28/0x10. Because of the
> > particularity of the IC, before the panel enters sleep hid must
> > stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> > This doesn't look very spec-compliant.
>
> Presumably you could be more spec compliant if we used
> "panel_follower" in this case? Would that be a better solution?

In the "panel_follower" solution, the phenomenon is the same.
The current order is
ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare,
ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before
ili9882t_disable.
>
>
> > So in order to solve this
> > problem, the IC can handle it through the exception mechanism when
> > it cannot receive 0X28/0X10 command. Handling exceptions requires a reset
> > 50ms delay. Refer to vendor detailed analysis [1].
> >
> > Ilitek vendor also suggested switching the page before entering sleep to
> > avoid panel IC not receiving 0x28/0x10 command.
> >
> > Note: 0x28 is display off, 0x10 is sleep in.
> >
> > [1]: https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence
> >
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > index bbfcffe65623..0a1dd987b204 100644
> > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel)
> >         return container_of(panel, struct ili9882t, base);
> >  }
> >
> > +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> > +{
> > +       u8 switch_cmd[] = {0x98, 0x82, 0x00};
>
> Can't you just replace the last 0x00 above with "page" and get rid of
> the manual assignment below?
>
>
> > +       int ret;
> > +
> > +       switch_cmd[2] = page;
> > +
> > +       ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3);
>
> Instead of hardcoding 3, should use ARRAY_SIZE().
>
>
> > +       if (ret) {
> > +               dev_err(&dsi->dev,
> > +                       "error switching panel controller page (%d)\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
>
> optional: It feels like it would be nice to somehow use the
> "_INIT_SWITCH_PAGE_CMD" macro I suggested in patch #1 instead of
> having to hardcode 0x98, 0x82 again. In patch #1 I already suggested
> breaking out the function to send a sequence of commands. If you had
> that function take a pointer instead of hardcoding it to look at
> ->init_cmds then you could probably use the same function that you do
> at init time?
>
>
> >  static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> >  {
> >         struct mipi_dsi_device *dsi = ili->dsi;
> > @@ -444,8 +461,10 @@ static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> >  static int ili9882t_disable(struct drm_panel *panel)
> >  {
> >         struct ili9882t *ili = to_ili9882t(panel);
> > +       struct mipi_dsi_device *dsi = ili->dsi;
> >         int ret;
> >
> > +       ili9882t_switch_page(dsi, 0x00);
> >         ret = ili9882t_enter_sleep_mode(ili);
> >         if (ret < 0) {
> >                 dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> > @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> >         gpiod_set_value(ili->enable_gpio, 1);
> >         usleep_range(1000, 2000);
> >         gpiod_set_value(ili->enable_gpio, 0);
> > -       usleep_range(1000, 2000);
> > +       usleep_range(40000, 50000);
>
> nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
> uses the longer delay, so that'll save ~9 ms. The only reason for the
> range is to optimize kernel wakeups which is really not a concern
> here.

We need 50ms delay to meet the requirement.