| Message ID | 1527294472-4451-1-git-send-email-abhinavk@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 25, 2018 at 05:27:51PM -0700, Abhinav Kumar wrote: > Add support for Truly NT35597 panel used > in MSM reference platforms. > > This panel supports both single DSI and dual DSI > modes. > > However, this patch series adds support only for > dual DSI mode. > > Changes in v4: > - Fix the license identifier > - Fix formatting issues for the regulator loads > - Fix error messages and return code > > 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 | 576 ++++++++++++++++++++++++++++ > 3 files changed, 585 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 25682ff..2fcd9b1 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -177,4 +177,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 DSI > + Video Mode panel > endmenu > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index f26efc1..056ea93 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -18,3 +18,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..a57cbf0 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c > @@ -0,0 +1,576 @@ > +// SPDX-License-Identifier: GPL-2.0 I guess it is up to Sean and Rob if they want to accept // comments for SPDX. I'm not sure there is a hard and fast rule about it. > +/* > + * 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 <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" The reason why the coding style insists on commas for the last member of an listy is that if you added another item the resulting patch would only need one line instead of three. For example: + "foo", instead of: - "vdispn" + "vdispn", + "foo", <snip>
On Fri, May 25, 2018 at 05:27:51PM -0700, Abhinav Kumar wrote: > Add support for Truly NT35597 panel used > in MSM reference platforms. > > This panel supports both single DSI and dual DSI > modes. > > However, this patch series adds support only for > dual DSI mode. > > Changes in v4: > - Fix the license identifier > - Fix formatting issues for the regulator loads > - Fix error messages and return code > > 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 | 576 ++++++++++++++++++++++++++++ > 3 files changed, 585 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 25682ff..2fcd9b1 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -177,4 +177,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 DSI > + Video Mode panel > endmenu > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index f26efc1..056ea93 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -18,3 +18,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..a57cbf0 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c > @@ -0,0 +1,576 @@ > +// 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 <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 truly_wqxga { > + 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 videomode vm; > + > + struct mipi_dsi_device *dsi[2]; > + > + bool prepared; > + bool enabled; > +}; > + > +static inline struct truly_wqxga *panel_to_truly_wqxga(struct drm_panel *panel) > +{ > + return container_of(panel, struct truly_wqxga, panel); > +} > + > +static int truly_wqxga_power_on(struct truly_wqxga *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; > + > + msleep(20); > + gpiod_set_value(ctx->reset_gpio, 1); > + msleep(20); > + gpiod_set_value(ctx->reset_gpio, 0); > + msleep(20); > + gpiod_set_value(ctx->reset_gpio, 1); > + msleep(50); Why is this needed? Could you please comment? Also, it seems like this is active low? You should specify that in the dt, and let gpiod translate the value. > + > + return 0; > +} > + > +static int truly_wqxga_power_off(struct truly_wqxga *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_wqxga_disable(struct drm_panel *panel) > +{ > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > + > + if (!ctx->enabled) > + return 0; > + > + if (ctx->backlight) { > + ctx->backlight->props.power = FB_BLANK_POWERDOWN; > + backlight_update_status(ctx->backlight); You can use the backlight_enable/disable helpers, and please check the return value. > + } > + > + ctx->enabled = false; > + return 0; > +} > + > +static int truly_wqxga_unprepare(struct drm_panel *panel) > +{ > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > + struct mipi_dsi_device **dsis = ctx->dsi; IMO, this obfuscation isn't too useful (and prevents us from using ARRAY_SIZE below). > + int ret = 0, i; > + > + if (!ctx->prepared) > + return 0; > + > + dsis[0]->mode_flags = 0; > + dsis[1]->mode_flags = 0; > + > + for (i = 0; i < 2; i++) s/2/ARRAY_SIZE(ctx->dsi)/ > + if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0) > + ret = -ECOMM; I guess it's alright that we're continuing through errors, however we should probably log the actual error value as well as the channel number in a DEBUG message. > + > + msleep(120); Please comment. > + > + for (i = 0; i < 2; i++) ARRAY_SIZE nit here. > + if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0) > + ret = -ECOMM; > + > + truly_wqxga_power_off(ctx); You don't check the return value. > + > + ctx->prepared = false; > + return ret; > +} > + > +#define MAX_LEN 5 > +struct { > + u8 commands[MAX_LEN]; > + int size; > +} panel_cmds[] = { /* CMD2_P0 */ > + { { 0xff, 0x20 }, 2 }, > + { { 0xfb, 0x01 }, 2 }, > + { { 0x00, 0x01 }, 2 }, > + { { 0x01, 0x55 }, 2 }, > + { { 0x02, 0x45 }, 2 }, > + { { 0x05, 0x40 }, 2 }, > + { { 0x06, 0x19 }, 2 }, > + { { 0x07, 0x1e }, 2 }, > + { { 0x0b, 0x73 }, 2 }, > + { { 0x0c, 0x73 }, 2 }, > + { { 0x0e, 0xb0 }, 2 }, > + { { 0x0f, 0xae }, 2 }, > + { { 0x11, 0xb8 }, 2 }, > + { { 0x13, 0x00 }, 2 }, > + { { 0x58, 0x80 }, 2 }, > + { { 0x59, 0x01 }, 2 }, > + { { 0x5a, 0x00 }, 2 }, > + { { 0x5b, 0x01 }, 2 }, > + { { 0x5c, 0x80 }, 2 }, > + { { 0x5d, 0x81 }, 2 }, > + { { 0x5e, 0x00 }, 2 }, > + { { 0x5f, 0x01 }, 2 }, > + { { 0x72, 0x11 }, 2 }, > + { { 0x68, 0x03 }, 2 }, > + /* CMD2_P4 */ > + { { 0xFF, 0x24 }, 2 }, > + { { 0xFB, 0x01 }, 2 }, > + { { 0x00, 0x1C }, 2 }, > + { { 0x01, 0x0B }, 2 }, > + { { 0x02, 0x0C }, 2 }, > + { { 0x03, 0x01 }, 2 }, > + { { 0x04, 0x0F }, 2 }, > + { { 0x05, 0x10 }, 2 }, > + { { 0x06, 0x10 }, 2 }, > + { { 0x07, 0x10 }, 2 }, > + { { 0x08, 0x89 }, 2 }, > + { { 0x09, 0x8A }, 2 }, > + { { 0x0A, 0x13 }, 2 }, > + { { 0x0B, 0x13 }, 2 }, > + { { 0x0C, 0x15 }, 2 }, > + { { 0x0D, 0x15 }, 2 }, > + { { 0x0E, 0x17 }, 2 }, > + { { 0x0F, 0x17 }, 2 }, > + { { 0x10, 0x1C }, 2 }, > + { { 0x11, 0x0B }, 2 }, > + { { 0x12, 0x0C }, 2 }, > + { { 0x13, 0x01 }, 2 }, > + { { 0x14, 0x0F }, 2 }, > + { { 0x15, 0x10 }, 2 }, > + { { 0x16, 0x10 }, 2 }, > + { { 0x17, 0x10 }, 2 }, > + { { 0x18, 0x89 }, 2 }, > + { { 0x19, 0x8A }, 2 }, > + { { 0x1A, 0x13 }, 2 }, > + { { 0x1B, 0x13 }, 2 }, > + { { 0x1C, 0x15 }, 2 }, > + { { 0x1D, 0x15 }, 2 }, > + { { 0x1E, 0x17 }, 2 }, > + { { 0x1F, 0x17 }, 2 }, > + /* STV */ > + { { 0x20, 0x40 }, 2 }, > + { { 0x21, 0x01 }, 2 }, > + { { 0x22, 0x00 }, 2 }, > + { { 0x23, 0x40 }, 2 }, > + { { 0x24, 0x40 }, 2 }, > + { { 0x25, 0x6D }, 2 }, > + { { 0x26, 0x40 }, 2 }, > + { { 0x27, 0x40 }, 2 }, > + /* Vend */ > + { { 0xE0, 0x00 }, 2 }, > + { { 0xDC, 0x21 }, 2 }, > + { { 0xDD, 0x22 }, 2 }, > + { { 0xDE, 0x07 }, 2 }, > + { { 0xDF, 0x07 }, 2 }, > + { { 0xE3, 0x6D }, 2 }, > + { { 0xE1, 0x07 }, 2 }, > + { { 0xE2, 0x07 }, 2 }, > + /* UD */ > + { { 0x29, 0xD8 }, 2 }, > + { { 0x2A, 0x2A }, 2 }, > + /* CLK */ > + { { 0x4B, 0x03 }, 2 }, > + { { 0x4C, 0x11 }, 2 }, > + { { 0x4D, 0x10 }, 2 }, > + { { 0x4E, 0x01 }, 2 }, > + { { 0x4F, 0x01 }, 2 }, > + { { 0x50, 0x10 }, 2 }, > + { { 0x51, 0x00 }, 2 }, > + { { 0x52, 0x80 }, 2 }, > + { { 0x53, 0x00 }, 2 }, > + { { 0x56, 0x00 }, 2 }, > + { { 0x54, 0x07 }, 2 }, > + { { 0x58, 0x07 }, 2 }, > + { { 0x55, 0x25 }, 2 }, > + /* Reset XDONB */ > + { { 0x5B, 0x43 }, 2 }, > + { { 0x5C, 0x00 }, 2 }, > + { { 0x5F, 0x73 }, 2 }, > + { { 0x60, 0x73 }, 2 }, > + { { 0x63, 0x22 }, 2 }, > + { { 0x64, 0x00 }, 2 }, > + { { 0x67, 0x08 }, 2 }, > + { { 0x68, 0x04 }, 2 }, > + /* Resolution:1440x2560 */ > + { { 0x72, 0x02 }, 2 }, > + /* mux */ > + { { 0x7A, 0x80 }, 2 }, > + { { 0x7B, 0x91 }, 2 }, > + { { 0x7C, 0xD8 }, 2 }, > + { { 0x7D, 0x60 }, 2 }, > + { { 0x7F, 0x15 }, 2 }, > + { { 0x75, 0x15 }, 2 }, > + /* ABOFF */ > + { { 0xB3, 0xC0 }, 2 }, > + { { 0xB4, 0x00 }, 2 }, > + { { 0xB5, 0x00 }, 2 }, > + /* Source EQ */ > + { { 0x78, 0x00 }, 2 }, > + { { 0x79, 0x00 }, 2 }, > + { { 0x80, 0x00 }, 2 }, > + { { 0x83, 0x00 }, 2 }, > + /* FP BP */ > + { { 0x93, 0x0A }, 2 }, > + { { 0x94, 0x0A }, 2 }, > + /* Inversion Type */ > + { { 0x8A, 0x00 }, 2 }, > + { { 0x9B, 0xFF }, 2 }, > + /* IMGSWAP =1 @PortSwap=1 */ > + { { 0x9D, 0xB0 }, 2 }, > + { { 0x9F, 0x63 }, 2 }, > + { { 0x98, 0x10 }, 2 }, > + /* FRM */ > + { { 0xEC, 0x00 }, 2 }, > + /* CMD1 */ > + { { 0xFF, 0x10 }, 2 }, > + /* 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 }, > +}; How many of these commands are specific to MTP? Decoding the cryptic comments, it seems like this probably wouldn't work on any other boards? I see a couple of duplicate writes as well. Finally, all but one command is 2 bytes, with the outlier being 4 bytes. Yet, MAX_LEN is 5. If we _must_ keep the huge list of obfuscated register writes, could we eliminate the size member? > + > +static int truly_wqxga_prepare(struct drm_panel *panel) > +{ > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > + struct mipi_dsi_device **dsis = ctx->dsi; > + int ret, i, j; > + > + if (ctx->prepared) > + return 0; > + > + ret = truly_wqxga_power_on(ctx); > + if (ret < 0) > + return ret; > + > + dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM; > + dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM; > + > + for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) { > + for (i = 0; i < 2; i++) { Can you flip i and j? Having the inner loop as i is pretty confusing. Also, drop dsis and do s/2/ARRAY_SIZE(ctx->dsi)/ > + ret = mipi_dsi_dcs_write_buffer(dsis[i], > + panel_cmds[j].commands, > + panel_cmds[j].size); > + if (ret < 0) { > + dev_err(ctx->dev, > + "failed to tx cmd [%d], err: %d\n", > + j, ret); > + return ret; > + } > + } > + } > + > + for (i = 0; i < 2; i++) { Same here. > + if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) { > + dev_err(ctx->dev, "failed to exit sleep mode\n"); s/dev_err/DRM_DEV_ERROR/ please apply this to all dev_* messages. > + return -ECOMM; Why not return the actual error? > + } > + } > + > + msleep(78); Why 78 ms? This stuff needs comments. > + > + for (i = 0; i < 2; i++) > + if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) { > + dev_err(ctx->dev, "failed to send display on\n"); > + return -ECOMM; Same comments here. > + } > + msleep(78); > + > + ctx->prepared = true; > + > + return 0; > +} > + > +static int truly_wqxga_enable(struct drm_panel *panel) > +{ > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > + > + if (ctx->enabled) > + return 0; > + > + if (ctx->backlight) { > + ctx->backlight->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(ctx->backlight); > + } You can use the backlight_enable/disable helpers, and please check the return value. > + ctx->enabled = true; > + > + return 0; > +} > + > +static int truly_wqxga_get_modes(struct drm_panel *panel) > +{ > + struct drm_connector *connector = panel->connector; > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > + struct drm_display_mode *mode; > + > + mode = drm_mode_create(connector->dev); > + if (!mode) { > + dev_err(ctx->dev, "failed to create a new display mode\n"); > + return 0; > + } > + > + drm_display_mode_from_videomode(&ctx->vm, mode); > + connector->display_info.width_mm = 74; > + connector->display_info.height_mm = 131; > + 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_wqxga_drm_funcs = { > + .disable = truly_wqxga_disable, > + .unprepare = truly_wqxga_unprepare, > + .prepare = truly_wqxga_prepare, > + .enable = truly_wqxga_enable, > + .get_modes = truly_wqxga_get_modes, > +}; > + > +static int truly_wqxga_panel_add(struct truly_wqxga *ctx) > +{ > + struct device *dev = ctx->dev; > + int ret, i; > + > + 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)) { > + dev_err(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)) { > + dev_err(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); > + > + ret = of_get_videomode(dev->of_node, &ctx->vm, 0); Use of_get_drm_display_mode() and store it as a drm_display_mode. This will simplify get_modes() > + if (ret < 0) > + return ret; > + > + drm_panel_init(&ctx->panel); > + ctx->panel.dev = dev; > + ctx->panel.funcs = &truly_wqxga_drm_funcs; > + drm_panel_add(&ctx->panel); > + > + return 0; > +} > + > +static void truly_wqxga_panel_del(struct truly_wqxga *ctx) > +{ > + if (ctx->panel.dev) ctx is a device managed resource whose parent device is assigned to panel.dev (it's also stored in ctx->dev). So I don't think this check could ever be false without some terrible side effects. It is probably worth checking whether you need to store dev in all of these places, you might find you can trim it out of ctx. So with that in mind, just stick the drm_panel_remove() call at the callsites of truly_wqxga_del() and delete this function. > + drm_panel_remove(&ctx->panel); > +} > + > +static int truly_wqxga_probe(struct mipi_dsi_device *dsi) > +{ > + struct device *dev = &dsi->dev; > + struct truly_wqxga *ctx; > + struct mipi_dsi_device *secondary; > + struct device_node *dsi1; > + struct mipi_dsi_host *dsi1_host; > + int ret = 0; > + const struct mipi_dsi_device_info info = {.type = "trulynt35597", > + .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. > + */ > + > + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1); > + if (!dsi1) { > + dev_err(dev, "failed to get remote node for secondary DSI\n"); > + ret = -ENODEV; > + goto err_get_remote; > + } > + > + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1); > + if (!dsi1_host) { > + dev_err(dev, "failed to find dsi host\n"); > + ret = -EPROBE_DEFER; > + goto err_host; > + } > + > + of_node_put(dsi1); > + > + /* register the second DSI device */ > + secondary = mipi_dsi_device_register_full(dsi1_host, &info); > + nit: extra line While I'm nitpicking, this name leaves something to be desired. Everything else is dsi1, so perhaps this could be dsi1_device to be consistent. > + if (IS_ERR(secondary)) { > + dev_err(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] = secondary; > + > + ret = truly_wqxga_panel_add(ctx); > + if (ret) { > + dev_err(dev, "failed to add panel\n"); > + goto err_panel_add; > + } > + > + /* configure master DSI device */ > + dsi->lanes = 4; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS | > + MIPI_DSI_MODE_LPM; > + > + ret = mipi_dsi_attach(dsi); > + if (ret < 0) { > + dev_err(dev, "master dsi attach failed\n"); > + goto err_dsi_attach; > + } > + > + /* configure secondary DSI device */ > + secondary->lanes = 4; > + secondary->format = MIPI_DSI_FMT_RGB888; > + secondary->mode_flags = MIPI_DSI_MODE_VIDEO | > + MIPI_DSI_CLOCK_NON_CONTINUOUS | > + MIPI_DSI_MODE_LPM; > + > + ret = mipi_dsi_attach(secondary); > + if (ret < 0) { > + dev_err(dev, "mipi_dsi_attach on secondary failed\n"); > + goto err_dsi_attach_sec; > + } Replace the duplicated attach code with a loop: for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) { struct mipi_dsi_device *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, "master dsi attach failed\n"); goto err_dsi_attach; } } > + > + return 0; > + > +err_dsi_attach_sec: > + mipi_dsi_detach(ctx->dsi[0]); > +err_dsi_attach: > + truly_wqxga_panel_del(ctx); > +err_panel_add: > + mipi_dsi_device_unregister(secondary); > +err_dsi_device: > +err_host: > + of_node_put(dsi1); > +err_get_remote: > + return ret; > +} > + > +static int truly_wqxga_remove(struct mipi_dsi_device *dsi) > +{ > + struct truly_wqxga *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]); > + } > + truly_wqxga_panel_del(ctx); > + > + return 0; > +} > + > +static const struct of_device_id truly_wqxga_of_match[] = { > + { .compatible = "truly,nt35597", }, > + { } > +}; > +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_wqxga_probe, > + .remove = truly_wqxga_remove, > +}; > +module_mipi_dsi_driver(truly_wqxga_driver); > + > +MODULE_DESCRIPTION("Truly NT35597 DSI Panel Driver"); > +MODULE_LICENSE("GPL v2"); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Wed, May 30, 2018 at 09:47:25AM -0600, Jordan Crouse wrote: > On Fri, May 25, 2018 at 05:27:51PM -0700, Abhinav Kumar wrote: > > Add support for Truly NT35597 panel used > > in MSM reference platforms. > > > > This panel supports both single DSI and dual DSI > > modes. > > > > However, this patch series adds support only for > > dual DSI mode. > > > > Changes in v4: > > - Fix the license identifier > > - Fix formatting issues for the regulator loads > > - Fix error messages and return code > > > > 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 | 576 ++++++++++++++++++++++++++++ > > 3 files changed, 585 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 25682ff..2fcd9b1 100644 > > --- a/drivers/gpu/drm/panel/Kconfig > > +++ b/drivers/gpu/drm/panel/Kconfig > > @@ -177,4 +177,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 DSI > > + Video Mode panel > > endmenu > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > > index f26efc1..056ea93 100644 > > --- a/drivers/gpu/drm/panel/Makefile > > +++ b/drivers/gpu/drm/panel/Makefile > > @@ -18,3 +18,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..a57cbf0 > > --- /dev/null > > +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c > > @@ -0,0 +1,576 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > I guess it is up to Sean and Rob if they want to accept // comments for SPDX. > I'm not sure there is a hard and fast rule about it. > Hard meh from me. Looks like there are plenty of examples of both in drm. Sean > > +/* > > + * 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 <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" > > The reason why the coding style insists on commas for the last member of an > listy is that if you added another item the resulting patch would > only need one line instead of three. For example: > > + "foo", > > instead of: > > - "vdispn" > + "vdispn", > + "foo", > > <snip> > > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
Hi Sean Thanks for your review. Some responses below. Please help to check. Thanks Abhinav On 2018-05-30 12:12, Sean Paul wrote: > On Fri, May 25, 2018 at 05:27:51PM -0700, Abhinav Kumar wrote: >> Add support for Truly NT35597 panel used >> in MSM reference platforms. >> >> This panel supports both single DSI and dual DSI >> modes. >> >> However, this patch series adds support only for >> dual DSI mode. >> >> Changes in v4: >> - Fix the license identifier >> - Fix formatting issues for the regulator loads >> - Fix error messages and return code >> >> 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 | 576 >> ++++++++++++++++++++++++++++ >> 3 files changed, 585 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 25682ff..2fcd9b1 100644 >> --- a/drivers/gpu/drm/panel/Kconfig >> +++ b/drivers/gpu/drm/panel/Kconfig >> @@ -177,4 +177,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 DSI >> + Video Mode panel >> endmenu >> diff --git a/drivers/gpu/drm/panel/Makefile >> b/drivers/gpu/drm/panel/Makefile >> index f26efc1..056ea93 100644 >> --- a/drivers/gpu/drm/panel/Makefile >> +++ b/drivers/gpu/drm/panel/Makefile >> @@ -18,3 +18,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..a57cbf0 >> --- /dev/null >> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c >> @@ -0,0 +1,576 @@ >> +// 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 <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" >> +}; [Abhinav] Will add a ',' here as per jordan's comment >> + >> +static unsigned long const regulator_enable_loads[] = { >> + 62000, >> + 100000, >> + 100000, >> +}; >> + >> +static unsigned long const regulator_disable_loads[] = { >> + 80, >> + 100, >> + 100, >> +}; >> + >> +struct truly_wqxga { >> + 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 videomode vm; >> + >> + struct mipi_dsi_device *dsi[2]; >> + >> + bool prepared; >> + bool enabled; >> +}; >> + >> +static inline struct truly_wqxga *panel_to_truly_wqxga(struct >> drm_panel *panel) >> +{ >> + return container_of(panel, struct truly_wqxga, panel); >> +} >> + >> +static int truly_wqxga_power_on(struct truly_wqxga *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; >> + >> + msleep(20); >> + gpiod_set_value(ctx->reset_gpio, 1); >> + msleep(20); >> + gpiod_set_value(ctx->reset_gpio, 0); >> + msleep(20); >> + gpiod_set_value(ctx->reset_gpio, 1); >> + msleep(50); > > Why is this needed? Could you please comment? > > Also, it seems like this is active low? You should specify that in the > dt, and > let gpiod translate the value. [Abhinav] This is the panel's reset sequence which demands this delay. I can comment the same. Its active HIGH. I can leave a comment in the bindings mentioning the same. Let me know if this is sufficient. > >> + >> + return 0; >> +} >> + >> +static int truly_wqxga_power_off(struct truly_wqxga *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_wqxga_disable(struct drm_panel *panel) >> +{ >> + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); >> + >> + if (!ctx->enabled) >> + return 0; >> + >> + if (ctx->backlight) { >> + ctx->backlight->props.power = FB_BLANK_POWERDOWN; >> + backlight_update_status(ctx->backlight); > > You can use the backlight_enable/disable helpers, and please check the > return > value. > [Abhinav] Can you please provide me the reference to the helper you are referring to? Other panels were using backlight_update_status(), Hence i am not able to locate it. Yes, I will check the return value in v5. >> + } >> + >> + ctx->enabled = false; >> + return 0; >> +} >> + >> +static int truly_wqxga_unprepare(struct drm_panel *panel) >> +{ >> + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); >> + struct mipi_dsi_device **dsis = ctx->dsi; > > IMO, this obfuscation isn't too useful (and prevents us from using > ARRAY_SIZE > below). [Abhinav] Agreed, will fix this in v5. > >> + int ret = 0, i; >> + >> + if (!ctx->prepared) >> + return 0; >> + >> + dsis[0]->mode_flags = 0; >> + dsis[1]->mode_flags = 0; >> + >> + for (i = 0; i < 2; i++) > > s/2/ARRAY_SIZE(ctx->dsi)/ > >> + if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0) >> + ret = -ECOMM; > > I guess it's alright that we're continuing through errors, however we > should > probably log the actual error value as well as the channel number in a > DEBUG > message. > [Abhinav] Agreed, will print an error message. >> + >> + msleep(120); > > Please comment. [Abhinav] Agreed, will explain this. > >> + >> + for (i = 0; i < 2; i++) > > ARRAY_SIZE nit here. > [Abhinav] Agreed, will use ARRAY_SIZE here >> + if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0) >> + ret = -ECOMM; >> + >> + truly_wqxga_power_off(ctx); > > You don't check the return value. [Abhinav] Agreed, will check return value. > >> + >> + ctx->prepared = false; >> + return ret; >> +} >> + >> +#define MAX_LEN 5 >> +struct { >> + u8 commands[MAX_LEN]; >> + int size; >> +} panel_cmds[] = { /* CMD2_P0 */ >> + { { 0xff, 0x20 }, 2 }, >> + { { 0xfb, 0x01 }, 2 }, >> + { { 0x00, 0x01 }, 2 }, >> + { { 0x01, 0x55 }, 2 }, >> + { { 0x02, 0x45 }, 2 }, >> + { { 0x05, 0x40 }, 2 }, >> + { { 0x06, 0x19 }, 2 }, >> + { { 0x07, 0x1e }, 2 }, >> + { { 0x0b, 0x73 }, 2 }, >> + { { 0x0c, 0x73 }, 2 }, >> + { { 0x0e, 0xb0 }, 2 }, >> + { { 0x0f, 0xae }, 2 }, >> + { { 0x11, 0xb8 }, 2 }, >> + { { 0x13, 0x00 }, 2 }, >> + { { 0x58, 0x80 }, 2 }, >> + { { 0x59, 0x01 }, 2 }, >> + { { 0x5a, 0x00 }, 2 }, >> + { { 0x5b, 0x01 }, 2 }, >> + { { 0x5c, 0x80 }, 2 }, >> + { { 0x5d, 0x81 }, 2 }, >> + { { 0x5e, 0x00 }, 2 }, >> + { { 0x5f, 0x01 }, 2 }, >> + { { 0x72, 0x11 }, 2 }, >> + { { 0x68, 0x03 }, 2 }, >> + /* CMD2_P4 */ >> + { { 0xFF, 0x24 }, 2 }, >> + { { 0xFB, 0x01 }, 2 }, >> + { { 0x00, 0x1C }, 2 }, >> + { { 0x01, 0x0B }, 2 }, >> + { { 0x02, 0x0C }, 2 }, >> + { { 0x03, 0x01 }, 2 }, >> + { { 0x04, 0x0F }, 2 }, >> + { { 0x05, 0x10 }, 2 }, >> + { { 0x06, 0x10 }, 2 }, >> + { { 0x07, 0x10 }, 2 }, >> + { { 0x08, 0x89 }, 2 }, >> + { { 0x09, 0x8A }, 2 }, >> + { { 0x0A, 0x13 }, 2 }, >> + { { 0x0B, 0x13 }, 2 }, >> + { { 0x0C, 0x15 }, 2 }, >> + { { 0x0D, 0x15 }, 2 }, >> + { { 0x0E, 0x17 }, 2 }, >> + { { 0x0F, 0x17 }, 2 }, >> + { { 0x10, 0x1C }, 2 }, >> + { { 0x11, 0x0B }, 2 }, >> + { { 0x12, 0x0C }, 2 }, >> + { { 0x13, 0x01 }, 2 }, >> + { { 0x14, 0x0F }, 2 }, >> + { { 0x15, 0x10 }, 2 }, >> + { { 0x16, 0x10 }, 2 }, >> + { { 0x17, 0x10 }, 2 }, >> + { { 0x18, 0x89 }, 2 }, >> + { { 0x19, 0x8A }, 2 }, >> + { { 0x1A, 0x13 }, 2 }, >> + { { 0x1B, 0x13 }, 2 }, >> + { { 0x1C, 0x15 }, 2 }, >> + { { 0x1D, 0x15 }, 2 }, >> + { { 0x1E, 0x17 }, 2 }, >> + { { 0x1F, 0x17 }, 2 }, >> + /* STV */ >> + { { 0x20, 0x40 }, 2 }, >> + { { 0x21, 0x01 }, 2 }, >> + { { 0x22, 0x00 }, 2 }, >> + { { 0x23, 0x40 }, 2 }, >> + { { 0x24, 0x40 }, 2 }, >> + { { 0x25, 0x6D }, 2 }, >> + { { 0x26, 0x40 }, 2 }, >> + { { 0x27, 0x40 }, 2 }, >> + /* Vend */ >> + { { 0xE0, 0x00 }, 2 }, >> + { { 0xDC, 0x21 }, 2 }, >> + { { 0xDD, 0x22 }, 2 }, >> + { { 0xDE, 0x07 }, 2 }, >> + { { 0xDF, 0x07 }, 2 }, >> + { { 0xE3, 0x6D }, 2 }, >> + { { 0xE1, 0x07 }, 2 }, >> + { { 0xE2, 0x07 }, 2 }, >> + /* UD */ >> + { { 0x29, 0xD8 }, 2 }, >> + { { 0x2A, 0x2A }, 2 }, >> + /* CLK */ >> + { { 0x4B, 0x03 }, 2 }, >> + { { 0x4C, 0x11 }, 2 }, >> + { { 0x4D, 0x10 }, 2 }, >> + { { 0x4E, 0x01 }, 2 }, >> + { { 0x4F, 0x01 }, 2 }, >> + { { 0x50, 0x10 }, 2 }, >> + { { 0x51, 0x00 }, 2 }, >> + { { 0x52, 0x80 }, 2 }, >> + { { 0x53, 0x00 }, 2 }, >> + { { 0x56, 0x00 }, 2 }, >> + { { 0x54, 0x07 }, 2 }, >> + { { 0x58, 0x07 }, 2 }, >> + { { 0x55, 0x25 }, 2 }, >> + /* Reset XDONB */ >> + { { 0x5B, 0x43 }, 2 }, >> + { { 0x5C, 0x00 }, 2 }, >> + { { 0x5F, 0x73 }, 2 }, >> + { { 0x60, 0x73 }, 2 }, >> + { { 0x63, 0x22 }, 2 }, >> + { { 0x64, 0x00 }, 2 }, >> + { { 0x67, 0x08 }, 2 }, >> + { { 0x68, 0x04 }, 2 }, >> + /* Resolution:1440x2560 */ >> + { { 0x72, 0x02 }, 2 }, >> + /* mux */ >> + { { 0x7A, 0x80 }, 2 }, >> + { { 0x7B, 0x91 }, 2 }, >> + { { 0x7C, 0xD8 }, 2 }, >> + { { 0x7D, 0x60 }, 2 }, >> + { { 0x7F, 0x15 }, 2 }, >> + { { 0x75, 0x15 }, 2 }, >> + /* ABOFF */ >> + { { 0xB3, 0xC0 }, 2 }, >> + { { 0xB4, 0x00 }, 2 }, >> + { { 0xB5, 0x00 }, 2 }, >> + /* Source EQ */ >> + { { 0x78, 0x00 }, 2 }, >> + { { 0x79, 0x00 }, 2 }, >> + { { 0x80, 0x00 }, 2 }, >> + { { 0x83, 0x00 }, 2 }, >> + /* FP BP */ >> + { { 0x93, 0x0A }, 2 }, >> + { { 0x94, 0x0A }, 2 }, >> + /* Inversion Type */ >> + { { 0x8A, 0x00 }, 2 }, >> + { { 0x9B, 0xFF }, 2 }, >> + /* IMGSWAP =1 @PortSwap=1 */ >> + { { 0x9D, 0xB0 }, 2 }, >> + { { 0x9F, 0x63 }, 2 }, >> + { { 0x98, 0x10 }, 2 }, >> + /* FRM */ >> + { { 0xEC, 0x00 }, 2 }, >> + /* CMD1 */ >> + { { 0xFF, 0x10 }, 2 }, >> + /* 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 }, >> +}; > > How many of these commands are specific to MTP? Decoding the cryptic > comments, > it seems like this probably wouldn't work on any other boards? > > I see a couple of duplicate writes as well. > > Finally, all but one command is 2 bytes, with the outlier being 4 > bytes. Yet, > MAX_LEN is 5. If we _must_ keep the huge list of obfuscated register > writes, > could we eliminate the size member? [Abhinav] The name is probably misleading you here. MTP mentioned here is something specific to the panel. It has no relation to our MTP board. Hence none of these commands are specific to our MTP board. They are all related to the panel. There are no duplicates. The sequences of the panel often demand the same command to be sent again in different places. You are right, all the commands except one have 2 bytes of payload. However, the 4 byte long command has to be sent in between. If we want to get rid of the extra member element, I can break the commands into 3 sets. First set of x commands with 2 byte payload, then the 4 byte payload command, then the remaining 2 byte payload commands. Let me know if that works for you. I have tried to leave as much comments as possible about the command sets. It will be difficult to explain more. > > >> + >> +static int truly_wqxga_prepare(struct drm_panel *panel) >> +{ >> + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); >> + struct mipi_dsi_device **dsis = ctx->dsi; >> + int ret, i, j; >> + >> + if (ctx->prepared) >> + return 0; >> + >> + ret = truly_wqxga_power_on(ctx); >> + if (ret < 0) >> + return ret; >> + >> + dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM; >> + dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) { >> + for (i = 0; i < 2; i++) { > > Can you flip i and j? Having the inner loop as i is pretty confusing. > [Abhinav] Agreed, will do > Also, drop dsis and do s/2/ARRAY_SIZE(ctx->dsi)/ > [Abhinav] Agreed, will do >> + ret = mipi_dsi_dcs_write_buffer(dsis[i], >> + panel_cmds[j].commands, >> + panel_cmds[j].size); >> + if (ret < 0) { >> + dev_err(ctx->dev, >> + "failed to tx cmd [%d], err: %d\n", >> + j, ret); >> + return ret; >> + } >> + } >> + } >> + >> + for (i = 0; i < 2; i++) { > > Same here. [Abhinav] Agreed, will do > >> + if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) { >> + dev_err(ctx->dev, "failed to exit sleep mode\n"); > > s/dev_err/DRM_DEV_ERROR/ please apply this to all dev_* messages. > [Abhinav] Agreed, will do >> + return -ECOMM; > > Why not return the actual error? > [Abhinav] Agreed, will do >> + } >> + } >> + >> + msleep(78); > > Why 78 ms? This stuff needs comments. > [Abhinav] Agreed, will do >> + >> + for (i = 0; i < 2; i++) >> + if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) { >> + dev_err(ctx->dev, "failed to send display on\n"); >> + return -ECOMM; > > Same comments here. > >> + } >> + msleep(78); >> + >> + ctx->prepared = true; >> + >> + return 0; >> +} >> + >> +static int truly_wqxga_enable(struct drm_panel *panel) >> +{ >> + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); >> + >> + if (ctx->enabled) >> + return 0; >> + >> + if (ctx->backlight) { >> + ctx->backlight->props.power = FB_BLANK_UNBLANK; >> + backlight_update_status(ctx->backlight); >> + } > > You can use the backlight_enable/disable helpers, and please check the > return > value. > [Abhinav] Same as above, can you please provide a reference to the API you are referring here? >> + ctx->enabled = true; >> + >> + return 0; >> +} >> + >> +static int truly_wqxga_get_modes(struct drm_panel *panel) >> +{ >> + struct drm_connector *connector = panel->connector; >> + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); >> + struct drm_display_mode *mode; >> + >> + mode = drm_mode_create(connector->dev); >> + if (!mode) { >> + dev_err(ctx->dev, "failed to create a new display mode\n"); >> + return 0; >> + } >> + >> + drm_display_mode_from_videomode(&ctx->vm, mode); >> + connector->display_info.width_mm = 74; >> + connector->display_info.height_mm = 131; >> + 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_wqxga_drm_funcs = { >> + .disable = truly_wqxga_disable, >> + .unprepare = truly_wqxga_unprepare, >> + .prepare = truly_wqxga_prepare, >> + .enable = truly_wqxga_enable, >> + .get_modes = truly_wqxga_get_modes, >> +}; >> + >> +static int truly_wqxga_panel_add(struct truly_wqxga *ctx) >> +{ >> + struct device *dev = ctx->dev; >> + int ret, i; >> + >> + 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)) { >> + dev_err(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)) { >> + dev_err(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); >> + >> + ret = of_get_videomode(dev->of_node, &ctx->vm, 0); > > Use of_get_drm_display_mode() and store it as a drm_display_mode. This > will > simplify get_modes() [Abhinav] Agreed, will do. > >> + if (ret < 0) >> + return ret; >> + >> + drm_panel_init(&ctx->panel); >> + ctx->panel.dev = dev; >> + ctx->panel.funcs = &truly_wqxga_drm_funcs; >> + drm_panel_add(&ctx->panel); >> + >> + return 0; >> +} >> + >> +static void truly_wqxga_panel_del(struct truly_wqxga *ctx) >> +{ >> + if (ctx->panel.dev) > > ctx is a device managed resource whose parent device is assigned to > panel.dev > (it's also stored in ctx->dev). So I don't think this check could ever > be false > without some terrible side effects. It is probably worth checking > whether you > need to store dev in all of these places, you might find you can trim > it out of > ctx. > > So with that in mind, just stick the drm_panel_remove() call at the > callsites of > truly_wqxga_del() and delete this function. > [Abhinav] Agreed, I can replace truly_wqxga_del() instances with just drm_panel_remove(). Regarding the other comment, are you suggesting to remove dev from ctx and just use (ctx->panel.dev) as they both are the same? >> + drm_panel_remove(&ctx->panel); >> +} >> + >> +static int truly_wqxga_probe(struct mipi_dsi_device *dsi) >> +{ >> + struct device *dev = &dsi->dev; >> + struct truly_wqxga *ctx; >> + struct mipi_dsi_device *secondary; >> + struct device_node *dsi1; >> + struct mipi_dsi_host *dsi1_host; >> + int ret = 0; >> + const struct mipi_dsi_device_info info = {.type = "trulynt35597", >> + .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. >> + */ >> + >> + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1); >> + if (!dsi1) { >> + dev_err(dev, "failed to get remote node for secondary DSI\n"); >> + ret = -ENODEV; >> + goto err_get_remote; >> + } >> + >> + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1); >> + if (!dsi1_host) { >> + dev_err(dev, "failed to find dsi host\n"); >> + ret = -EPROBE_DEFER; >> + goto err_host; >> + } >> + >> + of_node_put(dsi1); >> + >> + /* register the second DSI device */ >> + secondary = mipi_dsi_device_register_full(dsi1_host, &info); >> + > > nit: extra line > > While I'm nitpicking, this name leaves something to be desired. > Everything else > is dsi1, so perhaps this could be dsi1_device to be consistent. > [Abhinav] Agreed, will do. >> + if (IS_ERR(secondary)) { >> + dev_err(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] = secondary; >> + >> + ret = truly_wqxga_panel_add(ctx); >> + if (ret) { >> + dev_err(dev, "failed to add panel\n"); >> + goto err_panel_add; >> + } >> + >> + /* configure master DSI device */ >> + dsi->lanes = 4; >> + dsi->format = MIPI_DSI_FMT_RGB888; >> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >> MIPI_DSI_CLOCK_NON_CONTINUOUS | >> + MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_attach(dsi); >> + if (ret < 0) { >> + dev_err(dev, "master dsi attach failed\n"); >> + goto err_dsi_attach; >> + } >> + >> + /* configure secondary DSI device */ >> + secondary->lanes = 4; >> + secondary->format = MIPI_DSI_FMT_RGB888; >> + secondary->mode_flags = MIPI_DSI_MODE_VIDEO | >> + MIPI_DSI_CLOCK_NON_CONTINUOUS | >> + MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_attach(secondary); >> + if (ret < 0) { >> + dev_err(dev, "mipi_dsi_attach on secondary failed\n"); >> + goto err_dsi_attach_sec; >> + } > > Replace the duplicated attach code with a loop: > > for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) { > struct mipi_dsi_device *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, "master dsi attach > failed\n"); > goto err_dsi_attach; > } > } > [Abhinav] Agreed, will do. >> + >> + return 0; >> + >> +err_dsi_attach_sec: >> + mipi_dsi_detach(ctx->dsi[0]); >> +err_dsi_attach: >> + truly_wqxga_panel_del(ctx); >> +err_panel_add: >> + mipi_dsi_device_unregister(secondary); >> +err_dsi_device: >> +err_host: >> + of_node_put(dsi1); >> +err_get_remote: >> + return ret; >> +} >> + >> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi) >> +{ >> + struct truly_wqxga *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]); >> + } >> + truly_wqxga_panel_del(ctx); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id truly_wqxga_of_match[] = { >> + { .compatible = "truly,nt35597", }, >> + { } >> +}; >> +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_wqxga_probe, >> + .remove = truly_wqxga_remove, >> +}; >> +module_mipi_dsi_driver(truly_wqxga_driver); >> + >> +MODULE_DESCRIPTION("Truly NT35597 DSI Panel Driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project >>
On Wed, May 30, 2018 at 03:37:26PM -0700, abhinavk@codeaurora.org wrote: > Hi Sean > > Thanks for your review. > > Some responses below. Please help to check. > > Thanks > > Abhinav Looks like this got stuck in review. Some comments below, which I hope will match Sean's opinion. > On 2018-05-30 12:12, Sean Paul wrote: > > On Fri, May 25, 2018 at 05:27:51PM -0700, Abhinav Kumar wrote: > > > Add support for Truly NT35597 panel used > > > in MSM reference platforms. > > > > > > This panel supports both single DSI and dual DSI > > > modes. > > > > > > However, this patch series adds support only for > > > dual DSI mode. > > > > > > Changes in v4: > > > - Fix the license identifier > > > - Fix formatting issues for the regulator loads > > > - Fix error messages and return code > > > > > > 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 | 576 > > > ++++++++++++++++++++++++++++ > > > 3 files changed, 585 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 25682ff..2fcd9b1 100644 > > > --- a/drivers/gpu/drm/panel/Kconfig > > > +++ b/drivers/gpu/drm/panel/Kconfig > > > @@ -177,4 +177,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 DSI > > > + Video Mode panel > > > endmenu > > > diff --git a/drivers/gpu/drm/panel/Makefile > > > b/drivers/gpu/drm/panel/Makefile > > > index f26efc1..056ea93 100644 > > > --- a/drivers/gpu/drm/panel/Makefile > > > +++ b/drivers/gpu/drm/panel/Makefile > > > @@ -18,3 +18,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..a57cbf0 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c > > > @@ -0,0 +1,576 @@ > > > +// 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 <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" > > > +}; > [Abhinav] Will add a ',' here as per jordan's comment > > > + > > > +static unsigned long const regulator_enable_loads[] = { > > > + 62000, > > > + 100000, > > > + 100000, > > > +}; > > > + > > > +static unsigned long const regulator_disable_loads[] = { > > > + 80, > > > + 100, > > > + 100, > > > +}; > > > + > > > +struct truly_wqxga { > > > + 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 videomode vm; > > > + > > > + struct mipi_dsi_device *dsi[2]; > > > + > > > + bool prepared; > > > + bool enabled; > > > +}; > > > + > > > +static inline struct truly_wqxga *panel_to_truly_wqxga(struct > > > drm_panel *panel) > > > +{ > > > + return container_of(panel, struct truly_wqxga, panel); > > > +} > > > + > > > +static int truly_wqxga_power_on(struct truly_wqxga *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; > > > + > > > + msleep(20); > > > + gpiod_set_value(ctx->reset_gpio, 1); > > > + msleep(20); > > > + gpiod_set_value(ctx->reset_gpio, 0); > > > + msleep(20); > > > + gpiod_set_value(ctx->reset_gpio, 1); > > > + msleep(50); > > > > Why is this needed? Could you please comment? > > > > Also, it seems like this is active low? You should specify that in the > > dt, and > > let gpiod translate the value. > > [Abhinav] This is the panel's reset sequence which demands this delay. I can > comment the same. > Its active HIGH. I can leave a comment in the bindings mentioning the same. > Let me know if this is sufficient. According to the code that reset is active low, otherwise that last gpiod_set_value() would leave the reset asserted. In either case I think what Sean meant to say was that the code here should be assuming that the reset is active high (i.e. the reset deassert should be gpiod_set_value(..., 0) and the device tree should have GPIO_ACTIVE_LOW in the specifier for the reset GPIO, which will then cause the gpiod library to properly do the inversion for you. > > > > > > + > > > + return 0; > > > +} > > > + > > > +static int truly_wqxga_power_off(struct truly_wqxga *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_wqxga_disable(struct drm_panel *panel) > > > +{ > > > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > > > + > > > + if (!ctx->enabled) > > > + return 0; > > > + > > > + if (ctx->backlight) { > > > + ctx->backlight->props.power = FB_BLANK_POWERDOWN; > > > + backlight_update_status(ctx->backlight); > > > > You can use the backlight_enable/disable helpers, and please check the > > return > > value. > > > [Abhinav] Can you please provide me the reference to the helper you are > referring to? > Other panels were using backlight_update_status(), Hence i am not able to > locate it. > Yes, I will check the return value in v5. I think Sean was referring to backlight_enable() and backlight_disable() in include/linux/backlight.h. You should use those instead of the low- level backlight_update_status(). If you look at the implementation of the new helpers, you'll understand why. =) > > > > + } > > > + > > > + ctx->enabled = false; > > > + return 0; > > > +} > > > + > > > +static int truly_wqxga_unprepare(struct drm_panel *panel) > > > +{ > > > + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); > > > + struct mipi_dsi_device **dsis = ctx->dsi; > > > > IMO, this obfuscation isn't too useful (and prevents us from using > > ARRAY_SIZE > > below). > [Abhinav] Agreed, will fix this in v5. > > > > > + int ret = 0, i; > > > + > > > + if (!ctx->prepared) > > > + return 0; > > > + > > > + dsis[0]->mode_flags = 0; > > > + dsis[1]->mode_flags = 0; > > > + > > > + for (i = 0; i < 2; i++) > > > > s/2/ARRAY_SIZE(ctx->dsi)/ > > > > > + if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0) > > > + ret = -ECOMM; > > > > I guess it's alright that we're continuing through errors, however we > > should > > probably log the actual error value as well as the channel number in a > > DEBUG > > message. > > > [Abhinav] Agreed, will print an error message. > > > + > > > + msleep(120); > > > > Please comment. > [Abhinav] Agreed, will explain this. > > > > > + > > > + for (i = 0; i < 2; i++) > > > > ARRAY_SIZE nit here. > > > [Abhinav] Agreed, will use ARRAY_SIZE here > > > + if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0) > > > + ret = -ECOMM; > > > + > > > + truly_wqxga_power_off(ctx); > > > > You don't check the return value. > [Abhinav] Agreed, will check return value. > > > > > + > > > + ctx->prepared = false; > > > + return ret; > > > +} > > > + > > > +#define MAX_LEN 5 That's odd, you set MAX_LEN to 5, but the longest command in the sequence below is 4 bytes. That means you're effectively wasting at least 1 byte per command. > > > +struct { > > > + u8 commands[MAX_LEN]; > > > + int size; This can be unsigned. > > > +} panel_cmds[] = { /* CMD2_P0 */ You probably want to make this static and const? > > > + { { 0xff, 0x20 }, 2 }, > > > + { { 0xfb, 0x01 }, 2 }, > > > + { { 0x00, 0x01 }, 2 }, > > > + { { 0x01, 0x55 }, 2 }, > > > + { { 0x02, 0x45 }, 2 }, > > > + { { 0x05, 0x40 }, 2 }, > > > + { { 0x06, 0x19 }, 2 }, > > > + { { 0x07, 0x1e }, 2 }, > > > + { { 0x0b, 0x73 }, 2 }, > > > + { { 0x0c, 0x73 }, 2 }, > > > + { { 0x0e, 0xb0 }, 2 }, > > > + { { 0x0f, 0xae }, 2 }, > > > + { { 0x11, 0xb8 }, 2 }, > > > + { { 0x13, 0x00 }, 2 }, > > > + { { 0x58, 0x80 }, 2 }, > > > + { { 0x59, 0x01 }, 2 }, > > > + { { 0x5a, 0x00 }, 2 }, > > > + { { 0x5b, 0x01 }, 2 }, > > > + { { 0x5c, 0x80 }, 2 }, > > > + { { 0x5d, 0x81 }, 2 }, > > > + { { 0x5e, 0x00 }, 2 }, > > > + { { 0x5f, 0x01 }, 2 }, > > > + { { 0x72, 0x11 }, 2 }, > > > + { { 0x68, 0x03 }, 2 }, > > > + /* CMD2_P4 */ > > > + { { 0xFF, 0x24 }, 2 }, > > > + { { 0xFB, 0x01 }, 2 }, > > > + { { 0x00, 0x1C }, 2 }, > > > + { { 0x01, 0x0B }, 2 }, > > > + { { 0x02, 0x0C }, 2 }, > > > + { { 0x03, 0x01 }, 2 }, > > > + { { 0x04, 0x0F }, 2 }, > > > + { { 0x05, 0x10 }, 2 }, > > > + { { 0x06, 0x10 }, 2 }, > > > + { { 0x07, 0x10 }, 2 }, > > > + { { 0x08, 0x89 }, 2 }, > > > + { { 0x09, 0x8A }, 2 }, > > > + { { 0x0A, 0x13 }, 2 }, > > > + { { 0x0B, 0x13 }, 2 }, > > > + { { 0x0C, 0x15 }, 2 }, > > > + { { 0x0D, 0x15 }, 2 }, > > > + { { 0x0E, 0x17 }, 2 }, > > > + { { 0x0F, 0x17 }, 2 }, > > > + { { 0x10, 0x1C }, 2 }, > > > + { { 0x11, 0x0B }, 2 }, > > > + { { 0x12, 0x0C }, 2 }, > > > + { { 0x13, 0x01 }, 2 }, > > > + { { 0x14, 0x0F }, 2 }, > > > + { { 0x15, 0x10 }, 2 }, > > > + { { 0x16, 0x10 }, 2 }, > > > + { { 0x17, 0x10 }, 2 }, > > > + { { 0x18, 0x89 }, 2 }, > > > + { { 0x19, 0x8A }, 2 }, > > > + { { 0x1A, 0x13 }, 2 }, > > > + { { 0x1B, 0x13 }, 2 }, > > > + { { 0x1C, 0x15 }, 2 }, > > > + { { 0x1D, 0x15 }, 2 }, > > > + { { 0x1E, 0x17 }, 2 }, > > > + { { 0x1F, 0x17 }, 2 }, > > > + /* STV */ > > > + { { 0x20, 0x40 }, 2 }, > > > + { { 0x21, 0x01 }, 2 }, > > > + { { 0x22, 0x00 }, 2 }, > > > + { { 0x23, 0x40 }, 2 }, > > > + { { 0x24, 0x40 }, 2 }, > > > + { { 0x25, 0x6D }, 2 }, > > > + { { 0x26, 0x40 }, 2 }, > > > + { { 0x27, 0x40 }, 2 }, > > > + /* Vend */ > > > + { { 0xE0, 0x00 }, 2 }, > > > + { { 0xDC, 0x21 }, 2 }, > > > + { { 0xDD, 0x22 }, 2 }, > > > + { { 0xDE, 0x07 }, 2 }, > > > + { { 0xDF, 0x07 }, 2 }, > > > + { { 0xE3, 0x6D }, 2 }, > > > + { { 0xE1, 0x07 }, 2 }, > > > + { { 0xE2, 0x07 }, 2 }, > > > + /* UD */ > > > + { { 0x29, 0xD8 }, 2 }, > > > + { { 0x2A, 0x2A }, 2 }, > > > + /* CLK */ > > > + { { 0x4B, 0x03 }, 2 }, > > > + { { 0x4C, 0x11 }, 2 }, > > > + { { 0x4D, 0x10 }, 2 }, > > > + { { 0x4E, 0x01 }, 2 }, > > > + { { 0x4F, 0x01 }, 2 }, > > > + { { 0x50, 0x10 }, 2 }, > > > + { { 0x51, 0x00 }, 2 }, > > > + { { 0x52, 0x80 }, 2 }, > > > + { { 0x53, 0x00 }, 2 }, > > > + { { 0x56, 0x00 }, 2 }, > > > + { { 0x54, 0x07 }, 2 }, > > > + { { 0x58, 0x07 }, 2 }, > > > + { { 0x55, 0x25 }, 2 }, > > > + /* Reset XDONB */ > > > + { { 0x5B, 0x43 }, 2 }, > > > + { { 0x5C, 0x00 }, 2 }, > > > + { { 0x5F, 0x73 }, 2 }, > > > + { { 0x60, 0x73 }, 2 }, > > > + { { 0x63, 0x22 }, 2 }, > > > + { { 0x64, 0x00 }, 2 }, > > > + { { 0x67, 0x08 }, 2 }, > > > + { { 0x68, 0x04 }, 2 }, > > > + /* Resolution:1440x2560 */ > > > + { { 0x72, 0x02 }, 2 }, > > > + /* mux */ > > > + { { 0x7A, 0x80 }, 2 }, > > > + { { 0x7B, 0x91 }, 2 }, > > > + { { 0x7C, 0xD8 }, 2 }, > > > + { { 0x7D, 0x60 }, 2 }, > > > + { { 0x7F, 0x15 }, 2 }, > > > + { { 0x75, 0x15 }, 2 }, > > > + /* ABOFF */ > > > + { { 0xB3, 0xC0 }, 2 }, > > > + { { 0xB4, 0x00 }, 2 }, > > > + { { 0xB5, 0x00 }, 2 }, > > > + /* Source EQ */ > > > + { { 0x78, 0x00 }, 2 }, > > > + { { 0x79, 0x00 }, 2 }, > > > + { { 0x80, 0x00 }, 2 }, > > > + { { 0x83, 0x00 }, 2 }, > > > + /* FP BP */ > > > + { { 0x93, 0x0A }, 2 }, > > > + { { 0x94, 0x0A }, 2 }, > > > + /* Inversion Type */ > > > + { { 0x8A, 0x00 }, 2 }, > > > + { { 0x9B, 0xFF }, 2 }, > > > + /* IMGSWAP =1 @PortSwap=1 */ > > > + { { 0x9D, 0xB0 }, 2 }, > > > + { { 0x9F, 0x63 }, 2 }, > > > + { { 0x98, 0x10 }, 2 }, > > > + /* FRM */ > > > + { { 0xEC, 0x00 }, 2 }, > > > + /* CMD1 */ > > > + { { 0xFF, 0x10 }, 2 }, > > > + /* 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 }, > > > +}; > > > > How many of these commands are specific to MTP? Decoding the cryptic > > comments, > > it seems like this probably wouldn't work on any other boards? > > > > I see a couple of duplicate writes as well. > > > > Finally, all but one command is 2 bytes, with the outlier being 4 bytes. > > Yet, > > MAX_LEN is 5. If we _must_ keep the huge list of obfuscated register > > writes, > > could we eliminate the size member? > > [Abhinav] The name is probably misleading you here. MTP mentioned here is > something > specific to the panel. It has no relation to our MTP board. > Hence none of these commands are specific to our MTP board. They are all > related to > the panel. > > There are no duplicates. The sequences of the panel often demand the same > command to > be sent again in different places. > > You are right, all the commands except one have 2 bytes of payload. > However, the 4 byte long command has to be sent in between. > If we want to get rid of the extra member element, I can break the commands > into 3 > sets. > First set of x commands with 2 byte payload, then the 4 byte payload > command, then the > remaining 2 byte payload commands. > Let me know if that works for you. > > I have tried to leave as much comments as possible about the command sets. > It will be difficult to explain more. Given your explanation, I think the above is fine. Modulo the MAX_LEN, signedness and static const comments. > > > +static int truly_wqxga_probe(struct mipi_dsi_device *dsi) > > > +{ > > > + struct device *dev = &dsi->dev; > > > + struct truly_wqxga *ctx; > > > + struct mipi_dsi_device *secondary; > > > + struct device_node *dsi1; > > > + struct mipi_dsi_host *dsi1_host; > > > + int ret = 0; > > > + const struct mipi_dsi_device_info info = {.type = "trulynt35597", > > > + .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. > > > + */ > > > + > > > + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1); > > > + if (!dsi1) { > > > + dev_err(dev, "failed to get remote node for secondary DSI\n"); > > > + ret = -ENODEV; > > > + goto err_get_remote; > > > + } > > > + > > > + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1); > > > + if (!dsi1_host) { > > > + dev_err(dev, "failed to find dsi host\n"); > > > + ret = -EPROBE_DEFER; > > > + goto err_host; > > > + } > > > + > > > + of_node_put(dsi1); > > > + > > > + /* register the second DSI device */ > > > + secondary = mipi_dsi_device_register_full(dsi1_host, &info); Since this does dual DSI, are you aware of the dual-channel DSI bindings that Archit (Cc'ed) has recently proposed? https://patchwork.freedesktop.org/patch/237057/ > > > + > > > > nit: extra line > > > > While I'm nitpicking, this name leaves something to be desired. > > Everything else > > is dsi1, so perhaps this could be dsi1_device to be consistent. > > > [Abhinav] Agreed, will do. > > > > + if (IS_ERR(secondary)) { > > > + dev_err(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] = secondary; > > > + > > > + ret = truly_wqxga_panel_add(ctx); > > > + if (ret) { > > > + dev_err(dev, "failed to add panel\n"); > > > + goto err_panel_add; > > > + } > > > + > > > + /* configure master DSI device */ > > > + dsi->lanes = 4; > > > + dsi->format = MIPI_DSI_FMT_RGB888; > > > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | > > > MIPI_DSI_CLOCK_NON_CONTINUOUS | > > > + MIPI_DSI_MODE_LPM; > > > + > > > + ret = mipi_dsi_attach(dsi); > > > + if (ret < 0) { > > > + dev_err(dev, "master dsi attach failed\n"); > > > + goto err_dsi_attach; > > > + } > > > + > > > + /* configure secondary DSI device */ > > > + secondary->lanes = 4; > > > + secondary->format = MIPI_DSI_FMT_RGB888; > > > + secondary->mode_flags = MIPI_DSI_MODE_VIDEO | > > > + MIPI_DSI_CLOCK_NON_CONTINUOUS | > > > + MIPI_DSI_MODE_LPM; > > > + > > > + ret = mipi_dsi_attach(secondary); > > > + if (ret < 0) { > > > + dev_err(dev, "mipi_dsi_attach on secondary failed\n"); > > > + goto err_dsi_attach_sec; > > > + } > > > > Replace the duplicated attach code with a loop: > > > > for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) { > > struct mipi_dsi_device *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, "master dsi attach > > failed\n"); > > goto err_dsi_attach; > > } > > } > > > [Abhinav] Agreed, will do. > > > + > > > + return 0; > > > + > > > +err_dsi_attach_sec: > > > + mipi_dsi_detach(ctx->dsi[0]); > > > +err_dsi_attach: > > > + truly_wqxga_panel_del(ctx); > > > +err_panel_add: > > > + mipi_dsi_device_unregister(secondary); > > > +err_dsi_device: > > > +err_host: > > > + of_node_put(dsi1); > > > +err_get_remote: > > > + return ret; > > > +} > > > + > > > +static int truly_wqxga_remove(struct mipi_dsi_device *dsi) > > > +{ > > > + struct truly_wqxga *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]); > > > + } > > > + truly_wqxga_panel_del(ctx); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id truly_wqxga_of_match[] = { > > > + { .compatible = "truly,nt35597", }, > > > + { } > > > +}; > > > +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match); > > > + > > > +static struct mipi_dsi_driver truly_wqxga_driver = { > > > + .driver = { > > > + .name = "panel_truly_nt35597", I generally prefer dashes to underscores in driver names, in this case: "panel-truly-nt35597". Thierry
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 25682ff..2fcd9b1 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -177,4 +177,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 DSI + Video Mode panel endmenu diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index f26efc1..056ea93 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -18,3 +18,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..a57cbf0 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c @@ -0,0 +1,576 @@ +// 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 <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 truly_wqxga { + 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 videomode vm; + + struct mipi_dsi_device *dsi[2]; + + bool prepared; + bool enabled; +}; + +static inline struct truly_wqxga *panel_to_truly_wqxga(struct drm_panel *panel) +{ + return container_of(panel, struct truly_wqxga, panel); +} + +static int truly_wqxga_power_on(struct truly_wqxga *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; + + msleep(20); + gpiod_set_value(ctx->reset_gpio, 1); + msleep(20); + gpiod_set_value(ctx->reset_gpio, 0); + msleep(20); + gpiod_set_value(ctx->reset_gpio, 1); + msleep(50); + + return 0; +} + +static int truly_wqxga_power_off(struct truly_wqxga *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_wqxga_disable(struct drm_panel *panel) +{ + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); + + if (!ctx->enabled) + return 0; + + if (ctx->backlight) { + ctx->backlight->props.power = FB_BLANK_POWERDOWN; + backlight_update_status(ctx->backlight); + } + + ctx->enabled = false; + return 0; +} + +static int truly_wqxga_unprepare(struct drm_panel *panel) +{ + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); + struct mipi_dsi_device **dsis = ctx->dsi; + int ret = 0, i; + + if (!ctx->prepared) + return 0; + + dsis[0]->mode_flags = 0; + dsis[1]->mode_flags = 0; + + for (i = 0; i < 2; i++) + if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0) + ret = -ECOMM; + + msleep(120); + + for (i = 0; i < 2; i++) + if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0) + ret = -ECOMM; + + truly_wqxga_power_off(ctx); + + ctx->prepared = false; + return ret; +} + +#define MAX_LEN 5 +struct { + u8 commands[MAX_LEN]; + int size; +} panel_cmds[] = { /* CMD2_P0 */ + { { 0xff, 0x20 }, 2 }, + { { 0xfb, 0x01 }, 2 }, + { { 0x00, 0x01 }, 2 }, + { { 0x01, 0x55 }, 2 }, + { { 0x02, 0x45 }, 2 }, + { { 0x05, 0x40 }, 2 }, + { { 0x06, 0x19 }, 2 }, + { { 0x07, 0x1e }, 2 }, + { { 0x0b, 0x73 }, 2 }, + { { 0x0c, 0x73 }, 2 }, + { { 0x0e, 0xb0 }, 2 }, + { { 0x0f, 0xae }, 2 }, + { { 0x11, 0xb8 }, 2 }, + { { 0x13, 0x00 }, 2 }, + { { 0x58, 0x80 }, 2 }, + { { 0x59, 0x01 }, 2 }, + { { 0x5a, 0x00 }, 2 }, + { { 0x5b, 0x01 }, 2 }, + { { 0x5c, 0x80 }, 2 }, + { { 0x5d, 0x81 }, 2 }, + { { 0x5e, 0x00 }, 2 }, + { { 0x5f, 0x01 }, 2 }, + { { 0x72, 0x11 }, 2 }, + { { 0x68, 0x03 }, 2 }, + /* CMD2_P4 */ + { { 0xFF, 0x24 }, 2 }, + { { 0xFB, 0x01 }, 2 }, + { { 0x00, 0x1C }, 2 }, + { { 0x01, 0x0B }, 2 }, + { { 0x02, 0x0C }, 2 }, + { { 0x03, 0x01 }, 2 }, + { { 0x04, 0x0F }, 2 }, + { { 0x05, 0x10 }, 2 }, + { { 0x06, 0x10 }, 2 }, + { { 0x07, 0x10 }, 2 }, + { { 0x08, 0x89 }, 2 }, + { { 0x09, 0x8A }, 2 }, + { { 0x0A, 0x13 }, 2 }, + { { 0x0B, 0x13 }, 2 }, + { { 0x0C, 0x15 }, 2 }, + { { 0x0D, 0x15 }, 2 }, + { { 0x0E, 0x17 }, 2 }, + { { 0x0F, 0x17 }, 2 }, + { { 0x10, 0x1C }, 2 }, + { { 0x11, 0x0B }, 2 }, + { { 0x12, 0x0C }, 2 }, + { { 0x13, 0x01 }, 2 }, + { { 0x14, 0x0F }, 2 }, + { { 0x15, 0x10 }, 2 }, + { { 0x16, 0x10 }, 2 }, + { { 0x17, 0x10 }, 2 }, + { { 0x18, 0x89 }, 2 }, + { { 0x19, 0x8A }, 2 }, + { { 0x1A, 0x13 }, 2 }, + { { 0x1B, 0x13 }, 2 }, + { { 0x1C, 0x15 }, 2 }, + { { 0x1D, 0x15 }, 2 }, + { { 0x1E, 0x17 }, 2 }, + { { 0x1F, 0x17 }, 2 }, + /* STV */ + { { 0x20, 0x40 }, 2 }, + { { 0x21, 0x01 }, 2 }, + { { 0x22, 0x00 }, 2 }, + { { 0x23, 0x40 }, 2 }, + { { 0x24, 0x40 }, 2 }, + { { 0x25, 0x6D }, 2 }, + { { 0x26, 0x40 }, 2 }, + { { 0x27, 0x40 }, 2 }, + /* Vend */ + { { 0xE0, 0x00 }, 2 }, + { { 0xDC, 0x21 }, 2 }, + { { 0xDD, 0x22 }, 2 }, + { { 0xDE, 0x07 }, 2 }, + { { 0xDF, 0x07 }, 2 }, + { { 0xE3, 0x6D }, 2 }, + { { 0xE1, 0x07 }, 2 }, + { { 0xE2, 0x07 }, 2 }, + /* UD */ + { { 0x29, 0xD8 }, 2 }, + { { 0x2A, 0x2A }, 2 }, + /* CLK */ + { { 0x4B, 0x03 }, 2 }, + { { 0x4C, 0x11 }, 2 }, + { { 0x4D, 0x10 }, 2 }, + { { 0x4E, 0x01 }, 2 }, + { { 0x4F, 0x01 }, 2 }, + { { 0x50, 0x10 }, 2 }, + { { 0x51, 0x00 }, 2 }, + { { 0x52, 0x80 }, 2 }, + { { 0x53, 0x00 }, 2 }, + { { 0x56, 0x00 }, 2 }, + { { 0x54, 0x07 }, 2 }, + { { 0x58, 0x07 }, 2 }, + { { 0x55, 0x25 }, 2 }, + /* Reset XDONB */ + { { 0x5B, 0x43 }, 2 }, + { { 0x5C, 0x00 }, 2 }, + { { 0x5F, 0x73 }, 2 }, + { { 0x60, 0x73 }, 2 }, + { { 0x63, 0x22 }, 2 }, + { { 0x64, 0x00 }, 2 }, + { { 0x67, 0x08 }, 2 }, + { { 0x68, 0x04 }, 2 }, + /* Resolution:1440x2560 */ + { { 0x72, 0x02 }, 2 }, + /* mux */ + { { 0x7A, 0x80 }, 2 }, + { { 0x7B, 0x91 }, 2 }, + { { 0x7C, 0xD8 }, 2 }, + { { 0x7D, 0x60 }, 2 }, + { { 0x7F, 0x15 }, 2 }, + { { 0x75, 0x15 }, 2 }, + /* ABOFF */ + { { 0xB3, 0xC0 }, 2 }, + { { 0xB4, 0x00 }, 2 }, + { { 0xB5, 0x00 }, 2 }, + /* Source EQ */ + { { 0x78, 0x00 }, 2 }, + { { 0x79, 0x00 }, 2 }, + { { 0x80, 0x00 }, 2 }, + { { 0x83, 0x00 }, 2 }, + /* FP BP */ + { { 0x93, 0x0A }, 2 }, + { { 0x94, 0x0A }, 2 }, + /* Inversion Type */ + { { 0x8A, 0x00 }, 2 }, + { { 0x9B, 0xFF }, 2 }, + /* IMGSWAP =1 @PortSwap=1 */ + { { 0x9D, 0xB0 }, 2 }, + { { 0x9F, 0x63 }, 2 }, + { { 0x98, 0x10 }, 2 }, + /* FRM */ + { { 0xEC, 0x00 }, 2 }, + /* CMD1 */ + { { 0xFF, 0x10 }, 2 }, + /* 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_wqxga_prepare(struct drm_panel *panel) +{ + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); + struct mipi_dsi_device **dsis = ctx->dsi; + int ret, i, j; + + if (ctx->prepared) + return 0; + + ret = truly_wqxga_power_on(ctx); + if (ret < 0) + return ret; + + dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM; + dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM; + + for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) { + for (i = 0; i < 2; i++) { + ret = mipi_dsi_dcs_write_buffer(dsis[i], + panel_cmds[j].commands, + panel_cmds[j].size); + if (ret < 0) { + dev_err(ctx->dev, + "failed to tx cmd [%d], err: %d\n", + j, ret); + return ret; + } + } + } + + for (i = 0; i < 2; i++) { + if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) { + dev_err(ctx->dev, "failed to exit sleep mode\n"); + return -ECOMM; + } + } + + msleep(78); + + for (i = 0; i < 2; i++) + if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) { + dev_err(ctx->dev, "failed to send display on\n"); + return -ECOMM; + } + msleep(78); + + ctx->prepared = true; + + return 0; +} + +static int truly_wqxga_enable(struct drm_panel *panel) +{ + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); + + if (ctx->enabled) + return 0; + + if (ctx->backlight) { + ctx->backlight->props.power = FB_BLANK_UNBLANK; + backlight_update_status(ctx->backlight); + } + ctx->enabled = true; + + return 0; +} + +static int truly_wqxga_get_modes(struct drm_panel *panel) +{ + struct drm_connector *connector = panel->connector; + struct truly_wqxga *ctx = panel_to_truly_wqxga(panel); + struct drm_display_mode *mode; + + mode = drm_mode_create(connector->dev); + if (!mode) { + dev_err(ctx->dev, "failed to create a new display mode\n"); + return 0; + } + + drm_display_mode_from_videomode(&ctx->vm, mode); + connector->display_info.width_mm = 74; + connector->display_info.height_mm = 131; + 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_wqxga_drm_funcs = { + .disable = truly_wqxga_disable, + .unprepare = truly_wqxga_unprepare, + .prepare = truly_wqxga_prepare, + .enable = truly_wqxga_enable, + .get_modes = truly_wqxga_get_modes, +}; + +static int truly_wqxga_panel_add(struct truly_wqxga *ctx) +{ + struct device *dev = ctx->dev; + int ret, i; + + 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)) { + dev_err(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)) { + dev_err(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); + + ret = of_get_videomode(dev->of_node, &ctx->vm, 0); + if (ret < 0) + return ret; + + drm_panel_init(&ctx->panel); + ctx->panel.dev = dev; + ctx->panel.funcs = &truly_wqxga_drm_funcs; + drm_panel_add(&ctx->panel); + + return 0; +} + +static void truly_wqxga_panel_del(struct truly_wqxga *ctx) +{ + if (ctx->panel.dev) + drm_panel_remove(&ctx->panel); +} + +static int truly_wqxga_probe(struct mipi_dsi_device *dsi) +{ + struct device *dev = &dsi->dev; + struct truly_wqxga *ctx; + struct mipi_dsi_device *secondary; + struct device_node *dsi1; + struct mipi_dsi_host *dsi1_host; + int ret = 0; + const struct mipi_dsi_device_info info = {.type = "trulynt35597", + .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. + */ + + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1); + if (!dsi1) { + dev_err(dev, "failed to get remote node for secondary DSI\n"); + ret = -ENODEV; + goto err_get_remote; + } + + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1); + if (!dsi1_host) { + dev_err(dev, "failed to find dsi host\n"); + ret = -EPROBE_DEFER; + goto err_host; + } + + of_node_put(dsi1); + + /* register the second DSI device */ + secondary = mipi_dsi_device_register_full(dsi1_host, &info); + + if (IS_ERR(secondary)) { + dev_err(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] = secondary; + + ret = truly_wqxga_panel_add(ctx); + if (ret) { + dev_err(dev, "failed to add panel\n"); + goto err_panel_add; + } + + /* configure master DSI device */ + dsi->lanes = 4; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS | + MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_attach(dsi); + if (ret < 0) { + dev_err(dev, "master dsi attach failed\n"); + goto err_dsi_attach; + } + + /* configure secondary DSI device */ + secondary->lanes = 4; + secondary->format = MIPI_DSI_FMT_RGB888; + secondary->mode_flags = MIPI_DSI_MODE_VIDEO | + MIPI_DSI_CLOCK_NON_CONTINUOUS | + MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_attach(secondary); + if (ret < 0) { + dev_err(dev, "mipi_dsi_attach on secondary failed\n"); + goto err_dsi_attach_sec; + } + + return 0; + +err_dsi_attach_sec: + mipi_dsi_detach(ctx->dsi[0]); +err_dsi_attach: + truly_wqxga_panel_del(ctx); +err_panel_add: + mipi_dsi_device_unregister(secondary); +err_dsi_device: +err_host: + of_node_put(dsi1); +err_get_remote: + return ret; +} + +static int truly_wqxga_remove(struct mipi_dsi_device *dsi) +{ + struct truly_wqxga *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]); + } + truly_wqxga_panel_del(ctx); + + return 0; +} + +static const struct of_device_id truly_wqxga_of_match[] = { + { .compatible = "truly,nt35597", }, + { } +}; +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_wqxga_probe, + .remove = truly_wqxga_remove, +}; +module_mipi_dsi_driver(truly_wqxga_driver); + +MODULE_DESCRIPTION("Truly NT35597 DSI Panel Driver"); +MODULE_LICENSE("GPL v2");