Received-SPF: pass (google.com: domain of linux-kernel+bounces-85918-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240228011133.1238439-1-hsinyi@chromium.org> <20240228011133.1238439-3-hsinyi@chromium.org>
In-Reply-To: <20240228011133.1238439-3-hsinyi@chromium.org>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 28 Feb 2024 16:21:37 -0800
Message-ID: <CAD=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name
To: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Tue, Feb 27, 2024 at 5:11=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org> =
wrote:
>
> @@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] =
=3D {
>         { /* sentinal */ }
>  };
>
> -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> +static bool edid_has_name(struct edid *edid, const char *panel_name)
> +{
> +       int i, j;
> +       char buf[13];
> +

Should have some comment about why this can't use
drm_edid_get_monitor_name(). AKA because panels seem to be storing the
monitor name tagged as raw strings instead of as the name. Should
probably also have some of the other checks from
is_display_descriptor() like checking for clock of 0 and pad1 of 0.


> +       for (i =3D 0; i < 4; i++) {

Instead of 4, I think this can be ARRAY_SIZE(edid->detailed_timings), right=
?


> +               strncpy(buf, edid->detailed_timings[i].data.other_data.da=
ta.str.str,
> +                       sizeof(buf));

I can never quite remember which of the strXcpy() routines are frowned
upon and which are the golden children at the moment. ...but I don't
think we really need the copy nor the local buffer anyway, right?
You're already going through this thing 1 byte at a time so just
compare it straight away.


> +               for (j =3D 0; j < 13; j++) {
> +                       if (buf[j] =3D=3D 0x0a) {

Instead of hardcoding 0x0a, I think you want '\n', no?


> +                               buf[j] =3D '\0';
> +                               break;
> +                       }
> +               }
> +               buf[12] =3D '\0';
> +               if (strncmp(panel_name, buf, strlen(panel_name)) =3D=3D 0=
)
> +                       return true;

Untested, but I think with my suggestions above the function becomes this:

static bool edid_has_name(struct edid *edid, const char *panel_name)
{
    int i, j;

    /*
     * We can't use drm_edid_get_monitor_name() since many eDP panels store
     * their name as a raw string. We'll accept either a string or name
     * match as long as the panel ID also matches.
     */
    for (i =3D 0; i < ARRAY_SIZE(edid->detailed_timings); i++) {
        struct detailed_timing *timing =3D &edid->detailed_timings[i];

        if (timing->pixel_clock !=3D 0 ||
            timing->data.other_data.pad1 !=3D 0 ||
            (timing->data.other_data.type !=3D EDID_DETAIL_MONITOR_NAME &&
             timing->data.other_data.type !=3D EDID_DETAIL_MONITOR_STRING))
                continue;

        for (j =3D 0; j < ARRAY_SIZE(timing->data.other_data.data.str.str);=
 j++) {
            const char *str =3D timing->data.other_data.data.str.str;

            if (panel_name[j] =3D=3D '\0') {
                if (str[j] =3D=3D '\0' || str[j] =3D=3D '\n')
                    return true;
                break;
            }
        }
        if (j =3D=3D ARRAY_SIZE(timing->data.other_data.data.str.str) &&
            panel_name[j] =3D=3D '\0')
            return true;
    }

    return false;
}