All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies
@ 2011-05-26  0:04 achew
  2011-05-26  0:04 ` [PATCH 2/5 v2] [media] ov9740: Correct print in ov9740_reg_rmw() achew
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: achew @ 2011-05-26  0:04 UTC (permalink / raw)
  To: g.liakhovetski, mchehab, olof; +Cc: linux-media, linux-kernel, Andrew Chew

From: Andrew Chew <achew@nvidia.com>

Made all hex number casing use lower-case throughout the entire driver
for consistency.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/media/video/ov9740.c |  111 +++++++++++++++++++++---------------------
 1 files changed, 55 insertions(+), 56 deletions(-)

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 4d4ee4f..96811e4 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -44,12 +44,12 @@
 #define OV9740_Y_ADDR_START_LO		0x0347
 #define OV9740_X_ADDR_END_HI		0x0348
 #define OV9740_X_ADDR_END_LO		0x0349
-#define OV9740_Y_ADDR_END_HI		0x034A
-#define OV9740_Y_ADDR_END_LO		0x034B
-#define OV9740_X_OUTPUT_SIZE_HI		0x034C
-#define OV9740_X_OUTPUT_SIZE_LO		0x034D
-#define OV9740_Y_OUTPUT_SIZE_HI		0x034E
-#define OV9740_Y_OUTPUT_SIZE_LO		0x034F
+#define OV9740_Y_ADDR_END_HI		0x034a
+#define OV9740_Y_ADDR_END_LO		0x034b
+#define OV9740_X_OUTPUT_SIZE_HI		0x034c
+#define OV9740_X_OUTPUT_SIZE_LO		0x034d
+#define OV9740_Y_OUTPUT_SIZE_HI		0x034e
+#define OV9740_Y_OUTPUT_SIZE_LO		0x034f
 
 /* IO Control Registers */
 #define OV9740_IO_CREL00		0x3002
@@ -89,28 +89,28 @@
 #define OV9740_TIMING_CTRL35		0x3835
 
 /* Banding Filter */
-#define OV9740_AEC_MAXEXPO_60_H		0x3A02
-#define OV9740_AEC_MAXEXPO_60_L		0x3A03
-#define OV9740_AEC_B50_STEP_HI		0x3A08
-#define OV9740_AEC_B50_STEP_LO		0x3A09
-#define OV9740_AEC_B60_STEP_HI		0x3A0A
-#define OV9740_AEC_B60_STEP_LO		0x3A0B
-#define OV9740_AEC_CTRL0D		0x3A0D
-#define OV9740_AEC_CTRL0E		0x3A0E
-#define OV9740_AEC_MAXEXPO_50_H		0x3A14
-#define OV9740_AEC_MAXEXPO_50_L		0x3A15
+#define OV9740_AEC_MAXEXPO_60_H		0x3a02
+#define OV9740_AEC_MAXEXPO_60_L		0x3a03
+#define OV9740_AEC_B50_STEP_HI		0x3a08
+#define OV9740_AEC_B50_STEP_LO		0x3a09
+#define OV9740_AEC_B60_STEP_HI		0x3a0a
+#define OV9740_AEC_B60_STEP_LO		0x3a0b
+#define OV9740_AEC_CTRL0D		0x3a0d
+#define OV9740_AEC_CTRL0E		0x3a0e
+#define OV9740_AEC_MAXEXPO_50_H		0x3a14
+#define OV9740_AEC_MAXEXPO_50_L		0x3a15
 
 /* AEC/AGC Control */
 #define OV9740_AEC_ENABLE		0x3503
-#define OV9740_GAIN_CEILING_01		0x3A18
-#define OV9740_GAIN_CEILING_02		0x3A19
-#define OV9740_AEC_HI_THRESHOLD		0x3A11
-#define OV9740_AEC_3A1A			0x3A1A
-#define OV9740_AEC_CTRL1B_WPT2		0x3A1B
-#define OV9740_AEC_CTRL0F_WPT		0x3A0F
-#define OV9740_AEC_CTRL10_BPT		0x3A10
-#define OV9740_AEC_CTRL1E_BPT2		0x3A1E
-#define OV9740_AEC_LO_THRESHOLD		0x3A1F
+#define OV9740_GAIN_CEILING_01		0x3a18
+#define OV9740_GAIN_CEILING_02		0x3a19
+#define OV9740_AEC_HI_THRESHOLD		0x3a11
+#define OV9740_AEC_3A1A			0x3a1a
+#define OV9740_AEC_CTRL1B_WPT2		0x3a1b
+#define OV9740_AEC_CTRL0F_WPT		0x3a0f
+#define OV9740_AEC_CTRL10_BPT		0x3a10
+#define OV9740_AEC_CTRL1E_BPT2		0x3a1e
+#define OV9740_AEC_LO_THRESHOLD		0x3a1f
 
 /* BLC Control */
 #define OV9740_BLC_AUTO_ENABLE		0x4002
@@ -132,7 +132,7 @@
 #define OV9740_VT_SYS_CLK_DIV		0x0303
 #define OV9740_VT_PIX_CLK_DIV		0x0301
 #define OV9740_PLL_CTRL3010		0x3010
-#define OV9740_VFIFO_CTRL00		0x460E
+#define OV9740_VFIFO_CTRL00		0x460e
 
 /* ISP Control */
 #define OV9740_ISP_CTRL00		0x5000
@@ -141,9 +141,9 @@
 #define OV9740_ISP_CTRL05		0x5005
 #define OV9740_ISP_CTRL12		0x5012
 #define OV9740_ISP_CTRL19		0x5019
-#define OV9740_ISP_CTRL1A		0x501A
-#define OV9740_ISP_CTRL1E		0x501E
-#define OV9740_ISP_CTRL1F		0x501F
+#define OV9740_ISP_CTRL1A		0x501a
+#define OV9740_ISP_CTRL1E		0x501e
+#define OV9740_ISP_CTRL1F		0x501f
 #define OV9740_ISP_CTRL20		0x5020
 #define OV9740_ISP_CTRL21		0x5021
 
@@ -158,12 +158,12 @@
 #define OV9740_AWB_ADV_CTRL04		0x5187
 #define OV9740_AWB_ADV_CTRL05		0x5188
 #define OV9740_AWB_ADV_CTRL06		0x5189
-#define OV9740_AWB_ADV_CTRL07		0x518A
-#define OV9740_AWB_ADV_CTRL08		0x518B
-#define OV9740_AWB_ADV_CTRL09		0x518C
-#define OV9740_AWB_ADV_CTRL10		0x518D
-#define OV9740_AWB_ADV_CTRL11		0x518E
-#define OV9740_AWB_CTRL0F		0x518F
+#define OV9740_AWB_ADV_CTRL07		0x518a
+#define OV9740_AWB_ADV_CTRL08		0x518b
+#define OV9740_AWB_ADV_CTRL09		0x518c
+#define OV9740_AWB_ADV_CTRL10		0x518d
+#define OV9740_AWB_ADV_CTRL11		0x518e
+#define OV9740_AWB_CTRL0F		0x518f
 #define OV9740_AWB_CTRL10		0x5190
 #define OV9740_AWB_CTRL11		0x5191
 #define OV9740_AWB_CTRL12		0x5192
@@ -241,36 +241,36 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	/* Un-documented OV9740 registers */
 	{ 0x5800, 0x29 }, { 0x5801, 0x25 }, { 0x5802, 0x20 }, { 0x5803, 0x21 },
 	{ 0x5804, 0x26 }, { 0x5805, 0x2e }, { 0x5806, 0x11 }, { 0x5807, 0x0c },
-	{ 0x5808, 0x09 }, { 0x5809, 0x0a }, { 0x580A, 0x0e }, { 0x580B, 0x16 },
-	{ 0x580C, 0x06 }, { 0x580D, 0x02 }, { 0x580E, 0x00 }, { 0x580F, 0x00 },
+	{ 0x5808, 0x09 }, { 0x5809, 0x0a }, { 0x580a, 0x0e }, { 0x580b, 0x16 },
+	{ 0x580c, 0x06 }, { 0x580d, 0x02 }, { 0x580e, 0x00 }, { 0x580f, 0x00 },
 	{ 0x5810, 0x04 }, { 0x5811, 0x0a }, { 0x5812, 0x05 }, { 0x5813, 0x02 },
 	{ 0x5814, 0x00 }, { 0x5815, 0x00 }, { 0x5816, 0x03 }, { 0x5817, 0x09 },
-	{ 0x5818, 0x0f }, { 0x5819, 0x0a }, { 0x581A, 0x07 }, { 0x581B, 0x08 },
-	{ 0x581C, 0x0b }, { 0x581D, 0x14 }, { 0x581E, 0x28 }, { 0x581F, 0x23 },
+	{ 0x5818, 0x0f }, { 0x5819, 0x0a }, { 0x581a, 0x07 }, { 0x581b, 0x08 },
+	{ 0x581c, 0x0b }, { 0x581d, 0x14 }, { 0x581e, 0x28 }, { 0x581f, 0x23 },
 	{ 0x5820, 0x1d }, { 0x5821, 0x1e }, { 0x5822, 0x24 }, { 0x5823, 0x2a },
 	{ 0x5824, 0x4f }, { 0x5825, 0x6f }, { 0x5826, 0x5f }, { 0x5827, 0x7f },
-	{ 0x5828, 0x9f }, { 0x5829, 0x5f }, { 0x582A, 0x8f }, { 0x582B, 0x9e },
-	{ 0x582C, 0x8f }, { 0x582D, 0x9f }, { 0x582E, 0x4f }, { 0x582F, 0x87 },
+	{ 0x5828, 0x9f }, { 0x5829, 0x5f }, { 0x582a, 0x8f }, { 0x582b, 0x9e },
+	{ 0x582c, 0x8f }, { 0x582d, 0x9f }, { 0x582e, 0x4f }, { 0x582f, 0x87 },
 	{ 0x5830, 0x86 }, { 0x5831, 0x97 }, { 0x5832, 0xae }, { 0x5833, 0x3f },
 	{ 0x5834, 0x8e }, { 0x5835, 0x7c }, { 0x5836, 0x7e }, { 0x5837, 0xaf },
-	{ 0x5838, 0x8f }, { 0x5839, 0x8f }, { 0x583A, 0x9f }, { 0x583B, 0x7f },
-	{ 0x583C, 0x5f },
+	{ 0x5838, 0x8f }, { 0x5839, 0x8f }, { 0x583a, 0x9f }, { 0x583b, 0x7f },
+	{ 0x583c, 0x5f },
 
 	/* Y Gamma */
 	{ 0x5480, 0x07 }, { 0x5481, 0x18 }, { 0x5482, 0x2c }, { 0x5483, 0x4e },
 	{ 0x5484, 0x5e }, { 0x5485, 0x6b }, { 0x5486, 0x77 }, { 0x5487, 0x82 },
-	{ 0x5488, 0x8c }, { 0x5489, 0x95 }, { 0x548A, 0xa4 }, { 0x548B, 0xb1 },
-	{ 0x548C, 0xc6 }, { 0x548D, 0xd8 }, { 0x548E, 0xe9 },
+	{ 0x5488, 0x8c }, { 0x5489, 0x95 }, { 0x548a, 0xa4 }, { 0x548b, 0xb1 },
+	{ 0x548c, 0xc6 }, { 0x548d, 0xd8 }, { 0x548e, 0xe9 },
 
 	/* UV Gamma */
 	{ 0x5490, 0x0f }, { 0x5491, 0xff }, { 0x5492, 0x0d }, { 0x5493, 0x05 },
 	{ 0x5494, 0x07 }, { 0x5495, 0x1a }, { 0x5496, 0x04 }, { 0x5497, 0x01 },
-	{ 0x5498, 0x03 }, { 0x5499, 0x53 }, { 0x549A, 0x02 }, { 0x549B, 0xeb },
-	{ 0x549C, 0x02 }, { 0x549D, 0xa0 }, { 0x549E, 0x02 }, { 0x549F, 0x67 },
-	{ 0x54A0, 0x02 }, { 0x54A1, 0x3b }, { 0x54A2, 0x02 }, { 0x54A3, 0x18 },
-	{ 0x54A4, 0x01 }, { 0x54A5, 0xe7 }, { 0x54A6, 0x01 }, { 0x54A7, 0xc3 },
-	{ 0x54A8, 0x01 }, { 0x54A9, 0x94 }, { 0x54AA, 0x01 }, { 0x54AB, 0x72 },
-	{ 0x54AC, 0x01 }, { 0x54AD, 0x57 },
+	{ 0x5498, 0x03 }, { 0x5499, 0x53 }, { 0x549a, 0x02 }, { 0x549b, 0xeb },
+	{ 0x549c, 0x02 }, { 0x549d, 0xa0 }, { 0x549e, 0x02 }, { 0x549f, 0x67 },
+	{ 0x54a0, 0x02 }, { 0x54a1, 0x3b }, { 0x54a2, 0x02 }, { 0x54a3, 0x18 },
+	{ 0x54a4, 0x01 }, { 0x54a5, 0xe7 }, { 0x54a6, 0x01 }, { 0x54a7, 0xc3 },
+	{ 0x54a8, 0x01 }, { 0x54a9, 0x94 }, { 0x54aa, 0x01 }, { 0x54ab, 0x72 },
+	{ 0x54ac, 0x01 }, { 0x54ad, 0x57 },
 
 	/* AWB */
 	{ OV9740_AWB_CTRL00,		0xf0 },
@@ -296,18 +296,18 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	{ OV9740_AWB_CTRL14,		0x00 },
 
 	/* CIP */
-	{ 0x530D, 0x12 },
+	{ 0x530d, 0x12 },
 
 	/* CMX */
 	{ 0x5380, 0x01 }, { 0x5381, 0x00 }, { 0x5382, 0x00 }, { 0x5383, 0x17 },
 	{ 0x5384, 0x00 }, { 0x5385, 0x01 }, { 0x5386, 0x00 }, { 0x5387, 0x00 },
-	{ 0x5388, 0x00 }, { 0x5389, 0xe0 }, { 0x538A, 0x00 }, { 0x538B, 0x20 },
-	{ 0x538C, 0x00 }, { 0x538D, 0x00 }, { 0x538E, 0x00 }, { 0x538F, 0x16 },
+	{ 0x5388, 0x00 }, { 0x5389, 0xe0 }, { 0x538a, 0x00 }, { 0x538b, 0x20 },
+	{ 0x538c, 0x00 }, { 0x538d, 0x00 }, { 0x538e, 0x00 }, { 0x538f, 0x16 },
 	{ 0x5390, 0x00 }, { 0x5391, 0x9c }, { 0x5392, 0x00 }, { 0x5393, 0xa0 },
 	{ 0x5394, 0x18 },
 
 	/* 50/60 Detection */
-	{ 0x3C0A, 0x9c }, { 0x3C0B, 0x3f },
+	{ 0x3c0a, 0x9c }, { 0x3c0b, 0x3f },
 
 	/* Output Select */
 	{ OV9740_IO_OUTPUT_SEL01,	0x00 },
@@ -909,7 +909,6 @@ static struct v4l2_subdev_core_ops ov9740_core_ops = {
 	.g_register		= ov9740_get_register,
 	.s_register		= ov9740_set_register,
 #endif
-
 };
 
 static struct v4l2_subdev_video_ops ov9740_video_ops = {
-- 
1.7.5.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5 v2] [media] ov9740: Correct print in ov9740_reg_rmw()
  2011-05-26  0:04 [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies achew
@ 2011-05-26  0:04 ` achew
  2011-05-26  0:04 ` [PATCH 3/5 v2] [media] ov9740: Fixed some settings achew
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: achew @ 2011-05-26  0:04 UTC (permalink / raw)
  To: g.liakhovetski, mchehab, olof; +Cc: linux-media, linux-kernel, Andrew Chew

From: Andrew Chew <achew@nvidia.com>

The register width of the ov9740 is 16-bits, so correct the debug print
to reflect this.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/media/video/ov9740.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 96811e4..d5c9061 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -537,7 +537,8 @@ static int ov9740_reg_rmw(struct i2c_client *client, u16 reg, u8 set, u8 unset)
 	ret = ov9740_reg_read(client, reg, &val);
 	if (ret < 0) {
 		dev_err(&client->dev,
-			"[Read]-Modify-Write of register %02x failed!\n", reg);
+			"[Read]-Modify-Write of register 0x%04x failed!\n",
+			reg);
 		return ret;
 	}
 
@@ -547,7 +548,8 @@ static int ov9740_reg_rmw(struct i2c_client *client, u16 reg, u8 set, u8 unset)
 	ret = ov9740_reg_write(client, reg, val);
 	if (ret < 0) {
 		dev_err(&client->dev,
-			"Read-Modify-[Write] of register %02x failed!\n", reg);
+			"Read-Modify-[Write] of register 0x%04x failed!\n",
+			reg);
 		return ret;
 	}
 
-- 
1.7.5.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5 v2] [media] ov9740: Fixed some settings
  2011-05-26  0:04 [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies achew
  2011-05-26  0:04 ` [PATCH 2/5 v2] [media] ov9740: Correct print in ov9740_reg_rmw() achew
