[PATCH 0/4] ARM: S5P: Add platform support for MIPI-CSI2 devices

[PATCH 0/4] ARM: S5P: Add platform support for MIPI-CSI2 devices

[Sylwester Nawrocki]

Hello,

the following patch series adds S5P platform support for MIPI-CSI2 devices.
It has been rebased onto next-samsung branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git 

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis
[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis
[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices
[PATCH 4/4] mach-s5pv210: Add MIPI-CSI DPHY clock definition


Thanks.

--
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Sylwester Nawrocki]

Added resource definitions for mipi-csis interface, naming
changed for consistency with s5pv310 where there are two
instances of the device.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/include/mach/irqs.h       |    2 +-
 arch/arm/mach-s5pv210/include/mach/map.h        |    4 ++++
 arch/arm/mach-s5pv210/include/mach/regs-clock.h |    5 +----
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-s5pv210/include/mach/irqs.h
index 119b95f..8b5994c 100644
--- a/arch/arm/mach-s5pv210/include/mach/irqs.h
+++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
@@ -65,7 +65,7 @@
 #define IRQ_HSMMC0		S5P_IRQ_VIC1(26)
 #define IRQ_HSMMC1		S5P_IRQ_VIC1(27)
 #define IRQ_HSMMC2		S5P_IRQ_VIC1(28)
-#define IRQ_MIPICSI		S5P_IRQ_VIC1(29)
+#define IRQ_MIPICSI0		S5P_IRQ_VIC1(29)
 #define IRQ_MIPIDSI		S5P_IRQ_VIC1(30)
 #define IRQ_ONENAND_AUDI	S5P_IRQ_VIC1(31)
 
diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
index 861d7fe..8f376f1 100644
--- a/arch/arm/mach-s5pv210/include/mach/map.h
+++ b/arch/arm/mach-s5pv210/include/mach/map.h
@@ -107,6 +107,9 @@
 #define S5PV210_PA_DMC0		(0xF0000000)
 #define S5PV210_PA_DMC1		(0xF1400000)
 
+/* MIPI CSI */
+#define S5PV210_PA_CSIS		0xFA600000
+
 /* compatibiltiy defines. */
 #define S3C_PA_UART		S5PV210_PA_UART
 #define S3C_PA_HSMMC0		S5PV210_PA_HSMMC(0)
@@ -123,6 +126,7 @@
 #define S5P_PA_FIMC0		S5PV210_PA_FIMC0
 #define S5P_PA_FIMC1		S5PV210_PA_FIMC1
 #define S5P_PA_FIMC2		S5PV210_PA_FIMC2
+#define S5P_PA_CSIS0		S5PV210_PA_CSIS
 
 #define SAMSUNG_PA_ADC		S5PV210_PA_ADC
 #define SAMSUNG_PA_CFCON	S5PV210_PA_CFCON
diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
index ebaabe0..4c45b74 100644
--- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
+++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
@@ -161,7 +161,7 @@
 #define S5P_MDNIE_SEL		S5P_CLKREG(0x7008)
 #define S5P_MIPI_PHY_CON0	S5P_CLKREG(0x7200)
 #define S5P_MIPI_PHY_CON1	S5P_CLKREG(0x7204)
-#define S5P_MIPI_CONTROL	S5P_CLKREG(0xE814)
+#define S5P_MIPI_DPHY_CONTROL	S5P_CLKREG(0xE814)
 
 #define S5P_IDLE_CFG_TL_MASK	(3 << 30)
 #define S5P_IDLE_CFG_TM_MASK	(3 << 28)
@@ -195,9 +195,6 @@
 #define S5P_OTHERS_RET_UART		(1 << 28)
 #define S5P_OTHERS_USB_SIG_MASK		(1 << 16)
 
-/* MIPI */
-#define S5P_MIPI_DPHY_EN		(3)
-
 /* S5P_DAC_CONTROL */
 #define S5P_DAC_ENABLE			(1)
 #define S5P_DAC_DISABLE			(0)
-- 
1.7.1

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Sylwester Nawrocki]

Add IRQ and register base address definitions for mipi-csis devices.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv310/include/mach/irqs.h |    3 +++
 arch/arm/mach-s5pv310/include/mach/map.h  |    5 +++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-s5pv310/include/mach/irqs.h b/arch/arm/mach-s5pv310/include/mach/irqs.h
index 99e7dad..db63773 100644
--- a/arch/arm/mach-s5pv310/include/mach/irqs.h
+++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
@@ -83,6 +83,9 @@
 #define IRQ_HSMMC2		COMBINER_IRQ(29, 2)
 #define IRQ_HSMMC3		COMBINER_IRQ(29, 3)
 
+#define IRQ_MIPICSI0		COMBINER_IRQ(30, 0)
+#define IRQ_MIPICSI1		COMBINER_IRQ(30, 1)
+
 #define IRQ_ONENAND_AUDI	COMBINER_IRQ(34, 0)
 
 #define IRQ_EINT4		COMBINER_IRQ(37, 0)
diff --git a/arch/arm/mach-s5pv310/include/mach/map.h b/arch/arm/mach-s5pv310/include/mach/map.h
index 7acf4e7..9905bfe 100644
--- a/arch/arm/mach-s5pv310/include/mach/map.h
+++ b/arch/arm/mach-s5pv310/include/mach/map.h
@@ -56,6 +56,9 @@
 #define S5PV310_PA_GPIO2		(0x11000000)
 #define S5PV310_PA_GPIO3		(0x03860000)
 
+#define S5PV310_PA_CSIS0		(0x11880000)
+#define S5PV310_PA_CSIS1		(0x11890000)
+
 #define S5PV310_PA_HSMMC(x)		(0x12510000 + ((x) * 0x10000))
 
 #define S5PV310_PA_SROMC		(0x12570000)
@@ -95,5 +98,7 @@
 #define S3C_PA_IIC7			S5PV310_PA_IIC(7)
 #define S3C_PA_RTC			S5PV310_PA_RTC
 #define S3C_PA_WDT			S5PV310_PA_WATCHDOG
+#define S5P_PA_CSIS0			S5PV310_PA_CSIS0
+#define S5P_PA_CSIS1			S5PV310_PA_CSIS1
 
 #endif /* __ASM_ARCH_MAP_H */
-- 
1.7.1

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Sylwester Nawrocki]

There may be up to 2 MIPI-CSI2 interfaces depending on SoC version.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/plat-s5p/Kconfig                 |   10 ++++++++
 arch/arm/plat-s5p/Makefile                |    2 +
 arch/arm/plat-s5p/dev-csis0.c             |   34 +++++++++++++++++++++++++++++
 arch/arm/plat-s5p/dev-csis1.c             |   33 ++++++++++++++++++++++++++++
 arch/arm/plat-samsung/include/plat/csis.h |   28 +++++++++++++++++++++++
 arch/arm/plat-samsung/include/plat/devs.h |    3 ++
 6 files changed, 110 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-s5p/dev-csis0.c
 create mode 100644 arch/arm/plat-s5p/dev-csis1.c
 create mode 100644 arch/arm/plat-samsung/include/plat/csis.h

diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
index 65dbfa8..2fb0c88 100644
--- a/arch/arm/plat-s5p/Kconfig
+++ b/arch/arm/plat-s5p/Kconfig
@@ -56,3 +56,13 @@ config S5P_DEV_ONENAND
 	bool
 	help
 	  Compile in platform device definition for OneNAND controller
+
+config S5P_DEV_CSIS0
+	bool
+	help
+	  Compile in platform device definitions for MIPI-CSI2 interface 0
+
+config S5P_DEV_CSIS1
+	bool
+	help
+	  Compile in platform device definitions for MIPI-CSI2 interface 1
diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
index de65238..2b73173 100644
--- a/arch/arm/plat-s5p/Makefile
+++ b/arch/arm/plat-s5p/Makefile
@@ -28,3 +28,5 @@ obj-$(CONFIG_S5P_DEV_FIMC0)	+= dev-fimc0.o
 obj-$(CONFIG_S5P_DEV_FIMC1)	+= dev-fimc1.o
 obj-$(CONFIG_S5P_DEV_FIMC2)	+= dev-fimc2.o
 obj-$(CONFIG_S5P_DEV_ONENAND)	+= dev-onenand.o
