Re: [PATCH 1/5] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 11:39:37AM +0100, Jacopo Mondi wrote:
> Simplify the mipi_csis_s_stream() function.
> 
> This actually fixes a bug, as if calling the subdev's s_stream(1) fails,
> mipi_csis_stop_stream() was not called.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 58 ++++++++++++----------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index c4d1a6fe5007..60e7f0452582 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -910,43 +910,51 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
>  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> -	int ret = 0;
> +	int ret;
>  
> -	if (enable) {
> -		ret = mipi_csis_calculate_params(csis);
> -		if (ret < 0)
> -			return ret;
> +	if (!enable) {
> +		mutex_lock(&csis->lock);
>  
> -		mipi_csis_clear_counters(csis);
> +		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
>  
> -		ret = pm_runtime_resume_and_get(csis->dev);
> -		if (ret < 0)
> -			return ret;
> +		mipi_csis_stop_stream(csis);
> +		if (csis->debug.enable)
> +			mipi_csis_log_counters(csis, true);
> +
> +		mutex_unlock(&csis->lock);
> +
> +		pm_runtime_put(csis->dev);
> +
> +		return 0;
>  	}
>  
> -	mutex_lock(&csis->lock);
> +	ret = mipi_csis_calculate_params(csis);
> +	if (ret < 0)
> +		return ret;
>  
> -	if (enable) {
> -		mipi_csis_start_stream(csis);
> -		ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
> -		if (ret < 0)
> -			goto unlock;
> +	mipi_csis_clear_counters(csis);
>  
> -		mipi_csis_log_counters(csis, true);
> -	} else {
> -		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> +	ret = pm_runtime_resume_and_get(csis->dev);
> +	if (ret < 0)
> +		return ret;
>  
> -		mipi_csis_stop_stream(csis);
> +	mutex_lock(&csis->lock);
>  
> -		if (csis->debug.enable)
> -			mipi_csis_log_counters(csis, true);
> -	}
> +	mipi_csis_start_stream(csis);
> +	ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	mipi_csis_log_counters(csis, true);
>  
> -unlock:
>  	mutex_unlock(&csis->lock);
>  
> -	if (!enable || ret < 0)
> -		pm_runtime_put(csis->dev);
> +	return 0;
> +
> +out:

I'd name the label error instead of out, as "out" and "done" usually
refer to either normal code paths or combined success+error paths. With
that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll prepare a consolidated series with other pending patches and send a
pull request. If this is the only change, and if you're fine with I'll,
I can update this locally.

> +	mipi_csis_stop_stream(csis);
> +	mutex_unlock(&csis->lock);
> +	pm_runtime_put(csis->dev);
>  
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart