On (20/10/29 10:59), Bingbu Cao wrote:
[..]
Looks good to me,
Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
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()?
> + /* 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
