linux-kernel - Re: [PATCH] drm/panel: xinpeng-xpp055c272: transition to mipi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <70f0dbf2-dd84-4a50-94cc-1d388c5c93fe@gmail.com>
Date: Tue, 7 Jan 2025 09:51:41 +0530
From: Tejas Vipin <tejasvipin76@...il.com>
To: Doug Anderson <dianders@...omium.org>, neil.armstrong@...aro.org
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
 tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
 quic_jesszhan@...cinc.com, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: xinpeng-xpp055c272: transition to mipi_dsi
 wrapped functions



On 1/7/25 5:37 AM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 30, 2024 at 2:10 AM <neil.armstrong@...aro.org> 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@mail.gmail.com/
> 
> 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