Date:   Wed, 24 Jul 2019 10:01:05 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        Sam Ravnborg <sam@...nborg.org>
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        dri-devel@...ts.freedesktop.org,
        Douglas Anderson <dianders@...omium.org>,
        linux-tegra@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        kernel@...labora.com, linux-samsung-soc@...r.kernel.org,
        linux-rockchip@...ts.infradead.org,
        Vincent Abriou <vincent.abriou@...com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        David Airlie <airlied@...ux.ie>, Chen-Yu Tsai <wens@...e.org>,
        Kukjin Kim <kgene@...nel.org>,
        NXP Linux Team <linux-imx@....com>,
        Dave Airlie <airlied@...hat.com>,
        freedreno@...ts.freedesktop.org,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Jonas Karlman <jonas@...boo.se>, linux-arm-msm@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, Jyri Sarha <jsarha@...com>,
        Alexios Zavras <alexios.zavras@...el.com>,
        Mamta Shukla <mamtashukla555@...il.com>,
        linux-mediatek@...ts.infradead.org,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sean Paul <sean@...rly.run>,
        linux-arm-kernel@...ts.infradead.org,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        amd-gfx@...ts.freedesktop.org,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        linux-kernel@...r.kernel.org, Todor Tomov <todor.tomov@...aro.org>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Huang Rui <ray.huang@....com>,
        Alex Deucher <alexander.deucher@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Christian König <christian.koenig@....com>,
        Gerd Hoffmann <kraxel@...hat.com>
Subject: Re: [PATCH v4 14/23] drm/tilcdc: Provide ddc symlink in connector
 sysfs directory

Hi

Am 23.07.19 um 14:44 schrieb Andrzej Pietrasiewicz:
> Hi Sam,
> 
> W dniu 23.07.2019 o 11: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 <andrzej.p@...labora.com>
>>> ---
>>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
>>>   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,
>>>       tfp410_connector->mod = mod;
>>>         connector = &tfp410_connector->base;
>>> +    connector->ddc = mod->i2c;
>>>         drm_connector_init(dev, connector, &tfp410_connector_funcs,
>>>               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.
> 
> 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 = drm_connector_init(connector, funcs, ...);
	if (ret)
		return ret;

	if (!ddc)
		return 0;

	connector->ddc = ddc;
	/* set up sysfs */

	return 0;
}

Best regards
Thomas

> Andrzej
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists