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

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

[Marek Vasut]

Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
To: linux-media@vger.kernel.org
---
 .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
new file mode 100644
index 000000000000..c21703983360
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
@@ -0,0 +1,37 @@
+Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
+
+The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
+receiving up to four analog stream and multiplexing them into up to four
+MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
+
+Required Properties:
+- compatible: value should be "isil,isl79987"
+- pd-gpios: a GPIO spec for the Power Down pin (active high)
+
+Option Properties:
+- isil,num-inputs: Number of connected inputs (1, 2 or 4)
+
+For further reading on port node refer to
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	i2c_master {
+		isl7998x_mipi@44 {
+			compatible = "isil,isl79987";
+			reg = <0x44>;
+			isil,num-inputs = <4>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_videoadc>;
+			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
+			status = "okay";
+
+			port {
+				isl79987_to_mipi_csi2: endpoint {
+					remote-endpoint = <&mipi_csi2_in>;
+					clock-lanes = <0>;
+					data-lanes = <1 2>;
+				};
+			};
+		};
+	};
-- 
2.20.1

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

Hi Marek,
   thanks for the patch

On Mon, May 20, 2019 at 10:18:11PM +0200, Marek Vasut wrote:
> Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> To: linux-media@vger.kernel.org
> ---
>  .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> new file mode 100644
> index 000000000000..c21703983360
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> @@ -0,0 +1,37 @@
> +Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
> +
> +The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
> +receiving up to four analog stream and multiplexing them into up to four
> +MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
> +

The documentation is not public, so I can only read what's reported on
the website and the short public datasheet at [1]

From my understanding of the product page, both the ISL79987 and
ILS79988 devices support up to 4 analog inputs, and provide a CSI-2
output and a BT656 output respectively.

What am I reading wrong ?

[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"
> +- pd-gpios: a GPIO spec for the Power Down pin (active high)
> +
> +Option Properties:
> +- isil,num-inputs: Number of connected inputs (1, 2 or 4)

Can't you derive this from the number of connected input endpoints
instead of providing a custom property?

> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +

I think a description of the supported ports and their intended
usages is required here. You have up to 4 inputs and 1 output port,
how do you expect them to be numbered? is port@4 the last input or the
output one?

> +Example:
> +
> +	i2c_master {
> +		isl7998x_mipi@44 {
> +			compatible = "isil,isl79987";
> +			reg = <0x44>;
> +			isil,num-inputs = <4>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_videoadc>;
> +			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
> +			status = "okay";
> +
> +			port {
> +				isl79987_to_mipi_csi2: endpoint {
> +					remote-endpoint = <&mipi_csi2_in>;
> +					clock-lanes = <0>;
> +					data-lanes = <1 2>;
> +				};

I see from the example you only support one output port? How do you
model the input ones.

Thanks
   j

> +			};
> +		};
> +	};
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marek Vasut]

On 5/28/19 1:47 PM, Jacopo Mondi wrote:
> Hi Marek,
>    thanks for the patch

Hi,

> On Mon, May 20, 2019 at 10:18:11PM +0200, Marek Vasut wrote:
>> Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> To: linux-media@vger.kernel.org
>> ---
>>  .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>> new file mode 100644
>> index 000000000000..c21703983360
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>> @@ -0,0 +1,37 @@
>> +Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
>> +
>> +The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
>> +receiving up to four analog stream and multiplexing them into up to four
>> +MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
>> +
> 
> The documentation is not public, so I can only read what's reported on
> the website and the short public datasheet at [1]

Right

> From my understanding of the product page, both the ISL79987 and
> ILS79988 devices support up to 4 analog inputs, and provide a CSI-2
> output and a BT656 output respectively.
> 
> What am I reading wrong ?

ISL79987 is analog video to mipi csi2 ; I have this chip.
ISL79988 is analog video to bt656 ; I don't have this chip.

