linux-kernel - Re: [PATCH] drm/panel: himax-hx83112a: 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=XsHuhprN-ijw8-ZmOKV6CnCrR=s4X4L57p9J8e02MTBg@mail.gmail.com>
Date: Tue, 10 Sep 2024 14:19:53 -0700
From: Doug Anderson <dianders@...omium.org>
To: Tejas Vipin <tejasvipin76@...il.com>, neil.armstrong@...aro.org, 
	Abhishek Tamboli <abhishektamboli9@...il.com>
Cc: Jessica Zhang <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, 
	Javier Martinez Canillas <javierm@...hat.com>
Subject: Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions

Hi,

On Sat, Sep 7, 2024 at 1:32 AM Tejas Vipin <tejasvipin76@...il.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@...il.com>
> >>
> >> Reviewed-by: Jessica Zhang <quic_jesszhan@...cinc.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?

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