All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers
@ 2013-01-23 19:31 Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation Sylwester Nawrocki
                   ` (13 more replies)
  0 siblings, 14 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This series includes the updated device tree bindings documentation and
the V4L2 OF parser (v4). There were just couple minor changes since v3:
     - improved clock-lanes property description,
     - grammar corrections of the example dts snippet description,
     - minor comments corrections.

This series also includes patches adding device tree support for Exynos4
SoC camera subsystem. Changes in this part include:
 - dropped patch adding OF_DEV_AUXDATA entries as this series has
   been tested with the new Exynos4 clocks driver;
 - max-data-lanes property of the CSIS device node replaced with bus-width;
 - added clock-frequency property to FIMC device nodes;
 - corrected FIMC-LITE devices registration (the code didn't consider
   they are child nodes of the fimc-is node);
 - removed the "inactive" camera port pinctrl state, it is now optional
   and handling of it wasn't implemeted at the driver yet anyway;

I have also tested this patch set with the FIMC-IS (the camera ISP
subsystem with a dedicated ARM MCU) driver.

For the media patches 01..09/14 I intend to send a pull request within
a few days. Patches 10..13/14 I'd like to be applied by the Samsung
platforms maintainer.

Patch 14/14 is an example only.

Thank you for all reviews!

Guennadi Liakhovetski (2):
  [media] Add common video interfaces OF bindings documentation
  [media] Add a V4L2 OF parser

Sylwester Nawrocki (12):
  s5p-csis: Add device tree support
  s5p-fimc: Add device tree support for FIMC devices
  s5p-fimc: Add device tree support for FIMC-LITE devices
  s5p-fimc: Change platform subdevs registration method
  s5p-fimc: Add device tree support for the main media device driver
  s5p-fimc: Add device tree based sensors registration
  s5p-fimc: Use pinctrl API for camera ports configuration
  ARM: dts: Add camera to node exynos4.dtsi
  ARM: dts: Add ISP power domain node for Exynos4x12
  ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
  ARM: dts: Add camera pinctrl nodes for Exynos4x12 SoCs
  ARM: dts: Add camera device nodes nodes for PQ board

 .../devicetree/bindings/media/soc/samsung-fimc.txt |  184 +++++++
 .../bindings/media/soc/samsung-mipi-csis.txt       |   82 +++
 .../devicetree/bindings/media/video-interfaces.txt |  204 ++++++++
 arch/arm/boot/dts/exynos4.dtsi                     |   64 +++
 arch/arm/boot/dts/exynos4412-slp_pq.dts            |  169 +++++++
 arch/arm/boot/dts/exynos4x12-pinctrl.dtsi          |   33 +-
 arch/arm/boot/dts/exynos4x12.dtsi                  |   52 ++
 drivers/media/platform/s5p-fimc/fimc-capture.c     |    2 +-
 drivers/media/platform/s5p-fimc/fimc-core.c        |   94 ++--
 drivers/media/platform/s5p-fimc/fimc-lite.c        |   65 ++-
 drivers/media/platform/s5p-fimc/fimc-mdevice.c     |  526 +++++++++++++++-----
 drivers/media/platform/s5p-fimc/fimc-mdevice.h     |    6 +
 drivers/media/platform/s5p-fimc/mipi-csis.c        |  158 ++++--
 drivers/media/platform/s5p-fimc/mipi-csis.h        |    1 +
 drivers/media/v4l2-core/Makefile                   |    3 +
 drivers/media/v4l2-core/v4l2-of.c                  |  253 ++++++++++
 include/media/s5p_fimc.h                           |   17 +
 include/media/v4l2-of.h                            |   79 +++
 18 files changed, 1771 insertions(+), 221 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
 create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
 create mode 100644 Documentation/devicetree/bindings/media/video-interfaces.txt
 create mode 100644 drivers/media/v4l2-core/v4l2-of.c
 create mode 100644 include/media/v4l2-of.h

--
1.7.9.5

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-24 10:16   ` Laurent Pinchart
  2013-01-23 19:31 ` [PATCH RFC v4 02/14] [media] Add a V4L2 OF parser Sylwester Nawrocki
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

This patch adds a document describing common OF bindings for video
capture, output and video processing devices. It is curently mainly
focused on video capture devices, with data busses defined by
standards like ITU-R BT.656 or MIPI-CSI2.
It also documents a method of describing data links between devices.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Reviewed-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Rob Herring <rob.herring@calxeda.com>
---

Changes since v3:
 - improved clock-lanes property description,
 - grammar corrections of the example dts snippet description.
---
 .../devicetree/bindings/media/video-interfaces.txt |  204 ++++++++++++++++++++
 1 file changed, 204 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/video-interfaces.txt

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
new file mode 100644
index 0000000..0da126f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -0,0 +1,204 @@
+Common bindings for video data receiver and transmitter interfaces
+
+General concept
+---------------
+
+Video data pipelines usually consist of external devices, e.g. camera sensors,
+controlled over an I2C, SPI or UART bus, and SoC internal IP blocks, including
+video DMA engines and video data processors.
+
+SoC internal blocks are described by DT nodes, placed similarly to other SoC
+blocks.  External devices are represented as child nodes of their respective
+bus controller nodes, e.g. I2C.
+
+Data interfaces on all video devices are described by their child 'port' nodes.
+Configuration of a port depends on other devices participating in the data
+transfer and is described by 'endpoint' subnodes.
+
+dev {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	port@0 {
+		endpoint@0 { ... };
+		endpoint@1 { ... };
+	};
+	port@1 { ... };
+};
+
+If a port can be configured to work with more than one other device on the same
+bus, an 'endpoint' child node must be provided for each of them.  If more than
+one port is present in a device node or there is more than one endpoint at a
+port, a common scheme, using '#address-cells', '#size-cells' and 'reg' properties
+is used.
+
+Two 'endpoint' nodes are linked with each other through their 'remote-endpoint'
+phandles.  An endpoint subnode of a device contains all properties needed for
+configuration of this device for data exchange with the other device.  In most
+cases properties at the peer 'endpoint' nodes will be identical, however
+they might need to be different when there is any signal modifications on the
+bus between two devices, e.g. there are logic signal inverters on the lines.
+
+Required properties
+-------------------
+
+If there is more than one 'port' or more than one 'endpoint' node following
+properties are required in relevant parent node:
+
+- #address-cells : number of cells required to define port number, should be 1.
+- #size-cells    : should be zero.
+
+Optional endpoint properties
+----------------------------
+
+- remote-endpoint: phandle to an 'endpoint' subnode of the other device node.
+- slave-mode: a boolean property, run the link in slave mode. Default is master
+  mode.
+- bus-width: number of data lines, valid for parallel busses.
+- data-shift: on parallel data busses, if bus-width is used to specify the
+  number of data lines, data-shift can be used to specify which data lines are
+  used, e.g. "bus-width=<10>; data-shift=<2>;" means, that lines 9:2 are used.
+- hsync-active: active state of HSYNC signal, 0/1 for LOW/HIGH respectively.
+- vsync-active: active state of VSYNC signal, 0/1 for LOW/HIGH respectively.
+  Note, that if HSYNC and VSYNC polarities are not specified, embedded
+  synchronization may be required, where supported.
+- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
+- field-even-active: field signal level during the even field data transmission.
+- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
+  signal.
+- data-lanes: an array of physical data lane indexes. Position of an entry
+  determines the logical lane number, while the value of an entry indicates
+  physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
+  "data-lanes = <1>, <2>;", assuming the clock lane is on hardware lane 0.
+  This property is valid for serial busses only (e.g. MIPI CSI-2).
+- clock-lanes: an array of physical clock lane indexes. Position of an entry
+  determines the logical lane number, while the value of an entry indicates
+  physical lane, e.g. for a MIPI CSI-2 bus we could have "clock-lanes = <0>;",
+  which places the clock lane on hardware lane 0. This property is valid for
+  serial busses only (e.g. MIPI CSI-2). Note that for the MIPI CSI-2 bus this
+  array contains only one entry.
+- clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
+  clock mode.
+
+Example
+-------
+
+The example snippet below describes two data pipelines.  ov772x and imx074 are
+camera sensors with a parallel and serial (MIPI CSI-2) video bus respectively.
+Both sensors are on the I2C control bus corresponding to the i2c0 controller
+node.  ov772x sensor is linked directly to the ceu0 video host interface.
+imx074 is linked to ceu0 through the MIPI CSI-2 receiver (csi2). ceu0 has a
+(single) DMA engine writing captured data to memory.  ceu0 node has a single
+'port' node which indicates that at any time only one of the following data
+pipelines can be active: ov772x -> ceu0 or imx074 -> csi2 -> ceu0.
+
+	ceu0: ceu@0xfe910000 {
+		compatible = "renesas,sh-mobile-ceu";
+		reg = <0xfe910000 0xa0>;
+		interrupts = <0x880>;
+
+		mclk: master_clock {
+			compatible = "renesas,ceu-clock";
+			#clock-cells = <1>;
+			clock-frequency = <50000000>;	/* Max clock frequency */
+			clock-output-names = "mclk";
+		};
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			ceu0_1: endpoint@1 {
+				reg = <1>;		/* Local endpoint # */
+				remote = <&ov772x_1_1>;	/* Remote phandle */
+				bus-width = <8>;	/* Used data lines */
+				data-shift = <0>;	/* Lines 7:0 are used */
+
+				/* If hsync-active/vsync-active are missing,
+				   embedded bt.605 sync is used */
+				hsync-active = <1>;	/* Active high */
+				vsync-active = <1>;	/* Active high */
+				data-active = <1>;	/* Active high */
+				pclk-sample = <1>;	/* Rising */
+			};
+
+			ceu0_0: endpoint@0 {
+				reg = <0>;
+				remote = <&csi2_2>;
+				immutable;
+			};
+		};
+	};
+
+	i2c0: i2c@0xfff20000 {
+		...
+		ov772x_1: camera@0x21 {
+			compatible = "omnivision,ov772x";
+			reg = <0x21>;
+			vddio-supply = <&regulator1>;
+			vddcore-supply = <&regulator2>;
+
+			clock-frequency = <20000000>;
+			clocks = <&mclk 0>;
+			clock-names = "xclk";
+
+			port {
+				/* With 1 endpoint per port no need in addresses. */
+				ov772x_1_1: endpoint {
+					bus-width = <8>;
+					remote-endpoint = <&ceu0_1>;
+					hsync-active = <1>;
+					vsync-active = <0>; /* Who came up with an
+							       inverter here ?... */
+					data-active = <1>;
+					pclk-sample = <1>;
+				};
+			};
+		};
+
+		imx074: camera@0x1a {
+			compatible = "sony,imx074";
+			reg = <0x1a>;
+			vddio-supply = <&regulator1>;
+			vddcore-supply = <&regulator2>;
+
+			clock-frequency = <30000000>;	/* Shared clock with ov772x_1 */
+			clocks = <&mclk 0>;
+			clock-names = "sysclk";		/* Assuming this is the
+							   name in the datasheet */
+			port {
+				imx074_1: endpoint {
+					clock-lanes = <0>;
+					data-lanes = <1>, <2>;
+					remote-endpoint = <&csi2_1>;
+				};
+			};
+		};
+	};
+
+	csi2: csi2@0xffc90000 {
+		compatible = "renesas,sh-mobile-csi2";
+		reg = <0xffc90000 0x1000>;
+		interrupts = <0x17a0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@1 {
+			compatible = "renesas,csi2c";	/* One of CSI2I and CSI2C. */
+			reg = <1>;			/* CSI-2 PHY #1 of 2: PHY_S,
+							   PHY_M has port address 0,
+							   is unused. */
+			csi2_1: endpoint {
+				clock-lanes = <0>;
+				data-lanes = <2>, <1>;
+				remote-endpoint = <&imx074_1>;
+			};
+		};
+		port@2 {
+			reg = <2>;			/* port 2: link to the CEU */
+
+			csi2_2: endpoint {
+				immutable;
+				remote-endpoint = <&ceu0_0>;
+			};
+		};
+	};
--
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v4 02/14] [media] Add a V4L2 OF parser
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 03/14] s5p-csis: Add device tree support Sylwester Nawrocki
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Add a V4L2 OF parser, implementing bindings documented in
Documentation/devicetree/bindings/media/video-interfaces.txt.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
[s.nawrocki@samsung.com: various corrections and improvements
since the initial version]
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---

Changes since v3:
 - minor corrections of the comments,
 - replaced parent->name with parent->full_name.
---
 drivers/media/v4l2-core/Makefile  |    3 +
 drivers/media/v4l2-core/v4l2-of.c |  253 +++++++++++++++++++++++++++++++++++++
 include/media/v4l2-of.h           |   79 ++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/media/v4l2-core/v4l2-of.c
 create mode 100644 include/media/v4l2-of.h

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index c2d61d4..00f64d6 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -9,6 +9,9 @@ videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
 ifeq ($(CONFIG_COMPAT),y)
   videodev-objs += v4l2-compat-ioctl32.o
 endif
+ifeq ($(CONFIG_OF),y)
+  videodev-objs += v4l2-of.o
+endif

 obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
new file mode 100644
index 0000000..15d4396
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -0,0 +1,253 @@
+/*
+ * V4L2 OF binding parsing library
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include <media/v4l2-of.h>
+
+/**
+ * v4l2_of_parse_mipi_csi2() - parse MIPI CSI-2 bus properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to v4l2_of_endpoint data structure
+ *
+ * Return: 0 on success or negative error value otherwise.
+ */
+int v4l2_of_parse_mipi_csi2(const struct device_node *node,
+			    struct v4l2_of_endpoint *endpoint)
+{
+	struct v4l2_of_mipi_csi2 *mipi_csi2 = &endpoint->mipi_csi_2;
+	u32 data_lanes[ARRAY_SIZE(mipi_csi2->data_lanes)];
+	struct property *prop;
+	const __be32 *lane = NULL;
+	u32 v;
+	int i = 0;
+
+	prop = of_find_property(node, "data-lanes", NULL);
+	if (!prop)
+		return -EINVAL;
+	do {
+		lane = of_prop_next_u32(prop, lane, &data_lanes[i]);
+	} while (lane && i++ < ARRAY_SIZE(data_lanes));
+
+	mipi_csi2->num_data_lanes = i;
+	while (i--)
+		mipi_csi2->data_lanes[i] = data_lanes[i];
+
+	if (!of_property_read_u32(node, "clock-lanes", &v))
+		mipi_csi2->clock_lane = v;
+
+	if (of_get_property(node, "clock-noncontinuous", &v))
+		endpoint->mbus_flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_of_parse_mipi_csi2);
+
+/**
+ * v4l2_of_parse_parallel_bus() - parse parallel bus properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to v4l2_of_endpoint data structure
+ */
+void v4l2_of_parse_parallel_bus(const struct device_node *node,
+				struct v4l2_of_endpoint *endpoint)
+{
+	unsigned int flags = 0;
+	u32 v;
+
+	if (WARN_ON(!endpoint))
+		return;
+
+	if (!of_property_read_u32(node, "hsync-active", &v))
+		flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH :
+			V4L2_MBUS_HSYNC_ACTIVE_LOW;
+
+	if (!of_property_read_u32(node, "vsync-active", &v))
+		flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH :
+			V4L2_MBUS_VSYNC_ACTIVE_LOW;
+
+	if (!of_property_read_u32(node, "pclk-sample", &v))
+		flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING :
+			V4L2_MBUS_PCLK_SAMPLE_FALLING;
+
+	if (!of_property_read_u32(node, "field-even-active", &v))
+		flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH :
+			V4L2_MBUS_FIELD_EVEN_LOW;
+	if (flags)
+		endpoint->mbus_type = V4L2_MBUS_PARALLEL;
+	else
+		endpoint->mbus_type = V4L2_MBUS_BT656;
+
+	if (!of_property_read_u32(node, "data-active", &v))
+		flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
+			V4L2_MBUS_DATA_ACTIVE_LOW;
+
+	if (of_get_property(node, "slave-mode", &v))
+		flags |= V4L2_MBUS_SLAVE;
+
+	if (!of_property_read_u32(node, "bus-width", &v))
+		endpoint->parallel.bus_width = v;
+
+	if (!of_property_read_u32(node, "data-shift", &v))
+		endpoint->parallel.data_shift = v;
+
+	endpoint->mbus_flags = flags;
+}
+EXPORT_SYMBOL(v4l2_of_parse_parallel_bus);
+
+/**
+ * v4l2_of_parse_endpoint() - parse all endpoint node properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to v4l2_of_endpoint data structure
+ *
+ * All properties are optional. If none are found, we don't set any flags.
+ * This means the port has a static configuration and no properties have
+ * to be specified explicitly.
+ * If any properties that identify the bus as parallel are found and
+ * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise
+ * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
+ * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
+ * The caller should hold a reference to @node.
+ */
+void v4l2_of_parse_endpoint(const struct device_node *node,
+			    struct v4l2_of_endpoint *endpoint)
+{
+	const struct device_node *port_node = of_get_parent(node);
+	bool data_lanes_present = false;
+
+	memset(endpoint, 0, sizeof(*endpoint));
+
+	endpoint->local_node = node;
+	/*
+	 * It doesn't matter whether the two calls below succeed. If they
+	 * don't then the default value 0 is used.
+	 */
+	of_property_read_u32(port_node, "reg", &endpoint->port);
+	of_property_read_u32(node, "reg", &endpoint->addr);
+
+	v4l2_of_parse_parallel_bus(node, endpoint);
+
+	/* If any parallel bus properties have been found, skip serial ones. */
+	if (endpoint->parallel.bus_width || endpoint->parallel.data_shift ||
+	    endpoint->mbus_flags) {
+		/* Default parallel bus-master. */
+		if (!(endpoint->mbus_flags & V4L2_MBUS_SLAVE))
+			endpoint->mbus_flags |= V4L2_MBUS_MASTER;
+		return;
+	}
+
+	endpoint->mbus_type = V4L2_MBUS_CSI2;
+
+	if (!v4l2_of_parse_mipi_csi2(node, endpoint))
+		data_lanes_present = true;
+
+	if ((endpoint->mipi_csi_2.clock_lane || data_lanes_present) &&
+	    !(endpoint->mbus_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
+		/* Default CSI-2: continuous clock. */
+		endpoint->mbus_flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+	}
+}
+EXPORT_SYMBOL(v4l2_of_parse_endpoint);
+
+/*
+ * Return a refcounted next 'endpoint' device_node. Contrary to the common OF
+ * practice, we do not drop the reference to previous, users have to do it
+ * themselves, when they're done with the node.
+ */
+struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
+					struct device_node *previous)
+{
+	struct device_node *child, *port;
+
+	if (!parent)
+		return NULL;
+
+	if (!previous) {
+		/*
+		 * If this is the first call, we have to find a port within
+		 * this node.
+		 */
+		for_each_child_of_node(parent, port) {
+			if (!of_node_cmp(port->name, "port"))
+				break;
+		}
+		if (port) {
+			/* Found a port, get a link. */
+			child = of_get_next_child(port, NULL);
+			of_node_put(port);
+		} else {
+			child = NULL;
+		}
+		if (!child)
+			pr_err("%s(): Invalid DT: %s has no link children!\n",
+			       __func__, parent->full_name);
+	} else {
+		port = of_get_parent(previous);
+		if (!port)
+			/* Hm, has someone given us the root node?... */
+			return NULL;
+
+		/* Avoid dropping previous refcount to 0. */
+		of_node_get(previous);
+		child = of_get_next_child(port, previous);
+		if (child) {
+			of_node_put(port);
+			return child;
+		}
+
+		/* No more links under this port, try the next one. */
+		do {
+			port = of_get_next_child(parent, port);
+			if (!port)
+				return NULL;
+		} while (of_node_cmp(port->name, "port"));
+
+		/* Pick up the first link on this port. */
+		child = of_get_next_child(port, NULL);
+		of_node_put(port);
+	}
+
+	return child;
+}
+EXPORT_SYMBOL(v4l2_of_get_next_endpoint);
+
+/**
+ * v4l2_of_get_remote_port_parent() - get remote port's parent node
+ * @node: pointer to local endpoint device_node
+ *
+ * Return: Remote device node associated with remote endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *v4l2_of_get_remote_port_parent(
+			       const struct device_node *node)
+{
+	struct device_node *re, *tmp;
+
+	/* Get remote endpoint node. */
+	re = of_parse_phandle(node, "remote-endpoint", 0);
+	if (!re)
+		return NULL;
+
+	/* Remote port. */
+	tmp = of_get_parent(re);
+	of_node_put(re);
+	if (!tmp)
+		return NULL;
+
+	/* Remote device node. */
+	re = of_get_parent(tmp);
+	of_node_put(tmp);
+
+	return re;
+}
+EXPORT_SYMBOL(v4l2_of_get_remote_port_parent);
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
new file mode 100644
index 0000000..1aba3b3
--- /dev/null
+++ b/include/media/v4l2-of.h
@@ -0,0 +1,79 @@
+/*
+ * V4L2 OF binding parsing library
+ *
+ * Copyright (C) 2012 Renesas Electronics Corp.
+ * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef _V4L2_OF_H
+#define _V4L2_OF_H
+
+#include <linux/list.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+
+#include <media/v4l2-mediabus.h>
+
+struct device_node;
+
+struct v4l2_of_mipi_csi2 {
+	unsigned char data_lanes[4];
+	unsigned char clock_lane;
+	unsigned short num_data_lanes;
+};
+
+struct v4l2_of_endpoint {
+	unsigned int port;
+	unsigned int addr;
+	struct list_head head;
+	const struct device_node *local_node;
+	const __be32 *remote;
+	enum v4l2_mbus_type mbus_type;
+	unsigned int mbus_flags;
+	union {
+		struct {
+			unsigned char bus_width;
+			unsigned char data_shift;
+		} parallel;
+		struct v4l2_of_mipi_csi2 mipi_csi_2;
+	};
+};
+
+#ifdef CONFIG_OF
+int v4l2_of_parse_mipi_csi2(const struct device_node *node,
+			    struct v4l2_of_endpoint *endpoint);
+void v4l2_of_parse_parallel_bus(const struct device_node *node,
+				struct v4l2_of_endpoint *endpoint);
+void v4l2_of_parse_endpoint(const struct device_node *node,
+			    struct v4l2_of_endpoint *link);
+struct device_node *v4l2_of_get_next_endpoint(const struct device_node *parent,
+					struct device_node *previous);
+struct device_node *v4l2_of_get_remote_port_parent(
+					const struct device_node *node);
+#else /* CONFIG_OF */
+
+static inline int v4l2_of_parse_endpoint(const struct device_node *node,
+					struct v4l2_of_endpoint *link)
+{
+	return -ENOSYS;
+}
+
+static inline struct device_node *v4l2_of_get_next_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
+static inline struct device_node *v4l2_of_get_remote_endpoint(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_OF */
+
+#endif /* _V4L2_OF_H */
--
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 03/14] s5p-csis: Add device tree support
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v4 02/14] [media] Add a V4L2 OF parser Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 04/14] s5p-fimc: Add device tree support for FIMC devices Sylwester Nawrocki
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
device. This patch support for binding the driver to the MIPI-CSIS
devices instantiated from device tree and for parsing all SoC and
board specific properties.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v3:
 - 'max-data-lanes' property replaced with 'bus-width'
---
 .../bindings/media/soc/samsung-mipi-csis.txt       |   82 ++++++++++
 drivers/media/platform/s5p-fimc/mipi-csis.c        |  158 +++++++++++++++-----
 drivers/media/platform/s5p-fimc/mipi-csis.h        |    1 +
 3 files changed, 205 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
new file mode 100644
index 0000000..00af24a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
@@ -0,0 +1,82 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:
+
+- compatible	  : "samsung,s5pv210-csis" for S5PV210 SoCs,
+		    "samsung,exynos4210-csis" for Exynos4210 and later SoCs;
+- reg		  : physical base address and size of the device memory mapped
+		    registers;
+- interrupts      : should contain MIPI CSIS interrupt; the format of the
+		    interrupt specifier depends on the interrupt controller;
+- bus-width	  : maximum number of data lanes supported (SoC specific);
+- vddio-supply    : MIPI CSIS I/O and PLL voltage supply (e.g. 1.8V);
+- vddcore-supply  : MIPI CSIS Core voltage supply (e.g. 1.1V).
+
+Optional properties:
+
+- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
+		    value when this property is not specified is 166 MHz;
+- samsung,csis-wclk : CSI-2 wrapper clock selection. If this property is present
+		    external clock from CMU will be used, if not bus clock will
+		    be selected.
+
+The device node should contain one 'port' child node with one child 'endpoint'
+node, as outlined in the common media bindings specification. See
+Documentation/devicetree/bindings/media/video-interfaces.txt for details.
+The following are properties specific to these nodes.
+
+port node
+---------
+
+- reg		  : (required) must be 3 for camera C input (CSIS0) or 4 for
+		    camera D input (CSIS1);
+
+endpoint node
+-------------
+
+- data-lanes	  : (required) an array specifying active physical MIPI-CSI2
+		    data input lanes and their mapping to logical lanes; the
+		    array's content is unused, only its length is meaningful;
+
+- samsung,csis-hs-settle : (optional) differential receiver (HS-RX) settle time;
+
+
+Example:
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+/* SoC properties */
+
+	aliases {
+		csis0 = &csis_0;
+	};
+
+	csis_0: csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		bus-width = <4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+/* Board properties */
+
+	csis_0: csis@11880000 {
+		clock-frequency = <166000000>;
+		vddio-supply = <&reg0>;
+		vddcore-supply = <&reg1>;
+		port {
+			reg = <3>; /* 3 - CSIS0, 4 - CSIS1 */
+			csis0_ep: endpoint {
+				remote-endpoint = <...>;
+				data-lanes = <1>, <2>;
+				samsung,csis-hs-settle = <12>;
+			};
+		};
+	};
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
index 9815b67..c25dbc4 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.c
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
@@ -19,12 +19,14 @@
 #include <linux/kernel.h>
 #include <linux/memory.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/videodev2.h>
+#include <media/v4l2-of.h>
 #include <media/v4l2-subdev.h>
 #include <linux/platform_data/mipi-csis.h>
 #include "mipi-csis.h"
@@ -113,6 +115,7 @@ static char *csi_clock_name[] = {
 	[CSIS_CLK_GATE] = "csis",
 };
 #define NUM_CSIS_CLOCKS	ARRAY_SIZE(csi_clock_name)
+#define DEFAULT_SCLK_CSIS_FREQ	166000000UL
 
 static const char * const csis_supply_name[] = {
 	"vddcore",  /* CSIS Core (1.0V, 1.1V or 1.2V) suppply */
@@ -167,6 +170,11 @@ struct csis_pktbuf {
  * @clock: CSIS clocks
  * @irq: requested s5p-mipi-csis irq number
  * @flags: the state variable for power and streaming control
+ * @clock_frequency: device bus clock frequency
+ * @hs_settle: HS-RX settle time
+ * @num_lanes: number of MIPI-CSI data lanes used
+ * @max_num_lanes: maximum number of MIPI-CSI data lanes supported
+ * @wclk_ext: CSI wrapper clock: 0 - bus clock, 1 - external SCLK_CAM
  * @csis_fmt: current CSIS pixel format
  * @format: common media bus format for the source and sink pad
  * @slock: spinlock protecting structure members below
@@ -184,6 +192,13 @@ struct csis_state {
 	struct clk *clock[NUM_CSIS_CLOCKS];
 	int irq;
 	u32 flags;
+
+	u32 clk_frequency;
+	u32 hs_settle;
+	u32 num_lanes;
+	u32 max_num_lanes;
+	u8 wclk_ext;
+
 	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt format;
 
@@ -273,7 +288,6 @@ static void s5pcsis_reset(struct csis_state *state)
 
 static void s5pcsis_system_enable(struct csis_state *state, int on)
 {
-	struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
 	u32 val, mask;
 
 	val = s5pcsis_read(state, S5PCSIS_CTRL);
@@ -286,7 +300,7 @@ static void s5pcsis_system_enable(struct csis_state *state, int on)
 	val = s5pcsis_read(state, S5PCSIS_DPHYCTRL);
 	val &= ~S5PCSIS_DPHYCTRL_ENABLE;
 	if (on) {
-		mask = (1 << (pdata->lanes + 1)) - 1;
+		mask = (1 << (state->num_lanes + 1)) - 1;
 		val |= (mask & S5PCSIS_DPHYCTRL_ENABLE);
 	}
 	s5pcsis_write(state, S5PCSIS_DPHYCTRL, val);
@@ -321,15 +335,14 @@ static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle)
 
 static void s5pcsis_set_params(struct csis_state *state)
 {
-	struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
 	u32 val;
 
 	val = s5pcsis_read(state, S5PCSIS_CONFIG);
-	val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (pdata->lanes - 1);
+	val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (state->num_lanes - 1);
 	s5pcsis_write(state, S5PCSIS_CONFIG, val);
 
 	__s5pcsis_set_format(state);
-	s5pcsis_set_hsync_settle(state, pdata->hs_settle);
+	s5pcsis_set_hsync_settle(state, state->hs_settle);
 
 	val = s5pcsis_read(state, S5PCSIS_CTRL);
 	if (state->csis_fmt->data_alignment == 32)
@@ -338,7 +351,7 @@ static void s5pcsis_set_params(struct csis_state *state)
 		val &= ~S5PCSIS_CTRL_ALIGN_32BIT;
 
 	val &= ~S5PCSIS_CTRL_WCLK_EXTCLK;
-	if (pdata->wclk_source)
+	if (state->wclk_ext)
 		val |= S5PCSIS_CTRL_WCLK_EXTCLK;
 	s5pcsis_write(state, S5PCSIS_CTRL, val);
 
@@ -696,55 +709,117 @@ static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int s5pcsis_get_platform_data(struct platform_device *pdev,
+				     struct csis_state *state)
+{
+	struct s5p_platform_mipi_csis *pdata = pdev->dev.platform_data;
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "Platform data not specified\n");
+		return -EINVAL;
+	}
+	state->clk_frequency = pdata->clk_rate;
+	state->num_lanes = pdata->lanes;
+	state->hs_settle = pdata->hs_settle;
+	state->index = max(0, pdev->id);
+	if (state->index == 1)
+		state->max_num_lanes = CSIS1_MAX_LANES;
+	else
+		state->max_num_lanes = CSIS0_MAX_LANES;
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static int s5pcsis_parse_dt(struct platform_device *pdev,
+			    struct csis_state *state)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct v4l2_of_endpoint endpoint;
+	int ret;
+
+	state->index = of_alias_get_id(node, "csis");
+	if (state->index < 0 || state->index >= CSIS_MAX_ENTITIES)
+		return -EINVAL;
+
+	ret = of_property_read_u32(node, "bus-width",
+				   &state->max_num_lanes);
+	if (ret < 0)
+		return ret;
+
+	if (of_property_read_u32(node, "clock-frequency",
+				 &state->clk_frequency))
+		state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+	/*
+	 * Get endpoint node. We care only about first child
+	 * nodes since these are the only ones available.
+	 */
+	while ((node = of_get_next_child(node, NULL))) {
+		if (!of_node_cmp(node->name, "endpoint"))
+			break;
+		of_node_put(node);
+	};
+	if (!node)
+		return -EINVAL;
+	of_property_read_u32(node, "samsung,csis-hs-settle",
+					&state->hs_settle);
+	state->wclk_ext = of_property_read_bool(node,
+					"samsung,csis-wclk");
+	ret = v4l2_of_parse_mipi_csi2(node, &endpoint);
+	state->num_lanes = endpoint.mipi_csi_2.num_data_lanes;
+
+	of_node_put(node);
+	return 0;
+}
+#else
+#define s5pcsis_parse_dt(pdev, state) (-ENOSYS)
+#endif
+
 static int __devinit s5pcsis_probe(struct platform_device *pdev)
 {
-	struct s5p_platform_mipi_csis *pdata;
+	struct device *dev = &pdev->dev;
 	struct resource *mem_res;
 	struct csis_state *state;
 	int ret = -ENOMEM;
 	int i;
 
-	state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return -ENOMEM;
 
 	mutex_init(&state->lock);
 	spin_lock_init(&state->slock);
-
 	state->pdev = pdev;
-	state->index = max(0, pdev->id);
 
-	pdata = pdev->dev.platform_data;
-	if (pdata == NULL) {
-		dev_err(&pdev->dev, "Platform data not fully specified\n");
-		return -EINVAL;
-	}
+	if (dev->of_node)
+		ret = s5pcsis_parse_dt(pdev, state);
+	else
+		ret = s5pcsis_get_platform_data(pdev, state);
+	if (ret < 0)
+		return ret;
 
-	if ((state->index == 1 && pdata->lanes > CSIS1_MAX_LANES) ||
-	    pdata->lanes > CSIS0_MAX_LANES) {
-		dev_err(&pdev->dev, "Unsupported number of data lanes: %d\n",
-			pdata->lanes);
+	if (state->num_lanes == 0 || state->num_lanes > state->max_num_lanes) {
+		dev_err(dev, "Unsupported number of data lanes: %d (max. %d)\n",
+			state->num_lanes, state->max_num_lanes);
 		return -EINVAL;
 	}
 
 	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	state->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
+	state->regs = devm_request_and_ioremap(dev, mem_res);
 	if (state->regs == NULL) {
-		dev_err(&pdev->dev, "Failed to request and remap io memory\n");
+		dev_err(dev, "Failed to request and remap io memory\n");
 		return -ENXIO;
 	}
 
 	state->irq = platform_get_irq(pdev, 0);
 	if (state->irq < 0) {
-		dev_err(&pdev->dev, "Failed to get irq\n");
+		dev_err(dev, "Failed to get irq\n");
 		return state->irq;
 	}
 
 	for (i = 0; i < CSIS_NUM_SUPPLIES; i++)
 		state->supplies[i].supply = csis_supply_name[i];
 
-	ret = regulator_bulk_get(&pdev->dev, CSIS_NUM_SUPPLIES,
-				 state->supplies);
+	ret = regulator_bulk_get(dev, CSIS_NUM_SUPPLIES, state->supplies);
 	if (ret)
 		return ret;
 
@@ -753,21 +828,22 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 		goto e_clkput;
 
 	clk_enable(state->clock[CSIS_CLK_MUX]);
-	if (pdata->clk_rate)
-		clk_set_rate(state->clock[CSIS_CLK_MUX], pdata->clk_rate);
+	if (state->clk_frequency)
+		clk_set_rate(state->clock[CSIS_CLK_MUX], state->clk_frequency);
 	else
-		dev_WARN(&pdev->dev, "No clock frequency specified!\n");
+		dev_WARN(dev, "No clock frequency specified!\n");
 
-	ret = devm_request_irq(&pdev->dev, state->irq, s5pcsis_irq_handler,
-			       0, dev_name(&pdev->dev), state);
+	ret = devm_request_irq(dev, state->irq, s5pcsis_irq_handler,
+			       0, dev_name(dev), state);
 	if (ret) {
-		dev_err(&pdev->dev, "Interrupt request failed\n");
+		dev_err(dev, "Interrupt request failed\n");
 		goto e_regput;
 	}
 
 	v4l2_subdev_init(&state->sd, &s5pcsis_subdev_ops);
 	state->sd.owner = THIS_MODULE;
-	strlcpy(state->sd.name, dev_name(&pdev->dev), sizeof(state->sd.name));
+	snprintf(state->sd.name, sizeof(state->sd.name), "%s.%d",
+		 CSIS_SUBDEV_NAME, state->index);
 	state->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	state->csis_fmt = &s5pcsis_formats[0];
 
@@ -787,10 +863,12 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 
 	/* .. and a pointer to the subdev. */
 	platform_set_drvdata(pdev, &state->sd);
-
 	memcpy(state->events, s5pcsis_events, sizeof(state->events));
+	pm_runtime_enable(dev);
 
-	pm_runtime_enable(&pdev->dev);
+	dev_info(&pdev->dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
+		 state->num_lanes, state->hs_settle, state->wclk_ext,
+		 state->clk_frequency);
 	return 0;
 
 e_regput:
@@ -916,13 +994,21 @@ static const struct dev_pm_ops s5pcsis_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(s5pcsis_suspend, s5pcsis_resume)
 };
 
+static const struct of_device_id s5pcsis_of_match[] __devinitconst = {
+	{ .compatible = "samsung,s5pv210-csis" },
+	{ .compatible = "samsung,exynos4210-csis" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s5pcsis_of_match);
+
 static struct platform_driver s5pcsis_driver = {
 	.probe		= s5pcsis_probe,
 	.remove		= __devexit_p(s5pcsis_remove),
 	.driver		= {
-		.name	= CSIS_DRIVER_NAME,
-		.owner	= THIS_MODULE,
-		.pm	= &s5pcsis_pm_ops,
+		.of_match_table = of_match_ptr(s5pcsis_of_match),
+		.name		= CSIS_DRIVER_NAME,
+		.owner		= THIS_MODULE,
+		.pm		= &s5pcsis_pm_ops,
 	},
 };
 
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.h b/drivers/media/platform/s5p-fimc/mipi-csis.h
index 2709286..28c11c4 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.h
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.h
@@ -11,6 +11,7 @@
 #define S5P_MIPI_CSIS_H_
 
 #define CSIS_DRIVER_NAME	"s5p-mipi-csis"
+#define CSIS_SUBDEV_NAME	CSIS_DRIVER_NAME
 #define CSIS_MAX_ENTITIES	2
 #define CSIS0_MAX_LANES		4
 #define CSIS1_MAX_LANES		2
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 04/14] s5p-fimc: Add device tree support for FIMC devices
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (2 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 03/14] s5p-csis: Add device tree support Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 05/14] s5p-fimc: Add device tree support for FIMC-LITE devices Sylwester Nawrocki
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This patch adds support for FIMC devices instantiated from devicetree
for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
capture interface.

Multiple SoC revisions specific parameters are defined statically in
the driver and are used for both dt and non-dt. The driver's static
data is selected based on the compatible property. Previously the
platform device name was used to match driver data and a specific
SoC/IP version.

Aliases are used to determine an index of the IP which is essential
for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v3:
 - added optional clock-frequency property to specify local bus
   (FIMCn_LCLK) clock frequency
---
 .../devicetree/bindings/media/soc/samsung-fimc.txt |   76 ++++++++++++++++
 drivers/media/platform/s5p-fimc/fimc-capture.c     |    2 +-
 drivers/media/platform/s5p-fimc/fimc-core.c        |   93 +++++++++++++-------
 3 files changed, 140 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
new file mode 100644
index 0000000..1d12142
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -0,0 +1,76 @@
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem comprises of multiple sub-devices that are
+represented by separate platform devices. Some of the IPs come in different
+variants accross the SoC revisions (FIMC) and some remain mostly unchanged
+(MIPI CSIS, FIMC-LITE).
+
+All those sub-subdevices are defined as parent nodes of the common device
+node, which also includes common properties of the whole subsystem not really
+specific to any single sub-device, like common camera port pins or external
+clocks for image sensors attached to the SoC.
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible	   : must be "samsung,fimc", "simple-bus"
+
+The 'camera' node must include at least one 'fimc' child node.
+
+
+'fimc' device nodes
+-------------------
+
+Required properties:
+
+- compatible : "samsung,s5pv210-fimc" for S5PV210,
+	       "samsung,exynos4210-fimc" for Exynos4210,
+	       "samsung,exynos4212-fimc" for Exynos4212/4412 SoCs;
+- reg	     : physical base address and size of the device memory mapped
+	       registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+
+For every fimc node a numbered alias should be present in the aliases node.
+Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
+the IP's instance index.
+
+Optional properties
+
+ - clock-frequency - FIMC local clock (LCLK) frequency
+
+Example:
+
+	aliases {
+		csis0 = &csis_0;
+		fimc0 = &fimc_0;
+	};
+
+	camera {
+		compatible = "samsung,fimc", "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		status = "okay";
+
+		pinctrl-names = "default", "inactive";
+		pinctrl-0 = <&cam_port_a_clk_active>;
+		pinctrl-1 = <&cam_port_a_clk_idle>;
+
+		fimc_0: fimc@11800000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11800000 0x1000>;
+			interrupts = <0 85 0>;
+			status = "okay";
+		};
+
+		csis_0: csis@11880000 {
+			compatible = "samsung,exynos4210-csis";
+			reg = <0x11880000 0x1000>;
+			interrupts = <0 78 0>;
+			max-data-lanes = <4>;
+		};
+	};
+
+[1] Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index 35998a3..7329e14 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -1893,7 +1893,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
 
 	v4l2_subdev_init(sd, &fimc_subdev_ops);
 	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
-	snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->pdev->id);
+	snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->id);
 
 	fimc->vid_cap.sd_pads[FIMC_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	fimc->vid_cap.sd_pads[FIMC_SD_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index a02cbf9..5a5d44b 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -21,6 +21,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <media/v4l2-ioctl.h>
@@ -876,58 +878,74 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
 	return 0;
 }
 
+static const struct of_device_id fimc_of_match[];
+
 static int fimc_probe(struct platform_device *pdev)
 {
-	const struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev);
-	struct s5p_platform_fimc *pdata;
+	struct fimc_drvdata *drv_data = NULL;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	u32 lclk_freq = 0;
 	struct fimc_dev *fimc;
 	struct resource *res;
 	int ret = 0;
 
-	if (pdev->id >= drv_data->num_entities) {
-		dev_err(&pdev->dev, "Invalid platform device id: %d\n",
-			pdev->id);
-		return -EINVAL;
-	}
-
-	fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+	fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
 	if (!fimc)
 		return -ENOMEM;
 
-	fimc->id = pdev->id;
+	if (dev->of_node) {
+		of_id = of_match_node(fimc_of_match, dev->of_node);
+		if (of_id)
+			drv_data = (struct fimc_drvdata *)of_id->data;
+		fimc->id = of_alias_get_id(dev->of_node, "fimc");
+
+		of_property_read_u32(dev->of_node, "clock-frequency",
+							&lclk_freq);
+	} else {
+		drv_data = fimc_get_drvdata(pdev);
+		fimc->id = pdev->id;
+	}
+
+	if (!drv_data || fimc->id < 0 || fimc->id >= drv_data->num_entities) {
+		dev_err(dev, "Invalid driver data or device index (%d)\n",
+			fimc->id);
+		return -EINVAL;
+	}
 
 	fimc->variant = drv_data->variant[fimc->id];
 	fimc->pdev = pdev;
-	pdata = pdev->dev.platform_data;
-	fimc->pdata = pdata;
-
 	init_waitqueue_head(&fimc->irq_queue);
 	spin_lock_init(&fimc->slock);
 	mutex_init(&fimc->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+	fimc->regs = devm_request_and_ioremap(dev, res);
 	if (fimc->regs == NULL) {
-		dev_err(&pdev->dev, "Failed to obtain io memory\n");
+		dev_err(dev, "Failed to obtain io memory\n");
 		return -ENOENT;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res == NULL) {
-		dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+		dev_err(dev, "Failed to get IRQ resource\n");
 		return -ENXIO;
 	}
 
 	ret = fimc_clk_get(fimc);
 	if (ret)
 		return ret;
-	clk_set_rate(fimc->clock[CLK_BUS], drv_data->lclk_frequency);
+
+	if (lclk_freq == 0)
+		lclk_freq = drv_data->lclk_frequency;
+
+	clk_set_rate(fimc->clock[CLK_BUS], lclk_freq);
 	clk_enable(fimc->clock[CLK_BUS]);
 
-	ret = devm_request_irq(&pdev->dev, res->start, fimc_irq_handler,
-			       0, dev_name(&pdev->dev), fimc);
+	ret = devm_request_irq(dev, res->start, fimc_irq_handler,
+			       0, dev_name(dev), fimc);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to install irq (%d)\n", ret);
+		dev_err(dev, "failed to install irq (%d)\n", ret);
 		goto err_clk;
 	}
 
@@ -936,23 +954,23 @@ static int fimc_probe(struct platform_device *pdev)
 		goto err_clk;
 
 	platform_set_drvdata(pdev, fimc);
-	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err_sd;
 	/* Initialize contiguous memory allocator */
-	fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
 		goto err_pm;
 	}
 
-	dev_dbg(&pdev->dev, "FIMC.%d registered successfully\n", fimc->id);
+	dev_dbg(dev, "FIMC.%d registered successfully\n", fimc->id);
 
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 	return 0;
 err_pm:
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 err_sd:
 	fimc_unregister_capture_subdev(fimc);
 err_clk:
@@ -1264,10 +1282,24 @@ static const struct platform_device_id fimc_driver_ids[] = {
 		.name		= "exynos4x12-fimc",
 		.driver_data	= (unsigned long)&fimc_drvdata_exynos4x12,
 	},
-	{},
+	{ },
 };
 MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
 
+static const struct of_device_id fimc_of_match[] = {
+	{
+		.compatible = "samsung,s5pv210-fimc",
+		.data = &fimc_drvdata_s5pv210,
+	}, {
+		.compatible = "samsung,exynos4210-fimc",
+		.data = &fimc_drvdata_exynos4210,
+	}, {
+		.compatible = "samsung,exynos4212-fimc",
+		.data = &fimc_drvdata_exynos4x12,
+	},
+	{ /* sentinel */ },
+};
+
 static const struct dev_pm_ops fimc_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume)
 	SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
@@ -1278,9 +1310,10 @@ static struct platform_driver fimc_driver = {
 	.remove		= __devexit_p(fimc_remove),
 	.id_table	= fimc_driver_ids,
 	.driver = {
-		.name	= FIMC_MODULE_NAME,
-		.owner	= THIS_MODULE,
-		.pm     = &fimc_pm_ops,
+		.of_match_table = of_match_ptr(fimc_of_match),
+		.name		= FIMC_MODULE_NAME,
+		.owner		= THIS_MODULE,
+		.pm     	= &fimc_pm_ops,
 	}
 };
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 05/14] s5p-fimc: Add device tree support for FIMC-LITE devices
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (3 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 04/14] s5p-fimc: Add device tree support for FIMC devices Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 06/14] s5p-fimc: Change platform subdevs registration method Sylwester Nawrocki
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This patch add support for binding the driver to FIMC-LITE devices
instantiated from the device tree.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/media/soc/samsung-fimc.txt |   15 +++++
 drivers/media/platform/s5p-fimc/fimc-lite.c        |   65 ++++++++++++++------
 2 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 1d12142..aedc2d1 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -41,6 +41,21 @@ Optional properties
 
  - clock-frequency - FIMC local clock (LCLK) frequency
 
+'fimc-lite' device nodes
+-----------------------
+
+Required properties:
+
+- compatible : should be "samsung,exynos4212-fimc" for Exynos4212 and
+	       Exynos4412 SoCs;
+- reg	     : physical base address and size of the device memory mapped
+	       registers;
+- interrupts : should contain FIMC-LITE interrupt;
+
+For every fimc-lite node a numbered alias should be present in the aliases
+node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
+specifying the IP's instance index.
+
 Example:
 
 	aliases {
diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c b/drivers/media/platform/s5p-fimc/fimc-lite.c
index cb96ebb..be7e6f1 100644
--- a/drivers/media/platform/s5p-fimc/fimc-lite.c
+++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/types.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -1480,18 +1481,34 @@ static int fimc_lite_clk_get(struct fimc_lite *fimc)
 	return ret;
 }
 
+static const struct of_device_id flite_of_match[];
+
 static int __devinit fimc_lite_probe(struct platform_device *pdev)
 {
-	struct flite_drvdata *drv_data = fimc_lite_get_drvdata(pdev);
+	struct flite_drvdata *drv_data = NULL;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
 	struct fimc_lite *fimc;
 	struct resource *res;
 	int ret;
 
-	fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+	fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
 	if (!fimc)
 		return -ENOMEM;
 
-	fimc->index = pdev->id;
+	if (dev->of_node) {
+		of_id = of_match_node(flite_of_match, dev->of_node);
+		if (of_id)
+			drv_data = (struct flite_drvdata *)of_id->data;
+		fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
+	} else {
+		drv_data = fimc_lite_get_drvdata(pdev);
+		fimc->index = pdev->id;
+	}
+
+	if (!drv_data || fimc->index < 0 || fimc->index >= FIMC_LITE_MAX_DEVS)
+		return -EINVAL;
+
 	fimc->variant = drv_data->variant[fimc->index];
 	fimc->pdev = pdev;
 
@@ -1500,15 +1517,15 @@ static int __devinit fimc_lite_probe(struct platform_device *pdev)
 	mutex_init(&fimc->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+	fimc->regs = devm_request_and_ioremap(dev, res);
 	if (fimc->regs == NULL) {
-		dev_err(&pdev->dev, "Failed to obtain io memory\n");
+		dev_err(dev, "Failed to obtain io memory\n");
 		return -ENOENT;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res == NULL) {
-		dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+		dev_err(dev, "Failed to get IRQ resource\n");
 		return -ENXIO;
 	}
 
@@ -1516,10 +1533,10 @@ static int __devinit fimc_lite_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = devm_request_irq(&pdev->dev, res->start, flite_irq_handler,
-			       0, dev_name(&pdev->dev), fimc);
+	ret = devm_request_irq(dev, res->start, flite_irq_handler,
+			       0, dev_name(dev), fimc);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to install irq (%d)\n", ret);
+		dev_err(dev, "Failed to install irq (%d)\n", ret);
 		goto err_clk;
 	}
 
@@ -1529,23 +1546,23 @@ static int __devinit fimc_lite_probe(struct platform_device *pdev)
 		goto err_clk;
 
 	platform_set_drvdata(pdev, fimc);
-	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err_sd;
 
-	fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
 		goto err_pm;
 	}
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 
-	dev_dbg(&pdev->dev, "FIMC-LITE.%d registered successfully\n",
+	dev_dbg(dev, "FIMC-LITE.%d registered successfully\n",
 		fimc->index);
 	return 0;
 err_pm:
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 err_sd:
 	fimc_lite_unregister_capture_subdev(fimc);
 err_clk:
@@ -1636,6 +1653,12 @@ static int __devexit fimc_lite_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct dev_pm_ops fimc_lite_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
+	SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
+			   NULL)
+};
+
 static struct flite_variant fimc_lite0_variant_exynos4 = {
 	.max_width		= 8192,
 	.max_height		= 8192,
@@ -1661,17 +1684,21 @@ static struct platform_device_id fimc_lite_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, fimc_lite_driver_ids);
 
-static const struct dev_pm_ops fimc_lite_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
-	SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
-			   NULL)
+static const struct of_device_id flite_of_match[] = {
+	{
+		.compatible = "samsung,exynos4212-fimc-lite",
+		.data = &fimc_lite_drvdata_exynos4,
+	},
+	{ /* sentinel */ },
 };
+MODULE_DEVICE_TABLE(of, flite_of_match);
 
 static struct platform_driver fimc_lite_driver = {
 	.probe		= fimc_lite_probe,
 	.remove		= __devexit_p(fimc_lite_remove),
 	.id_table	= fimc_lite_driver_ids,
 	.driver = {
+		.of_match_table = of_match_ptr(flite_of_match),
 		.name		= FIMC_LITE_DRV_NAME,
 		.owner		= THIS_MODULE,
 		.pm		= &fimc_lite_pm_ops,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 06/14] s5p-fimc: Change platform subdevs registration method
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (4 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 05/14] s5p-fimc: Add device tree support for FIMC-LITE devices Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 07/14] s5p-fimc: Add device tree support for the main media device driver Sylwester Nawrocki
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

The previous method of registering platform entities into the main
driver using driver_find() and then iterating over devices bound to
a driver was racy and is being removed here. Nothing was preventing
module from unloading during a call to try_module_get(driver->owner).
Instead, we look up a device first and then check for its driver while
holding device lock.

The platform sub-devices are looked up and registered to the top
level driver. When any sub-device is not yet initialized and ready
the main driver's probe() will be deferred.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |  203 +++++++++++++-----------
 1 file changed, 108 insertions(+), 95 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 501ac48..abf6000 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -1,8 +1,8 @@
 /*
  * S5P/EXYNOS4 SoC series camera host interface media device driver
  *
- * Copyright (C) 2011 Samsung Electronics Co., Ltd.
- * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
+ * Copyright (C) 2011 - 2012 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <s.nawrocki@samsung.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published
@@ -312,138 +312,149 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 }
 
 /*
- * MIPI CSIS and FIMC platform devices registration.
+ * MIPI-CSIS, FIMC and FIMC-LITE platform devices registration.
  */
-static int fimc_register_callback(struct device *dev, void *p)
+
+static int register_fimc_lite_entity(struct fimc_md *fmd,
+				     struct fimc_lite *fimc_lite)
 {
-	struct fimc_dev *fimc = dev_get_drvdata(dev);
 	struct v4l2_subdev *sd;
-	struct fimc_md *fmd = p;
 	int ret;
 
-	if (fimc == NULL || fimc->id >= FIMC_MAX_DEVS)
-		return 0;
+	if (WARN_ON(fimc_lite->index >= FIMC_LITE_MAX_DEVS ||
+		    fmd->fimc_lite[fimc_lite->index]))
+		return -EBUSY;
 
-	sd = &fimc->vid_cap.subdev;
-	sd->grp_id = GRP_ID_FIMC;
+	sd = &fimc_lite->subdev;
+	sd->grp_id = GRP_ID_FLITE;
 	v4l2_set_subdev_hostdata(sd, (void *)&fimc_pipeline_ops);
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	if (ret) {
-		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d (%d)\n",
-			 fimc->id, ret);
-		return ret;
-	}
-
-	fmd->fimc[fimc->id] = fimc;
-	return 0;
+	if (!ret)
+		fmd->fimc_lite[fimc_lite->index] = fimc_lite;
+	else
+		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.LITE%d\n",
+			 fimc_lite->index);
+	return ret;
 }
 
-static int fimc_lite_register_callback(struct device *dev, void *p)
+static int register_fimc_entity(struct fimc_md *fmd, struct fimc_dev *fimc)
 {
-	struct fimc_lite *fimc = dev_get_drvdata(dev);
-	struct fimc_md *fmd = p;
+	struct v4l2_subdev *sd;
 	int ret;
 
-	if (fimc == NULL || fimc->index >= FIMC_LITE_MAX_DEVS)
-		return 0;
+	if (WARN_ON(fimc->id >= FIMC_MAX_DEVS || fmd->fimc[fimc->id]))
+		return -EBUSY;
 
-	fimc->subdev.grp_id = GRP_ID_FLITE;
-	v4l2_set_subdev_hostdata(&fimc->subdev, (void *)&fimc_pipeline_ops);
+	sd = &fimc->vid_cap.subdev;
+	sd->grp_id = GRP_ID_FIMC;
+	v4l2_set_subdev_hostdata(sd, (void *)&fimc_pipeline_ops);
 
-	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, &fimc->subdev);
-	if (ret) {
-		v4l2_err(&fmd->v4l2_dev,
-			 "Failed to register FIMC-LITE.%d (%d)\n",
-			 fimc->index, ret);
-		return ret;
+	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+	if (!ret) {
+		fmd->fimc[fimc->id] = fimc;
+		fimc->vid_cap.user_subdev_api = fmd->user_subdev_api;
+	} else {
+		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d (%d)\n",
+			 fimc->id, ret);
 	}
-
-	fmd->fimc_lite[fimc->index] = fimc;
-	return 0;
+	return ret;
 }
 
-static int csis_register_callback(struct device *dev, void *p)
+static int register_csis_entity(struct fimc_md *fmd,
+				struct platform_device *pdev,
+				struct v4l2_subdev *sd)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct platform_device *pdev;
-	struct fimc_md *fmd = p;
+	struct device_node *node = pdev->dev.of_node;
 	int id, ret;
 
-	if (!sd)
-		return 0;
-	pdev = v4l2_get_subdevdata(sd);
-	if (!pdev || pdev->id < 0 || pdev->id >= CSIS_MAX_ENTITIES)
+	id = node ? of_alias_get_id(node, "csis") : max(0, pdev->id);
+
+	if (WARN_ON(id >= CSIS_MAX_ENTITIES || fmd->csis[id].sd))
+		return -EBUSY;
+
+	if (WARN_ON(id >= CSIS_MAX_ENTITIES))
 		return 0;
-	v4l2_info(sd, "csis%d sd: %s\n", pdev->id, sd->name);
 
-	id = pdev->id < 0 ? 0 : pdev->id;
 	sd->grp_id = GRP_ID_CSIS;
-
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
 	if (!ret)
 		fmd->csis[id].sd = sd;
 	else
 		v4l2_err(&fmd->v4l2_dev,
-			 "Failed to register CSIS subdevice: %d\n", ret);
+			 "Failed to register MIPI-CSIS.%d (%d)\n", id, ret);
 	return ret;
 }
 
-/**
- * fimc_md_register_platform_entities - register FIMC and CSIS media entities
- */
-static int fimc_md_register_platform_entities(struct fimc_md *fmd)
+static int fimc_md_register_platform_entity(struct fimc_md *fmd,
+					    struct platform_device *pdev,
+					    int plat_entity)
 {
-	struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
-	struct device_driver *driver;
-	int ret, i;
-
-	driver = driver_find(FIMC_MODULE_NAME, &platform_bus_type);
-	if (!driver) {
-		v4l2_warn(&fmd->v4l2_dev,
-			 "%s driver not found, deffering probe\n",
-			 FIMC_MODULE_NAME);
-		return -EPROBE_DEFER;
-	}
-
-	ret = driver_for_each_device(driver, NULL, fmd,
-				     fimc_register_callback);
-	if (ret)
-		return ret;
-
-	driver = driver_find(FIMC_LITE_DRV_NAME, &platform_bus_type);
-	if (driver && try_module_get(driver->owner)) {
-		ret = driver_for_each_device(driver, NULL, fmd,
-					     fimc_lite_register_callback);
-		if (ret)
-			return ret;
-		module_put(driver->owner);
-	}
-	/*
-	 * Check if there is any sensor on the MIPI-CSI2 bus and
-	 * if not skip the s5p-csis module loading.
-	 */
-	if (pdata == NULL)
-		return 0;
-	for (i = 0; i < pdata->num_clients; i++) {
-		if (pdata->isp_info[i].bus_type == FIMC_MIPI_CSI2) {
-			ret = 1;
+	struct device *dev = &pdev->dev;
+	int ret = -EPROBE_DEFER;
+	void *drvdata;
+
+	/* Lock to ensure dev->driver won't change. */
+	device_lock(dev);
+
+	if (!dev->driver || !try_module_get(dev->driver->owner))
+		goto dev_unlock;
+
+	drvdata = dev_get_drvdata(dev);
+	/* Some subdev didn't probe succesfully id drvdata is NULL */
+	if (drvdata) {
+		switch (plat_entity) {
+		case IDX_FIMC:
+			ret = register_fimc_entity(fmd, drvdata);
+			break;
+		case IDX_FLITE:
+			ret = register_fimc_lite_entity(fmd, drvdata);
 			break;
+		case IDX_CSIS:
+			ret = register_csis_entity(fmd, pdev, drvdata);
+			break;
+		default:
+			ret = -ENODEV;
 		}
 	}
-	if (!ret)
-		return 0;
 
-	driver = driver_find(CSIS_DRIVER_NAME, &platform_bus_type);
-	if (!driver || !try_module_get(driver->owner)) {
-		v4l2_warn(&fmd->v4l2_dev,
-			 "%s driver not found, deffering probe\n",
-			 CSIS_DRIVER_NAME);
-		return -EPROBE_DEFER;
+	module_put(dev->driver->owner);
+dev_unlock:
+	device_unlock(dev);
+	if (ret == -EPROBE_DEFER)
+		dev_info(&fmd->pdev->dev, "deferring %s device registration\n",
+			dev_name(dev));
+	else if (ret < 0)
+		dev_err(&fmd->pdev->dev, "%s device registration failed (%d)\n",
+			dev_name(dev), ret);
+	return ret;
+}
+
+static int fimc_md_pdev_match(struct device *dev, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	int plat_entity = -1;
+	int ret;
+	char *p;
+
+	if (!get_device(dev))
+		return -ENODEV;
+
+	if (!strcmp(pdev->name, CSIS_DRIVER_NAME)) {
+		plat_entity = IDX_CSIS;
+	} else if (!strcmp(pdev->name, FIMC_LITE_DRV_NAME)) {
+		plat_entity = IDX_FLITE;
+	} else {
+		p = strstr(pdev->name, "fimc");
+		if (p && *(p + 4) == 0)
+			plat_entity = IDX_FIMC;
 	}
 
-	return driver_for_each_device(driver, NULL, fmd,
-				      csis_register_callback);
+	if (plat_entity >= 0)
+		ret = fimc_md_register_platform_entity(data, pdev,
+						       plat_entity);
+	put_device(dev);
+	return 0;
 }
 
 static void fimc_md_unregister_entities(struct fimc_md *fmd)
@@ -477,6 +488,7 @@ static void fimc_md_unregister_entities(struct fimc_md *fmd)
 		fimc_md_unregister_sensor(fmd->sensor[i].subdev);
 		fmd->sensor[i].subdev = NULL;
 	}
+	v4l2_info(&fmd->v4l2_dev, "Unregistered all entities\n");
 }
 
 /**
@@ -941,7 +953,8 @@ static int fimc_md_probe(struct platform_device *pdev)
 	/* Protect the media graph while we're registering entities */
 	mutex_lock(&fmd->media_dev.graph_mutex);
 
-	ret = fimc_md_register_platform_entities(fmd);
+	ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
+					fimc_md_pdev_match);
 	if (ret)
 		goto err_unlock;
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 07/14] s5p-fimc: Add device tree support for the main media device driver
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (5 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 06/14] s5p-fimc: Change platform subdevs registration method Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 08/14] s5p-fimc: Add device tree based sensors registration Sylwester Nawrocki
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This patch adds changes required for the main camera media device
driver to be initialized on systems instantiated from the device tree.

The platform devices corresponding to child nodes of the 'camera'
node are looked up and and registered as sub-devices to the common
media device. The main driver's probing is deferred if any of the
sub-device drivers is not yet initialized and ready.

An OF matching table is added for the main driver associated with
the 'camera' node.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-core.c    |    1 -
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |   77 +++++++++++++++++++++---
 drivers/media/platform/s5p-fimc/fimc-mdevice.h |    4 ++
 include/media/s5p_fimc.h                       |    1 +
 4 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 5a5d44b..720ffee 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -1284,7 +1284,6 @@ static const struct platform_device_id fimc_driver_ids[] = {
 	},
 	{ },
 };
-MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
 
 static const struct of_device_id fimc_of_match[] = {
 	{
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index abf6000..3cd2b31 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -17,6 +17,8 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
@@ -457,6 +459,51 @@ static int fimc_md_pdev_match(struct device *dev, void *data)
 	return 0;
 }
 
+/* Register FIMC, FIMC-LITE and CSIS media entities */
+#ifdef CONFIG_OF
+static int fimc_md_register_of_platform_entities(struct fimc_md *fmd,
+						 struct device_node *parent)
+{
+	struct device_node *node;
+	int ret = 0;
+
+	for_each_available_child_of_node(parent, node) {
+		struct platform_device *pdev;
+		int plat_entity = -1;
+
+		pdev = of_find_device_by_node(node);
+		if (!pdev)
+			continue;
+
+		/* If driver of any entity isn't ready try all again later. */
+		if (!strcmp(node->name, CSIS_OF_NODE_NAME))
+			plat_entity = IDX_CSIS;
+		else if (!strcmp(node->name, FIMC_LITE_OF_NODE_NAME))
+			plat_entity = IDX_FLITE;
+		else if	(!strcmp(node->name, FIMC_OF_NODE_NAME))
+			plat_entity = IDX_FIMC;
+
+		if (plat_entity >= 0)
+			ret = fimc_md_register_platform_entity(fmd, pdev,
+							plat_entity);
+		put_device(&pdev->dev);
+		if (ret < 0)
+			break;
+
+		/* FIMC-IS child devices */
+		if (plat_entity == IDX_IS_ISP) {
+			ret = fimc_md_register_of_platform_entities(fmd, node);
+			if (ret < 0)
+				break;
+		}
+	}
+
+	return ret;
+}
+#else
+#define fimc_md_register_platform_entities(fmd) (-ENOSYS)
+#endif
+
 static void fimc_md_unregister_entities(struct fimc_md *fmd)
 {
 	int i;
@@ -931,8 +978,8 @@ static int fimc_md_probe(struct platform_device *pdev)
 	v4l2_dev = &fmd->v4l2_dev;
 	v4l2_dev->mdev = &fmd->media_dev;
 	v4l2_dev->notify = fimc_sensor_notify;
-	snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s",
-		 dev_name(&pdev->dev));
+	strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
+
 
 	ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
 	if (ret < 0) {
@@ -948,13 +995,16 @@ static int fimc_md_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk;
 
-	fmd->user_subdev_api = false;
+	fmd->user_subdev_api = (dev->of_node != NULL);
 
 	/* Protect the media graph while we're registering entities */
 	mutex_lock(&fmd->media_dev.graph_mutex);
 
-	ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
-					fimc_md_pdev_match);
+	if (fmd->pdev->dev.of_node)
+		ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
+	else
+		ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
+						fimc_md_pdev_match);
 	if (ret)
 		goto err_unlock;
 
@@ -1002,12 +1052,25 @@ static int __devexit fimc_md_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct platform_device_id fimc_driver_ids[] __always_unused = {
+	{ .name = "s5p-fimc-md" },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+
+static const struct of_device_id fimc_md_of_match[] __initconst = {
+	{ .compatible = "samsung,fimc" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, fimc_md_of_match);
+
 static struct platform_driver fimc_md_driver = {
 	.probe		= fimc_md_probe,
 	.remove		= __devexit_p(fimc_md_remove),
 	.driver = {
-		.name	= "s5p-fimc-md",
-		.owner	= THIS_MODULE,
+		.of_match_table = fimc_md_of_match,
+		.name		= "s5p-fimc-md",
+		.owner		= THIS_MODULE,
 	}
 };
 
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.h b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
index da7d992..38006ba 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
@@ -21,6 +21,10 @@
 #include "fimc-lite.h"
 #include "mipi-csis.h"
 
+#define FIMC_OF_NODE_NAME	"fimc"
+#define FIMC_LITE_OF_NODE_NAME	"fimc-lite"
+#define CSIS_OF_NODE_NAME	"csis"
+
 /* Group IDs of sensor, MIPI-CSIS, FIMC-LITE and the writeback subdevs. */
 #define GRP_ID_SENSOR		(1 << 8)
 #define GRP_ID_FIMC_IS_SENSOR	(1 << 9)
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index eaea62a..fab66ed 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -68,6 +68,7 @@ enum fimc_subdev_index {
 	IDX_SENSOR,
 	IDX_CSIS,
 	IDX_FLITE,
+	IDX_IS_ISP,
 	IDX_FIMC,
 	IDX_MAX,
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 08/14] s5p-fimc: Add device tree based sensors registration
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (6 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 07/14] s5p-fimc: Add device tree support for the main media device driver Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 09/14] s5p-fimc: Use pinctrl API for camera ports configuration Sylwester Nawrocki
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

The sensor (I2C and/or SPI client) devices are instantiated by their
corresponding control bus drivers. Since the I2C client's master clock
is often provided by a video bus receiver (host interface) or other
than I2C/SPI controller device, the drivers of those client devices
are not accessing hardware in their driver's probe() callback. Instead,
after enabling clock, the host driver calls back into a sub-device
when it wants to activate them. This pattern is used by some in-tree
drivers and this patch also uses it for DT case. This patch is intended
as a first step for adding device tree support to the S5P/Exynos SoC
camera drivers. The second one is adding support for asynchronous
sub-devices registration and clock control from sub-device driver
level. The bindings shall not change when asynchronous probing support
is added.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/media/soc/samsung-fimc.txt |   89 ++++++++
 drivers/media/platform/s5p-fimc/fimc-mdevice.c     |  238 +++++++++++++++++---
 include/media/s5p_fimc.h                           |   16 ++
 3 files changed, 315 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index aedc2d1..0538990 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -56,6 +56,29 @@ For every fimc-lite node a numbered alias should be present in the aliases
 node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
 specifying the IP's instance index.
 
+
+'parallel-ports' node
+---------------------
+
+This node should contain child 'port' nodes specifying active parallel video
+input ports. It includes camera A and camera B inputs. 'reg' property in the
+port nodes specifies data input - 0, 1 indicates input A, B respectively.
+
+Optional properties
+
+- samsung,camclk-out	 : specifies clock output for remote sensor,
+			   0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
+
+Image sensor nodes
+------------------
+
+The sensor device nodes should be added as their control bus controller
+(e.g. I2C0) child nodes and linked to a port created under csis or
+parallel-ports node, using common bindings for video input interfaces,
+.i.e. port/endpoint node pairs. The implementation of this binding requires
+at clock-frequency property to be present under sensor device nodes.
+
+
 Example:
 
 	aliases {
@@ -63,6 +86,47 @@ Example:
 		fimc0 = &fimc_0;
 	};
 
+	/* Parallel bus IF sensor */
+	i2c_0: i2c@13860000 {
+		s5k6aa: sensor@3c {
+			compatible = "samsung,s5k6aafx";
+			reg = <0x3c>;
+			vddio-supply = <...>;
+
+			clock-frequency = <24000000>;
+			clocks = <...>;
+			clock-names = "mclk";
+
+			port {
+				s5k6aa_ep: endpoint {
+					remote-endpoint = <&fimc0_ep>;
+					bus-width = <8>;
+					hsync-active = <0>;
+					hsync-active = <1>;
+					pclk-sample = <1>;
+				};
+			};
+		};
+	};
+
+	/* MIPI CSI-2 bus IF sensor */
+	s5c73m3: sensor@0x1a {
+		compatible = "samsung,s5c73m3";
+		reg = <0x1a>;
+		vddio-supply = <...>;
+
+		clock-frequency = <24000000>;
+		clocks = <...>;
+		clock-names = "mclk";
+
+		port {
+			s5c73m3_1: endpoint {
+				data-lanes = <1>, <2>, <3>, <4>;
+				remote-endpoint = <&csis0_ep>;
+			};
+		};
+	};
+
 	camera {
 		compatible = "samsung,fimc", "simple-bus";
 		#address-cells = <1>;
@@ -73,6 +137,21 @@ Example:
 		pinctrl-0 = <&cam_port_a_clk_active>;
 		pinctrl-1 = <&cam_port_a_clk_idle>;
 
+		/* parallel camera ports */
+		parallel-ports {
+			/* camera A input */
+			port@0 {
+				reg = <1>;
+				fimc0_ep: endpoint {
+					remote-endpoint = <&s5k6aa_ep>;
+					bus-width = <8>;
+					hsync-active = <0>;
+					hsync-active = <1>;
+					pclk-sample = <1>;
+				};
+			};
+		};
+
 		fimc_0: fimc@11800000 {
 			compatible = "samsung,exynos4210-fimc";
 			reg = <0x11800000 0x1000>;
@@ -85,6 +164,16 @@ Example:
 			reg = <0x11880000 0x1000>;
 			interrupts = <0 78 0>;
 			max-data-lanes = <4>;
+			/* camera C input */
+			port {
+				reg = <3>;
+				csis0_ep: endpoint {
+					remote-endpoint = <&s5c73m3_ep>;
+					data-lanes = <1>, <2>, <3>, <4>;
+					samsung,csis-hs-settle = <12>;
+					samsung,camclk-out = <0>;
+				};
+			};
 		};
 	};
 
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 3cd2b31..a21db59 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -19,11 +19,15 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
 #include <media/media-device.h>
 #include <media/s5p_fimc.h>
 
@@ -248,7 +252,7 @@ static struct v4l2_subdev *fimc_md_register_sensor(struct fimc_md *fmd,
 	sd->grp_id = GRP_ID_SENSOR;
 
 	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice %s\n",
-		  s_info->pdata.board_info->type);
+		  sd->name);
 	return sd;
 }
 
@@ -260,17 +264,186 @@ static void fimc_md_unregister_sensor(struct v4l2_subdev *sd)
 	if (!client)
 		return;
 	v4l2_device_unregister_subdev(sd);
-	adapter = client->adapter;
-	i2c_unregister_device(client);
-	if (adapter)
-		i2c_put_adapter(adapter);
+
+	if (!client->dev.of_node) {
+		adapter = client->adapter;
+		i2c_unregister_device(client);
+		if (adapter)
+			i2c_put_adapter(adapter);
+	}
+}
+
+#ifdef CONFIG_OF
+/* Register I2C client subdev associated with @node. */
+static int fimc_md_of_add_sensor(struct fimc_md *fmd,
+				 struct device_node *node, int index)
+{
+	struct fimc_sensor_info *si;
+	struct i2c_client *client;
+	struct v4l2_subdev *sd;
+	int ret;
+
+	if (index >= ARRAY_SIZE(fmd->sensor))
+		return -EINVAL;
+	si = &fmd->sensor[index];
+
+	client = of_find_i2c_device_by_node(node);
+	if (!client)
+		return -EPROBE_DEFER;
+
+	device_lock(&client->dev);
+
+	if (!client->driver ||
+	    !try_module_get(client->driver->driver.owner)) {
+		ret = -EPROBE_DEFER;
+		goto dev_put;
+	}
+
+	/* Enable sensor's master clock */
+	ret = __fimc_md_set_camclk(fmd, si, true);
+	if (ret < 0)
+		goto mod_put;
+	sd = i2c_get_clientdata(client);
+
+	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+	__fimc_md_set_camclk(fmd, si, false);
+	if (ret < 0)
+		goto mod_put;
+
+	v4l2_set_subdev_hostdata(sd, si);
+	sd->grp_id = GRP_ID_SENSOR;
+	si->subdev = sd;
+	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
+		  sd->name, fmd->num_sensors);
+	fmd->num_sensors++;
+
+mod_put:
+	module_put(client->driver->driver.owner);
+dev_put:
+	device_unlock(&client->dev);
+	put_device(&client->dev);
+	return ret;
+}
+
+/* Parse port node and register as a sub-device any sensor specified there. */
+static int fimc_md_parse_port_node(struct fimc_md *fmd,
+				   struct device_node *port,
+				   unsigned int index)
+{
+	struct device_node *rem, *endpoint;
+	struct s5p_fimc_isp_info *pd;
+	struct v4l2_of_endpoint bus_cfg;
+	u32 tmp, reg = 0;
+	int ret;
+
+	if (index >= FIMC_MAX_SENSORS ||
+	    of_property_read_u32(port, "reg", &reg))
+		return -EINVAL;
+
+	pd = &fmd->sensor[index].pdata;
+	pd->mux_id = (reg - 1) & 0x1;
+
+	endpoint = of_get_child_by_name(port, "endpoint");
+	if (!endpoint)
+		return 0;
+
+	rem = v4l2_of_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (rem == NULL) {
+		v4l2_info(&fmd->v4l2_dev,
+			 "Remote device at endpoint %s not found\n",
+			 endpoint->full_name);
+		return 0;
+	}
+	if (!of_property_read_u32(rem, "samsung,camclk-out", &tmp))
+		pd->clk_id = tmp;
+
+	if (!of_property_read_u32(rem, "clock-frequency", &tmp))
+		pd->clk_frequency = tmp;
+
+	if (pd->clk_frequency == 0) {
+		v4l2_err(&fmd->v4l2_dev,
+			 "Wrong or unspecified sensor clock frequency\n");
+		of_node_put(rem);
+		return -EINVAL;
+	}
+
+	if (fimc_input_is_parallel(reg)) {
+		v4l2_of_parse_parallel_bus(endpoint, &bus_cfg);
+		if (bus_cfg.mbus_type == V4L2_MBUS_PARALLEL)
+			pd->bus_type = FIMC_ITU_601;
+		else
+			pd->bus_type = FIMC_ITU_656;
+		pd->flags = bus_cfg.mbus_flags;
+	} else if (fimc_input_is_mipi_csi(reg)) {
+		/*
+		 * MIPI CSI-2: only input mux selection
+		 * and sensor's clock frequency is needed.
+		 */
+		pd->bus_type = FIMC_MIPI_CSI2;
+	} else {
+		v4l2_err(&fmd->v4l2_dev,
+			 "Wrong reg property value (%u) at node %s\n",
+			 reg, rem->full_name);
+	}
+
+	ret = fimc_md_of_add_sensor(fmd, rem, index);
+	of_node_put(rem);
+
+	return ret;
 }
 
+/* Register all SoC external sub-devices */
+static int fimc_md_of_sensors_register(struct fimc_md *fmd,
+				       struct device_node *np)
+{
+	struct device_node *parent = fmd->pdev->dev.of_node;
+	struct device_node *node, *ports;
+	int index = 0;
+	int ret;
+
+	/* Attach sensors linked to MIPI CSI-2 receivers */
+	for_each_available_child_of_node(parent, node) {
+		struct device_node *port;
+
+		if (of_node_cmp(node->name, "csis"))
+			continue;
+
+		port = of_get_child_by_name(node, "port");
+		if (!port)
+			return -EINVAL;
+
+		ret = fimc_md_parse_port_node(fmd, port, index);
+		if (ret < 0)
+			return ret;
+		index++;
+	}
+
+	/* Attach sensors listed in the parallel-ports node */
+	ports = of_get_child_by_name(parent, "parallel-ports");
+	if (!ports)
+		return 0;
+
+	for_each_child_of_node(ports, node) {
+		ret = fimc_md_parse_port_node(fmd, node, index);
+		if (ret < 0)
+			break;
+		index++;
+	}
+
+	return 0;
+}
+#else
+#define fimc_md_of_sensors_register(fmd, np) (-ENOSYS)
+#endif
+
 static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 {
 	struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
+	struct device_node *of_node = fmd->pdev->dev.of_node;
 	struct fimc_dev *fd = NULL;
-	int num_clients, ret, i;
+	int num_clients = 0;
+	int ret, i;
 
 	/*
 	 * Runtime resume one of the FIMC entities to make sure
@@ -281,34 +454,41 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 			fd = fmd->fimc[i];
 	if (!fd)
 		return -ENXIO;
+
 	ret = pm_runtime_get_sync(&fd->pdev->dev);
 	if (ret < 0)
 		return ret;
 
-	WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
-	num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
+	if (of_node) {
+		fmd->num_sensors = 0;
+		ret = fimc_md_of_sensors_register(fmd, of_node);
+	} else if (pdata) {
+		WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
+		num_clients = min_t(u32, pdata->num_clients,
+				    ARRAY_SIZE(fmd->sensor));
+		fmd->num_sensors = num_clients;
 
-	fmd->num_sensors = num_clients;
-	for (i = 0; i < num_clients; i++) {
-		struct v4l2_subdev *sd;
+		for (i = 0; i < num_clients; i++) {
+			struct v4l2_subdev *sd;
 
-		fmd->sensor[i].pdata = pdata->isp_info[i];
-		ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
-		if (ret)
-			break;
-		sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
-		ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
+			fmd->sensor[i].pdata = pdata->isp_info[i];
+			ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
+			if (ret)
+				break;
+			sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
+			ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
 
-		if (!IS_ERR(sd)) {
+			if (IS_ERR(sd)) {
+				fmd->sensor[i].subdev = NULL;
+				ret = PTR_ERR(sd);
+				break;
+			}
 			fmd->sensor[i].subdev = sd;
-		} else {
-			fmd->sensor[i].subdev = NULL;
-			ret = PTR_ERR(sd);
-			break;
+			if (ret)
+				break;
 		}
-		if (ret)
-			break;
 	}
+
 	pm_runtime_put(&fd->pdev->dev);
 	return ret;
 }
@@ -959,11 +1139,12 @@ static DEVICE_ATTR(subdev_conf_mode, S_IWUSR | S_IRUGO,
 
 static int fimc_md_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct v4l2_device *v4l2_dev;
 	struct fimc_md *fmd;
 	int ret;
 
-	fmd = devm_kzalloc(&pdev->dev, sizeof(*fmd), GFP_KERNEL);
+	fmd = devm_kzalloc(dev, sizeof(*fmd), GFP_KERNEL);
 	if (!fmd)
 		return -ENOMEM;
 
@@ -973,7 +1154,7 @@ static int fimc_md_probe(struct platform_device *pdev)
 	strlcpy(fmd->media_dev.model, "SAMSUNG S5P FIMC",
 		sizeof(fmd->media_dev.model));
 	fmd->media_dev.link_notify = fimc_md_link_notify;
-	fmd->media_dev.dev = &pdev->dev;
+	fmd->media_dev.dev = dev;
 
 	v4l2_dev = &fmd->v4l2_dev;
 	v4l2_dev->mdev = &fmd->media_dev;
@@ -981,7 +1162,7 @@ static int fimc_md_probe(struct platform_device *pdev)
 	strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
 
 
-	ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
+	ret = v4l2_device_register(dev, &fmd->v4l2_dev);
 	if (ret < 0) {
 		v4l2_err(v4l2_dev, "Failed to register v4l2_device: %d\n", ret);
 		return ret;
@@ -1008,11 +1189,12 @@ static int fimc_md_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unlock;
 
-	if (pdev->dev.platform_data) {
+	if (dev->platform_data || dev->of_node) {
 		ret = fimc_md_register_sensor_entities(fmd);
 		if (ret)
 			goto err_unlock;
 	}
+
 	ret = fimc_md_create_links(fmd);
 	if (ret)
 		goto err_unlock;
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index fab66ed..fbea5c7 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -14,6 +14,22 @@
 
 #include <media/media-entity.h>
 
+/*
+ * Enumeration of data inputs to the camera subsystem.
+ */
+enum fimc_input {
+	FIMC_INPUT_PARALLEL_0	= 1,
+	FIMC_INPUT_PARALLEL_1,
+	FIMC_INPUT_MIPI_CSI2_0	= 3,
+	FIMC_INPUT_MIPI_CSI2_1,
+	FIMC_INPUT_WRITEBACK_A	= 5,
+	FIMC_INPUT_WRITEBACK_B,
+	FIMC_INPUT_WRITEBACK_ISP = 5,
+};
+
+#define fimc_input_is_parallel(x) ((x) == 1 || (x) == 2)
+#define fimc_input_is_mipi_csi(x) ((x) == 3 || (x) == 4)
+
 enum cam_bus_type {
 	FIMC_ITU_601 = 1,
 	FIMC_ITU_656,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 09/14] s5p-fimc: Use pinctrl API for camera ports configuration
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (7 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 08/14] s5p-fimc: Add device tree based sensors registration Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 10/14] ARM: dts: Add camera to node exynos4.dtsi Sylwester Nawrocki
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

Before the camera ports can be used the pinmux needs to be configured
properly. This patch adds a function to get the pinctrl states and to
set default camera port pinmux state during the media driver's probe().
The camera port(s) are configured for video bus operation in this way.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyugmin Park <kyungmin.park@samsung.com>

Changes since v3:
 - removed the "inactive" pinctrl state, it will be added later if required
---
 .../devicetree/bindings/media/soc/samsung-fimc.txt |    4 ++++
 drivers/media/platform/s5p-fimc/fimc-mdevice.c     |   14 ++++++++++++++
 drivers/media/platform/s5p-fimc/fimc-mdevice.h     |    2 ++
 3 files changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 0538990..f844b4a 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -18,6 +18,10 @@ Required properties:
 
 - compatible	   : must be "samsung,fimc", "simple-bus"
 
+- pinctrl-names    : pinctrl names for camera port pinmux control, at least
+		     "default" need to be specified.
+- pinctrl-0...N	   : pinctrl properties corresponding to pinctrl-names
+
 The 'camera' node must include at least one 'fimc' child node.
 
 
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index a21db59..c3cd2ad 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -1137,6 +1137,14 @@ static ssize_t fimc_md_sysfs_store(struct device *dev,
 static DEVICE_ATTR(subdev_conf_mode, S_IWUSR | S_IRUGO,
 		   fimc_md_sysfs_show, fimc_md_sysfs_store);
 
+static int fimc_md_get_pinctrl(struct fimc_md *fmd)
+{
+	fmd->pinctl = devm_pinctrl_get_select_default(&fmd->pdev->dev);
+	if (IS_ERR(fmd->pinctl))
+		return PTR_ERR(fmd->pinctl);
+	return 0;
+}
+
 static int fimc_md_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1181,6 +1189,12 @@ static int fimc_md_probe(struct platform_device *pdev)
 	/* Protect the media graph while we're registering entities */
 	mutex_lock(&fmd->media_dev.graph_mutex);
 
+	if (dev->of_node) {
+		ret = fimc_md_get_pinctrl(fmd);
+		if (ret < 0)
+			goto err_unlock;
+	}
+
 	if (fmd->pdev->dev.of_node)
 		ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
 	else
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.h b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
index 38006ba..8be68a7 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
@@ -10,6 +10,7 @@
 #define FIMC_MDEVICE_H_
 
 #include <linux/clk.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <media/media-device.h>
@@ -85,6 +86,7 @@ struct fimc_md {
 	struct media_device media_dev;
 	struct v4l2_device v4l2_dev;
 	struct platform_device *pdev;
+	struct pinctrl *pinctl;
 	bool user_subdev_api;
 	spinlock_t slock;
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 10/14] ARM: dts: Add camera to node exynos4.dtsi
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (8 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 09/14] s5p-fimc: Use pinctrl API for camera ports configuration Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 11/14] ARM: dts: Add ISP power domain node for Exynos4x12 Sylwester Nawrocki
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This patch adds common FIMC device nodes for all Exynos4 SoCs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi |   64 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 0ad4899..12b46e1 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -36,6 +36,12 @@
 		i2c5 = &i2c_5;
 		i2c6 = &i2c_6;
 		i2c7 = &i2c_7;
+		csis0 = &csis_0;
+		csis1 = &csis_1;
+		fimc0 = &fimc_0;
+		fimc1 = &fimc_1;
+		fimc2 = &fimc_2;
+		fimc3 = &fimc_3;
 	};
 
 	pd_mfc: mfc-power-domain@10023C40 {
@@ -110,6 +116,64 @@
 		status = "disabled";
 	};
 
+	camera {
+		compatible = "samsung,fimc", "simple-bus";
+		status = "disabled";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		fimc_0: fimc@11800000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11800000 0x1000>;
+			interrupts = <0 84 0>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+
+		fimc_1: fimc@11810000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11810000 0x1000>;
+			interrupts = <0 85 0>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+
+		fimc_2: fimc@11820000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11820000 0x1000>;
+			interrupts = <0 86 0>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+
+		fimc_3: fimc@11830000 {
+			compatible = "samsung,exynos4210-fimc";
+			reg = <0x11830000 0x1000>;
+			interrupts = <0 87 0>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+
+		csis_0: csis@11880000 {
+			compatible = "samsung,exynos4210-csis";
+			reg = <0x11880000 0x4000>;
+			interrupts = <0 78 0>;
+			bus-width = <4>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+
+		csis_1: csis@11890000 {
+			compatible = "samsung,exynos4210-csis";
+			reg = <0x11890000 0x4000>;
+			interrupts = <0 80 0>;
+			bus-width = <2>;
+			samsung,power-domain = <&pd_cam>;
+			status = "disabled";
+		};
+	};
+
 	watchdog@10060000 {
 		compatible = "samsung,s3c2410-wdt";
 		reg = <0x10060000 0x100>;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 11/14] ARM: dts: Add ISP power domain node for Exynos4x12
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (9 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 10/14] ARM: dts: Add camera to node exynos4.dtsi Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 12/14] ARM: dts: Add FIMC and MIPI CSIS device nodes " Sylwester Nawrocki
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

The ISP power domain is a common power domain for fimc-lite
and fimc-is (ISP) devices.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4x12.dtsi |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 06134c6..0293b6f 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -28,6 +28,11 @@
 		pinctrl3 = &pinctrl_3;
 	};
 
+	pd_isp: isp-power-domain@10023CA0 {
+		compatible = "samsung,exynos4210-pd";
+		reg = <0x10023CA0 0x20>;
+	};
+
 	combiner:interrupt-controller@10440000 {
 		interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
 			     <0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 12/14] ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (10 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 11/14] ARM: dts: Add ISP power domain node for Exynos4x12 Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 13/14] ARM: dts: Add camera pinctrl nodes for Exynos4x12 SoCs Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 14/14] ARM: dts: Add camera device nodes nodes for PQ board Sylwester Nawrocki
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

Add common camera node and fimc nodes specific to Exynos4212 and
Exynos4412 SoCs. fimc-is is a node for the Exynos4x12 FIMC-IS
subsystem and fimc-lite nodes are created as its child nodes,
among others due to FIMC-LITE device dependencies on FIMC-IS
related clocks.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4x12.dtsi |   47 +++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 0293b6f..4e57e11 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -26,6 +26,8 @@
 		pinctrl1 = &pinctrl_1;
 		pinctrl2 = &pinctrl_2;
 		pinctrl3 = &pinctrl_3;
+		fimc-lite0 = &fimc_lite_0;
+		fimc-lite1 = &fimc_lite_1;
 	};
 
 	pd_isp: isp-power-domain@10023CA0 {
@@ -220,4 +222,49 @@
 			reg = <0x10020704 0x8>;
 		};
 	};
+
+	camera {
+		fimc_0: fimc@11800000 {
+			compatible = "samsung,exynos4212-fimc";
+		};
+
+		fimc_1: fimc@11810000 {
+			compatible = "samsung,exynos4212-fimc";
+		};
+
+		fimc_2: fimc@11820000 {
+			compatible = "samsung,exynos4212-fimc";
+		};
+
+		fimc_3: fimc@11830000 {
+			compatible = "samsung,exynos4212-fimc";
+		};
+
+		fimc_is: fimc-is@12000000 {
+			compatible = "samsung,exynos4212-fimc-is", "simple-bus";
+			reg = <0x12000000 0x260000>;
+			interrupts = <0 90 0>, <0 95 0>;
+			samsung,power-domain = <&pd_isp>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			fimc_lite_0: fimc-lite@12390000 {
+				compatible = "samsung,exynos4212-fimc-lite";
+				reg = <0x12390000 0x1000>;
+				interrupts = <0 105 0>;
+				samsung,power-domain = <&pd_isp>;
+				status = "disabled";
+			};
+
+			fimc_lite_1: fimc-lite@123A0000 {
+				compatible = "samsung,exynos4212-fimc-lite";
+				reg = <0x123A0000 0x1000>;
+				interrupts = <0 106 0>;
+				samsung,power-domain = <&pd_isp>;
+				status = "disabled";
+			};
+		};
+	};
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 13/14] ARM: dts: Add camera pinctrl nodes for Exynos4x12 SoCs
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (11 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 12/14] ARM: dts: Add FIMC and MIPI CSIS device nodes " Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  2013-01-23 19:31 ` [PATCH RFC v3 14/14] ARM: dts: Add camera device nodes nodes for PQ board Sylwester Nawrocki
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

