linux-kernel - Re: [PATCH] drm/panel: raydium-rm692e5: 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] [day] [month] [year] [list]

Date: Tue, 18 Jun 2024 19:28:58 +0530
From: Tejas Vipin <tejasvipin76@...il.com>
To: Doug Anderson <dianders@...omium.org>
Cc: neil.armstrong@...aro.org, quic_jesszhan@...cinc.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 airlied@...il.com, daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: raydium-rm692e5: transition to mipi_dsi
 wrapped functions



On 6/18/24 7:06 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2024 at 5:25 AM Tejas Vipin <tejasvipin76@...il.com> wrote:
>>
>>>>         rm692e5_reset(ctx);
>>>>
>>>> -       ret = rm692e5_on(ctx);
>>>> -       if (ret < 0) {
>>>> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
>>>> +       dsi_ctx.accum_err = rm692e5_on(ctx);
>>>> +       if (dsi_ctx.accum_err) {
>>>> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
>>>
>>> I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
>>> and then you don't need to declare a new one there.
>>>
>>> ...also, you don't need to add an error message since rm692e5_on()
>>> will have already printed one (since the "multi" style functions
>>> always print error messages for you).
>>
>> I'm guessing that the change about regulator_bulk_enable and
>> rm692e5 should also be applied to all the other panels where
>> similar behavior occurs?
> 
> Yeah, I'd say so.
> 
> 
>>>>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>>>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>>> -               return ret;
>>>> +               return dsi_ctx.accum_err;
>>>
>>> Not new for your patch, but it seems odd that we don't do this error
>>> handling (re-assert reset and disable the regulator) for errors later
>>> in the function. Shouldn't it do that? It feels like the error
>>> handling should be in an "err" label and we should end up doing that
>>> any time we return an error code... What do you think?
>>
>> Personally I don't think this is necessary because imo labels
>> only get useful when there's a couple of them and/or they're
>> jumped to multiple times. I don't think either would happen in
>> this particular function. But I guess if you have some convention
>> in mind, then it could be done?
> 
> I think mostly my suggestion was just that we should also do the
> gpiod_set_value_cansleep() and regulator_bulk_disable() at the end of
> rm692e5_prepare() if `dsi_ctx.accum_err` is non-zero. Then you've got
> two places doing the same thing: here and at the end of the function.
> 
> ...oh, but everything below here is already a no-op if the error is
> set. ...so I guess looking at it closer, my suggestion wouldn't be a
> "goto" but would instead be to just move the gpio/regulator call to
> the end. What do you think?

Yeah, sounds good. I'll be doing that.

> 
> -Doug