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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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
>
