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: <20220628085949.2147920-1-rexnie3@gmail.com>
In-Reply-To: <20220628085949.2147920-1-rexnie3@gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 28 Jun 2022 07:52:31 -0700
Message-ID: <CAD=FV=U8oR-L7XRJH7qVG2p2m1J8obnQSTzA2moZZ8Kmy2SzFw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] drm/panel-edp: Add eDP innolux panel support
To:     Rex Nie <rexnie3@gmail.com>
Cc:     Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Sandeep Panda <spanda@codeaurora.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Hsin-Yi Wang <hsinyi@chromium.org>,
        Rob Herring <robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Tue, Jun 28, 2022 at 2:00 AM Rex Nie <rexnie3@gmail.com> wrote:
>
> Add support for the 14" innolux,n140hca-eac eDP panel.
>
> Signed-off-by: Rex Nie <rexnie3@gmail.com>
> Acked-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 3626469c4cc2..2a8fcdffe80c 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1355,6 +1355,29 @@ static const struct panel_desc innolux_n125hce_gn1 = {
>         },
>  };
>
> +static const struct display_timing innolux_n140hca_eac_timing = {
> +       .pixelclock = { 72600000, 76420000, 80240000 },
> +       .hactive = { 1920, 1920, 1920 },
> +       .hfront_porch = { 80, 80, 80 },
> +       .hback_porch = { 190, 190, 190 },
> +       .hsync_len = { 60, 60, 60 },
> +       .vactive = { 1080, 1080, 1080 },
> +       .vfront_porch = { 6, 6, 6 },
> +       .vback_porch = { 38, 38, 38 },
> +       .vsync_len = { 8, 8, 8 },
> +       .flags = DISPLAY_FLAGS_VSYNC_LOW | DISPLAY_FLAGS_HSYNC_LOW,
> +};

A few questions:

1. If I'm doing my math right, you're saying that this panel runs at
30 Hz refresh rate. Truly? While I won't dismiss that as impossible,
it feels unlikely. Specifically:

In [2]: 72600000 / ((1920 + 80 + 190 + 60) * (1080 + 6 + 38 + 8))
Out[2]: 28.50412249705536

In [3]: 80240000 / ((1920 + 80 + 190 + 60) * (1080 + 6 + 38 + 8))
Out[3]: 31.503729878288183

NOTE: I managed to dig up a datasheet for this panel and the datasheet
I have shows it as a 60 Hz refresh rate panel.


2. You're using the "struct display_timing" here instead of the
"struct drm_display_mode". That can be OK, but can I ask why exactly?


3. Are you sure you need to add this entry? Moving forward I'm trying
to encourage people to use the generic "edp-panel". Mostly you'd want
to add a hardcoded panel here if:

a) Devices have already shipped using hardcoded timings and we don't
want to risk breaking something in the field with "edp-panel".

b) You're trying to support some eDP controller that can't handle the
generic "edp-panel". In this case I'm OK with landing changes but I'd
strongly encourage you to update the controller to handle things.


> +static const struct panel_desc innolux_n140hca_eac = {
> +       .timings = &innolux_n140hca_eac_timing,
> +       .num_timings = 1,
> +       .bpc = 6,

Is it really 6 bpc? The datasheet I dug up claims 16777216 colors
which would be 8 bpc. The EDID from that same datasheet also claims 8
bpc.


> +       .size = {
> +               .width = 309,
> +               .height = 174,
> +       },

Where are your delays? I know in old code these were hard to figure
out from the panel spec, but the kernel doc comments now translate it
into standard eDP terminology so this should be trivially easy for you
to provide.