+obj-$(CONFIG_S5P_DEV_CSIS0)	+= dev-csis0.o
+obj-$(CONFIG_S5P_DEV_CSIS1)	+= dev-csis1.o
diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
new file mode 100644
index 0000000..2b1ba43
--- /dev/null
+++ b/arch/arm/plat-s5p/dev-csis0.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * S5P series device definition for MIPI-CSI device 0
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <mach/map.h>
+
+static struct resource s5p_csis_resource[] = {
+	[0] = {
+		.start = S5P_PA_CSIS0,
+		.end   = S5P_PA_CSIS0 + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = IRQ_MIPICSI0,
+		.end   = IRQ_MIPICSI0,
+		.flags = IORESOURCE_IRQ,
+	}
+};
+
+struct platform_device s5p_device_mipi_csis0 = {
+	.name		  = "s5p-mipi-csis",
+	.id		  = 0,
+	.num_resources	  = 2,
+	.resource	  = &s5p_csis_resource[0],
+};
diff --git a/arch/arm/plat-s5p/dev-csis1.c b/arch/arm/plat-s5p/dev-csis1.c
new file mode 100644
index 0000000..7f21ea8
--- /dev/null
+++ b/arch/arm/plat-s5p/dev-csis1.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * S5P series device definition for MIPI-CSI device 1
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <mach/map.h>
+
+static struct resource s5p_csis_resource[] = {
+	[0] = {
+		.start = S5P_PA_CSIS1,
+		.end   = S5P_PA_CSIS1 + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = IRQ_MIPICSI1,
+		.end   = IRQ_MIPICSI1,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+struct platform_device s5p_device_mipi_csis1 = {
+	.name		  = "s5p-mipi-csis",
+	.id		  = 1,
+	.num_resources	  = 2,
+	.resource	  = &s5p_csis_resource[0],
+};
diff --git a/arch/arm/plat-samsung/include/plat/csis.h b/arch/arm/plat-samsung/include/plat/csis.h
new file mode 100644
index 0000000..6ea3a40
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/csis.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010 Samsung Electronics Co., Ltd
+ *
+ * S5P series MIPI CSI2 device support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef PLAT_CSIS_H_
+#define PLAT_CSIS_H_ __FILE__
+
+/**
+ * struct s3c_platform_csis - default platform data for MIPI CSI2 interface
+ * @clock_rate: bus clock frequency
+ * @nlanes: number of MIPI data lanes used
+ * @alignment: MIPI data alignment in bits
+ * @hs_settle: HSYNC settle time (see CSIS_DPHYCTRL register description)
+ */
+struct s3c_platform_csis {
+	unsigned long clk_rate;
+	u8 nlanes;
+	u8 alignment;
+	u8 hs_settle;
+};
+
+#endif /* PLAT_CSIS_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
index 2d82a6c..7935a8b 100644
--- a/arch/arm/plat-samsung/include/plat/devs.h
+++ b/arch/arm/plat-samsung/include/plat/devs.h
@@ -122,6 +122,9 @@ extern struct platform_device s5p_device_fimc0;
 extern struct platform_device s5p_device_fimc1;
 extern struct platform_device s5p_device_fimc2;
 
+extern struct platform_device s5p_device_mipi_csis0;
+extern struct platform_device s5p_device_mipi_csis1;
+
 /* s3c2440 specific devices */
 
 #ifdef CONFIG_CPU_S3C2440
-- 
1.7.1

[PATCH 4/4] mach-s5pv210: Add MIPI-CSI DPHY clock definition

[Sylwester Nawrocki]

MIPI DPHY control register requires special handling since
it is shared between CSI (camera serial interface) and DSI
(display serial interface). By creating this clock
a serialized interface is provided for mipi-csi and mipi-dsi
drivers, so DPHYs may be safely controlled by both drivers.
Similarly dsim_dphy clock could be added for mipi-dsim.

---

I am not quite sure about "csis_dphy" clock, perhaps power domain
handling code would better place for MIPI DPHY control.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/clock.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index 019c3a6..14e42ca 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -185,6 +185,19 @@ static int s5pv210_clk_mask1_ctrl(struct clk *clk, int enable)
 	return s5p_gatectrl(S5P_CLK_SRC_MASK1, clk, enable);
 }
 
+static int s5pv210_clk_mipi_dphy_ctrl(struct clk *clk, int enable)
+{
+	return s5p_gatectrl(S5P_MIPI_DPHY_CONTROL, clk, enable);
+}
+
+/* The common parent clock for mipi-csi/dsi clocks. */
+static struct clk clk_vp_mipi_dphy = {
+	.name		= "vpclk_csis_dphy",
+	.id		= -1,
+	.enable		= s5pv210_clk_mipi_dphy_ctrl,
+	.ctrlbit	= (1 << 0),
+};
+
 static struct clk clk_sclk_hdmi27m = {
 	.name		= "sclk_hdmi27m",
 	.id		= -1,
@@ -323,6 +336,18 @@ static struct clk init_clocks_disable[] = {
 		.enable		= s5pv210_clk_ip0_ctrl,
 		.ctrlbit	= (1 << 4),
 	}, {
+		.name		= "csis",
+		.id		= -1,
+		.parent		= &clk_pclk_dsys.clk,
+		.enable		= s5pv210_clk_ip0_ctrl,
+		.ctrlbit	= (1 << 31),
+	}, {
+		.name		= "csis_dphy",
+		.id		= -1,
+		.parent		= &clk_vp_mipi_dphy,
+		.enable		= s5pv210_clk_mipi_dphy_ctrl,
+		.ctrlbit	= (1 << 1),
+	}, {
 		.name		= "rot",
 		.id		= -1,
 		.parent		= &clk_hclk_dsys.clk,
-- 
1.7.1

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Jamie Iles]

Hi Sylwester,

On Thu, Dec 02, 2010 at 05:37:41PM +0100, Sylwester Nawrocki wrote:
> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
> new file mode 100644
> index 0000000..2b1ba43
> --- /dev/null
> +++ b/arch/arm/plat-s5p/dev-csis0.c
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * S5P series device definition for MIPI-CSI device 0
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <mach/map.h>
> +
> +static struct resource s5p_csis_resource[] = {
> +	[0] = {
> +		.start = S5P_PA_CSIS0,
> +		.end   = S5P_PA_CSIS0 + SZ_4K - 1,
> +		.flags = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start = IRQ_MIPICSI0,
> +		.end   = IRQ_MIPICSI0,
> +		.flags = IORESOURCE_IRQ,
> +	}
> +};
Do you really need the [0] and [1] here? These are only needed if you are 
sparsely initialising an array.

> +
> +struct platform_device s5p_device_mipi_csis0 = {
> +	.name		  = "s5p-mipi-csis",
> +	.id		  = 0,
> +	.num_resources	  = 2,
.num_resources = ARRAY_SIZE(s5p_csis_resource),
> +	.resource	  = &s5p_csis_resource[0],
.resource = s5p_csis_resource,
> diff --git a/arch/arm/plat-s5p/dev-csis1.c b/arch/arm/plat-s5p/dev-csis1.c
> new file mode 100644
> index 0000000..7f21ea8
> --- /dev/null
> +++ b/arch/arm/plat-s5p/dev-csis1.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * S5P series device definition for MIPI-CSI device 1
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <mach/map.h>
> +
> +static struct resource s5p_csis_resource[] = {
> +	[0] = {
> +		.start = S5P_PA_CSIS1,
> +		.end   = S5P_PA_CSIS1 + SZ_4K - 1,
> +		.flags = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start = IRQ_MIPICSI1,
> +		.end   = IRQ_MIPICSI1,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +struct platform_device s5p_device_mipi_csis1 = {
> +	.name		  = "s5p-mipi-csis",
> +	.id		  = 1,
> +	.num_resources	  = 2,
> +	.resource	  = &s5p_csis_resource[0],
> +};
The same goes here as for device 0.
> diff --git a/arch/arm/plat-samsung/include/plat/csis.h b/arch/arm/plat-samsung/include/plat/csis.h
> new file mode 100644
> index 0000000..6ea3a40
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/csis.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics Co., Ltd
> + *
> + * S5P series MIPI CSI2 device support
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef PLAT_CSIS_H_
> +#define PLAT_CSIS_H_ __FILE__
> +
> +/**
> + * struct s3c_platform_csis - default platform data for MIPI CSI2 interface
> + * @clock_rate: bus clock frequency
Should be @clk_rate to match the code below.
> + * @nlanes: number of MIPI data lanes used
> + * @alignment: MIPI data alignment in bits
> + * @hs_settle: HSYNC settle time (see CSIS_DPHYCTRL register description)
> + */
> +struct s3c_platform_csis {
> +	unsigned long clk_rate;
> +	u8 nlanes;
> +	u8 alignment;
> +	u8 hs_settle;
> +};
> +
> +#endif /* PLAT_CSIS_H_ */

Jamie

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Sylwester Nawrocki]

Hi Jamie,

On 12/02/2010 06:15 PM, Jamie Iles wrote:
> Hi Sylwester,
> 
> On Thu, Dec 02, 2010 at 05:37:41PM +0100, Sylwester Nawrocki wrote:
>> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
>> new file mode 100644
>> index 0000000..2b1ba43
>> --- /dev/null
>> +++ b/arch/arm/plat-s5p/dev-csis0.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright (C) 2010 Samsung Electronics
>> + *
>> + * S5P series device definition for MIPI-CSI device 0
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <mach/map.h>
>> +
>> +static struct resource s5p_csis_resource[] = {
>> +	[0] = {
>> +		.start = S5P_PA_CSIS0,
>> +		.end   = S5P_PA_CSIS0 + SZ_4K - 1,
>> +		.flags = IORESOURCE_MEM,
>> +	},
>> +	[1] = {
>> +		.start = IRQ_MIPICSI0,
>> +		.end   = IRQ_MIPICSI0,
>> +		.flags = IORESOURCE_IRQ,
>> +	}
>> +};
> Do you really need the [0] and [1] here? These are only needed if you are 
> sparsely initialising an array.

I agree explicit indices are not really needed. I just followed the style of
all other resource definitions already there.
I am not quite sure what is the preference of the maintainer, I guess they want
all devices to follow same style for consistence.

Thanks,
Sylwester

-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Jamie Iles]

On Thu, Dec 02, 2010 at 06:39:50PM +0100, Sylwester Nawrocki wrote:
> On 12/02/2010 06:15 PM, Jamie Iles wrote:
> > On Thu, Dec 02, 2010 at 05:37:41PM +0100, Sylwester Nawrocki wrote:
[...]
> >> +static struct resource s5p_csis_resource[] = {
> >> +	[0] = {
> >> +		.start = S5P_PA_CSIS0,
> >> +		.end   = S5P_PA_CSIS0 + SZ_4K - 1,
> >> +		.flags = IORESOURCE_MEM,
> >> +	},
> >> +	[1] = {
> >> +		.start = IRQ_MIPICSI0,
> >> +		.end   = IRQ_MIPICSI0,
> >> +		.flags = IORESOURCE_IRQ,
> >> +	}
> >> +};
> > Do you really need the [0] and [1] here? These are only needed if you are 
> > sparsely initialising an array.
> 
> I agree explicit indices are not really needed. I just followed the style of
> all other resource definitions already there.
> I am not quite sure what is the preference of the maintainer, I guess they want
> all devices to follow same style for consistence.
Ok, that's fair enough, it just looked a bit odd to me!

Jamie

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Jassi Brar]

On Fri, Dec 3, 2010 at 2:39 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> Hi Jamie,
>
> On 12/02/2010 06:15 PM, Jamie Iles wrote:
>> Hi Sylwester,
>>
>> On Thu, Dec 02, 2010 at 05:37:41PM +0100, Sylwester Nawrocki wrote:
>>> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
>>> new file mode 100644
>>> index 0000000..2b1ba43
>>> --- /dev/null
>>> +++ b/arch/arm/plat-s5p/dev-csis0.c
>>> @@ -0,0 +1,34 @@
>>> +/*
>>> + * Copyright (C) 2010 Samsung Electronics
>>> + *
>>> + * S5P series device definition for MIPI-CSI device 0
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> +*/
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/platform_device.h>
>>> +#include <mach/map.h>
>>> +
>>> +static struct resource s5p_csis_resource[] = {
>>> + ? ?[0] = {
>>> + ? ? ? ? ? ?.start = S5P_PA_CSIS0,
>>> + ? ? ? ? ? ?.end ? = S5P_PA_CSIS0 + SZ_4K - 1,
>>> + ? ? ? ? ? ?.flags = IORESOURCE_MEM,
>>> + ? ?},
>>> + ? ?[1] = {
>>> + ? ? ? ? ? ?.start = IRQ_MIPICSI0,
>>> + ? ? ? ? ? ?.end ? = IRQ_MIPICSI0,
>>> + ? ? ? ? ? ?.flags = IORESOURCE_IRQ,
>>> + ? ?}
>>> +};
>> Do you really need the [0] and [1] here? These are only needed if you are
>> sparsely initialising an array.
>
> I agree explicit indices are not really needed. I just followed the style of
> all other resource definitions already there.
> I am not quite sure what is the preference of the maintainer, I guess they want
> all devices to follow same style for consistence.

And that style has its origin because explicit indices are as because
they stress the fact that
drivers expect  a particular resource at a particular index (when we
have more than 1
resource of a type).
IMO we should use some macro shared between platform devices and their drivers.
Something like -
static struct resource s5p_csis_resource[] = {
       [PA_CSIS] = {
               .start = S5P_PA_CSIS0,
               .end   = S5P_PA_CSIS0 + SZ_4K - 1,
               .flags = IORESOURCE_MEM,
       },
       [IRQ_CSIS] = {
               .start = IRQ_MIPICSI0,
               .end   = IRQ_MIPICSI0,
               .flags = IORESOURCE_IRQ,
       }
};

[PATCH 0/4] ARM: S5P: Add platform support for MIPI-CSI2 devices

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Hello,
> 
Hi :-)

> the following patch series adds S5P platform support for MIPI-CSI2
devices.
> It has been rebased onto next-samsung branch at:
> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git

It would be more helpful to me to use the 'for-next' next time.
> 
> [PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis
> [PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis
> [PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices
> [PATCH 4/4] mach-s5pv210: Add MIPI-CSI DPHY clock definition
> 

And used following commit title in others...

ARM: S5PV210: xxx
ARM: S5P: xxx

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Added resource definitions for mipi-csis interface, naming
> changed for consistency with s5pv310 where there are two
> instances of the device.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-s5pv210/include/mach/irqs.h       |    2 +-
>  arch/arm/mach-s5pv210/include/mach/map.h        |    4 ++++
>  arch/arm/mach-s5pv210/include/mach/regs-clock.h |    5 +----
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-
> s5pv210/include/mach/irqs.h
> index 119b95f..8b5994c 100644
> --- a/arch/arm/mach-s5pv210/include/mach/irqs.h
> +++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
> @@ -65,7 +65,7 @@
>  #define IRQ_HSMMC0		S5P_IRQ_VIC1(26)
>  #define IRQ_HSMMC1		S5P_IRQ_VIC1(27)
>  #define IRQ_HSMMC2		S5P_IRQ_VIC1(28)
> -#define IRQ_MIPICSI		S5P_IRQ_VIC1(29)
> +#define IRQ_MIPICSI0		S5P_IRQ_VIC1(29)

Firstly don't use directly IRQ_MIPICSI0 which is used in platform device.
Because it can be confused that there would be another MIPICSI such as
MIPICSI1.
In that case, just use like following. For example,

---
@@ -132,5 +132,6 @@
 #define IRQ_LCD_FIFO           IRQ_LCD0
 #define IRQ_LCD_VSYNC          IRQ_LCD1
 #define IRQ_LCD_SYSTEM         IRQ_LCD2
+#define IRQ_MIPI_CSIS0         IRQ_MIPICSI

 #endif /* ASM_ARCH_IRQS_H */
---

And I'm confused about the name of MIPI-CSI...
Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.

Maybe the meaning is Camera Serial Interface Slave...
So...how about to use just one such as "IRQ_MIPI_CSIS" and
"S5PV210_PA_MIPI_CSIS".

---
@@ -65,7 +65,7 @@
 #define IRQ_HSMMC0             S5P_IRQ_VIC1(26)
 #define IRQ_HSMMC1             S5P_IRQ_VIC1(27)
 #define IRQ_HSMMC2             S5P_IRQ_VIC1(28)
-#define IRQ_MIPICSI            S5P_IRQ_VIC1(29)
+#define IRQ_MIPI_CSIS          S5P_IRQ_VIC1(29)
 #define IRQ_MIPIDSI            S5P_IRQ_VIC1(30)
 #define IRQ_ONENAND_AUDI       S5P_IRQ_VIC1(31)

@@ -132,5 +132,6 @@
 #define IRQ_LCD_FIFO           IRQ_LCD0
 #define IRQ_LCD_VSYNC          IRQ_LCD1
 #define IRQ_LCD_SYSTEM         IRQ_LCD2
+#define IRQ_MIPI_CSIS0         IRQ_MIPI_CSIS

 #endif /* ASM_ARCH_IRQS_H */
---

>  #define IRQ_MIPIDSI		S5P_IRQ_VIC1(30)
>  #define IRQ_ONENAND_AUDI	S5P_IRQ_VIC1(31)
> 
> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
> s5pv210/include/mach/map.h
> index 861d7fe..8f376f1 100644
> --- a/arch/arm/mach-s5pv210/include/mach/map.h
> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
> @@ -107,6 +107,9 @@
>  #define S5PV210_PA_DMC0		(0xF0000000)
>  #define S5PV210_PA_DMC1		(0xF1400000)
> 
> +/* MIPI CSI */

No need comment.

> +#define S5PV210_PA_CSIS		0xFA600000
> +
>  /* compatibiltiy defines. */
>  #define S3C_PA_UART		S5PV210_PA_UART
>  #define S3C_PA_HSMMC0		S5PV210_PA_HSMMC(0)
> @@ -123,6 +126,7 @@
>  #define S5P_PA_FIMC0		S5PV210_PA_FIMC0
>  #define S5P_PA_FIMC1		S5PV210_PA_FIMC1
>  #define S5P_PA_FIMC2		S5PV210_PA_FIMC2
> +#define S5P_PA_CSIS0		S5PV210_PA_CSIS
> 
Same as above.

>  #define SAMSUNG_PA_ADC		S5PV210_PA_ADC
>  #define SAMSUNG_PA_CFCON	S5PV210_PA_CFCON
> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
b/arch/arm/mach-
> s5pv210/include/mach/regs-clock.h
> index ebaabe0..4c45b74 100644
> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> @@ -161,7 +161,7 @@
>  #define S5P_MDNIE_SEL		S5P_CLKREG(0x7008)
>  #define S5P_MIPI_PHY_CON0	S5P_CLKREG(0x7200)
>  #define S5P_MIPI_PHY_CON1	S5P_CLKREG(0x7204)
> -#define S5P_MIPI_CONTROL	S5P_CLKREG(0xE814)
> +#define S5P_MIPI_DPHY_CONTROL	S5P_CLKREG(0xE814)
> 
>  #define S5P_IDLE_CFG_TL_MASK	(3 << 30)
>  #define S5P_IDLE_CFG_TM_MASK	(3 << 28)
> @@ -195,9 +195,6 @@
>  #define S5P_OTHERS_RET_UART		(1 << 28)
>  #define S5P_OTHERS_USB_SIG_MASK		(1 << 16)
> 
> -/* MIPI */
> -#define S5P_MIPI_DPHY_EN		(3)
> -

Great.

>  /* S5P_DAC_CONTROL */
>  #define S5P_DAC_ENABLE			(1)
>  #define S5P_DAC_DISABLE			(0)
> --


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Add IRQ and register base address definitions for mipi-csis devices.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-s5pv310/include/mach/irqs.h |    3 +++
>  arch/arm/mach-s5pv310/include/mach/map.h  |    5 +++++
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv310/include/mach/irqs.h b/arch/arm/mach-
> s5pv310/include/mach/irqs.h
> index 99e7dad..db63773 100644
> --- a/arch/arm/mach-s5pv310/include/mach/irqs.h
> +++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
> @@ -83,6 +83,9 @@
>  #define IRQ_HSMMC2		COMBINER_IRQ(29, 2)
>  #define IRQ_HSMMC3		COMBINER_IRQ(29, 3)
> 
> +#define IRQ_MIPICSI0		COMBINER_IRQ(30, 0)
> +#define IRQ_MIPICSI1		COMBINER_IRQ(30, 1)
> +
>  #define IRQ_ONENAND_AUDI	COMBINER_IRQ(34, 0)
> 
>  #define IRQ_EINT4		COMBINER_IRQ(37, 0)
> diff --git a/arch/arm/mach-s5pv310/include/mach/map.h b/arch/arm/mach-
> s5pv310/include/mach/map.h
> index 7acf4e7..9905bfe 100644
> --- a/arch/arm/mach-s5pv310/include/mach/map.h
> +++ b/arch/arm/mach-s5pv310/include/mach/map.h
> @@ -56,6 +56,9 @@
>  #define S5PV310_PA_GPIO2		(0x11000000)
>  #define S5PV310_PA_GPIO3		(0x03860000)
> 
> +#define S5PV310_PA_CSIS0		(0x11880000)
> +#define S5PV310_PA_CSIS1		(0x11890000)
> +
>  #define S5PV310_PA_HSMMC(x)		(0x12510000 + ((x) * 0x10000))
> 
>  #define S5PV310_PA_SROMC		(0x12570000)
> @@ -95,5 +98,7 @@
>  #define S3C_PA_IIC7			S5PV310_PA_IIC(7)
>  #define S3C_PA_RTC			S5PV310_PA_RTC
>  #define S3C_PA_WDT			S5PV310_PA_WATCHDOG
> +#define S5P_PA_CSIS0			S5PV310_PA_CSIS0
> +#define S5P_PA_CSIS1			S5PV310_PA_CSIS1
> 
>  #endif /* __ASM_ARCH_MAP_H */
> --

Please refer to previous my comment about the naming.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> There may be up to 2 MIPI-CSI2 interfaces depending on SoC version.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/plat-s5p/Kconfig                 |   10 ++++++++
>  arch/arm/plat-s5p/Makefile                |    2 +
>  arch/arm/plat-s5p/dev-csis0.c             |   34
> +++++++++++++++++++++++++++++
>  arch/arm/plat-s5p/dev-csis1.c             |   33
> ++++++++++++++++++++++++++++
>  arch/arm/plat-samsung/include/plat/csis.h |   28 +++++++++++++++++++++++
>  arch/arm/plat-samsung/include/plat/devs.h |    3 ++
>  6 files changed, 110 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/plat-s5p/dev-csis0.c
>  create mode 100644 arch/arm/plat-s5p/dev-csis1.c
>  create mode 100644 arch/arm/plat-samsung/include/plat/csis.h
> 
> diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
> index 65dbfa8..2fb0c88 100644
> --- a/arch/arm/plat-s5p/Kconfig
> +++ b/arch/arm/plat-s5p/Kconfig
> @@ -56,3 +56,13 @@ config S5P_DEV_ONENAND
>  	bool
>  	help
>  	  Compile in platform device definition for OneNAND controller
> +
> +config S5P_DEV_CSIS0
> +	bool
> +	help
> +	  Compile in platform device definitions for MIPI-CSI2 interface 0

MIPI-CSI2...you mean MIPI-CSI version 2. We _really_ need to separate the
version here?
How about just for MIPI-CSIS channel 0 ?

> +
> +config S5P_DEV_CSIS1
> +	bool
> +	help
> +	  Compile in platform device definitions for MIPI-CSI2 interface 1

MIPI-CSIS channel 1 ?

> diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
> index de65238..2b73173 100644
> --- a/arch/arm/plat-s5p/Makefile
> +++ b/arch/arm/plat-s5p/Makefile
> @@ -28,3 +28,5 @@ obj-$(CONFIG_S5P_DEV_FIMC0)	+= dev-fimc0.o
>  obj-$(CONFIG_S5P_DEV_FIMC1)	+= dev-fimc1.o
>  obj-$(CONFIG_S5P_DEV_FIMC2)	+= dev-fimc2.o
>  obj-$(CONFIG_S5P_DEV_ONENAND)	+= dev-onenand.o
> +obj-$(CONFIG_S5P_DEV_CSIS0)	+= dev-csis0.o
> +obj-$(CONFIG_S5P_DEV_CSIS1)	+= dev-csis1.o
> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
> new file mode 100644
> index 0000000..2b1ba43
> --- /dev/null
> +++ b/arch/arm/plat-s5p/dev-csis0.c
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * S5P series device definition for MIPI-CSI device 0

MIPI-CSIS channel 0

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <mach/map.h>
> +
> +static struct resource s5p_csis_resource[] = {

Hmm...s5p_mipi_csis0_resource is more helpful even though static.

> +	[0] = {
> +		.start = S5P_PA_CSIS0,
> +		.end   = S5P_PA_CSIS0 + SZ_4K - 1,

Please refer to previous my comments about the name.

> +		.flags = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start = IRQ_MIPICSI0,
> +		.end   = IRQ_MIPICSI0,

Same.

> +		.flags = IORESOURCE_IRQ,
> +	}
> +};
> +
> +struct platform_device s5p_device_mipi_csis0 = {
> +	.name		  = "s5p-mipi-csis",
> +	.id		  = 0,
> +	.num_resources	  = 2,

= ARRAY_SIZE(s5p_mipi_csis0_resource),

> +	.resource	  = &s5p_csis_resource[0],

= s5p_mipi_csis0_resource,

> +};
> diff --git a/arch/arm/plat-s5p/dev-csis1.c b/arch/arm/plat-s5p/dev-csis1.c
> new file mode 100644
> index 0000000..7f21ea8
> --- /dev/null
> +++ b/arch/arm/plat-s5p/dev-csis1.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * S5P series device definition for MIPI-CSI device 1

MIPI-CSIS channel 1

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <mach/map.h>
> +
> +static struct resource s5p_csis_resource[] = {
> +	[0] = {
> +		.start = S5P_PA_CSIS1,
> +		.end   = S5P_PA_CSIS1 + SZ_4K - 1,

Name.

> +		.flags = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start = IRQ_MIPICSI1,
> +		.end   = IRQ_MIPICSI1,

Name.

> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +struct platform_device s5p_device_mipi_csis1 = {
> +	.name		  = "s5p-mipi-csis",
> +	.id		  = 1,
> +	.num_resources	  = 2,

Same as above.

> +	.resource	  = &s5p_csis_resource[0],

Same.

> +};
> diff --git a/arch/arm/plat-samsung/include/plat/csis.h b/arch/arm/plat-
> samsung/include/plat/csis.h
> new file mode 100644
> index 0000000..6ea3a40
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/csis.h

Do you think really needed this for "plat-samsung" not just "plat-s5p"?

> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2010 Samsung Electronics Co., Ltd
> + *
> + * S5P series MIPI CSI2 device support
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef PLAT_CSIS_H_
> +#define PLAT_CSIS_H_ __FILE__
> +
> +/**
> + * struct s3c_platform_csis - default platform data for MIPI CSI2
interface

...

> + * @clock_rate: bus clock frequency

clk_rate as Jamie Iles's comment.

> + * @nlanes: number of MIPI data lanes used

How about just "lanes".

> + * @alignment: MIPI data alignment in bits
> + * @hs_settle: HSYNC settle time (see CSIS_DPHYCTRL register description)

Actually, not mentioned HS-RX settle time in CSIS_DPHYCTRL register of
datasheet.

+ * @hs_settle: HS-RX settle time.

> + */
> +struct s3c_platform_csis {

s5p_platform_mipi_csis

> +	unsigned long clk_rate;
> +	u8 nlanes;
> +	u8 alignment;
> +	u8 hs_settle;
> +};
> +
> +#endif /* PLAT_CSIS_H_ */
> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-
> samsung/include/plat/devs.h
> index 2d82a6c..7935a8b 100644
> --- a/arch/arm/plat-samsung/include/plat/devs.h
> +++ b/arch/arm/plat-samsung/include/plat/devs.h
> @@ -122,6 +122,9 @@ extern struct platform_device s5p_device_fimc0;
>  extern struct platform_device s5p_device_fimc1;
>  extern struct platform_device s5p_device_fimc2;
> 
> +extern struct platform_device s5p_device_mipi_csis0;
> +extern struct platform_device s5p_device_mipi_csis1;
> +
>  /* s3c2440 specific devices */
> 
>  #ifdef CONFIG_CPU_S3C2440
> --


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Jassi Brar]

On Fri, Dec 3, 2010 at 1:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Sylwester Nawrocki wrote:
>>
>> Add IRQ and register base address definitions for mipi-csis devices.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> ?arch/arm/mach-s5pv310/include/mach/irqs.h | ? ?3 +++
>> ?arch/arm/mach-s5pv310/include/mach/map.h ?| ? ?5 +++++
>> ?2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv310/include/mach/irqs.h b/arch/arm/mach-
>> s5pv310/include/mach/irqs.h
>> index 99e7dad..db63773 100644
>> --- a/arch/arm/mach-s5pv310/include/mach/irqs.h
>> +++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
>> @@ -83,6 +83,9 @@
>> ?#define IRQ_HSMMC2 ? ? ? ? ? COMBINER_IRQ(29, 2)
>> ?#define IRQ_HSMMC3 ? ? ? ? ? COMBINER_IRQ(29, 3)
>>
>> +#define IRQ_MIPICSI0 ? ? ? ? COMBINER_IRQ(30, 0)
>> +#define IRQ_MIPICSI1 ? ? ? ? COMBINER_IRQ(30, 1)
>> +
>> ?#define IRQ_ONENAND_AUDI ? ? COMBINER_IRQ(34, 0)
>>
>> ?#define IRQ_EINT4 ? ? ? ? ? ?COMBINER_IRQ(37, 0)
>> diff --git a/arch/arm/mach-s5pv310/include/mach/map.h b/arch/arm/mach-
>> s5pv310/include/mach/map.h
>> index 7acf4e7..9905bfe 100644
>> --- a/arch/arm/mach-s5pv310/include/mach/map.h
>> +++ b/arch/arm/mach-s5pv310/include/mach/map.h
>> @@ -56,6 +56,9 @@
>> ?#define S5PV310_PA_GPIO2 ? ? ? ? ? ? (0x11000000)
>> ?#define S5PV310_PA_GPIO3 ? ? ? ? ? ? (0x03860000)
>>
>> +#define S5PV310_PA_CSIS0 ? ? ? ? ? ? (0x11880000)
>> +#define S5PV310_PA_CSIS1 ? ? ? ? ? ? (0x11890000)

Let us please not contain constants in brackets.

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Jassi Brar]

On Fri, Dec 3, 2010 at 1:37 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> There may be up to 2 MIPI-CSI2 interfaces depending on SoC version.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> ?arch/arm/plat-s5p/Kconfig ? ? ? ? ? ? ? ? | ? 10 ++++++++
> ?arch/arm/plat-s5p/Makefile ? ? ? ? ? ? ? ?| ? ?2 +
> ?arch/arm/plat-s5p/dev-csis0.c ? ? ? ? ? ? | ? 34 +++++++++++++++++++++++++++++
> ?arch/arm/plat-s5p/dev-csis1.c ? ? ? ? ? ? | ? 33 ++++++++++++++++++++++++++++
> ?arch/arm/plat-samsung/include/plat/csis.h | ? 28 +++++++++++++++++++++++
> ?arch/arm/plat-samsung/include/plat/devs.h | ? ?3 ++
> ?6 files changed, 110 insertions(+), 0 deletions(-)
> ?create mode 100644 arch/arm/plat-s5p/dev-csis0.c
> ?create mode 100644 arch/arm/plat-s5p/dev-csis1.c
> ?create mode 100644 arch/arm/plat-samsung/include/plat/csis.h
>
> diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
> index 65dbfa8..2fb0c88 100644
> --- a/arch/arm/plat-s5p/Kconfig
> +++ b/arch/arm/plat-s5p/Kconfig
> @@ -56,3 +56,13 @@ config S5P_DEV_ONENAND
> ? ? ? ?bool
> ? ? ? ?help
> ? ? ? ? ?Compile in platform device definition for OneNAND controller
> +
> +config S5P_DEV_CSIS0
> + ? ? ? bool
> + ? ? ? help
> + ? ? ? ? Compile in platform device definitions for MIPI-CSI2 interface 0
> +
> +config S5P_DEV_CSIS1
> + ? ? ? bool
> + ? ? ? help
> + ? ? ? ? Compile in platform device definitions for MIPI-CSI2 interface 1
> diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
> index de65238..2b73173 100644
> --- a/arch/arm/plat-s5p/Makefile
> +++ b/arch/arm/plat-s5p/Makefile
> @@ -28,3 +28,5 @@ obj-$(CONFIG_S5P_DEV_FIMC0) ? += dev-fimc0.o
> ?obj-$(CONFIG_S5P_DEV_FIMC1) ? ?+= dev-fimc1.o
> ?obj-$(CONFIG_S5P_DEV_FIMC2) ? ?+= dev-fimc2.o
> ?obj-$(CONFIG_S5P_DEV_ONENAND) ?+= dev-onenand.o
> +obj-$(CONFIG_S5P_DEV_CSIS0) ? ?+= dev-csis0.o
> +obj-$(CONFIG_S5P_DEV_CSIS1) ? ?+= dev-csis1.o

When we do have defines for each device, why not have both in one file
with each contained in ifdef for it's define ? That will reduce file count.

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Kyungmin Park]

On Fri, Dec 3, 2010 at 1:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Sylwester Nawrocki wrote:
>>
>> Added resource definitions for mipi-csis interface, naming
>> changed for consistency with s5pv310 where there are two
>> instances of the device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> ?arch/arm/mach-s5pv210/include/mach/irqs.h ? ? ? | ? ?2 +-
>> ?arch/arm/mach-s5pv210/include/mach/map.h ? ? ? ?| ? ?4 ++++
>> ?arch/arm/mach-s5pv210/include/mach/regs-clock.h | ? ?5 +----
>> ?3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-
>> s5pv210/include/mach/irqs.h
>> index 119b95f..8b5994c 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/irqs.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
>> @@ -65,7 +65,7 @@
>> ?#define IRQ_HSMMC0 ? ? ? ? ? S5P_IRQ_VIC1(26)
>> ?#define IRQ_HSMMC1 ? ? ? ? ? S5P_IRQ_VIC1(27)
>> ?#define IRQ_HSMMC2 ? ? ? ? ? S5P_IRQ_VIC1(28)
>> -#define IRQ_MIPICSI ? ? ? ? ?S5P_IRQ_VIC1(29)
>> +#define IRQ_MIPICSI0 ? ? ? ? S5P_IRQ_VIC1(29)
>
> Firstly don't use directly IRQ_MIPICSI0 which is used in platform device.
> Because it can be confused that there would be another MIPICSI such as
> MIPICSI1.
> In that case, just use like following. For example,
Hi,

First respect others work.
my impression is that you forced the developers to work as you want.

okay who summit this code? If you think name is odd, then you check it
before you submitted.
When you commit the code is okay, but other should be considered as yours.

>
> ---
> @@ -132,5 +132,6 @@
> ?#define IRQ_LCD_FIFO ? ? ? ? ? IRQ_LCD0
> ?#define IRQ_LCD_VSYNC ? ? ? ? ?IRQ_LCD1
> ?#define IRQ_LCD_SYSTEM ? ? ? ? IRQ_LCD2
> +#define IRQ_MIPI_CSIS0 ? ? ? ? IRQ_MIPICSI
>
> ?#endif /* ASM_ARCH_IRQS_H */
> ---
>
> And I'm confused about the name of MIPI-CSI...
> Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.
>
> Maybe the meaning is Camera Serial Interface Slave...
> So...how about to use just one such as "IRQ_MIPI_CSIS" and
> "S5PV210_PA_MIPI_CSIS".

I think it's better use the common word, CSI. I can't find CSIS word
at googling.

>
> ---
> @@ -65,7 +65,7 @@
> ?#define IRQ_HSMMC0 ? ? ? ? ? ? S5P_IRQ_VIC1(26)
> ?#define IRQ_HSMMC1 ? ? ? ? ? ? S5P_IRQ_VIC1(27)
> ?#define IRQ_HSMMC2 ? ? ? ? ? ? S5P_IRQ_VIC1(28)
> -#define IRQ_MIPICSI ? ? ? ? ? ?S5P_IRQ_VIC1(29)
> +#define IRQ_MIPI_CSIS ? ? ? ? ?S5P_IRQ_VIC1(29)
> ?#define IRQ_MIPIDSI ? ? ? ? ? ?S5P_IRQ_VIC1(30)
> ?#define IRQ_ONENAND_AUDI ? ? ? S5P_IRQ_VIC1(31)
>
> @@ -132,5 +132,6 @@
> ?#define IRQ_LCD_FIFO ? ? ? ? ? IRQ_LCD0
> ?#define IRQ_LCD_VSYNC ? ? ? ? ?IRQ_LCD1
> ?#define IRQ_LCD_SYSTEM ? ? ? ? IRQ_LCD2
> +#define IRQ_MIPI_CSIS0 ? ? ? ? IRQ_MIPI_CSIS

In this case, I can't find any good reason since it's first used.
as s3c series used the different names compare to s5p series. it's
okay make these wrapper.
but it's new one and not used previous time.

Thank you,
Kyungmin Park
>
> ?#endif /* ASM_ARCH_IRQS_H */
> ---
>
>> ?#define IRQ_MIPIDSI ? ? ? ? ?S5P_IRQ_VIC1(30)
>> ?#define IRQ_ONENAND_AUDI ? ? S5P_IRQ_VIC1(31)
>>
>> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
>> s5pv210/include/mach/map.h
>> index 861d7fe..8f376f1 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/map.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
>> @@ -107,6 +107,9 @@
>> ?#define S5PV210_PA_DMC0 ? ? ? ? ? ? ?(0xF0000000)
>> ?#define S5PV210_PA_DMC1 ? ? ? ? ? ? ?(0xF1400000)
>>
>> +/* MIPI CSI */
>
> No need comment.
>
>> +#define S5PV210_PA_CSIS ? ? ? ? ? ? ?0xFA600000
>> +
>> ?/* compatibiltiy defines. */
>> ?#define S3C_PA_UART ? ? ? ? ?S5PV210_PA_UART
>> ?#define S3C_PA_HSMMC0 ? ? ? ? ? ? ? ?S5PV210_PA_HSMMC(0)
>> @@ -123,6 +126,7 @@
>> ?#define S5P_PA_FIMC0 ? ? ? ? S5PV210_PA_FIMC0
>> ?#define S5P_PA_FIMC1 ? ? ? ? S5PV210_PA_FIMC1
>> ?#define S5P_PA_FIMC2 ? ? ? ? S5PV210_PA_FIMC2
>> +#define S5P_PA_CSIS0 ? ? ? ? S5PV210_PA_CSIS
>>
> Same as above.
>
>> ?#define SAMSUNG_PA_ADC ? ? ? ? ? ? ? S5PV210_PA_ADC
>> ?#define SAMSUNG_PA_CFCON ? ? S5PV210_PA_CFCON
>> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> b/arch/arm/mach-
>> s5pv210/include/mach/regs-clock.h
>> index ebaabe0..4c45b74 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>> @@ -161,7 +161,7 @@
>> ?#define S5P_MDNIE_SEL ? ? ? ? ? ? ? ?S5P_CLKREG(0x7008)
>> ?#define S5P_MIPI_PHY_CON0 ? ?S5P_CLKREG(0x7200)
>> ?#define S5P_MIPI_PHY_CON1 ? ?S5P_CLKREG(0x7204)
>> -#define S5P_MIPI_CONTROL ? ? S5P_CLKREG(0xE814)
>> +#define S5P_MIPI_DPHY_CONTROL ? ? ? ?S5P_CLKREG(0xE814)
>>
>> ?#define S5P_IDLE_CFG_TL_MASK (3 << 30)
>> ?#define S5P_IDLE_CFG_TM_MASK (3 << 28)
>> @@ -195,9 +195,6 @@
>> ?#define S5P_OTHERS_RET_UART ? ? ? ? ?(1 << 28)
>> ?#define S5P_OTHERS_USB_SIG_MASK ? ? ? ? ? ? ?(1 << 16)
>>
>> -/* MIPI */
>> -#define S5P_MIPI_DPHY_EN ? ? ? ? ? ? (3)
>> -
>
> Great.
>
>> ?/* S5P_DAC_CONTROL */
>> ?#define S5P_DAC_ENABLE ? ? ? ? ? ? ? ? ? ? ? (1)
>> ?#define S5P_DAC_DISABLE ? ? ? ? ? ? ? ? ? ? ?(0)
>> --
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Kyungmin Park]

On Fri, Dec 3, 2010 at 1:59 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Fri, Dec 3, 2010 at 1:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> Sylwester Nawrocki wrote:
>>>
>>> Add IRQ and register base address definitions for mipi-csis devices.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> ?arch/arm/mach-s5pv310/include/mach/irqs.h | ? ?3 +++
>>> ?arch/arm/mach-s5pv310/include/mach/map.h ?| ? ?5 +++++
>>> ?2 files changed, 8 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-s5pv310/include/mach/irqs.h b/arch/arm/mach-
>>> s5pv310/include/mach/irqs.h
>>> index 99e7dad..db63773 100644
>>> --- a/arch/arm/mach-s5pv310/include/mach/irqs.h
>>> +++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
>>> @@ -83,6 +83,9 @@
>>> ?#define IRQ_HSMMC2 ? ? ? ? ? COMBINER_IRQ(29, 2)
>>> ?#define IRQ_HSMMC3 ? ? ? ? ? COMBINER_IRQ(29, 3)
>>>
>>> +#define IRQ_MIPICSI0 ? ? ? ? COMBINER_IRQ(30, 0)
>>> +#define IRQ_MIPICSI1 ? ? ? ? COMBINER_IRQ(30, 1)
>>> +
>>> ?#define IRQ_ONENAND_AUDI ? ? COMBINER_IRQ(34, 0)
>>>
>>> ?#define IRQ_EINT4 ? ? ? ? ? ?COMBINER_IRQ(37, 0)
>>> diff --git a/arch/arm/mach-s5pv310/include/mach/map.h b/arch/arm/mach-
>>> s5pv310/include/mach/map.h
>>> index 7acf4e7..9905bfe 100644
>>> --- a/arch/arm/mach-s5pv310/include/mach/map.h
>>> +++ b/arch/arm/mach-s5pv310/include/mach/map.h
>>> @@ -56,6 +56,9 @@
>>> ?#define S5PV310_PA_GPIO2 ? ? ? ? ? ? (0x11000000)
>>> ?#define S5PV310_PA_GPIO3 ? ? ? ? ? ? (0x03860000)
>>>
>>> +#define S5PV310_PA_CSIS0 ? ? ? ? ? ? (0x11880000)
>>> +#define S5PV310_PA_CSIS1 ? ? ? ? ? ? (0x11890000)
>
> Let us please not contain constants in brackets.

I think consistency is more higher than your comment normally.

if you think these, then make a cleanup first.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Kyungmin Park]

On Fri, Dec 3, 2010 at 2:14 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Fri, Dec 3, 2010 at 1:37 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> There may be up to 2 MIPI-CSI2 interfaces depending on SoC version.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> ?arch/arm/plat-s5p/Kconfig ? ? ? ? ? ? ? ? | ? 10 ++++++++
>> ?arch/arm/plat-s5p/Makefile ? ? ? ? ? ? ? ?| ? ?2 +
>> ?arch/arm/plat-s5p/dev-csis0.c ? ? ? ? ? ? | ? 34 +++++++++++++++++++++++++++++
>> ?arch/arm/plat-s5p/dev-csis1.c ? ? ? ? ? ? | ? 33 ++++++++++++++++++++++++++++
>> ?arch/arm/plat-samsung/include/plat/csis.h | ? 28 +++++++++++++++++++++++
>> ?arch/arm/plat-samsung/include/plat/devs.h | ? ?3 ++
>> ?6 files changed, 110 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/plat-s5p/dev-csis0.c
>> ?create mode 100644 arch/arm/plat-s5p/dev-csis1.c
>> ?create mode 100644 arch/arm/plat-samsung/include/plat/csis.h
>>
>> diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
>> index 65dbfa8..2fb0c88 100644
>> --- a/arch/arm/plat-s5p/Kconfig
>> +++ b/arch/arm/plat-s5p/Kconfig
>> @@ -56,3 +56,13 @@ config S5P_DEV_ONENAND
>> ? ? ? ?bool
>> ? ? ? ?help
>> ? ? ? ? ?Compile in platform device definition for OneNAND controller
>> +
>> +config S5P_DEV_CSIS0
>> + ? ? ? bool
>> + ? ? ? help
>> + ? ? ? ? Compile in platform device definitions for MIPI-CSI2 interface 0
>> +
>> +config S5P_DEV_CSIS1
>> + ? ? ? bool
>> + ? ? ? help
>> + ? ? ? ? Compile in platform device definitions for MIPI-CSI2 interface 1
>> diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
>> index de65238..2b73173 100644
>> --- a/arch/arm/plat-s5p/Makefile
>> +++ b/arch/arm/plat-s5p/Makefile
>> @@ -28,3 +28,5 @@ obj-$(CONFIG_S5P_DEV_FIMC0) ? += dev-fimc0.o
>> ?obj-$(CONFIG_S5P_DEV_FIMC1) ? ?+= dev-fimc1.o
>> ?obj-$(CONFIG_S5P_DEV_FIMC2) ? ?+= dev-fimc2.o
>> ?obj-$(CONFIG_S5P_DEV_ONENAND) ?+= dev-onenand.o
>> +obj-$(CONFIG_S5P_DEV_CSIS0) ? ?+= dev-csis0.o
>> +obj-$(CONFIG_S5P_DEV_CSIS1) ? ?+= dev-csis1.o
>
> When we do have defines for each device, why not have both in one file
> with each contained in ifdef for it's define ? That will reduce file count.

To Jassi,

Please make a conclusion from LSI first, I'm confused, I insisted it
as you, but kgene rejected it.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Jassi Brar]

