On Thu, Jul 16, 2026 at 04:31:53PM -0700, Doug Anderson wrote: Hello Doug, thanks a lot for the review!
> Hi, > > On Thu, Jul 16, 2026 at 3:09 PM Michail Tatas <[email protected]> wrote: > > > > @@ -159,36 +159,23 @@ static int ili9805_power_off(struct ili9805 *ctx) > > > > static int ili9805_activate(struct ili9805 *ctx) > > { > > - struct mipi_dsi_device *dsi = ctx->dsi; > > - struct device *dev = &dsi->dev; > > - int i, ret; > > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > > + int i; > > > > for (i = 0; i < ctx->desc->init_length; i++) { > > const struct ili9805_instr *instr = &ctx->desc->init[i]; > > > > - ret = mipi_dsi_dcs_write_buffer(ctx->dsi, instr->data, > > instr->len); > > - if (ret < 0) > > - return ret; > > + mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, > > instr->len); > > > > if (instr->delay > 0) > > - msleep(instr->delay); > > - } > > What you've done is an improvement, but it's not all the way there. > Specifically, we'd really want to get rid of the whole "struct > ili9805_instr" type and instead each panel should have an init > function. The "struct ili9805_desc" should have a pointer to the init > function instead of a pointer to the init data. I am more than happy to do these changes as well. My only concern is that I do not have hardware to test the panel. Are compiling and static analysis sufficient tests for these changes ? > For details, you can see a preivous email about this [1], which then > has further links if you want to dig into details. You can see that > Chintan eventually implemented this in commit a89d9a327d06 > ("drm/panel: novatek-nt36672a: Inline panel init sequences"). > > If landing this patch without cleaning up the init sequences is really > important to you, I could be convinced. However, the init sequences > aren't all that big and it would be nice if you could clean it up all > at once. It could be one patch or two. > It is not all that important, I can clean up the init sequences and send it as part of the v2 > [1] > http://lore.kernel.org/r/CAD=FV=UCyfjiqcpYCM5ePz-auX4g=i_+i78ivvvya8r1xta...@mail.gmail.com > > > > @@ -211,25 +198,13 @@ static int ili9805_prepare(struct drm_panel *panel) > > > > static int ili9805_deactivate(struct ili9805 *ctx) > > { > > - struct mipi_dsi_device *dsi = ctx->dsi; > > - struct device *dev = &dsi->dev; > > - int ret; > > - > > - ret = mipi_dsi_dcs_set_display_off(ctx->dsi); > > - if (ret < 0) { > > - dev_err(dev, "Failed to set display OFF (%d)\n", ret); > > - return ret; > > - } > > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > > > > - usleep_range(5000, 10000); > > - > > - ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); > > - if (ret < 0) { > > - dev_err(dev, "Failed to enter sleep mode (%d)\n", ret); > > - return ret; > > - } > > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); > > + mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000); > > + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); > > > > - return 0; > > + return dsi_ctx.accum_err; > > Nobody looks at the return code of this function. Since you're > touching it anyway, can you change ili9805_deactivate() to return > "void"? I wouldn't object if you made ili9805_power_off() return > "void" in the same patch too, even though it's a bit unrelated to the > rest of the patch. > Good point regarding the return types, I will change them to return void. I will send a v2 with the requested changes. Regards, Michail
