[PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions

[neil.armstrong at linaro.org]

On 10/09/2024 23:19, Doug Anderson wrote:
> Hi,
> 
> On Sat, Sep 7, 2024 at 1:32 AM Tejas Vipin <tejasvipin76 at gmail.com> wrote:
>>
>> On 9/7/24 3:53 AM, Jessica Zhang wrote:
>>>
>>>
>>> On 9/6/2024 3:14 PM, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 9/4/2024 7:15 AM, Tejas Vipin wrote:
>>>>> Changes the himax-hx83112a panel to use multi style functions for
>>>>> improved error handling.
>>>>>
>>>>> Signed-off-by: Tejas Vipin <tejasvipin76 at gmail.com>
>>>>
>>>> Reviewed-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>
>>> Hi Tejas,
>>>
>>> Just a heads up, it seems that this might be a duplicate of this change [1]?
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
>>
>> Ah, thanks for letting me know. I hadn't realized someone else had
>> started working on this too.
>>
>> However, I would argue that my patch [2] is a better candidate for merging
>> because of the following reasons:
>>
>> 1) Removes unnecessary error printing:
>> The mipi_dsi_*_multi() functions all have inbuilt error printing which
>> makes printing errors after hx83112a_on unnecessary as is addressed in
>> [2] like so:
>>
>>> -     ret = hx83112a_on(ctx);
>>> +     ret = hx83112a_on(ctx->dsi);
>>>        if (ret < 0) {
>>> -             dev_err(dev, "Failed to initialize panel: %d\n", ret);
>>>                gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>>                regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> -             return ret;
>>>        }
>>
>> [2] also removes the unnecessary dev_err after regulator_bulk_enable as was
>> addressed in [3] like so:
>>
>>>        ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> -     if (ret < 0) {
>>> -             dev_err(dev, "Failed to enable regulators: %d\n", ret);
>>> +     if (ret < 0)
>>>                return ret;
>>> -     }
>>
>> 2) Better formatting
>>
>> The mipi_dsi_dcs_write_seq_multi statements in [1] aren't formatted
>> quite right according to what has been done so far. They are written as
>> such in [1]:
>>
>>> +     mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
>>>                               0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4, 0x0e);
>>
>> Where they should be written as such in [2]:
>>
>>> +     mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
>>> +                                  0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4, 0x0e);
>>
>> All in all, the module generated using my patch ends up being a teensy
>> bit smaller (1% smaller). But if chronology is what is important, then
>> it would at least be nice to see the above changes as part of Abhishek's
>> patch too. And I'll be sure to look at the mail in the drm inbox now
>> onwards :p
>>
>> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
>> [2] https://lore.kernel.org/all/20240904141521.554451-1-tejasvipin76@gmail.com/
>> [3] https://lore.kernel.org/all/CAD=FV=XRZKL_ppjUKDK61fQkWhHiQCJLfmVBS7wSo4sUux2g8Q@mail.gmail.com/
> 
> I would tend to agree that Tejas's patch looks slightly better, but
> Abhishek's patch appears to have been posted first.
> 
> Neil: any idea what to do here? Maybe a Co-Developed-by or something?
> ...or we could land Abhishek and Tejas could post a followup for the
> extra cleanup?

Yeah usually I take the first one when they are equal, but indeed Tejas
cleanup up a little further and better aligned the parameters so I think
Tejas's one is a better looking version.

In this case we should apply Teja's one, nothing personal Abhishek!

> 
> Abhishek: are you planning to post more _multi cleanups? If so, please
> make sure to CC Tejas (who has been posting a bunch of them) and
> perhaps me since I've been helping to review them a bit.
> 
> -Doug