[PATCH v7 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX

[PATCH v7 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX

[Maxime Ripard]

Hi,

Here is the seventh attempt at supporting the MIPI-CSI2 RX block from
Cadence.

This IP block is able to receive CSI data over up to 4 lanes, and
split it to over 4 streams. Those streams are basically the interfaces
to the video grabbers that will perform the capture.

It is able to map streams to both CSI datatypes and virtual channels,
dynamically. This is unclear at this point what the right way to
support it would be, so the driver only uses a static mapping between
the virtual channels and streams, and ignores the data types.

Let me know what you think!
Maxime

Changes from v6:
  - Added Sakari's Acked-by
  - Added Kconfig help
  - Removed the comment in the probe next to the kmalloc

Changes from v5:
  - Use SPDX license header
  - Fix the lane mapping logic and map unused logical lanes only to unused
    physical lanes. Added a comment to explain why.

Changes from v4:
  - Rebased on top of 4.15
  - Fixed a lane mapping issue that prevented the CSI2-RX device to operate
    properly.
  - Reworded the output endpoints documentation in the binding

Changes from v3:
  - Removed stale printk
  - Propagate start/stop functions error code to s_stream
  - Renamed the DT bindings files
  - Clarified the output ports wording in the DT binding doc
  - Added a define for the maximum number of lanes
  - Rebased on top of Sakari's serie
  - Gathered tags based on the reviews

Changes from v2:
  - Added reference counting for the controller initialisation
  - Fixed checkpatch warnings
  - Moved the sensor initialisation after the DPHY configuration
  - Renamed the sensor fields to source for consistency
  - Defined some variables
  - Renamed a few structures variables
  - Added internal and external phy errors messages
  - Reworked the binding slighty by making the external D-PHY optional
  - Moved the notifier registration in the probe function
  - Removed some clocks that are not system clocks
  - Added clocks enabling where needed
  - Added the code to remap the data lanes
  - Changed the memory allocator for the non-devm function, and a
    comment explaining why
  - Reworked the binding wording

Changes from v1:
  - Amended the DT bindings as suggested by Rob
  - Rebase on top of 4.13-rc1 and latest Niklas' serie iteration

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

 .../devicetree/bindings/media/cdns,csi2rx.txt      | 100 +++++
 drivers/media/platform/Kconfig                     |   1 +
 drivers/media/platform/Makefile                    |   2 +
 drivers/media/platform/cadence/Kconfig             |  17 +
 drivers/media/platform/cadence/Makefile            |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c       | 465 +++++++++++++++++++++
 6 files changed, 586 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt
 create mode 100644 drivers/media/platform/cadence/Kconfig
 create mode 100644 drivers/media/platform/cadence/Makefile
 create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

-- 
2.14.3

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

[Maxime Ripard]

The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
4 CSI-2 lanes, and can route the frames to up to 4 streams, 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>
Acked-by: Benoit Parrot <bparrot@ti.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 .../devicetree/bindings/media/cdns,csi2rx.txt      | 100 +++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt

diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.txt b/Documentation/devicetree/bindings/media/cdns,csi2rx.txt
new file mode 100644
index 000000000000..56d51902b2eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.txt
@@ -0,0 +1,100 @@
+Cadence MIPI-CSI2 RX controller
+===============================
+
+The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4 CSI
+lanes in input, and 4 different pixel streams in output.
+
+Required properties:
+  - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
+  - reg: base address and size of the memory mapped region
+  - clocks: phandles to the clocks driving the controller
+  - clock-names: must contain:
+    * sys_clk: main 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 external D-PHY, phy-names must be provided
+  - phy-names: must contain dphy, if the implementation uses an
+               external D-PHY
+
+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 input
+           1    Stream 0 output
+           2    Stream 1 output
+           3    Stream 2 output
+           4    Stream 3 output
+
+           The stream output 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:
+
+csi2rx: csi-bridge@0d060000 {
+	compatible = "cdns,csi2rx";
+	reg = <0x0d060000 0x1000>;
+	clocks = <&byteclock>, <&byteclock>
+		 <&coreclock>, <&coreclock>,
+		 <&coreclock>, <&coreclock>;
+	clock-names = "sys_clk", "p_clk",
+		      "pixel_if0_clk", "pixel_if1_clk",
+		      "pixel_if2_clk", "pixel_if3_clk";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi2rx_in_sensor: endpoint {
+				remote-endpoint = <&sensor_out_csi2rx>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			csi2rx_out_grabber0: endpoint {
+				remote-endpoint = <&grabber0_in_csi2rx>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
+
+			csi2rx_out_grabber1: endpoint {
+				remote-endpoint = <&grabber1_in_csi2rx>;
+			};
+		};
+
+		port@3 {
+			reg = <3>;
+
+			csi2rx_out_grabber2: endpoint {
+				remote-endpoint = <&grabber2_in_csi2rx>;
+			};
+		};
+
+		port@4 {
+			reg = <4>;
+
+			csi2rx_out_grabber3: endpoint {
+				remote-endpoint = <&grabber3_in_csi2rx>;
+			};
+		};
+	};
+};
-- 
2.14.3

[PATCH v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
bridge between a CSI-2 bus and pixel grabbers.

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

It also support dynamic mapping of the CSI-2 virtual channels to the
associated pixel grabbers, but that isn't allowed at the moment either.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cadence/Kconfig       |  17 +
 drivers/media/platform/cadence/Makefile      |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c | 465 +++++++++++++++++++++++++++
 5 files changed, 486 insertions(+)
 create mode 100644 drivers/media/platform/cadence/Kconfig
 create mode 100644 drivers/media/platform/cadence/Makefile
 create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index fd0c99859d6f..6e790a317fbc 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
 #
 # Platform multimedia device configuration
 #
+source "drivers/media/platform/cadence/Kconfig"
 
 source "drivers/media/platform/davinci/Kconfig"
 
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 003b0bb2cddf..1cd2984c55d1 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -3,6 +3,8 @@
 # Makefile for the video capture/playback device drivers.
 #
 
+obj-$(CONFIG_VIDEO_CADENCE)		+= cadence/
+
 obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
 
 obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
new file mode 100644
index 000000000000..18f061e5cbd1
--- /dev/null
+++ b/drivers/media/platform/cadence/Kconfig
@@ -0,0 +1,17 @@
+config VIDEO_CADENCE
+	bool "Cadence Video Devices"
+
+if VIDEO_CADENCE
+
+config VIDEO_CADENCE_CSI2RX
+	tristate "Cadence MIPI-CSI2 RX Controller"
+	depends on MEDIA_CONTROLLER
+	depends on VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	help
+	  Support for the Cadence MIPI CSI2 Receiver controller.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cdns-csi2rx.
+
+endif
diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
new file mode 100644
index 000000000000..99a4086b7448
--- /dev/null
+++ b/drivers/media/platform/cadence/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
new file mode 100644
index 000000000000..353a9b10197b
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,465 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Cadence MIPI-CSI2 RX Controller v1.3
+ *
+ * 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/phy/phy.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 CSI2RX_DEVICE_CFG_REG			0x000
+
+#define CSI2RX_SOFT_RESET_REG			0x004
+#define CSI2RX_SOFT_RESET_PROTOCOL			BIT(1)
+#define CSI2RX_SOFT_RESET_FRONT				BIT(0)
+
+#define CSI2RX_STATIC_CFG_REG			0x008
+#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)	((plane) << (16 + (llane) * 4))
+#define CSI2RX_STATIC_CFG_LANES_MASK			GENMASK(11, 8)
+
+#define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
+
+#define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_START			BIT(0)
+
+#define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
+#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
+#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
+
+#define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
+#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
+
+#define CSI2RX_LANES_MAX	4
+#define CSI2RX_STREAMS_MAX	4
+
+enum csi2rx_pads {
+	CSI2RX_PAD_SINK,
+	CSI2RX_PAD_SOURCE_STREAM0,
+	CSI2RX_PAD_SOURCE_STREAM1,
+	CSI2RX_PAD_SOURCE_STREAM2,
+	CSI2RX_PAD_SOURCE_STREAM3,
+	CSI2RX_PAD_MAX,
+};
+
+struct csi2rx_priv {
+	struct device			*dev;
+	atomic_t			count;
+
+	void __iomem			*base;
+	struct clk			*sys_clk;
+	struct clk			*p_clk;
+	struct clk			*pixel_clk[CSI2RX_STREAMS_MAX];
+	struct phy			*dphy;
+
+	u8				lanes[CSI2RX_LANES_MAX];
+	u8				num_lanes;
+	u8				max_lanes;
+	u8				max_streams;
+	bool				has_internal_dphy;
+
+	struct v4l2_subdev		subdev;
+	struct v4l2_async_notifier	notifier;
+	struct media_pad		pads[CSI2RX_PAD_MAX];
+
+	/* Remote source */
+	struct v4l2_async_subdev	asd;
+	struct v4l2_subdev		*source_subdev;
+	int				source_pad;
+};
+
+static inline
+struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct csi2rx_priv, subdev);
+}
+
+static void csi2rx_reset(struct csi2rx_priv *csi2rx)
+{
+	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
+	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+	usleep_range(10, 20);
+
+	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+}
+
+static int csi2rx_start(struct csi2rx_priv *csi2rx)
+{
+	unsigned int i;
+	unsigned long lanes_used = 0;
+	u32 reg;
+	int ret;
+
+	/*
+	 * We're not the first users, there's no need to enable the
+	 * whole controller.
+	 */
+	if (atomic_inc_return(&csi2rx->count) > 1)
+		return 0;
+
+	clk_prepare_enable(csi2rx->p_clk);
+
+	csi2rx_reset(csi2rx);
+
+	reg = csi2rx->num_lanes << 8;
+	for (i = 0; i < csi2rx->num_lanes; i++) {
+		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
+		set_bit(csi2rx->lanes[i], &lanes_used);
+	}
+
+	/*
+	 * Even the unused lanes need to be mapped. In order to avoid
+	 * to map twice to the same physical lane, keep the lanes used
+	 * in the previous loop, and only map unused physical lanes to
+	 * the rest of our logical lanes.
+	 */
+	for (i = csi2rx->num_lanes; i < csi2rx->max_lanes; i++) {
+		unsigned int idx = find_first_zero_bit(&lanes_used,
+						       sizeof(lanes_used));
+		set_bit(idx, &lanes_used);
+		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, i + 1);
+	}
+
+	writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
+
+	ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
+	if (ret)
+		return ret;
+
+	/*
+	 * Create a static mapping between the CSI virtual channels
+	 * and the output stream.
+	 *
+	 * This should be enhanced, but v4l2 lacks the support for
+	 * changing that mapping dynamically.
+	 *
+	 * We also cannot enable and disable independant streams here,
+	 * hence the reference counting.
+	 */
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		clk_prepare_enable(csi2rx->pixel_clk[i]);
+
+		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
+		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
+
+		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
+		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
+		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
+
+		writel(CSI2RX_STREAM_CTRL_START,
+		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+	}
+
+	clk_prepare_enable(csi2rx->sys_clk);
+
+	clk_disable_unprepare(csi2rx->p_clk);
+
+	return 0;
+}
+
+static int csi2rx_stop(struct csi2rx_priv *csi2rx)
+{
+	unsigned int i;
+
+	/*
+	 * Let the last user turn off the lights
+	 */
+	if (!atomic_dec_and_test(&csi2rx->count))
+		return 0;
+
+	clk_prepare_enable(csi2rx->p_clk);
+
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+		clk_disable_unprepare(csi2rx->pixel_clk[i]);
+	}
+
+	clk_disable_unprepare(csi2rx->p_clk);
+
+	return v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false);
+}
+
+static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
+	int ret;
+
+	if (enable)
+		ret = csi2rx_start(csi2rx);
+	else
+		ret = csi2rx_stop(csi2rx);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
+	.s_stream	= csi2rx_s_stream,
+};
+
+static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
+	.video		= &csi2rx_video_ops,
+};
+
+static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
+			      struct v4l2_subdev *s_subdev,
+			      struct v4l2_async_subdev *asd)
+{
+	struct v4l2_subdev *subdev = notifier->sd;
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	csi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
+							 s_subdev->fwnode,
+							 MEDIA_PAD_FL_SOURCE);
+	if (csi2rx->source_pad < 0) {
+		dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
+			s_subdev->name);
+		return csi2rx->source_pad;
+	}
+
+	csi2rx->source_subdev = s_subdev;
+
+	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
+		csi2rx->source_pad);
+
+	return media_create_pad_link(&csi2rx->source_subdev->entity,
+				     csi2rx->source_pad,
+				     &csi2rx->subdev.entity, 0,
+				     MEDIA_LNK_FL_ENABLED |
+				     MEDIA_LNK_FL_IMMUTABLE);
+}
+
+static const struct v4l2_async_notifier_operations csi2rx_notifier_ops = {
+	.bound		= csi2rx_async_bound,
+};
+
+static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
+				struct platform_device *pdev)
+{
+	struct resource *res;
+	unsigned char i;
+	u32 reg;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(csi2rx->base))
+		return PTR_ERR(csi2rx->base);
+
+	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
+	if (IS_ERR(csi2rx->sys_clk)) {
+		dev_err(&pdev->dev, "Couldn't get sys clock\n");
+		return PTR_ERR(csi2rx->sys_clk);
+	}
+
+	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
+	if (IS_ERR(csi2rx->p_clk)) {
+		dev_err(&pdev->dev, "Couldn't get P clock\n");
+		return PTR_ERR(csi2rx->p_clk);
+	}
+
+	csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
+	if (IS_ERR(csi2rx->dphy)) {
+		dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
+		return PTR_ERR(csi2rx->dphy);
+	}
+
+	/*
+	 * FIXME: Once we'll have external D-PHY support, the check
+	 * will need to be removed.
+	 */
+	if (csi2rx->dphy) {
+		dev_err(&pdev->dev, "External D-PHY not supported yet\n");
+		return -EINVAL;
+	}
+
+	clk_prepare_enable(csi2rx->p_clk);
+	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
+	clk_disable_unprepare(csi2rx->p_clk);
+
+	csi2rx->max_lanes = (reg & 7);
+	if (csi2rx->max_lanes > CSI2RX_LANES_MAX) {
+		dev_err(&pdev->dev, "Invalid number of lanes: %u\n",
+			csi2rx->max_lanes);
+		return -EINVAL;
+	}
+
+	csi2rx->max_streams = ((reg >> 4) & 7);
+	if (csi2rx->max_streams > CSI2RX_STREAMS_MAX) {
+		dev_err(&pdev->dev, "Invalid number of streams: %u\n",
+			csi2rx->max_streams);
+		return -EINVAL;
+	}
+
+	csi2rx->has_internal_dphy = (reg & BIT(3)) ? true : false;
+
+	/*
+	 * FIXME: Once we'll have internal D-PHY support, the check
+	 * will need to be removed.
+	 */
+	if (csi2rx->has_internal_dphy) {
+		dev_err(&pdev->dev, "Internal D-PHY not supported yet\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		char clk_name[16];
+
+		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
+		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
+		if (IS_ERR(csi2rx->pixel_clk[i])) {
+			dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
+			return PTR_ERR(csi2rx->pixel_clk[i]);
+		}
+	}
+
+	return 0;
+}
+
+static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
+{
+	struct v4l2_fwnode_endpoint v4l2_ep;
+	struct fwnode_handle *fwh;
+	struct device_node *ep;
+	int ret;
+
+	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	fwh = of_fwnode_handle(ep);
+	ret = v4l2_fwnode_endpoint_parse(fwh, &v4l2_ep);
+	if (ret) {
+		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
+		of_node_put(ep);
+		return ret;
+	}
+
+	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
+		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
+			v4l2_ep.bus_type);
+		of_node_put(ep);
+		return -EINVAL;
+	}
+
+	memcpy(csi2rx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes,
+	       sizeof(csi2rx->lanes));
+	csi2rx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
+	if (csi2rx->num_lanes > csi2rx->max_lanes) {
+		dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
+			csi2rx->num_lanes);
+		of_node_put(ep);
+		return -EINVAL;
+	}
+
+	csi2rx->asd.match.fwnode.fwnode = fwnode_graph_get_remote_port_parent(fwh);
+	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	of_node_put(ep);
+
+	csi2rx->notifier.subdevs = devm_kzalloc(csi2rx->dev,
+						sizeof(*csi2rx->notifier.subdevs),
+						GFP_KERNEL);
+	if (!csi2rx->notifier.subdevs)
+		return -ENOMEM;
+
+	csi2rx->notifier.subdevs[0] = &csi2rx->asd;
+	csi2rx->notifier.num_subdevs = 1;
+	csi2rx->notifier.ops = &csi2rx_notifier_ops;
+
+	return v4l2_async_subdev_notifier_register(&csi2rx->subdev,
+						   &csi2rx->notifier);
+}
+
+static int csi2rx_probe(struct platform_device *pdev)
+{
+	struct csi2rx_priv *csi2rx;
+	unsigned int i;
+	int ret;
+
+	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
+	if (!csi2rx)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, csi2rx);
+	csi2rx->dev = &pdev->dev;
+
+	ret = csi2rx_get_resources(csi2rx, pdev);
+	if (ret)
+		goto err_free_priv;
+
+	ret = csi2rx_parse_dt(csi2rx);
+	if (ret)
+		goto err_free_priv;
+
+	csi2rx->subdev.owner = THIS_MODULE;
+	csi2rx->subdev.dev = &pdev->dev;
+	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
+	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
+	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
+		 KBUILD_MODNAME, dev_name(&pdev->dev));
+
+	/* Create our media pads */
+	csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
+		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
+				     csi2rx->pads);
+	if (ret)
+		goto err_free_priv;
+
+	ret = v4l2_async_register_subdev(&csi2rx->subdev);
+	if (ret < 0)
+		goto err_free_priv;
+
+	dev_info(&pdev->dev,
+		 "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n",
+		 csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams,
+		 csi2rx->has_internal_dphy ? "internal" : "no");
+
+	return 0;
+
+err_free_priv:
+	kfree(csi2rx);
+	return ret;
+}
+
+static int csi2rx_remove(struct platform_device *pdev)
+{
+	struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
+
+	v4l2_async_unregister_subdev(&csi2rx->subdev);
+	kfree(csi2rx);
+
+	return 0;
+}
+
+static const struct of_device_id csi2rx_of_table[] = {
+	{ .compatible = "cdns,csi2rx" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, csi2rx_of_table);
+
+static struct platform_driver csi2rx_driver = {
+	.probe	= csi2rx_probe,
+	.remove	= csi2rx_remove,
+
+	.driver	= {
+		.name		= "cdns-csi2rx",
+		.of_match_table	= csi2rx_of_table,
+	},
+};
+module_platform_driver(csi2rx_driver);
-- 
2.14.3

Re: [PATCH v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Thursday, 8 February 2018 17:08:30 EET Maxime Ripard wrote:
> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
> bridge between a CSI-2 bus and pixel grabbers.
> 
> It supports operating with internal or external D-PHY, with up to 4 lanes,
> or without any D-PHY. The current code only supports the former case.

Don't you support the latter case, not the former ? The driver bails out with 
an error if an internal or external PHY is present.

> It also support dynamic mapping of the CSI-2 virtual channels to the
> associated pixel grabbers, but that isn't allowed at the moment either.
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cadence/Kconfig       |  17 +
>  drivers/media/platform/cadence/Makefile      |   1 +
>  drivers/media/platform/cadence/cdns-csi2rx.c | 465 ++++++++++++++++++++++++
>  5 files changed, 486 insertions(+)
>  create mode 100644 drivers/media/platform/cadence/Kconfig
>  create mode 100644 drivers/media/platform/cadence/Makefile
>  create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index fd0c99859d6f..6e790a317fbc 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
>  #
>  # Platform multimedia device configuration
>  #
> +source "drivers/media/platform/cadence/Kconfig"
> 
>  source "drivers/media/platform/davinci/Kconfig"
> 
> diff --git a/drivers/media/platform/Makefile
> b/drivers/media/platform/Makefile index 003b0bb2cddf..1cd2984c55d1 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -3,6 +3,8 @@
>  # Makefile for the video capture/playback device drivers.
>  #
> 
> +obj-$(CONFIG_VIDEO_CADENCE)		+= cadence/
> +
>  obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
> 
>  obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
> diff --git a/drivers/media/platform/cadence/Kconfig
> b/drivers/media/platform/cadence/Kconfig new file mode 100644
> index 000000000000..18f061e5cbd1
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -0,0 +1,17 @@
> +config VIDEO_CADENCE
> +	bool "Cadence Video Devices"
> +
> +if VIDEO_CADENCE
> +
> +config VIDEO_CADENCE_CSI2RX
> +	tristate "Cadence MIPI-CSI2 RX Controller"
> +	depends on MEDIA_CONTROLLER
> +	depends on VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE
> +	help
> +	  Support for the Cadence MIPI CSI2 Receiver controller.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called cdns-csi2rx.
> +
> +endif
> diff --git a/drivers/media/platform/cadence/Makefile
> b/drivers/media/platform/cadence/Makefile new file mode 100644
> index 000000000000..99a4086b7448
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> b/drivers/media/platform/cadence/cdns-csi2rx.c new file mode 100644
> index 000000000000..353a9b10197b
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,465 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * 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/phy/phy.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 CSI2RX_DEVICE_CFG_REG			0x000
> +
> +#define CSI2RX_SOFT_RESET_REG			0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL			BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT				BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG			0x008
> +#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)	((plane) << (16 + 
(llane)
> * 4)) +#define CSI2RX_STATIC_CFG_LANES_MASK			GENMASK(11, 8)
> +
> +#define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
> +
> +#define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
> +#define CSI2RX_STREAM_CTRL_START			BIT(0)
> +
> +#define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
> +
> +#define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
> +
> +#define CSI2RX_LANES_MAX	4
> +#define CSI2RX_STREAMS_MAX	4
> +
> +enum csi2rx_pads {
> +	CSI2RX_PAD_SINK,
> +	CSI2RX_PAD_SOURCE_STREAM0,
> +	CSI2RX_PAD_SOURCE_STREAM1,
> +	CSI2RX_PAD_SOURCE_STREAM2,
> +	CSI2RX_PAD_SOURCE_STREAM3,
> +	CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> +	struct device			*dev;
> +	atomic_t			count;
> +
> +	void __iomem			*base;
> +	struct clk			*sys_clk;
> +	struct clk			*p_clk;
> +	struct clk			*pixel_clk[CSI2RX_STREAMS_MAX];
> +	struct phy			*dphy;
> +
> +	u8				lanes[CSI2RX_LANES_MAX];
> +	u8				num_lanes;
> +	u8				max_lanes;
> +	u8				max_streams;
> +	bool				has_internal_dphy;
> +
> +	struct v4l2_subdev		subdev;
> +	struct v4l2_async_notifier	notifier;
> +	struct media_pad		pads[CSI2RX_PAD_MAX];
> +
> +	/* Remote source */
> +	struct v4l2_async_subdev	asd;
> +	struct v4l2_subdev		*source_subdev;
> +	int				source_pad;
> +};
> +
> +static inline
> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct csi2rx_priv, subdev);
> +}
> +
> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> +{
> +	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> +	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> +	usleep_range(10, 20);

That's a very short range. Is it worth getting the scheduler involved, 
wouldn't udelay() be better ?

> +	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> +	unsigned int i;
> +	unsigned long lanes_used = 0;
> +	u32 reg;
> +	int ret;
> +
> +	/*
> +	 * We're not the first users, there's no need to enable the
> +	 * whole controller.
> +	 */
> +	if (atomic_inc_return(&csi2rx->count) > 1)
> +		return 0;

You have a race condition here. If s_stream(1) is called twice in parallel, 
the second call will return immediately while the first call might still be 
starting the stream. The caller could thus proceed with the stream not enabled 
yet, which in some cases could be an issue. If you want to fix this you'll 
need a mutex.

> +	clk_prepare_enable(csi2rx->p_clk);

No need to check for errors ?

> +	csi2rx_reset(csi2rx);
> +
> +	reg = csi2rx->num_lanes << 8;
> +	for (i = 0; i < csi2rx->num_lanes; i++) {
> +		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
> +		set_bit(csi2rx->lanes[i], &lanes_used);
> +	}
> +
> +	/*
> +	 * Even the unused lanes need to be mapped. In order to avoid
> +	 * to map twice to the same physical lane, keep the lanes used
> +	 * in the previous loop, and only map unused physical lanes to
> +	 * the rest of our logical lanes.
> +	 */
> +	for (i = csi2rx->num_lanes; i < csi2rx->max_lanes; i++) {
> +		unsigned int idx = find_first_zero_bit(&lanes_used,
> +						       sizeof(lanes_used));
> +		set_bit(idx, &lanes_used);
> +		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, i + 1);
> +	}
> +
> +	writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +
> +	ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
> +	if (ret)
> +		return ret;

Doesn't the CSI-2 receiver need to be fully started before the sensor ? 
Otherwise I believe it could have trouble synchronizing to the LP-11 state. 
That's a PHY issue though, so maybe you plan to fix this later when adding 
support for PHYs ?

> +	/*
> +	 * Create a static mapping between the CSI virtual channels
> +	 * and the output stream.
> +	 *
> +	 * This should be enhanced, but v4l2 lacks the support for
> +	 * changing that mapping dynamically.
> +	 *
> +	 * We also cannot enable and disable independant streams here,
> +	 * hence the reference counting.
> +	 */

If you start all streams in one go, will s_stream(1) be called multiple times 
? If not, you could possibly skip the whole reference counting and avoid 
locking.

> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		clk_prepare_enable(csi2rx->pixel_clk[i]);
> +
> +		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> +		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
> +		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
> +		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_CTRL_START,
> +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +	}
> +
> +	clk_prepare_enable(csi2rx->sys_clk);
> +
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> +{
> +	unsigned int i;
> +
> +	/*
> +	 * Let the last user turn off the lights
> +	 */
> +	if (!atomic_dec_and_test(&csi2rx->count))
> +		return 0;
> +
> +	clk_prepare_enable(csi2rx->p_clk);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> +		clk_disable_unprepare(csi2rx->pixel_clk[i]);
> +	}
> +
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false);
> +}
> +
> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +	int ret;
> +
> +	if (enable)
> +		ret = csi2rx_start(csi2rx);
> +	else
> +		ret = csi2rx_stop(csi2rx);

