arm: omap3evm: Add support for an MT9M032 based camera board.

PATCH Superseded Martin Hostettler 3 participants 7 messages Dec 14, 2011 – Dec 15, 2011

Subsystems (4)

Arm (32-bit) Media (V4L/DVB) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Martin Hostettler

author

2011-12-14 01:25 UTC #0 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Adds board support for an MT9M032 based camera to omap3evm.

Signed-off-by Martin Hostettler <[email protected]>

---
 arch/arm/mach-omap2/Makefile                |    3 +-
 arch/arm/mach-omap2/board-omap3evm-camera.c |  155 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-omap3evm.c        |    4 +
 3 files changed, 161 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/board-omap3evm-camera.c

Changes in V3
* Added missing copyright and attribution.
* switched to gpio_request_array for gpio init.
* removed device_initcall and added call to omap3_evm_camera_init into omap3_evm_init

Changes in V2:
* ported to current mainline
* Style fixes
* Fix error handling

arch/arm/mach-omap2/Makefile +2 -1

index b009f17..6045789 100644
@@ -196,7 +196,8 @@ obj-$(CONFIG_MACH_OMAP3530_LV_SOM)      += board-omap3logic.o
 obj-$(CONFIG_MACH_OMAP3_TORPEDO)        += board-omap3logic.o
 obj-$(CONFIG_MACH_ENCORE)		+= board-omap3encore.o
 obj-$(CONFIG_MACH_OVERO)		+= board-overo.o
-obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o
+obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o \
+					   board-omap3evm-camera.o
 obj-$(CONFIG_MACH_OMAP3_PANDORA)	+= board-omap3pandora.o
 obj-$(CONFIG_MACH_OMAP_3430SDP)		+= board-3430sdp.o
 obj-$(CONFIG_MACH_NOKIA_N8X0)		+= board-n8x0.o

arch/arm/mach-omap2/board-omap3evm-camera.c new file +155 -0

155 lines not on this page · raw message

arch/arm/mach-omap2/board-omap3evm.c +4 -0

index ec00b2e..1b50539 100644
@@ -617,6 +617,8 @@ static struct gpio omap3_evm_ehci_gpios[] __initdata = {
 	{ OMAP3_EVM_EHCI_SELECT, GPIOF_OUT_INIT_LOW,   "select EHCI port" },
 };
 
+int omap3_evm_camera_init(void);
+
 static void __init omap3_evm_init(void)
 {
 	omap3_evm_get_revision();

@@ -672,6 +674,8 @@ static void __init omap3_evm_init(void)
 		pr_err("error setting wl12xx data\n");
 	platform_device_register(&omap3evm_wlan_regulator);
 #endif
+
+	omap3_evm_camera_init();
 }
 
 MACHINE_START(OMAP3EVM, "OMAP3 EVM")

Signature · 1 line

1.7.2.5

Igor Grinberg

2011-12-14 09:31 UTC #1 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Hi Martin,

Quoted 64 lines · Martin Hostettler

Adds board support for an MT9M032 based camera to omap3evm.

Signed-off-by Martin Hostettler <[email protected]>

---
 arch/arm/mach-omap2/Makefile                |    3 +-
 arch/arm/mach-omap2/board-omap3evm-camera.c |  155 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-omap3evm.c        |    4 +
 3 files changed, 161 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/board-omap3evm-camera.c

Changes in V3
* Added missing copyright and attribution.
* switched to gpio_request_array for gpio init.
* removed device_initcall and added call to omap3_evm_camera_init into omap3_evm_init

Changes in V2:
* ported to current mainline
* Style fixes
* Fix error handling

arch/arm/mach-omap2/Makefile +2 -1

index b009f17..6045789 100644
@@ -196,7 +196,8 @@ obj-$(CONFIG_MACH_OMAP3530_LV_SOM)      += board-omap3logic.o
 obj-$(CONFIG_MACH_OMAP3_TORPEDO)        += board-omap3logic.o
 obj-$(CONFIG_MACH_ENCORE)		+= board-omap3encore.o
 obj-$(CONFIG_MACH_OVERO)		+= board-overo.o
-obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o
+obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o \
+					   board-omap3evm-camera.o
 obj-$(CONFIG_MACH_OMAP3_PANDORA)	+= board-omap3pandora.o
 obj-$(CONFIG_MACH_OMAP_3430SDP)		+= board-3430sdp.o
 obj-$(CONFIG_MACH_NOKIA_N8X0)		+= board-n8x0.o

arch/arm/mach-omap2/board-omap3evm-camera.c new file +24 -0

new file mode 100644
index 0000000..bffd5b8
@@ -0,0 +1,155 @@
+/*
+ * Copyright (C) 2011 Texas Instruments Inc
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <[email protected]>
+ * Authors:
+ *    Vaibhav Hiremath <[email protected]>
+ *    Martin Hostettler <[email protected]>
+ *
+ * Board intregration for a MT9M032 camera connected to IMAGE_CONN and I2C Bus 2
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */

arch/arm/mach-omap2/board-omap3evm-camera.c line 34

Quoted 10 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +10 -0

+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio.h>
+#include <plat/mux.h>
+#include "mux.h"
+
+#include "../../../drivers/media/video/omap3isp/isp.h"

Laurent,
In one of the previous reviews, you stated:
"I'll probably split it and move the part required by board files to include/media/omap3isp.h".
Is there any progress on that?

Quoted 42 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +42 -0

+#include "media/mt9m032.h"
+
+#include "devices.h"
+
+#define EVM_TWL_GPIO_BASE OMAP_MAX_GPIO_LINES
+#define GPIO98_VID_DEC_RES	98
+#define nCAM_VD_SEL		157
+
+#define MT9M032_I2C_BUS_NUM	2
+
+
+enum omap3evmdc_mux {
+	MUX_TVP5146,
+	MUX_CAMERA_SENSOR,
+	MUX_EXP_CAMERA_SENSOR,
+};
+
+/**
+ * omap3evm_set_mux - Sets mux to enable signal routing to
+ *                           different peripherals present on new EVM board
+ * @mux_id: enum, mux id to enable
+ *
+ * Returns 0 for success or a negative error code
+ */
+static int omap3evm_set_mux(enum omap3evmdc_mux mux_id)
+{
+	/* Set GPIO6 = 1 */
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 6, 1);
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+
+	switch (mux_id) {
+	case MUX_TVP5146:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 1);
+		break;
+
+	case MUX_CAMERA_SENSOR:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 0);
+		break;
+
+	case MUX_EXP_CAMERA_SENSOR:

arch/arm/mach-omap2/board-omap3evm-camera.c line 86

Quoted 10 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +10 -0

+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 1);
+		break;
+
+	default:
+		pr_err("omap3evm-camera: Invalid mux id #%d\n", mux_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}

I don't really care about that, but I don't see any mux being set in the above function so the name and comments are misleading.

Quoted 44 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +44 -0

+
+static struct mt9m032_platform_data mt9m032_platform_data = {
+	.ext_clock = 13500000,
+	.pll_pre_div = 6,
+	.pll_mul = 120,
+	.pll_out_div = 5,
+	.invert_pixclock = 1,
+};
+
+static struct i2c_board_info camera_i2c_devices[] = {
+	{
+		I2C_BOARD_INFO(MT9M032_NAME, MT9M032_I2C_ADDR),
+		.platform_data = &mt9m032_platform_data,
+	},
+};
+
+static struct isp_subdev_i2c_board_info camera_i2c_subdevs[] = {
+	{
+		.board_info = &camera_i2c_devices[0],
+		.i2c_adapter_id = MT9M032_I2C_BUS_NUM,
+	},
+	{},
+};
+
+static struct isp_v4l2_subdevs_group camera_subdevs[] = {
+	{
+		.subdevs = camera_i2c_subdevs,
+		.interface = ISP_INTERFACE_PARALLEL,
+		.bus = {
+			.parallel = {
+				.data_lane_shift = 1,
+				.clk_pol = 0,
+				.bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
+			}
+		},
+	},
+	{},
+};
+
+static struct isp_platform_data isp_platform_data = {
+	.subdevs = camera_subdevs,
+};
+
+

