[PATCH 1/2] media: ov5645: Supported external clock is 24MHz

[PATCH 1/2] media: ov5645: Supported external clock is 24MHz

[Todor Tomov]

The external clock frequency was set to 23.88MHz by mistake
because of a platform which cannot get closer to 24MHz.
The supported by the driver external clock is 24MHz so
set it correctly and also fix the values of the pixel
clock and link clock.
However allow 1% tolerance to the external clock as this
difference is small enough to be insignificant.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
---
 drivers/media/i2c/ov5645.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index b3f7625..1722cda 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -510,8 +510,8 @@ static const struct reg_value ov5645_setting_full[] = {
 };
 
 static const s64 link_freq[] = {
-	222880000,
-	334320000
+	224000000,
+	336000000
 };
 
 static const struct ov5645_mode_info ov5645_mode_info_data[] = {
@@ -520,7 +520,7 @@ static const struct ov5645_mode_info ov5645_mode_info_data[] = {
 		.height = 960,
 		.data = ov5645_setting_sxga,
 		.data_size = ARRAY_SIZE(ov5645_setting_sxga),
-		.pixel_clock = 111440000,
+		.pixel_clock = 112000000,
 		.link_freq = 0 /* an index in link_freq[] */
 	},
 	{
@@ -528,7 +528,7 @@ static const struct ov5645_mode_info ov5645_mode_info_data[] = {
 		.height = 1080,
 		.data = ov5645_setting_1080p,
 		.data_size = ARRAY_SIZE(ov5645_setting_1080p),
-		.pixel_clock = 167160000,
+		.pixel_clock = 168000000,
 		.link_freq = 1 /* an index in link_freq[] */
 	},
 	{
@@ -536,7 +536,7 @@ static const struct ov5645_mode_info ov5645_mode_info_data[] = {
 		.height = 1944,
 		.data = ov5645_setting_full,
 		.data_size = ARRAY_SIZE(ov5645_setting_full),
-		.pixel_clock = 167160000,
+		.pixel_clock = 168000000,
 		.link_freq = 1 /* an index in link_freq[] */
 	},
 };
@@ -1145,7 +1145,8 @@ static int ov5645_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (xclk_freq != 23880000) {
+	/* external clock must be 24MHz, allow 1% tolerance */
+	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
 		dev_err(dev, "external clock frequency %u is not supported\n",
 			xclk_freq);
 		return -EINVAL;
-- 
2.7.4

[PATCH 2/2] media: ov5645: Report number of skip frames

[Todor Tomov]

The OV5645 supports automatic exposure (AE) and automatic white
balance (AWB). When streaming is started it takes up to 5 frames
until the AE and AWB converge and output a frame with good quality.

Implement g_skip_frames to report number of frames to be skipped
when streaming is started.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
---
 drivers/media/i2c/ov5645.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 1722cda..00bc3c0 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -70,6 +70,9 @@
 #define OV5645_SDE_SAT_U		0x5583
 #define OV5645_SDE_SAT_V		0x5584
 
+/* Number of frames needed for AE and AWB to converge */
+#define OV5645_NUM_OF_SKIP_FRAMES 5
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 	return 0;
 }
 
+static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
+{
+	*frames = OV5645_NUM_OF_SKIP_FRAMES;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_core_ops ov5645_core_ops = {
 	.s_power = ov5645_s_power,
 };
@@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_selection = ov5645_get_selection,
 };
 
+static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = {
+	.g_skip_frames = ov5645_get_skip_frames,
+};
+
 static const struct v4l2_subdev_ops ov5645_subdev_ops = {
 	.core = &ov5645_core_ops,
 	.video = &ov5645_video_ops,
 	.pad = &ov5645_subdev_pad_ops,
+	.sensor = &ov5645_sensor_ops,
 };
 
 static int ov5645_probe(struct i2c_client *client,
-- 
2.7.4

Re: [PATCH 2/2] media: ov5645: Report number of skip frames

[Sakari Ailus]

Hi Todor,

On Mon, Jun 18, 2018 at 11:06:59AM +0300, Todor Tomov wrote:
> The OV5645 supports automatic exposure (AE) and automatic white
> balance (AWB). When streaming is started it takes up to 5 frames
> until the AE and AWB converge and output a frame with good quality.

The frames aren't bad as such; it's just that the AE hasn't converged yet.
I presume the number of the frames needed depends on the lighting
conditions.

The g_skip_frames is intended to tell the frames really are bad, i.e.
distorted or broken somehow.

I wouldn't discard them on the grounds of unconverged exposure. If we did,
then on which other grounds should the frames be discarded as well? Does
the white balance and focus need to be converged as well before considering
the frames good, for instance?

If you need this, I'd use a control instead to tell AE has converged.

I wonder what others think.

> 
> Implement g_skip_frames to report number of frames to be skipped
> when streaming is started.
> 
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> ---
>  drivers/media/i2c/ov5645.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 1722cda..00bc3c0 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -70,6 +70,9 @@
>  #define OV5645_SDE_SAT_U		0x5583
>  #define OV5645_SDE_SAT_V		0x5584
>  
> +/* Number of frames needed for AE and AWB to converge */
> +#define OV5645_NUM_OF_SKIP_FRAMES 5
> +
>  struct reg_value {
>  	u16 reg;
>  	u8 val;
> @@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  	return 0;
>  }
>  
> +static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> +{
> +	*frames = OV5645_NUM_OF_SKIP_FRAMES;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_core_ops ov5645_core_ops = {
>  	.s_power = ov5645_s_power,
>  };
> @@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  	.get_selection = ov5645_get_selection,
>  };
>  
> +static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = {
> +	.g_skip_frames = ov5645_get_skip_frames,
> +};
> +
>  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
>  	.core = &ov5645_core_ops,
>  	.video = &ov5645_video_ops,
>  	.pad = &ov5645_subdev_pad_ops,
> +	.sensor = &ov5645_sensor_ops,
>  };
>  
>  static int ov5645_probe(struct i2c_client *client,
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/2] media: ov5645: Report number of skip frames

[Todor Tomov]

Hi Sakari,

Thank you for the reply and sorry for not following up on this for so long.

On 19.06.2018 07:04, Sakari Ailus wrote:
> Hi Todor,
> 
> On Mon, Jun 18, 2018 at 11:06:59AM +0300, Todor Tomov wrote:
>> The OV5645 supports automatic exposure (AE) and automatic white
>> balance (AWB). When streaming is started it takes up to 5 frames
>> until the AE and AWB converge and output a frame with good quality.
> 
> The frames aren't bad as such; it's just that the AE hasn't converged yet.
> I presume the number of the frames needed depends on the lighting
> conditions.

Yes, the number of frames (for AE and AWB converge) is different in
different conditions. From testing I see usually 4 frames are needed,
in more rare cases - 5 frames. The sensor doesn't provide any information
about the state of the algorithms - are they currently converged or not.
So the driver has no way to check that or tell for sure.

> 
> The g_skip_frames is intended to tell the frames really are bad, i.e.
> distorted or broken somehow.

I was about to say that if the exposure is severely wrong then they are
bad enough to not be used, which means that they are anyway bad, however...

> 
> I wouldn't discard them on the grounds of unconverged exposure. If we did,
> then on which other grounds should the frames be discarded as well? Does
> the white balance and focus need to be converged as well before considering
> the frames good, for instance?

...I agree that this measure for a bad vs not bad frame is quite subjective
(when we have no feedback about the state of the algorithms).

> 
> If you need this, I'd use a control instead to tell AE has converged.
> 
> I wonder what others think.

It would be nice to hear any ideas if anyone has any. I wonder whether the
driver can do anything useful besides leaving it to the userspace.

Best regards,
Todor

> 
>>
>> Implement g_skip_frames to report number of frames to be skipped
>> when streaming is started.
>>
>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>> ---
>>  drivers/media/i2c/ov5645.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>> index 1722cda..00bc3c0 100644
>> --- a/drivers/media/i2c/ov5645.c
>> +++ b/drivers/media/i2c/ov5645.c
>> @@ -70,6 +70,9 @@
>>  #define OV5645_SDE_SAT_U		0x5583
>>  #define OV5645_SDE_SAT_V		0x5584
>>  
>> +/* Number of frames needed for AE and AWB to converge */
>> +#define OV5645_NUM_OF_SKIP_FRAMES 5
>> +
>>  struct reg_value {
>>  	u16 reg;
>>  	u8 val;
>> @@ -1071,6 +1074,13 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>>  	return 0;
>>  }
>>  
>> +static int ov5645_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>> +{
>> +	*frames = OV5645_NUM_OF_SKIP_FRAMES;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct v4l2_subdev_core_ops ov5645_core_ops = {
>>  	.s_power = ov5645_s_power,
>>  };
>> @@ -1088,10 +1098,15 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>>  	.get_selection = ov5645_get_selection,
>>  };
>>  
>> +static const struct v4l2_subdev_sensor_ops ov5645_sensor_ops = {
>> +	.g_skip_frames = ov5645_get_skip_frames,
>> +};
>> +
>>  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
>>  	.core = &ov5645_core_ops,
>>  	.video = &ov5645_video_ops,
>>  	.pad = &ov5645_subdev_pad_ops,
>> +	.sensor = &ov5645_sensor_ops,
>>  };
>>  
>>  static int ov5645_probe(struct i2c_client *client,
>> -- 
>> 2.7.4
>>
>