On Thu, 22 Sep 2011, Guennadi wrote:

On Thu, 22 Sep 2011, Josh Wu wrote:

quoted

This patch
1. add ISI_MCK parent setting code when add ISI device.
2. add ov2640 support on board file.
3. define isi_mck clock in sam9g45 chip file.

Signed-off-by: Josh Wu <redacted>
---
 arch/arm/mach-at91/at91sam9g45.c         |    3 +
 arch/arm/mach-at91/at91sam9g45_devices.c |  105 +++++++++++++++++++++++++++++-
 arch/arm/mach-at91/board-sam9m10g45ek.c  |   85 ++++++++++++++++++++++++-
 arch/arm/mach-at91/include/mach/board.h  |    3 +-

Personally, I think, it would be better to separate this into two patches 
at least: one for at91 core and one for the specific board, but that's up 
to arch maintainers to decide.

You also want to patch arch/arm/mach-at91/at91sam9263_devices.c, don't 
you?

As discussed in mail list, it will really break at91sam9263_devices.c's compilation.
So I will fix this in 9263_device.c, and merge this fix with mach-at91/include/mach/board.h change into one patch.
And other files as another patch.

quoted

 4 files changed, 193 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index e04c5fb..5e23d6d 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c

@@ -201,6 +201,7 @@ static struct clk *periph_clocks[] __initdata = {
 	// irq0
 };
 
+static struct clk pck1;

Hm, it really doesn't need any initialisation, not even for the .type 
field? .type=0 doesn't seem to be valid.

This line is only a forward declaration. Since the real definition is behind the code we use it.
It defined in later lines:

static struct clk pck1 = {
         .name           = "pck1",
         .pmc_mask       = AT91_PMC_PCK1,
         .type           = CLK_TYPE_PROGRAMMABLE,
         .id             = 1,
};

