linux-kernel - Re: [PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Jun 2023 11:57:29 +0200
From:   Paulo Pavacic <pavacic.p@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     neil.armstrong@...aro.org, sam@...nborg.org, airlied@...il.com,
        daniel@...ll.ch, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel

Hello Linus,

thank you for the comments.

čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@...aro.org> napisao je:
>
> Hi Paulo,
>
> thanks for your patch!
>
> Overall this looks very good.
>
> I doubt that the display controller is actually by Fannal, but I guess
> you tried to find out? We usually try to identify the underlying display
> controller so the driver can be named after it and reused for more
> display panels.

Yes, of course, the controller is ST7701S.

>
> Some minor questions/nitpicks below.
>
> On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <pavacic.p@...il.com> wrote:
>
> > +static int fannal_panel_enable(struct drm_panel *panel)
> > +{
> > +       struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
> > +
> > +       mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
> > +       mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08);
>
> Why is everything using mipi_dsi_generic_write_seq() instead of
> mipi_dsi_dcs_write_seq()?

Okay, I will replace it.

>
> Especially here, because 0x11 is certainly a command:
>
> > +       mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE
>
> Instead use:
>
>     ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>     if (ret)
>             return ret;
>
>
> > +       mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON
>
> Instead use:
>
>     ret = mipi_dsi_dcs_set_display_on(dsi);
>     if (ret)
>             return ret;
>

Okay I will replace all commands with a functions found here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_mipi_dsi.c#L995

> Yours,
> Linus Walleij

Best regards,
Paulo