Re: [PATCH] [v8, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel

Hi Jerry,

On Thu, 25 Apr 2019 at 08:40, Jerry Han jerry.han.hq@gmail.com wrote:

Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI panel.

V8:

• Modify communication address

V7:

• Add the information of the reviewer
• Remove unnecessary delays, The udelay_range code gracefully returns without hitting the scheduler on a delay of 0. (Derek)
• Merge the same data structures, like display_mode and off_cmds (Derek)
• Optimize the processing of results returned by devm_gpiod_get_optional (Derek)

V6:

• Add the information of the reviewer (Sam)
• Delete unnecessary header files #include <linux/fb.h> (Sam)
• The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them (Sam)
• ADD static, set_gpios function is not used outside this module (Sam)

V5:

• Added changelog

V4:

• Frefix all function maes with boe_ (Sam)
• Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam)
• Sort include lines alphabetically (Sam)
• Fixed entries in the makefile must be sorted alphabetically (Sam)
• Add send_mipi_cmds function to avoid duplicating the code (Sam)
• Add the necessary delay(reset_delay_t5) between reset and sending the initialization command (Rock wang)

V3:

• Remove unnecessary delays in sending initialization commands (Jitao Shi)

V2:

• Use SPDX identifier (Sam)
• Use necessary header files replace drmP.h (Sam)
• Delete unnecessary header files #include <linux/err.h> (Sam)
• Specifies a GPIOs array to control the reset timing, instead of reading "dsi-reset-sequence" data from DTS (Sam)
• Delete backlight_disable() function when already disabled (Sam)
• Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam)
• Move the necessary data in the DTS to the current file, like porch, display_mode and Init code etc. (Sam)
• Add compatible device "boe,himax8279d10p" (Sam)

Signed-off-by: Jerry Han jerry.han.hq@gmail.com Reviewed-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Derek Basehore dbasehore@chromium.org Cc: Jitao Shi jitao.shi@mediatek.com Cc: Rock wang rock_wang@himax.com.cn

---

While the DT has landed, this patch is not merged it seems. I think that Sam is waiting for a version which does not trigger so many check-patch warnings.

That said, a couple of comments if I may.

•   struct gpio_desc *enable_gpio;
  

•   struct gpio_desc *pp33_gpio;
  

•   struct gpio_desc *pp18_gpio;
  

DT claims that these gpios are required, yet one is using the optional gpio API. Is the code off, or does the DT need fixing?

+static int send_mipi_cmds(struct drm_panel *panel, const struct panel_cmd *cmds) +{

•   struct panel_info *pinfo = to_panel_info(panel);
  

•   unsigned int i = 0;
  

•   int err;
  

•   if (!cmds)
  

•           return -EFAULT;
  

•   for (i = 0; cmds[i].len != 0; i++) {
  

•           const struct panel_cmd *cmd = &cmds[i];
  

•           if (cmd->len == 2)
  

•                   err = mipi_dsi_dcs_write(pinfo->link,
  

•                                               cmd->data[1], NULL, 0);
  

•           else
  

•                   err = mipi_dsi_dcs_write(pinfo->link,
  

•                                               cmd->data[1], cmd->data + 2,
  

•                                               cmd->len - 2);
  

•           if (err < 0)
  

•                   return err;
  

•           usleep_range((cmd->data[0]) * 1000,
  

•                       (1 + cmd->data[0]) * 1000);
  

•   }
  

This format seems semi-magical. Any particular reason why you're not using the helpers? In particular, enter/exit sleep mode and set display on/off.

Speaking of which - the 8inch display uses then, yet they are missing for the 10inch one. Seems like there's a bug in there.

Plus we have some repeating patterns in the vendor/undocumented commands - good reason to get those into separate hunks. With the above in place, you can effectively drop the .off_cmd callback and sleep field from the INIT_CMD arrays.

Hence, less code to debug and maintain ;-)

HTH Emil