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: <66d293a8-f850-cb80-0c83-2ebf7e29d0c2@linaro.org>
Date:   Wed, 8 Mar 2023 12:13:53 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Jianhua Lu <lujianhua000@...il.com>,
        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>
Cc:     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 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.


[...]

> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
> @@ -0,0 +1,770 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Novatek NT36523 DriverIC panels driver
> + *
> + * Copyright (c) 2022, 2023 Jianhua Lu <lujianhua000@...il.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +
> +#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..


> +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?

Could you also implement a 60 (or whatever other value is also
implemented downstream) Hz mode?

> +	.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.

> +		.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?

> +		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?


Konrad
> +		}
> +
> +		pinfo->dsi[1] = mipi_dsi_device_register_full(dsi1_host, info);
> +		if (!pinfo->dsi[1]) {
> +			dev_err(dev, "cannot get secondary DSI device\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	pinfo->dsi[0] = dsi;
> +	mipi_dsi_set_drvdata(dsi, pinfo);
> +	drm_panel_init(&pinfo->panel, dev, &nt36523_panel_funcs, DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&pinfo->panel);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get backlight\n");
> +
> +	drm_panel_add(&pinfo->panel);
> +
> +	for (i = 0; i < DSI_NUM_MIN + pinfo->desc->is_dual_dsi; i++) {
> +		pinfo->dsi[i]->lanes = pinfo->desc->lanes;
> +		pinfo->dsi[i]->format = pinfo->desc->format;
> +		pinfo->dsi[i]->mode_flags = pinfo->desc->mode_flags;
> +
> +		ret = mipi_dsi_attach(pinfo->dsi[i]);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "cannot attach to DSI%d host.\n", i);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id nt36523_of_match[] = {
> +	{
> +		.compatible = "xiaomi,elish-boe-nt36523",
> +		.data = &elish_boe_desc,
> +	},
> +	{
> +		.compatible = "xiaomi,elish-csot-nt36523",
> +		.data = &elish_csot_desc,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, nt36523_of_match);
> +
> +static struct mipi_dsi_driver nt36523_driver = {
> +	.probe = nt36523_probe,
> +	.remove = nt36523_remove,
> +	.driver = {
> +		.name = "panel-novatek-nt36523",
> +		.of_match_table = nt36523_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(nt36523_driver);
> +
> +MODULE_AUTHOR("Jianhua Lu <lujianhua000@...il.com>");
> +MODULE_DESCRIPTION("DRM driver for Novatek NT36523 based MIPI DSI panels");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