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

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

[Maxime Ripard]

Hi,

Here is an 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.

This serie depends on the version 5 of the serie "v4l2-async: add subnotifier
registration for subdevices" from Niklas Söderlund.

Let me know what you think!
Maxime

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      |  98 ++++
 drivers/media/platform/Kconfig                     |   1 +
 drivers/media/platform/Makefile                    |   2 +
 drivers/media/platform/cadence/Kconfig             |  12 +
 drivers/media/platform/cadence/Makefile            |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c       | 494 +++++++++++++++++++++
 6 files changed, 608 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.13.5

--
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 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org>
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/media/cdns-csi2rx.txt      | 98 ++++++++++++++++++++++
 1 file changed, 98 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..2395030d8c72
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
@@ -0,0 +1,98 @@
+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 endpoint definitions as defined in
+           Documentation/devicetree/bindings/media/video-interfaces.txt. The
+           first port subnode should be the input endpoint, the next ones the
+           output, one for each stream supported by the CSI2-RX controller.
+           The ports ID must be the stream output number used in the
+           implementation, plus 1.
+
+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 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+
+			csi2rx_in_sensor: endpoint {
+				remote-endpoint = <&sensor_out_csi2rx>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
+			};
+		};
+
+		port@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			csi2rx_out_grabber0: endpoint {
+				remote-endpoint = <&grabber0_in_csi2rx>;
+			};
+		};
+
+		port@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+
+			csi2rx_out_grabber1: endpoint {
+				remote-endpoint = <&grabber1_in_csi2rx>;
+			};
+		};
+
+		port@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+
+			csi2rx_out_grabber2: endpoint {
+				remote-endpoint = <&grabber2_in_csi2rx>;
+			};
+		};
+
+		port@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+
+			csi2rx_out_grabber3: endpoint {
+				remote-endpoint = <&grabber3_in_csi2rx>;
+			};
+		};
+	};
+};
-- 
2.13.5

--
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 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings

[Sakari Ailus]

Hi Maxime,

On Mon, Sep 04, 2017 at 03:03:34PM +0200, Maxime Ripard wrote:
> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Acked-by: Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/media/cdns-csi2rx.txt      | 98 ++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt

Naming this according to the compatible string would make it easier to
find. The same pattern is used by a number of existing binding files.

Up to you.

> 
> diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> new file mode 100644
> index 000000000000..2395030d8c72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> @@ -0,0 +1,98 @@
> +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 endpoint definitions as defined in
> +           Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +           first port subnode should be the input endpoint, the next ones the
> +           output, one for each stream supported by the CSI2-RX controller.

While I guess the DT compiler won't rearrange the nodes, it'd be better to
define the port numbers explicitly, i.e. that input is number 0.

> +           The ports ID must be the stream output number used in the
> +           implementation, plus 1.

And also that outputs are from 1 to 4.

With that,

Acked-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org
--
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 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Monday, 4 September 2017 16:03:34 EEST Maxime Ripard wrote:
> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Acked-by: Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/media/cdns-csi2rx.txt      | 98 +++++++++++++++++++
>  1 file changed, 98 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..2395030d8c72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> @@ -0,0 +1,98 @@
> +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 endpoint definitions as defined in
> +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> The
> +           first port subnode should be the input endpoint, the next ones
> the
> +           output, one for each stream supported by the CSI2-RX controller.
> +           The ports ID must be the stream output number used in the
> +           implementation, plus 1.

This sounds a bit unclear to me. How about the following ?

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

> +
> +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 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;

Those two properties are not needed as the endpoint isn't numbered. Same for 
all other ports below.

With this fixed and the ports node description clarified,

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> +			reg = <0>;
> +
> +			csi2rx_in_sensor: endpoint {
> +				remote-endpoint = <&sensor_out_csi2rx>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
> +			};
> +		};
> +
> +		port@1 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <1>;
> +
> +			csi2rx_out_grabber0: endpoint {
> +				remote-endpoint = <&grabber0_in_csi2rx>;
> +			};
> +		};
> +
> +		port@2 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <2>;
> +
> +			csi2rx_out_grabber1: endpoint {
> +				remote-endpoint = <&grabber1_in_csi2rx>;
> +			};
> +		};
> +
> +		port@3 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <3>;
> +
> +			csi2rx_out_grabber2: endpoint {
> +				remote-endpoint = <&grabber2_in_csi2rx>;
> +			};
> +		};
> +
> +		port@4 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <4>;
> +
> +			csi2rx_out_grabber3: endpoint {
> +				remote-endpoint = <&grabber3_in_csi2rx>;
> +			};
> +		};
> +	};
> +};


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

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cadence/Kconfig       |  12 +
 drivers/media/platform/cadence/Makefile      |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c | 494 +++++++++++++++++++++++++++
 5 files changed, 510 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 1313cd533436..a79d96e9b723 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 9beadc760467..1d31eb51e9bb 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -2,6 +2,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..d1b6bbb6a0eb
