Messages in this thread | | | From | Dillon Min <> | Date | Wed, 21 Jul 2021 16:13:43 +0800 | 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@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
| |