Add separate nodes for the CAMCLK pin and turn off pull-up on camera
ports A, B. The video bus pins and the clock output (CAMCLK) pin need
separate nodes since full camera port is not used in some configurations,
e.g. for MIPI CSI-2 bus on CAMCLK is required and data/clock signal
use separate dedicated pins.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4x12-pinctrl.dtsi |   33 +++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
index 8e6115a..b70eced 100644
--- a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
@@ -401,15 +401,28 @@
 			samsung,pin-drv = <0>;
 		};
 
-		cam_port_a: cam-port-a {
+		cam_port_a_io: cam-port-a-io {
 			samsung,pins = "gpj0-0", "gpj0-1", "gpj0-2", "gpj0-3",
 					"gpj0-4", "gpj0-5", "gpj0-6", "gpj0-7",
-					"gpj1-0", "gpj1-1", "gpj1-2", "gpj1-3",
-					"gpj1-4";
+					"gpj1-0", "gpj1-1", "gpj1-2", "gpj1-4";
 			samsung,pin-function = <2>;
-			samsung,pin-pud = <3>;
+			samsung,pin-pud = <0>;
 			samsung,pin-drv = <0>;
 		};
+
+		cam_port_a_clk_active: cam-port-a-clk-active {
+			samsung,pins = "gpj1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <3>;
+		};
+
+		cam_port_a_clk_idle: cam-port-a-clk-idle {
+			samsung,pins = "gpj1-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <3>;
+		};
 	};
 
 	pinctrl@11000000 {
@@ -834,11 +847,17 @@
 			samsung,pin-drv = <0>;
 		};
 
