From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Jianhua Lu <lujianhua000@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Sam Ravnborg <sam@ravnborg.org>, David Airlie <airlied@gmail.com>,
Daniel Vetter <daniel@ffwll.ch>, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Linus Walleij <linus.walleij@linaro.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
phone-devel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] drm/panel: Add driver for Novatek NT36523
Date: Wed, 8 Mar 2023 13:33:39 +0100 [thread overview]
Message-ID: <1cbe9e29-13a4-574e-6d8c-b2506e7a36b3@linaro.org> (raw)
In-Reply-To: <ZAh3MSpQ30YyPAVe@Gentoo>
On 8.03.2023 12:53, Jianhua Lu wrote:
> On Wed, Mar 08, 2023 at 12:13:53PM +0100, Konrad Dybcio wrote:
>>
>>
>> On 8.03.2023 05:37, Jianhua Lu wrote:
>>> Add a driver for panels using the Novatek NT36523 display driver IC.
>>>
>>> Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
>>> ---
>>> Changes in v3:
>>> - Refactor source code
>>>
>>> Changes in v2:
>>> - Refactor and clean up source code
>> This is a veeery vague changelog, akin to "change patch", please
>> be more specific the next time around.
>>
> Acked, thanks
>>
>> [...]
>>
>>> +#define DSI_NUM_MIN 1
>>> +
>>> +/* Macro modified from mipi_dual_dsi_dcs_write_seq */
>>> +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \
>>> + do { \
>>> + static const u8 d[] = { cmd, seq }; \
>>> + int i, ret; \
>>> + for (i = 0; i < ARRAY_SIZE(dsi); i++) { \
>>> + ret = mipi_dsi_dcs_write_buffer(dsi[i], d, ARRAY_SIZE(d)); \
>>> + if (ret < 0) { \
>>> + dev_err_ratelimited( \
>>> + &dsi[i]->dev, "sending command %#02x failed: %d\n", \
>>> + cmd, ret); \
>>> + return ret; \
>>> + } \
>>> + } \
>>> + } while (0)
>>> +
>> This should definitely be put in a common file..
> Acked.
>>
>>
>>> +static const struct drm_display_mode elish_boe_mode = {
>>> + /* Current max freqency is 104HZ, TODO 120HZ */
>> Is it a DPU issue, or does the panel not work correctly when you
>> jack up the clocks? Did you use the correct init sequence and porches
>> for the 120Hz mode, including the mode switch command set?
>>
> panel will turn into bluescreen when freqency overs 104HZ, downstream
> seems use a dsi-cphy enhance feature to make it working. Init sequence
> and porches are right
Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
phy-type:
description: D-PHY (default) or C-PHY mode
enum: [ 10, 11 ]
default: 10
try setting that to 11 under your DSI PHYs (or one? not sure).
Konrad
>
>> Could you also implement a 60 (or whatever other value is also
>> implemented downstream) Hz mode?
> I will try to implement it.
>>
>>> + .clock = (1600 + 60 + 8 + 60) * (2560 + 26 + 4 + 168) * 104 / 1000,
>>> + .hdisplay = 1600,
>>> + .hsync_start = 1600 + 60,
>>> + .hsync_end = 1600 + 60 + 8,
>>> + .htotal = 1600 + 60 + 8 + 60,
>>> + .vdisplay = 2560,
>>> + .vsync_start = 2560 + 26,
>>> + .vsync_end = 2560 + 26 + 4,
>>> + .vtotal = 2560 + 26 + 4 + 168,
>>> +};
>>> +
>>> +static const struct drm_display_mode elish_csot_mode = {
>>> + /* Current max freqency is 104HZ, TODO 120HZ */
>>> + .clock = (1600 + 200 + 40 + 52) * (2560 + 26 + 4 + 168) * 104 / 1000,
>>> + .hdisplay = 1600,
>>> + .hsync_start = 1600 + 200,
>>> + .hsync_end = 1600 + 200 + 40,
>>> + .htotal = 1600 + 200 + 40 + 52,
>>> + .vdisplay = 2560,
>>> + .vsync_start = 2560 + 26,
>>> + .vsync_end = 2560 + 26 + 4,
>>> + .vtotal = 2560 + 26 + 4 + 168,
>>> +};
>>> +
>>> +static const struct panel_desc elish_boe_desc = {
>>> + .modes = &elish_boe_mode,
>>> + .dsi_info = {
>>> + .type = "NT36523",
>> That's a bit vague, the driver IC is not very telling about the
>> panel itself. Since we're not able to determine much more information,
>> this could contain the panel manufacturer and the device name.
> Acked.
>>
>>> + .channel = 0,
>>> + .node = NULL,
>>> + },
>>> + .width_mm = 127,
>>> + .height_mm = 203,
>>> + .bpc = 8,
>>> + .lanes = 3,
>>> + .format = MIPI_DSI_FMT_RGB888,
>>> + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM,
>>> + .init_sequence = elish_boe_init_sequence,
>>> + .is_dual_dsi = true,
>>> +};
>>> +
>> [...]
>>
>>> +static int nt36523_probe(struct mipi_dsi_device *dsi)
>>> +{
>> [...]
>>
>>> + /* If the panel is dual dsi, register DSI1 */
>>> + if (pinfo->desc->is_dual_dsi) {
>>> + info = &pinfo->desc->dsi_info;
>>> +
>>> + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
>>> + if (!dsi1) {
>>> + dev_err(dev, "cannot get secondary DSI node.\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
>>> + of_node_put(dsi1);
>> Shouldn't you put the reference only if it's found?
> thanks for spot it.
>>
>>> + if (!dsi1_host) {
>>> + dev_err(dev, "cannot get secondary DSI host\n");
>>> + return -EPROBE_DEFER;
>> dev_err_probe, here and in neighbouring exit return paths?
> Acked.
>>
>>
>> Konrad
next prev parent reply other threads:[~2023-03-08 12:33 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-08 4:37 [PATCH v3 1/2] dt-bindings: display: panel: Add Novatek NT36523 bindings Jianhua Lu
2023-03-08 4:37 ` [PATCH v3 2/2] drm/panel: Add driver for Novatek NT36523 Jianhua Lu
2023-03-08 11:13 ` Konrad Dybcio
2023-03-08 11:53 ` Jianhua Lu
2023-03-08 12:33 ` Konrad Dybcio [this message]
2023-03-08 13:37 ` Jianhua Lu
2023-03-09 22:37 ` Konrad Dybcio
2023-03-10 9:33 ` Jianhua Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1cbe9e29-13a4-574e-6d8c-b2506e7a36b3@linaro.org \
--to=konrad.dybcio@linaro.org \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lujianhua000@gmail.com \
--cc=neil.armstrong@linaro.org \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=sam@ravnborg.org \
--cc=thierry.reding@gmail.com \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).