Nitpicking, you could

	if (enable)
		return csi2rx_start(csi2rx);
	else
		return csi2tx_start(csi2tx);

> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> +	.s_stream	= csi2rx_s_stream,
> +};
> +
> +static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> +	.video		= &csi2rx_video_ops,
> +};
> +
> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> +			      struct v4l2_subdev *s_subdev,
> +			      struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = notifier->sd;
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	csi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> +							 s_subdev->fwnode,
> +							 MEDIA_PAD_FL_SOURCE);
> +	if (csi2rx->source_pad < 0) {
> +		dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
> +			s_subdev->name);
> +		return csi2rx->source_pad;
> +	}
> +
> +	csi2rx->source_subdev = s_subdev;
> +
> +	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
> +		csi2rx->source_pad);
> +
> +	return media_create_pad_link(&csi2rx->source_subdev->entity,
> +				     csi2rx->source_pad,
> +				     &csi2rx->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static const struct v4l2_async_notifier_operations csi2rx_notifier_ops = {
> +	.bound		= csi2rx_async_bound,
> +};
> +
> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	unsigned char i;
> +	u32 reg;

How about naming the variable dev_cfg to be a bit more explicit ?

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(csi2rx->base))
> +		return PTR_ERR(csi2rx->base);
> +
> +	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> +	if (IS_ERR(csi2rx->sys_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get sys clock\n");
> +		return PTR_ERR(csi2rx->sys_clk);
> +	}
> +
> +	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> +	if (IS_ERR(csi2rx->p_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get P clock\n");
> +		return PTR_ERR(csi2rx->p_clk);
> +	}
> +
> +	csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
> +	if (IS_ERR(csi2rx->dphy)) {
> +		dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
> +		return PTR_ERR(csi2rx->dphy);
> +	}
> +
> +	/*
> +	 * FIXME: Once we'll have external D-PHY support, the check
> +	 * will need to be removed.
> +	 */
> +	if (csi2rx->dphy) {
> +		dev_err(&pdev->dev, "External D-PHY not supported yet\n");
> +		return -EINVAL;
> +	}
> +
> +	clk_prepare_enable(csi2rx->p_clk);
> +	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	csi2rx->max_lanes = (reg & 7);

No need for outer parentheses.

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

Same here.

> +	if (csi2rx->max_streams > CSI2RX_STREAMS_MAX) {
> +		dev_err(&pdev->dev, "Invalid number of streams: %u\n",
> +			csi2rx->max_streams);
> +		return -EINVAL;
> +	}
> +
> +	csi2rx->has_internal_dphy = (reg & BIT(3)) ? true : false;

Same here.

> +
> +	/*
> +	 * FIXME: Once we'll have internal D-PHY support, the check
> +	 * will need to be removed.
> +	 */
> +	if (csi2rx->has_internal_dphy) {
> +		dev_err(&pdev->dev, "Internal D-PHY not supported yet\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		char clk_name[16];
> +
> +		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> +		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> +		if (IS_ERR(csi2rx->pixel_clk[i])) {
> +			dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
> +			return PTR_ERR(csi2rx->pixel_clk[i]);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> +{
> +	struct v4l2_fwnode_endpoint v4l2_ep;
> +	struct fwnode_handle *fwh;
> +	struct device_node *ep;
> +	int ret;
> +
> +	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	fwh = of_fwnode_handle(ep);
> +	ret = v4l2_fwnode_endpoint_parse(fwh, &v4l2_ep);
> +	if (ret) {
> +		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> +		of_node_put(ep);
> +		return ret;
> +	}
> +
> +	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> +			v4l2_ep.bus_type);
> +		of_node_put(ep);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(csi2rx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes,
> +	       sizeof(csi2rx->lanes));
> +	csi2rx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> +	if (csi2rx->num_lanes > csi2rx->max_lanes) {
> +		dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
> +			csi2rx->num_lanes);
> +		of_node_put(ep);
> +		return -EINVAL;
> +	}
> +
> +	csi2rx->asd.match.fwnode.fwnode =
> fwnode_graph_get_remote_port_parent(fwh);
> +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	of_node_put(ep);
> +
> +	csi2rx->notifier.subdevs = devm_kzalloc(csi2rx->dev,
> +						sizeof(*csi2rx->notifier.subdevs),
> +						GFP_KERNEL);
> +	if (!csi2rx->notifier.subdevs)
> +		return -ENOMEM;
> +
> +	csi2rx->notifier.subdevs[0] = &csi2rx->asd;
> +	csi2rx->notifier.num_subdevs = 1;
> +	csi2rx->notifier.ops = &csi2rx_notifier_ops;
> +
> +	return v4l2_async_subdev_notifier_register(&csi2rx->subdev,
> +						   &csi2rx->notifier);
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> +	struct csi2rx_priv *csi2rx;
> +	unsigned int i;
> +	int ret;
> +
> +	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
> +	if (!csi2rx)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, csi2rx);
> +	csi2rx->dev = &pdev->dev;
> +
> +	ret = csi2rx_get_resources(csi2rx, pdev);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	ret = csi2rx_parse_dt(csi2rx);
> +	if (ret)
> +		goto err_free_priv;
> +
> +	csi2rx->subdev.owner = THIS_MODULE;
> +	csi2rx->subdev.dev = &pdev->dev;
> +	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
> +	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
> +	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> +	/* Create our media pads */
> +	csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;

MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER is documented as

       -  Video pixel formatter. An entity capable of pixel formatting
          must have at least one sink pad and one source pad. Read
          pixel formatters read pixels from memory and perform a subset
          of unpacking, cropping, color keying, alpha multiplication
          and pixel encoding conversion. Write pixel formatters perform
          a subset of dithering, pixel encoding conversion and packing
          and write pixels to memory.

I don't think it's a good match in this case. Wouldn't 
MEDIA_ENT_F_VID_IF_BRIDGE be better ?

       - Video interface bridge. A video interface bridge entity must have at
         least one sink pad and at least one source pad. It receives video
         frames on its sink pad from an input video bus of one type (HDMI,
         eDP, MIPI CSI-2, ...), and outputs them on its source pad to an
         output video bus of another type (eDP, MIPI CSI-2, parallel, ...).

(I'm not sure I like the name "bridge" too much in this context, but that's a 
separate issue)

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

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

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

Hi Laurent,

On Thu, Feb 08, 2018 at 08:17:19PM +0200, Laurent Pinchart wrote:
> > +	/*
> > +	 * Create a static mapping between the CSI virtual channels
> > +	 * and the output stream.
> > +	 *
> > +	 * This should be enhanced, but v4l2 lacks the support for
> > +	 * changing that mapping dynamically.
> > +	 *
> > +	 * We also cannot enable and disable independant streams here,
> > +	 * hence the reference counting.
> > +	 */
> 
> If you start all streams in one go, will s_stream(1) be called multiple times 
> ? If not, you could possibly skip the whole reference counting and avoid 
> locking.

I guess that while we should expect the CSI-2 bus to be always
enabled, the downstream camera interface could be shutdown
independently, so I guess s_stream would be called each time one is
brought up or brought down?

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 v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

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

Hi Laurent,

On Thu, Feb 08, 2018 at 08:17:19PM +0200, Laurent Pinchart wrote:
> > +	/*
> > +	 * Create a static mapping between the CSI virtual channels
> > +	 * and the output stream.
> > +	 *
> > +	 * This should be enhanced, but v4l2 lacks the support for
> > +	 * changing that mapping dynamically.
> > +	 *
> > +	 * We also cannot enable and disable independant streams here,
> > +	 * hence the reference counting.
> > +	 */
> 
> If you start all streams in one go, will s_stream(1) be called multiple times 
> ? If not, you could possibly skip the whole reference counting and avoid 
> locking.

I guess that while we should expect the CSI-2 bus to be always
enabled, the downstream camera interface could be shutdown
independently, so I guess s_stream would be called each time one is
brought up or brought down?

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 v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Laurent Pinchart]

Hi Maxime,

On Wednesday, 14 February 2018 15:19:33 EET Maxime Ripard wrote:
> On Thu, Feb 08, 2018 at 08:17:19PM +0200, Laurent Pinchart wrote:
> >> +	/*
> >> +	 * Create a static mapping between the CSI virtual channels
> >> +	 * and the output stream.
> >> +	 *
> >> +	 * This should be enhanced, but v4l2 lacks the support for
> >> +	 * changing that mapping dynamically.
> >> +	 *
> >> +	 * We also cannot enable and disable independant streams here,
> >> +	 * hence the reference counting.
> >> +	 */
> > 
> > If you start all streams in one go, will s_stream(1) be called multiple
> > times ? If not, you could possibly skip the whole reference counting and
> > avoid locking.
> 
> I guess that while we should expect the CSI-2 bus to be always
> enabled, the downstream camera interface could be shutdown
> independently, so I guess s_stream would be called each time one is
> brought up or brought down?

That's the idea. However, we don't have support for multiplexed streams in 
mainline yet, so there's no way it can be implemented today in your driver.

-- 
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 v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Laurent Pinchart]

Hi Maxime,

On Wednesday, 14 February 2018 15:19:33 EET Maxime Ripard wrote:
> On Thu, Feb 08, 2018 at 08:17:19PM +0200, Laurent Pinchart wrote:
> >> +	/*
> >> +	 * Create a static mapping between the CSI virtual channels
> >> +	 * and the output stream.
> >> +	 *
> >> +	 * This should be enhanced, but v4l2 lacks the support for
> >> +	 * changing that mapping dynamically.
> >> +	 *
> >> +	 * We also cannot enable and disable independant streams here,
> >> +	 * hence the reference counting.
> >> +	 */
> > 
> > If you start all streams in one go, will s_stream(1) be called multiple
> > times ? If not, you could possibly skip the whole reference counting and
> > avoid locking.
> 
> I guess that while we should expect the CSI-2 bus to be always
> enabled, the downstream camera interface could be shutdown
> independently, so I guess s_stream would be called each time one is
> brought up or brought down?

That's the idea. However, we don't have support for multiplexed streams in 
mainline yet, so there's no way it can be implemented today in your driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

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

On Wed, Feb 14, 2018 at 05:03:26PM +0200, Laurent Pinchart wrote:
> On Wednesday, 14 February 2018 15:19:33 EET Maxime Ripard wrote:
> > On Thu, Feb 08, 2018 at 08:17:19PM +0200, Laurent Pinchart wrote:
> > >> +	/*
> > >> +	 * Create a static mapping between the CSI virtual channels
> > >> +	 * and the output stream.
> > >> +	 *
> > >> +	 * This should be enhanced, but v4l2 lacks the support for
> > >> +	 * changing that mapping dynamically.
> > >> +	 *
> > >> +	 * We also cannot enable and disable independant streams here,
> > >> +	 * hence the reference counting.
> > >> +	 */
> > > 
> > > If you start all streams in one go, will s_stream(1) be called multiple
> > > times ? If not, you could possibly skip the whole reference counting and
> > > avoid locking.
> > 
> > I guess that while we should expect the CSI-2 bus to be always
> > enabled, the downstream camera interface could be shutdown
> > independently, so I guess s_stream would be called each time one is
> > brought up or brought down?
> 
> That's the idea. However, we don't have support for multiplexed streams in 
> mainline yet, so there's no way it can be implemented today in your driver.

Ok, but we definitely plan on supporting that soon, so I guess that
wouldn't too much of a burden to keep it there.

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