[PATCH/RFC] ARM: S5P: Add platform helpers for camera GPIO configuration

[PATCH/RFC] ARM: S5P: Add platform helpers for camera GPIO configuration

[Sylwester Nawrocki]

Hello,

the following patch introduces platform helpers for the peripheral 
parallel camera bus configuration in S5PV210 and S5PV310 SoCs.
In those SoCs multiple camera host interface IP's are available and
can be attached to any of two ITU-R parrallel bus for external sensor.
That's why I didn't try to pass any platform device as a pointer to those
helpers. This is an initial proposal and any comments are appreciated.


[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

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


Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

[Sylwester Nawrocki]

Add functions for the parallel camera GPIO interface
configuration on S5PV210 and S5PV310 SoCs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <s.nawrocki@samsung.com>
---
 arch/arm/mach-s5pv210/Kconfig           |    5 +++
 arch/arm/mach-s5pv210/Makefile          |    1 +
 arch/arm/mach-s5pv210/setup-camera.c    |   53 +++++++++++++++++++++++++++++++
 arch/arm/mach-s5pv310/Kconfig           |    5 +++
 arch/arm/mach-s5pv310/Makefile          |    1 +
 arch/arm/mach-s5pv310/setup-camera.c    |   43 +++++++++++++++++++++++++
 arch/arm/plat-s5p/include/plat/camera.h |   26 +++++++++++++++
 7 files changed, 134 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-s5pv210/setup-camera.c
 create mode 100644 arch/arm/mach-s5pv310/setup-camera.c
 create mode 100644 arch/arm/plat-s5p/include/plat/camera.h

diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 53aabef..300993a 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -53,6 +53,11 @@ config S5PV210_SETUP_SDHCI_GPIO
 	help
 	  Common setup code for SDHCI gpio.
 
+config S5PV210_SETUP_CAMERA
+	bool
+	help
+	  Common setup code for the camera interfaces.
+
 menu "S5PC110 Machines"
 
 config MACH_AQUILA
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index ff1a0db..d6c9f0d 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_MACH_TORBRECK)	+= mach-torbreck.o
 obj-y				+= dev-audio.o
 obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
 
+obj-$(CONFIG_S5PV210_SETUP_CAMERA)	+= setup-camera.o
 obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
 obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
 obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
diff --git a/arch/arm/mach-s5pv210/setup-camera.c b/arch/arm/mach-s5pv210/setup-camera.c
new file mode 100644
index 0000000..e13c354
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-camera.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * S5PV310 camera interface GPIO configuration.
+ *
+ * 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/gpio.h>
+#include <plat/gpio-cfg.h>
+#include <plat/camera.h>
+
+/*
+ * Configure the camera parallel bus pins. The parallel bus can be multiplexed
+ * with any FIMC entity. Even multiple FIMC entities are allowed to be attached
+ * to a particular (A or B) gpio interface. This function should be called from
+ * a board setup code.
+ */
+int s5pv210_camif_cfg_gpio(enum s5p_camif_id id)
+{
+	u32 gpio8, gpio5;
+	int ret;
+	int i = 5;
+
+	switch (id) {
+	case S5P_CAMIF_A:
+		gpio8 = S5PV210_GPE0(0);
+		gpio5 = S5PV210_GPE1(0);
+		break;
+	case S5P_CAMIF_B:
+		gpio8 = S5PV210_GPJ0(0);
+		gpio5 = S5PV210_GPJ1(0);
+		break;
+	default:
+		WARN(1, "id: %d\n", id);
+		return -EINVAL;
+	}
+
+	ret = s3c_gpio_cfgall_range(gpio8, 8, S3C_GPIO_SFN(2),
+				    S3C_GPIO_PULL_UP);
+	if (ret)
+		return ret;
+
+	ret = s3c_gpio_cfgall_range(gpio5, 5, S3C_GPIO_SFN(2),
+				    S3C_GPIO_PULL_UP);
+
+	while (i-- && !ret)
+		ret = s5p_gpio_set_drvstr(S5PV210_GPE1(i),
+					  S5P_GPIO_DRVSTR_LV4);
+	return ret;
+}
diff --git a/arch/arm/mach-s5pv310/Kconfig b/arch/arm/mach-s5pv310/Kconfig
index b2a9acc..ccd1dc4 100644
--- a/arch/arm/mach-s5pv310/Kconfig
+++ b/arch/arm/mach-s5pv310/Kconfig
@@ -71,6 +71,11 @@ config S5PV310_DEV_SYSMMU
 	help
 	  Common setup code for SYSTEM MMU in S5PV310
 
