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:   Thu, 15 Jun 2023 21:54:48 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Paulo Pavacic <pavacic.p@...il.com>
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

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.

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()?

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;

Yours,
Linus Walleij