Re: [PATCH] media: i2c: Add ov9734 image sensor driver

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Sergey, thanks for review.

On 11/17/20 4:07 PM, Sergey Senozhatsky wrote:
> On (20/10/29 10:59), Bingbu Cao wrote:
> [..]
> 
> Looks good to me,
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> Several comments below.
> 
>> +static int ov9734_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct ov9734 *ov9734 = container_of(ctrl->handler,
>> +					     struct ov9734, ctrl_handler);
>> +	struct i2c_client *client = v4l2_get_subdevdata(&ov9734->sd);
>> +	s64 exposure_max;
>> +	int ret = 0;
>> +
>> +	/* Propagate change of current control to all related controls */
>> +	if (ctrl->id == V4L2_CID_VBLANK) {
>> +		/* Update max exposure while meeting expected vblanking */
>> +		exposure_max = ov9734->cur_mode->height + ctrl->val -
>> +			OV9734_EXPOSURE_MAX_MARGIN;
>> +		__v4l2_ctrl_modify_range(ov9734->exposure,
>> +					 ov9734->exposure->minimum,
>> +					 exposure_max, ov9734->exposure->step,
>> +					 exposure_max);
>> +	}
> 
> Should this be done after pm_runtime_get_if_in_use()?

My inaccurate understanding - as v4l2 see that this control was set by sub-device without error,
so it will record the value from the user as the new value, so update the exposure range to
allow user to set the exposure as well even it did not take affect.

Sakari, do you have any comments about this?

> 
>> +	/* V4L2 controls values will be applied only when power is already up */
>> +	if (!pm_runtime_get_if_in_use(&client->dev))
>> +		return 0;
>> +
> 
> Here.
> 
> [..]
> 
>> +static int ov9734_set_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	struct ov9734 *ov9734 = to_ov9734(sd);
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	int ret = 0;
>> +
>> +	if (ov9734->streaming == enable)
>> +		return 0;
>> +
>> +	mutex_lock(&ov9734->mutex);
>> +	if (enable) {
>> +		ret = pm_runtime_get_sync(&client->dev);
>> +		if (ret < 0) {
>> +			pm_runtime_put_noidle(&client->dev);
>> +			mutex_unlock(&ov9734->mutex);
>> +			return ret;
>> +		}
>> +
>> +		ret = ov9734_start_streaming(ov9734);
>> +		if (ret) {
>> +			enable = 0;
>> +			ov9734_stop_streaming(ov9734);
>> +			pm_runtime_put(&client->dev);
>> +		}
>> +	} else {
>> +		ov9734_stop_streaming(ov9734);
>> +		pm_runtime_put(&client->dev);
>> +	}
> 
> I assume that we will never see erroneous ->s_stream(0) calls or
>  ->s_stream(0) after unsuccessful ->s_stream(1). Otherwise we will
> do extra pm_runtime_put(), not matched by pm_runtime_get().
> 
> 	-ss
> 

-- 
Best regards,
Bingbu Cao