From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Hsin-Yi Wang <hsinyi@google.com>
Cc: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>,
mripard@kernel.org, airlied@gmail.com, daniel@ffwll.ch,
robh@kernel.org, krzysztof.kozlowski+dt@linaro.org,
conor+dt@kernel.org, dianders@google.com,
dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
Date: Thu, 18 Apr 2024 17:11:02 +0300 [thread overview]
Message-ID: <vbt2nxddw2dc7hkreq4iybv5zv5xyp32oajybeqsphgfrhzmn7@tskvckljmxpe> (raw)
In-Reply-To: <CACb=7PURWtS8bwT5EcAFHhu7deHd2Y8cNOattfdwyEYpOUcbnQ@mail.gmail.com>
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@linaro.org> 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@huaqin.corp-partner.google.com>
> > > ---
> > > 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@huaqin.corp-partner.google.com>
> > > + */
> > > +
> > > +#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@mail.gmail.com/
>
> 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
next prev parent reply other threads:[~2024-04-18 14:11 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
2024-04-18 22:59 ` Krzysztof Kozlowski
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
2024-04-18 11:46 ` Dmitry Baryshkov
2024-04-18 13:11 ` Hsin-Yi Wang
2024-04-18 14:11 ` Dmitry Baryshkov [this message]
2024-04-23 18:10 ` Hsin-Yi Wang
2024-04-23 20:41 ` Doug Anderson
2024-04-23 21:20 ` Dmitry Baryshkov
2024-04-24 21:04 ` Doug Anderson
2024-04-24 21:18 ` Hsin-Yi Wang
2024-04-24 21:49 ` Dmitry Baryshkov
2024-04-24 22:15 ` Hsin-Yi Wang
2024-04-24 22:50 ` Dmitry Baryshkov
2024-04-24 23:25 ` Doug Anderson
2024-04-24 23:37 ` Dmitry Baryshkov
2024-04-25 0:21 ` Doug Anderson
2024-04-24 22:27 ` Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vbt2nxddw2dc7hkreq4iybv5zv5xyp32oajybeqsphgfrhzmn7@tskvckljmxpe \
--to=dmitry.baryshkov@linaro.org \
--cc=airlied@gmail.com \
--cc=conor+dt@kernel.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dianders@google.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hsinyi@google.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lvzhaoxiong@huaqin.corp-partner.google.com \
--cc=mripard@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).