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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2e17e84d-aced-1f28-990f-689c8c56ce45@linaro.org>
Date:   Mon, 22 May 2023 16:47:17 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     neil.armstrong@...aro.org,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     Abhinav Kumar <quic_abhinavk@...cinc.com>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drm/panel: Add driver for Visionox r66451 panel

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@...cinc.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