Hi, On Wed, Sep 11, 2024 at 12:41 AM <neil.armstrong@xxxxxxxxxx> wrote: > > 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! Pushed to drm-misc-next: [1/1] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions commit: 32e5666b8a4d0f2aee39a0b2f8386cf9f86a8225
