Hi,
On Mon, Apr 22, 2024 at 2:03 AM Cong Yang
<[email protected]> wrote:
>
> The Starry HX83102 based mipi panel should never have been part of the boe
> tv101wum driver. Discussion with Doug and Linus in V1 [1], we need a
> separate driver to enable the hx83102 controller.
>
> In hx83102 driver, add DSI commands as macros. So it can add some panels
> with same control model in the future.
>
> [1]: https://lore.kernel.org/all/CACRpkdbzYZAS0=zBQJUC4CB2wj4s1h6n6aSAZQvdMV95r3zRUw@mail.gmail.com
>
> Signed-off-by: Cong Yang <[email protected]>
> ---
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile | 1 +
> .../gpu/drm/panel/panel-boe-tv101wum-nl6.c | 99 ---
> drivers/gpu/drm/panel/panel-himax-hx83102.c | 567 ++++++++++++++++++
> 4 files changed, 577 insertions(+), 99 deletions(-)
> create mode 100644 drivers/gpu/drm/panel/panel-himax-hx83102.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d037b3b8b999..eb378c897353 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -145,6 +145,15 @@ config DRM_PANEL_LVDS
> handling of power supplies or control signals. It implements automatic
> backlight handling if the panel is attached to a backlight controller.
>
> +config DRM_PANEL_HIMAX_HX83102
> + tristate "himax HX83102-based panels"
Capital "h" for "Himax".
> +#define DRV_NAME "panel-himax-hx83102"
I don't think DRV_NAME buys you very much. Get rid of this #define and
just use it below.
> +struct hx83102 {
> + struct drm_panel base;
> + struct mipi_dsi_device *dsi;
> +
> + const struct hx83102_panel_desc *desc;
> +
> + enum drm_panel_orientation orientation;
> + struct regulator *pp1800;
> + struct regulator *avee;
> + struct regulator *avdd;
> + struct gpio_desc *enable_gpio;
> +
> + bool prepared;
We're trying to get rid of the tracking of "prepared" in panels. You
should be able to delete this and remove the code dealing with it. The
core DRM code should ensure that your prepare/unprepare functions are
called appropriately.
> +struct hx83102_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;
> + } size;
> +
> + unsigned long mode_flags;
> + enum mipi_dsi_pixel_format format;
> + unsigned int lanes;
> + bool lp11_before_reset;
Seems like you can remove "lp11_before_reset" since it's always true
for this controller. If later you find someone using this controller
that needs this false then we can always add it back in.
I think you could also remove "bpc", "format", and "mode_flags". If
these are all the same controller then that will be common between all
the panels, right? So you shouldn't need to define those on a
per-panel basis... You could maybe even remove "lanes" unless some
people using this panel are expected to hook up fewer lanes...
> +static int starry_init_cmd(struct hx83102 *ctx)
> +{
> + struct mipi_dsi_device *dsi = ctx->dsi;
> +
> + mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> +
> + mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> + 0x36, 0x36, 0x36, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> + 0xFF, 0x8F, 0xFF, 0x08, 0x74, 0x33);
I know this is a sticking point between Linus W. and me, but I'm
really not a fan of all the hardcoded function calls since it bloats
the code so much. I think we need to stick with something more table
based at least for the majority of the commands. If I understand
correctly, Linus was OK w/ something table based as long as it was in
common code [1]. I think he also wanted the "delay" out of the table,
but since those always seem to be at the beginning or the end it seems
like we could still have the majority of the code as table based. Do
you want to make an attempt at that? If not I can try to find some
time to write up a patch in the next week or so.
[1] https://lore.kernel.org/r/CACRpkdYtM=5jdQddCqRFgBRXvcJEjk1ULJNKKFz7jhhkGxV59Q@mail.gmail.com
..also: nit that, in general, the Linux community prefers lowercase
hex, so 0xfa instead of 0xFA, for instance.
> +static int hx83102_get_modes(struct drm_panel *panel,
> + struct drm_connector *connector)
> +{
> + struct hx83102 *ctx = panel_to_hx83102(panel);
> + const struct drm_display_mode *m = ctx->desc->modes;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(connector->dev, m);
> + if (!mode) {
> + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> + return -ENOMEM;
> + }
nit: no need for an error message when drm_mode_duplicate() fails. It
is incredibly unlikely that the allocation will fail and the Linux
kernel will already do a stack crawl in the case that it does fail.
> + /*
> + * TODO: Remove once all drm drivers call
> + * drm_connector_set_orientation_from_panel()
> + */
> + drm_connector_set_panel_orientation(connector, ctx->orientation);
Pretty sure you can drop the call to
drm_connector_set_panel_orientation() here. I believe that nearly
everyone is using panel_bridge which will handle this for you.
> +static int hx83102_panel_add(struct hx83102 *ctx)
> +{
> + struct device *dev = &ctx->dsi->dev;
> + int err;
> +
> + ctx->avdd = devm_regulator_get(dev, "avdd");
> + if (IS_ERR(ctx->avdd))
> + return PTR_ERR(ctx->avdd);
> +
> + ctx->avee = devm_regulator_get(dev, "avee");
> + if (IS_ERR(ctx->avee))
> + return PTR_ERR(ctx->avee);
> +
> + ctx->pp1800 = devm_regulator_get(dev, "pp1800");
> + if (IS_ERR(ctx->pp1800))
> + return PTR_ERR(ctx->pp1800);
> +
> + ctx->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(ctx->enable_gpio)) {
> + dev_err(dev, "cannot get reset-gpios %ld\n",
it's not "reset-gpios".
> + PTR_ERR(ctx->enable_gpio));
> + return PTR_ERR(ctx->enable_gpio);
Rather than the above, use "dev_err_probe" and combine into one line. Untested:
if (IS_ERR(ctx->enable_gpio))
return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio), "Cannot get
enable GPIO\n");
> + }
> +
> + gpiod_set_value(ctx->enable_gpio, 0);
You don't need the above line. When you got the GPIO you already
passed "GPIOD_OUT_LOW" which means that this is redundant.
> +
> + ctx->base.prepare_prev_first = true;
> +
> + drm_panel_init(&ctx->base, dev, &hx83102_drm_funcs,
> + DRM_MODE_CONNECTOR_DSI);
> + err = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
> + if (err < 0) {
> + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err);
> + return err;
> + }
Could also use "dev_err_probe" to make the above one line. Not sure
you really need the of_node name, too...
if (err < 0)
return dev_err_probe(dev, err, "failed to get orientation\n");
-Doug
Hi,
Thanks for review.
Doug Anderson <[email protected]> 于2024年4月23日周二 05:24写道：
>
> Hi,
>
> On Mon, Apr 22, 2024 at 2:03 AM Cong Yang
> <[email protected]> wrote:
> >
> > The Starry HX83102 based mipi panel should never have been part of the boe
> > tv101wum driver. Discussion with Doug and Linus in V1 [1], we need a
> > separate driver to enable the hx83102 controller.
> >
> > In hx83102 driver, add DSI commands as macros. So it can add some panels
> > with same control model in the future.
> >
> > [1]: https://lore.kernel.org/all/CACRpkdbzYZAS0=zBQJUC4CB2wj4s1h6n6aSAZQvdMV95r3zRUw@mail.gmail.com
> >
> > Signed-off-by: Cong Yang <[email protected]>
> > ---
> > drivers/gpu/drm/panel/Kconfig | 9 +
> > drivers/gpu/drm/panel/Makefile | 1 +
> > .../gpu/drm/panel/panel-boe-tv101wum-nl6.c | 99 ---
> > drivers/gpu/drm/panel/panel-himax-hx83102.c | 567 ++++++++++++++++++
> > 4 files changed, 577 insertions(+), 99 deletions(-)
> > create mode 100644 drivers/gpu/drm/panel/panel-himax-hx83102.c
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index d037b3b8b999..eb378c897353 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -145,6 +145,15 @@ config DRM_PANEL_LVDS
> > handling of power supplies or control signals. It implements automatic
> > backlight handling if the panel is attached to a backlight controller.
> >
> > +config DRM_PANEL_HIMAX_HX83102
> > + tristate "himax HX83102-based panels"
>
> Capital "h" for "Himax".
Got it, fix in V3. Thanks.
>
>
> > +#define DRV_NAME "panel-himax-hx83102"
>
> I don't think DRV_NAME buys you very much. Get rid of this #define and
> just use it below.
Got it, fix in V3. Thanks.
>
>
> > +struct hx83102 {
> > + struct drm_panel base;
> > + struct mipi_dsi_device *dsi;
> > +
> > + const struct hx83102_panel_desc *desc;
> > +
> > + enum drm_panel_orientation orientation;
> > + struct regulator *pp1800;
> > + struct regulator *avee;
> > + struct regulator *avdd;
> > + struct gpio_desc *enable_gpio;
> > +
> > + bool prepared;
>
> We're trying to get rid of the tracking of "prepared" in panels. You
> should be able to delete this and remove the code dealing with it. The
> core DRM code should ensure that your prepare/unprepare functions are
> called appropriately.
Got it, fix in V3. Thanks.
>
>
>
> > +struct hx83102_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;
> > + } size;
> > +
> > + unsigned long mode_flags;
> > + enum mipi_dsi_pixel_format format;
> > + unsigned int lanes;
> > + bool lp11_before_reset;
>
> Seems like you can remove "lp11_before_reset" since it's always true
> for this controller. If later you find someone using this controller
> that needs this false then we can always add it back in.
>
> I think you could also remove "bpc", "format", and "mode_flags". If
> these are all the same controller then that will be common between all
> the panels, right? So you shouldn't need to define those on a
> per-panel basis... You could maybe even remove "lanes" unless some
> people using this panel are expected to hook up fewer lanes...
Okay, remove “lanes” together.
>
>
> > +static int starry_init_cmd(struct hx83102 *ctx)
> > +{
> > + struct mipi_dsi_device *dsi = ctx->dsi;
> > +
> > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> > +
> > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > + 0x36, 0x36, 0x36, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > + 0xFF, 0x8F, 0xFF, 0x08, 0x74, 0x33);
>
> I know this is a sticking point between Linus W. and me, but I'm
> really not a fan of all the hardcoded function calls since it bloats
> the code so much. I think we need to stick with something more table
> based at least for the majority of the commands. If I understand
> correctly, Linus was OK w/ something table based as long as it was in
> common code [1]. I think he also wanted the "delay" out of the table,
> but since those always seem to be at the beginning or the end it seems
> like we could still have the majority of the code as table based. Do
> you want to make an attempt at that? If not I can try to find some
> time to write up a patch in the next week or so.
Do you mean not add "delay" in the table? However, the delay
required by each panel may be different. How should this be handled?
It would be great if you could help provide a patch. Thank you so much.
>
> [1] https://lore.kernel.org/r/CACRpkdYtM=5jdQddCqRFgBRXvcJEjk1ULJNKKFz7jhhkGxV59Q@mail.gmail.com
>
> ...also: nit that, in general, the Linux community prefers lowercase
> hex, so 0xfa instead of 0xFA, for instance.
Done. Fix in V3.
>
>
> > +static int hx83102_get_modes(struct drm_panel *panel,
> > + struct drm_connector *connector)
> > +{
> > + struct hx83102 *ctx = panel_to_hx83102(panel);
> > + const struct drm_display_mode *m = ctx->desc->modes;
> > + struct drm_display_mode *mode;
> > +
> > + mode = drm_mode_duplicate(connector->dev, m);
> > + if (!mode) {
> > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> > + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> > + return -ENOMEM;
> > + }
>
> nit: no need for an error message when drm_mode_duplicate() fails. It
> is incredibly unlikely that the allocation will fail and the Linux
> kernel will already do a stack crawl in the case that it does fail.
Got it, fix in V3. Thanks.
>
>
> > + /*
> > + * TODO: Remove once all drm drivers call
> > + * drm_connector_set_orientation_from_panel()
> > + */
> > + drm_connector_set_panel_orientation(connector, ctx->orientation);
>
> Pretty sure you can drop the call to
> drm_connector_set_panel_orientation() here. I believe that nearly
> everyone is using panel_bridge which will handle this for you.
Got it, fix in V3. Thanks.
>
>
> > +static int hx83102_panel_add(struct hx83102 *ctx)
> > +{
> > + struct device *dev = &ctx->dsi->dev;
> > + int err;
> > +
> > + ctx->avdd = devm_regulator_get(dev, "avdd");
> > + if (IS_ERR(ctx->avdd))
> > + return PTR_ERR(ctx->avdd);
> > +
> > + ctx->avee = devm_regulator_get(dev, "avee");
> > + if (IS_ERR(ctx->avee))
> > + return PTR_ERR(ctx->avee);
> > +
> > + ctx->pp1800 = devm_regulator_get(dev, "pp1800");
> > + if (IS_ERR(ctx->pp1800))
> > + return PTR_ERR(ctx->pp1800);
> > +
> > + ctx->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > + if (IS_ERR(ctx->enable_gpio)) {
> > + dev_err(dev, "cannot get reset-gpios %ld\n",
>
> it's not "reset-gpios".
>
>
> > + PTR_ERR(ctx->enable_gpio));
> > + return PTR_ERR(ctx->enable_gpio);
>
> Rather than the above, use "dev_err_probe" and combine into one line. Untested:
>
> if (IS_ERR(ctx->enable_gpio))
> return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio), "Cannot get
> enable GPIO\n");
Got it, fix in V3. Thanks.
>
>
> > + }
> > +
> > + gpiod_set_value(ctx->enable_gpio, 0);
>
> You don't need the above line. When you got the GPIO you already
> passed "GPIOD_OUT_LOW" which means that this is redundant.
Got it, fix in V3. Thanks.
>
>
> > +
> > + ctx->base.prepare_prev_first = true;
> > +
> > + drm_panel_init(&ctx->base, dev, &hx83102_drm_funcs,
> > + DRM_MODE_CONNECTOR_DSI);
> > + err = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
> > + if (err < 0) {
> > + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err);
> > + return err;
> > + }
>
> Could also use "dev_err_probe" to make the above one line. Not sure
> you really need the of_node name, too...
Okay, fix in V3. Thanks.
>
> if (err < 0)
> return dev_err_probe(dev, err, "failed to get orientation\n");
>
> -Doug
Hi,
On Tue, Apr 23, 2024 at 2:37 AM cong yang
<[email protected]> wrote:
>
> > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > +{
> > > + struct mipi_dsi_device *dsi = ctx->dsi;
> > > +
> > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> > > +
> > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > + 0x36, 0x36, 0x36, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > + 0xFF, 0x8F, 0xFF, 0x08, 0x74, 0x33);
> >
> > I know this is a sticking point between Linus W. and me, but I'm
> > really not a fan of all the hardcoded function calls since it bloats
> > the code so much. I think we need to stick with something more table
> > based at least for the majority of the commands. If I understand
> > correctly, Linus was OK w/ something table based as long as it was in
> > common code [1]. I think he also wanted the "delay" out of the table,
> > but since those always seem to be at the beginning or the end it seems
> > like we could still have the majority of the code as table based. Do
> > you want to make an attempt at that? If not I can try to find some
> > time to write up a patch in the next week or so.
>
> Do you mean not add "delay" in the table? However, the delay
> required by each panel may be different. How should this be handled?
In the case of the "himax-hx83102" driver, it looks as if all the
delays are at the beginning or end of the init sequence. That means
you could just make those extra parameters that are set per-panel and
you're back to having a simple sequence without delays.
If you had panels that needed delays in a more complicated way, you
could keep the per-panel functions but just make the bulk of the
function calls apply a sequence. For instance:
static int my_panel_init_cmd(...)
{
ret = mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
if (ret)
return ret;
mdelay(100);
ret = mipi_dsi_dcs_write(dsi, ...);
if (ret)
return ret;
mdelay(50);
ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
if (ret)
return ret;
}
The vast majority of the work is still table driven so it doesn't
bloat the code, but you don't have the "delay" in the command sequence
since Linus didn't like it. I think something like the above would
make Linus happy and I'd be OK w/ it as well. Ideally you should still
make your command sequence as easy to understand as possible, kind of
like how we did with _INIT_SWITCH_PAGE_CMD() in
"panel-ilitek-ili9882t.c"
As part of this, you'd have to add a patch to create
mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
complicated?
> It would be great if you could help provide a patch. Thank you so much.
Sure, I can, though maybe you want to give it a shot with the above description?
-Doug
Hi,
Thanks reply.
Doug Anderson <[email protected]> 于2024年4月24日周三 00:26写道：
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:37 AM cong yang
> <[email protected]> wrote:
> >
> > > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > > +{
> > > > + struct mipi_dsi_device *dsi = ctx->dsi;
> > > > +
> > > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> > > > +
> > > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > > + 0x36, 0x36, 0x36, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > > + 0xFF, 0x8F, 0xFF, 0x08, 0x74, 0x33);
> > >
> > > I know this is a sticking point between Linus W. and me, but I'm
> > > really not a fan of all the hardcoded function calls since it bloats
> > > the code so much. I think we need to stick with something more table
> > > based at least for the majority of the commands. If I understand
> > > correctly, Linus was OK w/ something table based as long as it was in
> > > common code [1]. I think he also wanted the "delay" out of the table,
> > > but since those always seem to be at the beginning or the end it seems
> > > like we could still have the majority of the code as table based. Do
> > > you want to make an attempt at that? If not I can try to find some
> > > time to write up a patch in the next week or so.
> >
> > Do you mean not add "delay" in the table? However, the delay
> > required by each panel may be different. How should this be handled?
>
> In the case of the "himax-hx83102" driver, it looks as if all the
> delays are at the beginning or end of the init sequence. That means
> you could just make those extra parameters that are set per-panel and
> you're back to having a simple sequence without delays.
Do you mean add msleep in hx83102_enable()?
@@ -612,12 +604,15 @@ static int hx83102_enable(struct drm_panel *panel)
struct device *dev = &dsi->dev;
int ret;
+ msleep(60);
ret = ctx->desc->init_cmds(ctx);
if (ret) {
dev_err(dev, "Panel init cmds failed: %d\n", ret);
return ret;
}
+ msleep(60);
+
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>
> If you had panels that needed delays in a more complicated way, you
> could keep the per-panel functions but just make the bulk of the
> function calls apply a sequence. For instance:
>
> static int my_panel_init_cmd(...)
> {
> ret = mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
> if (ret)
> return ret;
> mdelay(100);
> ret = mipi_dsi_dcs_write(dsi, ...);
> if (ret)
> return ret;
> mdelay(50);
> ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> if (ret)
> return ret;
> }
>
> The vast majority of the work is still table driven so it doesn't
> bloat the code, but you don't have the "delay" in the command sequence
> since Linus didn't like it. I think something like the above would
> make Linus happy and I'd be OK w/ it as well. Ideally you should still
> make your command sequence as easy to understand as possible, kind of
> like how we did with _INIT_SWITCH_PAGE_CMD() in
> "panel-ilitek-ili9882t.c"
>
> As part of this, you'd have to add a patch to create
> mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
> complicated?
>
>
> > It would be great if you could help provide a patch. Thank you so much.
>
> Sure, I can, though maybe you want to give it a shot with the above description?
Sorry, I still don't seem to understand. How to encapsulate the parameters of
"HX83102_SETDISP, HX83102_SETCYC,....."? Different parameters for each panel.
I have sent my V3 but it does not contain the patch you want.
>
> -Doug
Hi,
On Tue, Apr 23, 2024 at 7:42 PM cong yang
<[email protected]> wrote:
>
> Hi,
> Thanks reply.
>
> Doug Anderson <[email protected]> 于2024年4月24日周三 00:26写道：
> >
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 2:37 AM cong yang
> > <[email protected]> wrote:
> > >
> > > > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > > > +{
> > > > > + struct mipi_dsi_device *dsi = ctx->dsi;
> > > > > +
> > > > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> > > > > +
> > > > > + mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > > > + 0x36, 0x36, 0x36, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > > > + 0xFF, 0x8F, 0xFF, 0x08, 0x74, 0x33);
> > > >
> > > > I know this is a sticking point between Linus W. and me, but I'm
> > > > really not a fan of all the hardcoded function calls since it bloats
> > > > the code so much. I think we need to stick with something more table
> > > > based at least for the majority of the commands. If I understand
> > > > correctly, Linus was OK w/ something table based as long as it was in
> > > > common code [1]. I think he also wanted the "delay" out of the table,
> > > > but since those always seem to be at the beginning or the end it seems
> > > > like we could still have the majority of the code as table based. Do
> > > > you want to make an attempt at that? If not I can try to find some
> > > > time to write up a patch in the next week or so.
> > >
> > > Do you mean not add "delay" in the table? However, the delay
> > > required by each panel may be different. How should this be handled?
> >
> > In the case of the "himax-hx83102" driver, it looks as if all the
> > delays are at the beginning or end of the init sequence. That means
> > you could just make those extra parameters that are set per-panel and
> > you're back to having a simple sequence without delays.
>
> Do you mean add msleep in hx83102_enable()?
>
> @@ -612,12 +604,15 @@ static int hx83102_enable(struct drm_panel *panel)
> struct device *dev = &dsi->dev;
> int ret;
>
> + msleep(60);
> ret = ctx->desc->init_cmds(ctx);
> if (ret) {
> dev_err(dev, "Panel init cmds failed: %d\n", ret);
> return ret;
> }
>
> + msleep(60);
> +
> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>
> >
> > If you had panels that needed delays in a more complicated way, you
> > could keep the per-panel functions but just make the bulk of the
> > function calls apply a sequence. For instance:
> >
> > static int my_panel_init_cmd(...)
> > {
> > ret = mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
> > if (ret)
> > return ret;
> > mdelay(100);
> > ret = mipi_dsi_dcs_write(dsi, ...);
> > if (ret)
> > return ret;
> > mdelay(50);
> > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> > if (ret)
> > return ret;
> > }
> >
> > The vast majority of the work is still table driven so it doesn't
> > bloat the code, but you don't have the "delay" in the command sequence
> > since Linus didn't like it. I think something like the above would
> > make Linus happy and I'd be OK w/ it as well. Ideally you should still
> > make your command sequence as easy to understand as possible, kind of
> > like how we did with _INIT_SWITCH_PAGE_CMD() in
> > "panel-ilitek-ili9882t.c"
> >
> > As part of this, you'd have to add a patch to create
> > mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
> > complicated?
> >
> >
> > > It would be great if you could help provide a patch. Thank you so much.
> >
> > Sure, I can, though maybe you want to give it a shot with the above description?
>
> Sorry, I still don't seem to understand. How to encapsulate the parameters of
> "HX83102_SETDISP, HX83102_SETCYC,....."? Different parameters for each panel.
> I have sent my V3 but it does not contain the patch you want.
It sounds as if Dmitry has come up with a solution that allows us to
keep using the functions like you've been using but avoid the code
bloat problems. ...so let's go with that. It sounds as if he's going
to send a patch before too long and then it should be pretty easy to
convert your patches over to use his new function.
[1] https://lore.kernel.org/r/CAA8EJprv3qBd1hfdWHrfhY=S0w2O70dZnYb6TVsS6AGRPxsYdw@mail.gmail.com