On Fri, Dec 3, 2010 at 2:34 PM, Kyungmin Park <kyungmin.park@samsung.com> wrote:
> On Fri, Dec 3, 2010 at 1:59 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>> Sylwester Nawrocki wrote:

>>> +#define S5PV310_PA_CSIS0 ? ? ? ? ? ? (0x11880000)
>>> +#define S5PV310_PA_CSIS1 ? ? ? ? ? ? (0x11890000)
>>
>> Let us please not contain constants in brackets.
>
> I think consistency is more higher than your comment normally.
>
> if you think these, then make a cleanup first.

Bad logic. One shouldn't hold good practice at ransom because of
the past bad ones.

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Kukjin Kim]

Kyungmin Park wrote:
> 
> On Fri, Dec 3, 2010 at 1:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Sylwester Nawrocki wrote:
> >>
> >> Added resource definitions for mipi-csis interface, naming
> >> changed for consistency with s5pv310 where there are two
> >> instances of the device.
> >>
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >> ?arch/arm/mach-s5pv210/include/mach/irqs.h ? ? ? | ? ?2 +-
> >> ?arch/arm/mach-s5pv210/include/mach/map.h ? ? ? ?| ? ?4 ++++
> >> ?arch/arm/mach-s5pv210/include/mach/regs-clock.h | ? ?5 +----
> >> ?3 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-
> >> s5pv210/include/mach/irqs.h
> >> index 119b95f..8b5994c 100644
> >> --- a/arch/arm/mach-s5pv210/include/mach/irqs.h
> >> +++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
> >> @@ -65,7 +65,7 @@
> >> ?#define IRQ_HSMMC0 ? ? ? ? ? S5P_IRQ_VIC1(26)
> >> ?#define IRQ_HSMMC1 ? ? ? ? ? S5P_IRQ_VIC1(27)
> >> ?#define IRQ_HSMMC2 ? ? ? ? ? S5P_IRQ_VIC1(28)
> >> -#define IRQ_MIPICSI ? ? ? ? ?S5P_IRQ_VIC1(29)
> >> +#define IRQ_MIPICSI0 ? ? ? ? S5P_IRQ_VIC1(29)
> >
> > Firstly don't use directly IRQ_MIPICSI0 which is used in platform
device.
> > Because it can be confused that there would be another MIPICSI such as
> > MIPICSI1.
> > In that case, just use like following. For example,
> Hi,
> 
> First respect others work.
> my impression is that you forced the developers to work as you want.
> 
> okay who summit this code? If you think name is odd, then you check it
> before you submitted.
> When you commit the code is okay, but other should be considered as yours.
> 

