On 1/7/25 5:37 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Dec 30, 2024 at 2:10 AM <neil.armstrong@xxxxxxxxxx> wrote:
>>
>>> static int xpp055c272_unprepare(struct drm_panel *panel)
>>> {
>>> struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
>>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> - int ret;
>>> -
>>> - ret = mipi_dsi_dcs_set_display_off(dsi);
>>> - if (ret < 0)
>>> - dev_err(ctx->dev, "failed to set display off: %d\n", ret);
>>> -
>>> - mipi_dsi_dcs_enter_sleep_mode(dsi);
>>> - if (ret < 0) {
>>> - dev_err(ctx->dev, "failed to enter sleep mode: %d\n", ret);
>>> - return ret;
>>> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>>> +
>>> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>>> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>>> + if (dsi_ctx.accum_err) {
>>> + dev_err(ctx->dev, "failed to enter sleep mode: %d\n",
>>> + dsi_ctx.accum_err);
>
> You should delete the above error message, right?
> mipi_dsi_dcs_enter_sleep_mode_multi() reports the error for you, I
> think.
>
>
>>> @@ -155,17 +147,19 @@ static int xpp055c272_prepare(struct drm_panel *panel)
>>> {
>>> struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
>>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> - int ret;
>>> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>>>
>>> dev_dbg(ctx->dev, "Resetting the panel\n");
>>> - ret = regulator_enable(ctx->vci);
>>> - if (ret < 0) {
>>> - dev_err(ctx->dev, "Failed to enable vci supply: %d\n", ret);
>>> - return ret;
>>> + dsi_ctx.accum_err = regulator_enable(ctx->vci);
>>> + if (dsi_ctx.accum_err) {
>>
>> I would rather keep ret instead of abusing dsi_ctx.accum_err, but it's already like
>> that in other converted driver so I won't oppose it...
>
> FWIW, we had this discussion before. I agree with what Tejas did here
> and I managed to convince Dmitry Baryshkov in the past. See:
>
> https://lore.kernel.org/all/CAA8EJpr_HYkXnP3XR9LpDhi1xkQfE_CKJzfzGrO5qd_pQYtiOw@xxxxxxxxxxxxxx/
>
> Looking specifically at this driver, using "ret" would have added
> complexity when we wanted to do "goto disable_vci" because in some
> cases the error code would be in "ret" and sometimes in "accum_err"...
>
>
>>> @@ -175,30 +169,19 @@ static int xpp055c272_prepare(struct drm_panel *panel)
>>> gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>>>
>>> /* T8: 20ms */
>>> - msleep(20);
>>> + mipi_dsi_msleep(&dsi_ctx, 20);
>
> Personally, I would have left the above msleep() alone. There can be
> no errors at this point in the code, right?
>
>
>>> - ret = xpp055c272_init_sequence(ctx);
>>> - if (ret < 0) {
>>> - dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
>>> - goto disable_iovcc;
>>> - }
>>> -
>>> - ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>>> - if (ret < 0) {
>>> - dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
>>> - goto disable_iovcc;
>>> - }
>>> + xpp055c272_init_sequence(&dsi_ctx);
>>> + dev_dbg(ctx->dev, "Panel init sequence done\n");
>
> Should the above print be only if "accum_err" is 0? That would match
> the previous behavior. I guess I would have also left the print as
> part of xpp055c272_init_sequence() unless there's a reason for moving
> it...
I don't think it should print only if accum_err is 0. In the previous
code, it would just print after all the msleeps and write_seqs are done,
with no error checking at any point.
The reason I've moved the print outside the function is because we are
able to reduce a couple lines of code by passing dsi_ctx to the function
instead of ctx. If I'd kept the print inside, it would require us to
declare a `struct device*` variable which would require ctx as far as
I've seen and just overall introduces some lines that we could otherwise
avoid. I've done this in a couple other panels too.
I'll do a v2 with the other suggested changes.
--
Tejas Vipin
