<div dir="ltr"><div dir="ltr">Hi Linus,<div><br></div><div>Thanks for your detailed review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 18 Jul 2021 at 08:31, Linus <span class="" id=":1i5.1" tabindex="-1" role="menuitem" aria-haspopup="true" style="">Walleij</span> <<span class="" id=":1i5.2" tabindex="-1" role="menuitem" aria-haspopup="true" style="">linus</span>.<span class="" id=":1i5.3" tabindex="-1" role="menuitem" aria-haspopup="true" style="">walleij</span>@<span class="" id=":1i5.4" tabindex="-1" role="menuitem" aria-haspopup="true" style="">linaro</span>.org> wrote:<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 Dillon,<br>
<br>
thanks for your patch!<br>
<br>
On Fri, Jul 16, 2021 at 12:20 PM <<a href="mailto:dillon.minfei@gmail.com" target="_blank">dillon.minfei@gmail.com</a>> wrote:<br>
<br>
> From: Dillon Min <<a href="mailto:dillon.minfei@gmail.com" target="_blank">dillon.minfei@gmail.com</a>><br>
><br>
> This driver combine tiny/ili9341.c mipi_dbi_interface driver<br>
> with mipi_dpi_interface driver, can support ili9341 with serial<br>
> mode or parallel rgb interface mode by register configuration.<br>
><br>
> Signed-off-by: Dillon Min <<a href="mailto:dillon.minfei@gmail.com" target="_blank">dillon.minfei@gmail.com</a>><br>
<br>
Nice!<br>
<br>
> +config DRM_PANEL_ILITEK_ILI9341<br>
> + tristate "Ilitek ILI9341 240x320 QVGA panels"<br>
> + depends on OF && SPI<br>
> + depends on DRM_KMS_HELPER<br>
> + depends on DRM_KMS_CMA_HELPER<br>
(...)<br>
> +#include <drm/drm_gem_framebuffer_helper.h><br>
> +#include <drm/drm_gem_cma_helper.h><br>
> +#include <drm/drm_fb_helper.h><br>
> +#include <drm/drm_gem_atomic_helper.h><br>
> +#include <drm/drm_atomic_helper.h><br>
<br>
Hm now there is a (partial) KMS driver in the panel driver, kinda, sorta.<br>
Is this the right split? I'm not the best with DRM infrastructure,<br>
just asking.<br></blockquote><div><br></div><div>I tried to remove one of these two headers, but got compile errors:</div><div class="gmail_quote"><br></div><span class="" id=":1i5.5" tabindex="-1" role="menuitem" aria-haspopup="true" style="">linux</span>/drivers/<span class="" id=":1i5.6" tabindex="-1" role="menuitem" aria-haspopup="true" style="">gpu</span>/<span class="" id=":1i5.7" tabindex="-1" role="menuitem" aria-haspopup="true" style="">drm</span>/panel/panel-<span class="" id=":1i5.8" tabindex="-1" role="menuitem" aria-haspopup="true" style="">ilitek</span>-ili9341.c:719:3: error: implicit declaration of function 'drm_atomic_helper_shutdown' [-<span class="" id=":1i5.9" tabindex="-1" role="menuitem" aria-haspopup="true" style="">Werror</span>=implicit-function-declaration]<br> 719 | <span class="" id=":1i5.10" tabindex="-1" role="menuitem" aria-haspopup="true" style="">drm</span>_atomic_helper_shutdown(<span class="" id=":1i5.11" tabindex="-1" role="menuitem" aria-haspopup="true" style="">drm</span>);<br> | ^~~~~~~~~~~~~~~~~~~~~~~~~~<br><div>or</div><div class="gmail_quote"><br></div><span class="" id=":1i5.12" tabindex="-1" role="menuitem" aria-haspopup="true" style="">linux</span>/drivers/<span class="" id=":1i5.13" tabindex="-1" role="menuitem" aria-haspopup="true" style="">gpu</span>/<span class="" id=":1i5.14" tabindex="-1" role="menuitem" aria-haspopup="true" style="">drm</span>/panel/panel-<span class="" id=":1i5.15" tabindex="-1" role="menuitem" aria-haspopup="true" style="">ilitek</span>-ili9341.c:562:16: error: 'drm_gem_simple_display_pipe_prepare_fb' undeclared here (not in a function)<br> 562 | .prepare_<span class="" id=":1i5.16" tabindex="-1" role="menuitem" aria-haspopup="true" style="">fb</span> = <span class="" id=":1i5.17" tabindex="-1" role="menuitem" aria-haspopup="true" style="">drm</span>_gem_simple_display_pipe_prepare_<span class="" id=":1i5.18" tabindex="-1" role="menuitem" aria-haspopup="true" style="">fb</span>,<br> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br><div><br></div><div>Actually, these two headers are merged from tiny/ili9341.c to support only-<span class="" id=":1i5.19" tabindex="-1" role="menuitem" aria-haspopup="true" style="">DBI</span> interface, I'm</div><div>not sure whether the maintainers will ask me to remove (tiny/ili9341.c) code from this patch.</div><div>If so, I will remove these headers.</div><div><br></div><div>But, It's a little strange to support different interfaces from different drivers.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +struct ili9341_config {<br>
> + u32 max_spi_speed;<br>
> + /** @mode: the drm display mode */<br>
> + const struct drm_display_mode mode;<br>
> + /* @ca: TODO: need comments for this register */<br>
> + u8 ca[ILI9341_CA_LEN];<br>
<br>
These are all in the datasheet but I guess you plan to add these<br>
TODOs later. (It's fine I suppose, the driver is already very nice.)<br></blockquote><div><br></div><div>Yes, I didn't get detailed information about these registers from this</div><div>panel <span class="" id=":1i5.20" tabindex="-1" role="menuitem" aria-haspopup="true" style="">datasheet</span>, so leave <span class="" id=":1i5.21" tabindex="-1" role="menuitem" aria-haspopup="true" style="">TODOs</span> here.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + struct regulator *vcc;<br>
<br>
Use the right name of the pin for the regulator. I guess this is actually<br>
vci. I would implement all three regulators and get them as bulk.<br>
See e.g. drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c<br>
for an example on how to get and enable several regulators<br>
using bulk.<br>
<br>
The regulator framework will provide dummy regulators if you<br>
didn't define some of them so it is fine to just provide one or two.<br>
<br></blockquote><div><br></div><div>Really appreciate your suggestion, will add to v2.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Yours,<br>
Linus Walleij<br>
</blockquote></div></div>