I can't get you :-(

Maybe your talking is to change the name? Why?...
When submitted it, used written name, IRQ_MIPICSI in datasheet.

But now, according to Sylwester's patch, used various name for it like
MIPICSI, MIPICSIS and so on.
So I suggested just one name in previous my comment.

> >
> > ---
> > @@ -132,5 +132,6 @@
> > ?#define IRQ_LCD_FIFO ? ? ? ? ? IRQ_LCD0
> > ?#define IRQ_LCD_VSYNC ? ? ? ? ?IRQ_LCD1
> > ?#define IRQ_LCD_SYSTEM ? ? ? ? IRQ_LCD2
> > +#define IRQ_MIPI_CSIS0 ? ? ? ? IRQ_MIPICSI
> >
> > ?#endif /* ASM_ARCH_IRQS_H */
> > ---
> >
> > And I'm confused about the name of MIPI-CSI...
> > Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.
> >
> > Maybe the meaning is Camera Serial Interface Slave...
> > So...how about to use just one such as "IRQ_MIPI_CSIS" and
> > "S5PV210_PA_MIPI_CSIS".
> 
> I think it's better use the common word, CSI. I can't find CSIS word
> at googling.
> 
But used it in datasheet. How about FIMC, I think, it depends on usage of
datasheet.

> >
> > ---
> > @@ -65,7 +65,7 @@
> > ?#define IRQ_HSMMC0 ? ? ? ? ? ? S5P_IRQ_VIC1(26)
> > ?#define IRQ_HSMMC1 ? ? ? ? ? ? S5P_IRQ_VIC1(27)
> > ?#define IRQ_HSMMC2 ? ? ? ? ? ? S5P_IRQ_VIC1(28)
> > -#define IRQ_MIPICSI ? ? ? ? ? ?S5P_IRQ_VIC1(29)
> > +#define IRQ_MIPI_CSIS ? ? ? ? ?S5P_IRQ_VIC1(29)
> > ?#define IRQ_MIPIDSI ? ? ? ? ? ?S5P_IRQ_VIC1(30)
> > ?#define IRQ_ONENAND_AUDI ? ? ? S5P_IRQ_VIC1(31)
> >
> > @@ -132,5 +132,6 @@
> > ?#define IRQ_LCD_FIFO ? ? ? ? ? IRQ_LCD0
> > ?#define IRQ_LCD_VSYNC ? ? ? ? ?IRQ_LCD1
> > ?#define IRQ_LCD_SYSTEM ? ? ? ? IRQ_LCD2
> > +#define IRQ_MIPI_CSIS0 ? ? ? ? IRQ_MIPI_CSIS
> 
> In this case, I can't find any good reason since it's first used.
> as s3c series used the different names compare to s5p series. it's
> okay make these wrapper.
> but it's new one and not used previous time.
> 
I already commented about that.
Shortly, IRQ_MIPI_CSIS0 is used as common name for S5PV210 and S5PV310 in
plat-s5p.
And IRQ_MIPI_CSI is used as specific name on S5PV210. So needed it.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Kukjin Kim]