@ 2011-05-26  0:04 ` achew
  2011-05-29 10:21   ` Guennadi Liakhovetski
  2011-05-26  0:04 ` [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs achew
  2011-05-26  0:04 ` [PATCH 5/5 v2] [media] ov9740: Add suspend/resume achew
  3 siblings, 1 reply; 13+ messages in thread
From: achew @ 2011-05-26  0:04 UTC (permalink / raw)
  To: g.liakhovetski, mchehab, olof; +Cc: linux-media, linux-kernel, Andrew Chew

From: Andrew Chew <achew@nvidia.com>

Based on vendor feedback, should issue a software reset at start of day.

Also, OV9740_ANALOG_CTRL15 needs to be changed so the sensor does not begin
streaming until it is ready (otherwise, results in a nonsense frame for the
initial frame).

For discontinuous clocks, need to change OV9740_MIPI_CTRL00.

Finally, OV9740_ISP_CTRL19 needs to be changed to really use YUYV ordering
(the previous value was for VYUY).

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/media/video/ov9740.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index d5c9061..9d7c74d 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -68,6 +68,7 @@
 #define OV9740_ANALOG_CTRL04		0x3604
 #define OV9740_ANALOG_CTRL10		0x3610
 #define OV9740_ANALOG_CTRL12		0x3612