quoted

 static struct clk_lookup periph_clocks_lookups[] = {
 	/* One additional fake clock for ohci */
 	CLKDEV_CON_ID("ohci_clk", &uhphs_clk),

@@ -215,6 +216,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
 	CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
 	CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+	/* ISI_MCK, which is provided by programmable clock(PCK1) */
+	CLKDEV_CON_DEV_ID("isi_mck", "atmel_isi.0", &pck1),
 };
 
 static struct clk_lookup usart_clocks_lookups[] = {

diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 600bffb..82eeac8 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c

@@ -16,7 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c-gpio.h>
 #include <linux/atmel-mci.h>
-
+#include <linux/clk.h>
 #include <linux/fb.h>
 #include <video/atmel_lcdc.h>
 

@@ -28,6 +28,8 @@
 #include <mach/at_hdmac.h>
 #include <mach/atmel-mci.h>
 
+#include <media/atmel-isi.h>
+
 #include "generic.h"
 
 

@@ -863,6 +865,107 @@ void __init at91_add_device_ac97(struct ac97c_platform_data *data)
 void __init at91_add_device_ac97(struct ac97c_platform_data *data) {}
 #endif
 
+/* --------------------------------------------------------------------
+ *  Image Sensor Interface
+ * -------------------------------------------------------------------- */
+#if defined(CONFIG_VIDEO_ATMEL_ISI) || defined(CONFIG_VIDEO_ATMEL_ISI_MODULE)
+static u64 isi_dmamask = DMA_BIT_MASK(32);
+static struct isi_platform_data isi_data;
+
+struct resource isi_resources[] = {
+	[0] = {
+		.start	= AT91SAM9G45_BASE_ISI,
+		.end	= AT91SAM9G45_BASE_ISI + SZ_16K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91SAM9G45_ID_ISI,
+		.end	= AT91SAM9G45_ID_ISI,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device at91sam9g45_isi_device = {
+	.name		= "atmel_isi",
+	.id		= 0,
+	.dev		= {
+			.dma_mask		= &isi_dmamask,
+			.coherent_dma_mask	= DMA_BIT_MASK(32),
+			.platform_data		= &isi_data,
+	},
+	.resource	= isi_resources,
+	.num_resources	= ARRAY_SIZE(isi_resources),
+};
+
+static int __init isi_set_clk_parent(void)
+{
+	struct clk *pck1;
+	struct clk *plla;
+	int ret;
+
+	/* ISI_MCK is supplied by PCK1 - set parent for it. */
+	pck1 = clk_get(NULL, "pck1");
+	if (IS_ERR(pck1)) {
+		printk(KERN_ERR "Failed to get PCK1\n");
+		ret = PTR_ERR(pck1);
+		goto err;
+	}
+
+	plla = clk_get(NULL, "plla");
+	if (IS_ERR(plla)) {
+		printk(KERN_ERR "Failed to get PLLA\n");
+		ret = PTR_ERR(plla);
+		goto err_pck1;
+	}
+	ret = clk_set_parent(pck1, plla);
+	clk_put(plla);
+	if (ret != 0) {
+		printk(KERN_ERR "Failed to set PCK1 parent\n");
+		goto err_pck1;
+	}
+	return ret;
+
+err_pck1:
+	clk_put(pck1);
+err:
+	return ret;
+}
+
+void __init at91_add_device_isi(struct isi_platform_data * data)
+{
+	if (!data)
+		return;
+	isi_data = *data;
+
+	at91_set_A_periph(AT91_PIN_PB20, 0);	/* ISI_D0 */
+	at91_set_A_periph(AT91_PIN_PB21, 0);	/* ISI_D1 */
+	at91_set_A_periph(AT91_PIN_PB22, 0);	/* ISI_D2 */
+	at91_set_A_periph(AT91_PIN_PB23, 0);	/* ISI_D3 */
+	at91_set_A_periph(AT91_PIN_PB24, 0);	/* ISI_D4 */
+	at91_set_A_periph(AT91_PIN_PB25, 0);	/* ISI_D5 */
+	at91_set_A_periph(AT91_PIN_PB26, 0);	/* ISI_D6 */
+	at91_set_A_periph(AT91_PIN_PB27, 0);	/* ISI_D7 */
+	at91_set_A_periph(AT91_PIN_PB28, 0);	/* ISI_PCK */
+	at91_set_A_periph(AT91_PIN_PB30, 0);	/* ISI_HSYNC */
+	at91_set_A_periph(AT91_PIN_PB29, 0);	/* ISI_VSYNC */
+	at91_set_B_periph(AT91_PIN_PB31, 0);	/* ISI_MCK (PCK1) */
+	at91_set_B_periph(AT91_PIN_PB8, 0);	/* ISI_PD8 */
+	at91_set_B_periph(AT91_PIN_PB9, 0);	/* ISI_PD9 */
+	at91_set_B_periph(AT91_PIN_PB10, 0);	/* ISI_PD10 */
+	at91_set_B_periph(AT91_PIN_PB11, 0);	/* ISI_PD11 */
+
+	platform_device_register(&at91sam9g45_isi_device);
+
+	if (isi_set_clk_parent())
+		printk(KERN_ERR "Fail to set parent for ISI_MCK.\n");
+
+	return;
+}
+#else
+static int __init isi_set_clk_parent(void) { }
+void __init at91_add_device_isi(struct isi_platform_data * data) { }
+#endif
+
 
 /* --------------------------------------------------------------------
  *  LCD Controller

diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
index ad234cc..d5293fb 100644
--- a/arch/arm/mach-at91/board-sam9m10g45ek.c
+++ b/arch/arm/mach-at91/board-sam9m10g45ek.c

@@ -23,11 +23,13 @@
 #include <linux/gpio_keys.h>
 #include <linux/input.h>
 #include <linux/leds.h>
-#include <linux/clk.h>
 #include <linux/atmel-mci.h>
+#include <linux/delay.h>
 
 #include <mach/hardware.h>
 #include <video/atmel_lcdc.h>
+#include <media/soc_camera.h>
+#include <media/atmel-isi.h>
 
 #include <asm/setup.h>
 #include <asm/mach-types.h>

@@ -185,6 +187,83 @@ static void __init ek_add_device_nand(void)
 	at91_add_device_nand(&ek_nand_data);
 }
 
+/*
+ *  ISI
+ */
+#if defined(CONFIG_VIDEO_ATMEL_ISI) || defined(CONFIG_VIDEO_ATMEL_ISI_MODULE)
+static struct isi_platform_data __initdata isi_data = {
+	.frate		= ISI_CFG1_FRATE_CAPTURE_ALL,
+	.has_emb_sync	= 0,
+	.emb_crc_sync = 0,
+	.hsync_act_low = 0,
+	.vsync_act_low = 0,
+	.pclk_act_falling = 0,

You don't need all the "= 0" assignments above - all uninitialised fields 
will be = 0. Also, if you have started aligning assignments, as in .frate 
and .has_emb_sync, would be better to align all of them.

Sure, I will align those lines.
But I think this "= 0" will make code more readable. It is better not to ignore it. For example, ".has_emb_sync = 0" means we are NOT "has embedded sync".

quoted

+	/* to use codec and preview path simultaneously */
+	.isi_full_mode = 1,
+	.data_width_flags = ISI_DATAWIDTH_8 | ISI_DATAWIDTH_10,
+	/* ISI_MCK is provided by PCK1  */
+	.isi_mck_hz = 25000000,
+};
+
+#else
+static struct isi_platform_data __initdata isi_data;

Hmm, that doesn't help a lot, does it? Either do not allocate isi_data if 
ISI is not selected in .config, or just remove the "#if" completely.

I prefer to remove the "#if" completely. 

quoted

+#endif
+
+/*
+ * soc-camera OV2640
+ */
+#if defined(CONFIG_SOC_CAMERA_OV2640)

... || defined(CONFIG_SOC_CAMERA_OV2640_MODULE)

sorry, I missed this. 

quoted

+static unsigned long isi_camera_query_bus_param(struct soc_camera_link *link)
+{
+	/* ISI board for ek using default 8-bits connection */
+	return SOCAM_DATAWIDTH_8;
+}
+
+static int i2c_camera_power(struct device *dev, int on)
+{
+	/* enable or disable the camera */
+	pr_debug("%s: %s the camera\n", __func__, on ? "ENABLE" : "DISABLE");
+	at91_set_gpio_output(AT91_PIN_PD13, on ? 0 : 1);

maybe just

at91_set_gpio_output(AT91_PIN_PD13, !on);

I'll change it. It is simpler.

quoted

+
+	if (!on)
+		goto out;
+
+	/* If enabled, give a reset impulse */
+	at91_set_gpio_output(AT91_PIN_PD12, 0);
+	msleep(20);
+	at91_set_gpio_output(AT91_PIN_PD12, 1);
+	msleep(100);
+
+out:
+	return 0;
+}
+
+static struct i2c_board_info i2c_camera = {
+	I2C_BOARD_INFO("ov2640", 0x30),
+};
+
+static struct soc_camera_link iclink_ov2640 = {
+	.bus_id		= 0,
+	.board_info	= &i2c_camera,
+	.i2c_adapter_id	= 0,
+	.power		= i2c_camera_power,
+	.query_bus_param	= isi_camera_query_bus_param,

You could as well make this alignment look better.

Sure. 

quoted

+};
+
+static struct platform_device isi_ov2640 = {
+	.name	= "soc-camera-pdrv",
+	.id	= 0,
+	.dev	= {
+		.platform_data = &iclink_ov2640,
+	},
+};
+
+static struct platform_device *devices[] __initdata = {
+	&isi_ov2640,
+};
+#else
+static struct platform_device *devices[] __initdata = {};
+#endif
 
 /*
  * LCD Controller

@@ -400,6 +479,10 @@ static void __init ek_board_init(void)
 	ek_add_device_nand();
 	/* I2C */
 	at91_add_device_i2c(0, NULL, 0);
+	/* ISI */
+	platform_add_devices(devices, ARRAY_SIZE(devices));

"devices" is a generic name, but you make it depend on 
CONFIG_SOC_CAMERA_OV2640. Why don't you do

static struct platform_device *devices[] __initdata = {
#if defined(CONFIG_SOC_CAMERA_OV2640) || defined(CONFIG_SOC_CAMERA_OV2640_MODULE)
&isi_ov2640,
#endif
};

and move

+	platform_add_devices(devices, ARRAY_SIZE(devices));

out of the /* ISI */ section to a more generic location?

Yes, make sense to me.

quoted

+	at91_add_device_isi(&isi_data);
+
 	/* LCD Controller */
 	at91_add_device_lcdc(&ek_lcdc_data);
 	/* Touch Screen */

diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index ed544a0..276d63a 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h

@@ -183,7 +183,8 @@ extern void __init at91_add_device_lcdc(struct atmel_lcdfb_info *data);
 extern void __init at91_add_device_ac97(struct ac97c_platform_data *data);
 
  /* ISI */
-extern void __init at91_add_device_isi(void);
+struct isi_platform_data;
+extern void __init at91_add_device_isi(struct isi_platform_data *data);
 
  /* Touchscreen Controller */
 struct at91_tsadcc_data {

-- 
1.6.3.3

Thanks
Guennadi

Best Regards,
Josh Wu