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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/09/2024 23:19, Doug Anderson wrote:
Hi,

On Sat, Sep 7, 2024 at 1:32 AM Tejas Vipin <tejasvipin76@xxxxxxxxx> 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@xxxxxxxxx>

Reviewed-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>

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@xxxxxxxxx/
[3] https://lore.kernel.org/all/CAD=FV=XRZKL_ppjUKDK61fQkWhHiQCJLfmVBS7wSo4sUux2g8Q@xxxxxxxxxxxxxx/

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux