Hi Doug, thanks for your review! I fixed most things except this: On Fri, Jun 25, 2021 at 6:42 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > +static int ws2401_disable(struct drm_panel *panel) > > +{ > > + struct ws2401 *ws = to_ws2401(panel); > > + > > + ws2401_command(ws, MIPI_DCS_SET_DISPLAY_OFF); > > + msleep(25); > > It feels weird / arbitrary the split between "disable" and "unprepare" > on this panel driver compared to the "db7430.c" one. In the other > driver you put the sleep mode here and in this driver you put the > sleep mode un "unpreapre". Is that for a reason, or just arbitrary? > Can it be consistent between the two drivers? > > I guess maybe this is because in "db7430" the power up order was > slightly different? This arbitrary sequence ordering is part of why we can't just have one display driver to rule them all... sadly they also make semantic sense. The vendor driver needs the display to come out of sleep right after the power-on to send the init sequence. The vendor driver does exactly this. The placement of these sleep mode in prepare/unprepare calls is because of this. Yours, Linus Walleij