[RFC/PATCH 0/13] Add device tree support for s5p-fimc SoC camera host interface driver

[RFC/PATCH 0/13] Add device tree support for s5p-fimc SoC camera host interface driver

[Sylwester Nawrocki]

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

Hi All,

This patch series adds device tree support for Samsung S5P/Exynos SoC
camera devices - camera host interface (FIMC), MIPI-CSI2 receiver
(a frontend to FIMC) and S5K6AA image sensor.

It also defines common "data-lanes" property for MIPI-CSI2 receivers and 
transmitters, as well as "video-bus-type" for image sensors, video encoders 
etc. and camera host interface drivers.

For an overview of the topology of devices in an SoC I have attached sample
graph generated from output at /dev/media? Media Controller devnode.
It's not generated from latest kernel, however it depicts well how the image
sensor (S5K6AA), MIPI-CSIS and FIMC devices are connected.

In coming few weeks I'm going to be offline and Bartlomiej will continue with
this RFC during that time.

The patches will be available in few hours at:
http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/camera-of

Base tree for this series is available at git repository:
git://git.infradead.org/users/kmpark/linux-samsung camera-of


Comments are welcome.


Regards,
Sylwester

[-- Attachment #2: camera_graph.ps --]
[-- Type: application/postscript, Size: 33350 bytes --]

[RFC/PATCH 01/13] ARM: Samsung: Extend MIPI PHY callback with an index argument

[Sylwester Nawrocki]

For systems instantiated from device tree struct platform_device id
field is always -1, add an 'id' argument to the s5p_csis_phy_enable()
function so the MIPI-CSIS hardware instance index can be passed in
by driver, for CONFIG_OF=y.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/plat-s5p/setup-mipiphy.c              |   20 ++++++++------------
 arch/arm/plat-samsung/include/plat/mipi_csis.h |   10 ++++++----
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/arm/plat-s5p/setup-mipiphy.c b/arch/arm/plat-s5p/setup-mipiphy.c
index 683c466..146ecc3 100644
--- a/arch/arm/plat-s5p/setup-mipiphy.c
+++ b/arch/arm/plat-s5p/setup-mipiphy.c
@@ -14,24 +14,19 @@
 #include <linux/spinlock.h>
 #include <mach/regs-clock.h>
 
-static int __s5p_mipi_phy_control(struct platform_device *pdev,
+static int __s5p_mipi_phy_control(struct platform_device *pdev, int id,
 				  bool on, u32 reset)
 {
 	static DEFINE_SPINLOCK(lock);
 	void __iomem *addr;
 	unsigned long flags;
-	int pid;
 	u32 cfg;
 
-	if (!pdev)
+	id = max(0, id);
+	if (id > 1)
 		return -EINVAL;
 
-	pid = (pdev->id == -1) ? 0 : pdev->id;
-
-	if (pid != 0 && pid != 1)
-		return -EINVAL;
-
-	addr = S5P_MIPI_DPHY_CONTROL(pid);
+	addr = S5P_MIPI_DPHY_CONTROL(id);
 
 	spin_lock_irqsave(&lock, flags);
 
@@ -52,12 +47,13 @@ static int __s5p_mipi_phy_control(struct platform_device *pdev,
 	return 0;
 }
 
-int s5p_csis_phy_enable(struct platform_device *pdev, bool on)
+int s5p_csis_phy_enable(struct platform_device *pdev, int id, bool on)
 {
-	return __s5p_mipi_phy_control(pdev, on, S5P_MIPI_DPHY_SRESETN);
+	return __s5p_mipi_phy_control(pdev, on, id, S5P_MIPI_DPHY_SRESETN);
 }
 
 int s5p_dsim_phy_enable(struct platform_device *pdev, bool on)
 {
-	return __s5p_mipi_phy_control(pdev, on, S5P_MIPI_DPHY_MRESETN);
+	return __s5p_mipi_phy_control(pdev, pdev->id, on,
+				      S5P_MIPI_DPHY_MRESETN);
 }
diff --git a/arch/arm/plat-samsung/include/plat/mipi_csis.h b/arch/arm/plat-samsung/include/plat/mipi_csis.h
index c45b1e8..609cf1e 100644
--- a/arch/arm/plat-samsung/include/plat/mipi_csis.h
+++ b/arch/arm/plat-samsung/include/plat/mipi_csis.h
@@ -34,10 +34,12 @@ struct s5p_platform_mipi_csis {
 
 /**
  * s5p_csis_phy_enable - global MIPI-CSI receiver D-PHY control
- * @pdev: MIPI-CSIS platform device
- * @on: true to enable D-PHY and deassert its reset
- *	false to disable D-PHY
+ * @pdev:   MIPI-CSIS platform device
+ * @id:     MIPI-CSIS harware instance index (0...1)
+ * @on:     true to enable D-PHY and deassert its reset
+ *          false to disable D-PHY
+ * @return: 0 on success, or negative error code on failure
  */
-int s5p_csis_phy_enable(struct platform_device *pdev, bool on);
+int s5p_csis_phy_enable(struct platform_device *pdev, int id, bool on);
 
 #endif /* __PLAT_SAMSUNG_MIPI_CSIS_H_ */
-- 
1.7.10

[RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Sylwester Nawrocki]

s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
(camera host interface DMA engine and image processor). This patch
adds support for instantiating the MIPI-CSIS devices from DT and
parsing all SoC and board specific properties from device tree.
The MIPI DPHY control callback is now called directly from within
the driver, the platform code must ensure this callback does the
right thing for each SoC.

The cell-index property is used to ensure proper signal routing,
from physical camera port, through MIPI-CSI2 receiver to the DMA
engine (FIMC?). It's also helpful in exposing the device topology
in user space at /dev/media? devnode (Media Controller API).

This patch also defines a common property ("data-lanes") for MIPI-CSI
receivers and transmitters.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/video/mipi.txt   |    5 +
 .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
 drivers/media/video/s5p-fimc/mipi-csis.c           |   97 +++++++++++++++-----
 drivers/media/video/s5p-fimc/mipi-csis.h           |    1 +
 4 files changed, 126 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
 create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt

diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt
new file mode 100644
index 0000000..5aed285
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/mipi.txt
@@ -0,0 +1,5 @@
+Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
+
+ - data-lanes : number of differential data lanes wired and actively used in
+		communication between the transmitter and the receiver, this
+		excludes the clock lane;
diff --git a/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
new file mode 100644
index 0000000..7bce6f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
@@ -0,0 +1,47 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:
+
+- compatible - one of :
+		"samsung,s5pv210-csis",
+		"samsung,exynos4210-csis",
+		"samsung,exynos4212-csis",
+		"samsung,exynos4412-csis";
+- 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;
+- cell-index      : the hardware instance index;
+- clock-frequency : The IP's main (system bus) clock frequency in Hz, the default
+		    value when this property is not specified is 166 MHz;
+- data-lanes      : number of physical MIPI-CSI2 lanes used;
+- samsung,csis-hs-settle : differential receiver (HS-RX) settle time;
+- 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).
+
+Example:
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+/* SoC properties */
+
+	csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		cell-index = <0>;
+	};
+
+/* Board properties */
+
+	csis@11880000 {
+		clock-frequency = <166000000>;
+		data-lanes = <2>;
+		samsung,csis-hs-settle = <12>;
+		vddio-supply = <&reg0>;
+		vddcore-supply = <&reg1>;
+	};
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c
index 2f73d9e..ffb820e 100644
--- a/drivers/media/video/s5p-fimc/mipi-csis.c
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -19,6 +19,7 @@
 #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>
@@ -80,10 +81,11 @@ 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[] = {
-	"vdd11", /* 1.1V or 1.2V (s5pc100) MIPI CSI suppply */
-	"vdd18", /* VDD 1.8V and MIPI CSI PLL supply */
+	"vddcore",  /* CSIS Core (1.0V, 1.1V or 1.2V) suppply */
+	"vddio",    /* CSIS I/O and PLL (1.8V) supply */
 };
 #define CSIS_NUM_SUPPLIES ARRAY_SIZE(csis_supply_name)
 
@@ -99,11 +101,15 @@ enum {
  *        protecting @format and @flags members
  * @pads: CSIS pads array
  * @sd: v4l2_subdev associated with CSIS device instance
+ * @index: the hardware instance index
  * @pdev: CSIS platform device
  * @regs: mmaped I/O registers memory
  * @clock: CSIS clocks
  * @irq: requested s5p-mipi-csis irq number
  * @flags: the state variable for power and streaming control
+ * @clock_freq: device main clock frequency
+ * @num_lanes: number of MIPI-CSI data lanes used
+ * @hs_settle: HS-RX settle time
  * @csis_fmt: current CSIS pixel format
  * @format: common media bus format for the source and sink pad
  */
@@ -111,12 +117,18 @@ struct csis_state {
 	struct mutex lock;
 	struct media_pad pads[CSIS_PADS_NUM];
 	struct v4l2_subdev sd;
+	int index;
 	struct platform_device *pdev;
 	void __iomem *regs;
 	struct regulator_bulk_data supplies[CSIS_NUM_SUPPLIES];
 	struct clk *clock[NUM_CSIS_CLOCKS];
 	int irq;
 	u32 flags;
+
+	u32 clk_frequency;
+	u32 hs_settle;
+	u32 num_lanes;
+
 	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt format;
 };
@@ -232,15 +244,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)
@@ -488,9 +499,38 @@ 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;
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np != NULL) {
+		if (of_property_read_u32(np, "clock-frequency",
+					 &state->clk_frequency))
+			state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+
+		if (of_property_read_u32(np, "data-lanes", &state->num_lanes))
+			state->num_lanes = 2;
+		of_property_read_u32(np, "cell-index", &state->index);
+
+		of_property_read_u32(np, "samsung,csis-hs-settle",
+				     &state->hs_settle);
+	} else {
+		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);
+	}
+	return 0;
+}
+
 static int __devinit s5pcsis_probe(struct platform_device *pdev)
 {
-	struct s5p_platform_mipi_csis *pdata;
 	struct resource *mem_res;
 	struct csis_state *state;
 	int ret = -ENOMEM;
@@ -503,16 +543,14 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 	mutex_init(&state->lock);
 	state->pdev = pdev;
 
-	pdata = pdev->dev.platform_data;
-	if (pdata == NULL || pdata->phy_enable == NULL) {
-		dev_err(&pdev->dev, "Platform data not fully specified\n");
-		return -EINVAL;
-	}
+	ret = s5pcsis_get_platform_data(pdev, state);
+	if (ret < 0)
+		return ret;
 
-	if ((pdev->id == 1 && pdata->lanes > CSIS1_MAX_LANES) ||
-	    pdata->lanes > CSIS0_MAX_LANES) {
+	if ((state->index == 1 && state->num_lanes > CSIS1_MAX_LANES) ||
+	     state->num_lanes > CSIS0_MAX_LANES) {
 		dev_err(&pdev->dev, "Unsupported number of data lanes: %d\n",
-			pdata->lanes);
+			state->num_lanes);
 		return -EINVAL;
 	}
 
@@ -542,8 +580,8 @@ 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");
 
@@ -556,7 +594,8 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 
 	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];
 
