[PATCH v3 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 TX controller

[PATCH v3 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 TX controller

[Maxime Ripard]

Hi,

Here is an attempt at supporting the MIPI-CSI2 TX block from Cadence.

This IP block is able to receive 4 video streams and stream them over
a MIPI-CSI2 link using up to 4 lanes. Those streams are basically the
interfaces to controllers generating some video signals, like a camera
or a pattern generator.

It is able to map input streams to CSI2 virtual channels and datatypes
dynamically. The streaming devices choose their virtual channels
through an additional signal that is transparent to the CSI2-TX. The
datatypes however are yet another additional input signal, and can be
mapped to any CSI2 datatypes.

Since v4l2 doesn't really allow for that setup at the moment, this
preliminary version is a rather dumb one in order to start the
discussion on how to address this properly.

Let me know what you think!
Maxime

Changes from v2:
  - Use SPDX license header
  - Use the lane mapping from DT

Changes from v1:
  - Add a subdev notifier and start our downstream subdevice in
    s_stream  
  - Based the decision to enable the stream or not on the link state
    instead of whether a format was being set on the pad
  - Put the controller back in reset when stopping the pipeline
  - Clarified the enpoints number in the DT binding
  - Added a default format for the pads
  - Added some missing const
  - Added more explicit comments
  - Rebased on 4.15

Maxime Ripard (2):
  dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings
  v4l: cadence: Add Cadence MIPI-CSI2 TX driver

 .../devicetree/bindings/media/cdns,csi2tx.txt      |  98 ++++
 drivers/media/platform/cadence/Kconfig             |   6 +
 drivers/media/platform/cadence/Makefile            |   1 +
 drivers/media/platform/cadence/cdns-csi2tx.c       | 582 +++++++++++++++++++++
 4 files changed, 687 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2tx.txt
 create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings

[Maxime Ripard]

The Cadence MIPI-CSI2 TX controller is a CSI2 bridge that supports up to 4
video streams and can output on up to 4 CSI-2 lanes, depending on the
hardware implementation.

It can operate with an external D-PHY, an internal one or no D-PHY at all
in some configurations.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 .../devicetree/bindings/media/cdns,csi2tx.txt      | 98 ++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2tx.txt

diff --git a/Documentation/devicetree/bindings/media/cdns,csi2tx.txt b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
new file mode 100644
index 000000000000..acbbd625a75f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
@@ -0,0 +1,98 @@
+Cadence MIPI-CSI2 TX controller
+===============================
+
+The Cadence MIPI-CSI2 TX controller is a CSI-2 bridge supporting up to
+4 CSI lanes in output, and up to 4 different pixel streams in input.
+
+Required properties:
+  - compatible: must be set to "cdns,csi2tx"
+  - reg: base address and size of the memory mapped region
+  - clocks: phandles to the clocks driving the controller
+  - clock-names: must contain:
+    * esc_clk: escape mode clock
+    * p_clk: register bank clock
+    * pixel_if[0-3]_clk: pixel stream output clock, one for each stream
+                         implemented in hardware, between 0 and 3
+
+Optional properties
+  - phys: phandle to the D-PHY. If it is set, phy-names need to be set
+  - phy-names: must contain dphy
+
+Required subnodes:
+  - ports: A ports node with one port child node per device input and output
+           port, in accordance with the video interface bindings defined in
+           Documentation/devicetree/bindings/media/video-interfaces.txt. The
+           port nodes numbered as follows.
+
+           Port Description
+           -----------------------------
+           0    CSI-2 output
+           1    Stream 0 input
+           2    Stream 1 input
+           3    Stream 2 input
+           4    Stream 3 input
+
+           The stream input port nodes are optional if they are not
+           connected to anything at the hardware level or implemented
+           in the design. Since there is only one endpoint per port,
+           the endpoints are not numbered.
+
+Example:
+
+csi2tx: csi-bridge@0d0e1000 {
+	compatible = "cdns,csi2tx";
+	reg = <0x0d0e1000 0x1000>;
+	clocks = <&byteclock>, <&byteclock>,
+		 <&coreclock>, <&coreclock>,
+		 <&coreclock>, <&coreclock>;
+	clock-names = "p_clk", "esc_clk",
+		      "pixel_if0_clk", "pixel_if1_clk",
+		      "pixel_if2_clk", "pixel_if3_clk";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi2tx_out: endpoint {
+				remote-endpoint = <&remote_in>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			csi2tx_in_stream0: endpoint {
+				remote-endpoint = <&stream0_out>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
+
+			csi2tx_in_stream1: endpoint {
+				remote-endpoint = <&stream1_out>;
+			};
+		};
+
+		port@3 {
+			reg = <3>;
+
+			csi2tx_in_stream2: endpoint {
+				remote-endpoint = <&stream2_out>;
+			};
+		};
+
+		port@4 {
+			reg = <4>;
+
+			csi2tx_in_stream3: endpoint {
+				remote-endpoint = <&stream3_out>;
+			};
+		};
+	};
+};
-- 
2.14.3

[PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Maxime Ripard]

The Cadence MIPI-CSI2 TX controller is an hardware block meant to be used
as a bridge between pixel interfaces and a CSI-2 bus.

It supports operating with an internal or external D-PHY, with up to 4
lanes, or without any D-PHY. The current code only supports the former
case.

While the virtual channel input on the pixel interface can be directly
mapped to CSI2, the datatype input is actually a selection signal (3-bits)
mapping to a table of up to 8 preconfigured datatypes/formats (programmed
at start-up)

The block supports up to 8 input datatypes.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/platform/cadence/Kconfig       |   6 +
 drivers/media/platform/cadence/Makefile      |   1 +
 drivers/media/platform/cadence/cdns-csi2tx.c | 582 +++++++++++++++++++++++++++
 3 files changed, 589 insertions(+)
 create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c

diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
index d1b6bbb6a0eb..db49328ee8b2 100644
--- a/drivers/media/platform/cadence/Kconfig
+++ b/drivers/media/platform/cadence/Kconfig
@@ -9,4 +9,10 @@ config VIDEO_CADENCE_CSI2RX
 	depends on VIDEO_V4L2_SUBDEV_API
 	select V4L2_FWNODE
 
+config VIDEO_CADENCE_CSI2TX
+	tristate "Cadence MIPI-CSI2 TX Controller"
+	depends on MEDIA_CONTROLLER
+	depends on VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+
 endif
diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
index 99a4086b7448..7fe992273162 100644
--- a/drivers/media/platform/cadence/Makefile
+++ b/drivers/media/platform/cadence/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
+obj-$(CONFIG_VIDEO_CADENCE_CSI2TX)	+= cdns-csi2tx.o
diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c b/drivers/media/platform/cadence/cdns-csi2tx.c
new file mode 100644
index 000000000000..3cc58c26d226
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2tx.c
@@ -0,0 +1,582 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Cadence MIPI-CSI2 TX Controller
+ *
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ */
+
+#include <linux/atomic.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define CSI2TX_DEVICE_CONFIG_REG	0x00
+
+#define CSI2TX_CONFIG_REG		0x20
+#define CSI2TX_CONFIG_CFG_REQ			BIT(2)
+#define CSI2TX_CONFIG_SRST_REQ			BIT(1)
+
+#define CSI2TX_DPHY_CFG_REG		0x28
+#define CSI2TX_DPHY_CFG_CLK_RESET		BIT(16)
+#define CSI2TX_DPHY_CFG_LANE_RESET(n)		BIT((n) + 12)
+#define CSI2TX_DPHY_CFG_MODE_MASK		GENMASK(9, 8)
+#define CSI2TX_DPHY_CFG_MODE_LPDT		(2 << 8)
+#define CSI2TX_DPHY_CFG_MODE_HS			(1 << 8)
+#define CSI2TX_DPHY_CFG_MODE_ULPS		(0 << 8)
+#define CSI2TX_DPHY_CFG_CLK_ENABLE		BIT(4)
+#define CSI2TX_DPHY_CFG_LANE_ENABLE(n)		BIT(n)
+
+#define CSI2TX_DPHY_CLK_WAKEUP_REG	0x2c
+#define CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(n)	((n) & 0xffff)
+
+#define CSI2TX_DT_CFG_REG(n)		(0x80 + (n) * 8)
+#define CSI2TX_DT_CFG_DT(n)			(((n) & 0x3f) << 2)
+
+#define CSI2TX_DT_FORMAT_REG(n)		(0x84 + (n) * 8)
+#define CSI2TX_DT_FORMAT_BYTES_PER_LINE(n)	(((n) & 0xffff) << 16)
+#define CSI2TX_DT_FORMAT_MAX_LINE_NUM(n)	((n) & 0xffff)
+
+#define CSI2TX_STREAM_IF_CFG_REG(n)	(0x100 + (n) * 4)
+#define CSI2TX_STREAM_IF_CFG_FILL_LEVEL(n)	((n) & 0x1f)
+
+#define CSI2RX_LANES_MAX	4
+#define CSI2TX_STREAMS_MAX	4
+
+enum csi2tx_pads {
+	CSI2TX_PAD_SOURCE,
+	CSI2TX_PAD_SINK_STREAM0,
+	CSI2TX_PAD_SINK_STREAM1,
+	CSI2TX_PAD_SINK_STREAM2,
+	CSI2TX_PAD_SINK_STREAM3,
+	CSI2TX_PAD_MAX,
+};
+
+struct csi2tx_fmt {
+	u32	mbus;
+	u32	dt;
+	u32	bpp;
+};
+
+struct csi2tx_priv {
+	struct device			*dev;
+	atomic_t			count;
+
+	void __iomem			*base;
+
+	struct clk			*esc_clk;
+	struct clk			*p_clk;
+	struct clk			*pixel_clk[CSI2TX_STREAMS_MAX];
+
+	struct v4l2_subdev		subdev;
+	struct v4l2_async_notifier	notifier;
+	struct media_pad		pads[CSI2TX_PAD_MAX];
+	struct v4l2_mbus_framefmt	pad_fmts[CSI2TX_PAD_MAX];
+
+	bool				has_internal_dphy;
+	u8				lanes[CSI2RX_LANES_MAX];
+	unsigned int			num_lanes;
+	unsigned int			max_lanes;
+	unsigned int			max_streams;
+
+	/* Remote source */
+	struct v4l2_async_subdev	asd;
+	struct v4l2_subdev		*sink_subdev;
+	int				sink_pad;
+};
+
+static const struct csi2tx_fmt csi2tx_formats[] = {
+	{
+		.mbus	= MEDIA_BUS_FMT_UYVY8_1X16,
+		.bpp	= 2,
+		.dt	= 0x1e,
+	},
+	{
+		.mbus	= MEDIA_BUS_FMT_RGB888_1X24,
+		.bpp	= 3,
+		.dt	= 0x24,
+	},
+};
+
+static const struct v4l2_mbus_framefmt fmt_default = {
+	.width		= 320,
+	.height		= 180,
+	.code		= MEDIA_BUS_FMT_RGB888_1X24,
+	.field		= V4L2_FIELD_NONE,
+	.colorspace	= V4L2_COLORSPACE_DEFAULT,
+};
+
+static inline
+struct csi2tx_priv *v4l2_subdev_to_csi2tx(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct csi2tx_priv, subdev);
+}
+
+static int csi2tx_init_cfg(struct v4l2_subdev *subdev,
+			   struct v4l2_subdev_pad_config *cfg)
+{
+	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
+	unsigned int i;
+
+	for (i = 0; i < subdev->entity.num_pads; i++)
+		csi2tx->pad_fmts[i] = fmt_default;
+
+	return 0;
+}
+
+static const struct csi2tx_fmt *csitx_get_fmt_from_mbus(struct v4l2_mbus_framefmt *mfmt)
+{
+	int i;
+
+	if (!mfmt)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(csi2tx_formats); i++)
+		if (csi2tx_formats[i].mbus == mfmt->code)
+			return &csi2tx_formats[i];
+
+	return NULL;
+}
+
+static int csi2tx_enum_mbus_code(struct v4l2_subdev *subdev,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->pad || code->index >= ARRAY_SIZE(csi2tx_formats))
+		return -EINVAL;
+
+	code->code = csi2tx_formats[code->index].mbus;
+
+	return 0;
+}
+
+static int csi2tx_get_pad_format(struct v4l2_subdev *subdev,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
+
+	fmt->format = csi2tx->pad_fmts[fmt->pad];
+
+	return 0;
+}
+
+static int csi2tx_set_pad_format(struct v4l2_subdev *subdev,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
+
+	csi2tx->pad_fmts[fmt->pad] = fmt->format;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_pad_ops csi2tx_pad_ops = {
+	.enum_mbus_code	= csi2tx_enum_mbus_code,
+	.get_fmt	= csi2tx_get_pad_format,
+	.init_cfg	= csi2tx_init_cfg,
+	.set_fmt	= csi2tx_set_pad_format,
+};
+
+static void csi2tx_reset(struct csi2tx_priv *csi2tx)
+{
+	writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
+
+	usleep_range(10, 20);
+}
+
+static int csi2tx_start(struct csi2tx_priv *csi2tx)
+{
+	struct media_entity *entity = &csi2tx->subdev.entity;
+	struct media_link *link;
+	u32 reg;
+	int i;
+
+	/*
+	 * We're not the first users, there's no need to enable the
+	 * whole controller.
+	 */
+	if (atomic_inc_return(&csi2tx->count) > 1)
+		return 0;
+
+	csi2tx_reset(csi2tx);
+
+	writel(CSI2TX_CONFIG_CFG_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
+
+	usleep_range(10, 20);
+
+	writel(CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(32),
+	       csi2tx->base + CSI2TX_DPHY_CLK_WAKEUP_REG);
+
+	/* Put our lanes (clock and data) out of reset */
+	reg = CSI2TX_DPHY_CFG_CLK_RESET | CSI2TX_DPHY_CFG_MODE_LPDT;
+	for (i = 0; i < csi2tx->num_lanes; i++)
+		reg |= CSI2TX_DPHY_CFG_LANE_RESET(csi2tx->lanes[i]);
+	writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
+
+	usleep_range(10, 20);
+
+	/* Enable our (clock and data) lanes */
+	reg |= CSI2TX_DPHY_CFG_CLK_ENABLE;
+	for (i = 0; i < csi2tx->num_lanes; i++)
+		reg |= CSI2TX_DPHY_CFG_LANE_ENABLE(csi2tx->lanes[i]);
+	writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
+
+	usleep_range(10, 20);
+
+	/* Switch to HS mode */
+	reg &= ~CSI2TX_DPHY_CFG_MODE_MASK;
+	writel(reg | CSI2TX_DPHY_CFG_MODE_HS,
+	       csi2tx->base + CSI2TX_DPHY_CFG_REG);
+
+	usleep_range(10, 20);
+
+	/*
+	 * Create a static mapping between the CSI virtual channels
+	 * and the input streams.
+	 *
+	 * This should be enhanced, but v4l2 lacks the support for
+	 * changing that mapping dynamically at the moment.
+	 */
+	list_for_each_entry(link, &entity->links, list) {
+		struct v4l2_mbus_framefmt *mfmt;
+		const struct csi2tx_fmt *fmt;
+		unsigned int stream;
+		int pad_idx = -1;
+
+		/* Only consider our enabled input pads */
+		for (i = CSI2TX_PAD_SINK_STREAM0; i < CSI2TX_PAD_MAX; i++) {
+			struct media_pad *pad = &csi2tx->pads[i];
+
+			if ((pad == link->sink) &&
+			    (link->flags & MEDIA_LNK_FL_ENABLED)) {
+				pad_idx = i;
+				break;
+			}
+		}
+
+		if (pad_idx < 0)
+			continue;
+
+		mfmt = &csi2tx->pad_fmts[i];
+		fmt = csitx_get_fmt_from_mbus(mfmt);
+		stream = pad_idx - CSI2TX_PAD_SINK_STREAM0;
+
+		/*
+		 * We use the stream ID there, but it's wrong.
+		 *
+		 * A stream could very well send a data type that is
+		 * not equal to its stream ID. We need to find a
+		 * proper way to address it.
+		 */
+		writel(CSI2TX_DT_CFG_DT(fmt->dt),
+		       csi2tx->base + CSI2TX_DT_CFG_REG(stream));
+
+		writel(CSI2TX_DT_FORMAT_BYTES_PER_LINE(mfmt->width * fmt->bpp) |
+		       CSI2TX_DT_FORMAT_MAX_LINE_NUM(mfmt->height + 1),
+		       csi2tx->base + CSI2TX_DT_FORMAT_REG(stream));
+
+		/*
+		 * TODO: This needs to be calculated based on the
+		 * output CSI2 clock rate.
+		 */
+		writel(CSI2TX_STREAM_IF_CFG_FILL_LEVEL(4),
+		       csi2tx->base + CSI2TX_STREAM_IF_CFG_REG(stream));
+	}
+
+	/* Disable the configuration mode */
+	writel(0, csi2tx->base + CSI2TX_CONFIG_REG);
+
+	return v4l2_subdev_call(csi2tx->sink_subdev, video, s_stream, true);
+}
+
+static int csi2tx_stop(struct csi2tx_priv *csi2tx)
+{
+	/*
+	 * Let the last user turn off the lights
+	 */
+	if (!atomic_dec_and_test(&csi2tx->count))
+		return 0;
+
+	writel(CSI2TX_CONFIG_CFG_REQ | CSI2TX_CONFIG_SRST_REQ,
+	       csi2tx->base + CSI2TX_CONFIG_REG);
+
+	return 0;
+}
+
+static int csi2tx_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
+	int ret;
+
+	if (enable)
+		ret = csi2tx_start(csi2tx);
+	else
+		ret = csi2tx_stop(csi2tx);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops csi2tx_video_ops = {
+	.s_stream	= csi2tx_s_stream,
+};
+
+static const struct v4l2_subdev_ops csi2tx_subdev_ops = {
+	.pad		= &csi2tx_pad_ops,
+	.video		= &csi2tx_video_ops,
+};
+
+static int csi2tx_async_bound(struct v4l2_async_notifier *notifier,
+			      struct v4l2_subdev *s_subdev,
+			      struct v4l2_async_subdev *asd)
+{
+	struct v4l2_subdev *subdev = notifier->sd;
+	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
+
+	csi2tx->sink_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
+						       s_subdev->fwnode,
+						       MEDIA_PAD_FL_SINK);
+	if (csi2tx->sink_pad < 0) {
+		dev_err(csi2tx->dev, "Couldn't find input pad for subdev %s\n",
+			s_subdev->name);
+		return csi2tx->sink_pad;
+	}
+
+	csi2tx->sink_subdev = s_subdev;
+
+	dev_dbg(csi2tx->dev, "Bound %s pad: %d\n", s_subdev->name,
+		csi2tx->sink_pad);
+
+	return media_create_pad_link(&csi2tx->sink_subdev->entity,
+				     csi2tx->sink_pad,
+				     &csi2tx->subdev.entity, 0,
+				     MEDIA_LNK_FL_ENABLED |
+				     MEDIA_LNK_FL_IMMUTABLE);
+}
+
+static const struct v4l2_async_notifier_operations csi2tx_notifier_ops = {
+	.bound		= csi2tx_async_bound,
+};
+
+static int csi2tx_get_resources(struct csi2tx_priv *csi2tx,
+				struct platform_device *pdev)
+{
+	struct resource *res;
+	u32 reg;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csi2tx->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(csi2tx->base)) {
+		dev_err(&pdev->dev, "Couldn't map our registers\n");
+		return PTR_ERR(csi2tx->base);
+	}
+
+	csi2tx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
+	if (IS_ERR(csi2tx->p_clk)) {
+		dev_err(&pdev->dev, "Couldn't get p_clk\n");
+		return PTR_ERR(csi2tx->p_clk);
+	}
+
+	csi2tx->esc_clk = devm_clk_get(&pdev->dev, "esc_clk");
+	if (IS_ERR(csi2tx->esc_clk)) {
+		dev_err(&pdev->dev, "Couldn't get the esc_clk\n");
+		return PTR_ERR(csi2tx->esc_clk);
+	}
+
+	clk_prepare_enable(csi2tx->p_clk);
+	reg = readl(csi2tx->base + CSI2TX_DEVICE_CONFIG_REG);
+	clk_disable_unprepare(csi2tx->p_clk);
+
+	csi2tx->max_lanes = (reg & 7);
+	if (csi2tx->max_lanes > 4) {
+		dev_err(&pdev->dev, "Invalid number of lanes: %u\n",
+			csi2tx->max_lanes);
+		return -EINVAL;
+	}
+
+	csi2tx->max_streams = ((reg >> 4) & 7);
+	if (csi2tx->max_streams > CSI2TX_STREAMS_MAX) {
+		dev_err(&pdev->dev, "Invalid number of streams: %u\n",
+			csi2tx->max_streams);
+		return -EINVAL;
+	}
+
+	csi2tx->has_internal_dphy = (reg & BIT(3)) ? true : false;
+
+	for (i = 0; i < csi2tx->max_streams; i++) {
+		char clk_name[16];
+
+		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
+		csi2tx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
+		if (IS_ERR(csi2tx->pixel_clk[i])) {
+			dev_err(&pdev->dev, "Couldn't get clock %s\n",
+				clk_name);
+			return PTR_ERR(csi2tx->pixel_clk[i]);
+		}
+	}
+
+	return 0;
+}
+
+static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx)
+{
+	struct v4l2_fwnode_endpoint v4l2_ep;
+	struct device_node *ep;
+	int ret;
+
+	ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
+	if (ret) {
+		dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n");
+		goto out;
+	}
+
+	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
+		dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n",
+			v4l2_ep.bus_type);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
+	if (csi2tx->num_lanes > csi2tx->max_lanes) {
+		dev_err(csi2tx->dev,
+			"Current configuration uses more lanes than supported\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes,
+	       sizeof(csi2tx->lanes));
+
+out:
+	of_node_put(ep);
+	return ret;
+}
+
+static int csi2tx_parse_dt(struct csi2tx_priv *csi2tx)
+{
+	struct fwnode_handle *fwh;
+	struct device_node *ep;
+
+	ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	fwh = of_fwnode_handle(ep);
+	csi2tx->asd.match.fwnode.fwnode = fwnode_graph_get_remote_port_parent(fwh);
+	csi2tx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	of_node_put(ep);
+
+	csi2tx->notifier.subdevs = devm_kzalloc(csi2tx->dev,
+						sizeof(*csi2tx->notifier.subdevs),
+						GFP_KERNEL);
+	if (!csi2tx->notifier.subdevs)
+		return -ENOMEM;
+
+	csi2tx->notifier.subdevs[0] = &csi2tx->asd;
+	csi2tx->notifier.num_subdevs = 1;
+	csi2tx->notifier.ops = &csi2tx_notifier_ops;
+
+	return v4l2_async_subdev_notifier_register(&csi2tx->subdev,
+						   &csi2tx->notifier);
+}
+
+static int csi2tx_probe(struct platform_device *pdev)
+{
+	struct csi2tx_priv *csi2tx;
+	int i, ret;
+
+	csi2tx = kzalloc(sizeof(*csi2tx), GFP_KERNEL);
+	if (!csi2tx)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, csi2tx);
+	csi2tx->dev = &pdev->dev;
+
+	ret = csi2tx_get_resources(csi2tx, pdev);
+	if (ret)
+		goto err_free_priv;
+
+	ret = csi2tx_parse_dt(csi2tx);
+	if (ret)
+		goto err_free_priv;
+
+	v4l2_subdev_init(&csi2tx->subdev, &csi2tx_subdev_ops);
+	csi2tx->subdev.owner = THIS_MODULE;
+	csi2tx->subdev.dev = &pdev->dev;
+	csi2tx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	v4l2_set_subdevdata(&csi2tx->subdev, &pdev->dev);
+	snprintf(csi2tx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
+		 KBUILD_MODNAME, dev_name(&pdev->dev));
+
+	ret = csi2tx_check_lanes(csi2tx);
+	if (ret)
+		goto err_free_priv;
+
+	/* Create our media pads */
+	csi2tx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+	csi2tx->pads[CSI2TX_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	for (i = CSI2TX_PAD_SINK_STREAM0; i < CSI2TX_PAD_MAX; i++)
+		csi2tx->pads[i].flags = MEDIA_PAD_FL_SINK;
+
+	ret = media_entity_pads_init(&csi2tx->subdev.entity, CSI2TX_PAD_MAX,
+				     csi2tx->pads);
+	if (ret)
+		goto err_free_priv;
+
+	ret = v4l2_async_register_subdev(&csi2tx->subdev);
+	if (ret < 0)
+		goto err_free_priv;
+
+	dev_info(&pdev->dev,
+		 "Probed CSI2TX with %u/%u lanes, %u streams, %s D-PHY\n",
+		 csi2tx->num_lanes, csi2tx->max_lanes, csi2tx->max_streams,
+		 csi2tx->has_internal_dphy ? "internal" : "no");
+
+	return 0;
+
+err_free_priv:
+	kfree(csi2tx);
+	return ret;
+}
+
+static int csi2tx_remove(struct platform_device *pdev)
+{
+	struct csi2tx_priv *csi2tx = platform_get_drvdata(pdev);
+
+	v4l2_async_unregister_subdev(&csi2tx->subdev);
+	kfree(csi2tx);
+
+	return 0;
+}
+
+static const struct of_device_id csi2tx_of_table[] = {
+	{ .compatible = "cdns,csi2tx" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, csi2tx_of_table);
+
+static struct platform_driver csi2tx_driver = {
+	.probe	= csi2tx_probe,
+	.remove	= csi2tx_remove,
+
+	.driver	= {
+		.name		= "cdns-csi2tx",
+		.of_match_table	= csi2tx_of_table,
+	},
+};
+module_platform_driver(csi2tx_driver);
-- 
2.14.3

Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings

[Sakari Ailus]

On Wed, Feb 07, 2018 at 03:26:42PM +0100, Maxime Ripard wrote:
> The Cadence MIPI-CSI2 TX controller is a CSI2 bridge that supports up to 4
> video streams and can output on up to 4 CSI-2 lanes, depending on the
> hardware implementation.
> 
> It can operate with an external D-PHY, an internal one or no D-PHY at all
> in some configurations.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Wednesday, 7 February 2018 16:26:42 EET Maxime Ripard wrote:
> The Cadence MIPI-CSI2 TX controller is a CSI2 bridge that supports up to 4
> video streams and can output on up to 4 CSI-2 lanes, depending on the
> hardware implementation.
> 
> It can operate with an external D-PHY, an internal one or no D-PHY at all
> in some configurations.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  .../devicetree/bindings/media/cdns,csi2tx.txt      | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt new file mode
> 100644
> index 000000000000..acbbd625a75f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> @@ -0,0 +1,98 @@
> +Cadence MIPI-CSI2 TX controller
> +===============================
> +
> +The Cadence MIPI-CSI2 TX controller is a CSI-2 bridge supporting up to
> +4 CSI lanes in output, and up to 4 different pixel streams in input.
> +
> +Required properties:
> +  - compatible: must be set to "cdns,csi2tx"
> +  - reg: base address and size of the memory mapped region
> +  - clocks: phandles to the clocks driving the controller
> +  - clock-names: must contain:
> +    * esc_clk: escape mode clock
> +    * p_clk: register bank clock
> +    * pixel_if[0-3]_clk: pixel stream output clock, one for each stream
> +                         implemented in hardware, between 0 and 3
> +
> +Optional properties
> +  - phys: phandle to the D-PHY. If it is set, phy-names need to be set
> +  - phy-names: must contain dphy

Nitpicking, I'd write "dphy" with double quotes.

> +Required subnodes:
> +  - ports: A ports node with one port child node per device input and
> output
> +           port, in accordance with the video interface bindings defined in
> +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> The
> +           port nodes numbered as follows.

s/numbered/are numbered/

> +
> +           Port Description
> +           -----------------------------
> +           0    CSI-2 output
> +           1    Stream 0 input
> +           2    Stream 1 input
> +           3    Stream 2 input
> +           4    Stream 3 input
> +
> +           The stream input port nodes are optional if they are not
> +           connected to anything at the hardware level or implemented
> +           in the design.

Are they optional (and thus valid if present), or should they be forbidden in 
case they're not implemented in the hardware ? I'd go for the latter and write

"One stream input port node is required per implemented hardware input, and no 
stream input port node can be present for unimplemented inputs."

> Since there is only one endpoint per port,
> +           the endpoints are not numbered.

I think it would be valid to number endpoints even if not required. I think 
that what you should document is that at most one endpoint is supported per 
port.

> +
> +Example:
> +
> +csi2tx: csi-bridge@0d0e1000 {
> +	compatible = "cdns,csi2tx";
> +	reg = <0x0d0e1000 0x1000>;
> +	clocks = <&byteclock>, <&byteclock>,
> +		 <&coreclock>, <&coreclock>,
> +		 <&coreclock>, <&coreclock>;
> +	clock-names = "p_clk", "esc_clk",
> +		      "pixel_if0_clk", "pixel_if1_clk",
> +		      "pixel_if2_clk", "pixel_if3_clk";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			csi2tx_out: endpoint {
> +				remote-endpoint = <&remote_in>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			csi2tx_in_stream0: endpoint {
> +				remote-endpoint = <&stream0_out>;
> +			};
> +		};
> +
> +		port@2 {
> +			reg = <2>;
> +
> +			csi2tx_in_stream1: endpoint {
> +				remote-endpoint = <&stream1_out>;
> +			};
> +		};
> +
> +		port@3 {
> +			reg = <3>;
> +
> +			csi2tx_in_stream2: endpoint {
> +				remote-endpoint = <&stream2_out>;
> +			};
> +		};
> +
> +		port@4 {
> +			reg = <4>;
> +
> +			csi2tx_in_stream3: endpoint {
> +				remote-endpoint = <&stream3_out>;
> +			};
> +		};
> +	};
> +};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Wednesday, 7 February 2018 16:26:43 EET Maxime Ripard wrote:
> The Cadence MIPI-CSI2 TX controller is an hardware block meant to be used
> as a bridge between pixel interfaces and a CSI-2 bus.
> 
> It supports operating with an internal or external D-PHY, with up to 4
> lanes, or without any D-PHY. The current code only supports the former
> case.
> 
> While the virtual channel input on the pixel interface can be directly
> mapped to CSI2, the datatype input is actually a selection signal (3-bits)
> mapping to a table of up to 8 preconfigured datatypes/formats (programmed
> at start-up)
> 
> The block supports up to 8 input datatypes.

