[<prev] [next>] [day] [month] [year] [list]
Message-ID: <ef458f71-d061-4a8f-c53c-aa325b577d42@samsung.com>
Date: Fri, 5 Jul 2019 11:44:52 +0200
From: Andrzej Hajda <a.hajda@...sung.com>
To: Thomas Zimmermann <tzimmermann@...e.de>,
Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
dri-devel@...ts.freedesktop.org
Cc: kernel@...labora.com, Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
"David (ChunMing) Zhou" <David1.Zhou@....com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Dave Airlie <airlied@...hat.com>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <maxime.ripard@...tlin.com>,
Sean Paul <sean@...rly.run>, Inki Dae <inki.dae@...sung.com>,
Seung-Woo Kim <sw0312.kim@...sung.com>,
Krzysztof Kozlowski <krzk@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-samsung-soc@...r.kernel.org,
linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org,
linux-rockchip@...ts.infradead.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct
drm_connector
On 01.07.2019 16:41, Thomas Zimmermann wrote:
> Hi
>
> Am 01.07.19 um 15:27 schrieb Andrzej Pietrasiewicz:
>> Hi Thomas,
>>
>> Thank you for your comments. Please see inline.
>>
>> W dniu 30.06.2019 o 10:12, Thomas Zimmermann pisze:
>>> Hi
>>>
>>> I like the idea, but would prefer a more structured approach.
>>>
>>> Setting connector->ddc before calling drm_sysfs_connector_add() seems
>>> error prone. The dependency is not really clear from the code or
>>> interfaces.
>>>
>>> The other thing is that drivers I mostly work on, ast and mgag200, have
>>> code like this:
>>>
>>> struct ast_i2c_chan {
>>> struct i2c_adapter adapter;
>>> struct drm_device *dev;
>>> struct i2c_algo_bit_data bit;
>>> };
>>>
>>> struct ast_connector {
>>> struct drm_connector base;
>>> struct ast_i2c_chan *i2c;
>>> };
>>>
>>> It already encodes the connection between connector and ddc adapter.
>>>
>>> I suggest to introduce struct drm_i2c_adapter
>>>
>>> struct drm_i2c_adapter {
>>> struct i2c_adapter base;
>>> struct drm_connector *connector;
>>> };
>>>
>>> and convert drivers over to it. Ast would look like this:
>>>
>>> struct ast_i2c_chan {
>>> struct drm_i2c_adapter adapter;
>>> struct i2c_algo_bit_data bit;
>>> };
>>>
>> There are few flavors of these drivers. In some of them
>> the i2c_adapter for ddc is allocated and initialized by
>> the driver, which must provide a place in memory to hold
>> the adapter. ast is an example of this approach.
>>
>> But there are others, such as for example exynos_hdmi.c.
>> It gets its ddc adapter with of_find_i2c_adapter_by_node()
>> and merely stores a pointer to it, while not managing the
>> memory needed to hold the i2c_adapter.
> I see.
>
>> Do you have any idea how to accommodate these various
>> flavors of drivers in the scheme you propose?
> Something like
>
> struct drm_i2c_adapter {
> struct i2c_adapter *adapter;
> struct drm_connector *connector;
> };
>
> with adapter either being allocated dynamically or acquired via
> of_find_i2c_adapter_by_node(); with separate init and finish functions
> for each variant. But it looks like over-abstraction to me. Without
> prototyping the idea, I cannot say if it's worth the effort.
>
> For something more simple, maybe just have a function to attach an i2c
> adapter to a connector:
>
> drm_connector_attach_i2c_adapter(connector, i2c_adapter)
>
> which sets the connector's ddc pointer and creates the sysfs entry if
> the connector's entry is already present.
I am not sure if such function is really necessary. Assigning ddc field
before connector registration seems to be much simpler and quite
standard - many fields are initialized this way.
Reviewed-by: Andrzej Hajda <a.hajda@...sung.com>
--
Regards
Andrzej
> Best regards
> Thomas
>
>> Andrzej
>>
>>
Powered by blists - more mailing lists