Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

17.04.2020 23:58, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 23:31, Laurent Pinchart пишет:
>>> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
>>>> 17.04.2020 22:30, Laurent Pinchart пишет:
>>>> ...
>>>>>>  #include <drm/drm_atomic.h>
>>>>>> +#include <drm/drm_bridge.h>
>>>>>
>>>>> You could add a forward declaration of struct drm_bridge instead, that
>>>>> can lower the compilation time a little bit.
>>>>
>>>> This include is not only for the struct, but also for the
>>>> drm_bridge_attach(). It looks to me that it should be nicer to keep the
>>>> include here.
>>>
>>> drm_bridge_attach() is called from .c files. In the .h file you can use
>>> a forward declaration. It's entirely up to you, but as a general rule, I
>>> personally try to use forward structure declarations in .h files as much
>>> as possible.
>>
>> The current Tegra DRM code is a bit inconsistent in regards to having
>> forward declarations, it doesn't have them more than have.
>>
>> I'll add a forward declaration if there will be need to make a v5, ok?
> 
> It's up to you, you don't have to use a forward declaration if you don't
> want to, I was just pointing out what I think is a best practice rule
> :-)

Alright, then I'll leave the include as-is in this patch since it should
be better to keep the code consistent even if it's a bit less optimal
than it could be, IMO.

We may return to cleaning up of driver includes later on.

>>>> ...
>>>>>> +	port = of_get_child_by_name(output->of_node, "port");
>>>>>
>>>>> Do you need to check for the presence of a port node first ? Can you
>>>>> just check the return value of drm_of_find_panel_or_bridge(), and fall
>>>>> back to "nvidia,panel" if it returns -ENODEV ?
>>>>
>>>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
>>>> error message about missing port node for every output that doesn't have
>>>> a graph specified in a device-tree (HDMI, DSI and etc).
>>>>
>>>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
>>>
>>> Ah yes indeed. That's not very nice.
>>
>> Please let me know if you'll have a better idea about how this could be
>> handled.
> 
> It should be good enough as-is I think. You may however want to support
> both "port" and "ports", as even when there's a single port node, it
> could be put inside a ports node.
> 

I'll make a v5 that will have additional patches for making
drm_of_find_panel_or_bridge() to better handle that case.

While at it, I'll also add a patch that will wrap RGB panel into bridge.

Thank you very much for the reviews!