--- /dev/null
+++ b/drivers/media/platform/cadence/Kconfig
@@ -0,0 +1,12 @@
+config VIDEO_CADENCE
+	bool "Cadence Video Devices"
+
+if VIDEO_CADENCE
+
+config VIDEO_CADENCE_CSI2RX
+	tristate "Cadence MIPI-CSI2 RX Controller v1.3"
+	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
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..e662b1890bba
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,494 @@
+/*
+ * Driver for Cadence MIPI-CSI2 RX Controller v1.3
+ *
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#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_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[4];
+	u8				num_lanes;
+	u8				max_lanes;
+	u8				max_streams;
+	bool				has_internal_dphy;
+
+	struct v4l2_subdev		subdev;
+	struct media_pad		pads[CSI2RX_PAD_MAX];
+
+	/* Remote source */
+	struct v4l2_async_subdev	asd;
+	struct device_node		*source_node;
+	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;
+	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);
+
+	printk("%s %d\n", __func__, __LINE__);
+
+	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]);
+
+	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;
+
+	printk("%s %d\n", __func__, __LINE__);
+
+	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);
+
+	if (enable)
+		csi2rx_start(csi2rx);
+	else
+		csi2rx_stop(csi2rx);
+
+	return 0;
+}
+
+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 = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	csi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
+							 &csi2rx->source_node->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 0;
+}
+
+static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	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 void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
+				struct v4l2_subdev *s_subdev,
+				struct v4l2_async_subdev *asd)
+{
+	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
+		csi2rx->source_pad);
+
+	csi2rx->source_subdev = NULL;
+	csi2rx->source_pad = -EINVAL;
+}
+
+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 > 4) {
+		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 device_node *ep, *remote;
+	int ret;
+
+	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
+	if (ret) {
+		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
+		goto out;
+	}
+
+	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
+		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
+			v4l2_ep.bus_type);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	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);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	remote = of_graph_get_remote_port_parent(ep);
+	if (!remote) {
+		dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
+
+	csi2rx->source_node = remote;
+	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
+	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+
+out:
+	of_node_put(ep);
+	return ret;
+}
+
+static int csi2rx_probe(struct platform_device *pdev)
+{
+	struct v4l2_async_subdev **subdevs;
+	struct csi2rx_priv *csi2rx;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Since the v4l2_subdev structure is embedded in our
+	 * csi2rx_priv structure, and that the structure is exposed to
+	 * the user-space, we cannot just use the devm_variant
+	 * here. Indeed, that would lead to a use-after-free in a
+	 * open() - unbind - close() pattern.
+	 */
+	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;
+
+	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
+	if (!subdevs) {
+		ret = -ENOMEM;
+		goto err_free_priv;
+	}
+	subdevs[0] = &csi2rx->asd;
+
+	ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
+						  csi2rx_async_bound,
+						  csi2rx_async_complete,
+						  csi2rx_async_unbind);
+	if (ret < 0) {
+		dev_err(csi2rx->dev, "Failed to register our notifier\n");
+		goto err_free_priv;
+	}
+
+	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.13.5

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

[Benoit Parrot]

Maxime,

Thanks for the patch.

Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Mon [2017-Sep-04 15:03:35 +0200]:
> 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cadence/Kconfig       |  12 +
>  drivers/media/platform/cadence/Makefile      |   1 +
>  drivers/media/platform/cadence/cdns-csi2rx.c | 494 +++++++++++++++++++++++++++
>  5 files changed, 510 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
>

<snip>

> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> new file mode 100644
> index 000000000000..e662b1890bba
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,494 @@
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#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_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[4];
> +	u8				num_lanes;
> +	u8				max_lanes;
> +	u8				max_streams;
> +	bool				has_internal_dphy;
> +
> +	struct v4l2_subdev		subdev;
> +	struct media_pad		pads[CSI2RX_PAD_MAX];
> +
> +	/* Remote source */
> +	struct v4l2_async_subdev	asd;
> +	struct device_node		*source_node;
> +	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;
> +	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);
> +
> +	printk("%s %d\n", __func__, __LINE__);

Some left over debug...

> +
> +	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]);
> +
> +	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;
> +
> +	printk("%s %d\n", __func__, __LINE__);

Same here... dev_dbg perhaps? 

> +
> +	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);
> +
> +	if (enable)
> +		csi2rx_start(csi2rx);
> +	else
> +		csi2rx_stop(csi2rx);
> +
> +	return 0;

Since we added the error checking in both csi2rx_start/csi2rx_stop
we should also propagate the error as well.
Something like:

	if (enable)
		return csi2rx_start(csi2rx);

	return csi2rx_stop(csi2rx);
...