+config S5PV310_SETUP_CAMERA
+	bool
+	help
+	  Common setup code for the camera interfaces.
+
 # machine support
 
 menu "S5PC210 Machines"
diff --git a/arch/arm/mach-s5pv310/Makefile b/arch/arm/mach-s5pv310/Makefile
index 036fb38..c1d6577 100644
--- a/arch/arm/mach-s5pv310/Makefile
+++ b/arch/arm/mach-s5pv310/Makefile
@@ -32,6 +32,7 @@ obj-y					+= dev-audio.o
 obj-$(CONFIG_S5PV310_DEV_PD)		+= dev-pd.o
 obj-$(CONFIG_S5PV310_DEV_SYSMMU)	+= dev-sysmmu.o
 
+obj-$(CONFIG_S5PV310_SETUP_CAMERA)	+= setup-camera.o
 obj-$(CONFIG_S5PV310_SETUP_I2C1)	+= setup-i2c1.o
 obj-$(CONFIG_S5PV310_SETUP_I2C2)	+= setup-i2c2.o
 obj-$(CONFIG_S5PV310_SETUP_I2C3)	+= setup-i2c3.o
diff --git a/arch/arm/mach-s5pv310/setup-camera.c b/arch/arm/mach-s5pv310/setup-camera.c
new file mode 100644
index 0000000..8ab239e
--- /dev/null
+++ b/arch/arm/mach-s5pv310/setup-camera.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * S5PV310 camera interface GPIO configuration.
+ *
+ * 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/gpio.h>
+#include <plat/gpio-cfg.h>
+#include <plat/camera.h>
+
+/* Configure the camera parallel bus pins. */
+int s5pv310_camif_cfg_gpio(enum s5p_camif_id id)
+{
+	u32 gpio8, gpio5;
+	u32 sfn;
+	int ret;
+
+	switch (id) {
+	case S5P_CAMIF_A:
+		gpio8 = S5PV310_GPJ0(0); /* PCLK, VSYNC, HREF, DATA[0:4] */
+		gpio5 = S5PV310_GPJ1(0); /* DATA[5:7], CLKOUT, FIELD */
+		sfn = S3C_GPIO_SFN(2);
+		break;
+	case S5P_CAMIF_B:
+		gpio8 = S5PV310_GPE0(0); /* DATA[0:7] */
+		gpio5 = S5PV310_GPE1(0); /* PCLK, VSYNC, HREF, CLKOUT, FIELD */
+		sfn = S3C_GPIO_SFN(3);
+		break;
+	default:
+		WARN(1, "id: %d\n", id);
+		return -EINVAL;
+	}
+
+	ret = s3c_gpio_cfgall_range(gpio8, 8, sfn, S3C_GPIO_PULL_UP);
+	if (ret)
+		return ret;
+
+	return s3c_gpio_cfgall_range(gpio5, 5, sfn, S3C_GPIO_PULL_UP);
+}
diff --git a/arch/arm/plat-s5p/include/plat/camera.h b/arch/arm/plat-s5p/include/plat/camera.h
new file mode 100644
index 0000000..f7c66ec
--- /dev/null
+++ b/arch/arm/plat-s5p/include/plat/camera.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * S5P series camera interface helper functions
+ *
+ * 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_CAMERA_H_
+#define PLAT_S5P_CAMERA_H_ __FILE__
+
+enum s5p_camif_id {
+	S5P_CAMIF_A,
+	S5P_CAMIF_B,
+};
+
+/**
+ * s5pvX10_camif_cfg_gpio - configure IO pins of the camera A/B interface
+ * @id: id of a camera gpio interface
+ */
+int s5pv210_camif_cfg_gpio(enum s5p_camif_id id);
+int s5pv310_camif_cfg_gpio(enum s5p_camif_id id);
+
+#endif /* PLAT_S5P_CAMERA_H_ */
-- 
1.7.4

[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

[Sylwester Nawrocki]

On 02/11/2011 07:07 PM, Sylwester Nawrocki wrote:
> Add functions for the parallel camera GPIO interface
> configuration on S5PV210 and S5PV310 SoCs.
> 

Kukjin, are you OK with general concept of those?
I would probably need to rework the patches after recent
S5PV310 -> EXYNOS4 renaming. And move camera.h 
to arch/arm/plat-samsung/include/plat/?

What's your opinion?

Thanks,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> 
> On 02/11/2011 07:07 PM, Sylwester Nawrocki wrote:
> > Add functions for the parallel camera GPIO interface
> > configuration on S5PV210 and S5PV310 SoCs.
> >
> 
> Kukjin, are you OK with general concept of those?
> I would probably need to rework the patches after recent
> S5PV310 -> EXYNOS4 renaming. And move camera.h
> to arch/arm/plat-samsung/include/plat/?
> 
> What's your opinion?

Oh, missed...yeah need to re-work based on exynos4 :)
And you don't need to move it into plat-samsung/ because plat-s5p/ is used
for Exynos also.

