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

[Jacopo Mondi <jacopo@jmondi.org>]

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