linux-kernel - Fwd: [PATCH 2/2] drm/panel: Add ilitek ili9341 panel driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Wed, 21 Jul 2021 16:13:43 +0800
From:   Dillon Min <dillon.minfei@...il.com>
To:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Fwd: [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@...aro.org> wrote:
>
> Hi Dillon,
>
> thanks for your patch!
>
> On Fri, Jul 16, 2021 at 12:20 PM <dillon.minfei@...il.com> wrote:
>
> > From: Dillon Min <dillon.minfei@...il.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@...il.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