Please refer to my comments on it.

Thanks.

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

[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Add functions for the parallel camera GPIO interface
> configuration on S5PV210 and S5PV310 SoCs.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <s.nawrocki@samsung.com>

Hmm...typo?
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

> ---
>  arch/arm/mach-s5pv210/Kconfig           |    5 +++
>  arch/arm/mach-s5pv210/Makefile          |    1 +
>  arch/arm/mach-s5pv210/setup-camera.c    |   53
> +++++++++++++++++++++++++++++++
>  arch/arm/mach-s5pv310/Kconfig           |    5 +++
>  arch/arm/mach-s5pv310/Makefile          |    1 +
>  arch/arm/mach-s5pv310/setup-camera.c    |   43 +++++++++++++++++++++++++
>  arch/arm/plat-s5p/include/plat/camera.h |   26 +++++++++++++++
>  7 files changed, 134 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-s5pv210/setup-camera.c
>  create mode 100644 arch/arm/mach-s5pv310/setup-camera.c
>  create mode 100644 arch/arm/plat-s5p/include/plat/camera.h
> 
> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
> index 53aabef..300993a 100644
> --- a/arch/arm/mach-s5pv210/Kconfig
> +++ b/arch/arm/mach-s5pv210/Kconfig
> @@ -53,6 +53,11 @@ config S5PV210_SETUP_SDHCI_GPIO
>  	help
>  	  Common setup code for SDHCI gpio.
> 
> +config S5PV210_SETUP_CAMERA

How about "S5PV210_SETUP_FIMC"?

As you know, it belong to FIMC block which is written in datasheet.
And this naming is more reasonable like SETUP_I2C...

> +	bool
> +	help
> +	  Common setup code for the camera interfaces.
> +
>  menu "S5PC110 Machines"
> 
>  config MACH_AQUILA
> diff --git a/arch/arm/mach-s5pv210/Makefile
b/arch/arm/mach-s5pv210/Makefile
> index ff1a0db..d6c9f0d 100644
> --- a/arch/arm/mach-s5pv210/Makefile
> +++ b/arch/arm/mach-s5pv210/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_MACH_TORBRECK)	+= mach-torbreck.o
>  obj-y				+= dev-audio.o
>  obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
> 
> +obj-$(CONFIG_S5PV210_SETUP_CAMERA)	+= setup-camera.o
>  obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
>  obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
>  obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
> diff --git a/arch/arm/mach-s5pv210/setup-camera.c b/arch/arm/mach-
> s5pv210/setup-camera.c
> new file mode 100644
> index 0000000..e13c354
> --- /dev/null
> +++ b/arch/arm/mach-s5pv210/setup-camera.c

So setup-fimc.c

> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd.
> + *
> + * S5PV310 camera interface GPIO configuration.
> + *
> + * 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/gpio.h>
> +#include <plat/gpio-cfg.h>
> +#include <plat/camera.h>
> +
> +/*
> + * Configure the camera parallel bus pins. The parallel bus can be
> multiplexed
> + * with any FIMC entity. Even multiple FIMC entities are allowed to be
> attached
> + * to a particular (A or B) gpio interface. This function should be
called
> from
> + * a board setup code.
> + */
> +int s5pv210_camif_cfg_gpio(enum s5p_camif_id id)

int s5pv210_fimc_cfg_gpio...

> +{
> +	u32 gpio8, gpio5;
> +	int ret;
> +	int i = 5;
> +
> +	switch (id) {
> +	case S5P_CAMIF_A:
> +		gpio8 = S5PV210_GPE0(0);
> +		gpio5 = S5PV210_GPE1(0);
> +		break;

Blank line?

> +	case S5P_CAMIF_B:
> +		gpio8 = S5PV210_GPJ0(0);
> +		gpio5 = S5PV210_GPJ1(0);
> +		break;

Same...

> +	default:
> +		WARN(1, "id: %d\n", id);
> +		return -EINVAL;
> +	}
> +
> +	ret = s3c_gpio_cfgall_range(gpio8, 8, S3C_GPIO_SFN(2),
> +				    S3C_GPIO_PULL_UP);
> +	if (ret)
> +		return ret;
> +
> +	ret = s3c_gpio_cfgall_range(gpio5, 5, S3C_GPIO_SFN(2),
> +				    S3C_GPIO_PULL_UP);

Where is "return ret;" ?

> +
> +	while (i-- && !ret)
> +		ret = s5p_gpio_set_drvstr(S5PV210_GPE1(i),
> +					  S5P_GPIO_DRVSTR_LV4);

Basically drive strength depends on each board. So I think, should be
removed here.

> +	return ret;
> +}
> diff --git a/arch/arm/mach-s5pv310/Kconfig b/arch/arm/mach-s5pv310/Kconfig
> index b2a9acc..ccd1dc4 100644
> --- a/arch/arm/mach-s5pv310/Kconfig
> +++ b/arch/arm/mach-s5pv310/Kconfig
> @@ -71,6 +71,11 @@ config S5PV310_DEV_SYSMMU
>  	help
>  	  Common setup code for SYSTEM MMU in S5PV310
> 
> +config S5PV310_SETUP_CAMERA

