linux-kernel - Re: [PATCH v1 4/4] drm/panel: ili9806e: Break some CMDS into helper functions

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=WAosZPSKdpwR6pjOmiy4hih=jXaMg2guuVgmc+qj-Csw@mail.gmail.com>
Date: Wed, 10 Jul 2024 12:12:57 -0700
From: Doug Anderson <dianders@...omium.org>
To: Michael Walle <mwalle@...nel.org>
Cc: Cong Yang <yangcong5@...qin.corp-partner.google.com>, quic_jesszhan@...cinc.com, 
	neil.armstrong@...aro.org, linus.walleij@...aro.org, airlied@...il.com, 
	dmitry.baryshkov@...aro.org, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 4/4] drm/panel: ili9806e: Break some CMDS into helper functions

Hi,

On Wed, Jul 10, 2024 at 2:02 AM Michael Walle <mwalle@...nel.org> wrote:
>
> On Wed Jul 10, 2024 at 10:47 AM CEST, Cong Yang wrote:
> > Break select page cmds into helper function.
>
> Why though? I don't find that anything easier to read. In fact, I
> deliberately chose not to factor that out into a function. It's just
> a sequence of magic commands, taken straight from the datasheet. So,
> I'd like to keep it that way.

The consensus of previous discussion on the lists was that folks
agreed that we should, where possible, make it more obvious what these
magic sequences of commands were doing. IMO separating out the page
switch command helps. Certainly I'm always happy to hear other
opinions, though.


> -michael
>
> > Signed-off-by: Cong Yang <yangcong5@...qin.corp-partner.google.com>
> > ---
> >  drivers/gpu/drm/panel/panel-ilitek-ili9806e.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
> > index e4a44cd26c4d..68fb9a1a4d80 100644
> > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9806e.c
> > @@ -35,6 +35,12 @@ struct ili9806e_panel {
> >       enum drm_panel_orientation orientation;
> >  };
> >
> > +#define ILI9806E_DCS_SWITCH_PAGE     0xff
> > +
> > +#define ili9806e_switch_page(ctx, page) \
> > +     mipi_dsi_dcs_write_seq_multi(ctx, ILI9806E_DCS_SWITCH_PAGE, \
> > +                                  0xff, 0x98, 0x06, 0x04, (page))
> > +
> >  static const char * const regulator_names[] = {
> >       "vdd",
> >       "vccio",
> > @@ -227,7 +233,7 @@ static void ili9806e_dsi_remove(struct mipi_dsi_device *dsi)
> >  static void com35h3p70ulc_init(struct mipi_dsi_multi_context *ctx)
> >  {
> >       /* Switch to page 1 */
> > -     mipi_dsi_dcs_write_seq_multi(ctx, 0xff, 0xff, 0x98, 0x06, 0x04, 0x01);
> > +     ili9806e_switch_page(ctx, 0x01);

I think with your change you should remove the "Switch to page X"
comments since they're now obvious. Other than that, I'm happy with:

Reviewed-by: Douglas Anderson <dianders@...omium.org>