Re: [PATCH 1/2] media: dt-bindings: Add Intersil ISL7998x DT bindings

[Marek Vasut <marex@denx.de>]

On 5/29/19 3:43 PM, Jacopo Mondi wrote:
> HI Marek,

Hi,

> On Wed, May 29, 2019 at 01:09:47PM +0200, Marek Vasut wrote:
>> On 5/29/19 1:04 PM, Ian Arkver wrote:
>>> Hi,
>>>
>>> On 29/05/2019 11:41, Marek Vasut wrote:
>>>> On 5/29/19 8:28 AM, Jacopo Mondi wrote:
>>>>
>>>> [...]
>>>>
>>>>>>>>> [1]
>>>>>>>>> https://www.renesas.com/eu/en/products/audio-video/video-decoders-encoders/video-decoders/device/ISL79987.html
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +Required Properties:
>>>>>>>>>> +- compatible: value should be "isil,isl79987"
>>>>>>>
>>>>>>> And here you might want to have 2 different compatibles for 79987 and
>>>>>>> 79988.
>>>>>>
>>>>>> The 79988 is not supported yet, do we want to have it in the binding
>>>>>> doc?
>>>>>>
>>>>>
>>>>> I got mislead by the isl7998x naming scheme you used...
>>>>>
>>>>> I would say that's up to you, the two chips seems very similar,
>>>>> and it might make sense to provide bindings that support both. At the
>>>>> same time, as long as the here defined bindings does not prevent
>>>>> future expansions to include the ISL79988, its support could be safely
>>>>> post-poned. In that case please s/isl7998x/isl79987/ in this document
>>>>> and do not mention BT565 in the description.
>>>>
>>>> Right
>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> I see from the example you only support one output port? How do you
>>>>>>>>> model the input ones.
>>>>>>>>
>>>>>>>> I don't . Do we model analog inputs now somehow ?
>>>>>>>
>>>>>>> I really think so, please see:
>>>>>>> Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
>>>>>>>
>>>>>>>
>>>>>>> And as an example of a board device tree using connectors to model
>>>>>>> analog input see how the cvbs input on Salvator-X is described:
>>>>>>>
>>>>>>>     cvbs-in {
>>>>>>>         compatible = "composite-video-connector";
>>>>>>>         label = "CVBS IN";
>>>>>>>
>>>>>>>         port {
>>>>>>>             cvbs_con: endpoint {
>>>>>>>                 remote-endpoint = <&adv7482_ain7>;
>>>>>>>             };
>>>>>>>         };
>>>>>>>     };
>>>>>>>
>>>>>>> I think you should provide 4 input ports, where to connect input from
>>>>>>> the analog connectors, and derive the number of enabled inputs from
>>>>>>> the number of endpoints connected to an active remote.
>>>>>>
>>>>>> Deriving the number of active physical inputs from some existing
>>>>>> binding
>>>>>> makes sense.
>>>>>>
>>>>>> However unlike the adv7482, the isl79987 does not support remapping the
>>>>>> physical inputs to ADCs in the chip. It does support some remapping of
>>>>>> physical inputs to MIPI CSI2 channels, but that's probably not very
>>>>>> useful.
>>>>>>
>>>>>
>>>>> I understand, but I will now use against you the argument you have
>>>>> correctly pointed out here below that DT should describe hardware, and
>>>>> the hardware has indeed 4 input ports..
>>>>
>>>> My question here is whether it makes sense to describe the ports even if
>>>> they cannot be muxed to different ADC. Does it ?
>>>
>>> Each input port can be either differential CVBS or single ended with a
>>> 2:1 input select mux. It would be nice to be able to describe this.
>>
>> Where do you see that ?
>>
>>> You cannot remap the inputs to different ADCs, but you can remap the
>>> ADCs to different VC IDs using the
>>> ISL7998x_REG_P5_LI_ENGINE_VC_ASSIGNMENT register. Describing each input
>>> would proivde somewhere to specify the vc-id.
>>
>> I think Jacopo mentioned above the input muxing and the MIPI CSI2 VC
>> muxing are two separate things. But I have to wonder, do we have a way
>> of muxing the VCs in the DT or via the media controller yet ?
> 
> I'm not sure I get what you mean with "input muxing", do you mean
> controlling which input is directed to which ADC ?

Yes

> I don't have the
> chip manual, but according to what you and Ian said this is not
> possible.

Correct. You can only remap ADC output(s) to different MIPI CSI2 VC(s).

> Selecting which input video stream is directed to which CSI-2 virtual
> channel in the output CSI-2 stream is not possible in mainline. There
> are patches in development that would allow you to do so, but their
> design is not fully finalized yet. They would, in any case, require
> you to have one sink pad for each input port

Right, and the iMX6 CSI2 driver which I use for development doesn't
support that yet either.

>, and while you could
> register as many pads as it is specified in your custom property,
> you would loose the notion of which input is connected to which port
> ie. when a single input (isil,num-inputs=1) is connected to an input
> port which is not the first one (anyway, quickly looking at 2/2 it
> seems to me you only register a single source pad for this device).
> 
> The DT bindings layout is an orthogonal problem though. My opinion is
> that as the chip has 4 available input connections it should have 4
> input ports in the bindings definition, and for each one you would
> register a sink pad.

Who would be the source for such an input "sink" pad though ? I looked
at
Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt,
as you suggested above, but this is for video _OUTPUT_ devices, not
input devices, so that's likely not what I want.

> Only the actually connected ones should be present
> in the DTS, so that the driver knows which input port is active and,
> once something that allows you to control VC will land in mainline,
> it will let you tell something like "I want the video stream
> received on the input port@2 sent in virtual channel x in the output
> CSI-2 stream", but again this is quite an orthogonal issue. Sure thing
> is that with the current design and implementation, which afaict does
> not provides any sink pad, this is frowned upon (but you can then say
> 'who cares, since it's not here yet' :)

Fine, so let's ignore this for now.

> Or what Ian said, if you can model a connector that provides 2 single
> ended inputs, each connected to an input port and muxed internally by
> the chip, you could only do that if you have numbered input ports I
> guess. Nobody requires you support any of these, but at least bindings
> should be defined in a future proof way, to avoid later changes to the
> ABI.

Well, if endpoint 0 is the MIPI CSI2 output, then you can always add
more endpoints (1..4) for the analog inputs . In this way , the bindings
are already future-proof.

Or shall the input ports come first (as endpoints 0..3) and be followed
by the MIPI CSI2 output (4) ? If so, that would make counting the
(varying number of) inputs a bit cumbersome in the driver.

> Anyway, you had my opinion, multiple times already, and as I'm not in
> any position to judge if something is acceptable for merge or not,
> I'll now get quiet hoping that this prolonged email exchange drags in
> opinion from more experienced people to tell you which direction you
> should take and if what you have here is fine already or not :)
> 
> Cheers
>    j
> 
>>
>> --
>> Best regards,
>> Marek Vasut

-- 
Best regards,
Marek Vasut