Re: [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power()

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Laurent

On Fri, Mar 11, 2022 at 03:55:32PM +0200, Laurent Pinchart wrote:
> The subdev .s_power() operation is deprecated. Drop it, requiring sensor
> drivers to correctly use runtime PM instead of relying on .s_power().
>
> As this driver has just been moved out of staging, and necessary drivers
> to implement a full camera pipeline are still in staging, no platform
> depends yet on this API being called. There is thus no risk of
> regression.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 7baedc854186..6e06d19c1334 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -937,7 +937,7 @@ 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;
> +	int ret = 0;
>
>  	if (enable) {
>  		ret = mipi_csis_calculate_params(csis);
> @@ -949,10 +949,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  		ret = pm_runtime_resume_and_get(csis->dev);
>  		if (ret < 0)
>  			return ret;
> -
> -		ret = v4l2_subdev_call(csis->src_sd, core, s_power, 1);
> -		if (ret < 0 && ret != -ENOIOCTLCMD)
> -			goto done;
>  	}
>
>  	mutex_lock(&csis->lock);
> @@ -973,9 +969,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  		csis->state |= ST_STREAMING;
>  	} else {
>  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> -		ret = v4l2_subdev_call(csis->src_sd, core, s_power, 0);
> -		if (ret == -ENOIOCTLCMD)
> -			ret = 0;
> +

I think mipi_csis_s_stream() could be simplified even more

Now it looks like

                if (enable) {
                        pm_resume_and_get();
                        ...
                }

                mutex_lock();

                if (enable) {

                } else {

                }

        out:
                mutex_unlock()
                if (ret < 0 || !enable)
                        pm_runtime_put()

                return ret;

Would it look better as

                if (!enable) {
                        lock();
                        ...
                        pm_runtime_put();
                        ...
                        unlock();
                        return;
                }

                /* non critical section stuff */
                pm_resume_and_get();

                lock();

                /* critical section stuff */
                if (ret)
                        goto out;

                unlock();

                return 0;

        out:
                pm_runtime_put();
                return ret;

This patch is good though, so the rework if desired can be done on top
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>  		mipi_csis_stop_stream(csis);
>  		csis->state &= ~ST_STREAMING;
>  		if (csis->debug.enable)
> @@ -985,7 +979,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  unlock:
>  	mutex_unlock(&csis->lock);
>
> -done:
>  	if (!enable || ret < 0)
>  		pm_runtime_put(csis->dev);
>
> --
> Regards,
>
> Laurent Pinchart
>