Re: [PATCH v7 4/8] drm/panel: support for auo,kd101n80-45na wuxga dsi video mode panel

Hi Jitao.

A few comments in the following.

Sam

On Sat, Oct 12, 2019 at 11:07:16AM +0800, Jitao Shi wrote:

Auo,kd101n80-45na's connector is same as boe,tv101wum-nl6. The most codes can be reuse. So auo,kd101n80-45na and boe,tv101wum-nl6 use one driver file. Add the different parts in driver data.

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

drivers/gpu/drm/panel/Kconfig | 6 +- .../gpu/drm/panel/panel-boe-tv101wum-nl6.c | 86 ++++++++++++++++--- 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index afcadb3585fb..0e887c978796 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -19,13 +19,13 @@ config DRM_PANEL_ARM_VERSATILE in the Versatile family syscon registers.

config DRM_PANEL_BOE_TV101WUM_NL6

• tristate "BOE TV101WUM 1200x1920 panel"

• tristate "BOE TV101WUM and AUO KD101N80 45NA 1200x1920 panel" depends on OF depends on DRM_MIPI_DSI depends on BACKLIGHT_CLASS_DEVICE help

•  Say Y here if you want to support for BOE TV101WUM WUXGA PANEL
  

•  DSI Video Mode panel
  

•  Say Y here if you want to support for BOE TV101WUM and AUO KD101N80
  

•  45NA WUXGA PANEL DSI Video Mode panel
  
  

config DRM_PANEL_LVDS tristate "Generic LVDS panel driver" diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c index af68236ea0e8..e6457f87bc61 100644 --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c @@ -35,6 +35,7 @@ struct panel_desc { enum mipi_dsi_pixel_format format; const struct panel_init_cmd *init_cmds; unsigned int lanes;

• bool discharge_on_disable;

};

struct boe_panel { @@ -372,6 +373,15 @@ static const struct panel_init_cmd boe_init_cmd[] = { {}, };

+static const struct panel_init_cmd auo_kd101n80_45na_init_cmd[] = {

• _INIT_DELAY_CMD(24),
• _INIT_DCS_CMD(0x11),
• _INIT_DELAY_CMD(120),
• _INIT_DCS_CMD(0x29),
• _INIT_DELAY_CMD(120),
• {},

+};

static inline struct boe_panel *to_boe_panel(struct drm_panel *panel) { return container_of(panel, struct boe_panel, base); @@ -452,20 +462,30 @@ static int boe_panel_unprepare(struct drm_panel *panel) if (!boe->prepared) return 0;

• ret = boe_panel_off(boe);
• if (ret < 0) {

• dev_err(panel->dev, "failed to set panel off: %d\n", ret);
  

• return ret;
  

• if (boe->desc->discharge_on_disable) {

• msleep(150);
  

• regulator_disable(boe->avee);
  

• regulator_disable(boe->avdd);
  

• usleep_range(5000, 7000);
  

• gpiod_set_value(boe->enable_gpio, 0);
  

• usleep_range(5000, 7000);
  

• regulator_disable(boe->pp1800);
  

The panel is not put into sleep mode in this case. (Done by boe_panel_off() - which is not the most descriptive name for that function). If this is on purpose then consider adding a comment.

• } else {

• ret = boe_panel_off(boe);
  

• if (ret < 0) {
  

• 	dev_err(panel->dev, "failed to set panel off: %d\n",
  

• 		ret);
  

• 	return ret;
  

• }
  

• msleep(150);
  

• gpiod_set_value(boe->enable_gpio, 0);
  

• usleep_range(500, 1000);
  

• regulator_disable(boe->avee);
  

• regulator_disable(boe->avdd);
  

• usleep_range(5000, 7000);
  

• regulator_disable(boe->pp1800);
  

  }

• msleep(150);

• gpiod_set_value(boe->enable_gpio, 0);

• usleep_range(500, 1000);

• regulator_disable(boe->avee);

• regulator_disable(boe->avdd);

• usleep_range(5000, 7000);

• regulator_disable(boe->pp1800);

• boe->prepared = false;

  return 0;

@@ -495,10 +515,14 @@ static int boe_panel_prepare(struct drm_panel *panel) if (ret < 0) goto poweroffavdd;

• msleep(100);

• usleep_range(5000, 10000);

  gpiod_set_value(boe->enable_gpio, 1);

• usleep_range(10000, 12000);

• usleep_range(1000, 2000);
• gpiod_set_value(boe->enable_gpio, 0);
• usleep_range(1000, 2000);
• gpiod_set_value(boe->enable_gpio, 1);
• usleep_range(6000, 10000);

This looks like a geneeral change. Consider pushing to the initial commit.

ret = boe_panel_init(boe); if (ret < 0) { @@ -530,6 +554,8 @@ static int boe_panel_enable(struct drm_panel *panel) if (boe->enabled) return 0;

• msleep(130);
• ret = backlight_enable(boe->backlight); if (ret) { dev_err(panel->dev, "Failed to enable backlight %d\n",

@@ -567,6 +593,35 @@ static const struct panel_desc boe_tv101wum_nl6_desc = { .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_LPM, .init_cmds = boe_init_cmd,

• .discharge_on_disable = false,

+};

+static const struct drm_display_mode auo_kd101n80_45na_default_mode = {

• .clock = 157000,
• .hdisplay = 1200,
• .hsync_start = 1200 + 80,
• .hsync_end = 1200 + 80 + 24,
• .htotal = 1200 + 80 + 24 + 36,
• .vdisplay = 1920,
• .vsync_start = 1920 + 16,
• .vsync_end = 1920 + 16 + 4,
• .vtotal = 1920 + 16 + 4 + 16,
• .vrefresh = 60,

+};

+static const struct panel_desc auo_kd101n80_45na_desc = {

• .modes = &auo_kd101n80_45na_default_mode,
• .bpc = 8,
• .size = {

• .width_mm = 135,
  

• .height_mm = 216,
  

• },
• .lanes = 4,
• .format = MIPI_DSI_FMT_RGB888,
• .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |

•       MIPI_DSI_MODE_LPM,
  

• .init_cmds = auo_kd101n80_45na_init_cmd,
• .discharge_on_disable = true,

};

static int boe_panel_get_modes(struct drm_panel *panel) @@ -693,6 +748,9 @@ static const struct of_device_id boe_of_match[] = { { .compatible = "boe,tv101wum-nl6", .data = &boe_tv101wum_nl6_desc },

• { .compatible = "auo,kd101n80-45na",

•  .data = &auo_kd101n80_45na_desc
  

• }, { /* sentinel */ }

}; MODULE_DEVICE_TABLE(of, boe_of_match); -- 2.21.0