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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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