arch/arm/mach-omap2/board-omap3evm-camera.c line 140

Quoted 10 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +10 -0

+static struct gpio setup_gpios[] = {
+	{ nCAM_VD_SEL,           GPIOF_OUT_INIT_HIGH, "nCAM_VD_SEL" },
+	{ EVM_TWL_GPIO_BASE + 2, GPIOF_OUT_INIT_LOW,  "T2_GPIO2" },
+	{ EVM_TWL_GPIO_BASE + 8, GPIOF_OUT_INIT_LOW, "nCAM_VD_EN" },
+};
+
+
+int __init omap3_evm_camera_init(void)
+{
+	int ret = -EINVAL;

No need to initialize the variable, as it is done three line below.

arch/arm/mach-omap2/board-omap3evm-camera.c line 147

Quoted 7 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0

+
+	omap_mux_init_gpio(nCAM_VD_SEL, OMAP_PIN_OUTPUT);
+	ret = gpio_request_array(setup_gpios, ARRAY_SIZE(setup_gpios));
+	if (ret < 0) {
+		pr_err("omap3evm-camera: Failed to setup camera signal routing.\n");
+		return ret;
+	}

It looks like both above calls (gpio_request and mux_init) can be moved to omap3evm_set_mux() function (or a renamed version of it), so all the GPIO stuff will be close to each other instead of requesting in one place and playing with values in another...

arch/arm/mach-omap2/board-omap3evm-camera.c line 148

Quoted 1 line

arch/arm/mach-omap2/board-omap3evm-camera.c new file +1 -0
+	omap3evm_set_mux(MUX_CAMERA_SENSOR);

So the plan is to add support for the 3 types,
but hard code to only one?
Can't this be runtime detected somehow?

Quoted 29 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0

+	ret = omap3_init_camera(&isp_platform_data);
+	if (ret < 0) {
+		gpio_free_array(setup_gpios, ARRAY_SIZE(setup_gpios));
+		return ret;
+	}
+	return 0;
+}

arch/arm/mach-omap2/board-omap3evm.c +4 -0

index ec00b2e..1b50539 100644
@@ -617,6 +617,8 @@ static struct gpio omap3_evm_ehci_gpios[] __initdata = {
 	{ OMAP3_EVM_EHCI_SELECT, GPIOF_OUT_INIT_LOW,   "select EHCI port" },
 };
 
+int omap3_evm_camera_init(void);
+
 static void __init omap3_evm_init(void)
 {
 	omap3_evm_get_revision();

@@ -672,6 +674,8 @@ static void __init omap3_evm_init(void)
 		pr_err("error setting wl12xx data\n");
 	platform_device_register(&omap3evm_wlan_regulator);
 #endif
+
+	omap3_evm_camera_init();
 }
 
 MACHINE_START(OMAP3EVM, "OMAP3 EVM")

Signature · 2 lines

Regards,
Igor.

Laurent Pinchart

2011-12-14 13:15 UTC #2 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Hi Igor,

Quoted 4 lines · On Wednesday 14 December 2011 10:31:35 Igor Grinberg wrote:

Quoted 3 lines · Martin Hostettler
Adds board support for an MT9M032 based camera to omap3evm.
Signed-off-by Martin Hostettler <[email protected]>

[snip]

Quoted 6 lines

Quoted 6 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/board-omap3evm-camera.c new file mode 100644 index 0000000..bffd5b8
arch/arm/mach-omap2/board-omap3evm-camera.c new file +0 -0
@@ -0,0 +1,155 @@