S5PV310_SETUP_FIMC ?

> +	bool
> +	help
> +	  Common setup code for the camera interfaces.
> +
>  # machine support
> 
>  menu "S5PC210 Machines"
> diff --git a/arch/arm/mach-s5pv310/Makefile
b/arch/arm/mach-s5pv310/Makefile
> index 036fb38..c1d6577 100644
> --- a/arch/arm/mach-s5pv310/Makefile
> +++ b/arch/arm/mach-s5pv310/Makefile
> @@ -32,6 +32,7 @@ obj-y					+=
dev-audio.o
>  obj-$(CONFIG_S5PV310_DEV_PD)		+= dev-pd.o
>  obj-$(CONFIG_S5PV310_DEV_SYSMMU)	+= dev-sysmmu.o
> 
> +obj-$(CONFIG_S5PV310_SETUP_CAMERA)	+= setup-camera.o
>  obj-$(CONFIG_S5PV310_SETUP_I2C1)	+= setup-i2c1.o
>  obj-$(CONFIG_S5PV310_SETUP_I2C2)	+= setup-i2c2.o
>  obj-$(CONFIG_S5PV310_SETUP_I2C3)	+= setup-i2c3.o
> diff --git a/arch/arm/mach-s5pv310/setup-camera.c b/arch/arm/mach-
> s5pv310/setup-camera.c
> new file mode 100644
> index 0000000..8ab239e
> --- /dev/null
> +++ b/arch/arm/mach-s5pv310/setup-camera.c

Almost same.

(snip)

> diff --git a/arch/arm/plat-s5p/include/plat/camera.h b/arch/arm/plat-
> s5p/include/plat/camera.h
> new file mode 100644
> index 0000000..f7c66ec
> --- /dev/null
> +++ b/arch/arm/plat-s5p/include/plat/camera.h

How about camport.h?

> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd.
> + *
> + * S5P series camera interface helper functions
> + *
> + * 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_CAMERA_H_
> +#define PLAT_S5P_CAMERA_H_ __FILE__
> +
> +enum s5p_camif_id {
> +	S5P_CAMIF_A,
> +	S5P_CAMIF_B,
> +};

Because I think, "CAMPORT" is more clearly.

enum s5p_camport_id {
	S5P_CAMPORT_A,
	S5P_CAMPORT_B,
};


> +
> +/**
> + * s5pvX10_camif_cfg_gpio - configure IO pins of the camera A/B interface
> + * @id: id of a camera gpio interface
> + */
> +int s5pv210_camif_cfg_gpio(enum s5p_camif_id id);
> +int s5pv310_camif_cfg_gpio(enum s5p_camif_id id);
> +
> +#endif /* PLAT_S5P_CAMERA_H_ */
> --



Thanks.

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

[PATCH] ARM: S5P: Add platform helpers for camera GPIO configuration

[Sylwester Nawrocki]

On 03/03/2011 08:36 AM, Kukjin Kim wrote:
> Sylwester Nawrocki wrote:
>>
>> Add functions for the parallel camera GPIO interface
>> configuration on S5PV210 and S5PV310 SoCs.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <s.nawrocki@samsung.com>
> 
> Hmm...typo?

Err, no. Looks like someone needs a glasses here..

> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
>> ---
>>  arch/arm/mach-s5pv210/Kconfig           |    5 +++
>>  arch/arm/mach-s5pv210/Makefile          |    1 +
>>  arch/arm/mach-s5pv210/setup-camera.c    |   53
>> +++++++++++++++++++++++++++++++
>>  arch/arm/mach-s5pv310/Kconfig           |    5 +++
>>  arch/arm/mach-s5pv310/Makefile          |    1 +
>>  arch/arm/mach-s5pv310/setup-camera.c    |   43 +++++++++++++++++++++++++
>>  arch/arm/plat-s5p/include/plat/camera.h |   26 +++++++++++++++
>>  7 files changed, 134 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-s5pv210/setup-camera.c
>>  create mode 100644 arch/arm/mach-s5pv310/setup-camera.c
>>  create mode 100644 arch/arm/plat-s5p/include/plat/camera.h
>>
>> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
>> index 53aabef..300993a 100644
>> --- a/arch/arm/mach-s5pv210/Kconfig
>> +++ b/arch/arm/mach-s5pv210/Kconfig
>> @@ -53,6 +53,11 @@ config S5PV210_SETUP_SDHCI_GPIO
>>  	help
>>  	  Common setup code for SDHCI gpio.
>>
>> +config S5PV210_SETUP_CAMERA
> 
> How about "S5PV210_SETUP_FIMC"?
> 
> As you know, it belong to FIMC block which is written in datasheet.
> And this naming is more reasonable like SETUP_I2C...

Yeah, makes sense. Will change that. 

> 
>> +	bool
>> +	help
>> +	  Common setup code for the camera interfaces.
>> +
>>  menu "S5PC110 Machines"
>>
>>  config MACH_AQUILA
>> diff --git a/arch/arm/mach-s5pv210/Makefile
> b/arch/arm/mach-s5pv210/Makefile
>> index ff1a0db..d6c9f0d 100644
>> --- a/arch/arm/mach-s5pv210/Makefile
>> +++ b/arch/arm/mach-s5pv210/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_MACH_TORBRECK)	+= mach-torbreck.o
>>  obj-y				+= dev-audio.o
>>  obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
>>
>> +obj-$(CONFIG_S5PV210_SETUP_CAMERA)	+= setup-camera.o
>>  obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
>>  obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
>>  obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
>> diff --git a/arch/arm/mach-s5pv210/setup-camera.c b/arch/arm/mach-
>> s5pv210/setup-camera.c
>> new file mode 100644
>> index 0000000..e13c354
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv210/setup-camera.c
> 
> So setup-fimc.c

Ok.

> 
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Copyright (C) 2011 Samsung Electronics Co., Ltd.
>> + *
>> + * S5PV310 camera interface GPIO configuration.
>> + *
>> + * 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/gpio.h>
>> +#include <plat/gpio-cfg.h>
>> +#include <plat/camera.h>
>> +
>> +/*
>> + * Configure the camera parallel bus pins. The parallel bus can be
>> multiplexed
>> + * with any FIMC entity. Even multiple FIMC entities are allowed to be
>> attached
>> + * to a particular (A or B) gpio interface. This function should be
> called
>> from
>> + * a board setup code.
>> + */
>> +int s5pv210_camif_cfg_gpio(enum s5p_camif_id id)
> 
> int s5pv210_fimc_cfg_gpio...
> 
Ok, that's fine with me.

