Hi Jacopo,
On 08/25/2018 04:53 PM, jacopo mondi wrote:
> Hi Hugues,
> one more comment on this patch..
>
> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>> Mode setting depends on last mode set, in particular
>> because of exposure calculation when downscale mode
>> change between subsampling and scaling.
>> At stream on the last mode was wrongly set to current mode,
>> so no change was detected and exposure calculation
>> was not made, fix this.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruc...@st.com>
>> ---
>> drivers/media/i2c/ov5640.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index c110a6a..923cc30 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -225,6 +225,7 @@ struct ov5640_dev {
>> struct v4l2_mbus_framefmt fmt;
>>
>> const struct ov5640_mode_info *current_mode;
>> + const struct ov5640_mode_info *last_mode;
>> enum ov5640_frame_rate current_fr;
>> struct v4l2_fract frame_interval;
>>
>> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
>> int ret;
>>
>> + if (!orig_mode)
>> + orig_mode = mode;
>> +
>
> Am I wrong or with the introduction of last_mode we could drop the
> 'orig_mode' parameter (which has confused me already :/ ) from the
> set_mode() function?
>
> Just set here 'orig_mode = sensor->last_mode' and make sure last_mode
> is intialized properly at probe time...
>
> Or is there some other value in keeping the orig_mode parameter here?
>
> Thanks
> j
I'm fine with that change, will push it in v3.
>
>> dn_mode = mode->dn_mode;
>> orig_dn_mode = orig_mode->dn_mode;
>>
>> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>> return ret;
>>
>> sensor->pending_mode_change = false;
>> + sensor->last_mode = mode;
>>
>> return 0;
>>
>> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int
>> enable)
>>
>> if (sensor->streaming == !enable) {
>> if (enable && sensor->pending_mode_change) {
>> - ret = ov5640_set_mode(sensor, sensor->current_mode);
>> + ret = ov5640_set_mode(sensor, sensor->last_mode);
>> +
>> if (ret)
>> goto out;
>>
>> --
>> 2.7.4
>>
BR Hugues.
