On Tue, Sep 10, 2024 at 02:19:53PM -0700, 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? > > Abhishek: are you planning to post more _multi cleanups? I’ve just started learning about the DRM subsystem, I don’t have specific plans for _multi cleanups at the moment. However, I would be happy to contribute further. >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. Yes, I’ll make sure to CC Tejas and you. Regards, ABhishek