>> +{
>> +	u32 gpio8, gpio5;
>> +	int ret;
>> +	int i = 5;
>> +
>> +	switch (id) {
>> +	case S5P_CAMIF_A:
>> +		gpio8 = S5PV210_GPE0(0);
>> +		gpio5 = S5PV210_GPE1(0);
>> +		break;
> 
> Blank line?
OK
> 
>> +	case S5P_CAMIF_B:
>> +		gpio8 = S5PV210_GPJ0(0);
>> +		gpio5 = S5PV210_GPJ1(0);
>> +		break;
> 
> Same...
> 
>> +	default:
>> +		WARN(1, "id: %d\n", id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = s3c_gpio_cfgall_range(gpio8, 8, S3C_GPIO_SFN(2),
>> +				    S3C_GPIO_PULL_UP);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = s3c_gpio_cfgall_range(gpio5, 5, S3C_GPIO_SFN(2),
>> +				    S3C_GPIO_PULL_UP);
> 
> Where is "return ret;" ?

No need, it's being checked below, int the "while" conditional.
> 
>> +
>> +	while (i-- && !ret)
>> +		ret = s5p_gpio_set_drvstr(S5PV210_GPE1(i),
>> +					  S5P_GPIO_DRVSTR_LV4);
> 
> Basically drive strength depends on each board. So I think, should be
> removed here.

Hmm, none of our boards works without this. And it might be hard to find
bug when someone forgets about setting gpio drive strength. I had some trouble
with this already. Without setting proper drive strength the image from sensor had
been black or the video capture was not working at all.
I think it's worth to add a parameter to the above function as it is expected
to fully configure the parallel camera bus pins. I prefer to have possibly
everything which touches the camera bus pins in there.

How about:

int s5pv210_fimc_setup_gpio(enum s5p_camport_id id, bool use_drvstr, 
			s5p_gpio_drvstr_t drvstr);

end then:
...
+
+	ret = s3c_gpio_cfgall_range(gpio5, 5, S3C_GPIO_SFN(2),
+				    S3C_GPIO_PULL_UP);
+	if (!use_drvstr)
+		return ret;
+
+	while (i-- && !ret)
+		ret = s5p_gpio_set_drvstr(S5PV210_GPE1(i), drvstr);
?

> 
>> +	return ret;
>> +}
>> diff --git a/arch/arm/mach-s5pv310/Kconfig b/arch/arm/mach-s5pv310/Kconfig
>> index b2a9acc..ccd1dc4 100644
>> --- a/arch/arm/mach-s5pv310/Kconfig
>> +++ b/arch/arm/mach-s5pv310/Kconfig
>> @@ -71,6 +71,11 @@ config S5PV310_DEV_SYSMMU
>>  	help
>>  	  Common setup code for SYSTEM MMU in S5PV310
>>
>> +config S5PV310_SETUP_CAMERA
> 
> S5PV310_SETUP_FIMC ?
OK.
> 
>> +	bool
>> +	help
>> +	  Common setup code for the camera interfaces.
>> +
>>  # machine support
>>
>>  menu "S5PC210 Machines"
>> diff --git a/arch/arm/mach-s5pv310/Makefile
> b/arch/arm/mach-s5pv310/Makefile
>> index 036fb38..c1d6577 100644
>> --- a/arch/arm/mach-s5pv310/Makefile
>> +++ b/arch/arm/mach-s5pv310/Makefile
>> @@ -32,6 +32,7 @@ obj-y					+=
> dev-audio.o
>>  obj-$(CONFIG_S5PV310_DEV_PD)		+= dev-pd.o
>>  obj-$(CONFIG_S5PV310_DEV_SYSMMU)	+= dev-sysmmu.o
>>
>> +obj-$(CONFIG_S5PV310_SETUP_CAMERA)	+= setup-camera.o
>>  obj-$(CONFIG_S5PV310_SETUP_I2C1)	+= setup-i2c1.o
>>  obj-$(CONFIG_S5PV310_SETUP_I2C2)	+= setup-i2c2.o
>>  obj-$(CONFIG_S5PV310_SETUP_I2C3)	+= setup-i2c3.o
>> diff --git a/arch/arm/mach-s5pv310/setup-camera.c b/arch/arm/mach-
>> s5pv310/setup-camera.c
>> new file mode 100644
>> index 0000000..8ab239e
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv310/setup-camera.c
> 
> Almost same.

For sure it's similar. I followed how setup-i2c[N].c are done. 
Should I merge camera port setup into setup-fimc.c under plat-s5p
and use preprocessor to switch between ARCHs? It doesn't seem right..

> 
> (snip)
> 
>> diff --git a/arch/arm/plat-s5p/include/plat/camera.h b/arch/arm/plat-
>> s5p/include/plat/camera.h
>> new file mode 100644
>> index 0000000..f7c66ec
>> --- /dev/null
>> +++ b/arch/arm/plat-s5p/include/plat/camera.h
> 
> How about camport.h?

Yeah, sounds better.

> 
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (C) 2011 Samsung Electronics Co., Ltd.
>> + *
>> + * S5P series camera interface helper functions
>> + *
>> + * 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_CAMERA_H_
>> +#define PLAT_S5P_CAMERA_H_ __FILE__
>> +
>> +enum s5p_camif_id {
>> +	S5P_CAMIF_A,
>> +	S5P_CAMIF_B,
>> +};
> 
> Because I think, "CAMPORT" is more clearly.
> 
> enum s5p_camport_id {
> 	S5P_CAMPORT_A,
> 	S5P_CAMPORT_B,
> };
>
 
Ok, agreed.

> 
>> +
>> +/**
>> + * s5pvX10_camif_cfg_gpio - configure IO pins of the camera A/B interface
>> + * @id: id of a camera gpio interface
>> + */
>> +int s5pv210_camif_cfg_gpio(enum s5p_camif_id id);
>> +int s5pv310_camif_cfg_gpio(enum s5p_camif_id id);
>> +
>> +#endif /* PLAT_S5P_CAMERA_H_ */
>> --

Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH v2] ARM: S5P: Add platform helpers for camera GPIO configuration

[Sylwester Nawrocki]

Add functions for configuration of the parallel camera
bus pins on S5PV210 and Exynos4 SoC.

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

Hello,

this is a corrected version of platform helpers for the peripheral
parallel camera bus configuration in S5PV210 and Exynos4 SoCs.

Changes since v1:
- Renamed files: camera.h/camport.h, setup-camera.c/setup-fimc.c
- Replaced "camif" with "fimc" in function and data type names
- Removed driver strength setup (it turned out than only the clock
  source pin needed maximum driver strength on the test boards, 
  so I've moved its configuration to the board code)


Regards,
Sylwester Nawrocki
---
 arch/arm/mach-exynos4/Kconfig            |    5 +++
 arch/arm/mach-exynos4/Makefile           |    1 +
 arch/arm/mach-exynos4/setup-fimc.c       |   44 ++++++++++++++++++++++++++++++
 arch/arm/mach-s5pv210/Kconfig            |    5 +++
 arch/arm/mach-s5pv210/Makefile           |    1 +
 arch/arm/mach-s5pv210/setup-fimc.c       |   43 +++++++++++++++++++++++++++++
 arch/arm/plat-s5p/include/plat/camport.h |   28 +++++++++++++++++++
 7 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-exynos4/setup-fimc.c
 create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
 create mode 100644 arch/arm/plat-s5p/include/plat/camport.h

diff --git a/arch/arm/mach-exynos4/Kconfig b/arch/arm/mach-exynos4/Kconfig
index ad03840..cfe8c33 100644
--- a/arch/arm/mach-exynos4/Kconfig
+++ b/arch/arm/mach-exynos4/Kconfig
@@ -71,6 +71,11 @@ config EXYNOS4_SETUP_SDHCI_GPIO
 	help
 	  Common setup code for SDHCI gpio.
 
+config EXYNOS4_SETUP_FIMC
+	bool
+	help
+	  Common setup code for the camera interfaces.
+
 # machine support
 
 menu "EXYNOS4 Machines"
diff --git a/arch/arm/mach-exynos4/Makefile b/arch/arm/mach-exynos4/Makefile
index 0558235..57770d6 100644
--- a/arch/arm/mach-exynos4/Makefile
+++ b/arch/arm/mach-exynos4/Makefile
@@ -32,6 +32,7 @@ obj-y					+= dev-audio.o
 obj-$(CONFIG_EXYNOS4_DEV_PD)		+= dev-pd.o
 obj-$(CONFIG_EXYNOS4_DEV_SYSMMU)	+= dev-sysmmu.o
 
+obj-$(CONFIG_EXYNOS4_SETUP_FIMC)	+= setup-fimc.o
 obj-$(CONFIG_EXYNOS4_SETUP_I2C1)	+= setup-i2c1.o
 obj-$(CONFIG_EXYNOS4_SETUP_I2C2)	+= setup-i2c2.o
 obj-$(CONFIG_EXYNOS4_SETUP_I2C3)	+= setup-i2c3.o
diff --git a/arch/arm/mach-exynos4/setup-fimc.c b/arch/arm/mach-exynos4/setup-fimc.c
new file mode 100644
index 0000000..6a45078
--- /dev/null
+++ b/arch/arm/mach-exynos4/setup-fimc.c
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * Exynos4 camera interface GPIO configuration.
+ *
+ * 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/gpio.h>
+#include <plat/gpio-cfg.h>
+#include <plat/camport.h>
+
+int exynos4_fimc_setup_gpio(enum s5p_camport_id id)
+{
+	u32 gpio8, gpio5;
+	u32 sfn;
+	int ret;
+
+	switch (id) {
+	case S5P_CAMPORT_A:
+		gpio8 = EXYNOS4_GPJ0(0); /* PCLK, VSYNC, HREF, DATA[0:4] */
+		gpio5 = EXYNOS4_GPJ1(0); /* DATA[5:7], CLKOUT, FIELD */
+		sfn = S3C_GPIO_SFN(2);
+		break;
+
+	case S5P_CAMPORT_B:
+		gpio8 = EXYNOS4_GPE0(0); /* DATA[0:7] */
+		gpio5 = EXYNOS4_GPE1(0); /* PCLK, VSYNC, HREF, CLKOUT, FIELD */
+		sfn = S3C_GPIO_SFN(3);
+		break;
+
+	default:
+		WARN(1, "Wrong camport id: %d\n", id);
+		return -EINVAL;
+	}
+
+	ret = s3c_gpio_cfgall_range(gpio8, 8, sfn, S3C_GPIO_PULL_UP);
+	if (ret)
+		return ret;
+
+	return s3c_gpio_cfgall_range(gpio5, 5, sfn, S3C_GPIO_PULL_UP);
+}
diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 53aabef..3b6d0d4 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -53,6 +53,11 @@ config S5PV210_SETUP_SDHCI_GPIO
 	help
 	  Common setup code for SDHCI gpio.
 
+config S5PV210_SETUP_FIMC
+	bool
+	help
+	  Common setup code for the camera interfaces.
+
 menu "S5PC110 Machines"
 
 config MACH_AQUILA
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index ff1a0db..11f1790 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -31,6 +31,7 @@ obj-y				+= dev-audio.o
 obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
 
 obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
+obj-$(CONFIG_S5PV210_SETUP_FIMC)	+= setup-fimc.o
 obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
 obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
 obj-$(CONFIG_S5PV210_SETUP_IDE)		+= setup-ide.o
diff --git a/arch/arm/mach-s5pv210/setup-fimc.c b/arch/arm/mach-s5pv210/setup-fimc.c
new file mode 100644
index 0000000..54cc5b1
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-fimc.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * S5PV210 camera interface GPIO configuration.
+ *
+ * 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/gpio.h>
+#include <plat/gpio-cfg.h>
+#include <plat/camport.h>
+
+int s5pv210_fimc_setup_gpio(enum s5p_camport_id id)
+{
+	u32 gpio8, gpio5;
+	int ret;
+
+	switch (id) {
+	case S5P_CAMPORT_A:
+		gpio8 = S5PV210_GPE0(0);
+		gpio5 = S5PV210_GPE1(0);
+		break;
+
+	case S5P_CAMPORT_B:
+		gpio8 = S5PV210_GPJ0(0);
+		gpio5 = S5PV210_GPJ1(0);
+		break;
+
+	default:
+		WARN(1, "Wrong camport id: %d\n", id);
+		return -EINVAL;
+	}
+
+	ret = s3c_gpio_cfgall_range(gpio8, 8, S3C_GPIO_SFN(2),
+				    S3C_GPIO_PULL_UP);
+	if (ret)
+		return ret;
+
+	return s3c_gpio_cfgall_range(gpio5, 5, S3C_GPIO_SFN(2),
+				     S3C_GPIO_PULL_UP);
+}
diff --git a/arch/arm/plat-s5p/include/plat/camport.h b/arch/arm/plat-s5p/include/plat/camport.h
new file mode 100644
index 0000000..71688c8
--- /dev/null
+++ b/arch/arm/plat-s5p/include/plat/camport.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * S5P series camera interface helper functions
+ *
+ * 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_CAMPORT_H_
+#define PLAT_S5P_CAMPORT_H_ __FILE__
+
+enum s5p_camport_id {
+	S5P_CAMPORT_A,
+	S5P_CAMPORT_B,
+};
+
+/*
+ * The helper functions to configure GPIO for the camera parallel bus.
+ * The camera port can be multiplexed with any FIMC entity, even multiple
+ * FIMC entities are allowed to be attached to a single port simultaneously.
+ * These functions are to be used in the board setup code.
+ */
+int s5pv210_fimc_setup_gpio(enum s5p_camport_id id);
+int exynos4_fimc_setup_gpio(enum s5p_camport_id id);
+
+#endif
-- 
1.7.4.1

[PATCH v2] ARM: S5P: Add platform helpers for camera GPIO configuration

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Add functions for configuration of the parallel camera
> bus pins on S5PV210 and Exynos4 SoC.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> Hello,
> 
> this is a corrected version of platform helpers for the peripheral
> parallel camera bus configuration in S5PV210 and Exynos4 SoCs.
> 
> Changes since v1:
> - Renamed files: camera.h/camport.h, setup-camera.c/setup-fimc.c
> - Replaced "camif" with "fimc" in function and data type names
> - Removed driver strength setup (it turned out than only the clock
>   source pin needed maximum driver strength on the test boards,
>   so I've moved its configuration to the board code)
> 
> 
> Regards,
> Sylwester Nawrocki
> ---
>  arch/arm/mach-exynos4/Kconfig            |    5 +++
>  arch/arm/mach-exynos4/Makefile           |    1 +
>  arch/arm/mach-exynos4/setup-fimc.c       |   44
> ++++++++++++++++++++++++++++++
>  arch/arm/mach-s5pv210/Kconfig            |    5 +++
>  arch/arm/mach-s5pv210/Makefile           |    1 +
>  arch/arm/mach-s5pv210/setup-fimc.c       |   43
> +++++++++++++++++++++++++++++
>  arch/arm/plat-s5p/include/plat/camport.h |   28 +++++++++++++++++++
>  7 files changed, 127 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-exynos4/setup-fimc.c
>  create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
>  create mode 100644 arch/arm/plat-s5p/include/plat/camport.h
> 
(snip)

Looks ok to me, will apply :)
Thanks.

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