> [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"
>> +- pd-gpios: a GPIO spec for the Power Down pin (active high)
>> +
>> +Option Properties:
>> +- isil,num-inputs: Number of connected inputs (1, 2 or 4)
> 
> Can't you derive this from the number of connected input endpoints
> instead of providing a custom property?

Input endpoints from where ?

>> +
>> +For further reading on port node refer to
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
> 
> I think a description of the supported ports and their intended
> usages is required here. You have up to 4 inputs and 1 output port,
> how do you expect them to be numbered? is port@4 the last input or the
> output one?

The only port is the MIPI CSI2 , see the example below.

>> +Example:
>> +
>> +	i2c_master {
>> +		isl7998x_mipi@44 {
>> +			compatible = "isil,isl79987";
>> +			reg = <0x44>;
>> +			isil,num-inputs = <4>;
>> +			pinctrl-names = "default";
>> +			pinctrl-0 = <&pinctrl_videoadc>;
>> +			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
>> +			status = "okay";
>> +
>> +			port {
>> +				isl79987_to_mipi_csi2: endpoint {
>> +					remote-endpoint = <&mipi_csi2_in>;
>> +					clock-lanes = <0>;
>> +					data-lanes = <1 2>;
>> +				};
> 
> 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 ?

-- 
Best regards,
Marek Vasut

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4679 bytes --]

Hi Marek,

On Tue, May 28, 2019 at 04:36:27PM +0200, Marek Vasut wrote:
> On 5/28/19 1:47 PM, Jacopo Mondi wrote:
> > Hi Marek,
> >    thanks for the patch
>
> Hi,
>
> > On Mon, May 20, 2019 at 10:18:11PM +0200, Marek Vasut wrote:
> >> Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> To: linux-media@vger.kernel.org
> >> ---
> >>  .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
> >>  1 file changed, 37 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> >> new file mode 100644
> >> index 000000000000..c21703983360
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> >> @@ -0,0 +1,37 @@
> >> +Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
> >> +
> >> +The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
> >> +receiving up to four analog stream and multiplexing them into up to four
> >> +MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
> >> +
> >
> > The documentation is not public, so I can only read what's reported on
> > the website and the short public datasheet at [1]
>
> Right
>
> > From my understanding of the product page, both the ISL79987 and
> > ILS79988 devices support up to 4 analog inputs, and provide a CSI-2
> > output and a BT656 output respectively.
> >
> > What am I reading wrong ?
>
> ISL79987 is analog video to mipi csi2 ; I have this chip.
> ISL79988 is analog video to bt656 ; I don't have this chip.
>

So please change the description to "Analog to MIPI CSI-2/BT565
decoder"

> > [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.


> >> +- pd-gpios: a GPIO spec for the Power Down pin (active high)
> >> +
> >> +Option Properties:
> >> +- isil,num-inputs: Number of connected inputs (1, 2 or 4)
> >
> > Can't you derive this from the number of connected input endpoints
> > instead of providing a custom property?
>
> Input endpoints from where ?
>

See below :)

> >> +
> >> +For further reading on port node refer to
> >> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >
> > I think a description of the supported ports and their intended
> > usages is required here. You have up to 4 inputs and 1 output port,
> > how do you expect them to be numbered? is port@4 the last input or the
> > output one?
>
> The only port is the MIPI CSI2 , see the example below.
>
> >> +Example:
> >> +
> >> +	i2c_master {
> >> +		isl7998x_mipi@44 {
> >> +			compatible = "isil,isl79987";
> >> +			reg = <0x44>;
> >> +			isil,num-inputs = <4>;
> >> +			pinctrl-names = "default";
> >> +			pinctrl-0 = <&pinctrl_videoadc>;
> >> +			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
> >> +			status = "okay";
> >> +
> >> +			port {
> >> +				isl79987_to_mipi_csi2: endpoint {
> >> +					remote-endpoint = <&mipi_csi2_in>;
> >> +					clock-lanes = <0>;
> >> +					data-lanes = <1 2>;
> >> +				};
> >
> > 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.

Also, you might want to provide 2 output ports, one CSI-2 and one
BT565 and parse the right one depending on the compatible string.

I would also place the input ports last (from port@2 to port@5) so
that we make easier to support similar chips with more inputs (if
any).

That said, I'm no expert of analog video, so others might have
different opinions :)

Thanks
   j

>
> --
> Best regards,
> Marek Vasut

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marek Vasut]

On 5/28/19 5:10 PM, Jacopo Mondi wrote:
[...]

>>> From my understanding of the product page, both the ISL79987 and
>>> ILS79988 devices support up to 4 analog inputs, and provide a CSI-2
>>> output and a BT656 output respectively.
>>>
>>> What am I reading wrong ?
>>
>> ISL79987 is analog video to mipi csi2 ; I have this chip.
>> ISL79988 is analog video to bt656 ; I don't have this chip.
>>
> 
> So please change the description to "Analog to MIPI CSI-2/BT565
> decoder"

Done

>>> [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 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.

> Also, you might want to provide 2 output ports, one CSI-2 and one
> BT565 and parse the right one depending on the compatible string.

The chip only has one physical output port (either CSI2 or parallel) and
DT should model the hardware, so I would expect there to be one output
port per chip ?

> I would also place the input ports last (from port@2 to port@5) so
> that we make easier to support similar chips with more inputs (if
> any).
> 
> That said, I'm no expert of analog video, so others might have
> different opinions :)
> 
> Thanks
>    j
> 
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3600 bytes --]

Hi Marek,

On Tue, May 28, 2019 at 07:49:57PM +0200, Marek Vasut wrote:
> On 5/28/19 5:10 PM, Jacopo Mondi wrote:
> [...]
>
> >>> From my understanding of the product page, both the ISL79987 and
> >>> ILS79988 devices support up to 4 analog inputs, and provide a CSI-2
> >>> output and a BT656 output respectively.
> >>>
> >>> What am I reading wrong ?
> >>
> >> ISL79987 is analog video to mipi csi2 ; I have this chip.
> >> ISL79988 is analog video to bt656 ; I don't have this chip.
> >>
> >
> > So please change the description to "Analog to MIPI CSI-2/BT565
> > decoder"
>
> Done
>

Great!

> >>> [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.

> [...]
>
> >>> 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..

> > Also, you might want to provide 2 output ports, one CSI-2 and one
> > BT565 and parse the right one depending on the compatible string.
>
> The chip only has one physical output port (either CSI2 or parallel) and
> DT should model the hardware, so I would expect there to be one output
> port per chip ?

You are right indeed, there should be one output port only, whose
content will be different between the two chips... Sorry for the
confusion.

Thanks
  j

>
> > I would also place the input ports last (from port@2 to port@5) so
> > that we make easier to support similar chips with more inputs (if
> > any).
> >
> > That said, I'm no expert of analog video, so others might have
> > different opinions :)
> >
> > Thanks
> >    j
> >
> >>
> >> --
> >> Best regards,
> >> Marek Vasut
>
>
> --
> Best regards,
> Marek Vasut

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marek Vasut]

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 ?

[...]

-- 
Best regards,
Marek Vasut

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

[Ian Arkver]

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.

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.

Regards,
Ian
> 
> [...]
> 

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

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

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

[Ian Arkver]

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

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 6223 bytes --]

HI Marek,

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 ? I don't have the
chip manual, but according to what you and Ian said this is not
possible.

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, 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. 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' :)

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sakari Ailus]

Hi Marek,

On Mon, May 20, 2019 at 10:18:11PM +0200, Marek Vasut wrote:
> Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> To: linux-media@vger.kernel.org
> ---
>  .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> new file mode 100644
> index 000000000000..c21703983360
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
> @@ -0,0 +1,37 @@
> +Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
> +
> +The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
> +receiving up to four analog stream and multiplexing them into up to four
> +MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
> +
> +Required Properties:
> +- compatible: value should be "isil,isl79987"
> +- pd-gpios: a GPIO spec for the Power Down pin (active high)
> +
> +Option Properties:
> +- isil,num-inputs: Number of connected inputs (1, 2 or 4)

The presence of ports describing connected Bt.656 inputs tells this.

> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.

Which endpoint properties are relevant for the endpoint(s) in the CSI-2 port?
How about the ports describing the Bt.656 interfaces? You should have
those, too...

> +
> +Example:
> +
> +	i2c_master {
> +		isl7998x_mipi@44 {
> +			compatible = "isil,isl79987";
> +			reg = <0x44>;
> +			isil,num-inputs = <4>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_videoadc>;
> +			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
> +			status = "okay";
> +
> +			port {
> +				isl79987_to_mipi_csi2: endpoint {
> +					remote-endpoint = <&mipi_csi2_in>;
> +					clock-lanes = <0>;
> +					data-lanes = <1 2>;
> +				};
> +			};
> +		};
> +	};
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Marek Vasut]

On 5/29/19 1:15 PM, Ian Arkver wrote:
> 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).

I don't, since I have no way to test the differential mode of operation.

[...]

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

[Marek Vasut]

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

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

[Marek Vasut]

On 7/1/19 10:04 AM, Sakari Ailus wrote:
> Hi Marek,
> 
> On Mon, May 20, 2019 at 10:18:11PM +0200, Marek Vasut wrote:
>> Add bindings for the Intersil ISL7998x BT656-to-MIPI-CSI2 decoder.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> To: linux-media@vger.kernel.org
>> ---
>>  .../bindings/media/i2c/isl7998x.txt           | 37 +++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/isl7998x.txt b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>> new file mode 100644
>> index 000000000000..c21703983360
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/isl7998x.txt
>> @@ -0,0 +1,37 @@
>> +Intersil ISL7998x BT656-to-MIPI-CSI2 decoder
>> +
>> +The Intersil ISL7998x is a BT656-to-MIPI-CSI decoder which, capable of
>> +receiving up to four analog stream and multiplexing them into up to four
>> +MIPI CSI2 virtual channels, using one MIPI clock lane and 1/2 data lanes.
>> +
>> +Required Properties:
>> +- compatible: value should be "isil,isl79987"
>> +- pd-gpios: a GPIO spec for the Power Down pin (active high)
>> +
>> +Option Properties:
>> +- isil,num-inputs: Number of connected inputs (1, 2 or 4)
> 
> The presence of ports describing connected Bt.656 inputs tells this.
> 
>> +
>> +For further reading on port node refer to
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> 
> Which endpoint properties are relevant for the endpoint(s) in the CSI-2 port?

clock-lanes, data-lanes and the remote-endpoint .

> How about the ports describing the Bt.656 interfaces? You should have
> those, too...

So who would be connected to these analog "input" endpoint ? What would
be their "remote-endpoint" ?

>> +
>> +Example:
>> +
>> +	i2c_master {
>> +		isl7998x_mipi@44 {
>> +			compatible = "isil,isl79987";
>> +			reg = <0x44>;
>> +			isil,num-inputs = <4>;
>> +			pinctrl-names = "default";
>> +			pinctrl-0 = <&pinctrl_videoadc>;
>> +			pd-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>;
>> +			status = "okay";
>> +
>> +			port {
>> +				isl79987_to_mipi_csi2: endpoint {
>> +					remote-endpoint = <&mipi_csi2_in>;
>> +					clock-lanes = <0>;
>> +					data-lanes = <1 2>;
>> +				};
>> +			};
>> +		};
>> +	};
>>
> 


-- 
Best regards,
Marek Vasut