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

[Emil Velikov]

Hi Jerry,

On Thu, 25 Apr 2019 at 08:40, Jerry Han <jerry.han.hq at 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 at gmail.com>
> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
> Reviewed-by: Derek Basehore <dbasehore at chromium.org>
> Cc: Jitao Shi <jitao.shi at mediatek.com>
> Cc: Rock wang <rock_wang at 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