Hi Linus,

Thanks for your detailed review.

On Sun, 18 Jul 2021 at 08:31, Linus Walleij <linus.walleij@linaro.org>
wrote:

> Hi Dillon,
>
> thanks for your patch!
>
> On Fri, Jul 16, 2021 at 12:20 PM <dillon.minfei@gmail.com> wrote:
>
> > From: Dillon Min <dillon.minfei@gmail.com>
> >
> > This driver combine tiny/ili9341.c mipi_dbi_interface driver
> > with mipi_dpi_interface driver, can support ili9341 with serial
> > mode or parallel rgb interface mode by register configuration.
> >
> > Signed-off-by: Dillon Min <dillon.minfei@gmail.com>
>
> Nice!
>
> > +config DRM_PANEL_ILITEK_ILI9341
> > +       tristate "Ilitek ILI9341 240x320 QVGA panels"
> > +       depends on OF && SPI
> > +       depends on DRM_KMS_HELPER
> > +       depends on DRM_KMS_CMA_HELPER
> (...)
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_gem_atomic_helper.h>
> > +#include <drm/drm_atomic_helper.h>
>
> Hm now there is a (partial) KMS driver in the panel driver, kinda, sorta.
> Is this the right split? I'm not the best with DRM infrastructure,
> just asking.
>

I tried to remove one of these two headers, but got compile errors:

linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:719:3: error: implicit
declaration of function 'drm_atomic_helper_shutdown' [-Werror
=implicit-function-declaration]
  719 |   drm_atomic_helper_shutdown(drm);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
or

linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:562:16: error:
'drm_gem_simple_display_pipe_prepare_fb' undeclared here (not in a function)
  562 |  .prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Actually, these two headers are merged from tiny/ili9341.c to support only-
DBI interface, I'm
not sure whether the maintainers will ask me to remove (tiny/ili9341.c)
code from this patch.
If so, I will remove these headers.

But, It's a little strange to support different interfaces from different
drivers.


> > +struct ili9341_config {
> > +       u32 max_spi_speed;
> > +       /** @mode: the drm display mode */
> > +       const struct drm_display_mode mode;
> > +       /* @ca: TODO: need comments for this register */
> > +       u8 ca[ILI9341_CA_LEN];
>
> These are all in the datasheet but I guess you plan to add these
> TODOs later. (It's fine I suppose, the driver is already very nice.)
>

Yes, I didn't get detailed information about these registers from this
panel datasheet, so leave TODOs here.


> > +       struct regulator *vcc;
>
> Use the right name of the pin for the regulator. I guess this is actually
> vci. I would implement all three regulators and get them as bulk.
> See e.g. drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> for an example on how to get and enable several regulators
> using bulk.
>
> The regulator framework will provide dummy regulators if you
> didn't define some of them so it is fine to just provide one or two.
>
>
Really appreciate your suggestion, will add to v2.


> Yours,
> Linus Walleij
>