Kukjin Kim wrote:
> 
> Kyungmin Park wrote:
> >

One more..

> > > And I'm confused about the name of MIPI-CSI...
> > > Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.
> > >
> > > Maybe the meaning is Camera Serial Interface Slave...
> > > So...how about to use just one such as "IRQ_MIPI_CSIS" and
> > > "S5PV210_PA_MIPI_CSIS".
> >
> > I think it's better use the common word, CSI. I can't find CSIS word
> > at googling.
> >
> But used it in datasheet. How about FIMC, I think, it depends on usage of
> datasheet.
> 
However, if required, it can be changed...
I meant to use written name in datasheet is better to us.

> > >
> > > ---
> > > @@ -65,7 +65,7 @@
> > > ?#define IRQ_HSMMC0 ? ? ? ? ? ? S5P_IRQ_VIC1(26)
> > > ?#define IRQ_HSMMC1 ? ? ? ? ? ? S5P_IRQ_VIC1(27)
> > > ?#define IRQ_HSMMC2 ? ? ? ? ? ? S5P_IRQ_VIC1(28)
> > > -#define IRQ_MIPICSI ? ? ? ? ? ?S5P_IRQ_VIC1(29)
> > > +#define IRQ_MIPI_CSIS ? ? ? ? ?S5P_IRQ_VIC1(29)
> > > ?#define IRQ_MIPIDSI ? ? ? ? ? ?S5P_IRQ_VIC1(30)
> > > ?#define IRQ_ONENAND_AUDI ? ? ? S5P_IRQ_VIC1(31)
> > >
> > > @@ -132,5 +132,6 @@
> > > ?#define IRQ_LCD_FIFO ? ? ? ? ? IRQ_LCD0
> > > ?#define IRQ_LCD_VSYNC ? ? ? ? ?IRQ_LCD1
> > > ?#define IRQ_LCD_SYSTEM ? ? ? ? IRQ_LCD2
> > > +#define IRQ_MIPI_CSIS0 ? ? ? ? IRQ_MIPI_CSIS
> >
> > In this case, I can't find any good reason since it's first used.
> > as s3c series used the different names compare to s5p series. it's
> > okay make these wrapper.
> > but it's new one and not used previous time.
> >
> I already commented about that.
> Shortly, IRQ_MIPI_CSIS0 is used as common name for S5PV210 and S5PV310 in
> plat-s5p.
> And IRQ_MIPI_CSI is used as specific name on S5PV210. So needed it.
> 
To use IRQ_MIPI_CSIS means just one on S5PV210.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Jamie Iles]

