From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Zimmermann Subject: Re: [PATCH v4 14/23] drm/tilcdc: Provide ddc symlink in connector sysfs directory Date: Wed, 24 Jul 2019 10:01:05 +0200 Message-ID: References: <20190723090532.GA787@ravnborg.org> <3ad60be5-49cf-4017-4b74-53a2d6272deb@collabora.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3hEKoYM3ihXgipFwLohglgI6I6K0PHVBV" Return-path: In-Reply-To: <3ad60be5-49cf-4017-4b74-53a2d6272deb@collabora.com> Sender: linux-kernel-owner@vger.kernel.org To: Andrzej Pietrasiewicz , Sam Ravnborg Cc: Neil Armstrong , Maxime Ripard , dri-devel@lists.freedesktop.org, Douglas Anderson , linux-tegra@vger.kernel.org, Thierry Reding , Laurent Pinchart , kernel@collabora.com, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Vincent Abriou , Krzysztof Kozlowski , Jonathan Hunter , David Airlie , Chen-Yu Tsai , Kukjin Kim , NXP Linux Team , Dave Airlie , freedreno@lists.freedesktop.org, Pengutronix Kernel Team , Jonas List-Id: dri-devel@lists.freedesktop.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3hEKoYM3ihXgipFwLohglgI6I6K0PHVBV Content-Type: multipart/mixed; boundary="KIKXUXCjxjc3xDRil2vLGI4WFDOmJ9a2q"; protected-headers="v1" From: Thomas Zimmermann To: Andrzej Pietrasiewicz , Sam Ravnborg Cc: Neil Armstrong , Maxime Ripard , dri-devel@lists.freedesktop.org, Douglas Anderson , linux-tegra@vger.kernel.org, Thierry Reding , Laurent Pinchart , kernel@collabora.com, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Vincent Abriou , Krzysztof Kozlowski , Jonathan Hunter , David Airlie , Chen-Yu Tsai , Kukjin Kim , NXP Linux Team , Dave Airlie , freedreno@lists.freedesktop.org, Pengutronix Kernel Team , Jonas Karlman , linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org, Jyri Sarha , Alexios Zavras , Mamta Shukla , linux-mediatek@lists.infradead.org, Rodrigo Vivi , Matthias Brugger , Thomas Gleixner , Sean Paul , linux-arm-kernel@lists.infradead.org, Jernej Skrabec , amd-gfx@lists.freedesktop.org, Tomi Valkeinen , Greg Kroah-Hartman , Seung-Woo Kim , linux-kernel@vger.kernel.org, Todor Tomov , Kyungmin Park , Huang Rui , Alex Deucher , Shawn Guo , =?UTF-8?Q?Christian_K=c3=b6nig?= , Gerd Hoffmann Message-ID: Subject: Re: [PATCH v4 14/23] drm/tilcdc: Provide ddc symlink in connector sysfs directory References: <20190723090532.GA787@ravnborg.org> <3ad60be5-49cf-4017-4b74-53a2d6272deb@collabora.com> In-Reply-To: <3ad60be5-49cf-4017-4b74-53a2d6272deb@collabora.com> --KIKXUXCjxjc3xDRil2vLGI4WFDOmJ9a2q Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Am 23.07.19 um 14:44 schrieb Andrzej Pietrasiewicz: > Hi Sam, >=20 > W dniu 23.07.2019 o=C2=A011:05, Sam Ravnborg pisze: >> Hi Andrzej >> >> On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote:= >>> Use the ddc pointer provided by the generic connector. >>> >>> Signed-off-by: Andrzej Pietrasiewicz >>> --- >>> =C2=A0 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + >>> =C2=A0 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c >>> b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c >>> index 62d014c20988..c373edb95666 100644 >>> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c >>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c >>> @@ -219,6 +219,7 @@ static struct drm_connector >>> *tfp410_connector_create(struct drm_device *dev, >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tfp410_connector->mod =3D mod; >>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 connector =3D &tfp410_connector= ->base; >>> +=C2=A0=C2=A0=C2=A0 connector->ddc =3D mod->i2c; >>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 drm_connector_init(dev, connect= or, &tfp410_connector_funcs, >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 DRM_MODE_CONNECTOR_DVID); >> >> When reading this code, it looks strange that we set connector->ddc >> *before* the call to init the connector. >> One could risk that drm_connector_init() used memset(..) to clear all >> fields or so, and it would break this order. >=20 > I verified the code of drm_connector_init() and cannot find any memset(= ) > invocations there. What is your actual concern? I think this echoes my concern about the implicit order of operation. It seems too easy to get this wrong. If you don't want to add an additional interface for setting the ddc field, why not add a dedicated initializer function that sets the ddc field? Something like this. int drm_connector_init_with_ddc(connector, funcs, ..., ddc) { ret =3D drm_connector_init(connector, funcs, ...); if (ret) return ret; if (!ddc) return 0; connector->ddc =3D ddc; /* set up sysfs */ return 0; } Best regards Thomas > Andrzej > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel --=20 Thomas Zimmermann Graphics Driver Developer SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany GF: Felix Imend=C3=B6rffer, Mary Higgins, Sri Rasiah HRB 21284 (AG N=C3=BCrnberg) --KIKXUXCjxjc3xDRil2vLGI4WFDOmJ9a2q-- --3hEKoYM3ihXgipFwLohglgI6I6K0PHVBV Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEchf7rIzpz2NEoWjlaA3BHVMLeiMFAl04EEEACgkQaA3BHVML eiNRWAf8CYxCmH/26EWFNpq9GZQjDMvAU5wdcW44Lnp0dMtgf/nqPvbEtkPYWt1D lMAlcSy9rrFFtW3E2HFwK5V9QbW9LvxdRaA7gK0ypMrYgmO5QECHQGMCaRxb/DpK 02ZW59khdYpqNbLfjZ3toTs6BiHuBS2OF5tHq4SbvHwdva4pzOQtwQ90TVrNcOp+ 8kSGLB/0+CcOvKhgjd1uyw1w5lE/cILPT4sfXYlutlSRsHTCA7FwXHwhoqMyfYK4 xlDAr72YN6/Lt9gLuKqWSFX9O9vvSpMMjp6nnzFwCguc+Nz53L4ra49cJ8qPP6dY wBV/ccooRzGp27r8ENmXR8wKLfTP8g== =ol/N -----END PGP SIGNATURE----- --3hEKoYM3ihXgipFwLohglgI6I6K0PHVBV--