The DT bindings document four input streams. Does this mean that, to use more 
than 4 data types, a system would need to transmit multiplexed streams on a 
single input stream with the 3 selection bits qualifying each sample ? That 
would be an interesting setup.

> Signed-off-by: Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/media/platform/cadence/Kconfig       |   6 +
>  drivers/media/platform/cadence/Makefile      |   1 +
>  drivers/media/platform/cadence/cdns-csi2tx.c | 582 ++++++++++++++++++++++++
>  3 files changed, 589 insertions(+)
>  create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c
> 
> diff --git a/drivers/media/platform/cadence/Kconfig
> b/drivers/media/platform/cadence/Kconfig index d1b6bbb6a0eb..db49328ee8b2
> 100644
> --- a/drivers/media/platform/cadence/Kconfig
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -9,4 +9,10 @@ config VIDEO_CADENCE_CSI2RX
>  	depends on VIDEO_V4L2_SUBDEV_API
>  	select V4L2_FWNODE
> 
> +config VIDEO_CADENCE_CSI2TX
> +	tristate "Cadence MIPI-CSI2 TX Controller"
> +	depends on MEDIA_CONTROLLER
> +	depends on VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE

A bit of documentation maybe ?

>  endif
> diff --git a/drivers/media/platform/cadence/Makefile
> b/drivers/media/platform/cadence/Makefile index 99a4086b7448..7fe992273162
> 100644
> --- a/drivers/media/platform/cadence/Makefile
> +++ b/drivers/media/platform/cadence/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
> +obj-$(CONFIG_VIDEO_CADENCE_CSI2TX)	+= cdns-csi2tx.o
> diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c
> b/drivers/media/platform/cadence/cdns-csi2tx.c new file mode 100644
> index 000000000000..3cc58c26d226
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2tx.c
> @@ -0,0 +1,582 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Cadence MIPI-CSI2 TX Controller
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define CSI2TX_DEVICE_CONFIG_REG	0x00
> +
> +#define CSI2TX_CONFIG_REG		0x20
> +#define CSI2TX_CONFIG_CFG_REQ			BIT(2)
> +#define CSI2TX_CONFIG_SRST_REQ			BIT(1)
> +
> +#define CSI2TX_DPHY_CFG_REG		0x28
> +#define CSI2TX_DPHY_CFG_CLK_RESET		BIT(16)
> +#define CSI2TX_DPHY_CFG_LANE_RESET(n)		BIT((n) + 12)
> +#define CSI2TX_DPHY_CFG_MODE_MASK		GENMASK(9, 8)
> +#define CSI2TX_DPHY_CFG_MODE_LPDT		(2 << 8)
> +#define CSI2TX_DPHY_CFG_MODE_HS			(1 << 8)
> +#define CSI2TX_DPHY_CFG_MODE_ULPS		(0 << 8)
> +#define CSI2TX_DPHY_CFG_CLK_ENABLE		BIT(4)
> +#define CSI2TX_DPHY_CFG_LANE_ENABLE(n)		BIT(n)
> +
> +#define CSI2TX_DPHY_CLK_WAKEUP_REG	0x2c
> +#define CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(n)	((n) & 0xffff)
> +
> +#define CSI2TX_DT_CFG_REG(n)		(0x80 + (n) * 8)
> +#define CSI2TX_DT_CFG_DT(n)			(((n) & 0x3f) << 2)
> +
> +#define CSI2TX_DT_FORMAT_REG(n)		(0x84 + (n) * 8)
> +#define CSI2TX_DT_FORMAT_BYTES_PER_LINE(n)	(((n) & 0xffff) << 16)
> +#define CSI2TX_DT_FORMAT_MAX_LINE_NUM(n)	((n) & 0xffff)
> +
> +#define CSI2TX_STREAM_IF_CFG_REG(n)	(0x100 + (n) * 4)
> +#define CSI2TX_STREAM_IF_CFG_FILL_LEVEL(n)	((n) & 0x1f)
> +
> +#define CSI2RX_LANES_MAX	4
> +#define CSI2TX_STREAMS_MAX	4
> +
> +enum csi2tx_pads {
> +	CSI2TX_PAD_SOURCE,
> +	CSI2TX_PAD_SINK_STREAM0,
> +	CSI2TX_PAD_SINK_STREAM1,
> +	CSI2TX_PAD_SINK_STREAM2,
> +	CSI2TX_PAD_SINK_STREAM3,
> +	CSI2TX_PAD_MAX,
> +};
> +
> +struct csi2tx_fmt {
> +	u32	mbus;
> +	u32	dt;
> +	u32	bpp;
> +};
> +
> +struct csi2tx_priv {
> +	struct device			*dev;
> +	atomic_t			count;
> +
> +	void __iomem			*base;
> +
> +	struct clk			*esc_clk;
> +	struct clk			*p_clk;
> +	struct clk			*pixel_clk[CSI2TX_STREAMS_MAX];
> +
> +	struct v4l2_subdev		subdev;
> +	struct v4l2_async_notifier	notifier;
> +	struct media_pad		pads[CSI2TX_PAD_MAX];
> +	struct v4l2_mbus_framefmt	pad_fmts[CSI2TX_PAD_MAX];
> +
> +	bool				has_internal_dphy;
> +	u8				lanes[CSI2RX_LANES_MAX];
> +	unsigned int			num_lanes;
> +	unsigned int			max_lanes;
> +	unsigned int			max_streams;
> +
> +	/* Remote source */
> +	struct v4l2_async_subdev	asd;
> +	struct v4l2_subdev		*sink_subdev;
> +	int				sink_pad;
> +};
> +
> +static const struct csi2tx_fmt csi2tx_formats[] = {
> +	{
> +		.mbus	= MEDIA_BUS_FMT_UYVY8_1X16,
> +		.bpp	= 2,
> +		.dt	= 0x1e,
> +	},
> +	{
> +		.mbus	= MEDIA_BUS_FMT_RGB888_1X24,
> +		.bpp	= 3,
> +		.dt	= 0x24,
> +	},
> +};
> +
> +static const struct v4l2_mbus_framefmt fmt_default = {
> +	.width		= 320,
> +	.height		= 180,

That's a pretty small default resolution. Is there any reason for not using a 
more common format ?

> +	.code		= MEDIA_BUS_FMT_RGB888_1X24,
> +	.field		= V4L2_FIELD_NONE,
> +	.colorspace	= V4L2_COLORSPACE_DEFAULT,
> +};
> +
> +static inline
> +struct csi2tx_priv *v4l2_subdev_to_csi2tx(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct csi2tx_priv, subdev);
> +}
> +
> +static int csi2tx_init_cfg(struct v4l2_subdev *subdev,
> +			   struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < subdev->entity.num_pads; i++)
> +		csi2tx->pad_fmts[i] = fmt_default;