+#define OV9740_ANALOG_CTRL15		0x3615
 #define OV9740_ANALOG_CTRL20		0x3620
 #define OV9740_ANALOG_CTRL21		0x3621
 #define OV9740_ANALOG_CTRL22		0x3622
@@ -222,6 +223,9 @@ struct ov9740_priv {
 };
 
 static const struct ov9740_reg ov9740_defaults[] = {
+	/* Software Reset */
+	{ OV9740_SOFTWARE_RESET,	0x01 },
+
 	/* Banding Filter */
 	{ OV9740_AEC_B50_STEP_HI,	0x00 },
 	{ OV9740_AEC_B50_STEP_LO,	0xe8 },
@@ -333,6 +337,7 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	{ OV9740_ANALOG_CTRL10,		0xa1 },
 	{ OV9740_ANALOG_CTRL12,		0x24 },
 	{ OV9740_ANALOG_CTRL22,		0x9f },
+	{ OV9740_ANALOG_CTRL15,		0xf0 },
 
 	/* Sensor Control */
 	{ OV9740_SENSOR_CTRL03,		0x42 },
@@ -385,7 +390,7 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	{ OV9740_LN_LENGTH_PCK_LO,	0x62 },
 
 	/* MIPI Control */
-	{ OV9740_MIPI_CTRL00,		0x44 },
+	{ OV9740_MIPI_CTRL00,		0x64 }, /* 0x44 for continuous clock */
 	{ OV9740_MIPI_3837,		0x01 },
 	{ OV9740_MIPI_CTRL01,		0x0f },
 	{ OV9740_MIPI_CTRL03,		0x05 },
@@ -393,6 +398,9 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	{ OV9740_VFIFO_RD_CTRL,		0x16 },
 	{ OV9740_MIPI_CTRL_3012,	0x70 },
 	{ OV9740_SC_CMMM_MIPI_CTR,	0x01 },
+
+	/* YUYV order */
+	{ OV9740_ISP_CTRL19,		0x02 },
 };
 
 static const struct ov9740_reg ov9740_regs_vga[] = {
-- 
1.7.5.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs
  2011-05-26  0:04 [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies achew
  2011-05-26  0:04 ` [PATCH 2/5 v2] [media] ov9740: Correct print in ov9740_reg_rmw() achew
  2011-05-26  0:04 ` [PATCH 3/5 v2] [media] ov9740: Fixed some settings achew
@ 2011-05-26  0:04 ` achew
  2011-05-29 10:36   ` Guennadi Liakhovetski
  2011-05-26  0:04 ` [PATCH 5/5 v2] [media] ov9740: Add suspend/resume achew
  3 siblings, 1 reply; 13+ messages in thread
From: achew @ 2011-05-26  0:04 UTC (permalink / raw)
  To: g.liakhovetski, mchehab, olof; +Cc: linux-media, linux-kernel, Andrew Chew

From: Andrew Chew <achew@nvidia.com>

Derive resolution-dependent register settings programmatically.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/media/video/ov9740.c |  210 +++++++++++++++++++++++-------------------
 1 files changed, 114 insertions(+), 96 deletions(-)

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 9d7c74d..6c28ae8 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -181,27 +181,8 @@
 #define OV9740_MIPI_CTRL_3012		0x3012
 #define OV9740_SC_CMMM_MIPI_CTR		0x3014
 
-/* supported resolutions */
-enum {
-	OV9740_VGA,
-	OV9740_720P,
-};
-
-struct ov9740_resolution {
-	unsigned int width;
-	unsigned int height;
-};
-
-static struct ov9740_resolution ov9740_resolutions[] = {
-	[OV9740_VGA] = {
-		.width	= 640,
-		.height	= 480,
-	},
-	[OV9740_720P] = {
-		.width	= 1280,
-		.height	= 720,
-	},
-};
+#define OV9740_MAX_WIDTH		1280
+#define OV9740_MAX_HEIGHT		720
 
 /* Misc. structures */
 struct ov9740_reg {
@@ -403,54 +384,6 @@ static const struct ov9740_reg ov9740_defaults[] = {
 	{ OV9740_ISP_CTRL19,		0x02 },
 };
 
-static const struct ov9740_reg ov9740_regs_vga[] = {
-	{ OV9740_X_ADDR_START_HI,	0x00 },
-	{ OV9740_X_ADDR_START_LO,	0xa0 },
-	{ OV9740_Y_ADDR_START_HI,	0x00 },
-	{ OV9740_Y_ADDR_START_LO,	0x00 },
-	{ OV9740_X_ADDR_END_HI,		0x04 },
-	{ OV9740_X_ADDR_END_LO,		0x63 },
-	{ OV9740_Y_ADDR_END_HI,		0x02 },
-	{ OV9740_Y_ADDR_END_LO,		0xd3 },
-	{ OV9740_X_OUTPUT_SIZE_HI,	0x02 },
-	{ OV9740_X_OUTPUT_SIZE_LO,	0x80 },
-	{ OV9740_Y_OUTPUT_SIZE_HI,	0x01 },
-	{ OV9740_Y_OUTPUT_SIZE_LO,	0xe0 },
-	{ OV9740_ISP_CTRL1E,		0x03 },
-	{ OV9740_ISP_CTRL1F,		0xc0 },
-	{ OV9740_ISP_CTRL20,		0x02 },
-	{ OV9740_ISP_CTRL21,		0xd0 },
-	{ OV9740_VFIFO_READ_START_HI,	0x01 },
-	{ OV9740_VFIFO_READ_START_LO,	0x40 },
-	{ OV9740_ISP_CTRL00,		0xff },
-	{ OV9740_ISP_CTRL01,		0xff },
-	{ OV9740_ISP_CTRL03,		0xff },
-};
-
-static const struct ov9740_reg ov9740_regs_720p[] = {
-	{ OV9740_X_ADDR_START_HI,	0x00 },
-	{ OV9740_X_ADDR_START_LO,	0x00 },
-	{ OV9740_Y_ADDR_START_HI,	0x00 },
-	{ OV9740_Y_ADDR_START_LO,	0x00 },
-	{ OV9740_X_ADDR_END_HI,		0x05 },
-	{ OV9740_X_ADDR_END_LO,		0x03 },
-	{ OV9740_Y_ADDR_END_HI,		0x02 },
-	{ OV9740_Y_ADDR_END_LO,		0xd3 },
-	{ OV9740_X_OUTPUT_SIZE_HI,	0x05 },
-	{ OV9740_X_OUTPUT_SIZE_LO,	0x00 },
-	{ OV9740_Y_OUTPUT_SIZE_HI,	0x02 },
-	{ OV9740_Y_OUTPUT_SIZE_LO,	0xd0 },
-	{ OV9740_ISP_CTRL1E,		0x05 },
-	{ OV9740_ISP_CTRL1F,		0x00 },
-	{ OV9740_ISP_CTRL20,		0x02 },
-	{ OV9740_ISP_CTRL21,		0xd0 },
-	{ OV9740_VFIFO_READ_START_HI,	0x02 },
-	{ OV9740_VFIFO_READ_START_LO,	0x30 },
-	{ OV9740_ISP_CTRL00,		0xff },
-	{ OV9740_ISP_CTRL01,		0xef },
-	{ OV9740_ISP_CTRL03,		0xff },
-};
-
 static enum v4l2_mbus_pixelcode ov9740_codes[] = {
 	V4L2_MBUS_FMT_YUYV8_2X8,
 };
@@ -727,39 +660,124 @@ static int ov9740_set_register(struct v4l2_subdev *sd,
 /* select nearest higher resolution for capture */
 static void ov9740_res_roundup(u32 *width, u32 *height)
 {
-	int i;
+	/* Width must be a multiple of 4 pixels. */
+	*width += *width % 4;
 
-	for (i = 0; i < ARRAY_SIZE(ov9740_resolutions); i++)
-		if ((ov9740_resolutions[i].width >= *width) &&
-		    (ov9740_resolutions[i].height >= *height)) {
-			*width = ov9740_resolutions[i].width;
-			*height = ov9740_resolutions[i].height;
-			return;
-		}
+	/* Max resolution is 1280x720 (720p). */
+	if (*width > OV9740_MAX_WIDTH)
+		*width = OV9740_MAX_WIDTH;
 
-	*width = ov9740_resolutions[OV9740_720P].width;
-	*height = ov9740_resolutions[OV9740_720P].height;
+	if (*height > OV9740_MAX_HEIGHT)
+		*height = OV9740_MAX_HEIGHT;
 }
 
 /* Setup registers according to resolution and color encoding */
-static int ov9740_set_res(struct i2c_client *client, u32 width)
+static int ov9740_set_res(struct i2c_client *client, u32 width, u32 height)
 {
+	u32 x_start;
+	u32 y_start;
+	u32 x_end;
+	u32 y_end;
+	bool scaling = 0;
+	u32 scale_input_x;
+	u32 scale_input_y;
 	int ret;
 
-	/* select register configuration for given resolution */
-	if (width == ov9740_resolutions[OV9740_VGA].width) {
-		dev_dbg(&client->dev, "Setting image size to 640x480\n");
-		ret = ov9740_reg_write_array(client, ov9740_regs_vga,
-					     ARRAY_SIZE(ov9740_regs_vga));
-	} else if (width == ov9740_resolutions[OV9740_720P].width) {
-		dev_dbg(&client->dev, "Setting image size to 1280x720\n");
-		ret = ov9740_reg_write_array(client, ov9740_regs_720p,
-					     ARRAY_SIZE(ov9740_regs_720p));
+	if ((width != OV9740_MAX_WIDTH) || (height != OV9740_MAX_HEIGHT))
+		scaling = 1;
+
+	/*
+	 * Try to use as much of the sensor area as possible when supporting
+	 * smaller resolutions.  Depending on the aspect ratio of the
+	 * chosen resolution, we can either use the full width of the sensor,
+	 * or the full height of the sensor (or both if the aspect ratio is
+	 * the same as 1280x720.
+	 */
+	if ((OV9740_MAX_WIDTH * height) > (OV9740_MAX_HEIGHT * width)) {
+		scale_input_x = (OV9740_MAX_HEIGHT * width) / height;
+		scale_input_y = OV9740_MAX_HEIGHT;
 	} else {
-		dev_err(&client->dev, "Failed to select resolution!\n");
-		return -EINVAL;
+		scale_input_x = OV9740_MAX_WIDTH;
+		scale_input_y = (OV9740_MAX_WIDTH * height) / width;
 	}
 
+	/* These describe the area of the sensor to use. */
+	x_start = (OV9740_MAX_WIDTH - scale_input_x) / 2;
+	y_start = (OV9740_MAX_HEIGHT - scale_input_y) / 2;
+	x_end = x_start + scale_input_x - 1;
+	y_end = y_start + scale_input_y - 1;
+
+	ret = ov9740_reg_write(client, OV9740_X_ADDR_START_HI, x_start >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_X_ADDR_START_LO, x_start & 0xff);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_ADDR_START_HI, y_start >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_ADDR_START_LO, y_start & 0xff);
+	if (ret)
+		goto done;
+
+	ret = ov9740_reg_write(client, OV9740_X_ADDR_END_HI, x_end >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_X_ADDR_END_LO, x_end & 0xff);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_ADDR_END_HI, y_end >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_ADDR_END_LO, y_end & 0xff);
+	if (ret)
+		goto done;
+
+	ret = ov9740_reg_write(client, OV9740_X_OUTPUT_SIZE_HI, width >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_X_OUTPUT_SIZE_LO, width & 0xff);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_OUTPUT_SIZE_HI, height >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_Y_OUTPUT_SIZE_LO, height & 0xff);
+	if (ret)
+		goto done;
+
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL1E, scale_input_x >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL1F, scale_input_x & 0xff);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL20, scale_input_y >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL21, scale_input_y & 0xff);
+	if (ret)
+		goto done;
+
+	ret = ov9740_reg_write(client, OV9740_VFIFO_READ_START_HI,
+			       (scale_input_x - width) >> 8);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_VFIFO_READ_START_LO,
+			       (scale_input_x - width) & 0xff);
+	if (ret)
+		goto done;
+
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL00, 0xff);
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL01, 0xef |
+							  (scaling << 4));
+	if (ret)
+		goto done;
+	ret = ov9740_reg_write(client, OV9740_ISP_CTRL03, 0xff);
+
+done:
 	return ret;
 }
 
@@ -787,7 +805,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
 	if (ret < 0)
 		return ret;
 
-	ret = ov9740_set_res(client, mf->width);
+	ret = ov9740_set_res(client, mf->width, mf->height);
 	if (ret < 0)
 		return ret;
 
@@ -824,8 +842,8 @@ static int ov9740_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 {
 	a->bounds.left		= 0;
 	a->bounds.top		= 0;
-	a->bounds.width		= ov9740_resolutions[OV9740_720P].width;
-	a->bounds.height	= ov9740_resolutions[OV9740_720P].height;
+	a->bounds.width		= OV9740_MAX_WIDTH;
+	a->bounds.height	= OV9740_MAX_HEIGHT;
 	a->defrect		= a->bounds;
 	a->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	a->pixelaspect.numerator	= 1;
@@ -838,8 +856,8 @@ static int ov9740_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 {
 	a->c.left		= 0;
 	a->c.top		= 0;
-	a->c.width		= ov9740_resolutions[OV9740_720P].width;
-	a->c.height		= ov9740_resolutions[OV9740_720P].height;
+	a->c.width		= OV9740_MAX_WIDTH;
+	a->c.height		= OV9740_MAX_HEIGHT;
 	a->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
 
 	return 0;
-- 
1.7.5.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5 v2] [media] ov9740: Add suspend/resume
  2011-05-26  0:04 [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies achew
                   ` (2 preceding siblings ...)
  2011-05-26  0:04 ` [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs achew
@ 2011-05-26  0:04 ` achew
  2011-05-29 10:58   ` Guennadi Liakhovetski
  3 siblings, 1 reply; 13+ messages in thread
From: achew @ 2011-05-26  0:04 UTC (permalink / raw)
  To: g.liakhovetski, mchehab, olof; +Cc: linux-media, linux-kernel, Andrew Chew

From: Andrew Chew <achew@nvidia.com>

On suspend, remember whether we are streaming or not, and at what frame format,
so that on resume, we can start streaming again.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/media/video/ov9740.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 6c28ae8..4abe943 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -201,6 +201,10 @@ struct ov9740_priv {
 
 	bool				flag_vflip;
 	bool				flag_hflip;
+
+	/* For suspend/resume. */
+	struct v4l2_mbus_framefmt	current_mf;
+	int				current_enable;
 };
 
 static const struct ov9740_reg ov9740_defaults[] = {
@@ -551,6 +555,8 @@ static int ov9740_s_stream(struct v4l2_subdev *sd, int enable)
 					       0x00);
 	}
 
+	priv->current_enable = enable;
+
 	return ret;
 }
 
@@ -786,6 +792,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
 			struct v4l2_mbus_framefmt *mf)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov9740_priv *priv = to_ov9740(sd);
 	enum v4l2_colorspace cspace;
 	enum v4l2_mbus_pixelcode code = mf->code;
 	int ret;
@@ -812,6 +819,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
 	mf->code	= code;
 	mf->colorspace	= cspace;
 
+	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
+
 	return ret;
 }
 
@@ -922,7 +931,37 @@ err:
 	return ret;
 }
 
+static int ov9740_suspend(struct soc_camera_device *icd, pm_message_t state)
+{
+	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+	struct ov9740_priv *priv = to_ov9740(sd);
+
+	if (priv->current_enable) {
+		int current_enable = priv->current_enable;
+
+		ov9740_s_stream(sd, 0);
+		priv->current_enable = current_enable;
+	}
+
+	return 0;
+}
+
+static int ov9740_resume(struct soc_camera_device *icd)
+{
+	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+	struct ov9740_priv *priv = to_ov9740(sd);
+
+	if (priv->current_enable) {
+		ov9740_s_fmt(sd, &priv->current_mf);
+		ov9740_s_stream(sd, priv->current_enable);
+	}
+
+	return 0;
+}
+
 static struct soc_camera_ops ov9740_ops = {
+	.suspend		= ov9740_suspend,
+	.resume			= ov9740_resume,
 	.set_bus_param		= ov9740_set_bus_param,
 	.query_bus_param	= ov9740_query_bus_param,
 	.controls		= ov9740_controls,
-- 
1.7.5.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/5 v2] [media] ov9740: Fixed some settings
  2011-05-26  0:04 ` [PATCH 3/5 v2] [media] ov9740: Fixed some settings achew
@ 2011-05-29 10:21   ` Guennadi Liakhovetski
  2011-06-01  1:12     ` Andrew Chew
  0 siblings, 1 reply; 13+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-29 10:21 UTC (permalink / raw)
  To: Andrew Chew; +Cc: mchehab, olof, linux-media, linux-kernel

On Wed, 25 May 2011, achew@nvidia.com wrote:

> From: Andrew Chew <achew@nvidia.com>
> 
> Based on vendor feedback, should issue a software reset at start of day.
> 
> Also, OV9740_ANALOG_CTRL15 needs to be changed so the sensor does not begin
> streaming until it is ready (otherwise, results in a nonsense frame for the
> initial frame).
> 
> For discontinuous clocks, need to change OV9740_MIPI_CTRL00.
> 
> Finally, OV9740_ISP_CTRL19 needs to be changed to really use YUYV ordering
> (the previous value was for VYUY).
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  drivers/media/video/ov9740.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
> index d5c9061..9d7c74d 100644
> --- a/drivers/media/video/ov9740.c
> +++ b/drivers/media/video/ov9740.c
> @@ -68,6 +68,7 @@
>  #define OV9740_ANALOG_CTRL04		0x3604
>  #define OV9740_ANALOG_CTRL10		0x3610
>  #define OV9740_ANALOG_CTRL12		0x3612
> +#define OV9740_ANALOG_CTRL15		0x3615
>  #define OV9740_ANALOG_CTRL20		0x3620
>  #define OV9740_ANALOG_CTRL21		0x3621
>  #define OV9740_ANALOG_CTRL22		0x3622
> @@ -222,6 +223,9 @@ struct ov9740_priv {
>  };
>  
>  static const struct ov9740_reg ov9740_defaults[] = {
> +	/* Software Reset */
> +	{ OV9740_SOFTWARE_RESET,	0x01 },
> +
>  	/* Banding Filter */
>  	{ OV9740_AEC_B50_STEP_HI,	0x00 },
>  	{ OV9740_AEC_B50_STEP_LO,	0xe8 },
> @@ -333,6 +337,7 @@ static const struct ov9740_reg ov9740_defaults[] = {
>  	{ OV9740_ANALOG_CTRL10,		0xa1 },
>  	{ OV9740_ANALOG_CTRL12,		0x24 },
>  	{ OV9740_ANALOG_CTRL22,		0x9f },
> +	{ OV9740_ANALOG_CTRL15,		0xf0 },
>  
>  	/* Sensor Control */
>  	{ OV9740_SENSOR_CTRL03,		0x42 },
> @@ -385,7 +390,7 @@ static const struct ov9740_reg ov9740_defaults[] = {
>  	{ OV9740_LN_LENGTH_PCK_LO,	0x62 },
>  
>  	/* MIPI Control */
> -	{ OV9740_MIPI_CTRL00,		0x44 },
> +	{ OV9740_MIPI_CTRL00,		0x64 }, /* 0x44 for continuous clock */

I think, the choice between continuous and discontinuous CSI-2 clock 
should become configurable. You can only use discontinuous clock with 
hosts, that support it, right? Whereas all hosts must support continuous 
clock. So, I'm not sure we should unconditionally switch to discontinuous 
clock here... Maybe it's better to keep it continuous until we make it 
configurable?

>  	{ OV9740_MIPI_3837,		0x01 },
>  	{ OV9740_MIPI_CTRL01,		0x0f },
>  	{ OV9740_MIPI_CTRL03,		0x05 },
> @@ -393,6 +398,9 @@ static const struct ov9740_reg ov9740_defaults[] = {
>  	{ OV9740_VFIFO_RD_CTRL,		0x16 },
>  	{ OV9740_MIPI_CTRL_3012,	0x70 },
>  	{ OV9740_SC_CMMM_MIPI_CTR,	0x01 },
> +
> +	/* YUYV order */
> +	{ OV9740_ISP_CTRL19,		0x02 },
>  };
>  
>  static const struct ov9740_reg ov9740_regs_vga[] = {
> -- 
> 1.7.5.2
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs
  2011-05-26  0:04 ` [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs achew
@ 2011-05-29 10:36   ` Guennadi Liakhovetski
  2011-06-01  1:50     ` Andrew Chew
  0 siblings, 1 reply; 13+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-29 10:36 UTC (permalink / raw)
  To: Andrew Chew; +Cc: mchehab, olof, linux-media, linux-kernel

Now, this is really the direction I like! Now it's becoming a real 
driver;) Thanks for doing this!

On Wed, 25 May 2011, achew@nvidia.com wrote:

> From: Andrew Chew <achew@nvidia.com>
> 
> Derive resolution-dependent register settings programmatically.
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  drivers/media/video/ov9740.c |  210 +++++++++++++++++++++++-------------------
>  1 files changed, 114 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
> index 9d7c74d..6c28ae8 100644
> --- a/drivers/media/video/ov9740.c
> +++ b/drivers/media/video/ov9740.c
> @@ -181,27 +181,8 @@
>  #define OV9740_MIPI_CTRL_3012		0x3012
>  #define OV9740_SC_CMMM_MIPI_CTR		0x3014
>  
> -/* supported resolutions */
> -enum {
> -	OV9740_VGA,
> -	OV9740_720P,
> -};
> -
> -struct ov9740_resolution {
> -	unsigned int width;
> -	unsigned int height;
> -};
> -
> -static struct ov9740_resolution ov9740_resolutions[] = {
> -	[OV9740_VGA] = {
> -		.width	= 640,
> -		.height	= 480,
> -	},
> -	[OV9740_720P] = {
> -		.width	= 1280,
> -		.height	= 720,
> -	},
> -};
> +#define OV9740_MAX_WIDTH		1280
> +#define OV9740_MAX_HEIGHT		720
>  
>  /* Misc. structures */
>  struct ov9740_reg {
> @@ -403,54 +384,6 @@ static const struct ov9740_reg ov9740_defaults[] = {
>  	{ OV9740_ISP_CTRL19,		0x02 },
>  };
>  
> -static const struct ov9740_reg ov9740_regs_vga[] = {
> -	{ OV9740_X_ADDR_START_HI,	0x00 },
> -	{ OV9740_X_ADDR_START_LO,	0xa0 },
> -	{ OV9740_Y_ADDR_START_HI,	0x00 },
> -	{ OV9740_Y_ADDR_START_LO,	0x00 },
> -	{ OV9740_X_ADDR_END_HI,		0x04 },
> -	{ OV9740_X_ADDR_END_LO,		0x63 },
> -	{ OV9740_Y_ADDR_END_HI,		0x02 },
> -	{ OV9740_Y_ADDR_END_LO,		0xd3 },
> -	{ OV9740_X_OUTPUT_SIZE_HI,	0x02 },
> -	{ OV9740_X_OUTPUT_SIZE_LO,	0x80 },
> -	{ OV9740_Y_OUTPUT_SIZE_HI,	0x01 },
> -	{ OV9740_Y_OUTPUT_SIZE_LO,	0xe0 },
> -	{ OV9740_ISP_CTRL1E,		0x03 },
> -	{ OV9740_ISP_CTRL1F,		0xc0 },
> -	{ OV9740_ISP_CTRL20,		0x02 },
> -	{ OV9740_ISP_CTRL21,		0xd0 },
> -	{ OV9740_VFIFO_READ_START_HI,	0x01 },
> -	{ OV9740_VFIFO_READ_START_LO,	0x40 },
> -	{ OV9740_ISP_CTRL00,		0xff },
> -	{ OV9740_ISP_CTRL01,		0xff },
> -	{ OV9740_ISP_CTRL03,		0xff },
> -};
> -
> -static const struct ov9740_reg ov9740_regs_720p[] = {
> -	{ OV9740_X_ADDR_START_HI,	0x00 },
> -	{ OV9740_X_ADDR_START_LO,	0x00 },
> -	{ OV9740_Y_ADDR_START_HI,	0x00 },
> -	{ OV9740_Y_ADDR_START_LO,	0x00 },
> -	{ OV9740_X_ADDR_END_HI,		0x05 },
> -	{ OV9740_X_ADDR_END_LO,		0x03 },
> -	{ OV9740_Y_ADDR_END_HI,		0x02 },
> -	{ OV9740_Y_ADDR_END_LO,		0xd3 },
> -	{ OV9740_X_OUTPUT_SIZE_HI,	0x05 },
> -	{ OV9740_X_OUTPUT_SIZE_LO,	0x00 },
> -	{ OV9740_Y_OUTPUT_SIZE_HI,	0x02 },
> -	{ OV9740_Y_OUTPUT_SIZE_LO,	0xd0 },
> -	{ OV9740_ISP_CTRL1E,		0x05 },
> -	{ OV9740_ISP_CTRL1F,		0x00 },
> -	{ OV9740_ISP_CTRL20,		0x02 },
> -	{ OV9740_ISP_CTRL21,		0xd0 },
> -	{ OV9740_VFIFO_READ_START_HI,	0x02 },
> -	{ OV9740_VFIFO_READ_START_LO,	0x30 },
> -	{ OV9740_ISP_CTRL00,		0xff },
> -	{ OV9740_ISP_CTRL01,		0xef },
> -	{ OV9740_ISP_CTRL03,		0xff },
> -};
> -
>  static enum v4l2_mbus_pixelcode ov9740_codes[] = {
>  	V4L2_MBUS_FMT_YUYV8_2X8,
>  };
> @@ -727,39 +660,124 @@ static int ov9740_set_register(struct v4l2_subdev *sd,
>  /* select nearest higher resolution for capture */
>  static void ov9740_res_roundup(u32 *width, u32 *height)
>  {
> -	int i;
> +	/* Width must be a multiple of 4 pixels. */
> +	*width += *width % 4;

No, this doesn't make it a multiple of 4, unless it was even;) Just take 5 
as an example. What you really want here is

	*width = ALIGN(*width, 4);

>  
> -	for (i = 0; i < ARRAY_SIZE(ov9740_resolutions); i++)
> -		if ((ov9740_resolutions[i].width >= *width) &&
> -		    (ov9740_resolutions[i].height >= *height)) {
> -			*width = ov9740_resolutions[i].width;
> -			*height = ov9740_resolutions[i].height;
> -			return;
> -		}
> +	/* Max resolution is 1280x720 (720p). */
> +	if (*width > OV9740_MAX_WIDTH)
> +		*width = OV9740_MAX_WIDTH;
>  
> -	*width = ov9740_resolutions[OV9740_720P].width;
> -	*height = ov9740_resolutions[OV9740_720P].height;
> +	if (*height > OV9740_MAX_HEIGHT)
> +		*height = OV9740_MAX_HEIGHT;
>  }
>  
>  /* Setup registers according to resolution and color encoding */
> -static int ov9740_set_res(struct i2c_client *client, u32 width)
> +static int ov9740_set_res(struct i2c_client *client, u32 width, u32 height)
>  {
> +	u32 x_start;
> +	u32 y_start;
> +	u32 x_end;
> +	u32 y_end;
> +	bool scaling = 0;
> +	u32 scale_input_x;
> +	u32 scale_input_y;
>  	int ret;
>  
> -	/* select register configuration for given resolution */
> -	if (width == ov9740_resolutions[OV9740_VGA].width) {
> -		dev_dbg(&client->dev, "Setting image size to 640x480\n");
> -		ret = ov9740_reg_write_array(client, ov9740_regs_vga,
> -					     ARRAY_SIZE(ov9740_regs_vga));
> -	} else if (width == ov9740_resolutions[OV9740_720P].width) {
> -		dev_dbg(&client->dev, "Setting image size to 1280x720\n");
> -		ret = ov9740_reg_write_array(client, ov9740_regs_720p,
> -					     ARRAY_SIZE(ov9740_regs_720p));
> +	if ((width != OV9740_MAX_WIDTH) || (height != OV9740_MAX_HEIGHT))
> +		scaling = 1;
> +
> +	/*
> +	 * Try to use as much of the sensor area as possible when supporting
> +	 * smaller resolutions.  Depending on the aspect ratio of the
> +	 * chosen resolution, we can either use the full width of the sensor,
> +	 * or the full height of the sensor (or both if the aspect ratio is
> +	 * the same as 1280x720.
> +	 */
> +	if ((OV9740_MAX_WIDTH * height) > (OV9740_MAX_HEIGHT * width)) {
> +		scale_input_x = (OV9740_MAX_HEIGHT * width) / height;
> +		scale_input_y = OV9740_MAX_HEIGHT;
>  	} else {
> -		dev_err(&client->dev, "Failed to select resolution!\n");
> -		return -EINVAL;
> +		scale_input_x = OV9740_MAX_WIDTH;
> +		scale_input_y = (OV9740_MAX_WIDTH * height) / width;
>  	}

I don'z know how this sensor works, but the above two divisions round 
down. And these are input sizes. Cannot it possibly lead to the output 
window being smaller, than required? Maybe you have to round up (hint: 
use DIV_ROUND_UP())?

>  
> +	/* These describe the area of the sensor to use. */
> +	x_start = (OV9740_MAX_WIDTH - scale_input_x) / 2;
> +	y_start = (OV9740_MAX_HEIGHT - scale_input_y) / 2;
> +	x_end = x_start + scale_input_x - 1;
> +	y_end = y_start + scale_input_y - 1;
> +
> +	ret = ov9740_reg_write(client, OV9740_X_ADDR_START_HI, x_start >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_X_ADDR_START_LO, x_start & 0xff);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_ADDR_START_HI, y_start >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_ADDR_START_LO, y_start & 0xff);
> +	if (ret)
> +		goto done;
> +
> +	ret = ov9740_reg_write(client, OV9740_X_ADDR_END_HI, x_end >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_X_ADDR_END_LO, x_end & 0xff);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_ADDR_END_HI, y_end >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_ADDR_END_LO, y_end & 0xff);
> +	if (ret)
> +		goto done;
> +
> +	ret = ov9740_reg_write(client, OV9740_X_OUTPUT_SIZE_HI, width >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_X_OUTPUT_SIZE_LO, width & 0xff);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_OUTPUT_SIZE_HI, height >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_Y_OUTPUT_SIZE_LO, height & 0xff);
> +	if (ret)
> +		goto done;
> +
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL1E, scale_input_x >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL1F, scale_input_x & 0xff);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL20, scale_input_y >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL21, scale_input_y & 0xff);
> +	if (ret)
> +		goto done;
> +
> +	ret = ov9740_reg_write(client, OV9740_VFIFO_READ_START_HI,
> +			       (scale_input_x - width) >> 8);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_VFIFO_READ_START_LO,
> +			       (scale_input_x - width) & 0xff);
> +	if (ret)
> +		goto done;
> +
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL00, 0xff);
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL01, 0xef |
> +							  (scaling << 4));
> +	if (ret)
> +		goto done;
> +	ret = ov9740_reg_write(client, OV9740_ISP_CTRL03, 0xff);
> +
> +done:
>  	return ret;
>  }
>  
> @@ -787,7 +805,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov9740_set_res(client, mf->width);
> +	ret = ov9740_set_res(client, mf->width, mf->height);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -824,8 +842,8 @@ static int ov9740_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
>  {
>  	a->bounds.left		= 0;
>  	a->bounds.top		= 0;
> -	a->bounds.width		= ov9740_resolutions[OV9740_720P].width;
> -	a->bounds.height	= ov9740_resolutions[OV9740_720P].height;
> +	a->bounds.width		= OV9740_MAX_WIDTH;
> +	a->bounds.height	= OV9740_MAX_HEIGHT;
>  	a->defrect		= a->bounds;
>  	a->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	a->pixelaspect.numerator	= 1;
> @@ -838,8 +856,8 @@ static int ov9740_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  {
>  	a->c.left		= 0;
>  	a->c.top		= 0;
> -	a->c.width		= ov9740_resolutions[OV9740_720P].width;
> -	a->c.height		= ov9740_resolutions[OV9740_720P].height;
> +	a->c.width		= OV9740_MAX_WIDTH;
> +	a->c.height		= OV9740_MAX_HEIGHT;
>  	a->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  
>  	return 0;
> -- 
> 1.7.5.2
> 

Very nice!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5 v2] [media] ov9740: Add suspend/resume
  2011-05-26  0:04 ` [PATCH 5/5 v2] [media] ov9740: Add suspend/resume achew
@ 2011-05-29 10:58   ` Guennadi Liakhovetski
  2011-06-01  1:58     ` Andrew Chew
  0 siblings, 1 reply; 13+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-29 10:58 UTC (permalink / raw)
  To: Andrew Chew; +Cc: mchehab, olof, linux-media, linux-kernel

On Wed, 25 May 2011, achew@nvidia.com wrote:

> From: Andrew Chew <achew@nvidia.com>
> 
> On suspend, remember whether we are streaming or not, and at what frame format,
> so that on resume, we can start streaming again.
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  drivers/media/video/ov9740.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
> index 6c28ae8..4abe943 100644
> --- a/drivers/media/video/ov9740.c
> +++ b/drivers/media/video/ov9740.c
> @@ -201,6 +201,10 @@ struct ov9740_priv {
>  
>  	bool				flag_vflip;
>  	bool				flag_hflip;
> +
> +	/* For suspend/resume. */
> +	struct v4l2_mbus_framefmt	current_mf;
> +	int				current_enable;

bool?

>  };
>  
>  static const struct ov9740_reg ov9740_defaults[] = {
> @@ -551,6 +555,8 @@ static int ov9740_s_stream(struct v4l2_subdev *sd, int enable)
>  					       0x00);
>  	}
>  
> +	priv->current_enable = enable;
> +
>  	return ret;
>  }
>  
> @@ -786,6 +792,7 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  			struct v4l2_mbus_framefmt *mf)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
>  	enum v4l2_colorspace cspace;
>  	enum v4l2_mbus_pixelcode code = mf->code;
>  	int ret;
> @@ -812,6 +819,8 @@ static int ov9740_s_fmt(struct v4l2_subdev *sd,
>  	mf->code	= code;
>  	mf->colorspace	= cspace;
>  
> +	memcpy(&priv->current_mf, mf, sizeof(struct v4l2_mbus_framefmt));
> +
>  	return ret;
>  }
>  
> @@ -922,7 +931,37 @@ err:
>  	return ret;
>  }
>  
> +static int ov9740_suspend(struct soc_camera_device *icd, pm_message_t state)
> +{
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
> +
> +	if (priv->current_enable) {
> +		int current_enable = priv->current_enable;
> +
> +		ov9740_s_stream(sd, 0);
> +		priv->current_enable = current_enable;

You don't need the local variable, just set

	priv->current_enable = true;

> +	}
> +
> +	return 0;
> +}
> +
> +static int ov9740_resume(struct soc_camera_device *icd)
> +{
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +	struct ov9740_priv *priv = to_ov9740(sd);
> +
> +	if (priv->current_enable) {
> +		ov9740_s_fmt(sd, &priv->current_mf);
> +		ov9740_s_stream(sd, priv->current_enable);
> +	}
> +
> +	return 0;
> +}
> +
>  static struct soc_camera_ops ov9740_ops = {
> +	.suspend		= ov9740_suspend,
> +	.resume			= ov9740_resume,

No, we don't want to use these, whey should disappear some time... Please, 
use .s_power() from struct v4l2_subdev_core_ops, you can check 
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/33105 
for an example. If your host is not using these ops, it has to be fixed. 
So far in the mainline only one soc-camera host driver is using these 
callbacks: pxa_camera.c, which, looking at your email address, I doubt is 
the driver, that you're using;)

>  	.set_bus_param		= ov9740_set_bus_param,
>  	.query_bus_param	= ov9740_query_bus_param,
>  	.controls		= ov9740_controls,
> -- 
> 1.7.5.2
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 3/5 v2] [media] ov9740: Fixed some settings
  2011-05-29 10:21   ` Guennadi Liakhovetski
@ 2011-06-01  1:12     ` Andrew Chew
  0 siblings, 0 replies; 13+ messages in thread
From: Andrew Chew @ 2011-06-01  1:12 UTC (permalink / raw)
  To: 'Guennadi Liakhovetski'; +Cc: mchehab, olof, linux-media, linux-kernel

> > +	{ OV9740_MIPI_CTRL00,		0x64 }, /* 0x44 for 
> continuous clock */
> 
> I think, the choice between continuous and discontinuous CSI-2 clock 
> should become configurable. You can only use discontinuous clock with 
> hosts, that support it, right? Whereas all hosts must support 
> continuous 
> clock. So, I'm not sure we should unconditionally switch to 
> discontinuous 
> clock here... Maybe it's better to keep it continuous until 
> we make it 
> configurable?

Yes, that's right.  The camera host needs to support discontinuous clocks.  I'll change it back to continuous clock by default, and change the comment to "0x64 for discontinuous clock", so we remember for when this becomes configurable.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs
  2011-05-29 10:36   ` Guennadi Liakhovetski
@ 2011-06-01  1:50     ` Andrew Chew
  2011-06-01 19:35       ` Guennadi Liakhovetski
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Chew @ 2011-06-01  1:50 UTC (permalink / raw)
  To: 'Guennadi Liakhovetski'; +Cc: mchehab, olof, linux-media, linux-kernel

> > +	/* Width must be a multiple of 4 pixels. */
> > +	*width += *width % 4;
> 
> No, this doesn't make it a multiple of 4, unless it was 
> even;) Just take 5 
> as an example. What you really want here is

Geez, you're right.  Not sure what was going on in my head when I did this.  Thanks for catching it.


> > +	/*
> > +	 * Try to use as much of the sensor area as possible 
> when supporting
> > +	 * smaller resolutions.  Depending on the aspect ratio of the
> > +	 * chosen resolution, we can either use the full width 
> of the sensor,
> > +	 * or the full height of the sensor (or both if the 
> aspect ratio is
> > +	 * the same as 1280x720.
> > +	 */
> > +	if ((OV9740_MAX_WIDTH * height) > (OV9740_MAX_HEIGHT * width)) {
> > +		scale_input_x = (OV9740_MAX_HEIGHT * width) / height;
> > +		scale_input_y = OV9740_MAX_HEIGHT;
> >  	} else {
> > -		dev_err(&client->dev, "Failed to select resolution!\n");
> > -		return -EINVAL;
> > +		scale_input_x = OV9740_MAX_WIDTH;
> > +		scale_input_y = (OV9740_MAX_WIDTH * height) / width;
> >  	}
> 
> I don'z know how this sensor works, but the above two divisions round 
> down. And these are input sizes. Cannot it possibly lead to 
> the output 
> window being smaller, than required? Maybe you have to round 
> up (hint: 
> use DIV_ROUND_UP())?

The intention is to do some ratio math without floating point instructions, which resulted in some algebraic twiddling (which is why that math looks so weird).  I think what's there is okay.  If there's any rounding at all (and there shouldn't be any rounding, if "standard" image dimensions are called for), then there's going to be some aspect ratio weirdness no matter which way you round that division.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 5/5 v2] [media] ov9740: Add suspend/resume
  2011-05-29 10:58   ` Guennadi Liakhovetski
@ 2011-06-01  1:58     ` Andrew Chew
  2011-06-01 19:51       ` Guennadi Liakhovetski
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Chew @ 2011-06-01  1:58 UTC (permalink / raw)
  To: 'Guennadi Liakhovetski'; +Cc: mchehab, olof, linux-media, linux-kernel

> > +	/* For suspend/resume. */
> > +	struct v4l2_mbus_framefmt	current_mf;
> > +	int				current_enable;
> 
> bool?

Are you sure you want this to be a bool?  This thing is trying to shadow the "enable" parameter of the s_stream() callback, and that enable parameter is int.


> > +static int ov9740_suspend(struct soc_camera_device *icd, 
> pm_message_t state)
> > +{
> > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > +	struct ov9740_priv *priv = to_ov9740(sd);
> > +
> > +	if (priv->current_enable) {
> > +		int current_enable = priv->current_enable;
> > +
> > +		ov9740_s_stream(sd, 0);
> > +		priv->current_enable = current_enable;
> 
> You don't need the local variable, just set
> 
> 	priv->current_enable = true;

I think I do need that local variable, the way the code is arranged now.  I'm trying to save the state of enablement inside of priv->current_enable, at the time we are suspending, so it won't necessarily be true.  And one of the side effects of calling ov9740_s_stream(sd, 0) is that priv->current_enable will be set to false, which is why I save off the value of priv->current_enable, and then restore it after the call to ov9740_s_stream().


> >  static struct soc_camera_ops ov9740_ops = {
> > +	.suspend		= ov9740_suspend,
> > +	.resume			= ov9740_resume,
> 
> No, we don't want to use these, whey should disappear some 
> time... Please, 
> use .s_power() from struct v4l2_subdev_core_ops, you can check 
> http://article.gmane.org/gmane.linux.drivers.video-input-infra
> structure/33105 
> for an example. If your host is not using these ops, it has 
> to be fixed. 
> So far in the mainline only one soc-camera host driver is using these 
> callbacks: pxa_camera.c, which, looking at your email 
> address, I doubt is 
> the driver, that you're using;)

Okay, will do.  Thanks for pointing that out :)

Is the camera host driver expected to directly call the sensor driver's s_power (via v4l2_subdev_call(sd, core, s_power, <some value>)?  Or does the v4l2 framework do this for you?  I didn't see an example of this in my last pull of linux-next.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs
  2011-06-01  1:50     ` Andrew Chew
@ 2011-06-01 19:35       ` Guennadi Liakhovetski
  0 siblings, 0 replies; 13+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-01 19:35 UTC (permalink / raw)
  To: Andrew Chew; +Cc: mchehab, olof, linux-media, linux-kernel

On Tue, 31 May 2011, Andrew Chew wrote:

> > > +	/* Width must be a multiple of 4 pixels. */
> > > +	*width += *width % 4;
> > 
> > No, this doesn't make it a multiple of 4, unless it was 
> > even;) Just take 5 
> > as an example. What you really want here is
> 
> Geez, you're right.  Not sure what was going on in my head when I did this.  Thanks for catching it.
> 
> 
> > > +	/*
> > > +	 * Try to use as much of the sensor area as possible 
> > when supporting
> > > +	 * smaller resolutions.  Depending on the aspect ratio of the
> > > +	 * chosen resolution, we can either use the full width 
> > of the sensor,
> > > +	 * or the full height of the sensor (or both if the 
> > aspect ratio is
> > > +	 * the same as 1280x720.
> > > +	 */
> > > +	if ((OV9740_MAX_WIDTH * height) > (OV9740_MAX_HEIGHT * width)) {
> > > +		scale_input_x = (OV9740_MAX_HEIGHT * width) / height;
> > > +		scale_input_y = OV9740_MAX_HEIGHT;
> > >  	} else {
> > > -		dev_err(&client->dev, "Failed to select resolution!\n");
> > > -		return -EINVAL;
> > > +		scale_input_x = OV9740_MAX_WIDTH;
> > > +		scale_input_y = (OV9740_MAX_WIDTH * height) / width;
> > >  	}
> > 
> > I don'z know how this sensor works, but the above two divisions round 
> > down. And these are input sizes. Cannot it possibly lead to 
> > the output 
> > window being smaller, than required? Maybe you have to round 
> > up (hint: 
> > use DIV_ROUND_UP())?
> 
> The intention is to do some ratio math without floating point 
> instructions,

Of course, DIV_ROUND_UP is integer maths only too, as well as (almost) all 
maths in the kernel.

> which resulted in some algebraic twiddling (which is why 
> that math looks so weird).

No, it doesn't look weird, I just wasn't sure, whether your maths would 
work in all situations. The only difference between yours and mine, is 
that yours rounds down, and mine rounds up. So, I'm not sure which one is 
better in this case.

> I think what's there is okay.  If there's 
> any rounding at all (and there shouldn't be any rounding, if "standard" 
> image dimensions are called for), then there's going to be some aspect 
> ratio weirdness no matter which way you round that division.

No, you should be prepared to handle all possible crazy resolution 
requests.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 5/5 v2] [media] ov9740: Add suspend/resume
  2011-06-01  1:58     ` Andrew Chew
@ 2011-06-01 19:51       ` Guennadi Liakhovetski
  0 siblings, 0 replies; 13+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-01 19:51 UTC (permalink / raw)
  To: Andrew Chew; +Cc: mchehab, olof, linux-media, linux-kernel

On Tue, 31 May 2011, Andrew Chew wrote:

> > > +	/* For suspend/resume. */
> > > +	struct v4l2_mbus_framefmt	current_mf;
> > > +	int				current_enable;
> > 
> > bool?
> 
> Are you sure you want this to be a bool?  This thing is trying to shadow 
> the "enable" parameter of the s_stream() callback, and that enable 
> parameter is int.

You use it as a bool.

> > > +static int ov9740_suspend(struct soc_camera_device *icd, 
> > pm_message_t state)
> > > +{
> > > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > > +	struct ov9740_priv *priv = to_ov9740(sd);
> > > +
> > > +	if (priv->current_enable) {
> > > +		int current_enable = priv->current_enable;
> > > +
> > > +		ov9740_s_stream(sd, 0);
> > > +		priv->current_enable = current_enable;
> > 
> > You don't need the local variable, just set
> > 
> > 	priv->current_enable = true;
> 
> I think I do need that local variable, the way the code is arranged now.  
> I'm trying to save the state of enablement inside of 
> priv->current_enable, at the time we are suspending, so it won't 
> necessarily be true.  And one of the side effects of calling 
> ov9740_s_stream(sd, 0) is that priv->current_enable will be set to 
> false, which is why I save off the value of priv->current_enable, and 
> then restore it after the call to ov9740_s_stream().

? Sorry, don't understand. You only _enter_ that "if" statement, if 
priv->current_enable is true. So, your local variable is _certainly_ true. 
And that's what you're then writing in priv->current_enable back again.

> > >  static struct soc_camera_ops ov9740_ops = {
> > > +	.suspend		= ov9740_suspend,
> > > +	.resume			= ov9740_resume,
> > 
> > No, we don't want to use these, whey should disappear some 
> > time... Please, 
> > use .s_power() from struct v4l2_subdev_core_ops, you can check 
> > http://article.gmane.org/gmane.linux.drivers.video-input-infra
> > structure/33105 
> > for an example. If your host is not using these ops, it has 
> > to be fixed. 
> > So far in the mainline only one soc-camera host driver is using these 
> > callbacks: pxa_camera.c, which, looking at your email 
> > address, I doubt is 
> > the driver, that you're using;)
> 
> Okay, will do.  Thanks for pointing that out :)
> 
> Is the camera host driver expected to directly call the sensor driver's 
> s_power (via v4l2_subdev_call(sd, core, s_power, <some value>)?  Or does 
> the v4l2 framework do this for you?  I didn't see an example of this in 
> my last pull of linux-next.

yes, the host driver has to call s_power explicitly.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-06-01 19:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-26  0:04 [PATCH 1/5 v2] [media] ov9740: Cleanup hex casing inconsistencies achew
2011-05-26  0:04 ` [PATCH 2/5 v2] [media] ov9740: Correct print in ov9740_reg_rmw() achew
2011-05-26  0:04 ` [PATCH 3/5 v2] [media] ov9740: Fixed some settings achew
2011-05-29 10:21   ` Guennadi Liakhovetski
2011-06-01  1:12     ` Andrew Chew
2011-05-26  0:04 ` [PATCH 4/5 v2] [media] ov9740: Remove hardcoded resolution regs achew
2011-05-29 10:36   ` Guennadi Liakhovetski
2011-06-01  1:50     ` Andrew Chew
2011-06-01 19:35       ` Guennadi Liakhovetski
2011-05-26  0:04 ` [PATCH 5/5 v2] [media] ov9740: Add suspend/resume achew
2011-05-29 10:58   ` Guennadi Liakhovetski
2011-06-01  1:58     ` Andrew Chew
2011-06-01 19:51       ` Guennadi Liakhovetski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.