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

[Ian Arkver <ian.arkver.dev@gmail.com>]

Hi Marek,

On 29/05/2019 12:09, 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 ?

Bits 0 and 1 of each channel page's Differential Clamping Control 4 
(0x39, ISL7998x_REG_Px_DEC_DIFF_CLMP_CTL_4).

I don't think you change it from the default (single ended on the first 
input).

Regards,
Ian

> 
>> 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 ?
>