linux-kernel - Re: [PATCH v3 2/2] drm/panel: Add driver for Novatek NT36523

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: <1cbe9e29-13a4-574e-6d8c-b2506e7a36b3@linaro.org>
Date:   Wed, 8 Mar 2023 13:33:39 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Jianhua Lu <lujianhua000@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.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>,
        Linus Walleij <linus.walleij@...aro.org>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] drm/panel: Add driver for Novatek NT36523



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