On Fri, Dec 03, 2010 at 10:37:10AM +0900, Jassi Brar wrote:
> On Fri, Dec 3, 2010 at 2:39 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
> >
> > Hi Jamie,
> >
> > On 12/02/2010 06:15 PM, Jamie Iles wrote:
> >> Hi Sylwester,
> >>
> >> On Thu, Dec 02, 2010 at 05:37:41PM +0100, Sylwester Nawrocki wrote:
> >>> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
> >>> new file mode 100644
> >>> index 0000000..2b1ba43
> >>> --- /dev/null
> >>> +++ b/arch/arm/plat-s5p/dev-csis0.c
> >>> @@ -0,0 +1,34 @@
> >>> +/*
> >>> + * Copyright (C) 2010 Samsung Electronics
> >>> + *
> >>> + * S5P series device definition for MIPI-CSI device 0
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License version 2 as
> >>> + * published by the Free Software Foundation.
> >>> +*/
> >>> +
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <mach/map.h>
> >>> +
> >>> +static struct resource s5p_csis_resource[] = {
> >>> + ? ?[0] = {
> >>> + ? ? ? ? ? ?.start = S5P_PA_CSIS0,
> >>> + ? ? ? ? ? ?.end ? = S5P_PA_CSIS0 + SZ_4K - 1,
> >>> + ? ? ? ? ? ?.flags = IORESOURCE_MEM,
> >>> + ? ?},
> >>> + ? ?[1] = {
> >>> + ? ? ? ? ? ?.start = IRQ_MIPICSI0,
> >>> + ? ? ? ? ? ?.end ? = IRQ_MIPICSI0,
> >>> + ? ? ? ? ? ?.flags = IORESOURCE_IRQ,
> >>> + ? ?}
> >>> +};
> >> Do you really need the [0] and [1] here? These are only needed if you are
> >> sparsely initialising an array.
> >
> > I agree explicit indices are not really needed. I just followed the style of
> > all other resource definitions already there.
> > I am not quite sure what is the preference of the maintainer, I guess they want
> > all devices to follow same style for consistence.
> 
> And that style has its origin because explicit indices are as because
> they stress the fact that
> drivers expect  a particular resource at a particular index (when we
> have more than 1
> resource of a type).
> IMO we should use some macro shared between platform devices and their drivers.
> Something like -
> static struct resource s5p_csis_resource[] = {
>        [PA_CSIS] = {
>                .start = S5P_PA_CSIS0,
>                .end   = S5P_PA_CSIS0 + SZ_4K - 1,
>                .flags = IORESOURCE_MEM,
>        },
>        [IRQ_CSIS] = {
>                .start = IRQ_MIPICSI0,
>                .end   = IRQ_MIPICSI0,
>                .flags = IORESOURCE_IRQ,
>        }
> };
Or you could use the .name field of struct resource and use 
platform_get_resource_byname() in the driver. The disadvantage would be that 
this takes up a few more bytes though.

Jamie

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Marek Szyprowski]

Hello,

On Friday, December 03, 2010 2:37 AM Jassi Brar wrote:

> On Fri, Dec 3, 2010 at 2:39 AM, Sylwester Nawrocki

<snip>

