Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230607151127.1542024-1-pavacic.p@gmail.com> <20230607151127.1542024-4-pavacic.p@gmail.com>
In-Reply-To: <20230607151127.1542024-4-pavacic.p@gmail.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Thu, 15 Jun 2023 21:54:48 +0200
Message-ID: <CACRpkdbrEA54qmfTKSsFRG9ZS4u8hM6P5TXtOjRAiW+TD_v-fQ@mail.gmail.com>
Subject: Re: [PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel
To:     Paulo Pavacic <pavacic.p@gmail.com>
Cc:     neil.armstrong@linaro.org, sam@ravnborg.org, airlied@gmail.com,
        daniel@ffwll.ch, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=E2=80=AFPM Paulo Pavacic <pavacic.p@gmail.com> =
wrote:

> +static int fannal_panel_enable(struct drm_panel *panel)
> +{
> +       struct mipi_dsi_device *dsi =3D to_mipi_dsi_device(panel->dev);
> +
> +       mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x1=
3);
> +       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 =3D 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 =3D mipi_dsi_dcs_set_display_on(dsi);
    if (ret)
            return ret;

Yours,
Linus Walleij