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;
Feedback-ID: i8771445c:Fastmail
Date:   Tue, 16 Aug 2022 15:46:24 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Jernej Skrabec <jernej.skrabec@gmail.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Samuel Holland <samuel@sholland.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Daniel Vetter <daniel@ffwll.ch>, Emma Anholt <emma@anholt.net>,
        David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Noralf =?utf-8?Q?Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        linux-sunxi@lists.linux.dev,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Phil Elwell <phil@raspberrypi.com>,
        Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@lists.infradead.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Dom Cobley <dom@raspberrypi.com>
Subject: Re: [PATCH v1 09/35] drm/modes: Move named modes parsing to a
 separate function
Message-ID: <20220816134624.cmb5s6i6pkdhu3qj@houat>
References: <20220728-rpi-analog-tv-properties-v1-0-3d53ae722097@cerno.tech>
 <20220728-rpi-analog-tv-properties-v1-9-3d53ae722097@cerno.tech>
 <CAMuHMdUiMEybnhgxgBXh1Cbv6syVe9iVU=sb17zHM72R8A2Dew@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="xbotrfh2f6jawwvu"
Content-Disposition: inline
In-Reply-To: <CAMuHMdUiMEybnhgxgBXh1Cbv6syVe9iVU=sb17zHM72R8A2Dew@mail.gmail.com>
Precedence: bulk


--xbotrfh2f6jawwvu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Aug 12, 2022 at 03:27:17PM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
>=20
> On Fri, Jul 29, 2022 at 6:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > The current construction of the named mode parsing doesn't allow to ext=
end
> > it easily. Let's move it to a separate function so we can add more
> > parameters and modes.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>=20
> 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/1371554419ae63cb54c2a377db0c1016fcf200=
bb.1657788997.git.geert@linux-m68k.org
> ;-)

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.

>=20
> > --- 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_whitel=
ist[] =3D {
> >         "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 =3D 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > +               int ret;
> > +
> > +               ret =3D str_has_prefix(name, drm_named_modes_whitelist[=
i]);
> > +               if (ret !=3D name_end)
> > +                       continue;
> > +
> > +               strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]=
);
> > +               cmdline_mode->specified =3D true;
> > +
> > +               return true;
> > +       }
> > +
> > +       return false;
>=20
> 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?

Maxime

--xbotrfh2f6jawwvu
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYvufsAAKCRDj7w1vZxhR
xRvYAP9DR7LzVwd0ITVjjsOrO+BQ9andM/0A4P6qCsx/ywFipQEA+bV9F+NyoFUN
oNXdDnnG7g+hOqMsx28nmzB45wcO6w0=
=uJzR
-----END PGP SIGNATURE-----

--xbotrfh2f6jawwvu--