Hi Jerry,
On Thu, 25 Apr 2019 at 08:40, Jerry Han <jerry.han.hq@xxxxxxxxx> 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@xxxxxxxxx>
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Reviewed-by: Derek Basehore <dbasehore@xxxxxxxxxxxx>
> Cc: Jitao Shi <jitao.shi@xxxxxxxxxxxx>
> Cc: Rock wang <rock_wang@xxxxxxxxxxxx>
> ---
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
