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

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

[Sylwester Nawrocki]

Hello,

the following is a second version of patch series adding S5P platform support
for MIPI-CSIS devices. Hopefully all the comments have been addressed, 
if not please let me know.

[PATCH 1/4 v2] ARM: S5PV210: Add resource definitions for MIPI CSIS
[PATCH 2/4 v2] ARM: S5PV310: Add resource definitions for MIPI CSIS
[PATCH 3/4 v2] ARM: S5P: Add platform support for MIPI CSIS devices
[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

Rebased onto for-next branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git 


Regards,
Sylwester


--
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH 1/4 v2] ARM: S5PV210: Add resource definitions for MIPI CSIS

[Sylwester Nawrocki]

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       |    3 ++-
 arch/arm/mach-s5pv210/include/mach/map.h        |    3 +++
 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..26710b3 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_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 */
diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
index 861d7fe..e50cdc7 100644
--- a/arch/arm/mach-s5pv210/include/mach/map.h
+++ b/arch/arm/mach-s5pv210/include/mach/map.h
@@ -107,6 +107,8 @@
 #define S5PV210_PA_DMC0		(0xF0000000)
 #define S5PV210_PA_DMC1		(0xF1400000)
 
+#define S5PV210_PA_MIPI_CSIS	0xFA600000
+
 /* compatibiltiy defines. */
 #define S3C_PA_UART		S5PV210_PA_UART
 #define S3C_PA_HSMMC0		S5PV210_PA_HSMMC(0)
@@ -123,6 +125,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_MIPI_CSIS0	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)
-
 /* S5P_DAC_CONTROL */
 #define S5P_DAC_ENABLE			(1)
 #define S5P_DAC_DISABLE			(0)
-- 
1.7.3.2

[PATCH 2/4 v2] ARM: S5PV310: Add resource definitions for MIPI CSIS

[Sylwester Nawrocki]

Add IRQ and register base address definitions for MIPI CSI slave 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 3c05c58..34f2144 100644
--- a/arch/arm/mach-s5pv310/include/mach/irqs.h
+++ b/arch/arm/mach-s5pv310/include/mach/irqs.h
@@ -86,6 +86,9 @@
 #define IRQ_HSMMC2		COMBINER_IRQ(29, 2)
 #define IRQ_HSMMC3		COMBINER_IRQ(29, 3)
 
+#define IRQ_MIPI_CSIS0		COMBINER_IRQ(30, 0)
+#define IRQ_MIPI_CSIS1		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 5399446..ab4b041 100644
--- a/arch/arm/mach-s5pv310/include/mach/map.h
+++ b/arch/arm/mach-s5pv310/include/mach/map.h
@@ -61,6 +61,9 @@
 #define S5PV310_PA_GPIO2		(0x11000000)
 #define S5PV310_PA_GPIO3		(0x03860000)
 
+#define S5PV310_PA_MIPI_CSIS0		0x11880000
+#define S5PV310_PA_MIPI_CSIS1		0x11890000
+
 #define S5PV310_PA_HSMMC(x)		(0x12510000 + ((x) * 0x10000))
 
 #define S5PV310_PA_SROMC		(0x12570000)
@@ -116,5 +119,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_MIPI_CSIS0		S5PV310_PA_MIPI_CSIS0
+#define S5P_PA_MIPI_CSIS1		S5PV310_PA_MIPI_CSIS1
 
 #endif /* __ASM_ARCH_MAP_H */
-- 
1.7.3.2

[PATCH 3/4 v2] ARM: S5P: Add platform support for MIPI CSIS devices

[Sylwester Nawrocki]

There may be up to two MIPI CSI slave interfaces depending on the 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             |   34 +++++++++++++++++++++++++++++
 arch/arm/plat-s5p/include/plat/csis.h     |   28 +++++++++++++++++++++++
 arch/arm/plat-samsung/include/plat/devs.h |    3 ++
 6 files changed, 111 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-s5p/include/plat/csis.h

diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
index 65dbfa8..6a161f3 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-CSIS channel 0
+
+config S5P_DEV_CSIS1
+	bool
+	help
+	  Compile in platform device definitions for 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..dfab1c8
--- /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-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_mipi_csis0_resource[] = {
+	[0] = {
+		.start = S5P_PA_MIPI_CSIS0,
+		.end   = S5P_PA_MIPI_CSIS0 + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = IRQ_MIPI_CSIS0,
+		.end   = IRQ_MIPI_CSIS0,
+		.flags = IORESOURCE_IRQ,
+	}
+};
+
+struct platform_device s5p_device_mipi_csis0 = {
+	.name		  = "s5p-mipi-csis",
+	.id		  = 0,
+	.num_resources	  = ARRAY_SIZE(s5p_mipi_csis0_resource),
+	.resource	  = 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..e3053f2
--- /dev/null
+++ b/arch/arm/plat-s5p/dev-csis1.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * S5P series device definition for 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_mipi_csis1_resource[] = {
+	[0] = {
+		.start = S5P_PA_MIPI_CSIS1,
+		.end   = S5P_PA_MIPI_CSIS1 + SZ_4K - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = IRQ_MIPI_CSIS1,
+		.end   = IRQ_MIPI_CSIS1,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+struct platform_device s5p_device_mipi_csis1 = {
+	.name		  = "s5p-mipi-csis",
+	.id		  = 1,
+	.num_resources	  = ARRAY_SIZE(s5p_mipi_csis1_resource),
+	.resource	  = s5p_mipi_csis1_resource,
+};
diff --git a/arch/arm/plat-s5p/include/plat/csis.h b/arch/arm/plat-s5p/include/plat/csis.h
new file mode 100644
index 0000000..51e308c
--- /dev/null
+++ b/arch/arm/plat-s5p/include/plat/csis.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010 Samsung Electronics
+ *
+ * S5P series MIPI CSI slave 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_S5P_CSIS_H_
+#define PLAT_S5P_CSIS_H_ __FILE__
+
+/**
+ * struct s5p_platform_mipi_csis - platform data for MIPI-CSIS
+ * @clk_rate: bus clock frequency
+ * @lanes: number of data lanes used
+ * @alignment: data alignment in bits
+ * @hs_settle: HS-RX settle time
+ */
+struct s5p_platform_mipi_csis {
+	unsigned long clk_rate;
+	u8 lanes;
+	u8 alignment;
+	u8 hs_settle;
+};
+
+#endif /* PLAT_S5P_CSIS_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
index dec0ded..01dadee 100644
--- a/arch/arm/plat-samsung/include/plat/devs.h
+++ b/arch/arm/plat-samsung/include/plat/devs.h
@@ -131,6 +131,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.3.2

[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

[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-csis 
and mipi-dsim 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_"dphy_clock", perhaps power domain
handling code would be better place for it.

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 b774ff1..b880b85 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.3.2

[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> 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-csis
> and mipi-dsim 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_"dphy_clock", perhaps power domain
> handling code would be better place for it.
> 
Yeah, it is MIPI DPHY enable/disable control register not clock control.
So its proper position is not here...

Hmm...how about driver's probe/open or some kind of setup in machine
directory?

> 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 b774ff1..b880b85 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),
> +};
> +

Hmm...

>  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),
> +	}, {

ok.

> +		.name		= "csis_dphy",
> +		.id		= -1,
> +		.parent		= &clk_vp_mipi_dphy,
> +		.enable		= s5pv210_clk_mipi_dphy_ctrl,
> +		.ctrlbit	= (1 << 1),
> +	}, {

Same as above.

>  		.name		= "rot",
>  		.id		= -1,
>  		.parent		= &clk_hclk_dsys.clk,
> --

Thanks.

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

[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

[Marek Szyprowski]

Hello,

On Friday, December 10, 2010 6:14 AM Kukjin Kim wrote:

> Sylwester Nawrocki wrote:
> >
> > 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-csis
> > and mipi-dsim 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_"dphy_clock", perhaps power domain
> > handling code would be better place for it.
> >
> Yeah, it is MIPI DPHY enable/disable control register not clock control.
> So its proper position is not here...
> 
> Hmm...how about driver's probe/open or some kind of setup in machine
> directory?

I'm not sure that this is the best way of handling this phy controller. Please
notice that mipi phy controller has different register location on S5PC110 and
S5PC210. Please also notice that S5PC210 has 2 mipi csci controllers and phy
interfaces, while s5pc110 has only one. Hiding all the logic and register
specific writes behind this 'csi_dphy' clock easily resolves all these issues
on all different samsung platforms and makes it easy to use it from the driver.
Similar patch has been proposed some time ago for usb_phy interface and imho
this is the right way to go.

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

[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

[Sylwester Nawrocki]

Hi,

On 2010-12-10 10:12, Marek Szyprowski wrote:
> Hello,
> 
> On Friday, December 10, 2010 6:14 AM Kukjin Kim wrote:
> 
>> Sylwester Nawrocki wrote:
>>>
>>> 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-csis
>>> and mipi-dsim 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_"dphy_clock", perhaps power domain
>>> handling code would be better place for it.
>>>
>> Yeah, it is MIPI DPHY enable/disable control register not clock control.
>> So its proper position is not here...
>>
>> Hmm...how about driver's probe/open or some kind of setup in machine
>> directory?
> 
> I'm not sure that this is the best way of handling this phy controller. Please
> notice that mipi phy controller has different register location on S5PC110 and
> S5PC210. Please also notice that S5PC210 has 2 mipi csci controllers and phy
> interfaces, while s5pc110 has only one. Hiding all the logic and register
> specific writes behind this 'csi_dphy' clock easily resolves all these issues
> on all different samsung platforms and makes it easy to use it from the driver.
> Similar patch has been proposed some time ago for usb_phy interface and imho
> this is the right way to go.
> 
More importantly S5P_MIPI_DPHY_CONTROL* register is shared between two
devices - MIPI-CSIS and MIPI-DSIM. It means some kind of serialization
mechanism need to be employed while accessing this register, a spinlock
seems to fit best here. It also means that if we decided to create the
platform callback such a code would have to be placed in a common
compilation unit which would be built when either mipi-csis or mipi-dsim
is selected. And this callback would have to account the differences
between various SoC flavours or it would have to be repeated for each
mach-s5pvXXX.
So if the platform callback is the preferred way, where do we put an
implementation of it?

Regards,
Sylwester

[PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY

[Marek Szyprowski]

Hello,

On Friday, December 10, 2010 1:01 PM Sylwester Nawrocki wrote:

> On 2010-12-10 10:12, Marek Szyprowski wrote:
> > On Friday, December 10, 2010 6:14 AM Kukjin Kim wrote:
> >> Sylwester Nawrocki wrote:
> >>>
> >>> 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-csis
> >>> and mipi-dsim 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_"dphy_clock", perhaps power domain
> >>> handling code would be better place for it.
> >>>
> >> Yeah, it is MIPI DPHY enable/disable control register not clock control.
> >> So its proper position is not here...
> >>
> >> Hmm...how about driver's probe/open or some kind of setup in machine
> >> directory?
> >
> > I'm not sure that this is the best way of handling this phy controller. Please
> > notice that mipi phy controller has different register location on S5PC110 and
> > S5PC210. Please also notice that S5PC210 has 2 mipi csci controllers and phy
> > interfaces, while s5pc110 has only one. Hiding all the logic and register
> > specific writes behind this 'csi_dphy' clock easily resolves all these issues
> > on all different samsung platforms and makes it easy to use it from the driver.
> > Similar patch has been proposed some time ago for usb_phy interface and imho
> > this is the right way to go.
> >
> More importantly S5P_MIPI_DPHY_CONTROL* register is shared between two
> devices - MIPI-CSIS and MIPI-DSIM. It means some kind of serialization
> mechanism need to be employed while accessing this register, a spinlock
> seems to fit best here. It also means that if we decided to create the
> platform callback such a code would have to be placed in a common
> compilation unit which would be built when either mipi-csis or mipi-dsim
> is selected. And this callback would have to account the differences
> between various SoC flavours or it would have to be repeated for each
> mach-s5pvXXX.
> So if the platform callback is the preferred way, where do we put an
> implementation of it?

That's yet another advantage of clock-based approach. All clock related operations
are automatically serialized by global clock API spinlock.

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

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

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Hello,
> 
Hi,

> the following is a second version of patch series adding S5P platform
support
> for MIPI-CSIS devices. Hopefully all the comments have been addressed,
> if not please let me know.
> 
> [PATCH 1/4 v2] ARM: S5PV210: Add resource definitions for MIPI CSIS
> [PATCH 2/4 v2] ARM: S5PV310: Add resource definitions for MIPI CSIS
> [PATCH 3/4 v2] ARM: S5P: Add platform support for MIPI CSIS devices
> [PATCH 4/4 v2] ARM: S5PV210: Add clock definition for MIPI-CSIS DPHY
> 
> Rebased onto for-next branch at:
> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
> 
Looks ok to me 1st, 2nd and 3rd to me...will apply.
And will check your 4th patch also soon.

Thanks.

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