[snip]

Quoted 7 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0

+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio.h>
+#include <plat/mux.h>
+#include "mux.h"

Quoted 8 lines

arch/arm/mach-omap2/board-omap3evm-camera.c
Quoted 2 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +2 -0
+
+#include "../../../drivers/media/video/omap3isp/isp.h"
Laurent,
In one of the previous reviews, you stated:
"I'll probably split it and move the part required by board files to include/media/omap3isp.h".
Is there any progress on that?

Yes, it has been half-fixed in mainline. Half only because all the structures and macros that should be used by board code are now in <media/omap3isp.h>, but some boards need to access OMAP3 ISP internals from board code, which still requires drivers/media/video/omap3isp/isp.h. This will eventually be fixed, when the generic struct clk object will be available.

After a quick look at this patch it seems that <media/omap3isp.h> should be enough here.

Quoted 1 line

Quoted 1 line
+#include "media/mt9m032.h"

And this should be <media/mt9m032.h>

Quoted 1 line

Quoted 1 line
+#include "devices.h"

Signature · 3 lines

Regards,

Laurent Pinchart

martin at neutronstar.dyndns.org

author

2011-12-14 18:22 UTC #3 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Quoted 35 lines · Laurent Pinchart

Hi Igor,
Quoted 4 lines · On Wednesday 14 December 2011 10:31:35 Igor Grinberg wrote:
Quoted 3 lines · Martin Hostettler
Adds board support for an MT9M032 based camera to omap3evm.
Signed-off-by Martin Hostettler <[email protected]>
[snip]
Quoted 6 lines
Quoted 6 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/board-omap3evm-camera.c new file mode 100644 index 0000000..bffd5b8
arch/arm/mach-omap2/board-omap3evm-camera.c new file +0 -0
@@ -0,0 +1,155 @@
[snip]
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio.h>
+#include <plat/mux.h>
+#include "mux.h"
Quoted 8 lines
arch/arm/mach-omap2/board-omap3evm-camera.c
Quoted 2 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +2 -0
+
+#include "../../../drivers/media/video/omap3isp/isp.h"
Laurent,
In one of the previous reviews, you stated:
"I'll probably split it and move the part required by board files to include/media/omap3isp.h".
Is there any progress on that?

Quoted 8 lines

Yes, it has been half-fixed in mainline. Half only because all the structures and macros that should be used by board code are now in <media/omap3isp.h>, but some boards need to access OMAP3 ISP internals from board code, which still requires drivers/media/video/omap3isp/isp.h. This will eventually be fixed, when the generic struct clk object will be available.
After a quick look at this patch it seems that <media/omap3isp.h> should be enough here.

Almost. The code uses ISPCTRL_PAR_BRIDGE_DISABLE which is only available from drivers/media/video/omap3isp/ispreg.h.

So i'd say it's better to keep that include than to duplicate this constant in the code.

What do you think?

By the way, it seems drivers/media/video/omap3isp/ispvideo.c is missing a #include <linux/module.h> at the moment. I had to patch that line in to get omap3isp to compile as module.

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
+#include "media/mt9m032.h"
And this should be <media/mt9m032.h>

I'll change this.

Regards,
- Martin Hostettler

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
+#include "devices.h"
Signature · 3 lines
Regards,

Laurent Pinchart

Laurent Pinchart

2011-12-15 12:02 UTC #4 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Hi Martin,

Quoted 44 lines · On Wednesday 14 December 2011 19:22:29 [email protected] wrote:

Quoted 40 lines · Laurent Pinchart
Quoted 4 lines · On Wednesday 14 December 2011 10:31:35 Igor Grinberg wrote:
Quoted 3 lines · Martin Hostettler
Adds board support for an MT9M032 based camera to omap3evm.
Signed-off-by Martin Hostettler <[email protected]>
[snip]
Quoted 6 lines
Quoted 6 lines
(quoted diff) +0 -0
b/arch/arm/mach-omap2/board-omap3evm-camera.c new file mode 100644 index 0000000..bffd5b8
arch/arm/mach-omap2/board-omap3evm-camera.c new file +0 -0
@@ -0,0 +1,155 @@
[snip]
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio.h>
+#include <plat/mux.h>
+#include "mux.h"
Quoted 8 lines
arch/arm/mach-omap2/board-omap3evm-camera.c
Quoted 2 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +2 -0
+
+#include "../../../drivers/media/video/omap3isp/isp.h"
Laurent,
In one of the previous reviews, you stated:
"I'll probably split it and move the part required by board files to include/media/omap3isp.h".
Is there any progress on that?
Yes, it has been half-fixed in mainline. Half only because all the structures and macros that should be used by board code are now in <media/omap3isp.h>, but some boards need to access OMAP3 ISP internals from board code, which still requires drivers/media/video/omap3isp/isp.h. This will eventually be fixed, when the generic struct clk object will be available.
Quoted 2 lines
After a quick look at this patch it seems that <media/omap3isp.h> should be enough here.

Quoted 7 lines

Almost. The code uses ISPCTRL_PAR_BRIDGE_DISABLE which is only available from drivers/media/video/omap3isp/ispreg.h.
So i'd say it's better to keep that include than to duplicate this constant in the code.
What do you think?

You should use ISP_BRIDGE_DISABLE instead. That one is defined in <media/omap3isp.h>

Quoted 3 lines

By the way, it seems drivers/media/video/omap3isp/ispvideo.c is missing a #include <linux/module.h> at the moment. I had to patch that line in to get omap3isp to compile as module.

http://patchwork.linuxtv.org/patch/8510/

The fix should get in v3.2.

Signature · 3 lines

Regards,

Laurent Pinchart

martin at neutronstar.dyndns.org

author

2011-12-14 18:14 UTC #5 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

In reply to Igor Grinberg

Quoted 132 lines · Igor Grinberg

Hi Martin,

Quoted 64 lines · Martin Hostettler

Adds board support for an MT9M032 based camera to omap3evm.

Signed-off-by Martin Hostettler <[email protected]>

---
 arch/arm/mach-omap2/Makefile                |    3 +-
 arch/arm/mach-omap2/board-omap3evm-camera.c |  155 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-omap3evm.c        |    4 +
 3 files changed, 161 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/board-omap3evm-camera.c

Changes in V3
* Added missing copyright and attribution.
* switched to gpio_request_array for gpio init.
* removed device_initcall and added call to omap3_evm_camera_init into omap3_evm_init

Changes in V2:
* ported to current mainline
* Style fixes
* Fix error handling

arch/arm/mach-omap2/Makefile +2 -1

index b009f17..6045789 100644
@@ -196,7 +196,8 @@ obj-$(CONFIG_MACH_OMAP3530_LV_SOM)      += board-omap3logic.o
 obj-$(CONFIG_MACH_OMAP3_TORPEDO)        += board-omap3logic.o
 obj-$(CONFIG_MACH_ENCORE)		+= board-omap3encore.o
 obj-$(CONFIG_MACH_OVERO)		+= board-overo.o
-obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o
+obj-$(CONFIG_MACH_OMAP3EVM)		+= board-omap3evm.o \
+					   board-omap3evm-camera.o
 obj-$(CONFIG_MACH_OMAP3_PANDORA)	+= board-omap3pandora.o
 obj-$(CONFIG_MACH_OMAP_3430SDP)		+= board-3430sdp.o
 obj-$(CONFIG_MACH_NOKIA_N8X0)		+= board-n8x0.o

arch/arm/mach-omap2/board-omap3evm-camera.c new file +24 -0

new file mode 100644
index 0000000..bffd5b8
@@ -0,0 +1,155 @@
+/*
+ * Copyright (C) 2011 Texas Instruments Inc
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <[email protected]>
+ * Authors:
+ *    Vaibhav Hiremath <[email protected]>
+ *    Martin Hostettler <[email protected]>
+ *
+ * Board intregration for a MT9M032 camera connected to IMAGE_CONN and I2C Bus 2
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */

arch/arm/mach-omap2/board-omap3evm-camera.c line 34

Quoted 10 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +10 -0

+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio.h>
+#include <plat/mux.h>
+#include "mux.h"
+
+#include "../../../drivers/media/video/omap3isp/isp.h"

Laurent,
In one of the previous reviews, you stated:
"I'll probably split it and move the part required by board files to include/media/omap3isp.h".
Is there any progress on that?

Quoted 48 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +48 -0

+#include "media/mt9m032.h"
+
+#include "devices.h"
+
+#define EVM_TWL_GPIO_BASE OMAP_MAX_GPIO_LINES
+#define GPIO98_VID_DEC_RES	98
+#define nCAM_VD_SEL		157
+
+#define MT9M032_I2C_BUS_NUM	2
+
+
+enum omap3evmdc_mux {
+	MUX_TVP5146,
+	MUX_CAMERA_SENSOR,
+	MUX_EXP_CAMERA_SENSOR,
+};
+
+/**
+ * omap3evm_set_mux - Sets mux to enable signal routing to
+ *                           different peripherals present on new EVM board
+ * @mux_id: enum, mux id to enable
+ *
+ * Returns 0 for success or a negative error code
+ */
+static int omap3evm_set_mux(enum omap3evmdc_mux mux_id)
+{
+	/* Set GPIO6 = 1 */
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 6, 1);
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+
+	switch (mux_id) {
+	case MUX_TVP5146:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 1);
+		break;
+
+	case MUX_CAMERA_SENSOR:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 0);
+		break;
+
+	case MUX_EXP_CAMERA_SENSOR:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 1);
+		break;
+
+	default:
+		pr_err("omap3evm-camera: Invalid mux id #%d\n", mux_id);
+		return -EINVAL;

Quoted 8 lines

arch/arm/mach-omap2/board-omap3evm-camera.c line 86
Quoted 4 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +4 -0
+	}
+
+	return 0;
+}
I don't really care about that, but I don't see any mux being set in the above function so the name and comments are misleading.

There's are video muxes on this board that's controlled by various gpios. It's not a mux in the omap chip if you've expected to see that.

As this is an evaluation board it has a bunch of video connectors that the user can choose from for different input devices.

Quoted 49 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +48 -0