> > I agree explicit indices are not really needed. I just followed the style of
> > all other resource definitions already there.
> > I am not quite sure what is the preference of the maintainer, I guess they want
> > all devices to follow same style for consistence.
> 
> And that style has its origin because explicit indices are as because
> they stress the fact that
> drivers expect  a particular resource at a particular index (when we
> have more than 1
> resource of a type).
> IMO we should use some macro shared between platform devices and their drivers.
> Something like -
> static struct resource s5p_csis_resource[] = {
>        [PA_CSIS] = {
>                .start = S5P_PA_CSIS0,
>                .end   = S5P_PA_CSIS0 + SZ_4K - 1,
>                .flags = IORESOURCE_MEM,
>        },
>        [IRQ_CSIS] = {
>                .start = IRQ_MIPICSI0,
>                .end   = IRQ_MIPICSI0,
>                .flags = IORESOURCE_IRQ,
>        }
> };

This looks like a huge over-engineering for me, especially for the simple device
that has only one register area and interrupt.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 0/4] ARM: S5P: Add platform support for MIPI-CSI2 devices

[Sylwester Nawrocki]

Hi Kukjin,

On 12/03/2010 05:11 AM, Kukjin Kim wrote:
> Sylwester Nawrocki wrote:
>>
>> Hello,
>>
> Hi :-)
> 
>> the following patch series adds S5P platform support for MIPI-CSI2
> devices.
>> It has been rebased onto next-samsung branch at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
> 
> It would be more helpful to me to use the 'for-next' next time.

OK, thank you for the hint. I'll keep that in mind or will be asking
in advance if possible.

>>
>> [PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis
>> [PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis
>> [PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices
>> [PATCH 4/4] mach-s5pv210: Add MIPI-CSI DPHY clock definition
>>
> 
> And used following commit title in others...
> 
> ARM: S5PV210: xxx
> ARM: S5P: xxx

Ok, will stick to that.

Thanks,
Sylwester

-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 2/4] mach-s5pv310: Add resource definitions for mipi-csis

[Sylwester Nawrocki]

On 12/03/2010 05:59 AM, Jassi Brar wrote:
> On Fri, Dec 3, 2010 at 1:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> Sylwester Nawrocki wrote:
>>>
>>> Add IRQ and register base address definitions for mipi-csis devices.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>  arch/arm/mach-s5pv310/include/mach/irqs.h |    3 +++
>>>  arch/arm/mach-s5pv310/include/mach/map.h  |    5 +++++
>>>  2 files changed, 8 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-s5pv310/include/mach/irqs.h b/arch/arm/mach-
>>> s5pv310/include/mach/irqs.h
>>> index 99e7dad..db63773 100644
>>> --- a/arch/arm/mach-s5pv310/include/mach/irqs.h
>>> +++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
>>> @@ -83,6 +83,9 @@
>>>  #define IRQ_HSMMC2           COMBINER_IRQ(29, 2)
>>>  #define IRQ_HSMMC3           COMBINER_IRQ(29, 3)
>>>
>>> +#define IRQ_MIPICSI0         COMBINER_IRQ(30, 0)
>>> +#define IRQ_MIPICSI1         COMBINER_IRQ(30, 1)
>>> +
>>>  #define IRQ_ONENAND_AUDI     COMBINER_IRQ(34, 0)
>>>
>>>  #define IRQ_EINT4            COMBINER_IRQ(37, 0)
>>> diff --git a/arch/arm/mach-s5pv310/include/mach/map.h b/arch/arm/mach-
>>> s5pv310/include/mach/map.h
>>> index 7acf4e7..9905bfe 100644
>>> --- a/arch/arm/mach-s5pv310/include/mach/map.h
>>> +++ b/arch/arm/mach-s5pv310/include/mach/map.h
>>> @@ -56,6 +56,9 @@
>>>  #define S5PV310_PA_GPIO2             (0x11000000)
>>>  #define S5PV310_PA_GPIO3             (0x03860000)
>>>
>>> +#define S5PV310_PA_CSIS0             (0x11880000)
>>> +#define S5PV310_PA_CSIS1             (0x11890000)
> 
> Let us please not contain constants in brackets.
> 
I don't like those brackets too, so I am going to remove them
in the next patch version. Unless anyone raises an objection.
It is hard to make everyone happy though. Sometimes good coding
practice prevails over consistency. However this time it looks like
me judgement was wrong ;-)

Thanks,
-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Sylwester Nawrocki]

On 12/03/2010 05:22 AM, Kukjin Kim wrote:
> Sylwester Nawrocki wrote:
>>
>> Added resource definitions for mipi-csis interface, naming
>> changed for consistency with s5pv310 where there are two
>> instances of the device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  arch/arm/mach-s5pv210/include/mach/irqs.h       |    2 +-
>>  arch/arm/mach-s5pv210/include/mach/map.h        |    4 ++++
>>  arch/arm/mach-s5pv210/include/mach/regs-clock.h |    5 +----
>>  3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-
>> s5pv210/include/mach/irqs.h
>> index 119b95f..8b5994c 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/irqs.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
>> @@ -65,7 +65,7 @@
>>  #define IRQ_HSMMC0		S5P_IRQ_VIC1(26)
>>  #define IRQ_HSMMC1		S5P_IRQ_VIC1(27)
>>  #define IRQ_HSMMC2		S5P_IRQ_VIC1(28)
>> -#define IRQ_MIPICSI		S5P_IRQ_VIC1(29)
>> +#define IRQ_MIPICSI0		S5P_IRQ_VIC1(29)
> 
> Firstly don't use directly IRQ_MIPICSI0 which is used in platform device.
> Because it can be confused that there would be another MIPICSI such as
> MIPICSI1.
> In that case, just use like following. For example,
> 
> ---
> @@ -132,5 +132,6 @@
>  #define IRQ_LCD_FIFO           IRQ_LCD0
>  #define IRQ_LCD_VSYNC          IRQ_LCD1
>  #define IRQ_LCD_SYSTEM         IRQ_LCD2
> +#define IRQ_MIPI_CSIS0         IRQ_MIPICSI
> 
>  #endif /* ASM_ARCH_IRQS_H */
> ---
> 
> And I'm confused about the name of MIPI-CSI...
> Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.

Yes, I agree it still looks a bit messy..
> 
> Maybe the meaning is Camera Serial Interface Slave...
> So...how about to use just one such as "IRQ_MIPI_CSIS" and
> "S5PV210_PA_MIPI_CSIS".
> 
> ---
> @@ -65,7 +65,7 @@
>  #define IRQ_HSMMC0             S5P_IRQ_VIC1(26)
>  #define IRQ_HSMMC1             S5P_IRQ_VIC1(27)
>  #define IRQ_HSMMC2             S5P_IRQ_VIC1(28)
> -#define IRQ_MIPICSI            S5P_IRQ_VIC1(29)
> +#define IRQ_MIPI_CSIS          S5P_IRQ_VIC1(29)
>  #define IRQ_MIPIDSI            S5P_IRQ_VIC1(30)
>  #define IRQ_ONENAND_AUDI       S5P_IRQ_VIC1(31)
> 
> @@ -132,5 +132,6 @@
>  #define IRQ_LCD_FIFO           IRQ_LCD0
>  #define IRQ_LCD_VSYNC          IRQ_LCD1
>  #define IRQ_LCD_SYSTEM         IRQ_LCD2
> +#define IRQ_MIPI_CSIS0         IRQ_MIPI_CSIS

Ok, agreed. I will incorporate that in next patch version.
IRQ_MIPI_CSIS0 for both s5pv210, s5pv310 in dev-csis?.c

> 
>  #endif /* ASM_ARCH_IRQS_H */
> ---
> 
>>  #define IRQ_MIPIDSI		S5P_IRQ_VIC1(30)
>>  #define IRQ_ONENAND_AUDI	S5P_IRQ_VIC1(31)
>>
>> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
>> s5pv210/include/mach/map.h
>> index 861d7fe..8f376f1 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/map.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
>> @@ -107,6 +107,9 @@
>>  #define S5PV210_PA_DMC0		(0xF0000000)
>>  #define S5PV210_PA_DMC1		(0xF1400000)
>>
>> +/* MIPI CSI */
> 
> No need comment.
> 
>> +#define S5PV210_PA_CSIS		0xFA600000
>> +
>>  /* compatibiltiy defines. */
>>  #define S3C_PA_UART		S5PV210_PA_UART
>>  #define S3C_PA_HSMMC0		S5PV210_PA_HSMMC(0)
>> @@ -123,6 +126,7 @@
>>  #define S5P_PA_FIMC0		S5PV210_PA_FIMC0
>>  #define S5P_PA_FIMC1		S5PV210_PA_FIMC1
>>  #define S5P_PA_FIMC2		S5PV210_PA_FIMC2
>> +#define S5P_PA_CSIS0		S5PV210_PA_CSIS
>>
> Same as above.
Ok, I am going to subsitute S5PV210_PA_CSIS with S5PV210_PA_MIPI_CSIS.
> 
>>  #define SAMSUNG_PA_ADC		S5PV210_PA_ADC
>>  #define SAMSUNG_PA_CFCON	S5PV210_PA_CFCON
>> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> b/arch/arm/mach-
>> s5pv210/include/mach/regs-clock.h
>> index ebaabe0..4c45b74 100644
>> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>> @@ -161,7 +161,7 @@
>>  #define S5P_MDNIE_SEL		S5P_CLKREG(0x7008)
>>  #define S5P_MIPI_PHY_CON0	S5P_CLKREG(0x7200)
>>  #define S5P_MIPI_PHY_CON1	S5P_CLKREG(0x7204)
>> -#define S5P_MIPI_CONTROL	S5P_CLKREG(0xE814)
>> +#define S5P_MIPI_DPHY_CONTROL	S5P_CLKREG(0xE814)
>>
>>  #define S5P_IDLE_CFG_TL_MASK	(3 << 30)
>>  #define S5P_IDLE_CFG_TM_MASK	(3 << 28)
>> @@ -195,9 +195,6 @@
>>  #define S5P_OTHERS_RET_UART		(1 << 28)
>>  #define S5P_OTHERS_USB_SIG_MASK		(1 << 16)
>>
>> -/* MIPI */
>> -#define S5P_MIPI_DPHY_EN		(3)
>> -
> 
> Great.
> 
>>  /* S5P_DAC_CONTROL */
>>  #define S5P_DAC_ENABLE			(1)
>>  #define S5P_DAC_DISABLE			(0)
>> --

Regards,
Sylwester

-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 3/4] plat-s5p: Add platform support for MIPI-CSI2 devices

[Sylwester Nawrocki]

