<div dir="ltr">Hi Emil Velikov:<div><br><div><span style="color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;font-variant-numeric:normal;font-variant-east-asian:normal;line-height:25.9875px;text-align:justify;background-color:rgb(247,248,250)">First of all, thank you for your comments.</span><br></div><div><br></div><div><span class="gmail-im" style="color:rgb(80,0,80)">> + struct gpio_desc *enable_gpio;<br>> + struct gpio_desc *pp33_gpio;<br>> + struct gpio_desc *pp18_gpio;<br></span>DT claims that these gpios are required, yet one is using the optional gpio API.<br>Is the code off, or does the DT need fixing? </div><div><br></div><div><span style="color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:14px;font-variant-numeric:normal;font-variant-east-asian:normal;line-height:26px;text-align:justify;background-color:rgb(247,248,250)">Thank you for your advice</span> , I will fix code.<br></div><div><br></div><div><span style="color:rgb(80,0,80)">static int send_mipi_cmds(struct drm_panel *panel, const struct panel_cmd *cmds)</span> </div><div>This format seems semi-magical. Any particular reason why you're not<br>using the helpers?<br>In particular, enter/exit sleep mode and set display on/off. </div><div><br></div><div><span style="color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;font-variant-numeric:normal;font-variant-east-asian:normal;line-height:25.9875px;text-align:justify;background-color:rgb(247,248,250)">Normally, we only need to download 0x28 0x10 commands to power-key down. </span></div><div><span style="color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;font-variant-numeric:normal;font-variant-east-asian:normal;line-height:25.9875px;text-align:justify;background-color:rgb(247,248,250)">I noticed that helper func (</span>mipi_dsi_dcs_set_display_off, mipi_dsi_dcs_enter_sleep_mode) help me send this commands(0x28 0x10).</div><div><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify"><br></span></div><div><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify">But based on previous debugging experience,</span><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify"> </span><span style="background-color:rgb(247,248,250);color:rgb(74,144,226);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify">Some vendors are special and may send other commands after sending cmd 0x28 0x10 or before send cmd 0x28 0x10.</span></div><div><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify">this problem may also occur in our vendor, </span></div><div><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify">so i think </span><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify">this above approach helps me manage code better.</span></div><div><span style="background-color:rgb(247,248,250);color:rgb(51,51,51);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify"><br></span></div><div style="text-align:justify"><font color="#333333" face="Arial, Microsoft YaHei, \\5FAE软雅黑, \\5B8B体, Malgun Gothic, Meiryo, sans-serif"><span style="font-size:13.6px;background-color:rgb(247,248,250)">Thanks.</span></font></div><div><span style="background-color:rgb(247,248,250);color:rgb(74,144,226);font-family:Arial,"Microsoft YaHei","\005fae\008f6f\0096c5\009ed1","\005b8b\004f53","Malgun Gothic",Meiryo,sans-serif;font-size:13.6px;text-align:justify"><br></span></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> 于2019年7月1日周一 下午9:41写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Jerry,<br>
<br>
On Thu, 25 Apr 2019 at 08:40, Jerry Han <<a href="mailto:jerry.han.hq@gmail.com" target="_blank">jerry.han.hq@gmail.com</a>> wrote:<br>
><br>
> Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI<br>
> panel.<br>
><br>
> V8:<br>
> - Modify communication address<br>
><br>
> V7:<br>
> - Add the information of the reviewer<br>
> - Remove unnecessary delays, The udelay_range code gracefully returns<br>
> without hitting the scheduler on a delay of 0. (Derek)<br>
> - Merge the same data structures, like display_mode and off_cmds (Derek)<br>
> - Optimize the processing of results returned by<br>
> devm_gpiod_get_optional (Derek)<br>
><br>
> V6:<br>
> - Add the information of the reviewer (Sam)<br>
> - Delete unnecessary header files #include <linux/fb.h> (Sam)<br>
> - The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them (Sam)<br>
> - ADD static, set_gpios function is not used outside this module (Sam)<br>
><br>
> V5:<br>
> - Added changelog<br>
><br>
> V4:<br>
> - Frefix all function maes with boe_ (Sam)<br>
> - Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam)<br>
> - Sort include lines alphabetically (Sam)<br>
> - Fixed entries in the makefile must be sorted alphabetically (Sam)<br>
> - Add send_mipi_cmds function to avoid duplicating the code (Sam)<br>
> - Add the necessary delay(reset_delay_t5) between reset and sending<br>
> the initialization command (Rock wang)<br>
><br>
> V3:<br>
> - Remove unnecessary delays in sending initialization commands (Jitao Shi)<br>
><br>
> V2:<br>
> - Use SPDX identifier (Sam)<br>
> - Use necessary header files replace drmP.h (Sam)<br>
> - Delete unnecessary header files #include <linux/err.h> (Sam)<br>
> - Specifies a GPIOs array to control the reset timing,<br>
> instead of reading "dsi-reset-sequence" data from DTS (Sam)<br>
> - Delete backlight_disable() function when already disabled (Sam)<br>
> - Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam)<br>
> - Move the necessary data in the DTS to the current file,<br>
> like porch, display_mode and Init code etc. (Sam)<br>
> - Add compatible device "boe,himax8279d10p" (Sam)<br>
><br>
> Signed-off-by: Jerry Han <<a href="mailto:jerry.han.hq@gmail.com" target="_blank">jerry.han.hq@gmail.com</a>><br>
> Reviewed-by: Sam Ravnborg <<a href="mailto:sam@ravnborg.org" target="_blank">sam@ravnborg.org</a>><br>
> Reviewed-by: Derek Basehore <<a href="mailto:dbasehore@chromium.org" target="_blank">dbasehore@chromium.org</a>><br>
> Cc: Jitao Shi <<a href="mailto:jitao.shi@mediatek.com" target="_blank">jitao.shi@mediatek.com</a>><br>
> Cc: Rock wang <<a href="mailto:rock_wang@himax.com.cn" target="_blank">rock_wang@himax.com.cn</a>><br>
> ---<br>
While the DT has landed, this patch is not merged it seems.<br>
I think that Sam is waiting for a version which does not trigger so<br>
many check-patch warnings.<br>
<br>
That said, a couple of comments if I may.<br>
<br>
> + struct gpio_desc *enable_gpio;<br>
> + struct gpio_desc *pp33_gpio;<br>
> + struct gpio_desc *pp18_gpio;<br>
DT claims that these gpios are required, yet one is using the optional gpio API.<br>
Is the code off, or does the DT need fixing?<br>
<br>
<br>
> +static int send_mipi_cmds(struct drm_panel *panel, const struct panel_cmd *cmds)<br>
> +{<br>
> + struct panel_info *pinfo = to_panel_info(panel);<br>
> + unsigned int i = 0;<br>
> + int err;<br>
> +<br>
> + if (!cmds)<br>
> + return -EFAULT;<br>
> +<br>
> + for (i = 0; cmds[i].len != 0; i++) {<br>
> + const struct panel_cmd *cmd = &cmds[i];<br>
> +<br>
> + if (cmd->len == 2)<br>
> + err = mipi_dsi_dcs_write(pinfo->link,<br>
> + cmd->data[1], NULL, 0);<br>
> + else<br>
> + err = mipi_dsi_dcs_write(pinfo->link,<br>
> + cmd->data[1], cmd->data + 2,<br>
> + cmd->len - 2);<br>
> +<br>
> + if (err < 0)<br>
> + return err;<br>
> +<br>
> + usleep_range((cmd->data[0]) * 1000,<br>
> + (1 + cmd->data[0]) * 1000);<br>
> + }<br>
> +<br>
This format seems semi-magical. Any particular reason why you're not<br>
using the helpers?<br>
In particular, enter/exit sleep mode and set display on/off.<br>
<br>
Speaking of which - the 8inch display uses then, yet they are missing<br>
for the 10inch one. Seems like there's a bug in there.<br>
<br>
Plus we have some repeating patterns in the vendor/undocumented<br>
commands - good reason to get those into separate hunks.<br>
With the above in place, you can effectively drop the .off_cmd<br>
callback and sleep field from the INIT_CMD arrays.<br>
<br>
Hence, less code to debug and maintain ;-)<br>
<br>
HTH<br>
Emil<br>
</blockquote></div>