Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver

[cong yang]

Hi,
 Thanks reply.

Doug Anderson <dianders@xxxxxxxxxxxx> 于2024年4月24日周三 00:26写道：
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:37 AM cong yang
> <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> 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