On 12/03/2010 05:53 AM, Kukjin Kim wrote:
> Sylwester Nawrocki wrote:
>>
>> There may be up to 2 MIPI-CSI2 interfaces depending on SoC version.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  arch/arm/plat-s5p/Kconfig                 |   10 ++++++++
>>  arch/arm/plat-s5p/Makefile                |    2 +
>>  arch/arm/plat-s5p/dev-csis0.c             |   34
>> +++++++++++++++++++++++++++++
>>  arch/arm/plat-s5p/dev-csis1.c             |   33
>> ++++++++++++++++++++++++++++
>>  arch/arm/plat-samsung/include/plat/csis.h |   28 +++++++++++++++++++++++
>>  arch/arm/plat-samsung/include/plat/devs.h |    3 ++
>>  6 files changed, 110 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/plat-s5p/dev-csis0.c
>>  create mode 100644 arch/arm/plat-s5p/dev-csis1.c
>>  create mode 100644 arch/arm/plat-samsung/include/plat/csis.h
>>
>> diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
>> index 65dbfa8..2fb0c88 100644
>> --- a/arch/arm/plat-s5p/Kconfig
>> +++ b/arch/arm/plat-s5p/Kconfig
>> @@ -56,3 +56,13 @@ config S5P_DEV_ONENAND
>>  	bool
>>  	help
>>  	  Compile in platform device definition for OneNAND controller
>> +
>> +config S5P_DEV_CSIS0
>> +	bool
>> +	help
>> +	  Compile in platform device definitions for MIPI-CSI2 interface 0
> 
> MIPI-CSI2...you mean MIPI-CSI version 2. We _really_ need to separate the
> version here?

Yup, version 2. Maybe that was not a best place for such an information..

> How about just for MIPI-CSIS channel 0 ?

Ok, will change that, sounds better.

> 
>> +
>> +config S5P_DEV_CSIS1
>> +	bool
>> +	help
>> +	  Compile in platform device definitions for MIPI-CSI2 interface 1
> 
> MIPI-CSIS channel 1 ?
> 
>> diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
>> index de65238..2b73173 100644
>> --- a/arch/arm/plat-s5p/Makefile
>> +++ b/arch/arm/plat-s5p/Makefile
>> @@ -28,3 +28,5 @@ obj-$(CONFIG_S5P_DEV_FIMC0)	+= dev-fimc0.o
>>  obj-$(CONFIG_S5P_DEV_FIMC1)	+= dev-fimc1.o
>>  obj-$(CONFIG_S5P_DEV_FIMC2)	+= dev-fimc2.o
>>  obj-$(CONFIG_S5P_DEV_ONENAND)	+= dev-onenand.o
>> +obj-$(CONFIG_S5P_DEV_CSIS0)	+= dev-csis0.o
>> +obj-$(CONFIG_S5P_DEV_CSIS1)	+= dev-csis1.o
>> diff --git a/arch/arm/plat-s5p/dev-csis0.c b/arch/arm/plat-s5p/dev-csis0.c
>> new file mode 100644
>> index 0000000..2b1ba43
>> --- /dev/null
>> +++ b/arch/arm/plat-s5p/dev-csis0.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Copyright (C) 2010 Samsung Electronics
>> + *
>> + * S5P series device definition for MIPI-CSI device 0
> 
> MIPI-CSIS channel 0
OK.
> 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <mach/map.h>
>> +
>> +static struct resource s5p_csis_resource[] = {
> 
> Hmm...s5p_mipi_csis0_resource is more helpful even though static.

I personally hate inserting indexes into variables' name, but will
change this.

> 
>> +	[0] = {
>> +		.start = S5P_PA_CSIS0,
>> +		.end   = S5P_PA_CSIS0 + SZ_4K - 1,
> 
> Please refer to previous my comments about the name.

OK, will use S5P_PA_MIPI_CSIS0..
> 
>> +		.flags = IORESOURCE_MEM,
>> +	},
>> +	[1] = {
>> +		.start = IRQ_MIPICSI0,
>> +		.end   = IRQ_MIPICSI0,
> 
> Same.

and IRQ_MIPI_CSIS0 here.
> 
>> +		.flags = IORESOURCE_IRQ,
>> +	}
>> +};
>> +
>> +struct platform_device s5p_device_mipi_csis0 = {
>> +	.name		  = "s5p-mipi-csis",
>> +	.id		  = 0,
>> +	.num_resources	  = 2,
> 
> = ARRAY_SIZE(s5p_mipi_csis0_resource),
> 
>> +	.resource	  = &s5p_csis_resource[0],
> 
> = s5p_mipi_csis0_resource,
OK.
> 
>> +};
>> diff --git a/arch/arm/plat-s5p/dev-csis1.c b/arch/arm/plat-s5p/dev-csis1.c
>> new file mode 100644
>> index 0000000..7f21ea8
>> --- /dev/null
>> +++ b/arch/arm/plat-s5p/dev-csis1.c
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright (C) 2010 Samsung Electronics
>> + *
>> + * S5P series device definition for MIPI-CSI device 1
> 
> MIPI-CSIS channel 1
OK.
> 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <mach/map.h>
>> +
>> +static struct resource s5p_csis_resource[] = {
>> +	[0] = {
>> +		.start = S5P_PA_CSIS1,
>> +		.end   = S5P_PA_CSIS1 + SZ_4K - 1,
> 
> Name.
> 
>> +		.flags = IORESOURCE_MEM,
>> +	},
>> +	[1] = {
>> +		.start = IRQ_MIPICSI1,
>> +		.end   = IRQ_MIPICSI1,
> 
> Name.
> 
>> +		.flags = IORESOURCE_IRQ,
>> +	},
>> +};
>> +struct platform_device s5p_device_mipi_csis1 = {
>> +	.name		  = "s5p-mipi-csis",
>> +	.id		  = 1,
>> +	.num_resources	  = 2,
> 
> Same as above.
> 
>> +	.resource	  = &s5p_csis_resource[0],
> 
> Same.
> 
>> +};
>> diff --git a/arch/arm/plat-samsung/include/plat/csis.h b/arch/arm/plat-
>> samsung/include/plat/csis.h
>> new file mode 100644
>> index 0000000..6ea3a40
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/csis.h
> 
> Do you think really needed this for "plat-samsung" not just "plat-s5p"?

No, in fact I have tried both. Eventually I left it in plat-samsung since
I believe we could have a common MIPI CSIS driver for the s3c and s5p SoCs.
I have got v4l2_subdev based driver ready and tested on s5pv210 and s5pv310
but have never tried if it works on s3c SoCs. So for now I think it could be
better to move csis.h to "plat-s5p" as you suggested.
I am going to incorporate that change in next patch version.

> 
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Copyright (C) 2010 Samsung Electronics Co., Ltd
>> + *
>> + * S5P series MIPI CSI2 device support
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef PLAT_CSIS_H_
>> +#define PLAT_CSIS_H_ __FILE__
>> +
>> +/**
>> + * struct s3c_platform_csis - default platform data for MIPI CSI2
> interface
> 
> ...
> 
>> + * @clock_rate: bus clock frequency
> 
> clk_rate as Jamie Iles's comment.

All right, I missed somehow the comment about the clock yesterday.
Thanks Jamie!

> 
>> + * @nlanes: number of MIPI data lanes used
> 
> How about just "lanes".

Ok, I don't mind changing that. It was supposed to mean n(umber of)lanes.

> 
>> + * @alignment: MIPI data alignment in bits
>> + * @hs_settle: HSYNC settle time (see CSIS_DPHYCTRL register description)
> 
> Actually, not mentioned HS-RX settle time in CSIS_DPHYCTRL register of
> datasheet.

Ups, my fault. But...  bits 27..31 in CSIS_DPHYCTRL are for HS-RX settle time,
aren't they?
The description is not in the datasheet but I believe it *should* be there.
I hoped someone updates the datasheet eventually. I have even reported it to
one of your colleagues, who helped me in solving the HS-RX settle related
issues. Missing details in the specs are real pain during development.
It is often impossible to get anything working without a reference code.

> 
> + * @hs_settle: HS-RX settle time.
Ok, thanks.

-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center

[PATCH 1/4] mach-s5pv210: Add platform definitions for mipi-csis

[Sylwester Nawrocki]

On 12/03/2010 06:30 AM, Kyungmin Park wrote:
> On Fri, Dec 3, 2010 at 1:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> Sylwester Nawrocki wrote:
>>>
>>> Added resource definitions for mipi-csis interface, naming
>>> changed for consistency with s5pv310 where there are two
>>> instances of the device.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>  arch/arm/mach-s5pv210/include/mach/irqs.h       |    2 +-
>>>  arch/arm/mach-s5pv210/include/mach/map.h        |    4 ++++
>>>  arch/arm/mach-s5pv210/include/mach/regs-clock.h |    5 +----
>>>  3 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-s5pv210/include/mach/irqs.h b/arch/arm/mach-
>>> s5pv210/include/mach/irqs.h
>>> index 119b95f..8b5994c 100644
>>> --- a/arch/arm/mach-s5pv210/include/mach/irqs.h
>>> +++ b/arch/arm/mach-s5pv210/include/mach/irqs.h
>>> @@ -65,7 +65,7 @@
>>>  #define IRQ_HSMMC0           S5P_IRQ_VIC1(26)
>>>  #define IRQ_HSMMC1           S5P_IRQ_VIC1(27)
>>>  #define IRQ_HSMMC2           S5P_IRQ_VIC1(28)
>>> -#define IRQ_MIPICSI          S5P_IRQ_VIC1(29)
>>> +#define IRQ_MIPICSI0         S5P_IRQ_VIC1(29)
>>
>> Firstly don't use directly IRQ_MIPICSI0 which is used in platform device.
>> Because it can be confused that there would be another MIPICSI such as
>> MIPICSI1.
>> In that case, just use like following. For example,
> Hi,
> 
> First respect others work.
> my impression is that you forced the developers to work as you want.
> 
> okay who summit this code? If you think name is odd, then you check it
> before you submitted.
> When you commit the code is okay, but other should be considered as yours.
> 
>>
>> ---
>> @@ -132,5 +132,6 @@
>>  #define IRQ_LCD_FIFO           IRQ_LCD0
>>  #define IRQ_LCD_VSYNC          IRQ_LCD1
>>  #define IRQ_LCD_SYSTEM         IRQ_LCD2
>> +#define IRQ_MIPI_CSIS0         IRQ_MIPICSI
>>
>>  #endif /* ASM_ARCH_IRQS_H */
>> ---
>>
>> And I'm confused about the name of MIPI-CSI...
>> Hmm...used MIPICSI or MIPI_CSI, sometimes MIPICSIS or MIPI_CSIS.
>>
>> Maybe the meaning is Camera Serial Interface Slave...
>> So...how about to use just one such as "IRQ_MIPI_CSIS" and
>> "S5PV210_PA_MIPI_CSIS".
> 
> I think it's better use the common word, CSI. I can't find CSIS word
> at googling.

Yeah, you are making a good point Kyungmin. I just wanted to make it more human
friendly by using CSI instead of CSIS. However I guess we would be
better off using exact names from the datasheet.
The worst thing is that the datasheet often lacks an explanation of what
some of that cryptic names like MIPI CSIS, FIMC, FIMD, etc. mean.

For those who still don't know:
MIPI CSIS - MIPI Camera Serial Interface Slave
FIMC - Fully Interactive Mobile Camera
FIMD - Fully Interactive Mobile Display


Regards,
-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center