Hi Abhinav,
I have a few comments below.
On Fri, 2018-08-17 at 19:06 -0700, Abhinav Kumar wrote:
From: "abhinavk@codeaurora.org" abhinavk@codeaurora.org
Add support for Truly NT35597 panel driver used in MSM reference platforms.
This panel driver supports both single DSI and dual DSI modes.
However, this patch series adds support only for dual DSI mode.
Changes in v6:
- Introduce panel config to store panel specific details
- Bring back the size member for the panel command structure to make the design more scalable
- Move the display mode from the DT to driver
- Change the compatible string to indicate which which board and panel it will be used for
- Rename the functions to match the panel driver
- Have a data member for each compatible string
- Remove the panel commands split as its not required for the panel init functionality
Signed-off-by: Archit Taneja architt@codeaurora.org Signed-off-by: Abhinav Kumar abhinavk@codeaurora.org
drivers/gpu/drm/panel/Kconfig | 8 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-truly-nt35597.c | 705 ++++++++++++++++++++++++++++ 3 files changed, 714 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 6020c30..7ae74c2 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -186,4 +186,12 @@ config DRM_PANEL_SITRONIX_ST7789V Say Y here if you want to enable support for the Sitronix ST7789V controller for 240x320 LCD panels
+config DRM_PANEL_TRULY_NT35597_WQXGA
- tristate "Truly WQXGA"
- depends on OF
- depends on DRM_MIPI_DSI
- select VIDEOMODE_HELPERS
- help
Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSIVideo Mode panelendmenu diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 5ccaaa9..80fd19f 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -19,3 +19,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o +obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c new file mode 100644 index 0000000..691be03 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c @@ -0,0 +1,705 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2018, The Linux Foundation. All rights reserved.
- */
+#include <linux/gpio/consumer.h> +#include <linux/of_graph.h> +#include <linux/regulator/consumer.h> +#include <linux/pinctrl/consumer.h> +#include <linux/of_device.h>
+#include <video/mipi_display.h> +#include <video/of_videomode.h> +#include <video/videomode.h>
+#include <drm/drmP.h> +#include <drm/drm_panel.h> +#include <drm/drm_mipi_dsi.h>
+static const char * const regulator_names[] = {
- "vdda",
- "vdispp",
- "vdispn",
+};
+static unsigned long const regulator_enable_loads[] = {
- 62000,
- 100000,
- 100000,
+};
+static unsigned long const regulator_disable_loads[] = {
- 80,
- 100,
- 100,
+};
+struct nt35597_config {
- u32 width_mm;
- u32 height_mm;
- char panel_name[256];
Why not const char *panel_name; ?
- void *panel_on_cmds;
const void *panel_on_cmds;
- u32 num_on_cmds;
- struct drm_display_mode *dm;
const struct drm_display_mode *dm;
+};
+struct truly_nt35597 {
- struct device *dev;
- struct drm_panel panel;
- struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
- struct gpio_desc *reset_gpio;
- struct gpio_desc *mode_gpio;
- struct backlight_device *backlight;
- struct mipi_dsi_device *dsi[2];
- const struct nt35597_config *config;
- bool prepared;
- bool enabled;
+};
+static inline struct truly_nt35597 *panel_to_ctx(struct drm_panel *panel) +{
- return container_of(panel, struct truly_nt35597, panel);
+}
+#define MAX_LEN 5 +#define SHORT_PACKET 2 +#define LONG_PACKET 4
+struct cmd_set {
- u8 commands[MAX_LEN];
- int size;
Consider changing this to u8 size;
+};
+static struct cmd_set qcom_2k_panel_magic_cmds[] = {
- /* CMD2_P0 */
[...]
- /* VBP+VSA=,VFP = 10H */
- { { 0x3B, 0x03, 0x0A, 0x0A }, 4 },
- /* FTE on */
- { { 0x35, 0x00 }, 2 },
- /* EN_BK =1(auto black) */
- { { 0xE5, 0x01 }, 2 },
- /* CMD mode(10) VDO mode(03) */
- { { 0xBB, 0x03 }, 2 },
- /* Non Reload MTP */
- { { 0xFB, 0x01 }, 2 },
+};
+static int truly_dcs_write(struct drm_panel *panel, u32 command) +{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int i, ret;
- for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) {
ret = mipi_dsi_dcs_write(ctx->dsi[i],command, NULL, 0);if (ret < 0) {DRM_DEV_ERROR(ctx->dev,"set_display_off cmd failed for dsi = %d\n",
This error message is not accurate. truly_dcs_write is also called from truly_nt35597_prepare.
i);}- }
- return ret;
+}
+static int truly_dcs_write_buf(struct drm_panel *panel,
- u32 size, u8 *buf)
+{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int ret = 0;
- int i;
- for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) {
ret = mipi_dsi_dcs_write_buffer(ctx->dsi[i],buf,size);if (ret < 0) {DRM_DEV_ERROR(ctx->dev,"failed to tx cmd [%d], err: %d\n",i, ret);return ret;}- }
- return ret;
+}
+#define truly_call_write_func(ret, func, panel, command) {\
- ret = func(panel, command); \
+}
+#define truly_write_buf_func(ret, func, panel, size, buf) {\
- ret = func(panel, size, buf); \
+}
These macros just obfuscate things. Just call the function directly below.
+static int truly_35597_power_on(struct truly_nt35597 *ctx) +{
- int ret, i;
- for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
ret = regulator_set_load(ctx->supplies[i].consumer,regulator_enable_loads[i]);if (ret)return ret;- }
- ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- if (ret < 0)
return ret;- /* Reset sequence of truly panel requires
* the reset pin to be pulled high for 10ms* followed by a low period of 10ms and then* pulled high again*/
The comment says 10 ms
- gpiod_set_value(ctx->reset_gpio, 1);
- msleep(20);
but then the sleep is for 20 ms. Maybe use usleep_range(10000, 20000); ?
- gpiod_set_value(ctx->reset_gpio, 0);
- msleep(20);
- gpiod_set_value(ctx->reset_gpio, 1);
So this is an active low reset? Should this maybe use GPIO_ACTIVE_LOW in the DT and then
gpiod_set_value(ctx->reset_gpio, 0); gpiod_set_value(ctx->reset_gpio, 1); gpiod_set_value(ctx->reset_gpio, 0);
here?
- return 0;
+}
+static int truly_nt35597_power_off(struct truly_nt35597 *ctx) +{
- int ret, i;
- gpiod_set_value(ctx->reset_gpio, 0);
- for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
ret = regulator_set_load(ctx->supplies[i].consumer,regulator_disable_loads[i]);if (ret)return ret;- }
- return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+static int truly_nt35597_disable(struct drm_panel *panel) +{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int ret;
- if (!ctx->enabled)
return 0;- if (ctx->backlight) {
ret = backlight_disable(ctx->backlight);if (ret < 0)DRM_DEV_ERROR(ctx->dev, "backlight disable failed %d\n",ret);- }
- ctx->enabled = false;
- return 0;
+}
+static int truly_nt35597_unprepare(struct drm_panel *panel) +{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int ret = 0;
- if (!ctx->prepared)
return 0;- ctx->dsi[0]->mode_flags = 0;
- ctx->dsi[1]->mode_flags = 0;
- truly_call_write_func(ret, truly_dcs_write, panel,
MIPI_DCS_SET_DISPLAY_OFF);
See above, just
ret = truly_dcs_write(panel, MIPI_DCS_SET_DISPLAY_OFF);
would be a lot more readable.
- if (ret < 0) {
DRM_DEV_ERROR(ctx->dev,"set_display_off cmd failed ret = %d\n",ret);- }
- /* 120ms delay required here as per DCS spec */
- msleep(120);
- truly_call_write_func(ret, truly_dcs_write, panel,
MIPI_DCS_ENTER_SLEEP_MODE);
See above.
- if (ret < 0) {
DRM_DEV_ERROR(ctx->dev,"enter_sleep cmd failed ret = %d\n",ret);- }
- ret = truly_nt35597_power_off(ctx);
- if (ret < 0)
DRM_DEV_ERROR(ctx->dev,"truly_nt35597_power_off failed ret = %d\n",ret);- ctx->prepared = false;
- return ret;
+}
+static int truly_nt35597_prepare(struct drm_panel *panel) +{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int ret;
- int i;
- struct cmd_set *panel_on_cmds;
- const struct nt35597_config *config;
- u32 num_cmds;
- if (ctx->prepared)
return 0;- ret = truly_35597_power_on(ctx);
- if (ret < 0)
return ret;- ctx->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
- ctx->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
- config = ctx->config;
- panel_on_cmds = (struct cmd_set *)config->panel_on_cmds;
- num_cmds = config->num_on_cmds;
- for (i = 0; i < num_cmds; i++)
truly_write_buf_func(ret, truly_dcs_write_buf,panel, panel_on_cmds[i].size,panel_on_cmds[i].commands);
See above. Also, why not stop immediately when a dcs write command fails?
- if (ret < 0) {
DRM_DEV_ERROR(ctx->dev,"panel_cmds_set failed ret = %d\n",ret);- }
- /* As per DSI spec need to wait for 120ms
* after sending the exit sleep DCS command*/- truly_call_write_func(ret, truly_dcs_write, panel,
MIPI_DCS_EXIT_SLEEP_MODE);
See above.
- if (ret < 0) {
DRM_DEV_ERROR(ctx->dev,"exit_sleep_mode cmd failed ret = %d\n",ret);- }
- msleep(120);
- truly_call_write_func(ret, truly_dcs_write, panel,
MIPI_DCS_SET_DISPLAY_ON);- if (ret < 0) {
DRM_DEV_ERROR(ctx->dev,"set_display_on cmd failed ret = %d\n",ret);- }
- /* Adding 120ms delay before the start of
* pixel stream to the panel after sending* the set_display_on DCS command*/- msleep(120);
- ctx->prepared = true;
- return 0;
+}
+static int truly_nt35597_enable(struct drm_panel *panel) +{
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- int ret;
- if (ctx->enabled)
return 0;- if (ctx->backlight) {
ret = backlight_enable(ctx->backlight);if (ret < 0)DRM_DEV_ERROR(ctx->dev, "backlight enable failed %d\n",ret);- }
- ctx->enabled = true;
- return 0;
+}
+static int truly_nt35597_get_modes(struct drm_panel *panel) +{
- struct drm_connector *connector = panel->connector;
- struct truly_nt35597 *ctx = panel_to_ctx(panel);
- struct drm_display_mode *mode;
- const struct nt35597_config *config;
- config = ctx->config;
- mode = drm_mode_create(connector->dev);
- if (!mode) {
DRM_DEV_ERROR(ctx->dev,"failed to create a new display mode\n");return 0;- }
- connector->display_info.width_mm = config->width_mm;
- connector->display_info.height_mm = config->height_mm;
- /* Copy the mode from the DT and add it */
This comment is not accurate, the mode is from inside the driver, currently qcom_sdm845_mtp_2k_mode below.
- drm_mode_copy(mode, config->dm);
- mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
- drm_mode_probed_add(connector, mode);
- return 1;
+}
+static const struct drm_panel_funcs truly_nt35597_drm_funcs = {
- .disable = truly_nt35597_disable,
- .unprepare = truly_nt35597_unprepare,
- .prepare = truly_nt35597_prepare,
- .enable = truly_nt35597_enable,
- .get_modes = truly_nt35597_get_modes,
+};
+static int truly_nt35597_panel_add(struct truly_nt35597 *ctx) +{
- struct device *dev = ctx->dev;
- int ret, i;
- const struct nt35597_config *config;
- config = ctx->config;
- for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++)
ctx->supplies[i].supply = regulator_names[i];- ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
ctx->supplies);- if (ret < 0)
return ret;- ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->reset_gpio)) {
DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",PTR_ERR(ctx->reset_gpio));return PTR_ERR(ctx->reset_gpio);- }
- ctx->mode_gpio = devm_gpiod_get(dev, "mode", GPIOD_OUT_LOW);
- if (IS_ERR(ctx->mode_gpio)) {
DRM_DEV_ERROR(dev, "cannot get mode gpio %ld\n",PTR_ERR(ctx->mode_gpio));return PTR_ERR(ctx->mode_gpio);- }
- /* dual port */
- gpiod_set_value(ctx->mode_gpio, 0);
- drm_panel_init(&ctx->panel);
- ctx->panel.dev = dev;
- ctx->panel.funcs = &truly_nt35597_drm_funcs;
- drm_panel_add(&ctx->panel);
- return 0;
+}
+static struct drm_display_mode qcom_sdm845_mtp_2k_mode = {
static const ...
- .name = "1440x2560",
- .clock = 268316,
- .hdisplay = 1440,
- .hsync_start = 1440 + 200,
- .hsync_end = 1440 + 200 + 32,
- .htotal = 1440 + 200 + 32 + 64,
- .vdisplay = 2560,
- .vsync_start = 2560 + 8,
- .vsync_end = 2560 + 8 + 1,
- .vtotal = 2560 + 8 + 1 + 7,
- .vrefresh = 60,
- .flags = 0,
+};
+static struct nt35597_config nt35597_dir = {
static const ...
- .width_mm = 74,
- .height_mm = 131,
- .panel_name = "qcom_sdm845_mtp_2k_panel",
- .dm = &qcom_sdm845_mtp_2k_mode,
- .panel_on_cmds = qcom_2k_panel_magic_cmds,
- .num_on_cmds = ARRAY_SIZE(qcom_2k_panel_magic_cmds),
+};
+static int truly_nt35597_probe(struct mipi_dsi_device *dsi) +{
- struct device *dev = &dsi->dev;
- struct truly_nt35597 *ctx;
- struct mipi_dsi_device *dsi1_device;
- struct device_node *dsi1;
- struct mipi_dsi_host *dsi1_host;
- struct mipi_dsi_device *dsi_dev;
- int ret = 0;
- int i;
- const struct mipi_dsi_device_info info = {.type = "trulynt35597",
.type should be on the next line.
.channel = 0,.node = NULL,- };
- ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
- if (!ctx)
return -ENOMEM;- /* This device represents itself as one with
* two input ports which are fed by the output* ports of the two DSI controllers . The DSI0* is the master controller and has most of the* panel related info in its child node.*/- ctx->config = of_device_get_match_data(dev);
- if (!ctx->config) {
dev_err(dev, "missing device configuration\n");return -ENODEV;- }
- dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
- if (!dsi1) {
DRM_DEV_ERROR(dev,"failed to get remote node for dsi1_device\n");ret = -ENODEV;goto err_get_remote;- }
- dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
- if (!dsi1_host) {
DRM_DEV_ERROR(dev, "failed to find dsi host\n");ret = -EPROBE_DEFER;goto err_host;- }
- of_node_put(dsi1);
- /* register the second DSI device */
- dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);
- if (IS_ERR(dsi1_device)) {
DRM_DEV_ERROR(dev, "failed to create dsi device\n");ret = PTR_ERR(dsi);goto err_dsi_device;- }
- mipi_dsi_set_drvdata(dsi, ctx);
- ctx->dev = dev;
- ctx->dsi[0] = dsi;
- ctx->dsi[1] = dsi1_device;
- ret = truly_nt35597_panel_add(ctx);
- if (ret) {
DRM_DEV_ERROR(dev, "failed to add panel\n");goto err_panel_add;- }
- for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) {
dsi_dev = ctx->dsi[i];dsi_dev->lanes = 4;dsi_dev->format = MIPI_DSI_FMT_RGB888;dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |MIPI_DSI_CLOCK_NON_CONTINUOUS;ret = mipi_dsi_attach(dsi_dev);if (ret < 0) {DRM_DEV_ERROR(dev,"dsi attach failed i = %d\n", i);goto err_dsi_attach;}- }
- return 0;
+err_dsi_attach:
- drm_panel_remove(&ctx->panel);
+err_panel_add:
- mipi_dsi_device_unregister(dsi1_device);
+err_dsi_device: +err_host:
- of_node_put(dsi1);
+err_get_remote:
- return ret;
+}
+static int truly_nt35597_remove(struct mipi_dsi_device *dsi) +{
- struct truly_nt35597 *ctx = mipi_dsi_get_drvdata(dsi);
- if (ctx->dsi[0])
mipi_dsi_detach(ctx->dsi[0]);- if (ctx->dsi[1]) {
mipi_dsi_detach(ctx->dsi[1]);mipi_dsi_device_unregister(ctx->dsi[1]);- }
- drm_panel_remove(&ctx->panel);
- return 0;
+}
+static const struct of_device_id truly_wqxga_of_match[] = {
Should this table be renamed to truly_nt36697_of_match?
- { .compatible = "qcom,sdm845-mtp-2K-display",
.data = &nt35597_dir,- },
- { }
+}; +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
+static struct mipi_dsi_driver truly_wqxga_driver = {
- .driver = {
.name = "panel_truly_nt35597",.of_match_table = truly_wqxga_of_match,- },
- .probe = truly_nt35597_probe,
- .remove = truly_nt35597_remove,
+}; +module_mipi_dsi_driver(truly_wqxga_driver);
+MODULE_DESCRIPTION("Truly NT35597 DSI Panel Driver"); +MODULE_LICENSE("GPL v2");
regards Philipp