Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 21 Sep 2023 15:22:39 +0200
From:   Maxime Ripard <mripard@kernel.org>
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        dri-devel@lists.freedesktop.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct
 drm_*_helper_funcs callbacks
Message-ID: <tqllt5ddlmho5corvdpnqab2ghz2344tbdtczett263y4uajlo@agdoqol4esir>
References: <20230914195138.1518065-1-javierm@redhat.com>
 <f5620d32-2705-498b-a65c-7dc663340a6d@suse.de>
 <87wmwo3q50.fsf@minerva.mail-host-address-is-not-set>
 <552hpgr7qzbjxuyei3n5m7rsn7ekwbdgzv25oe5vy6qb35gf23@q4etussk5jwl>
 <d1f26dbd-5f32-4eb2-af16-f15fe95cc2ec@suse.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="4sox4ca5522j7baz"
Content-Disposition: inline
In-Reply-To: <d1f26dbd-5f32-4eb2-af16-f15fe95cc2ec@suse.de>
Precedence: bulk


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

Hi,

On Thu, Sep 21, 2023 at 10:52:14AM +0200, Thomas Zimmermann wrote:
> Am 21.09.23 um 09:44 schrieb Maxime Ripard:
> > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrot=
e:
> > > Thomas Zimmermann <tzimmermann@suse.de> writes:
> > >=20
> > > > Hi
> > > >=20
> > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > > > The driver uses a naming convention where functions for struct dr=
m_*_funcs
> > > > > callbacks are named ssd130x_$object_$operation, while the callbac=
ks for
> > > > > struct drm_*_helper_funcs are named ssd130x_$object_helper_$opera=
tion.
> > > > >=20
> > > > > The idea is that this helper_ prefix in the function names denote=
 that are
> > > > > for struct drm_*_helper_funcs callbacks. This convention was copi=
ed from
> > > > > other drivers, when ssd130x was written but Maxime pointed out th=
at is the
> > > > > exception rather than the norm.
> > > >=20
> > > > I guess you found this in my code. I want to point out that I use t=
he
> > > > _helper infix to signal that these are callback for
> > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. T=
he
> > > > naming is intentional.
> > > >=20
> > >=20
> > > Yes, that's what tried to say in the commit message and indeed I got =
the
> > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe th=
ese
> > > function names are since first iteration of the driver, when was mean=
t to
> > > be a tiny driver.
> > >=20
> > > According to Maxime it's the exception rather than the rule and sugge=
sted
> > > to change it, I don't really have a strong opinion on either naming T=
BH.
> >=20
> > Maybe that's just me, but the helper in the name indeed throws me off. =
In my
> > mind, it's supposed to be used only for helpers, not functions implemen=
ting the
> > helpers hooks.
>=20
> Tying the function name to its _funcs structure makes perfect sense to me,
> as it helps to structure the driver code. So I always use the _helper_
> infix.
>=20
> In contrast, the DRM helpers that implement certain functionality does not
> seem to follow any naming scheme. For example drm_atomic_helper_check()
> implements struct drm_mode_config_funcs.atomic_check. To me, it's not
> obvious that these two belong together.

Right, but then we end up with functions that have helpers in their name
and are indeed helpers, and then we have functions that have helpers in
their name but are not meant to help anyone or be reused anywhere else.

> And in the same structure, there's fb_create, which is provided by
> drm_gem_fb_create_with_dirty(). This one doesn't even mention that
> it's a helper.

We should fix that then I guess?

Anyway, we're way past bikeshedding there :)

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZQxDnwAKCRDj7w1vZxhR
xfIlAPoCfb2reRTsVtu9qhrHlPz2UwCkF0z2C+wLodtroBc6IwD/WKlUWcJnrYCe
LHprscyABhnpC6rvcpeGF6+mLyQopg0=
=WXlm
-----END PGP SIGNATURE-----

--4sox4ca5522j7baz--