> +}
> +
> +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 = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	csi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> +							 &csi2rx->source_node->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 0;
> +}
> +
> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	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 void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
> +				struct v4l2_subdev *s_subdev,
> +				struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
> +		csi2rx->source_pad);
> +
> +	csi2rx->source_subdev = NULL;
> +	csi2rx->source_pad = -EINVAL;
> +}
> +
> +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 > 4) {

Should use a macro here.

> +		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 device_node *ep, *remote;
> +	int ret;
> +
> +	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> +	if (ret) {
> +		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> +		goto out;
> +	}
> +
> +	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> +			v4l2_ep.bus_type);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	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);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(ep);
> +	if (!remote) {
> +		dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
> +
> +	csi2rx->source_node = remote;
> +	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +
> +out:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> +	struct v4l2_async_subdev **subdevs;
> +	struct csi2rx_priv *csi2rx;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Since the v4l2_subdev structure is embedded in our
> +	 * csi2rx_priv structure, and that the structure is exposed to
> +	 * the user-space, we cannot just use the devm_variant
> +	 * here. Indeed, that would lead to a use-after-free in a
> +	 * open() - unbind - close() pattern.
> +	 */
> +	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;
> +
> +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> +	if (!subdevs) {
> +		ret = -ENOMEM;
> +		goto err_free_priv;
> +	}
> +	subdevs[0] = &csi2rx->asd;
> +

Shouldn't the comment related to lifetime of memory allocation be also applied here?
A reference to the "subdevs" pointer is taken internally so it might suffer the same fate.
Not sure how many "struct v4l2_async_subdev **subdevs" we would end up needing
but since here we are only dealing with one, why not just make it a member of the
struct csi2rx_priv object.

> +	ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
> +						  csi2rx_async_bound,
> +						  csi2rx_async_complete,
> +						  csi2rx_async_unbind);
> +	if (ret < 0) {
> +		dev_err(csi2rx->dev, "Failed to register our notifier\n");
> +		goto err_free_priv;
> +	}
> +
> +	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.13.5
> 

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

[Benoit Parrot]

Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org> wrote on Tue [2017-Sep-12 13:23:39 -0500]:

<snip>

> > +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > +{
> > +	unsigned int i;
> > +	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);
> > +
> > +	printk("%s %d\n", __func__, __LINE__);
> 
> Some left over debug...
> 
> > +
> > +	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]);
> > +
> > +	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));

I see here that we are setting the data_type to 0 (as we are not setting
it) so effectively capturing everything on the channel(s).
Will we be adding a method to select/filter specific data type?
For instance if we only want to grab YUV data in one stream and only
RGB24 in another. Of course that would not be possible here as is...

Benoit

> > +
> > +		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;
> > +}

<snip>
--
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 RX driver

[Maxime Ripard]

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

On Tue, Sep 12, 2017 at 02:13:12PM -0500, Benoit Parrot 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.
> > > +	 */
> > > +	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));
> 
> I see here that we are setting the data_type to 0 (as we are not
> setting it) so effectively capturing everything on the channel(s).
> Will we be adding a method to select/filter specific data type?  For
> instance if we only want to grab YUV data in one stream and only
> RGB24 in another. Of course that would not be possible here as is...

Ah, right, I forgot about that. I've actually started that discussion
on another thread for a transceiver, without much success though:
https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg117920.html

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

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

[Maxime Ripard]

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

Hi Benoit,

Thanks for your comments,

On Tue, Sep 12, 2017 at 01:23:39PM -0500, Benoit Parrot wrote:
> > +static int csi2rx_probe(struct platform_device *pdev)
> > +{
> > +	struct v4l2_async_subdev **subdevs;
> > +	struct csi2rx_priv *csi2rx;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/*
> > +	 * Since the v4l2_subdev structure is embedded in our
> > +	 * csi2rx_priv structure, and that the structure is exposed to
> > +	 * the user-space, we cannot just use the devm_variant
> > +	 * here. Indeed, that would lead to a use-after-free in a
> > +	 * open() - unbind - close() pattern.
> > +	 */
> > +	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
> > +	if (!csi2rx)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, csi2rx);
> > +	csi2rx->dev = &pdev->dev;

[snip]

> > +
> > +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> > +	if (!subdevs) {
> > +		ret = -ENOMEM;
> > +		goto err_free_priv;
> > +	}
> > +	subdevs[0] = &csi2rx->asd;
> > +
> 
> Shouldn't the comment related to lifetime of memory allocation be
> also applied here?  A reference to the "subdevs" pointer is taken
> internally so it might suffer the same fate.  Not sure how many
> "struct v4l2_async_subdev **subdevs" we would end up needing but
> since here we are only dealing with one, why not just make it a
> member of the struct csi2rx_priv object.

As far as I know, only the notifier will use that array. The notifier
will be removed before that array is de-allocated, and the user-space
never has access to it, so I'm not sure the same issue arises here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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