-		cam_port_b: cam-port-b {
+		cam_port_b_io: cam-port-b-io {
 			samsung,pins = "gpm0-0", "gpm0-1", "gpm0-2", "gpm0-3",
 					"gpm0-4", "gpm0-5", "gpm0-6", "gpm0-7",
-					"gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1",
-					"gpm2-2";
+					"gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1";
+			samsung,pin-function = <3>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};
+
+		cam_port_b_clk: cam-port-b-clk {
+			samsung,pins = "gpm2-2";
 			samsung,pin-function = <3>;
 			samsung,pin-pud = <3>;
 			samsung,pin-drv = <0>;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC v3 14/14] ARM: dts: Add camera device nodes nodes for PQ board
  2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
                   ` (12 preceding siblings ...)
  2013-01-23 19:31 ` [PATCH RFC v3 13/14] ARM: dts: Add camera pinctrl nodes for Exynos4x12 SoCs Sylwester Nawrocki
@ 2013-01-23 19:31 ` Sylwester Nawrocki
  13 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-23 19:31 UTC (permalink / raw)
  To: linux-media
  Cc: hverkuil, g.liakhovetski, laurent.pinchart, kyungmin.park,
	kgene.kim, grant.likely, rob.herring, thomas.abraham, t.figa,
	myungjoo.ham, sw0312.kim, prabhakar.lad, devicetree-discuss,
	linux-samsung-soc, Sylwester Nawrocki

This patch adds all nodes for camera devices on an example Exynos4412 SoC
based board. This is all what's required in the board dts file to enable
rear facing camera (S5C73M3 sensor).

The aliases node contains entries required for the camera processing
data path entity drivers.

The sensor nodes use standard port/remote-endpoint nodes convention.
Internal SoC links between entities are not specified this way and
are coded in the driver instead.

The S5C73M3 sensor uses two control buses: I2C and SPI. There are
two, i2c_0 and spi_1 bus controller child nodes assigned to it.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

This patch is intended as an example only.
---
 arch/arm/boot/dts/exynos4412-slp_pq.dts |  169 +++++++++++++++++++++++++++++++
 1 file changed, 169 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-slp_pq.dts b/arch/arm/boot/dts/exynos4412-slp_pq.dts
index 0ae5162..731fc3d 100644
--- a/arch/arm/boot/dts/exynos4412-slp_pq.dts
+++ b/arch/arm/boot/dts/exynos4412-slp_pq.dts
@@ -113,6 +113,35 @@
 		};
 	};
 
+	i2c_0: i2c@13860000 {
+		samsung,i2c-sda-delay = <100>;
+		samsung,i2c-slave-addr = <0x10>;
+		samsung,i2c-max-bus-freq = <400000>;
+		pinctrl-0 = <&i2c0_bus>;
+		pinctrl-names = "default";
+		status = "okay";
+
+		s5c73m3@3c {
+			compatible = "samsung,s5c73m3";
+			reg = <0x3c>;
+			gpios = <&gpm0 1 1>, /* ISP_STANDBY */
+				<&gpf1 3 1>; /* ISP_RESET */
+			vdd-int-supply = <&buck9_reg>;
+			vddio-cis-supply = <&ldo9_reg>;
+			vdda-supply = <&ldo17_reg>;
+			vddio-host-supply = <&ldo18_reg>;
+			vdd-af-supply = <&cam_af_reg>;
+			vdd-reg-supply = <&cam_io_reg>;
+			clock-frequency = <24000000>;
+
+			port {
+				s5c73m3_ep: endpoint {
+					remote-endpoint = <&csis0_ep>;
+				};
+			};
+		};
+	};
+
 	i2c@13890000 {
 		samsung,i2c-sda-delay = <100>;
 		samsung,i2c-slave-addr = <0x10>;
@@ -425,6 +454,34 @@
 		enable-active-high;
 	};
 
+	cam_af_reg: voltage-regulator@2 {
+	        compatible = "regulator-fixed";
+		regulator-name = "CAM_AF";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		gpio = <&gpm0 4 0>;
+		enable-active-high;
+	};
+
+	cam_io_reg: voltage-regulator@3 {
+	        compatible = "regulator-fixed";
+		regulator-name = "CAM_SENSOR_A";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		gpio = <&gpm0 2 0>;
+		enable-active-high;
+	};
+
+	cam_isp_core_reg: voltage-regulator@4 {
+	        compatible = "regulator-fixed";
+		regulator-name = "CAM_ISP_CORE_1.2V_EN";
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		gpio = <&gpm0 3 0>;
+		enable-active-high;
+		regulator-always-on;
+	};
+
 	fimd0_lcd: panel {
 		compatible = "s6e8ax0";
 		reset-gpio = <&gpy4 5 0>;
@@ -484,4 +541,116 @@
 		vusb_d-supply = <&ldo15_reg>;
 		vusb_a-supply = <&ldo12_reg>;
 	};
+
+	spi_1: spi@13930000 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&spi1_bus>;
+		status = "okay";
+
+		s5c73m3_spi: s5c73m3 {
+			compatible = "samsung,s5c73m3";
+			spi-max-frequency = <50000000>;
+			reg = <0>;
+			controller-data {
+				cs-gpio = <&gpb 5 0>;
+				samsung,spi-feedback-delay = <2>;
+			};
+		};
+	};
+
+	camera {
+		compatible = "samsung,fimc", "simple-bus";
+		status = "okay";
+
+		pinctrl-names = "default", "inactive";
+		pinctrl-0 = <&cam_port_a_clk_active>;
+		pinctrl-1 = <&cam_port_a_clk_idle>;
+
+		fimc_0: fimc@11800000 {
+			clock-frequency = <176000000>;
+			status = "okay";
+		};
+
+		fimc_1: fimc@11810000 {
+			clock-frequency = <176000000>;
+			status = "okay";
+		};
+
+		fimc_2: fimc@11820000 {
+			clock-frequency = <176000000>;
+			status = "okay";
+		};
+
+		fimc_3: fimc@11830000 {
+			clock-frequency = <176000000>;
+			status = "okay";
+		};
+
+		csis_0: csis@11880000 {
+			status = "okay";
+			vddcore-supply = <&ldo8_reg>;
+			vddio-supply = <&ldo10_reg>;
+			clock-frequency = <160000000>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port {
+				reg = <3>;  /* Camera C(3) MIPI */
+				csis0_ep: endpoint {
+					remote-endpoint = <&s5c73m3_ep>;
+					data-lanes = <1>, <2>, <3>, <4>;
+					samsung,csis-hs-settle = <12>;
+				};
+			};
+		};
+
+		csis_1: csis@11890000 {
+			status = "okay";
+			vddcore-supply = <&ldo8_reg>;
+			vddio-supply = <&ldo10_reg>;
+			clock-frequency = <160000000>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port {
+				reg = <4>;  /* Camera D (4) CSIS1 */
+				csis1_ep: endpoint {
+					remote-endpoint = <&is_s5k6a3_ep>;
+					data-lanes = <1>;
+					samsung,csis-hs-settle = <18>;
+					samsung,csis-wclk;
+				};
+			};
+		};
+
+		fimc-is@12000000 {
+			status = "okay";
+
+			fimc_lite_0: fimc-lite@12390000 {
+				status = "okay";
+			};
+
+			fimc_lite_1: fimc-lite@123A0000 {
+				status = "okay";
+			};
+
+			fimc-is-i2c@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				s5k6a3@10 {
+					compatible = "samsung,s5k6a3";
+					reg = <0x10>;
+					svdda-supply = <&cam_io_reg>;
+					svddio-supply = <&ldo19_reg>;
+					clock-frequency = <24000000>;
+					gpios = <&gpm1 6 0>;
+					port {
+						is_s5k6a3_ep: endpoint {
+							remote-endpoint = <&csis1_ep>;
+							data-lanes = <1>;
+						};
+					};
+				};
+			};
+		};
+	};
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-23 19:31 ` [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation Sylwester Nawrocki
@ 2013-01-24 10:16   ` Laurent Pinchart
  2013-01-24 18:30     ` Sylwester Nawrocki
  0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2013-01-24 10:16 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-media, hverkuil, g.liakhovetski, kyungmin.park, kgene.kim,
	grant.likely, rob.herring, thomas.abraham, t.figa, myungjoo.ham,
	sw0312.kim, prabhakar.lad, devicetree-discuss, linux-samsung-soc

Hi Sylwester,

Thanks for the patch.

On Wednesday 23 January 2013 20:31:16 Sylwester Nawrocki wrote:
> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> This patch adds a document describing common OF bindings for video
> capture, output and video processing devices. It is curently mainly
> focused on video capture devices, with data busses defined by
> standards like ITU-R BT.656 or MIPI-CSI2.
> It also documents a method of describing data links between devices.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> ---
> 
> Changes since v3:
>  - improved clock-lanes property description,
>  - grammar corrections of the example dts snippet description.
> ---
>  .../devicetree/bindings/media/video-interfaces.txt |  204 +++++++++++++++++
>  1 file changed, 204 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/media/video-interfaces.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> b/Documentation/devicetree/bindings/media/video-interfaces.txt new file
> mode 100644
> index 0000000..0da126f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -0,0 +1,204 @@
> +Common bindings for video data receiver and transmitter interfaces
> +
> +General concept
> +---------------
> +
> +Video data pipelines usually consist of external devices, e.g. camera
> +sensors, controlled over an I2C, SPI or UART bus, and SoC internal IP
> +blocks, including video DMA engines and video data processors.
> +
> +SoC internal blocks are described by DT nodes, placed similarly to other
> +SoC blocks.  External devices are represented as child nodes of their
> +respective bus controller nodes, e.g. I2C.
> +
> +Data interfaces on all video devices are described by their child 'port'
> +nodes. Configuration of a port depends on other devices participating in
> +the data transfer and is described by 'endpoint' subnodes.
> +
> +dev {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	port@0 {
> +		endpoint@0 { ... };
> +		endpoint@1 { ... };
> +	};
> +	port@1 { ... };
> +};
> +
> +If a port can be configured to work with more than one other device on the
> +same bus, an 'endpoint' child node must be provided for each of them.  If
> +more than one port is present in a device node or there is more than one
> +endpoint at a port, a common scheme, using '#address-cells', '#size-cells'
> +and 'reg' properties is used.

Wouldn't this cause problems if the device has both video ports and a child 
bus ? Using #address-cells and #size-cells for the video ports would prevent 
the child bus from being handled in the usual way.

A possible solution would be to number ports with a dash instead of a @, as 
done in pinctrl for instance. We would then get

	port-0 {
		endpoint-0 { ... };
		endpoint-1 { ... };
	};
	port-1 { ... };

> +Two 'endpoint' nodes are linked with each other through their
> +'remote-endpoint' phandles.  An endpoint subnode of a device contains all
> +properties needed for configuration of this device for data exchange with
> +the other device.  In most cases properties at the peer 'endpoint' nodes
> +will be identical, however they might need to be different when there is
> +any signal modifications on the bus between two devices, e.g. there are
> +logic signal inverters on the lines.
> +
> +Required properties
> +-------------------
> +
> +If there is more than one 'port' or more than one 'endpoint' node following
> +properties are required in relevant parent node:
> +
> +- #address-cells : number of cells required to define port number, should
> be 1.
> +- #size-cells    : should be zero.

I wonder if we should specify whether a port is a data sink or data source. A 
source can be connected to multiple sinks at the same time, but a sink can 
only be connected to a single source. If we want to perform automatic sanity 
checks in the core knowing the direction might help.

> +Optional endpoint properties
> +----------------------------
> +
> +- remote-endpoint: phandle to an 'endpoint' subnode of the other device
> +  node.
> +- slave-mode: a boolean property, run the link in slave mode.
> +  Default is master mode.

What are master and slave modes ? It might be worth it describing them.

> +- bus-width: number of data lines, valid for parallel busses.
> +- data-shift: on parallel data busses, if bus-width is used to specify the
> +  number of data lines, data-shift can be used to specify which data lines
> +  are used, e.g. "bus-width=<10>; data-shift=<2>;" means, that lines 9:2
> +  are used.
> +- hsync-active: active state of HSYNC signal, 0/1 for LOW/HIGH
> +  respectively.
> +- vsync-active: active state of VSYNC signal, 0/1 for LOW/HIGH
> +  respectively. Note, that if HSYNC and VSYNC polarities are not
> +  specified, embedded synchronization may be required, where supported.
> +- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
> +- field-even-active: field signal level during the even field data
> +  transmission.
> +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel
> +  clock signal.
> +- data-lanes: an array of physical data lane indexes. Position of an entry
> +  determines the logical lane number, while the value of an entry indicates
> +  physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
> +  "data-lanes = <1>, <2>;", assuming the clock lane is on hardware lane 0.
> +  This property is valid for serial busses only (e.g. MIPI CSI-2).
> +- clock-lanes: an array of physical clock lane indexes. Position of an
> +  entry determines the logical lane number, while the value of an entry
> +  indicates physical lane, e.g. for a MIPI CSI-2 bus we could have
> +  "clock-lanes = <0>;", which places the clock lane on hardware lane 0.
> +  This property is valid for serial busses only (e.g. MIPI CSI-2). Note
> +  that for the MIPI CSI-2 bus this array contains only one entry.
> +- clock-noncontinuous: a boolean property to allow MIPI CSI-2
> +  non-continuous clock mode.
> +
> +Example
> +-------
> +
> +The example snippet below describes two data pipelines.  ov772x and imx074
> +are camera sensors with a parallel and serial (MIPI CSI-2) video bus
> +respectively. Both sensors are on the I2C control bus corresponding to the
> +i2c0 controller node.  ov772x sensor is linked directly to the ceu0 video
> +host interface. imx074 is linked to ceu0 through the MIPI CSI-2 receiver
> +(csi2). ceu0 has a (single) DMA engine writing captured data to memory. 
> +ceu0 node has a single 'port' node which indicates that at any time only
> +one of the following data pipelines can be active: ov772x -> ceu0 or
> +imx074 -> csi2 -> ceu0.
> +
> +	ceu0: ceu@0xfe910000 {
> +		compatible = "renesas,sh-mobile-ceu";
> +		reg = <0xfe910000 0xa0>;
> +		interrupts = <0x880>;
> +
> +		mclk: master_clock {
> +			compatible = "renesas,ceu-clock";
> +			#clock-cells = <1>;
> +			clock-frequency = <50000000>;	/* Max clock frequency */
> +			clock-output-names = "mclk";
> +		};
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ceu0_1: endpoint@1 {
> +				reg = <1>;		/* Local endpoint # */
> +				remote = <&ov772x_1_1>;	/* Remote phandle */
> +				bus-width = <8>;	/* Used data lines */
> +				data-shift = <0>;	/* Lines 7:0 are used */

As data-shift is optional, shouldn't it be left out when equal to 0 ? It 
would, however, be nice to have a non-zero data-shift somewhere in the 
example.

> +
> +				/* If hsync-active/vsync-active are missing,
> +				   embedded bt.605 sync is used */
> +				hsync-active = <1>;	/* Active high */
> +				vsync-active = <1>;	/* Active high */
> +				data-active = <1>;	/* Active high */
> +				pclk-sample = <1>;	/* Rising */
> +			};
> +
> +			ceu0_0: endpoint@0 {
> +				reg = <0>;
> +				remote = <&csi2_2>;
> +				immutable;

What is the immutable property for her e?

> +			};
> +		};
> +	};
> +
> +	i2c0: i2c@0xfff20000 {
> +		...
> +		ov772x_1: camera@0x21 {
> +			compatible = "omnivision,ov772x";
> +			reg = <0x21>;
> +			vddio-supply = <&regulator1>;
> +			vddcore-supply = <&regulator2>;
> +
> +			clock-frequency = <20000000>;
> +			clocks = <&mclk 0>;
> +			clock-names = "xclk";
> +
> +			port {
> +				/* With 1 endpoint per port no need in addresses. */

s/in/for/ ?

> +				ov772x_1_1: endpoint {
> +					bus-width = <8>;
> +					remote-endpoint = <&ceu0_1>;
> +					hsync-active = <1>;
> +					vsync-active = <0>; /* Who came up with an
> +							       inverter here ?... */
> +					data-active = <1>;
> +					pclk-sample = <1>;
> +				};
> +			};
> +		};
> +
> +		imx074: camera@0x1a {
> +			compatible = "sony,imx074";
> +			reg = <0x1a>;
> +			vddio-supply = <&regulator1>;
> +			vddcore-supply = <&regulator2>;
> +
> +			clock-frequency = <30000000>;	/* Shared clock with ov772x_1 */
> +			clocks = <&mclk 0>;
> +			clock-names = "sysclk";		/* Assuming this is the
> +							   name in the datasheet */
> +			port {
> +				imx074_1: endpoint {
> +					clock-lanes = <0>;
> +					data-lanes = <1>, <2>;
> +					remote-endpoint = <&csi2_1>;
> +				};
> +			};
> +		};
> +	};
> +
> +	csi2: csi2@0xffc90000 {
> +		compatible = "renesas,sh-mobile-csi2";
> +		reg = <0xffc90000 0x1000>;
> +		interrupts = <0x17a0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@1 {
> +			compatible = "renesas,csi2c";	/* One of CSI2I and CSI2C. */
> +			reg = <1>;			/* CSI-2 PHY #1 of 2: PHY_S,
> +							   PHY_M has port address 0,
> +							   is unused. */
> +			csi2_1: endpoint {
> +				clock-lanes = <0>;
> +				data-lanes = <2>, <1>;
> +				remote-endpoint = <&imx074_1>;
> +			};
> +		};
> +		port@2 {
> +			reg = <2>;			/* port 2: link to the CEU */
> +
> +			csi2_2: endpoint {
> +				immutable;
> +				remote-endpoint = <&ceu0_0>;
> +			};
> +		};
> +	};

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-24 10:16   ` Laurent Pinchart
@ 2013-01-24 18:30     ` Sylwester Nawrocki
  2013-01-25  1:52       ` Laurent Pinchart
  0 siblings, 1 reply; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-24 18:30 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, hverkuil, g.liakhovetski, kyungmin.park, kgene.kim,
	grant.likely, rob.herring, thomas.abraham, t.figa, myungjoo.ham,
	sw0312.kim, prabhakar.lad, devicetree-discuss, linux-samsung-soc

Hi Laurent,

Thanks for the review.

On 01/24/2013 11:16 AM, Laurent Pinchart wrote:
[...]
>> +Data interfaces on all video devices are described by their child 'port'
>> +nodes. Configuration of a port depends on other devices participating in
>> +the data transfer and is described by 'endpoint' subnodes.
>> +
>> +dev {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	port@0 {
>> +		endpoint@0 { ... };
>> +		endpoint@1 { ... };
>> +	};
>> +	port@1 { ... };
>> +};
>> +
>> +If a port can be configured to work with more than one other device on the
>> +same bus, an 'endpoint' child node must be provided for each of them.  If
>> +more than one port is present in a device node or there is more than one
>> +endpoint at a port, a common scheme, using '#address-cells', '#size-cells'
>> +and 'reg' properties is used.
> 
> Wouldn't this cause problems if the device has both video ports and a child 
> bus ? Using #address-cells and #size-cells for the video ports would prevent 
> the child bus from being handled in the usual way.

Indeed, it looks like a serious issue in these bindings.

> A possible solution would be to number ports with a dash instead of a @, as 
> done in pinctrl for instance. We would then get
> 
> 	port-0 {
> 		endpoint-0 { ... };
> 		endpoint-1 { ... };
> 	};
> 	port-1 { ... };

Sounds like a good alternative, I can't think of any better solution at the
moment.

>> +Two 'endpoint' nodes are linked with each other through their
>> +'remote-endpoint' phandles.  An endpoint subnode of a device contains all
>> +properties needed for configuration of this device for data exchange with
>> +the other device.  In most cases properties at the peer 'endpoint' nodes
>> +will be identical, however they might need to be different when there is
>> +any signal modifications on the bus between two devices, e.g. there are
>> +logic signal inverters on the lines.
>> +
>> +Required properties
>> +-------------------
>> +
>> +If there is more than one 'port' or more than one 'endpoint' node following
>> +properties are required in relevant parent node:
>> +
>> +- #address-cells : number of cells required to define port number, should
>> be 1.
>> +- #size-cells    : should be zero.
> 
> I wonder if we should specify whether a port is a data sink or data source. A 
> source can be connected to multiple sinks at the same time, but a sink can 
> only be connected to a single source. If we want to perform automatic sanity 
> checks in the core knowing the direction might help.

Multiple sources can be linked to a single sink, but only one link can be 
active at any time.

So I'm not sure if knowing if a DT port is a data source or data sink would
let us to validate device tree structure statically in general.

Such source/sink property could be useful later at runtime, when data pipeline
is set up for streaming.

How do you think this could be represented ? By just having boolean 
properties like: 'source' and 'sink' in the port nodes ? Or perhaps in the 
endpoint nodes, since some devices might be bidirectional ? I don't recall 
any at the moment though.

>> +Optional endpoint properties
>> +----------------------------
>> +
>> +- remote-endpoint: phandle to an 'endpoint' subnode of the other device
>> +  node.
>> +- slave-mode: a boolean property, run the link in slave mode.
>> +  Default is master mode.
> 
> What are master and slave modes ? It might be worth it describing them.

This was originally proposed by Guennadi, I think he knows exactly what's
the meaning of this property. I'll dig into relevant documentation to 
find out and provide more detailed description.

>> +- bus-width: number of data lines, valid for parallel busses.
>> +- data-shift: on parallel data busses, if bus-width is used to specify the
>> +  number of data lines, data-shift can be used to specify which data lines
>> +  are used, e.g. "bus-width=<10>; data-shift=<2>;" means, that lines 9:2
>> +  are used.
>> +- hsync-active: active state of HSYNC signal, 0/1 for LOW/HIGH
>> +  respectively.
>> +- vsync-active: active state of VSYNC signal, 0/1 for LOW/HIGH
>> +  respectively. Note, that if HSYNC and VSYNC polarities are not
>> +  specified, embedded synchronization may be required, where supported.
>> +- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
>> +- field-even-active: field signal level during the even field data
>> +  transmission.
>> +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel
>> +  clock signal.
>> +- data-lanes: an array of physical data lane indexes. Position of an entry
>> +  determines the logical lane number, while the value of an entry indicates
>> +  physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
>> +  "data-lanes = <1>, <2>;", assuming the clock lane is on hardware lane 0.
>> +  This property is valid for serial busses only (e.g. MIPI CSI-2).
>> +- clock-lanes: an array of physical clock lane indexes. Position of an
>> +  entry determines the logical lane number, while the value of an entry
>> +  indicates physical lane, e.g. for a MIPI CSI-2 bus we could have
>> +  "clock-lanes = <0>;", which places the clock lane on hardware lane 0.
>> +  This property is valid for serial busses only (e.g. MIPI CSI-2). Note
>> +  that for the MIPI CSI-2 bus this array contains only one entry.
>> +- clock-noncontinuous: a boolean property to allow MIPI CSI-2
>> +  non-continuous clock mode.
>> +
>> +Example
>> +-------
>> +
>> +The example snippet below describes two data pipelines.  ov772x and imx074
>> +are camera sensors with a parallel and serial (MIPI CSI-2) video bus
>> +respectively. Both sensors are on the I2C control bus corresponding to the
>> +i2c0 controller node.  ov772x sensor is linked directly to the ceu0 video
>> +host interface. imx074 is linked to ceu0 through the MIPI CSI-2 receiver
>> +(csi2). ceu0 has a (single) DMA engine writing captured data to memory. 
>> +ceu0 node has a single 'port' node which indicates that at any time only
>> +one of the following data pipelines can be active: ov772x -> ceu0 or
>> +imx074 -> csi2 -> ceu0.
>> +
>> +	ceu0: ceu@0xfe910000 {
>> +		compatible = "renesas,sh-mobile-ceu";
>> +		reg = <0xfe910000 0xa0>;
>> +		interrupts = <0x880>;
>> +
>> +		mclk: master_clock {
>> +			compatible = "renesas,ceu-clock";
>> +			#clock-cells = <1>;
>> +			clock-frequency = <50000000>;	/* Max clock frequency */
>> +			clock-output-names = "mclk";
>> +		};
>> +
>> +		port {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			ceu0_1: endpoint@1 {
>> +				reg = <1>;		/* Local endpoint # */
>> +				remote = <&ov772x_1_1>;	/* Remote phandle */
>> +				bus-width = <8>;	/* Used data lines */
>> +				data-shift = <0>;	/* Lines 7:0 are used */
> 
> As data-shift is optional, shouldn't it be left out when equal to 0 ? It 
> would, however, be nice to have a non-zero data-shift somewhere in the 
> example.

Yes, good point. data-shift could be ommited. I'm going to increase the 
bus-width and make data-shit non-zero.

>> +
>> +				/* If hsync-active/vsync-active are missing,
>> +				   embedded bt.605 sync is used */
>> +				hsync-active = <1>;	/* Active high */
>> +				vsync-active = <1>;	/* Active high */
>> +				data-active = <1>;	/* Active high */
>> +				pclk-sample = <1>;	/* Rising */
>> +			};
>> +
>> +			ceu0_0: endpoint@0 {
>> +				reg = <0>;
>> +				remote = <&csi2_2>;
>> +				immutable;
> 
> What is the immutable property for her e?

I was staring at this yesterday and finally I forgot to remove it. It is
undocumented and I think it's not supposed to be here. Guennadi, would
you have any comments on that ?

>> +			};
>> +		};
>> +	};
>> +
>> +	i2c0: i2c@0xfff20000 {
>> +		...
>> +		ov772x_1: camera@0x21 {
>> +			compatible = "omnivision,ov772x";
>> +			reg = <0x21>;
>> +			vddio-supply = <&regulator1>;
>> +			vddcore-supply = <&regulator2>;
>> +
>> +			clock-frequency = <20000000>;
>> +			clocks = <&mclk 0>;
>> +			clock-names = "xclk";
>> +
>> +			port {
>> +				/* With 1 endpoint per port no need in addresses. */
> 
> s/in/for/ ?

I proposed same change to Guennadi, but he argued that "in" is also
commonly used. I agreed even though 'for' seemed more natural to me.
I would change it, unless there is a strong opposition. :)

--

Regards,
Sylwester

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-24 18:30     ` Sylwester Nawrocki
@ 2013-01-25  1:52       ` Laurent Pinchart
  2013-01-30 12:40         ` Sylwester Nawrocki
  0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2013-01-25  1:52 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-media, hverkuil, g.liakhovetski, kyungmin.park, kgene.kim,
	grant.likely, rob.herring, thomas.abraham, t.figa, myungjoo.ham,
	sw0312.kim, prabhakar.lad, devicetree-discuss, linux-samsung-soc

Hi Sylwester,

On Thursday 24 January 2013 19:30:10 Sylwester Nawrocki wrote:
> On 01/24/2013 11:16 AM, Laurent Pinchart wrote:
> [...]
> 
> >> +Data interfaces on all video devices are described by their child 'port'
> >> +nodes. Configuration of a port depends on other devices participating in
> >> +the data transfer and is described by 'endpoint' subnodes.
> >> +
> >> +dev {
> >> +	#address-cells = <1>;
> >> +	#size-cells = <0>;
> >> +	port@0 {
> >> +		endpoint@0 { ... };
> >> +		endpoint@1 { ... };
> >> +	};
> >> +	port@1 { ... };
> >> +};
> >> +
> >> +If a port can be configured to work with more than one other device on
> >> +the same bus, an 'endpoint' child node must be provided for each of
> >> +them. If more than one port is present in a device node or there is more
> >> +than one endpoint at a port, a common scheme, using '#address-cells',
> >> +'#size-cells' and 'reg' properties is used.
> > 
> > Wouldn't this cause problems if the device has both video ports and a
> > child bus ? Using #address-cells and #size-cells for the video ports would
> > prevent the child bus from being handled in the usual way.
> 
> Indeed, it looks like a serious issue in these bindings.
> 
> > A possible solution would be to number ports with a dash instead of a @,
> > as done in pinctrl for instance. We would then get
> > 
> > 	port-0 {
> > 		endpoint-0 { ... };
> > 		endpoint-1 { ... };
> > 	};
> > 	port-1 { ... };
> 
> Sounds like a good alternative, I can't think of any better solution at the
> moment.
> 
> >> +Two 'endpoint' nodes are linked with each other through their
> >> +'remote-endpoint' phandles.  An endpoint subnode of a device contains
> >> +all properties needed for configuration of this device for data exchange
> >> +with the other device.  In most cases properties at the peer 'endpoint'
> >> +nodes will be identical, however they might need to be different when
> >> +there is any signal modifications on the bus between two devices, e.g.
> >> +there are logic signal inverters on the lines.
> >> +
> >> +Required properties
> >> +-------------------
> >> +
> >> +If there is more than one 'port' or more than one 'endpoint' node
> >> following +properties are required in relevant parent node:
> >> +
> >> +- #address-cells : number of cells required to define port number,
> >> should be 1.
> >> +- #size-cells    : should be zero.
> > 
> > I wonder if we should specify whether a port is a data sink or data
> > source. A source can be connected to multiple sinks at the same time, but
> > a sink can only be connected to a single source. If we want to perform
> > automatic sanity checks in the core knowing the direction might help.
> 
> Multiple sources can be linked to a single sink, but only one link can be
> active at any time.
> 
> So I'm not sure if knowing if a DT port is a data source or data sink would
> let us to validate device tree structure statically in general.
>
> Such source/sink property could be useful later at runtime, when data
> pipeline is set up for streaming.

Yes, I was mostly thinking about runtime.

> How do you think this could be represented ? By just having boolean
> properties like: 'source' and 'sink' in the port nodes ? Or perhaps in the
> endpoint nodes, since some devices might be bidirectional ? I don't recall
> any at the moment though.

Source and sink properties would do. We could also use a direction property 
that could take sink, source and bidirectional values, but that might be more 
complex.

I don't think we will have bidirectional link (as that would most probably 
involve a very different kind of bus, and thus new bindings).

