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

Dmitry Baryshkov
2023-05-22T13:47:24Z

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@quicinc.com>
>>> ---
>>>   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