+
+static struct mt9m032_platform_data mt9m032_platform_data = {
+	.ext_clock = 13500000,
+	.pll_pre_div = 6,
+	.pll_mul = 120,
+	.pll_out_div = 5,
+	.invert_pixclock = 1,
+};
+
+static struct i2c_board_info camera_i2c_devices[] = {
+	{
+		I2C_BOARD_INFO(MT9M032_NAME, MT9M032_I2C_ADDR),
+		.platform_data = &mt9m032_platform_data,
+	},
+};
+
+static struct isp_subdev_i2c_board_info camera_i2c_subdevs[] = {
+	{
+		.board_info = &camera_i2c_devices[0],
+		.i2c_adapter_id = MT9M032_I2C_BUS_NUM,
+	},
+	{},
+};
+
+static struct isp_v4l2_subdevs_group camera_subdevs[] = {
+	{
+		.subdevs = camera_i2c_subdevs,
+		.interface = ISP_INTERFACE_PARALLEL,
+		.bus = {
+			.parallel = {
+				.data_lane_shift = 1,
+				.clk_pol = 0,
+				.bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
+			}
+		},
+	},
+	{},
+};
+
+static struct isp_platform_data isp_platform_data = {
+	.subdevs = camera_subdevs,
+};
+
+
+static struct gpio setup_gpios[] = {
+	{ nCAM_VD_SEL,           GPIOF_OUT_INIT_HIGH, "nCAM_VD_SEL" },
+	{ EVM_TWL_GPIO_BASE + 2, GPIOF_OUT_INIT_LOW,  "T2_GPIO2" },
+	{ EVM_TWL_GPIO_BASE + 8, GPIOF_OUT_INIT_LOW, "nCAM_VD_EN" },

Quoted 8 lines

arch/arm/mach-omap2/board-omap3evm-camera.c line 140
Quoted 6 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +6 -0
+};
+
+
+int __init omap3_evm_camera_init(void)
+{
+	int ret = -EINVAL;
No need to initialize the variable, as it is done three line below.

Yes, i'll fix that.

Quoted 13 lines

arch/arm/mach-omap2/board-omap3evm-camera.c line 147
Quoted 8 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0
+
+	omap_mux_init_gpio(nCAM_VD_SEL, OMAP_PIN_OUTPUT);
+	ret = gpio_request_array(setup_gpios, ARRAY_SIZE(setup_gpios));
+	if (ret < 0) {
+		pr_err("omap3evm-camera: Failed to setup camera signal routing.\n");
+		return ret;
+	}
It looks like both above calls (gpio_request and mux_init) can be moved to omap3evm_set_mux() function (or a renamed version of it), so all the GPIO stuff will be close to each other instead of requesting in one place and playing with values in another...

I'd like to keep the one time setup and the theoretically run time switchable parts seperate. It doesn't complicate the code and if a brave soul wants to connect different camera modules and switch between them it's a more reviewable patch from here.

Quoted 6 lines

arch/arm/mach-omap2/board-omap3evm-camera.c line 148
Quoted 2 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +1 -0
+	omap3evm_set_mux(MUX_CAMERA_SENSOR);
So the plan is to add support for the 3 types,
but hard code to only one?
Can't this be runtime detected somehow?

The mux code came from out of tree drivers. I did want to keep enough information so someone extending this board code for other setups doesn't have a hard time. I can't think of an reliable way to runtime detect what video source a specific use case would want. Ideally someone who needs one of the other video sources should add a more generic solution here.

Quoted 29 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +7 -0

+	ret = omap3_init_camera(&isp_platform_data);
+	if (ret < 0) {
+		gpio_free_array(setup_gpios, ARRAY_SIZE(setup_gpios));
+		return ret;
+	}
+	return 0;
+}

arch/arm/mach-omap2/board-omap3evm.c +4 -0

index ec00b2e..1b50539 100644
@@ -617,6 +617,8 @@ static struct gpio omap3_evm_ehci_gpios[] __initdata = {
 	{ OMAP3_EVM_EHCI_SELECT, GPIOF_OUT_INIT_LOW,   "select EHCI port" },
 };
 
+int omap3_evm_camera_init(void);
+
 static void __init omap3_evm_init(void)
 {
 	omap3_evm_get_revision();

@@ -672,6 +674,8 @@ static void __init omap3_evm_init(void)
 		pr_err("error setting wl12xx data\n");
 	platform_device_register(&omap3evm_wlan_regulator);
 #endif
+
+	omap3_evm_camera_init();
 }

Quoted 2 lines

arch/arm/mach-omap2/board-omap3evm.c static void __init omap3_evm_init(void) · line 681
Quoted 1 line
arch/arm/mach-omap2/board-omap3evm.c +0 -0
 MACHINE_START(OMAP3EVM, "OMAP3 EVM")

- Martin

Quoted 3 lines

Signature · 2 lines
Regards,
Igor.

Igor Grinberg

2011-12-15 10:18 UTC #6 lore raw

PATCH v3 1/1 arm: omap3evm: Add support for an MT9M032 based camera board.

Quoted 24 lines · martin at neutronstar.dyndns.org

