Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Dmitry Baryshkov]

On Thu, Apr 18, 2024 at 09:11:37PM +0800, Hsin-Yi Wang wrote:
> On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> >
> > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote:
> > > The kingdisplay panel is based on JD9365DA controller.
> > > Add a driver for it.
> > >
> > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/panel/Kconfig                 |   9 +
> > >  drivers/gpu/drm/panel/Makefile                |   1 +
> > >  .../drm/panel/panel-kingdisplay-kd101ne3.c    | 607 ++++++++++++++++++
> > >  3 files changed, 617 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > >
> > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > > index 154f5bf82980..2c73086cf102 100644
> > > --- a/drivers/gpu/drm/panel/Kconfig
> > > +++ b/drivers/gpu/drm/panel/Kconfig
> > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
> > >         24 bit RGB per pixel. It provides a MIPI DSI interface to
> > >         the host and has a built-in LED backlight.
> > >
> > > +config DRM_PANEL_KINGDISPLAY_KD101NE3
> > > +     tristate "Kingdisplay kd101ne3 panel"
> > > +     depends on OF
> > > +     depends on DRM_MIPI_DSI
> > > +     depends on BACKLIGHT_CLASS_DEVICE
> > > +     help
> > > +       Say Y if you want to enable support for panels based on the
> > > +       Kingdisplay kd101ne3 controller.
> > > +
> > >  config DRM_PANEL_LEADTEK_LTK050H3146W
> > >       tristate "Leadtek LTK050H3146W panel"
> > >       depends on OF
> > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > > index 24a02655d726..cbd414b98bb0 100644
> > > --- a/drivers/gpu/drm/panel/Makefile
> > > +++ b/drivers/gpu/drm/panel/Makefile
> > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
> > >  obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
> > >  obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
> > >  obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
> > >  obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> > >  obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> > >  obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > new file mode 100644
> > > index 000000000000..dbf0992f8b81
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > @@ -0,0 +1,607 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Panels based on the JD9365DA display controller.
> > > + * Author: Zhaoxiong Lv <lvzhaoxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <drm/drm_mipi_dsi.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +#include <video/mipi_display.h>
> > > +
> > > +struct panel_desc {
> > > +     const struct drm_display_mode *modes;
> > > +     unsigned int bpc;
> > > +
> > > +     /**
> > > +      * @width_mm: width of the panel's active display area
> > > +      * @height_mm: height of the panel's active display area
> > > +      */
> > > +     struct {
> > > +             unsigned int width_mm;
> > > +             unsigned int height_mm;
> >
> > Please move to the declared mode;
> >
> > > +     } size;
> > > +
> > > +     unsigned long mode_flags;
> > > +     enum mipi_dsi_pixel_format format;
> > > +     const struct panel_init_cmd *init_cmds;
> > > +     unsigned int lanes;
> > > +     bool discharge_on_disable;
> > > +     bool lp11_before_reset;
> > > +};
> > > +
> > > +struct kingdisplay_panel {
> > > +     struct drm_panel base;
> > > +     struct mipi_dsi_device *dsi;
> > > +
> > > +     const struct panel_desc *desc;
> > > +
> > > +     enum drm_panel_orientation orientation;
> > > +     struct regulator *pp3300;
> > > +     struct gpio_desc *enable_gpio;
> > > +};
> > > +
> > > +enum dsi_cmd_type {
> > > +     INIT_DCS_CMD,
> > > +     DELAY_CMD,
> > > +};
> > > +
> > > +struct panel_init_cmd {
> > > +     enum dsi_cmd_type type;
> > > +     size_t len;
> > > +     const char *data;
> > > +};
> > > +
> > > +#define _INIT_DCS_CMD(...) { \
> > > +     .type = INIT_DCS_CMD, \
> > > +     .len = sizeof((char[]){__VA_ARGS__}), \
> > > +     .data = (char[]){__VA_ARGS__} }
> > > +
> > > +#define _INIT_DELAY_CMD(...) { \
> > > +     .type = DELAY_CMD,\
> > > +     .len = sizeof((char[]){__VA_ARGS__}), \
> > > +     .data = (char[]){__VA_ARGS__} }
> >
> > This is the third panel driver using the same appoach. Can you use
> > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > the table, we should extract this framework to a common helper.
> > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> >
> The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> kernel size grows a lot since every sequence will be expanded.
> 
> Similar discussion in here:
> https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@xxxxxxxxxxxxxx/
> 
> This patch would increase the module size from 157K to 572K.
> scripts/bloat-o-meter shows chg +235.95%.
> 
> So maybe the common helper is better regarding the kernel module size?

Yes, let's get a framework done in a useful way.
I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
used instead (and it's up to the developer to select correct delay
function).

> 
> > > +
> > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > +     _INIT_DELAY_CMD(50),
> > > +     _INIT_DCS_CMD(0xE0, 0x00),

[skipped the body of the table]

> > > +     _INIT_DCS_CMD(0x0E, 0x48),
> > > +
> > > +     _INIT_DCS_CMD(0xE0, 0x00),

> > > +     _INIT_DCS_CMD(0X11),

Also, at least this is mipi_dsi_dcs_exit_sleep_mode().

> > > +     /* T6: 120ms */
> > > +     _INIT_DELAY_CMD(120),
> > > +     _INIT_DCS_CMD(0X29),

And this is mipi_dsi_dcs_set_display_on().

Having a single table enourages people to put known commands into the
table, the practice that must be frowned upon and forbidden.

We have functions for some of the standard DCS commands. So, maybe
instead of adding a single-table based approach we can improve
mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
error handling to a common part of enable() / prepare() function.

> > > +     _INIT_DELAY_CMD(20),
> > > +     {},
> > > +};

-- 
With best wishes
Dmitry