Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name

[Doug Anderson <dianders@chromium.org>]

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> @@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] = {
>         { /* 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 = 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.data.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 = 0; j < 13; j++) {
> +                       if (buf[j] == 0x0a) {

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


> +                               buf[j] = '\0';
> +                               break;
> +                       }
> +               }
> +               buf[12] = '\0';
> +               if (strncmp(panel_name, buf, strlen(panel_name)) == 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 = 0; i < ARRAY_SIZE(edid->detailed_timings); i++) {
        struct detailed_timing *timing = &edid->detailed_timings[i];

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

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

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

    return false;
}