@@ -590,7 +629,6 @@ e_clkput:
 
 static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 {
-	struct s5p_platform_mipi_csis *pdata = dev->platform_data;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
 	struct csis_state *state = sd_to_csis_state(sd);
@@ -602,7 +640,7 @@ static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 	mutex_lock(&state->lock);
 	if (state->flags & ST_POWERED) {
 		s5pcsis_stop_stream(state);
-		ret = pdata->phy_enable(state->pdev, false);
+		ret = s5p_csis_phy_enable(state->pdev, state->index, false);
 		if (ret)
 			goto unlock;
 		ret = regulator_bulk_disable(CSIS_NUM_SUPPLIES,
@@ -621,7 +659,6 @@ static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 
 static int s5pcsis_pm_resume(struct device *dev, bool runtime)
 {
-	struct s5p_platform_mipi_csis *pdata = dev->platform_data;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
 	struct csis_state *state = sd_to_csis_state(sd);
@@ -639,7 +676,7 @@ static int s5pcsis_pm_resume(struct device *dev, bool runtime)
 					    state->supplies);
 		if (ret)
 			goto unlock;
-		ret = pdata->phy_enable(state->pdev, true);
+		ret = s5p_csis_phy_enable(state->pdev, state->index, true);
 		if (!ret) {
 			state->flags |= ST_POWERED;
 		} else {
@@ -705,13 +742,25 @@ static const struct dev_pm_ops s5pcsis_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(s5pcsis_suspend, s5pcsis_resume)
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id s5pcsis_of_match[] __devinitconst = {
+	{ .compatible = "samsung,s5pv210-csis" },
+	{ .compatible = "samsung,exynos4210-csis" },
+	{ .compatible = "samsung,exynos4212-csis" },
+	{ .compatible = "samsung,exynos4412-csis" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s5pcsis_of_match);
+#endif
+
 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/video/s5p-fimc/mipi-csis.h b/drivers/media/video/s5p-fimc/mipi-csis.h
index 2709286..8f71e84 100644
--- a/drivers/media/video/s5p-fimc/mipi-csis.h
+++ b/drivers/media/video/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	"MIPI-CSIS"
 #define CSIS_MAX_ENTITIES	2
 #define CSIS0_MAX_LANES		4
 #define CSIS1_MAX_LANES		2
-- 
1.7.10

[RFC/PATCH 03/13] ARM: Samsung: Remove unused fields from FIMC and CSIS platform data

[Sylwester Nawrocki]

The MIPI-CSI2 bus data alignment is now being derived from the media
bus pixel code, the drivers don't use the corresponding structure
fields, so remove them. Also remove the s5p_csis_phy_enable callback
which is now used directly by s5p-csis driver.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/mach-nuri.c               |    3 ---
 arch/arm/mach-exynos/mach-universal_c210.c     |    3 ---
 arch/arm/plat-samsung/include/plat/mipi_csis.h |   13 +++----------
 include/media/s5p_fimc.h                       |    2 --
 4 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-nuri.c
index c9ca54a..1cd4ded 100644
--- a/arch/arm/mach-exynos/mach-nuri.c
+++ b/arch/arm/mach-exynos/mach-nuri.c
@@ -1148,9 +1148,7 @@ static struct platform_device cam_8m_12v_fixed_rdev = {
 static struct s5p_platform_mipi_csis mipi_csis_platdata = {
 	.clk_rate	= 166000000UL,
 	.lanes		= 2,
-	.alignment	= 32,
 	.hs_settle	= 12,
-	.phy_enable	= s5p_csis_phy_enable,
 };
 
 #define GPIO_CAM_MEGA_RST	EXYNOS4_GPY3(7) /* ISP_RESET */
@@ -1194,7 +1192,6 @@ static struct s5p_fimc_isp_info nuri_camera_sensors[] = {
 		.bus_type	= FIMC_MIPI_CSI2,
 		.board_info	= &m5mols_board_info,
 		.clk_frequency	= 24000000UL,
-		.csi_data_align	= 32,
 	},
 };
 
diff --git a/arch/arm/mach-exynos/mach-universal_c210.c b/arch/arm/mach-exynos/mach-universal_c210.c
index 7ebf79c..c2cba7a 100644
--- a/arch/arm/mach-exynos/mach-universal_c210.c
+++ b/arch/arm/mach-exynos/mach-universal_c210.c
@@ -908,9 +908,7 @@ static struct platform_device cam_s_if_fixed_reg_dev = {
 static struct s5p_platform_mipi_csis mipi_csis_platdata = {
 	.clk_rate	= 166000000UL,
 	.lanes		= 2,
-	.alignment	= 32,
 	.hs_settle	= 12,
-	.phy_enable	= s5p_csis_phy_enable,
 };
 
 #define GPIO_CAM_LEVEL_EN(n)	EXYNOS4_GPE4(n + 3)
@@ -974,7 +972,6 @@ static struct s5p_fimc_isp_info universal_camera_sensors[] = {
 		.board_info	= &m5mols_board_info,
 		.i2c_bus_num	= 0,
 		.clk_frequency	= 24000000UL,
-		.csi_data_align	= 32,
 	},
 };
 
diff --git a/arch/arm/plat-samsung/include/plat/mipi_csis.h b/arch/arm/plat-samsung/include/plat/mipi_csis.h
index 609cf1e..c35bf7a 100644
--- a/arch/arm/plat-samsung/include/plat/mipi_csis.h
+++ b/arch/arm/plat-samsung/include/plat/mipi_csis.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010-2011 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2010 - 2012 Samsung Electronics Co., Ltd.
  *
  * S5P series MIPI CSI slave device support
  *
@@ -15,21 +15,14 @@ struct platform_device;
 
 /**
  * struct s5p_platform_mipi_csis - platform data for S5P MIPI-CSIS driver
- * @clk_rate: bus clock frequency
- * @lanes: number of data lanes used
- * @alignment: data alignment in bits
+ * @clk_rate:  bus clock frequency
+ * @lanes:     number of data lanes used
  * @hs_settle: HS-RX settle time
- * @fixed_phy_vdd: false to enable external D-PHY regulator management in the
- *		   driver or true in case this regulator has no enable function
- * @phy_enable: pointer to a callback controlling D-PHY enable/reset
  */
 struct s5p_platform_mipi_csis {
 	unsigned long clk_rate;
 	u8 lanes;
-	u8 alignment;
 	u8 hs_settle;
-	bool fixed_phy_vdd;
-	int (*phy_enable)(struct platform_device *pdev, bool on);
 };
 
 /**
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 8587aaf..7be6f81 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -28,7 +28,6 @@ struct i2c_board_info;
  * @board_info: pointer to I2C subdevice's board info
  * @clk_frequency: frequency of the clock the host interface provides to sensor
  * @bus_type: determines bus type, MIPI, ITU-R BT.601 etc.
- * @csi_data_align: MIPI-CSI interface data alignment in bits
  * @i2c_bus_num: i2c control bus id the sensor is attached to
  * @mux_id: FIMC camera interface multiplexer index (separate for MIPI and ITU)
  * @clk_id: index of the SoC peripheral clock for sensors
@@ -38,7 +37,6 @@ struct s5p_fimc_isp_info {
 	struct i2c_board_info *board_info;
 	unsigned long clk_frequency;
 	enum cam_bus_type bus_type;
-	u16 csi_data_align;
 	u16 i2c_bus_num;
 	u16 mux_id;
 	u16 flags;
-- 
1.7.10

[RFC/PATCH 04/13] devicetree: Add common video devices bindings documentation

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/video/video.txt |   10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/video.txt

diff --git a/Documentation/devicetree/bindings/video/video.txt b/Documentation/devicetree/bindings/video/video.txt
new file mode 100644
index 0000000..9f6a637
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/video.txt
@@ -0,0 +1,10 @@
+Common properties of video data source devices (image sensor, video encoders, etc.)
+
+Video bus types
+---------------
+
+- video-bus-type : must be one of:
+
+    - itu-601   : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+    - itu-656   : parallel bus with embedded synchronization - ITU-R BT.656;
+    - mipi-csi2 : MIPI-CSI2 serial bus;
-- 
1.7.10

[RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../bindings/camera/soc/samsung-fimc.txt           |   66 ++++
 drivers/media/video/s5p-fimc/fimc-capture.c        |    2 +-
 drivers/media/video/s5p-fimc/fimc-core.c           |  410 +++++++++++---------
 drivers/media/video/s5p-fimc/fimc-core.h           |    2 -
 drivers/media/video/s5p-fimc/fimc-mdevice.c        |    8 +-
 5 files changed, 291 insertions(+), 197 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt

diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
new file mode 100644
index 0000000..1ec48e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
@@ -0,0 +1,66 @@
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem uses a dedicated device node associated with
+top level device driver that manages common properties of the whole subsystem,
+like common camera port pins or clocks for external image sensors. This
+aggregate node references related platform sub-devices - FIMC, FIMC-LITE,
+MIPI-CSIS [1], and it also contains nodes describing image sensors wired to
+the host SoC's video port and using I2C or SPI as the control bus.
+
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible	   : must be "samsung,fimc"
+- fimc-controllers : an array of phandles to 'fimc' device nodes,
+		     size of this array must be at least 1;
+
+Optional properties:
+
+- csi-rx-controllers : an array of phandles to 'csis' device nodes,
+		       it is required for sensors with MIPI-CSI2 bus;
+
+'fimc' device node
+------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,s5pv210-fimc"
+		"samsung,exynos4210-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device memory mapped
+	       registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+- cell-index : FIMC IP instance index, the number of available instances
+	       depends on the SoC revision. For S5PV210 valid values are:
+	       0...2, for Exynos4x1x: 0...3.
+
+Example:
+
+	fimc0: fimc@11800000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11800000 0x1000>;
+		interrupts = <0 85 0>;
+		cell-index = <0>;
+	};
+
+	csis0: csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		cell-index = <0>;
+	};
+
+	camera {
+		compatible = "samsung,fimc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		fimc-controllers = <&fimc0>;
+		csi-rx-controllers = <&csis0>;
+	};
+
+[1] Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 0549451..7585b2f 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -1684,7 +1684,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/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index fedcd56..30c6365 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/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>
@@ -188,6 +190,198 @@ static struct fimc_fmt fimc_formats[] = {
 	},
 };
 
+/* Image pixel limits, similar across several FIMC HW revisions. */
+static struct fimc_pix_limit s5p_pix_limit[4] = {
+	[0] = {
+		.scaler_en_w	= 3264,
+		.scaler_dis_w	= 8192,
+		.in_rot_en_h	= 1920,
+		.in_rot_dis_w	= 8192,
+		.out_rot_en_w	= 1920,
+		.out_rot_dis_w	= 4224,
+	},
+	[1] = {
+		.scaler_en_w	= 4224,
+		.scaler_dis_w	= 8192,
+		.in_rot_en_h	= 1920,
+		.in_rot_dis_w	= 8192,
+		.out_rot_en_w	= 1920,
+		.out_rot_dis_w	= 4224,
+	},
+	[2] = {
+		.scaler_en_w	= 1920,
+		.scaler_dis_w	= 8192,
+		.in_rot_en_h	= 1280,
+		.in_rot_dis_w	= 8192,
+		.out_rot_en_w	= 1280,
+		.out_rot_dis_w	= 1920,
+	},
+	[3] = {
+		.scaler_en_w	= 1920,
+		.scaler_dis_w	= 8192,
+		.in_rot_en_h	= 1366,
+		.in_rot_dis_w	= 8192,
+		.out_rot_en_w	= 1366,
+		.out_rot_dis_w	= 1920,
+	},
+};
+
+static struct fimc_variant fimc0_variant_s5p = {
+	.has_inp_rot	 = 1,
+	.has_out_rot	 = 1,
+	.has_cam_if	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 8,
+	.min_vsize_align = 16,
+	.out_buf_count	 = 4,
+	.pix_limit	 = &s5p_pix_limit[0],
+};
+
+static struct fimc_variant fimc2_variant_s5p = {
+	.has_cam_if	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 8,
+	.min_vsize_align = 16,
+	.out_buf_count	 = 4,
+	.pix_limit	 = &s5p_pix_limit[1],
+};
+
+static struct fimc_variant fimc0_variant_s5pv210 = {
+	.pix_hoff	 = 1,
+	.has_inp_rot	 = 1,
+	.has_out_rot	 = 1,
+	.has_cam_if	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 8,
+	.min_vsize_align = 16,
+	.out_buf_count	 = 4,
+	.pix_limit	 = &s5p_pix_limit[1],
+};
+
+static struct fimc_variant fimc1_variant_s5pv210 = {
+	.pix_hoff	 = 1,
+	.has_inp_rot	 = 1,
+	.has_out_rot	 = 1,
+	.has_cam_if	 = 1,
+	.has_mainscaler_ext = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 1,
+	.min_vsize_align = 1,
+	.out_buf_count	 = 4,
+	.pix_limit	 = &s5p_pix_limit[2],
+};
+
+static struct fimc_variant fimc2_variant_s5pv210 = {
+	.has_cam_if	 = 1,
+	.pix_hoff	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 8,
+	.min_vsize_align = 16,
+	.out_buf_count	 = 4,
+	.pix_limit	 = &s5p_pix_limit[2],
+};
+
+static struct fimc_variant fimc0_variant_exynos4 = {
+	.pix_hoff	 = 1,
+	.has_inp_rot	 = 1,
+	.has_out_rot	 = 1,
+	.has_cam_if	 = 1,
+	.has_cistatus2	 = 1,
+	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 2,
+	.min_vsize_align = 1,
+	.out_buf_count	 = 32,
+	.pix_limit	 = &s5p_pix_limit[1],
+};
+
+static struct fimc_variant fimc3_variant_exynos4 = {
+	.pix_hoff	 = 1,
+	.has_cam_if	 = 1,
+	.has_cistatus2	 = 1,
+	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
+	.min_inp_pixsize = 16,
+	.min_out_pixsize = 16,
+	.hor_offs_align	 = 2,
+	.min_vsize_align = 1,
+	.out_buf_count	 = 32,
+	.pix_limit	 = &s5p_pix_limit[3],
+};
+
+/* S5PC100 */
+static struct fimc_drvdata fimc_drvdata_s5p = {
+	.variant = {
+		[0] = &fimc0_variant_s5p,
+		[1] = &fimc0_variant_s5p,
+		[2] = &fimc2_variant_s5p,
+	},
+	.num_entities = 3,
+	.lclk_frequency = 133000000UL,
+};
+
+/* S5PV210, S5PC110 */
+static struct fimc_drvdata fimc_drvdata_s5pv210 = {
+	.variant = {
+		[0] = &fimc0_variant_s5pv210,
+		[1] = &fimc1_variant_s5pv210,
+		[2] = &fimc2_variant_s5pv210,
+	},
+	.num_entities = 3,
+	.lclk_frequency = 166000000UL,
+};
+
+/* EXYNOS4210, S5PV310, S5PC210 */
+static struct fimc_drvdata fimc_drvdata_exynos4 = {
+	.variant = {
+		[0] = &fimc0_variant_exynos4,
+		[1] = &fimc0_variant_exynos4,
+		[2] = &fimc0_variant_exynos4,
+		[3] = &fimc3_variant_exynos4,
+	},
+	.num_entities = 4,
+	.lclk_frequency = 166000000UL,
+};
+
+static struct platform_device_id fimc_driver_ids[] = {
+	{
+		.name		= "s5p-fimc",
+		.driver_data	= (unsigned long)&fimc_drvdata_s5p,
+	}, {
+		.name		= "s5pv210-fimc",
+		.driver_data	= (unsigned long)&fimc_drvdata_s5pv210,
+	}, {
+		.name		= "exynos4-fimc",
+		.driver_data	= (unsigned long)&fimc_drvdata_exynos4,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id fimc_of_match[] __devinitconst = {
+	{
+		.compatible = "samsung,s5pv210-fimc",
+		.data = &fimc_drvdata_s5pv210,
+	}, {
+		.compatible = "samsung,exynos4210-fimc",
+		.data = &fimc_drvdata_exynos4,
+	}, {
+		.compatible = "samsung,exynos4412-fimc",
+		.data = &fimc_drvdata_exynos4,
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, fimc_of_match);
+#endif
+
 struct fimc_fmt *fimc_get_format(unsigned int index)
 {
 	if (index >= ARRAY_SIZE(fimc_formats))
@@ -865,29 +1059,39 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
 
 static int fimc_probe(struct platform_device *pdev)
 {
-	struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev);
-	struct s5p_platform_fimc *pdata;
+	struct fimc_drvdata *drv_data = NULL;
+
+	const struct of_device_id *of_id;
 	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);
 	if (!fimc)
 		return -ENOMEM;
 
-	fimc->id = pdev->id;
+	if (pdev->dev.of_node) {
+		u32 id = 0;
+		of_id = of_match_node(fimc_of_match, pdev->dev.of_node);
+		if (of_id)
+			drv_data = of_id->data;
+
+		of_property_read_u32(pdev->dev.of_node, "cell-index", &id);
+		fimc->id = id;
+
+	} else {
+		drv_data = fimc_get_drvdata(pdev);
+		fimc->id = pdev->id;
+	}
+
+	if (drv_data == NULL || fimc->id >= drv_data->num_entities) {
+		dev_err(&pdev->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);
@@ -1036,181 +1240,6 @@ static int __devexit fimc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-/* Image pixel limits, similar across several FIMC HW revisions. */
-static struct fimc_pix_limit s5p_pix_limit[4] = {
-	[0] = {
-		.scaler_en_w	= 3264,
-		.scaler_dis_w	= 8192,
-		.in_rot_en_h	= 1920,
-		.in_rot_dis_w	= 8192,
-		.out_rot_en_w	= 1920,
-		.out_rot_dis_w	= 4224,
-	},
-	[1] = {
-		.scaler_en_w	= 4224,
-		.scaler_dis_w	= 8192,
-		.in_rot_en_h	= 1920,
-		.in_rot_dis_w	= 8192,
-		.out_rot_en_w	= 1920,
-		.out_rot_dis_w	= 4224,
-	},
-	[2] = {
-		.scaler_en_w	= 1920,
-		.scaler_dis_w	= 8192,
-		.in_rot_en_h	= 1280,
-		.in_rot_dis_w	= 8192,
-		.out_rot_en_w	= 1280,
-		.out_rot_dis_w	= 1920,
-	},
-	[3] = {
-		.scaler_en_w	= 1920,
-		.scaler_dis_w	= 8192,
-		.in_rot_en_h	= 1366,
-		.in_rot_dis_w	= 8192,
-		.out_rot_en_w	= 1366,
-		.out_rot_dis_w	= 1920,
-	},
-};
-
-static struct fimc_variant fimc0_variant_s5p = {
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.out_buf_count	 = 4,
-	.pix_limit	 = &s5p_pix_limit[0],
-};
-
-static struct fimc_variant fimc2_variant_s5p = {
-	.has_cam_if	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.out_buf_count	 = 4,
-	.pix_limit	 = &s5p_pix_limit[1],
-};
-
-static struct fimc_variant fimc0_variant_s5pv210 = {
-	.pix_hoff	 = 1,
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.out_buf_count	 = 4,
-	.pix_limit	 = &s5p_pix_limit[1],
-};
-
-static struct fimc_variant fimc1_variant_s5pv210 = {
-	.pix_hoff	 = 1,
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.has_mainscaler_ext = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 1,
-	.min_vsize_align = 1,
-	.out_buf_count	 = 4,
-	.pix_limit	 = &s5p_pix_limit[2],
-};
-
-static struct fimc_variant fimc2_variant_s5pv210 = {
-	.has_cam_if	 = 1,
-	.pix_hoff	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.out_buf_count	 = 4,
-	.pix_limit	 = &s5p_pix_limit[2],
-};
-
-static struct fimc_variant fimc0_variant_exynos4 = {
-	.pix_hoff	 = 1,
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.has_cistatus2	 = 1,
-	.has_mainscaler_ext = 1,
-	.has_alpha	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 2,
-	.min_vsize_align = 1,
-	.out_buf_count	 = 32,
-	.pix_limit	 = &s5p_pix_limit[1],
-};
-
-static struct fimc_variant fimc3_variant_exynos4 = {
-	.pix_hoff	 = 1,
-	.has_cam_if	 = 1,
-	.has_cistatus2	 = 1,
-	.has_mainscaler_ext = 1,
-	.has_alpha	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 2,
-	.min_vsize_align = 1,
-	.out_buf_count	 = 32,
-	.pix_limit	 = &s5p_pix_limit[3],
-};
-
-/* S5PC100 */
-static struct fimc_drvdata fimc_drvdata_s5p = {
-	.variant = {
-		[0] = &fimc0_variant_s5p,
-		[1] = &fimc0_variant_s5p,
-		[2] = &fimc2_variant_s5p,
-	},
-	.num_entities = 3,
-	.lclk_frequency = 133000000UL,
-};
-
-/* S5PV210, S5PC110 */
-static struct fimc_drvdata fimc_drvdata_s5pv210 = {
-	.variant = {
-		[0] = &fimc0_variant_s5pv210,
-		[1] = &fimc1_variant_s5pv210,
-		[2] = &fimc2_variant_s5pv210,
-	},
-	.num_entities = 3,
-	.lclk_frequency = 166000000UL,
-};
-
-/* EXYNOS4210, S5PV310, S5PC210 */
-static struct fimc_drvdata fimc_drvdata_exynos4 = {
-	.variant = {
-		[0] = &fimc0_variant_exynos4,
-		[1] = &fimc0_variant_exynos4,
-		[2] = &fimc0_variant_exynos4,
-		[3] = &fimc3_variant_exynos4,
-	},
-	.num_entities = 4,
-	.lclk_frequency = 166000000UL,
-};
-
-static struct platform_device_id fimc_driver_ids[] = {
-	{
-		.name		= "s5p-fimc",
-		.driver_data	= (unsigned long)&fimc_drvdata_s5p,
-	}, {
-		.name		= "s5pv210-fimc",
-		.driver_data	= (unsigned long)&fimc_drvdata_s5pv210,
-	}, {
-		.name		= "exynos4-fimc",
-		.driver_data	= (unsigned long)&fimc_drvdata_exynos4,
-	},
-	{},
-};
-MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
-
 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)
@@ -1221,9 +1250,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,
 	}
 };
 
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index 95b27ae..a1df84d 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -414,7 +414,6 @@ struct fimc_ctx;
  * @slock:	the spinlock protecting this data structure
  * @lock:	the mutex protecting this data structure
  * @pdev:	pointer to the FIMC platform device
- * @pdata:	pointer to the device platform data
  * @variant:	the IP variant information
  * @id:		FIMC device index (0..FIMC_MAX_DEVS)
  * @clock:	clocks required for FIMC operation
@@ -431,7 +430,6 @@ struct fimc_dev {
 	spinlock_t			slock;
 	struct mutex			lock;
 	struct platform_device		*pdev;
-	struct s5p_platform_fimc	*pdata;
 	struct fimc_variant		*variant;
 	u16				id;
 	struct clk			*clock[MAX_FIMC_CLOCKS];
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c b/drivers/media/video/s5p-fimc/fimc-mdevice.c
index 2d780e5..3ffc4f5e 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -335,15 +335,15 @@ static int fimc_register_callback(struct device *dev, void *p)
 	struct fimc_dev *fimc = dev_get_drvdata(dev);
 	struct v4l2_subdev *sd = &fimc->vid_cap.subdev;
 	struct fimc_md *fmd = p;
-	int ret = 0;
+	int ret;
 
-	if (!fimc || !fimc->pdev)
+	if (fimc == NULL)
 		return 0;
 
-	if (fimc->pdev->id < 0 || fimc->pdev->id >= FIMC_MAX_DEVS)
+	if (fimc->id >= FIMC_MAX_DEVS)
 		return 0;
 
-	fmd->fimc[fimc->pdev->id] = fimc;
+	fmd->fimc[fimc->id] = fimc;
 	sd->grp_id = FIMC_GROUP_ID;
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-- 
1.7.10

[RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Sylwester Nawrocki]

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

diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
index 1ec48e9..b459da2 100644
--- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
@@ -39,6 +39,21 @@ Required properties:
 	       depends on the SoC revision. For S5PV210 valid values are:
 	       0...2, for Exynos4x1x: 0...3.
 
+
+'fimc-lite' device node
+-----------------------
+
+Required properties:
+
+- compatible : should be one of:
+		"samsung,exynos4212-fimc";
+		"samsung,exynos4412-fimc";
+- reg	     : physical base address and size of the device's memory mapped
+	       registers;
+- interrupts : should contain FIMC-LITE interrupt;
+- cell-index : FIMC-LITE IP instance index;
+
+
 Example:
 
 	fimc0: fimc@11800000 {
diff --git a/drivers/media/video/s5p-fimc/fimc-lite.c b/drivers/media/video/s5p-fimc/fimc-lite.c
index 400d701a..a7ae149 100644
--- a/drivers/media/video/s5p-fimc/fimc-lite.c
+++ b/drivers/media/video/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>
@@ -36,6 +37,37 @@
 static int debug;
 module_param(debug, int, 0644);
 
+
+static struct flite_variant fimc_lite0_variant_exynos4 = {
+	.max_width		= 8192,
+	.max_height		= 8192,
+	.out_width_align	= 8,
+	.win_hor_offs_align	= 2,
+	.out_hor_offs_align	= 8,
+};
+
+/* EXYNOS4212, EXYNOS4412 */
+static struct flite_drvdata fimc_lite_drvdata_exynos4 = {
+	.variant = {
+		[0] = &fimc_lite0_variant_exynos4,
+		[1] = &fimc_lite0_variant_exynos4,
+	},
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id flite_of_match[] __devinitconst = {
+	{
+		.compatible = "samsung,exynos4412-fimc-lite",
+		.data = &fimc_lite_drvdata_exynos4,
+	}, {
+		.compatible = "samsung,exynos4212-fimc-lite",
+		.data = &fimc_lite_drvdata_exynos4,
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, fimc_of_match);
+#endif
+
 static const struct fimc_fmt fimc_lite_formats[] = {
 	{
 		.name		= "YUV 4:2:2 packed, YCbYCr",
@@ -1378,6 +1410,7 @@ static int fimc_lite_clk_get(struct fimc_lite *fimc)
 static int __devinit fimc_lite_probe(struct platform_device *pdev)
 {
 	struct flite_drvdata *drv_data = fimc_lite_get_drvdata(pdev);
+	const struct of_device_id *of_id;
 	struct fimc_lite *fimc;
 	struct resource *res;
 	int ret;
@@ -1386,7 +1419,20 @@ static int __devinit fimc_lite_probe(struct platform_device *pdev)
 	if (!fimc)
 		return -ENOMEM;
 
-	fimc->index = pdev->id;
+	if (pdev->dev.of_node) {
+		of_id = of_match_node(of_match_ptr(flite_of_match),
+				      pdev->dev.of_node);
+		if (of_id)
+			drv_data = of_id->data;
+		of_property_read_u32(pdev->dev.of_node, "cell-index",
+				     &fimc->index);
+	} else {
+		fimc->index = pdev->id;
+	}
+
+	if (drv_data == NULL || fimc->index >= FIMC_LITE_MAX_DEVS)
+		return -EINVAL;
+
 	fimc->variant = drv_data->variant[fimc->index];
 	fimc->pdev = pdev;
 
@@ -1530,20 +1576,10 @@ static int __devexit fimc_lite_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct flite_variant fimc_lite0_variant_exynos4 = {
-	.max_width		= 8192,
-	.max_height		= 8192,
-	.out_width_align	= 8,
-	.win_hor_offs_align	= 2,
-	.out_hor_offs_align	= 8,
-};
-
-/* EXYNOS4212, EXYNOS4412 */
-static struct flite_drvdata fimc_lite_drvdata_exynos4 = {
-	.variant = {
-		[0] = &fimc_lite0_variant_exynos4,
-		[1] = &fimc_lite0_variant_exynos4,
-	},
+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 platform_device_id fimc_lite_driver_ids[] = {
@@ -1555,17 +1591,12 @@ 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 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.10

[RFC/PATCH 07/13] media: s5p-fimc: Enable device tree based media device instantiation

[Sylwester Nawrocki]

Enable device tree based instantiation of the common platform device
associated with the top level media device driver. Handling of the
image sensor related properties, like video port pins configuration,
is not included in this patch.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-core.h    |    2 +-
 drivers/media/video/s5p-fimc/fimc-mdevice.c |  104 +++++++++++++++++++++++++--
 2 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index a1df84d..8c5c03f 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -431,7 +431,7 @@ struct fimc_dev {
 	struct mutex			lock;
 	struct platform_device		*pdev;
 	struct fimc_variant		*variant;
-	u16				id;
+	u32				id;
 	struct clk			*clock[MAX_FIMC_CLOCKS];
 	void __iomem			*regs;
 	wait_queue_head_t		irq_queue;
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c b/drivers/media/video/s5p-fimc/fimc-mdevice.c
index 3ffc4f5e..92c9887 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/video/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>
@@ -404,6 +406,87 @@ static int csis_register_callback(struct device *dev, void *p)
 	return ret;
 }
 
+static int fimc_md_register_of_plat_entities(struct fimc_md *fmd)
+{
+#ifdef CONFIG_OF
+	struct device_node *np = fmd->pdev->dev.of_node;
+	struct device_node *node;
+	struct platform_device *pdev;
+	struct v4l2_subdev *sd;
+	struct fimc_dev *fimc;
+	u32 index;
+	int ret;
+
+	for (index = 0; index < FIMC_MAX_DEVS; index++) {
+		node = of_parse_phandle(np, "fimc-controllers", index);
+		if (node == NULL)
+			break;
+
+		pdev = of_find_device_by_node(node);
+		of_node_put(node);
+
+		fimc = dev_get_drvdata(&pdev->dev);
+		if (fimc == NULL)
+			return -EPROBE_DEFER;
+
+		if (WARN_ON(fimc->id >= FIMC_MAX_DEVS || fmd->fimc[fimc->id]))
+			continue;
+
+		fmd->fimc[fimc->id] = fimc;
+		sd = &fimc->vid_cap.subdev;
+		sd->grp_id = FIMC_GROUP_ID;
+
+		ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+		if (ret) {
+			v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d\n",
+				 fimc->id);
+			return ret;
+		}
+
+		pr_notice("%s:%d succedded to register %s\n", __FILE__, __LINE__, node->full_name);
+	}
+
+	for (index = 0; index < CSIS_MAX_ENTITIES; index++) {
+		unsigned int id = 0;
+
+		node = of_parse_phandle(np, "csi-rx-controllers", index);
+		if (node == NULL)
+			break;
+
+		pdev = of_find_device_by_node(node);
+		if (pdev == NULL)
+			return -ENODEV;
+
+		of_property_read_u32(node, "cell-index", &id);
+		of_node_put(node);
+
+		if (!try_module_get(pdev->dev.driver->owner))
+			return -ENODEV;
+
+		sd = dev_get_drvdata(&pdev->dev);
+		if (sd == NULL) {
+			module_put(pdev->dev.driver->owner);
+			return -EPROBE_DEFER;
+		}
+
+		if (id < CSIS_MAX_ENTITIES && fmd->csis[id].sd == NULL) {
+			sd->grp_id = CSIS_GROUP_ID;
+			fmd->csis[id].sd = sd;
+
+			ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+			if (ret)
+				v4l2_err(&fmd->v4l2_dev,
+					 "Failed to register CSIS.%d\n", index);
+		}
+		module_put(pdev->dev.driver->owner);
+
+		if (ret)
+			return ret;
+	}
+#endif
+	return 0;
+}
+
 /**
  * fimc_md_register_platform_entities - register FIMC and CSIS media entities
  */
@@ -413,6 +496,9 @@ static int fimc_md_register_platform_entities(struct fimc_md *fmd)
 	struct device_driver *driver;
 	int ret, i;
 
+	if (fmd->pdev->dev.of_node)
+		return fimc_md_register_of_plat_entities(fmd);
+
 	driver = driver_find(FIMC_MODULE_NAME, &platform_bus_type);
 	if (!driver) {
 		v4l2_warn(&fmd->v4l2_dev,
@@ -490,6 +576,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");
 }
 
 /**
@@ -934,8 +1021,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) {
@@ -1004,12 +1091,21 @@ static int __devexit fimc_md_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id fimc_of_match[] __devinitconst = {
+	{ .compatible = "samsung,fimc" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, fimc_of_match);
+#endif
+
 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 = of_match_ptr(fimc_of_match),
+		.name		= "s5p-fimc-md",
+		.owner		= THIS_MODULE,
 	}
 };
 
-- 
1.7.10

[RFC/PATCH 08/13] ARM: dts: Add FIMC and MIPI-CSIS devices to Exynos4210 DT source

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4210-origen.dts   |   28 +++++++++++++++
 arch/arm/boot/dts/exynos4210-smdkv310.dts |   28 +++++++++++++++
 arch/arm/boot/dts/exynos4210.dtsi         |   54 +++++++++++++++++++++++++++++
 arch/arm/mach-exynos/mach-exynos4-dt.c    |   12 +++++++
 4 files changed, 122 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
index b8c4763..20915b1 100644
--- a/arch/arm/boot/dts/exynos4210-origen.dts
+++ b/arch/arm/boot/dts/exynos4210-origen.dts
@@ -134,4 +134,32 @@
 	i2c@138D0000 {
 		status = "disabled";
 	};
+
+	camera {
+		status = "disabled";
+	};
+
+	fimc@11800000 {
+		status = "disabled";
+	};
+
+	fimc@11810000 {
+		status = "disabled";
+	};
+
+	fimc@11820000 {
+		status = "disabled";
+	};
+
+	fimc@11830000 {
+		status = "disabled";
+	};
+
+	csis@11880000 {
+		status = "disabled";
+	};
+
+	csis@11890000 {
+		status = "disabled";
+	};
 };
diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 27afc8e..da7adb7 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -179,4 +179,32 @@
 	i2c@138D0000 {
 		status = "disabled";
 	};
+
+	camera {
+		status = "disabled";
+	};
+
+	fimc@11800000 {
+		status = "disabled";
+	};
+
+	fimc@11810000 {
+		status = "disabled";
+	};
+
+	fimc@11820000 {
+		status = "disabled";
+	};
+
+	fimc@11830000 {
+		status = "disabled";
+	};
+
+	csis@11880000 {
+		status = "disabled";
+	};
+
+	csis@11890000 {
+		status = "disabled";
+	};
 };
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index be3c57c..27eb245 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -183,6 +183,60 @@
 		};
 	};
 
+	fimc0: fimc@11800000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11800000 0x1000>;
+		interrupts = <0 84 0>;
+		cell-index = <0>;
+		pd = <&pd_cam>;
+	};
+
+	fimc1: fimc@11810000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11810000 0x1000>;
+		interrupts = <0 85 0>;
+		cell-index = <1>;
+		pd = <&pd_cam>;
+	};
+
+	fimc2: fimc@11820000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11820000 0x1000>;
+		interrupts = <0 86 0>;
+		cell-index = <2>;
+		pd = <&pd_cam>;
+	};
+
+	fimc3: fimc@11830000 {
+		compatible = "samsung,exynos4210-fimc";
+		reg = <0x11830000 0x1000>;
+		interrupts = <0 87 0>;
+		cell-index = <3>;
+		pd = <&pd_cam>;
+	};
+
+	csis0: csis@11880000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11880000 0x1000>;
+		interrupts = <0 78 0>;
+		cell-index = <0>;
+		pd = <&pd_cam>;
+	};
+
+	csis1: csis@11890000 {
+		compatible = "samsung,exynos4210-csis";
+		reg = <0x11890000 0x1000>;
+		interrupts = <0 79 0>;
+		cell-index = <1>;
+		pd = <&pd_cam>;
+	};
+
+	camera {
+		compatible = "samsung,fimc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+	};
+
 	gpio-controllers {
 		#address-cells = <1>;
 		#size-cells = <1>;
diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
index aa13ec0..6545a37 100644
--- a/arch/arm/mach-exynos/mach-exynos4-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
@@ -70,6 +70,18 @@ static const struct of_dev_auxdata exynos4210_auxdata_lookup[] __initconst = {
 	OF_DEV_AUXDATA("samsung,s5pv210-tvmixer", S5P_PA_MIXER, "s5p-mixer", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA0, "dma-pl330.0", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA1, "dma-pl330.1", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-csis", EXYNOS4_PA_MIPI_CSIS0,
+				"s5p-mipi-csis.0", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-csis", EXYNOS4_PA_MIPI_CSIS1,
+				"s5p-mipi-csis.1", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-fimc", EXYNOS4_PA_FIMC0,
+				"exynos4-fimc.0", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-fimc", EXYNOS4_PA_FIMC1,
+				"exynos4-fimc.1", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-fimc", EXYNOS4_PA_FIMC2,
+				"exynos4-fimc.2", NULL),
+	OF_DEV_AUXDATA("samsung,exynos4210-fimc", EXYNOS4_PA_FIMC3,
+				"exynos4-fimc.3", NULL),
 	{},
 };
 
-- 
1.7.10

[RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sylwester Nawrocki]

The driver initializes all board related properties except the s_power()
callback to board code. The platforms that require this callback are not
supported by this driver yet for CONFIG_OF=y.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
 drivers/media/video/s5k6aa.c                       |  129 ++++++++++++++------
 2 files changed, 146 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt

diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
new file mode 100644
index 0000000..6685a9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+------------------------------
+
+Required properties:
+
+- compatible : "samsung,s5k6aafx";
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
+		   or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
+		 or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default
+		    value when this property is not specified is 24 MHz;
+- data-lanes : number of physical lanes used (default 2 if not specified);
+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO
+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;
+
+
+Example:
+
+	gpl2: gpio-controller@0 {
+	};
+
+	reg0: regulator@0 {
+	};
+
+	reg1: regulator@1 {
+	};
+
+	reg2: regulator@2 {
+	};
+
+	reg3: regulator@3 {
+	};
+
+	s5k6aafx@3c {
+		compatible = "samsung,s5k6aafx";
+		reg = <0x3c>;
+		clock-frequency = <24000000>;
+		gpio-rst = <&gpl2 0 2 0 3>;
+		gpio-stby = <&gpl2 1 2 0 3>;
+		video-itu-601-bus;
+		vdd_core-supply = <&reg0>;
+		vdda-supply = <&reg1>;
+		vdd_reg-supply = <&reg2>;
+		vddio-supply = <&reg3>;
+	};
diff --git a/drivers/media/video/s5k6aa.c b/drivers/media/video/s5k6aa.c
index 6625e46..ed172bb 100644
--- a/drivers/media/video/s5k6aa.c
+++ b/drivers/media/video/s5k6aa.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/media.h>
 #include <linux/module.h>
+#include <linux/of_gpio.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -232,14 +233,14 @@ struct s5k6aa {
 	struct media_pad pad;
 
 	enum v4l2_mbus_type bus_type;
-	u8 mipi_lanes;
+	u32 mipi_lanes;
 
 	int (*s_power)(int enable);
 	struct regulator_bulk_data supplies[S5K6AA_NUM_SUPPLIES];
 	struct s5k6aa_gpio gpio[GPIO_NUM];
 
 	/* external master clock frequency */
-	unsigned long mclk_frequency;
+	u32 mclk_frequency;
 	/* ISP internal master clock frequency */
 	u16 clk_fop;
 	/* output pixel clock frequency range */
@@ -1519,68 +1520,109 @@ static void s5k6aa_free_gpios(struct s5k6aa *s5k6aa)
 	}
 }
 
-static int s5k6aa_configure_gpios(struct s5k6aa *s5k6aa,
-				  const struct s5k6aa_platform_data *pdata)
+static int s5k6aa_configure_gpios(struct s5k6aa *s5k6aa, struct device *dev)
 {
-	const struct s5k6aa_gpio *gpio = &pdata->gpio_stby;
+	const struct s5k6aa_platform_data *pdata = dev->platform_data;
+	struct device_node *np = dev->of_node;
+	const struct s5k6aa_gpio *pgpio;
+	struct s5k6aa_gpio gpio = { 0 };
 	int ret;
 
 	s5k6aa->gpio[STBY].gpio = -EINVAL;
 	s5k6aa->gpio[RST].gpio  = -EINVAL;
 
-	ret = s5k6aa_configure_gpio(gpio->gpio, gpio->level, "S5K6AA_STBY");
+	if (np) {
+		gpio.gpio = of_get_named_gpio(np, "gpio-stby", 0);
+		if (!of_get_property(np, "samsung,s5k6aa-inv-stby", NULL))
+			gpio.level = 1;
+	}
+	pgpio = np ? &gpio : &pdata->gpio_stby;
+	ret = s5k6aa_configure_gpio(pgpio->gpio, pgpio->level, "S5K6AA_STBY");
 	if (ret) {
 		s5k6aa_free_gpios(s5k6aa);
 		return ret;
 	}
-	s5k6aa->gpio[STBY] = *gpio;
-	if (gpio_is_valid(gpio->gpio))
-		gpio_set_value(gpio->gpio, 0);
+	s5k6aa->gpio[STBY] = *pgpio;
 
-	gpio = &pdata->gpio_reset;
-	ret = s5k6aa_configure_gpio(gpio->gpio, gpio->level, "S5K6AA_RST");
-	if (ret) {
-		s5k6aa_free_gpios(s5k6aa);
-		return ret;
+	if (np) {
+		gpio.gpio = of_get_named_gpio(np, "gpio-rst", 0);
+		if (!of_get_property(np, "samsung,s5k6aa-inv-rst", NULL))
+			gpio.level = 1;
+	}
+	pgpio = np ? &gpio : &pdata->gpio_reset;
+	ret = s5k6aa_configure_gpio(pgpio->gpio, pgpio->level, "S5K6AA_RST");
+	if (ret)
+		goto err;
+
+	s5k6aa->gpio[RST] = *pgpio;
+	return 0;
+ err:
+	s5k6aa_free_gpios(s5k6aa);
+	return ret;
+}
+
+static int s5k6aa_get_platform_data(struct s5k6aa *s5k6aa,
+				    struct i2c_client *client)
+{
+	const struct s5k6aa_platform_data *pdata = client->dev.platform_data;
+	struct device_node *np = client->dev.of_node;
+	const char *bus_type;
+
+	if (np == NULL) {
+		if (pdata == NULL) {
+			dev_err(&client->dev, "Platform data not specified\n");
+			return -EINVAL;
+		}
+		s5k6aa->mclk_frequency = pdata->mclk_frequency;
+		s5k6aa->bus_type = pdata->bus_type;
+		s5k6aa->mipi_lanes = pdata->nlanes;
+		s5k6aa->s_power	= pdata->set_power;
+		s5k6aa->inv_hflip = pdata->horiz_flip;
+		s5k6aa->inv_vflip = pdata->vert_flip;
+		return 0;
 	}
-	s5k6aa->gpio[RST] = *gpio;
-	if (gpio_is_valid(gpio->gpio))
-		gpio_set_value(gpio->gpio, 0);
 
+	if (of_property_read_u32(np, "clock-frequency",
+				  &s5k6aa->mclk_frequency))
+		s5k6aa->mclk_frequency = 24000000UL;
+
+	if (of_property_read_u32(np, "data-lanes", &s5k6aa->mipi_lanes))
+		s5k6aa->mipi_lanes = 2;
+
+	if (!of_property_read_string(np, "video-bus-type", &bus_type) &&
+	    !strcmp(bus_type, "mipi-csi2"))
+		s5k6aa->bus_type = V4L2_MBUS_CSI2;
+	else
+		s5k6aa->bus_type = V4L2_MBUS_PARALLEL;
+
+	s5k6aa->inv_hflip = of_property_read_bool(np, "samsung,s5k6aa-hflip");
+	s5k6aa->inv_vflip = of_property_read_bool(np, "samsung,s5k6aa-vflip");
 	return 0;
 }
 
+
 static int s5k6aa_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	const struct s5k6aa_platform_data *pdata = client->dev.platform_data;
 	struct v4l2_subdev *sd;
 	struct s5k6aa *s5k6aa;
-	int i, ret;
+	int i, ret = -EINVAL;
 
-	if (pdata == NULL) {
-		dev_err(&client->dev, "Platform data not specified\n");
-		return -EINVAL;
-	}
+	s5k6aa = devm_kzalloc(&client->dev, sizeof(*s5k6aa), GFP_KERNEL);
+	if (!s5k6aa)
+		return -ENOMEM;
 
-	if (pdata->mclk_frequency == 0) {
+	ret = s5k6aa_get_platform_data(s5k6aa, client);
+	if (ret < 0)
+		return ret;
+
+	if (s5k6aa->mclk_frequency == 0) {
 		dev_err(&client->dev, "MCLK frequency not specified\n");
 		return -EINVAL;
 	}
 
-	s5k6aa = kzalloc(sizeof(*s5k6aa), GFP_KERNEL);
-	if (!s5k6aa)
-		return -ENOMEM;
-
 	mutex_init(&s5k6aa->lock);
 
-	s5k6aa->mclk_frequency = pdata->mclk_frequency;
-	s5k6aa->bus_type = pdata->bus_type;
-	s5k6aa->mipi_lanes = pdata->nlanes;
-	s5k6aa->s_power	= pdata->set_power;
-	s5k6aa->inv_hflip = pdata->horiz_flip;
-	s5k6aa->inv_vflip = pdata->vert_flip;
-
 	sd = &s5k6aa->sd;
 	v4l2_i2c_subdev_init(sd, client, &s5k6aa_subdev_ops);
 	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
@@ -1592,9 +1634,9 @@ static int s5k6aa_probe(struct i2c_client *client,
 	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
 	ret = media_entity_init(&sd->entity, 1, &s5k6aa->pad, 0);
 	if (ret)
-		goto out_err1;
+		return ret;
 
-	ret = s5k6aa_configure_gpios(s5k6aa, pdata);
+	ret = s5k6aa_configure_gpios(s5k6aa, &client->dev);
 	if (ret)
 		goto out_err2;
 
@@ -1627,8 +1669,6 @@ out_err3:
 	s5k6aa_free_gpios(s5k6aa);
 out_err2:
 	media_entity_cleanup(&s5k6aa->sd.entity);
-out_err1:
-	kfree(s5k6aa);
 	return ret;
 }
 
@@ -1642,7 +1682,6 @@ static int s5k6aa_remove(struct i2c_client *client)
 	media_entity_cleanup(&sd->entity);
 	regulator_bulk_free(S5K6AA_NUM_SUPPLIES, s5k6aa->supplies);
 	s5k6aa_free_gpios(s5k6aa);
-	kfree(s5k6aa);
 
 	return 0;
 }
@@ -1653,9 +1692,18 @@ static const struct i2c_device_id s5k6aa_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, s5k6aa_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id s5k6aa_of_match[] __devinitconst = {
+	{ .compatible = "samsung,s5k6aafx" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s5k6aa_of_match);
+#endif
+
 
 static struct i2c_driver s5k6aa_i2c_driver = {
 	.driver = {
+		.of_match_table = of_match_ptr(s5k6aa_of_match),
 		.name = DRIVER_NAME
 	},
 	.probe		= s5k6aa_probe,
@@ -1665,6 +1713,7 @@ static struct i2c_driver s5k6aa_i2c_driver = {
 
 module_i2c_driver(s5k6aa_i2c_driver);
 
+MODULE_ALIAS("i2c:s5k6aafx");
 MODULE_DESCRIPTION("Samsung S5K6AA(FX) SXGA camera driver");
 MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
 MODULE_LICENSE("GPL");
-- 
1.7.10

[RFC/PATCH 10/13] ARM: dts: Add camera devices to exynos4210-nuri.dts

[Sylwester Nawrocki]

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

diff --git a/arch/arm/boot/dts/exynos4210-nuri.dts b/arch/arm/boot/dts/exynos4210-nuri.dts
index 103092c..d04f596 100644
--- a/arch/arm/boot/dts/exynos4210-nuri.dts
+++ b/arch/arm/boot/dts/exynos4210-nuri.dts
@@ -444,6 +444,21 @@
 
 		gpios = <&gpc1 3 4 3 0>,
 			<&gpc1 4 4 3 0>;
+
+		s5k6aafx: s5k6aafx@3c {
+			compatible = "samsung,s5k6aafx";
+			reg = <0x3c>;
+			clock-frequency = <24000000>;
+			gpio-rst = <&gpl2 1 2 0 3>;
+			samsung,s5k6aa-inv-rst;
+			gpio-stby = <&gpl2 0 2 0 3>;
+			samsung,s5k6aa-inv-stby;
+			video-bus-type = "itu-601";
+			vdd_core-supply = <&camv1_5_reg>;
+			vdda-supply = <&camvdda_reg>;
+			vdd_reg-supply = <&vpda_reg>;
+			vddio-supply = <&vtcam_reg>;
+		};
 	};
 
 	i2c7: i2c@138D0000 {
@@ -493,6 +508,7 @@
 			gpio-reset = <&gpf3 4 0 0 0>;
 			hdmi-en-supply = <&hdmi_reg>;
 		};
+
 	};
 
 	g2d: gpu@12800000 {
@@ -564,4 +580,42 @@
 	       reg = <0xf8183c80 0x20>;
 	};
 
+	csis0: csis@11880000 {
+		vddcore-supply = <&vusb_reg>;
+		vddio-supply = <&vmipi_reg>;
+	};
+
+	csis1: csis@11890000 {
+		status = "disabled";
+	};
+
+	camera {
+		compatible = "samsung,fimc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		fimc-controllers = <&fimc0 &fimc1>;
+		csi-rx-controllers = <&csis0>;
+		samsung,camport-a-gpios = <&gpj0 0 2 0 0>,
+					  <&gpj0 1 2 0 0>,
+					  <&gpj0 2 2 0 0>,
+					  <&gpj0 3 2 0 0>,
+					  <&gpj0 4 2 0 0>,
+					  <&gpj0 5 2 0 0>,
+					  <&gpj0 6 2 0 0>,
+					  <&gpj0 7 2 0 0>,
+					  <&gpj1 0 2 0 0>,
+					  <&gpj1 1 2 0 0>,
+					  <&gpj1 2 2 0 0>,
+					  <&gpj1 3 2 0 3>,
+					  <&gpj1 4 2 0 0>;
+		sensor@0 {
+			 i2c-client = <&s5k6aafx>;
+			 samsung,camif-mux-id = <0>;
+			 samsung,fimc-clk-id = <0>;
+			 video-bus-type = "itu-601";
+		};
+	};
+
 };
-- 
1.7.10

[RFC/PATCH 11/13] media: s5p-fimc: Keep local copy of sensors platform data

[Sylwester Nawrocki]

Use a copy of sensor platform data structure, rather than referencing
external platform data from the driver. This allows to fill the local
copy with values parsed from device tree when needed.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |    6 +++---
 drivers/media/video/s5p-fimc/fimc-lite.c    |    2 +-
 drivers/media/video/s5p-fimc/fimc-mdevice.c |   18 +++++++++---------
 drivers/media/video/s5p-fimc/fimc-mdevice.h |    2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 7585b2f..ec259fb 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -50,9 +50,9 @@ static int fimc_capture_hw_init(struct fimc_dev *fimc)
 	fimc_prepare_dma_offset(ctx, &ctx->d_frame);
 	fimc_set_yuv_order(ctx);
 
-	fimc_hw_set_camera_polarity(fimc, sensor->pdata);
-	fimc_hw_set_camera_type(fimc, sensor->pdata);
-	fimc_hw_set_camera_source(fimc, sensor->pdata);
+	fimc_hw_set_camera_polarity(fimc, &sensor->pdata);
+	fimc_hw_set_camera_type(fimc, &sensor->pdata);
+	fimc_hw_set_camera_source(fimc, &sensor->pdata);
 	fimc_hw_set_camera_offset(fimc, &ctx->s_frame);
 
 	ret = fimc_set_scaler_info(ctx);
diff --git a/drivers/media/video/s5p-fimc/fimc-lite.c b/drivers/media/video/s5p-fimc/fimc-lite.c
index a7ae149..4ee5b70 100644
--- a/drivers/media/video/s5p-fimc/fimc-lite.c
+++ b/drivers/media/video/s5p-fimc/fimc-lite.c
@@ -165,7 +165,7 @@ static int fimc_lite_hw_init(struct fimc_lite *fimc)
 	sensor = v4l2_get_subdev_hostdata(pipeline->subdevs[IDX_SENSOR]);
 	spin_lock_irqsave(&fimc->slock, flags);
 
-	flite_hw_set_camera_bus(fimc, sensor->pdata);
+	flite_hw_set_camera_bus(fimc, &sensor->pdata);
 	flite_hw_set_source_format(fimc, &fimc->inp_frame);
 	flite_hw_set_window_offset(fimc, &fimc->inp_frame);
 	flite_hw_set_output_dma(fimc, &fimc->out_frame, true);
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c b/drivers/media/video/s5p-fimc/fimc-mdevice.c
index 92c9887..1b3b13c 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -244,27 +244,27 @@ static struct v4l2_subdev *fimc_md_register_sensor(struct fimc_md *fmd,
 	if (!s_info || !fmd)
 		return NULL;
 
-	adapter = i2c_get_adapter(s_info->pdata->i2c_bus_num);
+	adapter = i2c_get_adapter(s_info->pdata.i2c_bus_num);
 	if (!adapter) {
 		v4l2_warn(&fmd->v4l2_dev,
 			  "Failed to get I2C adapter %d, deferring probe\n",
-			  s_info->pdata->i2c_bus_num);
+			  s_info->pdata.i2c_bus_num);
 		return ERR_PTR(-EPROBE_DEFER);
 	}
 	sd = v4l2_i2c_new_subdev_board(&fmd->v4l2_dev, adapter,
-				       s_info->pdata->board_info, NULL);
+				       s_info->pdata.board_info, NULL);
 	if (IS_ERR_OR_NULL(sd)) {
 		i2c_put_adapter(adapter);
 		v4l2_warn(&fmd->v4l2_dev,
 			  "Failed to acquire subdev %s, deferring probe\n",
-			  s_info->pdata->board_info->type);
+			  s_info->pdata.board_info->type);
 		return ERR_PTR(-EPROBE_DEFER);
 	}
 	v4l2_set_subdev_hostdata(sd, s_info);
 	sd->grp_id = SENSOR_GROUP_ID;
 
 	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice %s\n",
-		  s_info->pdata->board_info->type);
+		  s_info->pdata.board_info->type);
 	return sd;
 }
 
@@ -308,7 +308,7 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 	for (i = 0; i < num_clients; i++) {
 		struct v4l2_subdev *sd;
 
-		fmd->sensor[i].pdata = &pdata->isp_info[i];
+		fmd->sensor[i].pdata = pdata->isp_info[i];
 		ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
 		if (ret)
 			break;
@@ -713,11 +713,11 @@ static int fimc_md_create_links(struct fimc_md *fmd)
 
 		sensor = fmd->sensor[i].subdev;
 		s_info = v4l2_get_subdev_hostdata(sensor);
-		if (!s_info || !s_info->pdata)
+		if (!s_info)
 			continue;
 
 		source = NULL;
-		pdata = s_info->pdata;
+		pdata = &s_info->pdata;
 
 		switch (pdata->bus_type) {
 		case FIMC_MIPI_CSI2:
@@ -827,7 +827,7 @@ static int __fimc_md_set_camclk(struct fimc_md *fmd,
 					 struct fimc_sensor_info *s_info,
 					 bool on)
 {
-	struct s5p_fimc_isp_info *pdata = s_info->pdata;
+	struct s5p_fimc_isp_info *pdata = &s_info->pdata;
 	struct fimc_camclk_info *camclk;
 	int ret = 0;
 
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.h b/drivers/media/video/s5p-fimc/fimc-mdevice.h
index 3b8a349..bba85bf 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.h
@@ -52,7 +52,7 @@ struct fimc_camclk_info {
  * This data structure applies to image sensor and the writeback subdevs.
  */
 struct fimc_sensor_info {
-	struct s5p_fimc_isp_info *pdata;
+	struct s5p_fimc_isp_info pdata;
 	struct v4l2_subdev *subdev;
 	struct fimc_dev *host;
 	bool clk_on;
-- 
1.7.10

[RFC/PATCH 12/13] media: s5p-fimc: Add device tree based sensors registration

[Sylwester Nawrocki]

Add parsing of 'sensor' nodes specified as 'camera' child nodes.
Each 'sensor' node should contain a phandle indicating sensor I2C
client device. Sensors with SPI control bus are not yet supported.

Additionally it is required that the I2C client node (child node
of I2C bus controller node) contains 'clock-frequency' and
'video-bus-type' properties. These properties allow the host
controller to switch to proper video bus and set proper master
clock frequency for a sensor.

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

We might need something like this as a V4L2 core function.
But I wanted to have something settled first until proposing
any addtions to the V4L2 core modules.
---
 .../bindings/camera/soc/samsung-fimc.txt           |   22 ++++
 drivers/media/video/s5p-fimc/fimc-mdevice.c        |  114 +++++++++++++++++++-
 2 files changed, 133 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
index b459da2..ffe09ac 100644
--- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
@@ -54,6 +54,28 @@ Required properties:
 - cell-index : FIMC-LITE IP instance index;
 
 
+The 'sensor' nodes
+------------------
+
+Required properties:
+
+ - i2c-client : a phandle to an image sensor I2C client device;
+
+Optional properties:
+
+- samsung,camif-mux-id : FIMC video multiplexer input index; for camera
+			 port A, B, C the indexes are 0, 1, 0 respectively.
+			 If this property is not specified a default 0
+			 value will be used by driver.
+
+- samsung,fimc-camclk-id : the SoC CAM_MCLK clock output index. These clocks
+			   are master clocks for external image processors.
+			   If this property is not specified a default 0 value
+			   will be used by driver.
+
+ "video-bus-type" and "clock-frequency" properties must be specified at the
+ node referenced by 'i2c-client' phandle.
+
 Example:
 
 	fimc0: fimc@11800000 {
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c b/drivers/media/video/s5p-fimc/fimc-mdevice.c
index 1b3b13c..faf5665 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -19,6 +19,9 @@
 #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>
@@ -278,10 +281,109 @@ static void fimc_md_unregister_sensor(struct v4l2_subdev *sd)
 	v4l2_device_unregister_subdev(sd);
 	adapter = client->adapter;
 	i2c_unregister_device(client);
-	if (adapter)
+	if (adapter && client->dev.of_node == NULL)
 		i2c_put_adapter(adapter);
 }
 
+#ifdef CONFIG_OF
+static struct v4l2_subdev *fimc_md_create_sensor_subdev(struct fimc_md *fmd,
+						struct i2c_client *client,
+						struct fimc_sensor_info *sensor)
+{
+	struct v4l2_subdev *sd;
+	int ret;
+
+	if (!client->driver)
+		return ERR_PTR(-EAGAIN);
+
+	if (!try_module_get(client->driver->driver.owner))
+		return ERR_PTR(-EAGAIN);
+
+	/* Enable sensor's master clock */
+	ret = __fimc_md_set_camclk(fmd, sensor, true);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	sd = i2c_get_clientdata(client);
+
+	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+	module_put(client->driver->driver.owner);
+	__fimc_md_set_camclk(fmd, sensor, false);
+	if (ret)
+		return ERR_PTR(ret);
+
+	v4l2_set_subdev_hostdata(sd, sensor);
+	sd->grp_id = SENSOR_GROUP_ID;
+	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s\n",
+		  sd->name);
+
+	return sd;
+}
+
+static int fimc_md_of_sensors_register(struct fimc_md *fmd,
+				       struct device_node *np)
+{
+	struct device_node *i2c_node, *node = NULL;
+	struct s5p_fimc_isp_info *pdata;
+	struct fimc_sensor_info *sensor;
+	int ret, sensor_index = 0;
+	const char *bt;
+	u32 id, freq;
+
+	for_each_child_of_node(np, node) {
+		struct i2c_client *client = NULL;
+		struct v4l2_subdev *sd;
+
+		sensor = &fmd->sensor[sensor_index];
+		pdata = &sensor->pdata;
+
+		i2c_node = of_parse_phandle(node, "i2c-client", 0);
+		if (i2c_node)
+			client = of_find_i2c_device_by_node(i2c_node);
+		if (client == NULL) {
+			of_node_put(i2c_node);
+			return -EPROBE_DEFER;
+		}
+
+		ret = of_property_read_u32(node, "samsung,fimc-camclk-id", &id);
+		pdata->clk_id = ret ? 0 : id;
+
+		ret = of_property_read_u32(i2c_node, "clock-frequency", &freq);
+		pdata->clk_frequency = ret ? 12000000UL : freq;
+
+		ret = of_property_read_u32(node, "samsung,fimc-mux-id", &id);
+		pdata->mux_id = ret ? 0 : id;
+
+		if(!of_property_read_string(node, "video-bus-type", &bt)) {
+			if (!strcmp(bt, "itu-601"))
+				pdata->bus_type = FIMC_ITU_601;
+			else if (!strcmp(bt, "mipi-csi2"))
+				pdata->bus_type = FIMC_MIPI_CSI2;
+			else if (!strcmp(bt, "itu-656"))
+				pdata->bus_type = FIMC_ITU_656;
+		}
+
+		of_node_put(node);
+		put_device(&client->dev);
+		if (WARN_ON(pdata->bus_type == 0))
+			continue;
+
+		sd = fimc_md_create_sensor_subdev(fmd, client, sensor);
+
+		if (IS_ERR(sd)) {
+			sensor->subdev = NULL;
+			return PTR_ERR(sd);
+		}
+
+		sensor->subdev = sd;
+		sensor_index++;
+
+		fmd->num_sensors++;
+	}
+	return 0;
+}
+#endif /* CONFIG_OF */
+
 static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 {
 	struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
@@ -300,7 +402,13 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 	ret = pm_runtime_get_sync(&fd->pdev->dev);
 	if (ret < 0)
 		return ret;
-
+#ifdef CONFIG_OF
+	if (fmd->pdev->dev.of_node) {
+		ret = fimc_md_of_sensors_register(fmd, fmd->pdev->dev.of_node);
+		pm_runtime_put(&fd->pdev->dev);
+		return ret;
+	}
+#endif
 	WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
 	num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
 
@@ -1047,7 +1155,7 @@ static int fimc_md_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unlock;
 
-	if (pdev->dev.platform_data) {
+	if (pdev->dev.platform_data || pdev->dev.of_node) {
 		ret = fimc_md_register_sensor_entities(fmd);
 		if (ret)
 			goto err_unlock;
-- 
1.7.10

[RFC/PATCH 13/13] media: s5p-fimc: Add parallel video port pin configuration

[Sylwester Nawrocki]

This patch adds parsing of 'samsung,fimc-camport-a-gpios' and
'samsung,fimc-camport-b-gpios' properties from 'camera' node
and configuration of camera parallel port pins. It can be
in future replaced with equivalent pinctrl API calls.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyugmin Park <kyungmin.park@samsung.com>
---
 .../bindings/camera/soc/samsung-fimc.txt           |   12 +++
 drivers/media/video/s5p-fimc/fimc-mdevice.c        |   87 ++++++++++++++++++++
 drivers/media/video/s5p-fimc/fimc-mdevice.h        |   10 +++
 3 files changed, 109 insertions(+)

diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
index ffe09ac..efa5be5 100644
--- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
@@ -23,6 +23,18 @@ Optional properties:
 - csi-rx-controllers : an array of phandles to 'csis' device nodes,
 		       it is required for sensors with MIPI-CSI2 bus;
 
+- samsung,camport-a-gpios, samsung,camport-b-gpios : gpio specifier list for
+			   the parallel camera ports A and B respectively;
+
+  For 'video-itu-601-bus' the list length must be 13, for 'video-itu-656-bus'
+  at least 10 and for 'video-mipi-csi2-bus' the array should contain at least
+  one gpio (CLKOUT). The meaning of the gpios is as following: DATA[7:0], PCLK,
+  VSYNC, HREF, CLKOUT, FIELD. It is not required to list gpios exactly in that
+  order. All gpios listed here will be configured into camera port function.
+  In case of ITU-R BT.656 bus the VSYNC, HREF, and FIELD pins remain unused,
+  hence don't need to be specified here.
+
+
 'fimc' device node
 ------------------
 
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c b/drivers/media/video/s5p-fimc/fimc-mdevice.c
index faf5665..31bf852 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -1108,6 +1108,87 @@ 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_gpios(struct device_node *np,
+			       struct fimc_video_port *port,
+			       const char *prop_name)
+{
+	int i, err = -EINVAL;
+
+	for (i = 0; i < FIMC_MAX_PARALLEL_PORT_PINS; i++) {
+		port->gpios[i] = of_get_named_gpio(np, prop_name, i);
+		if (port->gpios[i] == -ENOENT && i > 0)
+			return 0;
+
+		if (gpio_is_valid(port->gpios[i])) {
+			err = gpio_request(port->gpios[i], "fimc");
+			if (!err)
+				continue;
+		}
+
+		pr_err("Failed to configure gpios: %s\n", prop_name);
+
+		while (--i >= 0) {
+			gpio_free(port->gpios[i]);
+			port->gpios[i] = -EINVAL;
+		}
+		break;
+	}
+
+	return err;
+}
+
+static int fimc_md_camport_setup(struct fimc_md *fmd, struct device_node *np)
+{
+	const char *gpio_props[] = { "samsung,camport-a-gpios",
+				     "samsung,camport-b-gpios" };
+	struct fimc_video_port *pport;
+	unsigned int ret;
+	int i;
+
+	pport = devm_kzalloc(&fmd->pdev->dev, FIMC_MAX_PARALLEL_PORTS *
+			     sizeof(*pport), GFP_KERNEL);
+	if (pport == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_props); i++) {
+		if (!of_find_property(np, gpio_props[i], NULL))
+			continue;
+		ret = __fimc_md_get_gpios(np, &pport[i], gpio_props[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	fmd->parallel_ports = pport;
+	return ret;
+}
+
+static void fimc_md_camport_release(struct fimc_md *fmd)
+{
+	struct fimc_video_port *pport = fmd->parallel_ports;
+	int port, i;
+
+	if (pport == NULL)
+		return;
+
+	for (port = 0; port < FIMC_MAX_PARALLEL_PORTS; port++, pport++) {
+		for (i = 0; i < FIMC_MAX_PARALLEL_PORT_PINS; i++) {
+			if (!gpio_is_valid(pport->gpios[i]))
+				continue;
+			gpio_free(pport->gpios[i]);
+		}
+	}
+}
+
+static int fimc_md_parse_dt(struct fimc_md *fmd)
+{
+	struct device_node *np = fmd->pdev->dev.of_node;
+
+	if (np == NULL)
+		return 0;
+
+	return fimc_md_camport_setup(fmd, np);
+}
+
 static int fimc_md_probe(struct platform_device *pdev)
 {
 	struct v4l2_device *v4l2_dev;
@@ -1155,11 +1236,16 @@ static int fimc_md_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unlock;
 
+	ret = fimc_md_parse_dt(fmd);
+	if (ret < 0)
+		goto err_unlock;
+
 	if (pdev->dev.platform_data || pdev->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;
@@ -1196,6 +1282,7 @@ static int __devexit fimc_md_remove(struct platform_device *pdev)
 	fimc_md_unregister_entities(fmd);
 	media_device_unregister(&fmd->media_dev);
 	fimc_md_put_clocks(fmd);
+	fimc_md_camport_release(fmd);
 	return 0;
 }
 
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.h b/drivers/media/video/s5p-fimc/fimc-mdevice.h
index bba85bf..1f7ebf8 100644
--- a/drivers/media/video/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.h
@@ -30,6 +30,10 @@
 
 #define FIMC_MAX_SENSORS	8
 #define FIMC_MAX_CAMCLKS	2
+/* Maximum number of supported parallel video ports */
+#define FIMC_MAX_PARALLEL_PORTS		2
+/* Maximum number of pins per 8-bit parallel port */
+#define FIMC_MAX_PARALLEL_PORT_PINS	13
 
 struct fimc_csis_info {
 	struct v4l2_subdev *sd;
@@ -58,8 +62,13 @@ struct fimc_sensor_info {
 	bool clk_on;
 };
 
+struct fimc_video_port {
+	int gpios[FIMC_MAX_PARALLEL_PORT_PINS];
+};
+
 /**
  * struct fimc_md - fimc media device information
+ * @parallel_ports: parallel video ports data
  * @csis: MIPI CSIS subdevs data
  * @sensor: array of registered sensor subdevs
  * @num_sensors: actual number of registered sensors
@@ -72,6 +81,7 @@ struct fimc_sensor_info {
  * @slock: spinlock protecting @sensor array
  */
 struct fimc_md {
+	struct fimc_video_port *parallel_ports;
 	struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
 	struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
 	int num_sensors;
-- 
1.7.10

[PATCH 14/14] s5p-fimc: Add FIMC and MIPI-CSIS devices to CAM power domain

[Sylwester Nawrocki]

From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-core.c |   15 +++++++++++++++
 drivers/media/video/s5p-fimc/mipi-csis.c |   15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index 30c6365..15c7cc6 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -1057,6 +1058,17 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
 	return 0;
 }
 
+static void bus_add_dev_to_pd(struct device *dev)
+{
+	struct device_node *np;
+
+	np = of_parse_phandle(dev->of_node, "pd", 0);
+	if (np)
+		pm_genpd_of_add_device(np, dev);
+
+	of_node_put(np);
+}
+
 static int fimc_probe(struct platform_device *pdev)
 {
 	struct fimc_drvdata *drv_data = NULL;
@@ -1072,6 +1084,9 @@ static int fimc_probe(struct platform_device *pdev)
 
 	if (pdev->dev.of_node) {
 		u32 id = 0;
+
+		bus_add_dev_to_pd(&pdev->dev);
+
 		of_id = of_match_node(fimc_of_match, pdev->dev.of_node);
 		if (of_id)
 			drv_data = of_id->data;
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c
index ffb820e..6858c92 100644
--- a/drivers/media/video/s5p-fimc/mipi-csis.c
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -22,6 +22,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -529,6 +530,17 @@ static int s5pcsis_get_platform_data(struct platform_device *pdev,
 	return 0;
 }
 
+static void bus_add_dev_to_pd(struct device *dev)
+{
+	struct device_node *np;
+
+	np = of_parse_phandle(dev->of_node, "pd", 0);
+	if (np)
+		pm_genpd_of_add_device(np, dev);
+
+	of_node_put(np);
+}
+
 static int __devinit s5pcsis_probe(struct platform_device *pdev)
 {
 	struct resource *mem_res;
@@ -543,6 +555,9 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 	mutex_init(&state->lock);
 	state->pdev = pdev;
 
+	if (pdev->dev.of_node)
+		bus_add_dev_to_pd(&pdev->dev);
+
 	ret = s5pcsis_get_platform_data(pdev, state);
 	if (ret < 0)
 		return ret;
-- 
1.7.10

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Guennadi Liakhovetski]

Hi Sylwester

Thanks for your comments to my RFC and for pointing out to this your 
earlier patch series. Unfortunately, I missed in in May, let me try to 
provide some thoughts about this, we should really try to converge our 
proposals. Maybe a discussion at KS would help too.

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
> (camera host interface DMA engine and image processor). This patch
> adds support for instantiating the MIPI-CSIS devices from DT and
> parsing all SoC and board specific properties from device tree.
> The MIPI DPHY control callback is now called directly from within
> the driver, the platform code must ensure this callback does the
> right thing for each SoC.
> 
> The cell-index property is used to ensure proper signal routing,
> from physical camera port, through MIPI-CSI2 receiver to the DMA
> engine (FIMC?). It's also helpful in exposing the device topology
> in user space at /dev/media? devnode (Media Controller API).
> 
> This patch also defines a common property ("data-lanes") for MIPI-CSI
> receivers and transmitters.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/devicetree/bindings/video/mipi.txt   |    5 +
>  .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
>  drivers/media/video/s5p-fimc/mipi-csis.c           |   97 +++++++++++++++-----
>  drivers/media/video/s5p-fimc/mipi-csis.h           |    1 +
>  4 files changed, 126 insertions(+), 24 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
>  create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt
> new file mode 100644
> index 0000000..5aed285
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> @@ -0,0 +1,5 @@
> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
> +
> + - data-lanes : number of differential data lanes wired and actively used in
> +		communication between the transmitter and the receiver, this
> +		excludes the clock lane;

Wouldn't it be better to use the standard "bus-width" DT property?

> diff --git a/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
> new file mode 100644
> index 0000000..7bce6f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
> @@ -0,0 +1,47 @@
> +Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
> +-----------------------------------------------------
> +
> +Required properties:
> +
> +- compatible - one of :
> +		"samsung,s5pv210-csis",
> +		"samsung,exynos4210-csis",
> +		"samsung,exynos4212-csis",
> +		"samsung,exynos4412-csis";
> +- 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;
> +- cell-index      : the hardware instance index;

Not sure whether this is absolutely needed... Wouldn't it be sufficient to 
just enumerate them during probing?

> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, the default
> +		    value when this property is not specified is 166 MHz;
> +- data-lanes      : number of physical MIPI-CSI2 lanes used;

ditto - bus-width?

> +- samsung,csis-hs-settle : differential receiver (HS-RX) settle time;
> +- 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).
> +
> +Example:
> +
> +	reg0: regulator@0 {
> +	};
> +
> +	reg1: regulator@1 {
> +	};
> +
> +/* SoC properties */
> +
> +	csis@11880000 {
> +		compatible = "samsung,exynos4210-csis";
> +		reg = <0x11880000 0x1000>;
> +		interrupts = <0 78 0>;
> +		cell-index = <0>;
> +	};
> +
> +/* Board properties */
> +
> +	csis@11880000 {
> +		clock-frequency = <166000000>;
> +		data-lanes = <2>;
> +		samsung,csis-hs-settle = <12>;
> +		vddio-supply = <&reg0>;
> +		vddcore-supply = <&reg1>;
> +	};

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 04/13] devicetree: Add common video devices bindings documentation

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/devicetree/bindings/video/video.txt |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/video.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/video.txt b/Documentation/devicetree/bindings/video/video.txt
> new file mode 100644
> index 0000000..9f6a637
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/video.txt
> @@ -0,0 +1,10 @@
> +Common properties of video data source devices (image sensor, video encoders, etc.)
> +
> +Video bus types
> +---------------
> +
> +- video-bus-type : must be one of:
> +
> +    - itu-601   : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> +    - itu-656   : parallel bus with embedded synchronization - ITU-R BT.656;

wikipedia tells me, that bt.601 is mostly a data encoding standard, it 
also defines bus-types, but recent versions also include serial busses.

> +    - mipi-csi2 : MIPI-CSI2 serial bus;

In general: are these at all needed? Wouldn't it be better to specify pads 
on sensors and interfaces to differentiate between serial and parallel 
connections. As for whether HSYNC and VSYNC are used - I see 3 
possibilities there:

1. real sync signals are used - the default.

2. embedded syncs shall be used, because sync signals are missing. This 
should (arguably) be derived from pinctrl - see this discussion:

http://lkml.indiana.edu/hypermail/linux/kernel/1205.2/index.html#03893

3. sync signals are present, but cannot be used, because one of the 
partners doesn't support them - .g_mbus_config() can be used to retrieve 
this information.

4. sync signals are available and supported by both peers, but for some 
reason the user prefers to use embedded sync data - is such a case 
feasible? Even if so, this should be run-time configurable then?

So, maybe we don't need these at all.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

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

>From the documentation below I think, I understand what it does, but why 
is it needed? It doesn't describe your video subsystem topology, right? 
How various subdevices are connected. It just lists them all in one 
node... A description for this patch would be very welcome IMHO and, 
maybe, such a node can be completely avoided?

Thanks
Guennadi

> ---
>  .../bindings/camera/soc/samsung-fimc.txt           |   66 ++++
>  drivers/media/video/s5p-fimc/fimc-capture.c        |    2 +-
>  drivers/media/video/s5p-fimc/fimc-core.c           |  410 +++++++++++---------
>  drivers/media/video/s5p-fimc/fimc-core.h           |    2 -
>  drivers/media/video/s5p-fimc/fimc-mdevice.c        |    8 +-
>  5 files changed, 291 insertions(+), 197 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> 
> diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> new file mode 100644
> index 0000000..1ec48e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> @@ -0,0 +1,66 @@
> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
> +----------------------------------------------
> +
> +The Exynos Camera subsystem uses a dedicated device node associated with
> +top level device driver that manages common properties of the whole subsystem,
> +like common camera port pins or clocks for external image sensors. This
> +aggregate node references related platform sub-devices - FIMC, FIMC-LITE,
> +MIPI-CSIS [1], and it also contains nodes describing image sensors wired to
> +the host SoC's video port and using I2C or SPI as the control bus.
> +
> +
> +Common 'camera' node
> +--------------------
> +
> +Required properties:
> +
> +- compatible	   : must be "samsung,fimc"
> +- fimc-controllers : an array of phandles to 'fimc' device nodes,
> +		     size of this array must be at least 1;
> +
> +Optional properties:
> +
> +- csi-rx-controllers : an array of phandles to 'csis' device nodes,
> +		       it is required for sensors with MIPI-CSI2 bus;
> +
> +'fimc' device node
> +------------------
> +
> +Required properties:
> +
> +- compatible : should be one of:
> +		"samsung,s5pv210-fimc"
> +		"samsung,exynos4210-fimc";
> +		"samsung,exynos4412-fimc";
> +- reg	     : physical base address and size of the device memory mapped
> +	       registers;
> +- interrupts : FIMC interrupt to the CPU should be described here;
> +- cell-index : FIMC IP instance index, the number of available instances
> +	       depends on the SoC revision. For S5PV210 valid values are:
> +	       0...2, for Exynos4x1x: 0...3.
> +
> +Example:
> +
> +	fimc0: fimc@11800000 {
> +		compatible = "samsung,exynos4210-fimc";
> +		reg = <0x11800000 0x1000>;
> +		interrupts = <0 85 0>;
> +		cell-index = <0>;
> +	};
> +
> +	csis0: csis@11880000 {
> +		compatible = "samsung,exynos4210-csis";
> +		reg = <0x11880000 0x1000>;
> +		interrupts = <0 78 0>;
> +		cell-index = <0>;
> +	};
> +
> +	camera {
> +		compatible = "samsung,fimc";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		fimc-controllers = <&fimc0>;
> +		csi-rx-controllers = <&csis0>;
> +	};
> +
> +[1] Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../bindings/camera/soc/samsung-fimc.txt           |   15 ++++
>  drivers/media/video/s5p-fimc/fimc-lite.c           |   73 ++++++++++++++------
>  2 files changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> index 1ec48e9..b459da2 100644
> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> @@ -39,6 +39,21 @@ Required properties:
>  	       depends on the SoC revision. For S5PV210 valid values are:
>  	       0...2, for Exynos4x1x: 0...3.
>  
> +
> +'fimc-lite' device node
> +-----------------------
> +
> +Required properties:
> +
> +- compatible : should be one of:
> +		"samsung,exynos4212-fimc";
> +		"samsung,exynos4412-fimc";
> +- reg	     : physical base address and size of the device's memory mapped
> +	       registers;
> +- interrupts : should contain FIMC-LITE interrupt;
> +- cell-index : FIMC-LITE IP instance index;

Same as in an earlier patch - not sure this is needed.

Thanks
Guennadi

> +
> +
>  Example:
>  
>  	fimc0: fimc@11800000 {
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

> The driver initializes all board related properties except the s_power()
> callback to board code. The platforms that require this callback are not
> supported by this driver yet for CONFIG_OF=y.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
>  drivers/media/video/s5k6aa.c                       |  129 ++++++++++++++------
>  2 files changed, 146 insertions(+), 40 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> 
> diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> new file mode 100644
> index 0000000..6685a9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> @@ -0,0 +1,57 @@
> +Samsung S5K6AAFX camera sensor
> +------------------------------
> +
> +Required properties:
> +
> +- compatible : "samsung,s5k6aafx";
> +- reg : base address of the device on I2C bus;

You said you ended up putting your sensors outside of I2C busses, is this 
one of changes, that are present in your git-tree but not in this series?

> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;

If this is a boolean property, it cannot be required? Otherwise, as 
discussed in a different patch comment, this property might not be 
required, and if it is, I would use the same definition for both the 
camera interface and for sensors.

> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
> +		   or 2.8V (2.6V to 3.0);

I think, underscores in property names are generally frowned upon.

> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
> +		 or 2.8V (2.5V to 3.1V);
> +
> +Optional properties:
> +
> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default
> +		    value when this property is not specified is 24 MHz;
> +- data-lanes : number of physical lanes used (default 2 if not specified);

bus-width?

> +- gpio-stby : specifies the S5K6AA_STBY GPIO
> +- gpio-rst : specifies the S5K6AA_RST GPIO

>From Documentation/devicetree/bindings/gpio/gpio.txt:

<quote>
GPIO properties should be named "[<name>-]gpios".
</quote>

> +- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
> +- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;

Isn't this provided by GPIO flags as described in include/linux/of_gpio.h 
by using the OF_GPIO_ACTIVE_LOW bit?

> +- samsung,s5k6aa-hflip : set the default horizontal image flipping;
> +- samsung,s5k6aa-vflip : set the default vertical image flipping;

This is a board property, specifying how the sensor is wired and mounted 
on the casing, right? IIRC, libv4l has a database of these for USB 
cameras. So, maybe it belongs to the user-space for embedded systems too? 
Or at least this shouldn't be Samsung-specific?

> +
> +
> +Example:
> +
> +	gpl2: gpio-controller@0 {
> +	};
> +
> +	reg0: regulator@0 {
> +	};
> +
> +	reg1: regulator@1 {
> +	};
> +
> +	reg2: regulator@2 {
> +	};
> +
> +	reg3: regulator@3 {
> +	};
> +
> +	s5k6aafx@3c {
> +		compatible = "samsung,s5k6aafx";
> +		reg = <0x3c>;
> +		clock-frequency = <24000000>;
> +		gpio-rst = <&gpl2 0 2 0 3>;
> +		gpio-stby = <&gpl2 1 2 0 3>;
> +		video-itu-601-bus;
> +		vdd_core-supply = <&reg0>;
> +		vdda-supply = <&reg1>;
> +		vdd_reg-supply = <&reg2>;
> +		vddio-supply = <&reg3>;
> +	};

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 12/13] media: s5p-fimc: Add device tree based sensors registration

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Sylwester Nawrocki wrote:

> Add parsing of 'sensor' nodes specified as 'camera' child nodes.
> Each 'sensor' node should contain a phandle indicating sensor I2C
> client device. Sensors with SPI control bus are not yet supported.
> 
> Additionally it is required that the I2C client node (child node
> of I2C bus controller node) contains 'clock-frequency' and
> 'video-bus-type' properties. These properties allow the host
> controller to switch to proper video bus and set proper master
> clock frequency for a sensor.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> We might need something like this as a V4L2 core function.
> But I wanted to have something settled first until proposing
> any addtions to the V4L2 core modules.
> ---
>  .../bindings/camera/soc/samsung-fimc.txt           |   22 ++++
>  drivers/media/video/s5p-fimc/fimc-mdevice.c        |  114 +++++++++++++++++++-
>  2 files changed, 133 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> index b459da2..ffe09ac 100644
> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> @@ -54,6 +54,28 @@ Required properties:
>  - cell-index : FIMC-LITE IP instance index;
>  
>  
> +The 'sensor' nodes
> +------------------
> +
> +Required properties:
> +
> + - i2c-client : a phandle to an image sensor I2C client device;
> +
> +Optional properties:
> +
> +- samsung,camif-mux-id : FIMC video multiplexer input index; for camera
> +			 port A, B, C the indexes are 0, 1, 0 respectively.
> +			 If this property is not specified a default 0
> +			 value will be used by driver.

Isn't it possible to have several clients connected to different mux 
inputs and switch between them at run-time? Even if only one of them can 
be active at any time? That's why I've introduced link nodes in my RFC to 
specify exactly which pads are connected.

> +
> +- samsung,fimc-camclk-id : the SoC CAM_MCLK clock output index. These clocks
> +			   are master clocks for external image processors.
> +			   If this property is not specified a default 0 value
> +			   will be used by driver.
> +
> + "video-bus-type" and "clock-frequency" properties must be specified at the
> + node referenced by 'i2c-client' phandle.
> +
>  Example:
>  
>  	fimc0: fimc@11800000 {

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Sylwester Nawrocki]

Hi Guennadi,

On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote:
> Hi Sylwester
>
> Thanks for your comments to my RFC and for pointing out to this your
> earlier patch series. Unfortunately, I missed in in May, let me try to
> provide some thoughts about this, we should really try to converge our
> proposals. Maybe a discussion at KS would help too.

Thank you for the review. I was happy to see your RFC, as previously
there seemed to be not much interest in DT among the media guys.
Certainly, we need to work on a common approach to ensure interoperability
of existing drivers and to avoid having people inventing different
bindings for common features. I would also expect some share of device
specific bindings, as diversity of media devices is significant.

I'd be great to discuss these things at KS, especially support for 
proper suspend/resume sequences. Also having common sessions with 
other subsystems folks, like ASoC, for example, might be a good idea.

I'm not sure if I'll be travelling to the KS though. :)

> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
>
>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>> (camera host interface DMA engine and image processor). This patch
>> adds support for instantiating the MIPI-CSIS devices from DT and
>> parsing all SoC and board specific properties from device tree.
>> The MIPI DPHY control callback is now called directly from within
>> the driver, the platform code must ensure this callback does the
>> right thing for each SoC.
>>
>> The cell-index property is used to ensure proper signal routing,
>> from physical camera port, through MIPI-CSI2 receiver to the DMA
>> engine (FIMC?). It's also helpful in exposing the device topology
>> in user space at /dev/media? devnode (Media Controller API).
>>
>> This patch also defines a common property ("data-lanes") for MIPI-CSI
>> receivers and transmitters.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/video/mipi.txt   |    5 +
>>   .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
>>   drivers/media/video/s5p-fimc/mipi-csis.c           |   97 +++++++++++++++-----
>>   drivers/media/video/s5p-fimc/mipi-csis.h           |    1 +
>>   4 files changed, 126 insertions(+), 24 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
>>   create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt
>> new file mode 100644
>> index 0000000..5aed285
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
>> @@ -0,0 +1,5 @@
>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
>> +
>> + - data-lanes : number of differential data lanes wired and actively used in
>> +		communication between the transmitter and the receiver, this
>> +		excludes the clock lane;
>
> Wouldn't it be better to use the standard "bus-width" DT property?

I can't see any problems with using "bus-width". It seems sufficient
and could indeed be better, without a need to invent new MIPI-CSI 
specific names. That was my first RFC on that and my perspective 
wasn't probably broad enough. :)

>> diff --git a/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>> new file mode 100644
>> index 0000000..7bce6f4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/samsung-mipi-csis.txt
>> @@ -0,0 +1,47 @@
>> +Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
>> +-----------------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible - one of :
>> +		"samsung,s5pv210-csis",
>> +		"samsung,exynos4210-csis",
>> +		"samsung,exynos4212-csis",
>> +		"samsung,exynos4412-csis";
>> +- 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;
>> +- cell-index      : the hardware instance index;
>
> Not sure whether this is absolutely needed... Wouldn't it be sufficient to
> just enumerate them during probing?

As Grant pointed to me, the "cell-index" property is something that we should
be avoiding. But I needed something like this in the driver,
to differentiate between the multiple IP instances. I cannot simply assign
the indexes in random way to the hardware instances. Each of the MIPI-CSI 
Slaves has different properties (e.g. supporting max. 2 or 4 data lanes).
Additionally, a particular MIPI-CSI Slave instance is hard-wired inside the
SoC to the FIMC input mux (cross-bar) and physical video port. IOW, an image
sensor/video port/MIPI-CSIS instance assignment is fixed. If two MIPI-CSI 
image sensors are connected, one of them will work only with MIPI-CSIS0, and
the other only with MIPI-CSIS1.

So the driver must be able to identify it's physical device (IO region + 
IRQ) precisely.

That said, I found out recently that a proper entries in the "aliases"
could be used instead, and I could finally abandon the "cell-index" idea.
Not sure how aliases approach is better but from what I can see it is
a preferred way to handle things like the above.

Please see Documentation/devicetree/bindings/sound/mxs-saif.txt and users
of of_alias_get_id() for more details.

So to summarize, the indexes are needed, but the current implementation 
is not necessarily good, and AFAICT the aliases approach could be the way
to go.

>> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, the default
>> +		    value when this property is not specified is 166 MHz;
>> +- data-lanes      : number of physical MIPI-CSI2 lanes used;
>
> ditto - bus-width?

Yes, agreed. Let's drop it in favour of "bus-width".

--

Regards,
Sylwester

Re: [RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Sylwester Nawrocki]

On 07/16/2012 11:15 AM, Guennadi Liakhovetski wrote:
>> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> @@ -39,6 +39,21 @@ Required properties:
>>   	       depends on the SoC revision. For S5PV210 valid values are:
>>   	       0...2, for Exynos4x1x: 0...3.
>>
>> +
>> +'fimc-lite' device node
>> +-----------------------
>> +
>> +Required properties:
>> +
>> +- compatible : should be one of:
>> +		"samsung,exynos4212-fimc";
>> +		"samsung,exynos4412-fimc";
>> +- reg	     : physical base address and size of the device's memory mapped
>> +	       registers;
>> +- interrupts : should contain FIMC-LITE interrupt;
>> +- cell-index : FIMC-LITE IP instance index;
>
> Same as in an earlier patch - not sure this is needed.

It is needed for setting up a pipeline of multiple sub-device
within a SoC. As I commented on patch 2/13 I'd like to replace
this with proper entries in the "aliases" node. Some sub-devices
have registers that these indexes need to be directly written to.

--

Thanks,
Sylwester

Re: [RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Sylwester Nawrocki]

On 07/16/2012 11:15 AM, Guennadi Liakhovetski wrote:
>> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> @@ -39,6 +39,21 @@ Required properties:
>>   	       depends on the SoC revision. For S5PV210 valid values are:
>>   	       0...2, for Exynos4x1x: 0...3.
>>
>> +
>> +'fimc-lite' device node
>> +-----------------------
>> +
>> +Required properties:
>> +
>> +- compatible : should be one of:
>> +		"samsung,exynos4212-fimc";
>> +		"samsung,exynos4412-fimc";
>> +- reg	     : physical base address and size of the device's memory mapped
>> +	       registers;
>> +- interrupts : should contain FIMC-LITE interrupt;
>> +- cell-index : FIMC-LITE IP instance index;
>
> Same as in an earlier patch - not sure this is needed.

It is needed for setting up a pipeline of multiple sub-device
within a SoC. As I commented on patch 2/13 I'd like to replace
this with proper entries in the "aliases" node. Some sub-devices
have registers that these indexes need to be directly written to.

--

Thanks,
Sylwester

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Sylwester Nawrocki]

On 07/16/2012 11:13 AM, Guennadi Liakhovetski wrote:
> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> 
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Karol Lewandowski<k.lewandowsk@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> 
>  From the documentation below I think, I understand what it does, but why
> is it needed? It doesn't describe your video subsystem topology, right?
> How various subdevices are connected. It just lists them all in one
> node... A description for this patch would be very welcome IMHO and,
> maybe, such a node can be completely avoided?

Sorry, I'll provide better description in next iteration.
It's true it doesn't describe the topology in detail, as there are
multiple one-to many possible connections between sub-devices. An exact
topology is coded in the driver and can be changed through MC API.
The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor" 
nodes just specify to which physical SoC camera port an image sensor
is connected.

Originally the all camera devices were supposed to land under common 
'camera' node. And a top level driver would be registering all platform 
devices. With this approach it would be possible to better control PM 
handling (which currently depends on an order of registering devices to 
the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA 
in such case, which was required to preserve platform device names, in order 
for the clock API to work. So I've moved some sub-devices out of 'camera' 
node and have added only a list of phandles to them in that node. This is 
rather a cheap workaround..

I think all camera sub-devices should be placed under common node, as there
are some properties that don't belong to any sub-node: GPIO config, clocks,
to name a few. Of course simpler devices might not need such a composite 
node. I think we can treat the sub-device interdependencies as an issue
separate from a need for a common node.

If some devices need to reflect the topology better, we probably could
include in some nodes (a list of) phandles to other nodes. This could ease
parsing the topology at the drivers, by using existing OF infrastructure.

--

Thanks,
Sylwester

Re: [RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Guennadi Liakhovetski]

Hi Sylwester

On Tue, 17 Jul 2012, Sylwester Nawrocki wrote:

> On 07/16/2012 11:15 AM, Guennadi Liakhovetski wrote:
> > > --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> > > +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
> > > @@ -39,6 +39,21 @@ Required properties:
> > >   	       depends on the SoC revision. For S5PV210 valid values are:
> > >   	       0...2, for Exynos4x1x: 0...3.
> > > 
> > > +
> > > +'fimc-lite' device node
> > > +-----------------------
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : should be one of:
> > > +		"samsung,exynos4212-fimc";
> > > +		"samsung,exynos4412-fimc";
> > > +- reg	     : physical base address and size of the device's memory
> > > mapped
> > > +	       registers;
> > > +- interrupts : should contain FIMC-LITE interrupt;
> > > +- cell-index : FIMC-LITE IP instance index;
> > 
> > Same as in an earlier patch - not sure this is needed.
> 
> It is needed for setting up a pipeline of multiple sub-device
> within a SoC. As I commented on patch 2/13 I'd like to replace
> this with proper entries in the "aliases" node. Some sub-devices
> have registers that these indexes need to be directly written to.

Aha, so, these are not purely software indices, that you just use to 
identify your devices, these are actual hardware values? You really have 
to write, e.g., "1" into a resizer register to connect it to a certain 
capture engine? Ok, in this case it indeed might make sense to have these 
values in the DT. Not sure how best to call these properties, or whether 
aliases is the best solution, but that's already a detail. And btw, if you 
do decide to keep this as a property, maybe this is one of the cases, 
where you'd want to mark it with your hardware type, like "fimc,cell-id" 
or similar?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Guennadi Liakhovetski]

On Tue, 17 Jul 2012, Sylwester Nawrocki wrote:

> On 07/16/2012 11:13 AM, Guennadi Liakhovetski wrote:
> > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Karol Lewandowski<k.lewandowsk@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > 
> >  From the documentation below I think, I understand what it does, but why
> > is it needed? It doesn't describe your video subsystem topology, right?
> > How various subdevices are connected. It just lists them all in one
> > node... A description for this patch would be very welcome IMHO and,
> > maybe, such a node can be completely avoided?
> 
> Sorry, I'll provide better description in next iteration.
> It's true it doesn't describe the topology in detail, as there are
> multiple one-to many possible connections between sub-devices. An exact
> topology is coded in the driver and can be changed through MC API.
> The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor" 
> nodes just specify to which physical SoC camera port an image sensor
> is connected.

So, don't you think my media-link child nodes are a good solution for 
this?

> Originally the all camera devices were supposed to land under common 
> 'camera' node. And a top level driver would be registering all platform 
> devices. With this approach it would be possible to better control PM 
> handling (which currently depends on an order of registering devices to 
> the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA 
> in such case, which was required to preserve platform device names, in order 
> for the clock API to work. So I've moved some sub-devices out of 'camera' 
> node and have added only a list of phandles to them in that node. This is 
> rather a cheap workaround..
> 
> I think all camera sub-devices should be placed under common node, as there
> are some properties that don't belong to any sub-node: GPIO config, clocks,
> to name a few. Of course simpler devices might not need such a composite 
> node. I think we can treat the sub-device interdependencies as an issue
> separate from a need for a common node.
> 
> If some devices need to reflect the topology better, we probably could
> include in some nodes (a list of) phandles to other nodes. This could ease
> parsing the topology at the drivers, by using existing OF infrastructure.

Ok, I think you have some good ideas in your RFC's, an interesting 
question now is - how to proceed. Do you think we'd be able to work out a 
combined RFC? Or would you prefer to make two versions and then see what 
others think? In either case it would be nice, I think, if you could try 
to separate what you see as common V4L DT bindings, then we could discuss 
that separately. Whereas what you think is private to your hardware, we 
can also look at for common ideas, or maybe even some of those properties 
we'll wake to make common too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sylwester Nawrocki]

Hi Guennadi,

On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> 
>> The driver initializes all board related properties except the s_power()
>> callback to board code. The platforms that require this callback are not
>> supported by this driver yet for CONFIG_OF=y.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
>>   drivers/media/video/s5k6aa.c                       |  129 ++++++++++++++------
>>   2 files changed, 146 insertions(+), 40 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>> new file mode 100644
>> index 0000000..6685a9c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>> @@ -0,0 +1,57 @@
>> +Samsung S5K6AAFX camera sensor
>> +------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible : "samsung,s5k6aafx";
>> +- reg : base address of the device on I2C bus;
> 
> You said you ended up putting your sensors outside of I2C busses, is this
> one of changes, that are present in your git-tree but not in this series?

No, I must have been not clear enough on that. Our idea was to keep
I2C slave device nodes as an I2C controller's child nodes, according
to the current convention.
The 'sensor' nodes (the 'camera''s children) would only contain a phandle
to a respective I2C slave node.

This implies that we cannot access I2C bus in I2C client's device probe() 
callback. An actual H/W access could begin only from within and after 
invocation of v4l2_subdev .registered callback..

>> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> 
> If this is a boolean property, it cannot be required? Otherwise, as
> discussed in a different patch comment, this property might not be
> required, and if it is, I would use the same definition for both the
> camera interface and for sensors.

Yeah, that's an omission, a leftover from previous versions. It should have 
been 'video-bus-type', for this series.

Here is the updated binding documentation:

8<----------------------------------------------------------------------
Required properties:

- compatible : "samsung,s5k6aafx";
- reg : base address of the device on I2C bus;
- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
- vdda-supply : analog circuits voltage supply 2.8V (2.6V to 3.0V);
- vdd_reg-supply : regulator input voltage supply 1.8V (1.7V to 1.9V)
                   or 2.8V (2.6V to 3.0);
- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
                 or 2.8V (2.5V to 3.1V);
- video-bus-type : sensor video output bus type, must be one of:
                   "itu-601", "mipi-csi2";
- clock-frequency : the sensor's master clock frequency in Hz;

Optional properties:

- data-lanes : number of physical MIPI-CSI2 data lanes used
               (default is 2 when this property is not specified);
- gpios : specifies gpios connected to the sensor's reset
          reset (RSTN) and standby (STBYN) pins, the order
          of gpios is: <RSTN, STBYN>; If any of these gpios
          is not used, its specifier shall be 0;
- samsung,s5k6aa-gpio-lvls : this property (bits [1:0]) specifies an active
                             state of the S5K6AAFX reset and stand-by signals;
                             the meaning of bits is: bit[1:0] = [RST, STBY],
                             e.g. bit[1] = 0 indicates the GPIO driving the
                             S5K6AAFX's standby pin should be set to 1 to bring
                             the sensor out from stand-by to normal operation
                             state.
- samsung,s5k6aa-hflip : sets the default horizontal image flipping;
- samsung,s5k6aa-vflip : sets the default vertical image flipping;
8<----------------------------------------------------------------------

>> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
>> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
>> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
>> +		   or 2.8V (2.6V to 3.0);
> 
> I think, underscores in property names are generally frowned upon.

Indeed, I felt something might be wrong here;) It's in this form because
I just copied regulator supply names from the existing driver. And I
didn't want to change them, to avoid updating them in all board files where
they're used. Thanks for pointing it out, I'll fix that in next iteration.

>> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
>> +		 or 2.8V (2.5V to 3.1V);
>> +
>> +Optional properties:
>> +
>> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default
>> +		    value when this property is not specified is 24 MHz;
>> +- data-lanes : number of physical lanes used (default 2 if not specified);
> 
> bus-width?

OK.

>> +- gpio-stby : specifies the S5K6AA_STBY GPIO
>> +- gpio-rst : specifies the S5K6AA_RST GPIO
> 
>  From Documentation/devicetree/bindings/gpio/gpio.txt:
> 
> <quote>
> GPIO properties should be named "[<name>-]gpios".
> </quote>

Thanks, these have been already removed in the next version, as can be seen
above.

>> +- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
>> +- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
> 
> Isn't this provided by GPIO flags as described in include/linux/of_gpio.h
> by using the OF_GPIO_ACTIVE_LOW bit?

Hmm, I wasn't aware of that. I'll see how this flag can be used.

>> +- samsung,s5k6aa-hflip : set the default horizontal image flipping;
>> +- samsung,s5k6aa-vflip : set the default vertical image flipping;
> 
> This is a board property, specifying how the sensor is wired and mounted
> on the casing, right? IIRC, libv4l has a database of these for USB

Yeah, that's exactly it. It could be used for compensating for an
"upside down" mounting.

> cameras. So, maybe it belongs to the user-space for embedded systems too?

Using library for these things is one of the solutions, which works well
for USB webcams. However, I wouldn't like enforcing that. Those properties 
still describe hardware IHMO, then why not add them into DT blob that is 
usually distributed with each board ? There wouldn't be a need to rely on 
any user-space database lookup, before the hardware can actually be used. 

> Or at least this shouldn't be Samsung-specific?

Yes, good point. If we agree we can keep them, perhaps we could standardize
those as boolean properties: horizontal-flip; and vertical-flip; ?

--

Thanks,
Sylwester

Re: [RFC/PATCH 04/13] devicetree: Add common video devices bindings documentation

[Sylwester Nawrocki]

Hi Guennadi,

On 07/16/2012 11:09 AM, Guennadi Liakhovetski wrote:
> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> 
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/video/video.txt |   10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/video/video.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/video.txt b/Documentation/devicetree/bindings/video/video.txt
>> new file mode 100644
>> index 0000000..9f6a637
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/video.txt
>> @@ -0,0 +1,10 @@
>> +Common properties of video data source devices (image sensor, video encoders, etc.)
>> +
>> +Video bus types
>> +---------------
>> +
>> +- video-bus-type : must be one of:
>> +
>> +    - itu-601   : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
>> +    - itu-656   : parallel bus with embedded synchronization - ITU-R BT.656;
> 
> wikipedia tells me, that bt.601 is mostly a data encoding standard, it
> also defines bus-types, but recent versions also include serial busses.

Hmm, indeed, I got slightly misled by the Samsung SoCs' User Manuals 
that used bt.601/bt.656 to distinguish between bus with sync signals
and with embedded sync.

>> +    - mipi-csi2 : MIPI-CSI2 serial bus;
> 
> In general: are these at all needed? Wouldn't it be better to specify pads
> on sensors and interfaces to differentiate between serial and parallel
> connections. As for whether HSYNC and VSYNC are used - I see 3

We have video buses and data receivers and transmitters attached to them.
The pads concept doesn't quite fit here for me. Specifying possible links 
with character string tags might be a way to go, but I'd like to hear
others' opinion on that. Do we have any bindings already that do something
similar ?

> possibilities there:
> 
> 1. real sync signals are used - the default.
> 
> 2. embedded syncs shall be used, because sync signals are missing. This
> should (arguably) be derived from pinctrl - see this discussion:
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/1205.2/index.html#03893

Hmm, I'm not terribly familiar with pinctrl API, but wouldn't it be 
required to have two pin group names: (data + clock + sync) signals and
(data + clock) (for embedded video synchronization) ? We would have to name
these two configurations somehow anyway, wouldn't we ?

Also, as Stephen pointed out, the control flow is supposed to be from
drivers to pin controller, not the other way around.

> 3. sync signals are present, but cannot be used, because one of the
> partners doesn't support them - .g_mbus_config() can be used to retrieve
> this information.

OK.

> 4. sync signals are available and supported by both peers, but for some
> reason the user prefers to use embedded sync data - is such a case
> feasible? Even if so, this should be run-time configurable then?

I've never seen such a situation. I would expect that if sync signal wires 
are routed, they shall be used. Otherwise only embedded synchronization
would be used, to save PCB area.
 
> So, maybe we don't need these at all.

We need something that would let drivers distinguish which (serial/
parallel) bus is supported on a board. And what type of synchronization
is used. I'm fine as long as this can be specified reliably in DT and
drivers of the transmitters/receivers can configure their output/input
interfaces. I'm not against dynamic configuration but static one also
need to be supported.

--
Regards,
Sylwester

Re: [RFC/PATCH 12/13] media: s5p-fimc: Add device tree based sensors registration

[Sylwester Nawrocki]

On 07/16/2012 11:51 AM, Guennadi Liakhovetski wrote:
[...]
>> diff --git a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> index b459da2..ffe09ac 100644
>> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>> @@ -54,6 +54,28 @@ Required properties:
>>   - cell-index : FIMC-LITE IP instance index;
>>
>>
>> +The 'sensor' nodes
>> +------------------
>> +
>> +Required properties:
>> +
>> + - i2c-client : a phandle to an image sensor I2C client device;
>> +
>> +Optional properties:
>> +
>> +- samsung,camif-mux-id : FIMC video multiplexer input index; for camera
>> +			 port A, B, C the indexes are 0, 1, 0 respectively.
>> +			 If this property is not specified a default 0
>> +			 value will be used by driver.
>
> Isn't it possible to have several clients connected to different mux
> inputs and switch between them at run-time? Even if only one of them can
> be active at any time? That's why I've introduced link nodes in my RFC to
> specify exactly which pads are connected.


Yes, of course it is. For such a configuration there would be multiple
'sensor' nodes and each of them would contain a 'samsung,camif-mux-id'
determining camera port the corresponding sensor/encoder is wired to.

Then switching between the sensors would be possible by setting up
a proper media link (or just through VIDIOC_S_INPUT ioctl, if that's
feasible) and then writing the mux input index and the camera port
type values to the control registers (in this specific case ports A,B 
(index 0, 1) are parallel and port C (index 0) is a serial port).
Thus using only input index wouldn't be sufficient to do the sensors
switch over.

--

Thanks,
Sylwester

Re: [RFC/PATCH 06/13] media: s5p-fimc: Add device tree support for FIMC-LITE

[Sylwester Nawrocki]

Hi Guennadi,

On 07/18/2012 09:57 AM, Guennadi Liakhovetski wrote:
> On Tue, 17 Jul 2012, Sylwester Nawrocki wrote:
>> On 07/16/2012 11:15 AM, Guennadi Liakhovetski wrote:
>>>> --- a/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>>>> +++ b/Documentation/devicetree/bindings/camera/soc/samsung-fimc.txt
>>>> @@ -39,6 +39,21 @@ Required properties:
>>>>    	       depends on the SoC revision. For S5PV210 valid values are:
>>>>    	       0...2, for Exynos4x1x: 0...3.
>>>>
>>>> +
>>>> +'fimc-lite' device node
>>>> +-----------------------
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible : should be one of:
>>>> +		"samsung,exynos4212-fimc";
>>>> +		"samsung,exynos4412-fimc";
>>>> +- reg	     : physical base address and size of the device's memory
>>>> mapped
>>>> +	       registers;
>>>> +- interrupts : should contain FIMC-LITE interrupt;
>>>> +- cell-index : FIMC-LITE IP instance index;
>>>
>>> Same as in an earlier patch - not sure this is needed.
>>
>> It is needed for setting up a pipeline of multiple sub-device
>> within a SoC. As I commented on patch 2/13 I'd like to replace
>> this with proper entries in the "aliases" node. Some sub-devices
>> have registers that these indexes need to be directly written to.
> 
> Aha, so, these are not purely software indices, that you just use to
> identify your devices, these are actual hardware values? You really have
> to write, e.g., "1" into a resizer register to connect it to a certain
> capture engine? Ok, in this case it indeed might make sense to have these

Yes, that's more or less how it works. If I select input mux id = 0, 
then MIPI-CSIS0 have to be used for streaming. For this FIMC-LITE device
there are some commands/registers within the device that sits after
FIMC-LITE in the data pipeline (not yet mainlined).

> values in the DT. Not sure how best to call these properties, or whether
> aliases is the best solution, but that's already a detail. And btw, if you

Yeah, the property seems more accurate. But I don't really mind, I could 
just use whatever is accepted in the mainline.

> do decide to keep this as a property, maybe this is one of the cases,
> where you'd want to mark it with your hardware type, like "fimc,cell-id"
> or similar?

I'm not sure about that. "cell-index" has well defined semantics. So
there should be no reason to use device specific property. I would have
then to use "samsung,fimc-cell-id", "samsung,fimc-lite-cell-id" and
"samsung,csis-cell-id", as all device specific bindings need to be 
prefixed with a manufacturer code.

--

Thanks,
Sylwester

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Sylwester Nawrocki]

On 07/18/2012 10:17 AM, Guennadi Liakhovetski wrote:
> On Tue, 17 Jul 2012, Sylwester Nawrocki wrote:
>> On 07/16/2012 11:13 AM, Guennadi Liakhovetski wrote:
>>> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
>>>
>>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>>>> Signed-off-by: Karol Lewandowski<k.lewandowsk@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>
>>>    From the documentation below I think, I understand what it does, but why
>>> is it needed? It doesn't describe your video subsystem topology, right?
>>> How various subdevices are connected. It just lists them all in one
>>> node... A description for this patch would be very welcome IMHO and,
>>> maybe, such a node can be completely avoided?
>>
>> Sorry, I'll provide better description in next iteration.
>> It's true it doesn't describe the topology in detail, as there are
>> multiple one-to many possible connections between sub-devices. An exact
>> topology is coded in the driver and can be changed through MC API.
>> The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor"
>> nodes just specify to which physical SoC camera port an image sensor
>> is connected.
> 
> So, don't you think my media-link child nodes are a good solution for
> this?

Not quite yet ;) It would be good to see some example implementation
and how it actually works.

>> Originally the all camera devices were supposed to land under common
>> 'camera' node. And a top level driver would be registering all platform
>> devices. With this approach it would be possible to better control PM
>> handling (which currently depends on an order of registering devices to
>> the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA
>> in such case, which was required to preserve platform device names, in order
>> for the clock API to work. So I've moved some sub-devices out of 'camera'
>> node and have added only a list of phandles to them in that node. This is
>> rather a cheap workaround..
>>
>> I think all camera sub-devices should be placed under common node, as there
>> are some properties that don't belong to any sub-node: GPIO config, clocks,
>> to name a few. Of course simpler devices might not need such a composite
>> node. I think we can treat the sub-device interdependencies as an issue
>> separate from a need for a common node.
>>
>> If some devices need to reflect the topology better, we probably could
>> include in some nodes (a list of) phandles to other nodes. This could ease
>> parsing the topology at the drivers, by using existing OF infrastructure.
> 
> Ok, I think you have some good ideas in your RFC's, an interesting
> question now is - how to proceed. Do you think we'd be able to work out a
> combined RFC? Or would you prefer to make two versions and then see what
> others think? In either case it would be nice, I think, if you could try
> to separate what you see as common V4L DT bindings, then we could discuss
> that separately. Whereas what you think is private to your hardware, we
> can also look at for common ideas, or maybe even some of those properties
> we'll wake to make common too.

I think we need a one combined RFC and continue discussions in one thread.
Still, our proposals are quite different, but I believe we need something
in between. I presume we should focus more to have common bindings for 
subdevs that are reused among different host/ISP devices, i.e. sensors and 
encoders. For simple host interfaces we can likely come up with common
binding patterns, but more complex processing pipelines may require 
a sort of individual approach.

The suspend/resume handling is still something I don't have an idea
on how the solution for might look like..
Instantiating all devices from a top level driver could help, but it
is only going to work when platforms are converted to the common clock
framework and have their clocks instantiated from device tree.

This week I'm out of office, and next one or two I have some pending
assignments. So there might be some delay before I can dedicate some 
reasonable amount of time to carry on with that topic.

I unfortunately won't be attending KS this time.

I'll try to prepare some summary with topics that appear common. And also 
to restructure my RFC series to better separate new common features and 
specific H/W support.

In the meantime we could possibly continue discussions in your RFC thread.

--

Regards,
Sylwester

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 17 July 2012 20:16:23 Sylwester Nawrocki wrote:
> On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote:
> > Hi Sylwester
> > 
> > Thanks for your comments to my RFC and for pointing out to this your
> > earlier patch series. Unfortunately, I missed in in May, let me try to
> > provide some thoughts about this, we should really try to converge our
> > proposals. Maybe a discussion at KS would help too.
> 
> Thank you for the review. I was happy to see your RFC, as previously
> there seemed to be not much interest in DT among the media guys.
> Certainly, we need to work on a common approach to ensure interoperability
> of existing drivers and to avoid having people inventing different
> bindings for common features. I would also expect some share of device
> specific bindings, as diversity of media devices is significant.
> 
> I'd be great to discuss these things at KS, especially support for
> proper suspend/resume sequences. Also having common sessions with
> other subsystems folks, like ASoC, for example, might be a good idea.
> 
> I'm not sure if I'll be travelling to the KS though. :)
> 
> > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> >> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
> >> (camera host interface DMA engine and image processor). This patch
> >> adds support for instantiating the MIPI-CSIS devices from DT and
> >> parsing all SoC and board specific properties from device tree.
> >> The MIPI DPHY control callback is now called directly from within
> >> the driver, the platform code must ensure this callback does the
> >> right thing for each SoC.
> >> 
> >> The cell-index property is used to ensure proper signal routing,
> >> from physical camera port, through MIPI-CSI2 receiver to the DMA
> >> engine (FIMC?). It's also helpful in exposing the device topology
> >> in user space at /dev/media? devnode (Media Controller API).
> >> 
> >> This patch also defines a common property ("data-lanes") for MIPI-CSI
> >> receivers and transmitters.
> >> 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   Documentation/devicetree/bindings/video/mipi.txt   |    5 +
> >>   .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
> >>   drivers/media/video/s5p-fimc/mipi-csis.c           |   97
> >>   +++++++++++++++----- drivers/media/video/s5p-fimc/mipi-csis.h         
> >>    |    1 +
> >>   4 files changed, 126 insertions(+), 24 deletions(-)
> >>   create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
> >>   create mode 100644
> >>   Documentation/devicetree/bindings/video/samsung-mipi-csis.txt>> 
> >> diff --git a/Documentation/devicetree/bindings/video/mipi.txt
> >> b/Documentation/devicetree/bindings/video/mipi.txt new file mode 100644
> >> index 0000000..5aed285
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> >> @@ -0,0 +1,5 @@
> >> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
> >> +
> >> + - data-lanes : number of differential data lanes wired and actively
> >> used in
> >> +		communication between the transmitter and the receiver, this
> >> +		excludes the clock lane;
> > 
> > Wouldn't it be better to use the standard "bus-width" DT property?
> 
> I can't see any problems with using "bus-width". It seems sufficient
> and could indeed be better, without a need to invent new MIPI-CSI
> specific names. That was my first RFC on that and my perspective
> wasn't probably broad enough. :)

What about CSI receivers that can reroute the lanes internally ? We would need 
to specify lane indices for each lane then, maybe with something like

clock-lane = <0>;
data-lanes = <2 3 1>;

For receivers that can't reroute lanes internally, the data-lanes property 
would be need to specify lanes in sequence.

data-lanes = <1 2 3>;

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 01/13] ARM: Samsung: Extend MIPI PHY callback with an index argument

[Laurent Pinchart]

Hi Sylwester,

On Friday 25 May 2012 21:52:40 Sylwester Nawrocki wrote:
> For systems instantiated from device tree struct platform_device id
> field is always -1, add an 'id' argument to the s5p_csis_phy_enable()
> function so the MIPI-CSIS hardware instance index can be passed in
> by driver, for CONFIG_OF=y.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/plat-s5p/setup-mipiphy.c              |   20 ++++++++------------
>  arch/arm/plat-samsung/include/plat/mipi_csis.h |   10 ++++++----
>  2 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/plat-s5p/setup-mipiphy.c
> b/arch/arm/plat-s5p/setup-mipiphy.c index 683c466..146ecc3 100644
> --- a/arch/arm/plat-s5p/setup-mipiphy.c
> +++ b/arch/arm/plat-s5p/setup-mipiphy.c
> @@ -14,24 +14,19 @@
>  #include <linux/spinlock.h>
>  #include <mach/regs-clock.h>
> 
> -static int __s5p_mipi_phy_control(struct platform_device *pdev,
> +static int __s5p_mipi_phy_control(struct platform_device *pdev, int id,
>  				  bool on, u32 reset)

What about removing the pdev argument, as it's now not needed ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Laurent Pinchart]

Hi Sylwester,

On Wednesday 18 July 2012 21:53:34 Sylwester Nawrocki wrote:
> On 07/18/2012 10:17 AM, Guennadi Liakhovetski wrote:
> > On Tue, 17 Jul 2012, Sylwester Nawrocki wrote:
> >> On 07/16/2012 11:13 AM, Guennadi Liakhovetski wrote:
> >>> On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> >>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >>>> Signed-off-by: Karol Lewandowski<k.lewandowsk@samsung.com>
> >>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >>>> 
> >>>    From the documentation below I think, I understand what it does, but
> >>>    why
> >>> 
> >>> is it needed? It doesn't describe your video subsystem topology, right?
> >>> How various subdevices are connected. It just lists them all in one
> >>> node... A description for this patch would be very welcome IMHO and,
> >>> maybe, such a node can be completely avoided?
> >> 
> >> Sorry, I'll provide better description in next iteration.
> >> It's true it doesn't describe the topology in detail, as there are
> >> multiple one-to many possible connections between sub-devices. An exact
> >> topology is coded in the driver and can be changed through MC API.
> >> The "samsung,camif-mux-id" and "video-bus-type" properties at "sensor"
> >> nodes just specify to which physical SoC camera port an image sensor
> >> is connected.
> > 
> > So, don't you think my media-link child nodes are a good solution for
> > this?
> 
> Not quite yet ;) It would be good to see some example implementation
> and how it actually works.
> 
> >> Originally the all camera devices were supposed to land under common
> >> 'camera' node. And a top level driver would be registering all platform
> >> devices. With this approach it would be possible to better control PM
> >> handling (which currently depends on an order of registering devices to
> >> the driver core). But then we discovered that we couldn't use
> >> OF_DEV_AUXDATA in such case, which was required to preserve platform
> >> device names, in order for the clock API to work. So I've moved some
> >> sub-devices out of 'camera' node and have added only a list of phandles
> >> to them in that node. This is rather a cheap workaround..
> >> 
> >> I think all camera sub-devices should be placed under common node, as
> >> there
> >> are some properties that don't belong to any sub-node: GPIO config,
> >> clocks,
> >> to name a few. Of course simpler devices might not need such a composite
> >> node. I think we can treat the sub-device interdependencies as an issue
> >> separate from a need for a common node.
> >> 
> >> If some devices need to reflect the topology better, we probably could
> >> include in some nodes (a list of) phandles to other nodes. This could
> >> ease
> >> parsing the topology at the drivers, by using existing OF infrastructure.
> > 
> > Ok, I think you have some good ideas in your RFC's, an interesting
> > question now is - how to proceed. Do you think we'd be able to work out a
> > combined RFC? Or would you prefer to make two versions and then see what
> > others think? In either case it would be nice, I think, if you could try
> > to separate what you see as common V4L DT bindings, then we could discuss
> > that separately. Whereas what you think is private to your hardware, we
> > can also look at for common ideas, or maybe even some of those properties
> > we'll wake to make common too.
> 
> I think we need a one combined RFC and continue discussions in one thread.

Agreed.

> Still, our proposals are quite different, but I believe we need something
> in between. I presume we should focus more to have common bindings for
> subdevs that are reused among different host/ISP devices, i.e. sensors and
> encoders. For simple host interfaces we can likely come up with common
> binding patterns, but more complex processing pipelines may require
> a sort of individual approach.
> 
> The suspend/resume handling is still something I don't have an idea
> on how the solution for might look like..
> Instantiating all devices from a top level driver could help, but it
> is only going to work when platforms are converted to the common clock
> framework and have their clocks instantiated from device tree.
> 
> This week I'm out of office, and next one or two I have some pending
> assignments. So there might be some delay before I can dedicate some
> reasonable amount of time to carry on with that topic.
> 
> I unfortunately won't be attending KS this time.

That's bad news :-( I still think this topic should be discussed during KS, I 
expect several developers to be interested. The media workshop might not be 
the best venue though, as we might need quite a lot of time.

Until KS let's continue the discussion by e-mail.

> I'll try to prepare some summary with topics that appear common. And also
> to restructure my RFC series to better separate new common features and
> specific H/W support.
> 
> In the meantime we could possibly continue discussions in your RFC thread.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Sylwester,

On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> >> The driver initializes all board related properties except the s_power()
> >> callback to board code. The platforms that require this callback are not
> >> supported by this driver yet for CONFIG_OF=y.
> >> 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> >>   drivers/media/video/s5k6aa.c                       |  129
> >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> >>   deletions(-)
> >>   create mode 100644
> >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
> >> mode 100644
> >> index 0000000..6685a9c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> >> @@ -0,0 +1,57 @@
> >> +Samsung S5K6AAFX camera sensor
> >> +------------------------------
> >> +
> >> +Required properties:
> >> +
> >> +- compatible : "samsung,s5k6aafx";
> >> +- reg : base address of the device on I2C bus;
> > 
> > You said you ended up putting your sensors outside of I2C busses, is this
> > one of changes, that are present in your git-tree but not in this series?
> 
> No, I must have been not clear enough on that. Our idea was to keep
> I2C slave device nodes as an I2C controller's child nodes, according
> to the current convention.
> The 'sensor' nodes (the 'camera''s children) would only contain a phandle
> to a respective I2C slave node.
> 
> This implies that we cannot access I2C bus in I2C client's device probe()
> callback. An actual H/W access could begin only from within and after
> invocation of v4l2_subdev .registered callback..

That's how I've envisioned the DT bindings for sensors as well, this sounds 
good. The real challenge will be to get hold of the subdev to register it 
without race conditions.

> >> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> > 
> > If this is a boolean property, it cannot be required? Otherwise, as
> > discussed in a different patch comment, this property might not be
> > required, and if it is, I would use the same definition for both the
> > camera interface and for sensors.
> 
> Yeah, that's an omission, a leftover from previous versions. It should have
> been 'video-bus-type', for this series.
> 
> Here is the updated binding documentation:
> 
> 8<----------------------------------------------------------------------
> Required properties:
> 
> - compatible : "samsung,s5k6aafx";
> - reg : base address of the device on I2C bus;
> - vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> - vdda-supply : analog circuits voltage supply 2.8V (2.6V to 3.0V);
> - vdd_reg-supply : regulator input voltage supply 1.8V (1.7V to 1.9V)
>                    or 2.8V (2.6V to 3.0);
> - vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
>                  or 2.8V (2.5V to 3.1V);
> - video-bus-type : sensor video output bus type, must be one of:
>                    "itu-601", "mipi-csi2";
> - clock-frequency : the sensor's master clock frequency in Hz;
> 
> Optional properties:
> 
> - data-lanes : number of physical MIPI-CSI2 data lanes used
>                (default is 2 when this property is not specified);
> - gpios : specifies gpios connected to the sensor's reset
>           reset (RSTN) and standby (STBYN) pins, the order
>           of gpios is: <RSTN, STBYN>; If any of these gpios
>           is not used, its specifier shall be 0;
> - samsung,s5k6aa-gpio-lvls : this property (bits [1:0]) specifies an active
>                              state of the S5K6AAFX reset and stand-by
> signals; the meaning of bits is: bit[1:0] = [RST, STBY], e.g. bit[1] = 0
> indicates the GPIO driving the S5K6AAFX's standby pin should be set to 1 to
> bring the sensor out from stand-by to normal operation state.
> - samsung,s5k6aa-hflip : sets the default horizontal image flipping;
> - samsung,s5k6aa-vflip : sets the default vertical image flipping;
> 8<----------------------------------------------------------------------
> 
> >> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> >> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
> >> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to
> >> 1.9V) +		   or 2.8V (2.6V to 3.0);
> > 
> > I think, underscores in property names are generally frowned upon.
> 
> Indeed, I felt something might be wrong here;) It's in this form because
> I just copied regulator supply names from the existing driver. And I
> didn't want to change them, to avoid updating them in all board files where
> they're used. Thanks for pointing it out, I'll fix that in next iteration.
> 
> >> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
> >> +		 or 2.8V (2.5V to 3.1V);
> >> +
> >> +Optional properties:
> >> +
> >> +- clock-frequency : the IP's main (system bus) clock frequency in Hz,
> >> the default +		    value when this property is not specified is 24 MHz;
> >> +- data-lanes : number of physical lanes used (default 2 if not
> >> specified);
> > 
> > bus-width?
> 
> OK.
> 
> >> +- gpio-stby : specifies the S5K6AA_STBY GPIO
> >> +- gpio-rst : specifies the S5K6AA_RST GPIO
> >> 
> >  From Documentation/devicetree/bindings/gpio/gpio.txt:
> > <quote>
> > GPIO properties should be named "[<name>-]gpios".
> > </quote>
> 
> Thanks, these have been already removed in the next version, as can be seen
> above.
> 
> >> +- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
> >> +- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
> > 
> > Isn't this provided by GPIO flags as described in include/linux/of_gpio.h
> > by using the OF_GPIO_ACTIVE_LOW bit?
> 
> Hmm, I wasn't aware of that. I'll see how this flag can be used.
> 
> >> +- samsung,s5k6aa-hflip : set the default horizontal image flipping;
> >> +- samsung,s5k6aa-vflip : set the default vertical image flipping;
> > 
> > This is a board property, specifying how the sensor is wired and mounted
> > on the casing, right? IIRC, libv4l has a database of these for USB
> 
> Yeah, that's exactly it. It could be used for compensating for an
> "upside down" mounting.
> 
> > cameras. So, maybe it belongs to the user-space for embedded systems too?
> 
> Using library for these things is one of the solutions, which works well
> for USB webcams. However, I wouldn't like enforcing that. Those properties
> still describe hardware IHMO, then why not add them into DT blob that is
> usually distributed with each board ? There wouldn't be a need to rely on
> any user-space database lookup, before the hardware can actually be used.

I agree. For USB devices on x86 we don't have many other options, but with DT 
we can describe the hardware properly.

> > Or at least this shouldn't be Samsung-specific?
> 
> Yes, good point. If we agree we can keep them, perhaps we could standardize
> those as boolean properties: horizontal-flip; and vertical-flip; ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Sylwester,

On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:
> The driver initializes all board related properties except the s_power()
> callback to board code. The platforms that require this callback are not
> supported by this driver yet for CONFIG_OF=y.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
>  drivers/media/video/s5k6aa.c                       |  129 ++++++++++++-----
>  2 files changed, 146 insertions(+), 40 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> 
> diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
> mode 100644
> index 0000000..6685a9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> @@ -0,0 +1,57 @@
> +Samsung S5K6AAFX camera sensor
> +------------------------------
> +
> +Required properties:
> +
> +- compatible : "samsung,s5k6aafx";
> +- reg : base address of the device on I2C bus;
> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
> +		   or 2.8V (2.6V to 3.0);
> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
> +		 or 2.8V (2.5V to 3.1V);
> +
> +Optional properties:
> +
> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the
> default 

Is that the input clock frequency ? Can't it vary ? Instead of accessing the 
sensor clock frequency from the FIMC driver you should reference a clock in 
the sensor DT node. That obviously requires generic clock support, which might 
not be available for your platform yet (that's one of the reasons the OMAP3 
ISP driver doesn't support DT yet).

> +		    value when this property is not specified is 24 MHz;
> +- data-lanes : number of physical lanes used (default 2 if not specified);
> +- gpio-stby : specifies the S5K6AA_STBY GPIO
> +- gpio-rst : specifies the S5K6AA_RST GPIO
> +- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
> +- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
> +- samsung,s5k6aa-hflip : set the default horizontal image flipping;
> +- samsung,s5k6aa-vflip : set the default vertical image flipping;
> +
> +
> +Example:
> +
> +	gpl2: gpio-controller@0 {
> +	};
> +
> +	reg0: regulator@0 {
> +	};
> +
> +	reg1: regulator@1 {
> +	};
> +
> +	reg2: regulator@2 {
> +	};
> +
> +	reg3: regulator@3 {
> +	};
> +
> +	s5k6aafx@3c {
> +		compatible = "samsung,s5k6aafx";
> +		reg = <0x3c>;
> +		clock-frequency = <24000000>;
> +		gpio-rst = <&gpl2 0 2 0 3>;
> +		gpio-stby = <&gpl2 1 2 0 3>;
> +		video-itu-601-bus;
> +		vdd_core-supply = <&reg0>;
> +		vdda-supply = <&reg1>;
> +		vdd_reg-supply = <&reg2>;
> +		vddio-supply = <&reg3>;
> +	};

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Sylwester Nawrocki]

Hi Laurent,

On 07/26/2012 04:38 PM, Laurent Pinchart wrote:
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
>>>> @@ -0,0 +1,5 @@
>>>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
>>>> +
>>>> + - data-lanes : number of differential data lanes wired and actively
>>>> used in
>>>> +		communication between the transmitter and the receiver, this
>>>> +		excludes the clock lane;
>>>
>>> Wouldn't it be better to use the standard "bus-width" DT property?
>>
>> I can't see any problems with using "bus-width". It seems sufficient
>> and could indeed be better, without a need to invent new MIPI-CSI
>> specific names. That was my first RFC on that and my perspective
>> wasn't probably broad enough. :)
> 
> What about CSI receivers that can reroute the lanes internally ? We would need
> to specify lane indices for each lane then, maybe with something like
> 
> clock-lane =<0>;
> data-lanes =<2 3 1>;

Sounds good to me. And the clock-lane could be made optional, as not all
devices would need it.
 
However, as far as I can see, there is currently no generic API for handling 
this kind of data structure. E.g. number of cells for the "interrupts"
property is specified with an additional "#interrupt-cells" property.

It would have been much easier to handle something like:

data-lanes = <2>, <3>, <1>;

i.e. an array of the lane indexes.

> For receivers that can't reroute lanes internally, the data-lanes property
> would be need to specify lanes in sequence.
> 
> data-lanes =<1 2 3>;

In this case we would be only interested in the number of cells in this
property, but how it could be retrieved ? With an array, it could have been
calculated from property length returned by of_property_find() (divided by
sizof(u32)).

--

Regards,
Sylwester

Re: [RFC/PATCH 01/13] ARM: Samsung: Extend MIPI PHY callback with an index argument

[Sylwester Nawrocki]

Hi Laurent,

On 07/26/2012 04:42 PM, Laurent Pinchart wrote:
> Hi Sylwester,
>
> On Friday 25 May 2012 21:52:40 Sylwester Nawrocki wrote:
>> For systems instantiated from device tree struct platform_device id
>> field is always -1, add an 'id' argument to the s5p_csis_phy_enable()
>> function so the MIPI-CSIS hardware instance index can be passed in
>> by driver, for CONFIG_OF=y.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   arch/arm/plat-s5p/setup-mipiphy.c              |   20 ++++++++------------
>>   arch/arm/plat-samsung/include/plat/mipi_csis.h |   10 ++++++----
>>   2 files changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/plat-s5p/setup-mipiphy.c
>> b/arch/arm/plat-s5p/setup-mipiphy.c index 683c466..146ecc3 100644
>> --- a/arch/arm/plat-s5p/setup-mipiphy.c
>> +++ b/arch/arm/plat-s5p/setup-mipiphy.c
>> @@ -14,24 +14,19 @@
>>   #include<linux/spinlock.h>
>>   #include<mach/regs-clock.h>
>>
>> -static int __s5p_mipi_phy_control(struct platform_device *pdev,
>> +static int __s5p_mipi_phy_control(struct platform_device *pdev, int id,
>>   				  bool on, u32 reset)
>
> What about removing the pdev argument, as it's now not needed ?

Indeed, it isn't useful any more. I'm not sure what I intended to
keep it for, perhaps just some sentimental reasons.. :)
Thanks for pointing out, I'll remove it in the next iteration.

--

Regards,
Sylwester

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sylwester Nawrocki]

Hi Laurent,

On 07/26/2012 05:21 PM, Laurent Pinchart wrote:
> On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:
>> The driver initializes all board related properties except the s_power()
>> callback to board code. The platforms that require this callback are not
>> supported by this driver yet for CONFIG_OF=y.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
>>   drivers/media/video/s5k6aa.c                       |  129 ++++++++++++-----
>>   2 files changed, 146 insertions(+), 40 deletions(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
>> mode 100644
>> index 0000000..6685a9c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
>> @@ -0,0 +1,57 @@
>> +Samsung S5K6AAFX camera sensor
>> +------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible : "samsung,s5k6aafx";
>> +- reg : base address of the device on I2C bus;
>> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
>> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
>> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
>> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
>> +		   or 2.8V (2.6V to 3.0);
>> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
>> +		 or 2.8V (2.5V to 3.1V);
>> +
>> +Optional properties:
>> +
>> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the
>> default
> 
> Is that the input clock frequency ? Can't it vary ? Instead of accessing the

Yes, the description is incorrect in this patch, it should read:

+- clock-frequency : the sensor's master clock frequency in Hz;

and be a required property. As in this patch:
https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7abe563

It could vary (as this is a PLL input frequency), so probably a range would
be a better alternative. Given that host device won't always be able to set 
this exact value...

> sensor clock frequency from the FIMC driver you should reference a clock in
> the sensor DT node. That obviously requires generic clock support, which might
> not be available for your platform yet (that's one of the reasons the OMAP3
> ISP driver doesn't support DT yet).

I agree it might be better, but waiting unknown number of kernel releases
for the platforms to get converted to common clock API is not a good 
alternative either. I guess we could have some transitional solutions
while other subsystems are getting adapted. 

Yet we need to specify the clock frequency range per sensor, so

1. either we specify it at a sensor node and host device driver references
   it, or
2. it could be added to a sensor specific child node of a host device
   mode, and then only the host would reference it, and sensor would
   reference a clock in its DT node; I guess it's not a problem that
   in most cases the camera host device is a clock provider.

--

Regards,
Sylwester

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Laurent Pinchart]

Hi Sylwester,

On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:
> On 07/26/2012 04:38 PM, Laurent Pinchart wrote:
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> >>>> @@ -0,0 +1,5 @@
> >>>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and
> >>>> transmitters
> >>>> +
> >>>> + - data-lanes : number of differential data lanes wired and actively
> >>>> used in
> >>>> +		communication between the transmitter and the receiver, this
> >>>> +		excludes the clock lane;
> >>> 
> >>> Wouldn't it be better to use the standard "bus-width" DT property?
> >> 
> >> I can't see any problems with using "bus-width". It seems sufficient
> >> and could indeed be better, without a need to invent new MIPI-CSI
> >> specific names. That was my first RFC on that and my perspective
> >> wasn't probably broad enough. :)
> > 
> > What about CSI receivers that can reroute the lanes internally ? We would
> > need to specify lane indices for each lane then, maybe with something
> > like
> > 
> > clock-lane =<0>;
> > data-lanes =<2 3 1>;
> 
> Sounds good to me. And the clock-lane could be made optional, as not all
> devices would need it.
> 
> However, as far as I can see, there is currently no generic API for handling
> this kind of data structure. E.g. number of cells for the "interrupts"
> property is specified with an additional "#interrupt-cells" property.
> 
> It would have been much easier to handle something like:
> 
> data-lanes = <2>, <3>, <1>;
> 
> i.e. an array of the lane indexes.

I'm fine with that.

> > For receivers that can't reroute lanes internally, the data-lanes property
> > would be need to specify lanes in sequence.
> > 
> > data-lanes =<1 2 3>;
> 
> In this case we would be only interested in the number of cells in this
> property, but how it could be retrieved ? With an array, it could have been
> calculated from property length returned by of_property_find() (divided by
> sizof(u32)).

Agreed.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Sylwester,

On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote:
> On 07/26/2012 05:21 PM, Laurent Pinchart wrote:
> > On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:
> >> The driver initializes all board related properties except the s_power()
> >> callback to board code. The platforms that require this callback are not
> >> supported by this driver yet for CONFIG_OF=y.
> >> 
> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> >>   drivers/media/video/s5k6aa.c                       |  129 +++++++++----
> >>   2 files changed, 146 insertions(+), 40 deletions(-)
> >>   create mode 100644
> >> 
> >> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
> >> mode 100644
> >> index 0000000..6685a9c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> >> @@ -0,0 +1,57 @@
> >> +Samsung S5K6AAFX camera sensor
> >> +------------------------------
> >> +
> >> +Required properties:
> >> +
> >> +- compatible : "samsung,s5k6aafx";
> >> +- reg : base address of the device on I2C bus;
> >> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> >> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> >> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
> >> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to
> >> 1.9V) +		   or 2.8V (2.6V to 3.0);
> >> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
> >> +		 or 2.8V (2.5V to 3.1V);
> >> +
> >> +Optional properties:
> >> +
> >> +- clock-frequency : the IP's main (system bus) clock frequency in Hz,
> >> the default
> > 
> > Is that the input clock frequency ? Can't it vary ? Instead of accessing
> > the
> Yes, the description is incorrect in this patch, it should read:
> 
> +- clock-frequency : the sensor's master clock frequency in Hz;
> 
> and be a required property. As in this patch:
> https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7
> abe563
> 
> It could vary (as this is a PLL input frequency), so probably a range would
> be a better alternative. Given that host device won't always be able to set
> this exact value...

A range sounds good, or perhaps a list of ranges. Sakari, what would you need 
for the SMIA++ driver ?

> > sensor clock frequency from the FIMC driver you should reference a clock
> > in the sensor DT node. That obviously requires generic clock support,
> > which might not be available for your platform yet (that's one of the
> > reasons the OMAP3 ISP driver doesn't support DT yet).
> 
> I agree it might be better, but waiting unknown number of kernel releases
> for the platforms to get converted to common clock API is not a good
> alternative either. I guess we could have some transitional solutions while
> other subsystems are getting adapted.

I agree, we need an interim solution.

> Yet we need to specify the clock frequency range per sensor, so
> 
> 1. either we specify it at a sensor node and host device driver references
>    it, or
> 2. it could be added to a sensor specific child node of a host device
>    mode, and then only the host would reference it, and sensor would
>    reference a clock in its DT node; I guess it's not a problem that
>    in most cases the camera host device is a clock provider.

The sensor will need to configure the clock rate, so a (list of) clock 
frequency range(s) will be needed in the sensor node anyway. As an interim 
solution the host can access that property. When the platform will be ported 
to the common clock API no modification to the DT will be needed.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices

[Sylwester Nawrocki]

Hi Laurent,

On 07/26/2012 04:54 PM, Laurent Pinchart wrote:
> On Wednesday 18 July 2012 21:53:34 Sylwester Nawrocki wrote:
>> I think we need a one combined RFC and continue discussions in one thread.
> 
> Agreed.
> 
>> Still, our proposals are quite different, but I believe we need something
>> in between. I presume we should focus more to have common bindings for
>> subdevs that are reused among different host/ISP devices, i.e. sensors and
>> encoders. For simple host interfaces we can likely come up with common
>> binding patterns, but more complex processing pipelines may require
>> a sort of individual approach.
>>
>> The suspend/resume handling is still something I don't have an idea
>> on how the solution for might look like..
>> Instantiating all devices from a top level driver could help, but it
>> is only going to work when platforms are converted to the common clock
>> framework and have their clocks instantiated from device tree.
>>
>> This week I'm out of office, and next one or two I have some pending
>> assignments. So there might be some delay before I can dedicate some
>> reasonable amount of time to carry on with that topic.
>>
>> I unfortunately won't be attending KS this time.
> 
> That's bad news :-( I still think this topic should be discussed during KS, I

Yeah, shit happens.. :) I guess -ENOBUDGET this time... I didn't really 
plan early to attend KS, I might be coming to ELCE though. However it's 
a rather distant event and we'll probably get most things settled by 
that time.

> expect several developers to be interested. The media workshop might not be
> the best venue though, as we might need quite a lot of time.
>
> Until KS let's continue the discussion by e-mail.

OK, thank you for taking time to review the RFCs.

--

Regards,
Sylwester

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Guennadi Liakhovetski]

On Thu, 26 Jul 2012, Laurent Pinchart wrote:

> Hi Sylwester,
> 
> On Tuesday 17 July 2012 20:16:23 Sylwester Nawrocki wrote:
> > On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote:
> > > Hi Sylwester
> > > 
> > > Thanks for your comments to my RFC and for pointing out to this your
> > > earlier patch series. Unfortunately, I missed in in May, let me try to
> > > provide some thoughts about this, we should really try to converge our
> > > proposals. Maybe a discussion at KS would help too.
> > 
> > Thank you for the review. I was happy to see your RFC, as previously
> > there seemed to be not much interest in DT among the media guys.
> > Certainly, we need to work on a common approach to ensure interoperability
> > of existing drivers and to avoid having people inventing different
> > bindings for common features. I would also expect some share of device
> > specific bindings, as diversity of media devices is significant.
> > 
> > I'd be great to discuss these things at KS, especially support for
> > proper suspend/resume sequences. Also having common sessions with
> > other subsystems folks, like ASoC, for example, might be a good idea.
> > 
> > I'm not sure if I'll be travelling to the KS though. :)
> > 
> > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > >> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
> > >> (camera host interface DMA engine and image processor). This patch
> > >> adds support for instantiating the MIPI-CSIS devices from DT and
> > >> parsing all SoC and board specific properties from device tree.
> > >> The MIPI DPHY control callback is now called directly from within
> > >> the driver, the platform code must ensure this callback does the
> > >> right thing for each SoC.
> > >> 
> > >> The cell-index property is used to ensure proper signal routing,
> > >> from physical camera port, through MIPI-CSI2 receiver to the DMA
> > >> engine (FIMC?). It's also helpful in exposing the device topology
> > >> in user space at /dev/media? devnode (Media Controller API).
> > >> 
> > >> This patch also defines a common property ("data-lanes") for MIPI-CSI
> > >> receivers and transmitters.
> > >> 
> > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > >> ---
> > >> 
> > >>   Documentation/devicetree/bindings/video/mipi.txt   |    5 +
> > >>   .../bindings/video/samsung-mipi-csis.txt           |   47 ++++++++++
> > >>   drivers/media/video/s5p-fimc/mipi-csis.c           |   97
> > >>   +++++++++++++++----- drivers/media/video/s5p-fimc/mipi-csis.h         
> > >>    |    1 +
> > >>   4 files changed, 126 insertions(+), 24 deletions(-)
> > >>   create mode 100644 Documentation/devicetree/bindings/video/mipi.txt
> > >>   create mode 100644
> > >>   Documentation/devicetree/bindings/video/samsung-mipi-csis.txt>> 
> > >> diff --git a/Documentation/devicetree/bindings/video/mipi.txt
> > >> b/Documentation/devicetree/bindings/video/mipi.txt new file mode 100644
> > >> index 0000000..5aed285
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> > >> @@ -0,0 +1,5 @@
> > >> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters
> > >> +
> > >> + - data-lanes : number of differential data lanes wired and actively
> > >> used in
> > >> +		communication between the transmitter and the receiver, this
> > >> +		excludes the clock lane;
> > > 
> > > Wouldn't it be better to use the standard "bus-width" DT property?
> > 
> > I can't see any problems with using "bus-width". It seems sufficient
> > and could indeed be better, without a need to invent new MIPI-CSI
> > specific names. That was my first RFC on that and my perspective
> > wasn't probably broad enough. :)
> 
> What about CSI receivers that can reroute the lanes internally ? We would need 
> to specify lane indices for each lane then, maybe with something like
> 
> clock-lane = <0>;

"clock-lanes" for uniformity?

> data-lanes = <2 3 1>;

Are there transmitters, that can reroute lanes too?

Thanks
Guennadi

> For receivers that can't reroute lanes internally, the data-lanes property 
> would be need to specify lanes in sequence.
> 
> data-lanes = <1 2 3>;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Thu, 26 Jul 2012, Laurent Pinchart wrote:

> Hi Sylwester,
> 
> On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > >> The driver initializes all board related properties except the s_power()
> > >> callback to board code. The platforms that require this callback are not
> > >> supported by this driver yet for CONFIG_OF=y.
> > >> 
> > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > >> ---
> > >> 
> > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> > >>   drivers/media/video/s5k6aa.c                       |  129
> > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > >>   deletions(-)
> > >>   create mode 100644
> > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>> 
> > >> diff --git
> > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
> > >> mode 100644
> > >> index 0000000..6685a9c
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > >> @@ -0,0 +1,57 @@
> > >> +Samsung S5K6AAFX camera sensor
> > >> +------------------------------
> > >> +
> > >> +Required properties:
> > >> +
> > >> +- compatible : "samsung,s5k6aafx";
> > >> +- reg : base address of the device on I2C bus;
> > > 
> > > You said you ended up putting your sensors outside of I2C busses, is this
> > > one of changes, that are present in your git-tree but not in this series?
> > 
> > No, I must have been not clear enough on that. Our idea was to keep
> > I2C slave device nodes as an I2C controller's child nodes, according
> > to the current convention.
> > The 'sensor' nodes (the 'camera''s children) would only contain a phandle
> > to a respective I2C slave node.
> > 
> > This implies that we cannot access I2C bus in I2C client's device probe()
> > callback. An actual H/W access could begin only from within and after
> > invocation of v4l2_subdev .registered callback..
> 
> That's how I've envisioned the DT bindings for sensors as well, this sounds 
> good. The real challenge will be to get hold of the subdev to register it 
> without race conditions.

Hrm... That's how early pre-subdev versions of soc-camera used to work, 
that's where all the <device>_video_probe() functions come from. But then 
we switched to dynamic i2c device registration. Do we want to switch all 
drivers back now?... Couldn't we "temporarily" use references from subdevs 
to hosts until the clock API is available?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > >> The driver initializes all board related properties except the
> > > >> s_power() callback to board code. The platforms that require this
> > > >> callback are not supported by this driver yet for CONFIG_OF=y.
> > > >> 
> > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > >> ---
> > > >> 
> > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > >>   deletions(-)
> > > >>   create mode 100644
> > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
> > > >> 
> > > >> diff --git
> > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new
> > > >> file
> > > >> mode 100644
> > > >> index 0000000..6685a9c
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > >> @@ -0,0 +1,57 @@
> > > >> +Samsung S5K6AAFX camera sensor
> > > >> +------------------------------
> > > >> +
> > > >> +Required properties:
> > > >> +
> > > >> +- compatible : "samsung,s5k6aafx";
> > > >> +- reg : base address of the device on I2C bus;
> > > > 
> > > > You said you ended up putting your sensors outside of I2C busses, is
> > > > this
> > > > one of changes, that are present in your git-tree but not in this
> > > > series?
> > > 
> > > No, I must have been not clear enough on that. Our idea was to keep
> > > I2C slave device nodes as an I2C controller's child nodes, according
> > > to the current convention.
> > > The 'sensor' nodes (the 'camera''s children) would only contain a
> > > phandle to a respective I2C slave node.
> > > 
> > > This implies that we cannot access I2C bus in I2C client's device
> > > probe() callback. An actual H/W access could begin only from within and
> > > after invocation of v4l2_subdev .registered callback..
> > 
> > That's how I've envisioned the DT bindings for sensors as well, this
> > sounds good. The real challenge will be to get hold of the subdev to
> > register it without race conditions.
> 
> Hrm... That's how early pre-subdev versions of soc-camera used to work,
> that's where all the <device>_video_probe() functions come from. But then
> we switched to dynamic i2c device registration. Do we want to switch all
> drivers back now?... Couldn't we "temporarily" use references from subdevs
> to hosts until the clock API is available?

I don't think that requires a reference from subdevs to hosts in the DT. The 
subdev will need the host to be probed before a clock can be available so you 
won't be able to access the hardware in the probe() function in the generic 
case. You will need to wait until the registered() subdev operation is called, 
at which point the host can be accessed through the v4l2_device.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Guennadi Liakhovetski]

On Fri, 27 Jul 2012, Laurent Pinchart wrote:

> Hi Sylwester,
> 
> On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:
> > On 07/26/2012 04:38 PM, Laurent Pinchart wrote:
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> > >>>> @@ -0,0 +1,5 @@
> > >>>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and
> > >>>> transmitters
> > >>>> +
> > >>>> + - data-lanes : number of differential data lanes wired and actively
> > >>>> used in
> > >>>> +		communication between the transmitter and the receiver, this
> > >>>> +		excludes the clock lane;
> > >>> 
> > >>> Wouldn't it be better to use the standard "bus-width" DT property?
> > >> 
> > >> I can't see any problems with using "bus-width". It seems sufficient
> > >> and could indeed be better, without a need to invent new MIPI-CSI
> > >> specific names. That was my first RFC on that and my perspective
> > >> wasn't probably broad enough. :)
> > > 
> > > What about CSI receivers that can reroute the lanes internally ? We would
> > > need to specify lane indices for each lane then, maybe with something
> > > like
> > > 
> > > clock-lane =<0>;
> > > data-lanes =<2 3 1>;
> > 
> > Sounds good to me. And the clock-lane could be made optional, as not all
> > devices would need it.
> > 
> > However, as far as I can see, there is currently no generic API for handling
> > this kind of data structure. E.g. number of cells for the "interrupts"
> > property is specified with an additional "#interrupt-cells" property.
> > 
> > It would have been much easier to handle something like:
> > 
> > data-lanes = <2>, <3>, <1>;
> > 
> > i.e. an array of the lane indexes.
> 
> I'm fine with that.

...on a second thought: shouldn't this be handled by pinctrl? Or is it 
some CSI-2 module internal lane switching, not the global SoC pin function 
configuration?

Thanks
Guennadi

> > > For receivers that can't reroute lanes internally, the data-lanes property
> > > would be need to specify lanes in sequence.
> > > 
> > > data-lanes =<1 2 3>;
> > 
> > In this case we would be only interested in the number of cells in this
> > property, but how it could be retrieved ? With an array, it could have been
> > calculated from property length returned by of_property_find() (divided by
> > sizof(u32)).
> 
> Agreed.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 31 July 2012 12:58:50 Guennadi Liakhovetski wrote:
> On Fri, 27 Jul 2012, Laurent Pinchart wrote:
> > On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:
> > > On 07/26/2012 04:38 PM, Laurent Pinchart wrote:
> > > >>>> --- /dev/null
> > > >>>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> > > >>>> @@ -0,0 +1,5 @@
> > > >>>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and
> > > >>>> transmitters
> > > >>>> +
> > > >>>> + - data-lanes : number of differential data lanes wired and
> > > >>>> actively
> > > >>>> used in
> > > >>>> +		communication between the transmitter and the receiver, this
> > > >>>> +		excludes the clock lane;
> > > >>> 
> > > >>> Wouldn't it be better to use the standard "bus-width" DT property?
> > > >> 
> > > >> I can't see any problems with using "bus-width". It seems sufficient
> > > >> and could indeed be better, without a need to invent new MIPI-CSI
> > > >> specific names. That was my first RFC on that and my perspective
> > > >> wasn't probably broad enough. :)
> > > > 
> > > > What about CSI receivers that can reroute the lanes internally ? We
> > > > would
> > > > need to specify lane indices for each lane then, maybe with something
> > > > like
> > > > 
> > > > clock-lane =<0>;
> > > > data-lanes =<2 3 1>;
> > > 
> > > Sounds good to me. And the clock-lane could be made optional, as not all
> > > devices would need it.
> > > 
> > > However, as far as I can see, there is currently no generic API for
> > > handling this kind of data structure. E.g. number of cells for the
> > > "interrupts" property is specified with an additional
> > > "#interrupt-cells" property.
> > > 
> > > It would have been much easier to handle something like:
> > > 
> > > data-lanes = <2>, <3>, <1>;
> > > 
> > > i.e. an array of the lane indexes.
> > 
> > I'm fine with that.
> 
> ...on a second thought: shouldn't this be handled by pinctrl? Or is it
> some CSI-2 module internal lane switching, not the global SoC pin function
> configuration?

On the hardware I came across, it's handled by the CSI2 receiver, not the SoC 
pinmux feature.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 31 July 2012 12:58:50 Guennadi Liakhovetski wrote:
> On Fri, 27 Jul 2012, Laurent Pinchart wrote:
> > On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote:
> > > On 07/26/2012 04:38 PM, Laurent Pinchart wrote:
> > > >>>> --- /dev/null
> > > >>>> +++ b/Documentation/devicetree/bindings/video/mipi.txt
> > > >>>> @@ -0,0 +1,5 @@
> > > >>>> +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and
> > > >>>> transmitters
> > > >>>> +
> > > >>>> + - data-lanes : number of differential data lanes wired and
> > > >>>> actively
> > > >>>> used in
> > > >>>> +		communication between the transmitter and the receiver, this
> > > >>>> +		excludes the clock lane;
> > > >>> 
> > > >>> Wouldn't it be better to use the standard "bus-width" DT property?
> > > >> 
> > > >> I can't see any problems with using "bus-width". It seems sufficient
> > > >> and could indeed be better, without a need to invent new MIPI-CSI
> > > >> specific names. That was my first RFC on that and my perspective
> > > >> wasn't probably broad enough. :)
> > > > 
> > > > What about CSI receivers that can reroute the lanes internally ? We
> > > > would
> > > > need to specify lane indices for each lane then, maybe with something
> > > > like
> > > > 
> > > > clock-lane =<0>;
> > > > data-lanes =<2 3 1>;
> > > 
> > > Sounds good to me. And the clock-lane could be made optional, as not all
> > > devices would need it.
> > > 
> > > However, as far as I can see, there is currently no generic API for
> > > handling this kind of data structure. E.g. number of cells for the
> > > "interrupts" property is specified with an additional
> > > "#interrupt-cells" property.
> > > 
> > > It would have been much easier to handle something like:
> > > 
> > > data-lanes = <2>, <3>, <1>;
> > > 
> > > i.e. an array of the lane indexes.
> > 
> > I'm fine with that.
> 
> ...on a second thought: shouldn't this be handled by pinctrl? Or is it
> some CSI-2 module internal lane switching, not the global SoC pin function
> configuration?

On the hardware I came across, it's handled by the CSI2 receiver, not the SoC 
pinmux feature.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Tue, 31 Jul 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > >> The driver initializes all board related properties except the
> > > > >> s_power() callback to board code. The platforms that require this
> > > > >> callback are not supported by this driver yet for CONFIG_OF=y.
> > > > >> 
> > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > >> ---
> > > > >> 
> > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > >>   deletions(-)
> > > > >>   create mode 100644
> > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
> > > > >> 
> > > > >> diff --git
> > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new
> > > > >> file
> > > > >> mode 100644
> > > > >> index 0000000..6685a9c
> > > > >> --- /dev/null
> > > > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > >> @@ -0,0 +1,57 @@
> > > > >> +Samsung S5K6AAFX camera sensor
> > > > >> +------------------------------
> > > > >> +
> > > > >> +Required properties:
> > > > >> +
> > > > >> +- compatible : "samsung,s5k6aafx";
> > > > >> +- reg : base address of the device on I2C bus;
> > > > > 
> > > > > You said you ended up putting your sensors outside of I2C busses, is
> > > > > this
> > > > > one of changes, that are present in your git-tree but not in this
> > > > > series?
> > > > 
> > > > No, I must have been not clear enough on that. Our idea was to keep
> > > > I2C slave device nodes as an I2C controller's child nodes, according
> > > > to the current convention.
> > > > The 'sensor' nodes (the 'camera''s children) would only contain a
> > > > phandle to a respective I2C slave node.
> > > > 
> > > > This implies that we cannot access I2C bus in I2C client's device
> > > > probe() callback. An actual H/W access could begin only from within and
> > > > after invocation of v4l2_subdev .registered callback..
> > > 
> > > That's how I've envisioned the DT bindings for sensors as well, this
> > > sounds good. The real challenge will be to get hold of the subdev to
> > > register it without race conditions.
> > 
> > Hrm... That's how early pre-subdev versions of soc-camera used to work,
> > that's where all the <device>_video_probe() functions come from. But then
> > we switched to dynamic i2c device registration. Do we want to switch all
> > drivers back now?... Couldn't we "temporarily" use references from subdevs
> > to hosts until the clock API is available?
> 
> I don't think that requires a reference from subdevs to hosts in the DT. The 
> subdev will need the host to be probed before a clock can be available so you 
> won't be able to access the hardware in the probe() function in the generic 
> case. You will need to wait until the registered() subdev operation is called, 
> at which point the host can be accessed through the v4l2_device.

Sure, I understand, but that's exactly what we wanted to avoid - 
succeeding client's i2c .probe() without even touching the hardware.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:
> On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > > >> The driver initializes all board related properties except the
> > > > > >> s_power() callback to board code. The platforms that require this
> > > > > >> callback are not supported by this driver yet for CONFIG_OF=y.
> > > > > >> 
> > > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > > >> Signed-off-by: Bartlomiej
> > > > > >> Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > > >> ---
> > > > > >> 
> > > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57
> > > > > >>   +++++++++
> > > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > > >>   deletions(-)
> > > > > >>   create mode 100644
> > > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
> > > > > >> 
> > > > > >> diff --git
> > > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > >> new
> > > > > >> file
> > > > > >> mode 100644
> > > > > >> index 0000000..6685a9c
> > > > > >> --- /dev/null
> > > > > >> +++
> > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > >> @@ -0,0 +1,57 @@
> > > > > >> +Samsung S5K6AAFX camera sensor
> > > > > >> +------------------------------
> > > > > >> +
> > > > > >> +Required properties:
> > > > > >> +
> > > > > >> +- compatible : "samsung,s5k6aafx";
> > > > > >> +- reg : base address of the device on I2C bus;
> > > > > > 
> > > > > > You said you ended up putting your sensors outside of I2C busses,
> > > > > > is this one of changes, that are present in your git-tree but not
> > > > > > in this series?
> > > > > 
> > > > > No, I must have been not clear enough on that. Our idea was to keep
> > > > > I2C slave device nodes as an I2C controller's child nodes, according
> > > > > to the current convention.
> > > > > The 'sensor' nodes (the 'camera''s children) would only contain a
> > > > > phandle to a respective I2C slave node.
> > > > > 
> > > > > This implies that we cannot access I2C bus in I2C client's device
> > > > > probe() callback. An actual H/W access could begin only from within
> > > > > and after invocation of v4l2_subdev .registered callback..
> > > > 
> > > > That's how I've envisioned the DT bindings for sensors as well, this
> > > > sounds good. The real challenge will be to get hold of the subdev to
> > > > register it without race conditions.
> > > 
> > > Hrm... That's how early pre-subdev versions of soc-camera used to work,
> > > that's where all the <device>_video_probe() functions come from. But
> > > then we switched to dynamic i2c device registration. Do we want to
> > > switch all drivers back now?... Couldn't we "temporarily" use references
> > > from subdevs to hosts until the clock API is available?
> > 
> > I don't think that requires a reference from subdevs to hosts in the DT.
> > The subdev will need the host to be probed before a clock can be
> > available so you won't be able to access the hardware in the probe()
> > function in the generic case. You will need to wait until the
> > registered() subdev operation is called, at which point the host can be
> > accessed through the v4l2_device.
> 
> Sure, I understand, but that's exactly what we wanted to avoid -
> succeeding client's i2c .probe() without even touching the hardware.

But should we allow host probe() to succeed if the sensor isn't present ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Tue, 31 Jul 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:
> > On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > > On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > > > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > > > >> The driver initializes all board related properties except the
> > > > > > >> s_power() callback to board code. The platforms that require this
> > > > > > >> callback are not supported by this driver yet for CONFIG_OF=y.
> > > > > > >> 
> > > > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > > > >> Signed-off-by: Bartlomiej
> > > > > > >> Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > > > >> ---
> > > > > > >> 
> > > > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57
> > > > > > >>   +++++++++
> > > > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > > > >>   deletions(-)
> > > > > > >>   create mode 100644
> > > > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt>>
> > > > > > >> 
> > > > > > >> diff --git
> > > > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > > >> new
> > > > > > >> file
> > > > > > >> mode 100644
> > > > > > >> index 0000000..6685a9c
> > > > > > >> --- /dev/null
> > > > > > >> +++
> > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > > > > > >> @@ -0,0 +1,57 @@
> > > > > > >> +Samsung S5K6AAFX camera sensor
> > > > > > >> +------------------------------
> > > > > > >> +
> > > > > > >> +Required properties:
> > > > > > >> +
> > > > > > >> +- compatible : "samsung,s5k6aafx";
> > > > > > >> +- reg : base address of the device on I2C bus;
> > > > > > > 
> > > > > > > You said you ended up putting your sensors outside of I2C busses,
> > > > > > > is this one of changes, that are present in your git-tree but not
> > > > > > > in this series?
> > > > > > 
> > > > > > No, I must have been not clear enough on that. Our idea was to keep
> > > > > > I2C slave device nodes as an I2C controller's child nodes, according
> > > > > > to the current convention.
> > > > > > The 'sensor' nodes (the 'camera''s children) would only contain a
> > > > > > phandle to a respective I2C slave node.
> > > > > > 
> > > > > > This implies that we cannot access I2C bus in I2C client's device
> > > > > > probe() callback. An actual H/W access could begin only from within
> > > > > > and after invocation of v4l2_subdev .registered callback..
> > > > > 
> > > > > That's how I've envisioned the DT bindings for sensors as well, this
> > > > > sounds good. The real challenge will be to get hold of the subdev to
> > > > > register it without race conditions.
> > > > 
> > > > Hrm... That's how early pre-subdev versions of soc-camera used to work,
> > > > that's where all the <device>_video_probe() functions come from. But
> > > > then we switched to dynamic i2c device registration. Do we want to
> > > > switch all drivers back now?... Couldn't we "temporarily" use references
> > > > from subdevs to hosts until the clock API is available?
> > > 
> > > I don't think that requires a reference from subdevs to hosts in the DT.
> > > The subdev will need the host to be probed before a clock can be
> > > available so you won't be able to access the hardware in the probe()
> > > function in the generic case. You will need to wait until the
> > > registered() subdev operation is called, at which point the host can be
> > > accessed through the v4l2_device.
> > 
> > Sure, I understand, but that's exactly what we wanted to avoid -
> > succeeding client's i2c .probe() without even touching the hardware.
> 
> But should we allow host probe() to succeed if the sensor isn't present ?

I think we should, yes. The host hardware is there and functional - 
whether or not all or some of the clients are failing. Theoretically 
clients can also be hot-plugged. Whether and how many video device nodes 
we create, that's a different question.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 31 July 2012 13:29:55 Guennadi Liakhovetski wrote:
> On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:
> > > On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > > > On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > > > > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > > > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > > > > >> The driver initializes all board related properties except
> > > > > > > >> the s_power() callback to board code. The platforms that
> > > > > > > >> require this callback are not supported by this driver yet
> > > > > > > >> for CONFIG_OF=y.
> > > > > > > >> 
> > > > > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > > > > >> Signed-off-by: Bartlomiej
> > > > > > > >> Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > > > > >> ---
> > > > > > > >> 
> > > > > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57
> > > > > > > >>   +++++++++
> > > > > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > > > > >>   deletions(-)
> > > > > > > >>   create mode 100644
> > > > > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >>   xt>>
> > > > > > > >> 
> > > > > > > >> diff --git
> > > > > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> new
> > > > > > > >> file
> > > > > > > >> mode 100644
> > > > > > > >> index 0000000..6685a9c
> > > > > > > >> --- /dev/null
> > > > > > > >> +++
> > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> @@ -0,0 +1,57 @@
> > > > > > > >> +Samsung S5K6AAFX camera sensor
> > > > > > > >> +------------------------------
> > > > > > > >> +
> > > > > > > >> +Required properties:
> > > > > > > >> +
> > > > > > > >> +- compatible : "samsung,s5k6aafx";
> > > > > > > >> +- reg : base address of the device on I2C bus;
> > > > > > > > 
> > > > > > > > You said you ended up putting your sensors outside of I2C
> > > > > > > > busses, is this one of changes, that are present in your git-
> > > > > > > > tree but not in this series?
> > > > > > > 
> > > > > > > No, I must have been not clear enough on that. Our idea was to
> > > > > > > keep I2C slave device nodes as an I2C controller's child nodes,
> > > > > > > according to the current convention. The 'sensor' nodes (the
> > > > > > > 'camera''s children) would only contain a phandle to a
> > > > > > > respective I2C slave node.
> > > > > > > 
> > > > > > > This implies that we cannot access I2C bus in I2C client's
> > > > > > > device probe() callback. An actual H/W access could begin only
> > > > > > > from within and after invocation of v4l2_subdev .registered
> > > > > > > callback..
> > > > > > 
> > > > > > That's how I've envisioned the DT bindings for sensors as well,
> > > > > > this sounds good. The real challenge will be to get hold of the
> > > > > > subdev to register it without race conditions.
> > > > > 
> > > > > Hrm... That's how early pre-subdev versions of soc-camera used to
> > > > > work, that's where all the <device>_video_probe() functions come
> > > > > from. But then we switched to dynamic i2c device registration. Do we
> > > > > want to switch all drivers back now?... Couldn't we "temporarily"
> > > > > use references from subdevs to hosts until the clock API is
> > > > > available?
> > > > 
> > > > I don't think that requires a reference from subdevs to hosts in the
> > > > DT. The subdev will need the host to be probed before a clock can be
> > > > available so you won't be able to access the hardware in the probe()
> > > > function in the generic case. You will need to wait until the
> > > > registered() subdev operation is called, at which point the host can
> > > > be accessed through the v4l2_device.
> > > 
> > > Sure, I understand, but that's exactly what we wanted to avoid -
> > > succeeding client's i2c .probe() without even touching the hardware.
> > 
> > But should we allow host probe() to succeed if the sensor isn't present ?
> 
> I think we should, yes. The host hardware is there and functional -
> whether or not all or some of the clients are failing. Theoretically
> clients can also be hot-plugged. Whether and how many video device nodes
> we create, that's a different question.

I think I can agree with you on this (although I could change my mind if this 
architecture turns out to result in unsolvable technical issues). That will 
involve a lot of work though.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Tue, 31 Jul 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 31 July 2012 13:29:55 Guennadi Liakhovetski wrote:
> > On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > > On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:
> > > > On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > > > > On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > > > > > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > > > > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > > > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > > > > > >> The driver initializes all board related properties except
> > > > > > > > >> the s_power() callback to board code. The platforms that
> > > > > > > > >> require this callback are not supported by this driver yet
> > > > > > > > >> for CONFIG_OF=y.
> > > > > > > > >> 
> > > > > > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > > > > > >> Signed-off-by: Bartlomiej
> > > > > > > > >> Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > > > > > >> ---
> > > > > > > > >> 
> > > > > > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57
> > > > > > > > >>   +++++++++
> > > > > > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > > > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > > > > > >>   deletions(-)
> > > > > > > > >>   create mode 100644
> > > > > > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > > >>   xt>>
> > > > > > > > >> 
> > > > > > > > >> diff --git
> > > > > > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > > >> xt
> > > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > > >> xt
> > > > > > > > >> new
> > > > > > > > >> file
> > > > > > > > >> mode 100644
> > > > > > > > >> index 0000000..6685a9c
> > > > > > > > >> --- /dev/null
> > > > > > > > >> +++
> > > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > > >> xt
> > > > > > > > >> @@ -0,0 +1,57 @@
> > > > > > > > >> +Samsung S5K6AAFX camera sensor
> > > > > > > > >> +------------------------------
> > > > > > > > >> +
> > > > > > > > >> +Required properties:
> > > > > > > > >> +
> > > > > > > > >> +- compatible : "samsung,s5k6aafx";
> > > > > > > > >> +- reg : base address of the device on I2C bus;
> > > > > > > > > 
> > > > > > > > > You said you ended up putting your sensors outside of I2C
> > > > > > > > > busses, is this one of changes, that are present in your git-
> > > > > > > > > tree but not in this series?
> > > > > > > > 
> > > > > > > > No, I must have been not clear enough on that. Our idea was to
> > > > > > > > keep I2C slave device nodes as an I2C controller's child nodes,
> > > > > > > > according to the current convention. The 'sensor' nodes (the
> > > > > > > > 'camera''s children) would only contain a phandle to a
> > > > > > > > respective I2C slave node.
> > > > > > > > 
> > > > > > > > This implies that we cannot access I2C bus in I2C client's
> > > > > > > > device probe() callback. An actual H/W access could begin only
> > > > > > > > from within and after invocation of v4l2_subdev .registered
> > > > > > > > callback..
> > > > > > > 
> > > > > > > That's how I've envisioned the DT bindings for sensors as well,
> > > > > > > this sounds good. The real challenge will be to get hold of the
> > > > > > > subdev to register it without race conditions.
> > > > > > 
> > > > > > Hrm... That's how early pre-subdev versions of soc-camera used to
> > > > > > work, that's where all the <device>_video_probe() functions come
> > > > > > from. But then we switched to dynamic i2c device registration. Do we
> > > > > > want to switch all drivers back now?... Couldn't we "temporarily"
> > > > > > use references from subdevs to hosts until the clock API is
> > > > > > available?
> > > > > 
> > > > > I don't think that requires a reference from subdevs to hosts in the
> > > > > DT. The subdev will need the host to be probed before a clock can be
> > > > > available so you won't be able to access the hardware in the probe()
> > > > > function in the generic case. You will need to wait until the
> > > > > registered() subdev operation is called, at which point the host can
> > > > > be accessed through the v4l2_device.
> > > > 
> > > > Sure, I understand, but that's exactly what we wanted to avoid -
> > > > succeeding client's i2c .probe() without even touching the hardware.
> > > 
> > > But should we allow host probe() to succeed if the sensor isn't present ?
> > 
> > I think we should, yes. The host hardware is there and functional -
> > whether or not all or some of the clients are failing. Theoretically
> > clients can also be hot-plugged. Whether and how many video device nodes
> > we create, that's a different question.
> 
> I think I can agree with you on this (although I could change my mind if this 
> architecture turns out to result in unsolvable technical issues). That will 
> involve a lot of work though.

There's however at least one more gotcha that occurs to me with this 
approach: if clients fail to probe, how do we find out about that and turn 
clocks back off? One improvement to turning clocks on immediately in 
host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But 
how do we find out, that probing failed? No notifier is called in this 
case. We could use a time-out, but that's ugly. I think, we could ever 
request a new notifier for this case. We could also require client drivers 
to call a V4L2 function in this case, but that's not very pretty either.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Sylwester Nawrocki]

On 07/31/2012 01:05 PM, Laurent Pinchart wrote:
>>>>> What about CSI receivers that can reroute the lanes internally ? We
>>>>> would
>>>>> need to specify lane indices for each lane then, maybe with something
>>>>> like
>>>>>
>>>>> clock-lane =<0>;
>>>>> data-lanes =<2 3 1>;
>>>>
>>>> Sounds good to me. And the clock-lane could be made optional, as not all
>>>> devices would need it.
>>>>
>>>> However, as far as I can see, there is currently no generic API for
>>>> handling this kind of data structure. E.g. number of cells for the
>>>> "interrupts" property is specified with an additional
>>>> "#interrupt-cells" property.
>>>>
>>>> It would have been much easier to handle something like:
>>>>
>>>> data-lanes = <2>, <3>, <1>;
>>>>
>>>> i.e. an array of the lane indexes.
>>>
>>> I'm fine with that.
>>
>> ...on a second thought: shouldn't this be handled by pinctrl? Or is it
>> some CSI-2 module internal lane switching, not the global SoC pin function
>> configuration?
> 
> On the hardware I came across, it's handled by the CSI2 receiver, not the SoC 
> pinmux feature.

Same here. Is there any hardware known to mux those MIPI-CSI
D-PHY high speed differential signals with general purpose IO pins ?

Or are there mostly dedicated pins used ?

However, if there are cases the lane configuration is done solely at CSI2
receiver level, there seems little point in involving the pinctrl API.

--

Regards,
Sylwester

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sylwester Nawrocki]

On 07/31/2012 02:26 PM, Guennadi Liakhovetski wrote:
>>>> But should we allow host probe() to succeed if the sensor isn't present ?
>>>
>>> I think we should, yes. The host hardware is there and functional -
>>> whether or not all or some of the clients are failing. Theoretically
>>> clients can also be hot-plugged. Whether and how many video device nodes
>>> we create, that's a different question.
>>
>> I think I can agree with you on this (although I could change my mind if this 
>> architecture turns out to result in unsolvable technical issues). That will 
>> involve a lot of work though.
> 
> There's however at least one more gotcha that occurs to me with this 
> approach: if clients fail to probe, how do we find out about that and turn 
> clocks back off? One improvement to turning clocks on immediately in 

Hmm, wouldn't it be the client that turns a clock on/off when needed ?
I'd like to preserve this functionality, so client drivers can have
full control on the power up/down sequences. While we are trying to
improve the current situation...

> host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But 
> how do we find out, that probing failed? No notifier is called in this 
> case. We could use a time-out, but that's ugly. I think, we could ever 
> request a new notifier for this case. We could also require client drivers 
> to call a V4L2 function in this case, but that's not very pretty either.

--

Regards,
Sylwester

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Guennadi Liakhovetski]

On Tue, 31 Jul 2012, Sylwester Nawrocki wrote:

> On 07/31/2012 02:26 PM, Guennadi Liakhovetski wrote:
> >>>> But should we allow host probe() to succeed if the sensor isn't present ?
> >>>
> >>> I think we should, yes. The host hardware is there and functional -
> >>> whether or not all or some of the clients are failing. Theoretically
> >>> clients can also be hot-plugged. Whether and how many video device nodes
> >>> we create, that's a different question.
> >>
> >> I think I can agree with you on this (although I could change my mind if this 
> >> architecture turns out to result in unsolvable technical issues). That will 
> >> involve a lot of work though.
> > 
> > There's however at least one more gotcha that occurs to me with this 
> > approach: if clients fail to probe, how do we find out about that and turn 
> > clocks back off? One improvement to turning clocks on immediately in 
> 
> Hmm, wouldn't it be the client that turns a clock on/off when needed ?
> I'd like to preserve this functionality, so client drivers can have
> full control on the power up/down sequences. While we are trying to
> improve the current situation...

Eventually, when the clock API is available - yes, the client would just 
call clk_enable() / clk_disable(). But for now isn't it host's job to do 
that? Or you want to add new API for that?

Thanks
Guennadi

> > host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But 
> > how do we find out, that probing failed? No notifier is called in this 
> > case. We could use a time-out, but that's ugly. I think, we could ever 
> > request a new notifier for this case. We could also require client drivers 
> > to call a V4L2 function in this case, but that's not very pretty either.

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sylwester Nawrocki]

On 07/31/2012 02:59 PM, Guennadi Liakhovetski wrote:
> On Tue, 31 Jul 2012, Sylwester Nawrocki wrote:
>> On 07/31/2012 02:26 PM, Guennadi Liakhovetski wrote:
>>>>>> But should we allow host probe() to succeed if the sensor isn't present ?
>>>>>
>>>>> I think we should, yes. The host hardware is there and functional -
>>>>> whether or not all or some of the clients are failing. Theoretically
>>>>> clients can also be hot-plugged. Whether and how many video device nodes
>>>>> we create, that's a different question.
>>>>
>>>> I think I can agree with you on this (although I could change my mind if this 
>>>> architecture turns out to result in unsolvable technical issues). That will 
>>>> involve a lot of work though.
>>>
>>> There's however at least one more gotcha that occurs to me with this 
>>> approach: if clients fail to probe, how do we find out about that and turn 
>>> clocks back off? One improvement to turning clocks on immediately in 
>>
>> Hmm, wouldn't it be the client that turns a clock on/off when needed ?
>> I'd like to preserve this functionality, so client drivers can have
>> full control on the power up/down sequences. While we are trying to
>> improve the current situation...
> 
> Eventually, when the clock API is available - yes, the client would just 
> call clk_enable() / clk_disable(). But for now isn't it host's job to do 
> that? Or you want to add new API for that?

Indeed, looking at existing drivers, the clocks' handling is now mostly
done in the host drivers. Only the omap3isp appears to do the right thing,
i.e. delegating control over the clock to client drivers, but it does it
with platform_data callbacks.

We've already discussed adding a new API for that,
http://www.mail-archive.com/linux-media@vger.kernel.org/msg35359.html

However using common clock API and binding a clock to client device
(either DT based or not) sounds like a best approach to me.

Waiting for the common clock API to be generally available maybe we
could add some clock ops at the v4l2_device ? Just a humble suggestion,
I'm not sure whether it is really good and needed or not.

--

Thanks,
Sylwester

Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 31 July 2012 14:38:35 Sylwester Nawrocki wrote:
> On 07/31/2012 01:05 PM, Laurent Pinchart wrote:
> >>>>> What about CSI receivers that can reroute the lanes internally ? We
> >>>>> would need to specify lane indices for each lane then, maybe with
> >>>>> something like
> >>>>> 
> >>>>> clock-lane =<0>;
> >>>>> data-lanes =<2 3 1>;
> >>>> 
> >>>> Sounds good to me. And the clock-lane could be made optional, as not
> >>>> all devices would need it.
> >>>> 
> >>>> However, as far as I can see, there is currently no generic API for
> >>>> handling this kind of data structure. E.g. number of cells for the
> >>>> "interrupts" property is specified with an additional
> >>>> "#interrupt-cells" property.
> >>>> 
> >>>> It would have been much easier to handle something like:
> >>>> 
> >>>> data-lanes = <2>, <3>, <1>;
> >>>> 
> >>>> i.e. an array of the lane indexes.
> >>> 
> >>> I'm fine with that.
> >> 
> >> ...on a second thought: shouldn't this be handled by pinctrl? Or is it
> >> some CSI-2 module internal lane switching, not the global SoC pin
> >> function configuration?
> > 
> > On the hardware I came across, it's handled by the CSI2 receiver, not the
> > SoC pinmux feature.
> 
> Same here. Is there any hardware known to mux those MIPI-CSI
> D-PHY high speed differential signals with general purpose IO pins ?
> 
> Or are there mostly dedicated pins used ?

The OMAP3 multiplexes the CSI pins with other functions.

> However, if there are cases the lane configuration is done solely at CSI2
> receiver level, there seems little point in involving the pinctrl API.

The OMAP3 handles lane routing in the CSI2 receiver.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 31 July 2012 15:28:52 Sylwester Nawrocki wrote:
> On 07/31/2012 02:59 PM, Guennadi Liakhovetski wrote:
> > On Tue, 31 Jul 2012, Sylwester Nawrocki wrote:
> >> On 07/31/2012 02:26 PM, Guennadi Liakhovetski wrote:
> >>>>>> But should we allow host probe() to succeed if the sensor isn't
> >>>>>> present ?
> >>>>> 
> >>>>> I think we should, yes. The host hardware is there and functional -
> >>>>> whether or not all or some of the clients are failing. Theoretically
> >>>>> clients can also be hot-plugged. Whether and how many video device
> >>>>> nodes
> >>>>> we create, that's a different question.
> >>>> 
> >>>> I think I can agree with you on this (although I could change my mind
> >>>> if this architecture turns out to result in unsolvable technical
> >>>> issues). That will involve a lot of work though.
> >>> 
> >>> There's however at least one more gotcha that occurs to me with this
> >>> approach: if clients fail to probe, how do we find out about that and
> >>> turn
> >>> clocks back off? One improvement to turning clocks on immediately in
> >> 
> >> Hmm, wouldn't it be the client that turns a clock on/off when needed ?
> >> I'd like to preserve this functionality, so client drivers can have
> >> full control on the power up/down sequences. While we are trying to
> >> improve the current situation...
> > 
> > Eventually, when the clock API is available - yes, the client would just
> > call clk_enable() / clk_disable(). But for now isn't it host's job to do
> > that? Or you want to add new API for that?
> 
> Indeed, looking at existing drivers, the clocks' handling is now mostly
> done in the host drivers. Only the omap3isp appears to do the right thing,
> i.e. delegating control over the clock to client drivers, but it does it
> with platform_data callbacks.
> 
> We've already discussed adding a new API for that,
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg35359.html
> 
> However using common clock API and binding a clock to client device
> (either DT based or not) sounds like a best approach to me.
> 
> Waiting for the common clock API to be generally available maybe we
> could add some clock ops at the v4l2_device ? Just a humble suggestion,
> I'm not sure whether it is really good and needed or not.

I'm fine with that (or something similar) as an interim solution.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Sakari Ailus]

Hi Laurent and Sylwester,

My apologies for the late answer.

On Fri, Jul 27, 2012 at 12:50:13AM +0200, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote:
> > On 07/26/2012 05:21 PM, Laurent Pinchart wrote:
> > > On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:
> > >> The driver initializes all board related properties except the s_power()
> > >> callback to board code. The platforms that require this callback are not
> > >> supported by this driver yet for CONFIG_OF=y.
> > >> 
> > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > >> Signed-off-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com>
> > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > >> ---
> > >> 
> > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57 +++++++++
> > >>   drivers/media/video/s5k6aa.c                       |  129 +++++++++----
> > >>   2 files changed, 146 insertions(+), 40 deletions(-)
> > >>   create mode 100644
> > >> 
> > >> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > >> 
> > >> diff --git
> > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
> > >> mode 100644
> > >> index 0000000..6685a9c
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
> > >> @@ -0,0 +1,57 @@
> > >> +Samsung S5K6AAFX camera sensor
> > >> +------------------------------
> > >> +
> > >> +Required properties:
> > >> +
> > >> +- compatible : "samsung,s5k6aafx";
> > >> +- reg : base address of the device on I2C bus;
> > >> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
> > >> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
> > >> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
> > >> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to
> > >> 1.9V) +		   or 2.8V (2.6V to 3.0);
> > >> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
> > >> +		 or 2.8V (2.5V to 3.1V);
> > >> +
> > >> +Optional properties:
> > >> +
> > >> +- clock-frequency : the IP's main (system bus) clock frequency in Hz,
> > >> the default
> > > 
> > > Is that the input clock frequency ? Can't it vary ? Instead of accessing
> > > the
> > Yes, the description is incorrect in this patch, it should read:
> > 
> > +- clock-frequency : the sensor's master clock frequency in Hz;
> > 
> > and be a required property. As in this patch:
> > https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7
> > abe563
> > 
> > It could vary (as this is a PLL input frequency), so probably a range would
> > be a better alternative. Given that host device won't always be able to set
> > this exact value...
> 
> A range sounds good, or perhaps a list of ranges. Sakari, what would you need 
> for the SMIA++ driver ?

Typically the sensor's external clock is derived from another clock using a
divisor. This means there's usually quite limited selection of possible
clock frequencies since the sensors usually have a small range of possible
frequencies such as 6 -- 27 MHz or even less.

Also it's important to choose the frequency in such a way that it doesn't
interfere with other devices in the system. The frequency also must be
picked so that one can achieve the desired highest data transfer rate. The
sensors are also quite flexible in their internal clock tree configuration,
but in the situation where the desired data rate is close to sensor limits
there may be additional constraints. Still it's always possible to come up
with a best value for the board while other values are inferior.

For these reasons I see little point in providing anything else than just a
single value for the external clock frequency. This value is board-specific.

> > > sensor clock frequency from the FIMC driver you should reference a clock
> > > in the sensor DT node. That obviously requires generic clock support,
> > > which might not be available for your platform yet (that's one of the
> > > reasons the OMAP3 ISP driver doesn't support DT yet).
> > 
> > I agree it might be better, but waiting unknown number of kernel releases
> > for the platforms to get converted to common clock API is not a good
> > alternative either. I guess we could have some transitional solutions while
> > other subsystems are getting adapted.
> 
> I agree, we need an interim solution.

The SMIA++ driver allows the platform data to have either the clock name or
a function pointer to set the clock frequency. If the clock name is there
it'll be used instead. The function pointer can be removed once it's no
longer needed.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk