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

[Jacopo Mondi <jacopo@jmondi.org>]

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