Re: [PATCH 2/2] drm/panel: Add driver for Visionox r66451 panel

[Dmitry Baryshkov]

On 22/05/2023 12:10, Neil Armstrong wrote:
> On 18/05/2023 02:19, Dmitry Baryshkov wrote:
> > On 16/05/2023 23:20, Jessica Zhang wrote:
> > > Add support for the 1080x2340 Visionox R66451 AMOLED DSI panel that
> > > comes with the Qualcomm HDK8350 display expansion pack.
> > >
> > > The panel enables display compression (DSC v1.2) by default.
> > >
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/panel/Kconfig                 |   8 +
> > >   drivers/gpu/drm/panel/Makefile                |   1 +
> > >   drivers/gpu/drm/panel/panel-visionox-r66451.c | 395 
> > > ++++++++++++++++++++++++++
> > >   3 files changed, 404 insertions(+)

[skipped]

> > > +
> > > +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > > +    if (ret < 0) {
> > > +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > > +        return ret;
> > > +    }
> > > +    msleep(120);
> > > +
> > > +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >
> > Should the mipi_dsi_dcs_set_display_on() (and maybe exit sleep mode) 
> > be a a part of _enable()? Correspondingly _off should IMO be a part of 
> > _disable callback().
>
> Nop, mipi_dsi_dcs_set_display_on() should be part of prepare, to be 
> called *before* the video stream starts, enable is called after in the 
> atomic modeset chain,

To summarize our discussion: some controllers do not support sending 
commands in non-LPM mode. Thus the panel driver should send all commands 
before the DSI host switches from LPM to VIDEO mode.

Is this corret?

> > > +    if (ret < 0) {
> > > +        dev_err(dev, "Failed on set display on: %d\n", ret);
> > > +        return ret;
> > > +    }
> > > +    msleep(20);
> >
> >
> > Do we need to add here the following line?
> >
> > dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> No, it will be changed by the DSI core.

Hmm, where?

> > > +
> > > +    return 0;
> > > +}

[skipped]

> > > +static int visionox_r66451_bl_update_status(struct backlight_device 
> > > *bl)
> > > +{
> > > +    struct mipi_dsi_device *dsi = bl_get_data(bl);
> > > +    u16 brightness = backlight_get_brightness(bl);
> > > +
> > > +    return mipi_dsi_dcs_set_display_brightness(dsi, 
> > > cpu_to_le16(brightness));
> >
> > mipi_dsi_dcs_set_display_brightness() already converts the brightness, 
> > so you don't need cpu_to_le16 here.
>
>
> The _large variant must be used instead.

But cpu_to_le16 is NOP, isn't it?

--
With best wishes
Dmitry