Quoted 23 lines · Igor Grinberg
Hi Martin,
Quoted 20 lines · Martin Hostettler
Adds board support for an MT9M032 based camera to omap3evm.
Signed-off-by Martin Hostettler <[email protected]>
---
 arch/arm/mach-omap2/Makefile                |    3 +-
 arch/arm/mach-omap2/board-omap3evm-camera.c |  155 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-omap3evm.c        |    4 +
 3 files changed, 161 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/board-omap3evm-camera.c
Changes in V3
* Added missing copyright and attribution.
* switched to gpio_request_array for gpio init.
* removed device_initcall and added call to omap3_evm_camera_init into omap3_evm_init
Changes in V2:
* ported to current mainline
* Style fixes
* Fix error handling

[...]

Quoted 40 lines

Quoted 31 lines

arch/arm/mach-omap2/board-omap3evm-camera.c new file +31 -0

+/**
+ * omap3evm_set_mux - Sets mux to enable signal routing to
+ *                           different peripherals present on new EVM board
+ * @mux_id: enum, mux id to enable
+ *
+ * Returns 0 for success or a negative error code
+ */
+static int omap3evm_set_mux(enum omap3evmdc_mux mux_id)
+{
+	/* Set GPIO6 = 1 */
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 6, 1);
+	gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+
+	switch (mux_id) {
+	case MUX_TVP5146:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 1);
+		break;
+
+	case MUX_CAMERA_SENSOR:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
+		gpio_set_value(nCAM_VD_SEL, 0);
+		break;
+
+	case MUX_EXP_CAMERA_SENSOR:
+		gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 1);
+		break;
+
+	default:
+		pr_err("omap3evm-camera: Invalid mux id #%d\n", mux_id);
+		return -EINVAL;

Quoted 8 lines

arch/arm/mach-omap2/board-omap3evm-camera.c line 86
Quoted 4 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +4 -0
+	}
+
+	return 0;
+}
I don't really care about that, but I don't see any mux being set in the above function so the name and comments are misleading.

Quoted 5 lines

There's are video muxes on this board that's controlled by various gpios. It's not a mux in the omap chip if you've expected to see that.
As this is an evaluation board it has a bunch of video connectors that the user can choose from for different input devices.

I see... Probably a comment explaining that would not hurt here.

[...]

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +4 -0
+
+	omap_mux_init_gpio(nCAM_VD_SEL, OMAP_PIN_OUTPUT);
+	ret = gpio_request_array(setup_gpios, ARRAY_SIZE(setup_gpios));
+	if (ret < 0) {
Quoted 8 lines
arch/arm/mach-omap2/board-omap3evm-camera.c line 147
Quoted 3 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +3 -0
+		pr_err("omap3evm-camera: Failed to setup camera signal routing.\n");
+		return ret;
+	}
It looks like both above calls (gpio_request and mux_init) can be moved to omap3evm_set_mux() function (or a renamed version of it), so all the GPIO stuff will be close to each other instead of requesting in one place and playing with values in another...

Quoted 4 lines

I'd like to keep the one time setup and the theoretically run time switchable parts seperate. It doesn't complicate the code and if a brave soul wants to connect different camera modules and switch between them it's a more reviewable patch from here.

Quoted 8 lines

Quoted 7 lines
arch/arm/mach-omap2/board-omap3evm-camera.c line 148
Quoted 3 lines
arch/arm/mach-omap2/board-omap3evm-camera.c new file +1 -0
+	omap3evm_set_mux(MUX_CAMERA_SENSOR);
So the plan is to add support for the 3 types,
but hard code to only one?
Can't this be runtime detected somehow?

Quoted 5 lines

The mux code came from out of tree drivers. I did want to keep enough information so someone extending this board code for other setups doesn't have a hard time. I can't think of an reliable way to runtime detect what video source a specific use case would want. Ideally someone who needs one of the other video sources should add a more generic solution here.

I'm Ok with it, but usually, stuff that is never used stays out... I think the way it should be is to have a platform driver that uses a callback to switch between the "muxes" on the extension.

[...]

Signature · 2 lines

Regards,
Igor.