.init_cfg() should initialize the formats stored in the cfg structure. The 
active formats stored in your driver-specific structure should be initialized 
at probe time.

> +
> +	return 0;
> +}
> +
> +static const struct csi2tx_fmt *csitx_get_fmt_from_mbus(struct
> v4l2_mbus_framefmt *mfmt)
> +{
> +	int i;

i can be unsigned.

> +
> +	if (!mfmt)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(csi2tx_formats); i++)
> +		if (csi2tx_formats[i].mbus == mfmt->code)
> +			return &csi2tx_formats[i];
> +
> +	return NULL;
> +}
> +
> +static int csi2tx_enum_mbus_code(struct v4l2_subdev *subdev,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->pad || code->index >= ARRAY_SIZE(csi2tx_formats))
> +		return -EINVAL;
> +
> +	code->code = csi2tx_formats[code->index].mbus;
> +
> +	return 0;
> +}
> +
> +static int csi2tx_get_pad_format(struct v4l2_subdev *subdev,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> +
> +	fmt->format = csi2tx->pad_fmts[fmt->pad];
> +
> +	return 0;
> +}
> +
> +static int csi2tx_set_pad_format(struct v4l2_subdev *subdev,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> +
> +	csi2tx->pad_fmts[fmt->pad] = fmt->format;

Doesn't the IP have frame width or height limitations ? Or format limitations 
?

> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops csi2tx_pad_ops = {
> +	.enum_mbus_code	= csi2tx_enum_mbus_code,
> +	.get_fmt	= csi2tx_get_pad_format,
> +	.init_cfg	= csi2tx_init_cfg,
> +	.set_fmt	= csi2tx_set_pad_format,
> +};
> +
> +static void csi2tx_reset(struct csi2tx_priv *csi2tx)
> +{
> +	writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> +
> +	usleep_range(10, 20);

As for the RX driver, udelay() might be better. Same comment for the other 
small delays below.

> +}
> +
> +static int csi2tx_start(struct csi2tx_priv *csi2tx)
> +{
> +	struct media_entity *entity = &csi2tx->subdev.entity;
> +	struct media_link *link;
> +	u32 reg;
> +	int i;

i can be unsigned.

> +
> +	/*
> +	 * We're not the first users, there's no need to enable the
> +	 * whole controller.
> +	 */
> +	if (atomic_inc_return(&csi2tx->count) > 1)
> +		return 0;

Same comment as for the RX driver regarding the potential race condition here.

> +
> +	csi2tx_reset(csi2tx);
> +
> +	writel(CSI2TX_CONFIG_CFG_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> +
> +	usleep_range(10, 20);
> +
> +	writel(CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(32),
> +	       csi2tx->base + CSI2TX_DPHY_CLK_WAKEUP_REG);
> +
> +	/* Put our lanes (clock and data) out of reset */
> +	reg = CSI2TX_DPHY_CFG_CLK_RESET | CSI2TX_DPHY_CFG_MODE_LPDT;
> +	for (i = 0; i < csi2tx->num_lanes; i++)
> +		reg |= CSI2TX_DPHY_CFG_LANE_RESET(csi2tx->lanes[i]);
> +	writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
> +
> +	usleep_range(10, 20);
> +
> +	/* Enable our (clock and data) lanes */
> +	reg |= CSI2TX_DPHY_CFG_CLK_ENABLE;
> +	for (i = 0; i < csi2tx->num_lanes; i++)
> +		reg |= CSI2TX_DPHY_CFG_LANE_ENABLE(csi2tx->lanes[i]);
> +	writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
> +
> +	usleep_range(10, 20);
> +
> +	/* Switch to HS mode */
> +	reg &= ~CSI2TX_DPHY_CFG_MODE_MASK;
> +	writel(reg | CSI2TX_DPHY_CFG_MODE_HS,
> +	       csi2tx->base + CSI2TX_DPHY_CFG_REG);
> +
> +	usleep_range(10, 20);
> +
> +	/*
> +	 * Create a static mapping between the CSI virtual channels
> +	 * and the input streams.
> +	 *
> +	 * This should be enhanced, but v4l2 lacks the support for
> +	 * changing that mapping dynamically at the moment.
> +	 */
> +	list_for_each_entry(link, &entity->links, list) {

I assume that you're protected from the race with userspace setting up links 
by the fact that upper layer should have called media_pipeline_start(). I 
would mention that explicitly in the comment.

> +		struct v4l2_mbus_framefmt *mfmt;
> +		const struct csi2tx_fmt *fmt;
> +		unsigned int stream;
> +		int pad_idx = -1;
> +
> +		/* Only consider our enabled input pads */
> +		for (i = CSI2TX_PAD_SINK_STREAM0; i < CSI2TX_PAD_MAX; i++) {
> +			struct media_pad *pad = &csi2tx->pads[i];
> +
> +			if ((pad == link->sink) &&
> +			    (link->flags & MEDIA_LNK_FL_ENABLED)) {
> +				pad_idx = i;
> +				break;
> +			}
> +		}
> +
> +		if (pad_idx < 0)
> +			continue;
> +
> +		mfmt = &csi2tx->pad_fmts[i];
> +		fmt = csitx_get_fmt_from_mbus(mfmt);

This can return NULL.

> +		stream = pad_idx - CSI2TX_PAD_SINK_STREAM0;
> +
> +		/*
> +		 * We use the stream ID there, but it's wrong.
> +		 *
> +		 * A stream could very well send a data type that is
> +		 * not equal to its stream ID. We need to find a
> +		 * proper way to address it.
> +		 */
> +		writel(CSI2TX_DT_CFG_DT(fmt->dt),
> +		       csi2tx->base + CSI2TX_DT_CFG_REG(stream));
> +
> +		writel(CSI2TX_DT_FORMAT_BYTES_PER_LINE(mfmt->width * fmt->bpp) |
> +		       CSI2TX_DT_FORMAT_MAX_LINE_NUM(mfmt->height + 1),
> +		       csi2tx->base + CSI2TX_DT_FORMAT_REG(stream));
> +
> +		/*
> +		 * TODO: This needs to be calculated based on the
> +		 * output CSI2 clock rate.
> +		 */
> +		writel(CSI2TX_STREAM_IF_CFG_FILL_LEVEL(4),
> +		       csi2tx->base + CSI2TX_STREAM_IF_CFG_REG(stream));
> +	}
> +
> +	/* Disable the configuration mode */
> +	writel(0, csi2tx->base + CSI2TX_CONFIG_REG);
> +
> +	return v4l2_subdev_call(csi2tx->sink_subdev, video, s_stream, true);

I'm pretty sure you should start the sink before the transmitter, otherwise it 
won't be able to synchronize to the LP-11 state.

> +}
> +
> +static int csi2tx_stop(struct csi2tx_priv *csi2tx)
> +{
> +	/*
> +	 * Let the last user turn off the lights
> +	 */
> +	if (!atomic_dec_and_test(&csi2tx->count))
> +		return 0;
> +
> +	writel(CSI2TX_CONFIG_CFG_REQ | CSI2TX_CONFIG_SRST_REQ,
> +	       csi2tx->base + CSI2TX_CONFIG_REG);
> +
> +	return 0;
> +}
> +
> +static int csi2tx_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> +	int ret;
> +
> +	if (enable)
> +		ret = csi2tx_start(csi2tx);
> +	else
> +		ret = csi2tx_stop(csi2tx);

How about

	if (enable)
		return csi2tx_start(csi2tx);
	else
		return csi2tx_stop(csi2tx);

> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2tx_video_ops = {
> +	.s_stream	= csi2tx_s_stream,
> +};
> +
> +static const struct v4l2_subdev_ops csi2tx_subdev_ops = {
> +	.pad		= &csi2tx_pad_ops,
> +	.video		= &csi2tx_video_ops,
> +};
> +
> +static int csi2tx_async_bound(struct v4l2_async_notifier *notifier,
> +			      struct v4l2_subdev *s_subdev,
> +			      struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = notifier->sd;
> +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> +
> +	csi2tx->sink_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> +						       s_subdev->fwnode,
> +						       MEDIA_PAD_FL_SINK);
> +	if (csi2tx->sink_pad < 0) {
> +		dev_err(csi2tx->dev, "Couldn't find input pad for subdev %s\n",
> +			s_subdev->name);
> +		return csi2tx->sink_pad;
> +	}
> +
> +	csi2tx->sink_subdev = s_subdev;
> +
> +	dev_dbg(csi2tx->dev, "Bound %s pad: %d\n", s_subdev->name,
> +		csi2tx->sink_pad);
> +
> +	return media_create_pad_link(&csi2tx->sink_subdev->entity,
> +				     csi2tx->sink_pad,
> +				     &csi2tx->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static const struct v4l2_async_notifier_operations csi2tx_notifier_ops = {
> +	.bound		= csi2tx_async_bound,
> +};
> +
> +static int csi2tx_get_resources(struct csi2tx_priv *csi2tx,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 reg;

Should you name the variable dev_cfg to make it a bit more explicit ?

> +	int i;

i can be unsigned.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	csi2tx->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(csi2tx->base)) {
> +		dev_err(&pdev->dev, "Couldn't map our registers\n");

devm_ioremap_resource() prints an error message, you don't need to duplicate 
that.

> +		return PTR_ERR(csi2tx->base);
> +	}
> +
> +	csi2tx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> +	if (IS_ERR(csi2tx->p_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get p_clk\n");
> +		return PTR_ERR(csi2tx->p_clk);
> +	}
> +
> +	csi2tx->esc_clk = devm_clk_get(&pdev->dev, "esc_clk");
> +	if (IS_ERR(csi2tx->esc_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get the esc_clk\n");
> +		return PTR_ERR(csi2tx->esc_clk);
> +	}
> +
> +	clk_prepare_enable(csi2tx->p_clk);
> +	reg = readl(csi2tx->base + CSI2TX_DEVICE_CONFIG_REG);
> +	clk_disable_unprepare(csi2tx->p_clk);
> +
> +	csi2tx->max_lanes = (reg & 7);

No need for outer parentheses.

> +	if (csi2tx->max_lanes > 4) {
> +		dev_err(&pdev->dev, "Invalid number of lanes: %u\n",
> +			csi2tx->max_lanes);
> +		return -EINVAL;
> +	}
> +
> +	csi2tx->max_streams = ((reg >> 4) & 7);

No need for outer parentheses.

> +	if (csi2tx->max_streams > CSI2TX_STREAMS_MAX) {
> +		dev_err(&pdev->dev, "Invalid number of streams: %u\n",
> +			csi2tx->max_streams);
> +		return -EINVAL;
> +	}
> +
> +	csi2tx->has_internal_dphy = (reg & BIT(3)) ? true : false;
> +
> +	for (i = 0; i < csi2tx->max_streams; i++) {
> +		char clk_name[16];
> +
> +		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> +		csi2tx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> +		if (IS_ERR(csi2tx->pixel_clk[i])) {
> +			dev_err(&pdev->dev, "Couldn't get clock %s\n",
> +				clk_name);
> +			return PTR_ERR(csi2tx->pixel_clk[i]);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx)
> +{
> +	struct v4l2_fwnode_endpoint v4l2_ep;
> +	struct device_node *ep;
> +	int ret;
> +
> +	ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> +	if (ret) {
> +		dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n");
> +		goto out;
> +	}
> +
> +	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n",
> +			v4l2_ep.bus_type);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> +	if (csi2tx->num_lanes > csi2tx->max_lanes) {
> +		dev_err(csi2tx->dev,
> +			"Current configuration uses more lanes than supported\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes,
> +	       sizeof(csi2tx->lanes));
> +
> +out:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static int csi2tx_parse_dt(struct csi2tx_priv *csi2tx)
> +{
> +	struct fwnode_handle *fwh;
> +	struct device_node *ep;
> +
> +	ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	fwh = of_fwnode_handle(ep);
> +	csi2tx->asd.match.fwnode.fwnode =
> fwnode_graph_get_remote_port_parent(fwh);
> +	csi2tx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	of_node_put(ep);
> +
> +	csi2tx->notifier.subdevs = devm_kzalloc(csi2tx->dev,
> +						sizeof(*csi2tx->notifier.subdevs),
> +						GFP_KERNEL);
> +	if (!csi2tx->notifier.subdevs)
> +		return -ENOMEM;
> +
> +	csi2tx->notifier.subdevs[0] = &csi2tx->asd;
> +	csi2tx->notifier.num_subdevs = 1;
> +	csi2tx->notifier.ops = &csi2tx_notifier_ops;
> +
> +	return v4l2_async_subdev_notifier_register(&csi2tx->subdev,
> +						   &csi2tx->notifier);
> +}
> +
> +static int csi2tx_probe(struct platform_device *pdev)
> +{
> +	struct csi2tx_priv *csi2tx;
> +	int i, ret;

I think i can be unsigned.

> +
> +	csi2tx = kzalloc(sizeof(*csi2tx), GFP_KERNEL);
> +	if (!csi2tx)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, csi2tx);
> +	csi2tx->dev = &pdev->dev;
> +
> +	ret = csi2tx_get_resources(csi2tx, pdev);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	ret = csi2tx_parse_dt(csi2tx);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	v4l2_subdev_init(&csi2tx->subdev, &csi2tx_subdev_ops);
> +	csi2tx->subdev.owner = THIS_MODULE;
> +	csi2tx->subdev.dev = &pdev->dev;
> +	csi2tx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	v4l2_set_subdevdata(&csi2tx->subdev, &pdev->dev);
> +	snprintf(csi2tx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> +	ret = csi2tx_check_lanes(csi2tx);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	/* Create our media pads */
> +	csi2tx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;

Same comment as for the RX driver, MEDIA_ENT_F_VID_IF_BRIDGE is better in my 
opinion.

> +	csi2tx->pads[CSI2TX_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	for (i = CSI2TX_PAD_SINK_STREAM0; i < CSI2TX_PAD_MAX; i++)
> +		csi2tx->pads[i].flags = MEDIA_PAD_FL_SINK;
> +
> +	ret = media_entity_pads_init(&csi2tx->subdev.entity, CSI2TX_PAD_MAX,
> +				     csi2tx->pads);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	ret = v4l2_async_register_subdev(&csi2tx->subdev);
> +	if (ret < 0)
> +		goto err_free_priv;
> +
> +	dev_info(&pdev->dev,
> +		 "Probed CSI2TX with %u/%u lanes, %u streams, %s D-PHY\n",
> +		 csi2tx->num_lanes, csi2tx->max_lanes, csi2tx->max_streams,
> +		 csi2tx->has_internal_dphy ? "internal" : "no");
> +
> +	return 0;
> +
> +err_free_priv:
> +	kfree(csi2tx);
> +	return ret;
> +}

[snip]

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Maxime Ripard]

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

Hi Laurent,

On Thu, Feb 08, 2018 at 10:05:05PM +0200, Laurent Pinchart wrote:
> On Wednesday, 7 February 2018 16:26:43 EET Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 TX controller is an hardware block meant to be used
> > as a bridge between pixel interfaces and a CSI-2 bus.
> > 
> > It supports operating with an internal or external D-PHY, with up to 4
> > lanes, or without any D-PHY. The current code only supports the former
> > case.
> > 
> > While the virtual channel input on the pixel interface can be directly
> > mapped to CSI2, the datatype input is actually a selection signal (3-bits)
> > mapping to a table of up to 8 preconfigured datatypes/formats (programmed
> > at start-up)
> > 
> > The block supports up to 8 input datatypes.
> 
> The DT bindings document four input streams. Does this mean that, to use more 
> than 4 data types, a system would need to transmit multiplexed streams on a 
> single input stream with the 3 selection bits qualifying each sample ? That 
> would be an interesting setup.

My understanding is that each input stream has an additional signal
that defines a data type encoded on 3 bits. So yeah, I guess that
would be possible if the upstream device is able to synchronize its
stream generation and the datatype sent.

> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  drivers/media/platform/cadence/Kconfig       |   6 +
> >  drivers/media/platform/cadence/Makefile      |   1 +
> >  drivers/media/platform/cadence/cdns-csi2tx.c | 582 ++++++++++++++++++++++++
> >  3 files changed, 589 insertions(+)
> >  create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c
> > 
> > diff --git a/drivers/media/platform/cadence/Kconfig
> > b/drivers/media/platform/cadence/Kconfig index d1b6bbb6a0eb..db49328ee8b2
> > 100644
> > --- a/drivers/media/platform/cadence/Kconfig
> > +++ b/drivers/media/platform/cadence/Kconfig
> > @@ -9,4 +9,10 @@ config VIDEO_CADENCE_CSI2RX
> >  	depends on VIDEO_V4L2_SUBDEV_API
> >  	select V4L2_FWNODE
> > 
> > +config VIDEO_CADENCE_CSI2TX
> > +	tristate "Cadence MIPI-CSI2 TX Controller"
> > +	depends on MEDIA_CONTROLLER
> > +	depends on VIDEO_V4L2_SUBDEV_API
> > +	select V4L2_FWNODE
> 
> A bit of documentation maybe ?

Yeah, it was already queued for the next iteration :)

> > +static const struct v4l2_mbus_framefmt fmt_default = {
> > +	.width		= 320,
> > +	.height		= 180,
> 
> That's a pretty small default resolution. Is there any reason for not using a 
> more common format ?

What would be your suggestion?

> > +static int csi2tx_init_cfg(struct v4l2_subdev *subdev,
> > +			   struct v4l2_subdev_pad_config *cfg)
> > +{
> > +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < subdev->entity.num_pads; i++)
> > +		csi2tx->pad_fmts[i] = fmt_default;
> 
> .init_cfg() should initialize the formats stored in the cfg structure. The 
> active formats stored in your driver-specific structure should be initialized 
> at probe time.

Ok, I'll change it.

> > +static int csi2tx_set_pad_format(struct v4l2_subdev *subdev,
> > +				 struct v4l2_subdev_pad_config *cfg,
> > +				 struct v4l2_subdev_format *fmt)
> > +{
> > +	struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > +
> > +	csi2tx->pad_fmts[fmt->pad] = fmt->format;
> 
> Doesn't the IP have frame width or height limitations ? Or format
> limitations ?

In its current state, not really. There's also no clear limitations
from the registers on the formats / resolution used, since it's not
configured at all in the device.

The only constraint is that there's no buffer in the IP, so the input
data rate and the output data rate should match. However, the way to
do that is currently uncertain, so I guess we can address that later
on.

> > +static void csi2tx_reset(struct csi2tx_priv *csi2tx)
> > +{
> > +	writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> > +
> > +	usleep_range(10, 20);
> 
> As for the RX driver, udelay() might be better. Same comment for the other 
> small delays below.

I was asked by Benoit to change it to usleep_range in an earlier
iteration, which one should I use?

I'll change the driver according to your other comments.
Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

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

Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings

[Maxime Ripard]

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

Hi Laurent,

On Thu, Feb 08, 2018 at 09:00:19PM +0200, Laurent Pinchart wrote:
> > +
> > +           Port Description
> > +           -----------------------------
> > +           0    CSI-2 output
> > +           1    Stream 0 input
> > +           2    Stream 1 input
> > +           3    Stream 2 input
> > +           4    Stream 3 input
> > +
> > +           The stream input port nodes are optional if they are not
> > +           connected to anything at the hardware level or implemented
> > +           in the design.
> 
> Are they optional (and thus valid if present), or should they be forbidden in 
> case they're not implemented in the hardware ? I'd go for the latter and write
> 
> "One stream input port node is required per implemented hardware input, and no 
> stream input port node can be present for unimplemented inputs."

That works for me.

> > Since there is only one endpoint per port,
> > +           the endpoints are not numbered.
> 
> I think it would be valid to number endpoints even if not required. I think 
> that what you should document is that at most one endpoint is supported per 
> port.

Sakari asked to have it worded that way in this review:
https://www.spinics.net/lists/linux-media/msg122713.html

What should I do?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

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

Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings

[Sakari Ailus]

Hi Laurent,

On Thu, Feb 08, 2018 at 09:00:19PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Wednesday, 7 February 2018 16:26:42 EET Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 TX controller is a CSI2 bridge that supports up to 4
> > video streams and can output on up to 4 CSI-2 lanes, depending on the
> > hardware implementation.
> > 
> > It can operate with an external D-PHY, an internal one or no D-PHY at all
> > in some configurations.
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  .../devicetree/bindings/media/cdns,csi2tx.txt      | 98 +++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> > b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt new file mode
> > 100644
> > index 000000000000..acbbd625a75f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt
> > @@ -0,0 +1,98 @@
> > +Cadence MIPI-CSI2 TX controller
> > +===============================
> > +
> > +The Cadence MIPI-CSI2 TX controller is a CSI-2 bridge supporting up to
> > +4 CSI lanes in output, and up to 4 different pixel streams in input.
> > +
> > +Required properties:
> > +  - compatible: must be set to "cdns,csi2tx"
> > +  - reg: base address and size of the memory mapped region
> > +  - clocks: phandles to the clocks driving the controller
> > +  - clock-names: must contain:
> > +    * esc_clk: escape mode clock
> > +    * p_clk: register bank clock
> > +    * pixel_if[0-3]_clk: pixel stream output clock, one for each stream
> > +                         implemented in hardware, between 0 and 3
> > +
> > +Optional properties
> > +  - phys: phandle to the D-PHY. If it is set, phy-names need to be set
> > +  - phy-names: must contain dphy
> 
> Nitpicking, I'd write "dphy" with double quotes.
> 
> > +Required subnodes:
> > +  - ports: A ports node with one port child node per device input and
> > output
> > +           port, in accordance with the video interface bindings defined in
> > +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> > The
> > +           port nodes numbered as follows.
> 
> s/numbered/are numbered/
> 
> > +
> > +           Port Description
> > +           -----------------------------
> > +           0    CSI-2 output
> > +           1    Stream 0 input
> > +           2    Stream 1 input
> > +           3    Stream 2 input
> > +           4    Stream 3 input
> > +
> > +           The stream input port nodes are optional if they are not
> > +           connected to anything at the hardware level or implemented
> > +           in the design.
> 
> Are they optional (and thus valid if present), or should they be forbidden in 
> case they're not implemented in the hardware ? I'd go for the latter and write
> 
> "One stream input port node is required per implemented hardware input, and no 
> stream input port node can be present for unimplemented inputs."
> 
> > Since there is only one endpoint per port,
> > +           the endpoints are not numbered.
> 
> I think it would be valid to number endpoints even if not required. I think 
> that what you should document is that at most one endpoint is supported per 
> port.

If you allow them to be numbered, then the driver must be able to use any
endpoint number. This provides no additional information on the hardware
while it is more difficult to parse.

That would require reg property in endpoints --- which is not defined here.

I think all new drivers pretty much define it in a similar way (while the
old definitions did not specify it explicitly).

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi