Hi Maxime, On Tue, Aug 16, 2022 at 3:46 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote: > On Fri, Aug 12, 2022 at 03:27:17PM +0200, Geert Uytterhoeven wrote: > > On Fri, Jul 29, 2022 at 6:36 PM Maxime Ripard <maxime@xxxxxxxxxx> 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 <maxime@xxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx > > ;-) > > Indeed, I forgot about that one, sorry :/ > > I think I'd still prefer to have the check for refresh + named mode > outside of the function, since I see them as an "integration" issue, not > a parsing one. > > It's not the named mode parsing that fails, but the fact that we both > have a valid refresh and a valid named mode. > > > > > > --- 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? > > Yeah, it's something I went back and forth to between the dev, and it's > an artifact. > > I'll drop that patch, take your version and move the refresh check to > drm_mode_parse_command_line_for_connector if that's alright for you? Fine for me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx 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
