Hi Jacopo,
Thank you for the patch.
On Sat, Mar 12, 2022 at 04:25:04PM +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@xxxxxxxxxx>
> ---
> 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..fa00b36fc394 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);
> +
> + pm_runtime_put(csis->dev);
> +
> + mutex_unlock(&csis->lock);
You can move the mutex_unlock() call before pm_runtime_put().
> +
> + 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:
> + mipi_csis_stop_stream(csis);
> + pm_runtime_put(csis->dev);
> + mutex_unlock(&csis->lock);
Here too.
When there's a single error path I'm tempted to just inline error
handling instead of using a goto, but I don't mind either way.
>
> return ret;
> }
--
Regards,
Laurent Pinchart