> >> +Optional endpoint properties
> >> +----------------------------
> >> +
> >> +- remote-endpoint: phandle to an 'endpoint' subnode of the other device
> >> +  node.
> >> +- slave-mode: a boolean property, run the link in slave mode.
> >> +  Default is master mode.
> > 
> > What are master and slave modes ? It might be worth it describing them.
> 
> This was originally proposed by Guennadi, I think he knows exactly what's
> the meaning of this property. I'll dig into relevant documentation to
> find out and provide more detailed description.

Thank you.

> >> +- bus-width: number of data lines, valid for parallel busses.
> >> +- data-shift: on parallel data busses, if bus-width is used to specify
> >> +  the number of data lines, data-shift can be used to specify which data
> >> +  lines are used, e.g. "bus-width=<10>; data-shift=<2>;" means, that
> >> +  lines 9:2 are used.
> >> +- hsync-active: active state of HSYNC signal, 0/1 for LOW/HIGH
> >> +  respectively.
> >> +- vsync-active: active state of VSYNC signal, 0/1 for LOW/HIGH
> >> +  respectively. Note, that if HSYNC and VSYNC polarities are not
> >> +  specified, embedded synchronization may be required, where supported.
> >> +- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
> >> +- field-even-active: field signal level during the even field data
> >> +  transmission.
> >> +- pclk-sample: sample data on rising (1) or falling (0) edge of the
> >> +  pixel clock signal.
> >> +- data-lanes: an array of physical data lane indexes. Position of an
> >> +  entry determines the logical lane number, while the value of an entry
> >> +  indicates physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
> >> +  "data-lanes = <1>, <2>;", assuming the clock lane is on hardware lane
> >> +  0. This property is valid for serial busses only (e.g. MIPI CSI-2).
> >> +- clock-lanes: an array of physical clock lane indexes. Position of an
> >> +  entry determines the logical lane number, while the value of an entry
> >> +  indicates physical lane, e.g. for a MIPI CSI-2 bus we could have
> >> +  "clock-lanes = <0>;", which places the clock lane on hardware lane 0.
> >> +  This property is valid for serial busses only (e.g. MIPI CSI-2). Note
> >> +  that for the MIPI CSI-2 bus this array contains only one entry.
> >> +- clock-noncontinuous: a boolean property to allow MIPI CSI-2
> >> +  non-continuous clock mode.
> >> +
> >> +Example
> >> +-------
> >> +
> >> +The example snippet below describes two data pipelines.  ov772x and
> >> +imx074 are camera sensors with a parallel and serial (MIPI CSI-2) video
> >> +bus respectively. Both sensors are on the I2C control bus corresponding
> >> +to the i2c0 controller node.  ov772x sensor is linked directly to the
> >> +ceu0 video host interface. imx074 is linked to ceu0 through the MIPI
> >> +CSI-2 receiver (csi2). ceu0 has a (single) DMA engine writing captured
> >> +data to memory.  ceu0 node has a single 'port' node which indicates that
> >> +at any time only one of the following data pipelines can be active:
> >> +ov772x -> ceu0 or imx074 -> csi2 -> ceu0.
> >> +
> >> +	ceu0: ceu@0xfe910000 {
> >> +		compatible = "renesas,sh-mobile-ceu";
> >> +		reg = <0xfe910000 0xa0>;
> >> +		interrupts = <0x880>;
> >> +
> >> +		mclk: master_clock {
> >> +			compatible = "renesas,ceu-clock";
> >> +			#clock-cells = <1>;
> >> +			clock-frequency = <50000000>;	/* Max clock frequency */
> >> +			clock-output-names = "mclk";
> >> +		};
> >> +
> >> +		port {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			ceu0_1: endpoint@1 {
> >> +				reg = <1>;		/* Local endpoint # */
> >> +				remote = <&ov772x_1_1>;	/* Remote phandle */
> >> +				bus-width = <8>;	/* Used data lines */
> >> +				data-shift = <0>;	/* Lines 7:0 are used */
> > 
> > As data-shift is optional, shouldn't it be left out when equal to 0 ? It
> > would, however, be nice to have a non-zero data-shift somewhere in the
> > example.
> 
> Yes, good point. data-shift could be ommited. I'm going to increase the
> bus-width and make data-shit non-zero.
> 
> >> +
> >> +				/* If hsync-active/vsync-active are missing,
> >> +				   embedded bt.605 sync is used */
> >> +				hsync-active = <1>;	/* Active high */
> >> +				vsync-active = <1>;	/* Active high */
> >> +				data-active = <1>;	/* Active high */
> >> +				pclk-sample = <1>;	/* Rising */
> >> +			};
> >> +
> >> +			ceu0_0: endpoint@0 {
> >> +				reg = <0>;
> >> +				remote = <&csi2_2>;
> >> +				immutable;
> > 
> > What is the immutable property for her e?
> 
> I was staring at this yesterday and finally I forgot to remove it. It is
> undocumented and I think it's not supposed to be here. Guennadi, would
> you have any comments on that ?
> 
> >> +			};
> >> +		};
> >> +	};
> >> +
> >> +	i2c0: i2c@0xfff20000 {
> >> +		...
> >> +		ov772x_1: camera@0x21 {
> >> +			compatible = "omnivision,ov772x";
> >> +			reg = <0x21>;
> >> +			vddio-supply = <&regulator1>;
> >> +			vddcore-supply = <&regulator2>;
> >> +
> >> +			clock-frequency = <20000000>;
> >> +			clocks = <&mclk 0>;
> >> +			clock-names = "xclk";
> >> +
> >> +			port {
> >> +				/* With 1 endpoint per port no need in addresses. */
> > 
> > s/in/for/ ?
> 
> I proposed same change to Guennadi, but he argued that "in" is also
> commonly used. I agreed even though 'for' seemed more natural to me.
> I would change it, unless there is a strong opposition. :)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-25  1:52       ` Laurent Pinchart
@ 2013-01-30 12:40         ` Sylwester Nawrocki
  2013-01-30 22:35           ` Sylwester Nawrocki
  0 siblings, 1 reply; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-30 12:40 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, hverkuil, g.liakhovetski, kyungmin.park, kgene.kim,
	grant.likely, rob.herring, thomas.abraham, t.figa, myungjoo.ham,
	sw0312.kim, prabhakar.lad, devicetree-discuss, linux-samsung-soc

Hi Laurent,

On 01/25/2013 02:52 AM, Laurent Pinchart wrote:
>>>> +Data interfaces on all video devices are described by their child 'port'
>>>> +nodes. Configuration of a port depends on other devices participating in
>>>> +the data transfer and is described by 'endpoint' subnodes.
>>>> +
>>>> +dev {
>>>> +	#address-cells = <1>;
>>>> +	#size-cells = <0>;
>>>> +	port@0 {
>>>> +		endpoint@0 { ... };
>>>> +		endpoint@1 { ... };
>>>> +	};
>>>> +	port@1 { ... };
>>>> +};
>>>> +
>>>> +If a port can be configured to work with more than one other device on
>>>> +the same bus, an 'endpoint' child node must be provided for each of
>>>> +them. If more than one port is present in a device node or there is more
>>>> +than one endpoint at a port, a common scheme, using '#address-cells',
>>>> +'#size-cells' and 'reg' properties is used.
>>>
>>> Wouldn't this cause problems if the device has both video ports and a
>>> child bus ? Using #address-cells and #size-cells for the video ports would
>>> prevent the child bus from being handled in the usual way.
>>
>> Indeed, it looks like a serious issue in these bindings.
>>
>>> A possible solution would be to number ports with a dash instead of a @,
>>> as done in pinctrl for instance. We would then get
>>>
>>> 	port-0 {
>>> 		endpoint-0 { ... };
>>> 		endpoint-1 { ... };
>>> 	};
>>> 	port-1 { ... };

One problem here is that index of the port or the endpoint node can have 
random value and don't need to start with 0, which is the case for the pinctrl
properties. It makes iterating over those nodes more difficult, instead
of using standard functions like of_node_cmp() we would need to search for 
sub-strings in the node name.

>> Sounds like a good alternative, I can't think of any better solution at the
>> moment.
>>
>>>> +Two 'endpoint' nodes are linked with each other through their
>>>> +'remote-endpoint' phandles.  An endpoint subnode of a device contains
>>>> +all properties needed for configuration of this device for data exchange
>>>> +with the other device.  In most cases properties at the peer 'endpoint'
>>>> +nodes will be identical, however they might need to be different when
>>>> +there is any signal modifications on the bus between two devices, e.g.
>>>> +there are logic signal inverters on the lines.
>>>> +
>>>> +Required properties
>>>> +-------------------
>>>> +
>>>> +If there is more than one 'port' or more than one 'endpoint' node
>>>> following +properties are required in relevant parent node:
>>>> +
>>>> +- #address-cells : number of cells required to define port number,
>>>> should be 1.
>>>> +- #size-cells    : should be zero.
>>>
>>> I wonder if we should specify whether a port is a data sink or data
>>> source. A source can be connected to multiple sinks at the same time, but
>>> a sink can only be connected to a single source. If we want to perform
>>> automatic sanity checks in the core knowing the direction might help.
>>
>> Multiple sources can be linked to a single sink, but only one link can be
>> active at any time.
>>
>> So I'm not sure if knowing if a DT port is a data source or data sink would
>> let us to validate device tree structure statically in general.
>>
>> Such source/sink property could be useful later at runtime, when data
>> pipeline is set up for streaming.
> 
> Yes, I was mostly thinking about runtime.
> 
>> How do you think this could be represented ? By just having boolean
>> properties like: 'source' and 'sink' in the port nodes ? Or perhaps in the
>> endpoint nodes, since some devices might be bidirectional ? I don't recall
>> any at the moment though.
> 
> Source and sink properties would do. We could also use a direction property 
> that could take sink, source and bidirectional values, but that might be more 
> complex.

Since we're going to allow multiple endpoints at a port to be active at any
time, for the reasons we discussed in IRC [1], I assume it's no longer
possible to perform sanity checks mentioned above in the core. Should we 
then keep the 'source', 'sink' properties in the port nodes ?

[1] http://linuxtv.org/irc/v4l/index.php?date=2013-01-29

> I don't think we will have bidirectional link (as that would most probably 
> involve a very different kind of bus, and thus new bindings).

--

Thanks,
Sylwester

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
  2013-01-30 12:40         ` Sylwester Nawrocki
@ 2013-01-30 22:35           ` Sylwester Nawrocki
  0 siblings, 0 replies; 20+ messages in thread
From: Sylwester Nawrocki @ 2013-01-30 22:35 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Sylwester Nawrocki, linux-media, hverkuil, g.liakhovetski,
	kyungmin.park, kgene.kim, grant.likely, rob.herring,
	thomas.abraham, t.figa, myungjoo.ham, sw0312.kim, prabhakar.lad,
	devicetree-discuss, linux-samsung-soc

Hi,

On 01/30/2013 01:40 PM, Sylwester Nawrocki wrote:
> On 01/25/2013 02:52 AM, Laurent Pinchart wrote:
>>>>> +Data interfaces on all video devices are described by their child 'port'
>>>>> +nodes. Configuration of a port depends on other devices participating in
>>>>> +the data transfer and is described by 'endpoint' subnodes.
>>>>> +
>>>>> +dev {
>>>>> +	#address-cells =<1>;
>>>>> +	#size-cells =<0>;
>>>>> +	port@0 {
>>>>> +		endpoint@0 { ... };
>>>>> +		endpoint@1 { ... };
>>>>> +	};
>>>>> +	port@1 { ... };
>>>>> +};
>>>>> +
>>>>> +If a port can be configured to work with more than one other device on
>>>>> +the same bus, an 'endpoint' child node must be provided for each of
>>>>> +them. If more than one port is present in a device node or there is more
>>>>> +than one endpoint at a port, a common scheme, using '#address-cells',
>>>>> +'#size-cells' and 'reg' properties is used.
>>>>
>>>> Wouldn't this cause problems if the device has both video ports and a
>>>> child bus ? Using #address-cells and #size-cells for the video ports would
>>>> prevent the child bus from being handled in the usual way.
>>>
>>> Indeed, it looks like a serious issue in these bindings.
>>>
>>>> A possible solution would be to number ports with a dash instead of a @,
>>>> as done in pinctrl for instance. We would then get
>>>>
>>>> 	port-0 {
>>>> 		endpoint-0 { ... };
>>>> 		endpoint-1 { ... };
>>>> 	};
>>>> 	port-1 { ... };

Another possible solution could be putting the port nodes under
a ports node, for these cases where a device has a child bus.
Where there is no conflict, the ports node could be omitted for
simplicity. Does it sound reasonable ?

device {
	ports {
		#address-cells = <1>;
		#size-cells = <0>;
		port@0 {	
			endpoint@0 {
				reg = <0>;
			};
			endpoint@1 { ... };
		};
		port@1 { ... };
	};
};

> One problem here is that index of the port or the endpoint node can have
> random value and don't need to start with 0, which is the case for the pinctrl
> properties. It makes iterating over those nodes more difficult, instead
> of using standard functions like of_node_cmp() we would need to search for
> sub-strings in the node name.

--

Thanks,
Sylwester

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2013-01-30 22:35 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-23 19:31 [PATCH RFC v3 00/14] V4L2 device tree bindings and OF helpers Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation Sylwester Nawrocki
2013-01-24 10:16   ` Laurent Pinchart
2013-01-24 18:30     ` Sylwester Nawrocki
2013-01-25  1:52       ` Laurent Pinchart
2013-01-30 12:40         ` Sylwester Nawrocki
2013-01-30 22:35           ` Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v4 02/14] [media] Add a V4L2 OF parser Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 03/14] s5p-csis: Add device tree support Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 04/14] s5p-fimc: Add device tree support for FIMC devices Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 05/14] s5p-fimc: Add device tree support for FIMC-LITE devices Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 06/14] s5p-fimc: Change platform subdevs registration method Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 07/14] s5p-fimc: Add device tree support for the main media device driver Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 08/14] s5p-fimc: Add device tree based sensors registration Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 09/14] s5p-fimc: Use pinctrl API for camera ports configuration Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 10/14] ARM: dts: Add camera to node exynos4.dtsi Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 11/14] ARM: dts: Add ISP power domain node for Exynos4x12 Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 12/14] ARM: dts: Add FIMC and MIPI CSIS device nodes " Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 13/14] ARM: dts: Add camera pinctrl nodes for Exynos4x12 SoCs Sylwester Nawrocki
2013-01-23 19:31 ` [PATCH RFC v3 14/14] ARM: dts: Add camera device nodes nodes for PQ board Sylwester Nawrocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.