lkml.org 
[lkml]   [2021]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectFwd: [PATCH 2/2] drm/panel: Add ilitek ili9341 panel driver
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

\
 
 \ /
  Last update: 2021-07-21 10:27    [W:2.300 / U:0.988 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site