Re: [PATCH v1 09/35] drm/modes: Move named modes parsing to a separate function
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2022-08-12 13:27:37
Also in:
dri-devel, linux-amlogic, linux-sunxi, lkml
Hi Maxime, On Fri, Jul 29, 2022 at 6:36 PM Maxime Ripard [off-list ref] wrote:
The current construction of the named mode parsing doesn't allow to extend it easily. Let's move it to a separate function so we can add more parameters and modes. Signed-off-by: Maxime Ripard <redacted>
Thanks for your patch, which looks similar to my "[PATCH v2 2/5] drm/modes: Extract drm_mode_parse_cmdline_named_mode()" (https://lore.kernel.org/dri-devel/1371554419ae63cb54c2a377db0c1016fcf200bb.1657788997.git.geert@linux-m68k.org (local) ;-)
quoted hunk ↗ jump to hunk
--- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c@@ -1773,6 +1773,28 @@ static const char * const drm_named_modes_whitelist[] = { "PAL", }; +static bool drm_mode_parse_cmdline_named_mode(const char *name, + unsigned int name_end, + struct drm_cmdline_mode *cmdline_mode) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { + int ret; + + ret = str_has_prefix(name, drm_named_modes_whitelist[i]); + if (ret != name_end) + continue; + + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]); + cmdline_mode->specified = true; + + return true; + } + + return false;
What's the point in returning a value, if it is never checked? Just make this function return void?
quoted hunk ↗ jump to hunk
+} + /** * drm_mode_parse_command_line_for_connector - parse command line modeline for connector * @mode_option: optional per connector mode option@@ -1848,18 +1870,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, parse_extras = true; } - /* First check for a named mode */ - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { - ret = str_has_prefix(name, drm_named_modes_whitelist[i]); - if (ret == mode_end) { - if (refresh_ptr) - return false; /* named + refresh is invalid */ + /* + * Having a mode that starts by a letter (and thus is named) and + * an at-sign (used to specify a refresh rate) is disallowed. + */ + if (!isdigit(name[0]) && refresh_ptr)
This condition may have to be relaxed, if we want to support e.g. "hd720p@50", cfr. my comments on "[PATCH v1 05/35] drm/connector: Add TV standard property".
+ return false; - strcpy(mode->name, drm_named_modes_whitelist[i]); - mode->specified = true; - break; - } - } + drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
This call needs to be conditional on mode_end being non-zero, cfr. my patch "[PATCH v2 1/5] drm/modes: parse_cmdline: Handle empty mode name part" (https://lore.kernel.org/dri-devel/302d0737539daa2053134e8f24fdf37e3d939e1e.1657788997.git.geert@linux-m68k.org (local)).
/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
if (!mode->specified && isdigit(name[0])) {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel