Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct drm_connector

[Andrzej Hajda <a.hajda@samsung.com>]

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@samsung.com>

 --
Regards
Andrzej



> Best regards
> Thomas
>
>> Andrzej
>>
>>