Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

Javier Martinez Canillas
2018-08-01T16:23:29Z

Hi Marco,

On 08/01/2018 05:49 PM, Marco Felsch wrote:
> Hi Javier,
>
> On 18-07-31 15:30, Marco Felsch wrote:
>> Hi Javier,
>>
>> On 18-07-31 14:52, Javier Martinez Canillas wrote:
>>> Hi Marco,
>>>
>>> On 07/31/2018 02:36 PM, Marco Felsch wrote:
>>>
>>> [snip]
>>>
>>>>>
>>>>> Yes, another thing that patch 19/22 should take into account is DTs that
>>>>> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
>>>>> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
>>>>>
>>>>> In other words, it should work both when input connectors are defined in
>>>>> the DT and when these are not defined and only an output port is defined.
>>>>
>>>> Yes, it would be a approach to map the output port dynamicaly to the
>>>> highest port number. I tried to keep things easy by a static mapping.
>>>> Maybe a follow up patch can change this behaviour.
>>>>
>>>> Anyway, input connectors aren't required. There must be at least one
>>>> port child node with a correct port-number in the DT.
>>>>
>>>
>>> Yes, that was my point. But your patch uses the port child reg property as
>>> the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
>>>
>>> If there's only one port child (for the output) then the DT binding says
>>> that the reg property isn't required, so this will be 0 and your patch will
>>> wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
>>> should be the first one in your enum tvp5150_ports and not the last one.
>>
>> Yes, now I got you. I implemted this in such a way in my first apporach.
>> But at the moment I don't know why I changed this. Maybe to keep the
>> decoder->input number in sync with the em28xx devices, which will set the
>> port by the s_routing() callback.
>>
>> Let me check this.
>
> I checked it again. Your're right, it should be doable but IMHO it isn't
> the right solution. I checked some drivers which use of_graph and all of
> them put the output at the end. So the tvp5150 will be the only one
> which maps the out put to the first pad and it isn't intuitive.
>

Ah, I didn't notice that the tvp5150 was the exception. I just mentioned due
DT maintainers always ask for driver changes to be DTB backward compatible.

> I discused it with a colleague. We think a better solution would be to fix
> the v4l2-core parser code to allow a independent dt-port<->pad mapping.
> Since now the pad's correspond to the port number. This mapping should
> be done by a driver callback, so each driver can do it's own custom
> mapping.
>

That sounds good to me.

> Regards,
> Marco
>

Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat