[PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support

[PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support

[Laurent Pinchart]

Hello,

This small patch series results from work on suspend/resume support in
the (in progress) ISI driver. It drops unneeded code in the
imx-mipi-csis related to system suspend/resume, allowing a
simplification of the runtime PM handlers.

Laurent Pinchart (4):
  media: imx: imx-mipi-csis: Don't use .s_power()
  media: imx: imx-mipi-csis: Drop unneeded system PM implementation
  media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend
    time
  media: imx: imx-mipi-csis: Simplify runtime PM implementation

 drivers/media/platform/imx/imx-mipi-csis.c | 138 +++++++--------------
 1 file changed, 44 insertions(+), 94 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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;
+
 		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

[PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation

[Laurent Pinchart]

There's no need to implement system suspend/resume manually, as video
pipelines are supposed to be suspended in a controlled and ordered
manner by the data sink driver at system suspend time (and similarly at
resume time). Drop the system suspend/resume handlers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 48 +++-------------------
 1 file changed, 5 insertions(+), 43 deletions(-)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 6e06d19c1334..3bdfe05a6c54 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -250,8 +250,6 @@
 
 enum {
 	ST_POWERED	= 1,
-	ST_STREAMING	= 2,
-	ST_SUSPENDED	= 4,
 };
 
 struct mipi_csis_event {
@@ -954,24 +952,17 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	mutex_lock(&csis->lock);
 
 	if (enable) {
-		if (csis->state & ST_SUSPENDED) {
-			ret = -EBUSY;
-			goto unlock;
-		}
-
 		mipi_csis_start_stream(csis);
 		ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
 		if (ret < 0)
 			goto unlock;
 
 		mipi_csis_log_counters(csis, true);
-
-		csis->state |= ST_STREAMING;
 	} else {
 		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
 
 		mipi_csis_stop_stream(csis);
-		csis->state &= ~ST_STREAMING;
+
 		if (csis->debug.enable)
 			mipi_csis_log_counters(csis, true);
 	}
@@ -1356,7 +1347,7 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
  * Suspend/resume
  */
 
-static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
+static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
@@ -1370,8 +1361,6 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
 			goto unlock;
 		mipi_csis_clk_disable(csis);
 		csis->state &= ~ST_POWERED;
-		if (!runtime)
-			csis->state |= ST_SUSPENDED;
 	}
 
 unlock:
@@ -1380,15 +1369,13 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
 	return ret ? -EAGAIN : 0;
 }
 
-static int mipi_csis_pm_resume(struct device *dev, bool runtime)
+static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
 	int ret = 0;
 
 	mutex_lock(&csis->lock);
-	if (!runtime && !(csis->state & ST_SUSPENDED))
-		goto unlock;
 
 	if (!(csis->state & ST_POWERED)) {
 		ret = mipi_csis_phy_enable(csis);
@@ -1398,10 +1385,6 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
 		csis->state |= ST_POWERED;
 		mipi_csis_clk_enable(csis);
 	}
-	if (csis->state & ST_STREAMING)
-		mipi_csis_start_stream(csis);
-
-	csis->state &= ~ST_SUSPENDED;
 
 unlock:
 	mutex_unlock(&csis->lock);
@@ -1409,30 +1392,9 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
 	return ret ? -EAGAIN : 0;
 }
 
-static int __maybe_unused mipi_csis_suspend(struct device *dev)
-{
-	return mipi_csis_pm_suspend(dev, false);
-}
-
-static int __maybe_unused mipi_csis_resume(struct device *dev)
-{
-	return mipi_csis_pm_resume(dev, false);
-}
-
-static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
-{
-	return mipi_csis_pm_suspend(dev, true);
-}
-
-static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
-{
-	return mipi_csis_pm_resume(dev, true);
-}
-
 static const struct dev_pm_ops mipi_csis_pm_ops = {
 	SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
 };
 
 /* -----------------------------------------------------------------------------
@@ -1557,7 +1519,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	/* Enable runtime PM. */
 	pm_runtime_enable(dev);
 	if (!pm_runtime_enabled(dev)) {
-		ret = mipi_csis_pm_resume(dev, true);
+		ret = mipi_csis_runtime_resume(dev);
 		if (ret < 0)
 			goto unregister_all;
 	}
@@ -1592,7 +1554,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
 	v4l2_async_unregister_subdev(&csis->sd);
 
 	pm_runtime_disable(&pdev->dev);
-	mipi_csis_pm_suspend(&pdev->dev, true);
+	mipi_csis_runtime_suspend(&pdev->dev);
 	mipi_csis_clk_disable(csis);
 	media_entity_cleanup(&csis->sd.entity);
 	mutex_destroy(&csis->lock);
-- 
Regards,

Laurent Pinchart

[PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time

[Laurent Pinchart]

Streaming is guaranteed to have been stopped by the time the device gets
runtime suspended, as pm_runtime_put() is called from .s_stream(0) only.
Drop the manual stop.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 3bdfe05a6c54..d656b8bfcc33 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -1355,7 +1355,6 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
 
 	mutex_lock(&csis->lock);
 	if (csis->state & ST_POWERED) {
-		mipi_csis_stop_stream(csis);
 		ret = mipi_csis_phy_disable(csis);
 		if (ret)
 			goto unlock;
-- 
Regards,

Laurent Pinchart

[PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Laurent Pinchart]

The runtime PM resume handler is guaranteed to be called on a suspended
device, and the suspend handler on a resumed device. The implementation
can thus be simplified.

While at it, rename the mipi_csis_device state field to powered, as the
now state contains a single flag only.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index d656b8bfcc33..f6ff8d50843c 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -248,10 +248,6 @@
 #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
 #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
 
-enum {
-	ST_POWERED	= 1,
-};
-
 struct mipi_csis_event {
 	bool debug;
 	u32 mask;
@@ -331,10 +327,10 @@ struct mipi_csis_device {
 	u32 hs_settle;
 	u32 clk_settle;
 
-	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
+	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
 	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
-	u32 state;
+	bool powered;
 
 	spinlock_t slock;	/* Protect events */
 	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
@@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
 
 	mutex_lock(&csis->lock);
 	mipi_csis_log_counters(csis, true);
-	if (csis->debug.enable && (csis->state & ST_POWERED))
+	if (csis->debug.enable && csis->powered)
 		mipi_csis_dump_regs(csis);
 	mutex_unlock(&csis->lock);
 
@@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
 	int ret = 0;
 
 	mutex_lock(&csis->lock);
-	if (csis->state & ST_POWERED) {
-		ret = mipi_csis_phy_disable(csis);
-		if (ret)
-			goto unlock;
-		mipi_csis_clk_disable(csis);
-		csis->state &= ~ST_POWERED;
-	}
+
+	ret = mipi_csis_phy_disable(csis);
+	if (ret)
+		goto unlock;
+
+	mipi_csis_clk_disable(csis);
+
+	csis->powered = false;
 
 unlock:
 	mutex_unlock(&csis->lock);
@@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
 
 	mutex_lock(&csis->lock);
 
-	if (!(csis->state & ST_POWERED)) {
-		ret = mipi_csis_phy_enable(csis);
-		if (ret)
-			goto unlock;
+	ret = mipi_csis_phy_enable(csis);
+	if (ret)
+		goto unlock;
 
-		csis->state |= ST_POWERED;
-		mipi_csis_clk_enable(csis);
-	}
+	mipi_csis_clk_enable(csis);
+
+	csis->powered = true;
 
 unlock:
 	mutex_unlock(&csis->lock);
-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support

[Laurent Pinchart]

On Fri, Mar 11, 2022 at 03:55:31PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This small patch series results from work on suspend/resume support in
> the (in progress) ISI driver. It drops unneeded code in the
> imx-mipi-csis related to system suspend/resume, allowing a
> simplification of the runtime PM handlers.

I forgot to include a prerequisite in the series, renaming csi_state to
mipi_csis_device. The series will thus not apply. I'll post that patch
shortly, but as it's just a rename, this series should still be
reviewable separately.

> Laurent Pinchart (4):
>   media: imx: imx-mipi-csis: Don't use .s_power()
>   media: imx: imx-mipi-csis: Drop unneeded system PM implementation
>   media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend
>     time
>   media: imx: imx-mipi-csis: Simplify runtime PM implementation
> 
>  drivers/media/platform/imx/imx-mipi-csis.c | 138 +++++++--------------
>  1 file changed, 44 insertions(+), 94 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Jacopo Mondi]

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
>

Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation

[Jacopo Mondi]

Hi Laurent,

   the commit title makes me think runtime_pm has been dropped

I would

media: imx: imx-mipi-csis: Drop manual PM tracking

On Fri, Mar 11, 2022 at 03:55:33PM +0200, Laurent Pinchart wrote:
> There's no need to implement system suspend/resume manually, as video
> pipelines are supposed to be suspended in a controlled and ordered
> manner by the data sink driver at system suspend time (and similarly at
> resume time). Drop the system suspend/resume handlers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 48 +++-------------------
>  1 file changed, 5 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 6e06d19c1334..3bdfe05a6c54 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -250,8 +250,6 @@
>
>  enum {
>  	ST_POWERED	= 1,
> -	ST_STREAMING	= 2,
> -	ST_SUSPENDED	= 4,
>  };

I see csis->state be dropped in the next patches so
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


>
>  struct mipi_csis_event {
> @@ -954,24 +952,17 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  	mutex_lock(&csis->lock);
>
>  	if (enable) {
> -		if (csis->state & ST_SUSPENDED) {
> -			ret = -EBUSY;
> -			goto unlock;
> -		}
> -
>  		mipi_csis_start_stream(csis);
>  		ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
>  		if (ret < 0)
>  			goto unlock;
>
>  		mipi_csis_log_counters(csis, true);
> -
> -		csis->state |= ST_STREAMING;
>  	} else {
>  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
>
>  		mipi_csis_stop_stream(csis);
> -		csis->state &= ~ST_STREAMING;
> +
>  		if (csis->debug.enable)
>  			mipi_csis_log_counters(csis, true);
>  	}
> @@ -1356,7 +1347,7 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
>   * Suspend/resume
>   */
>
> -static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> +static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> @@ -1370,8 +1361,6 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
>  			goto unlock;
>  		mipi_csis_clk_disable(csis);
>  		csis->state &= ~ST_POWERED;
> -		if (!runtime)
> -			csis->state |= ST_SUSPENDED;
>  	}
>
>  unlock:
> @@ -1380,15 +1369,13 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
>  	return ret ? -EAGAIN : 0;
>  }
>
> -static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> +static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
>  	int ret = 0;
>
>  	mutex_lock(&csis->lock);
> -	if (!runtime && !(csis->state & ST_SUSPENDED))
> -		goto unlock;
>
>  	if (!(csis->state & ST_POWERED)) {
>  		ret = mipi_csis_phy_enable(csis);
> @@ -1398,10 +1385,6 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
>  		csis->state |= ST_POWERED;
>  		mipi_csis_clk_enable(csis);
>  	}
> -	if (csis->state & ST_STREAMING)
> -		mipi_csis_start_stream(csis);
> -
> -	csis->state &= ~ST_SUSPENDED;
>
>  unlock:
>  	mutex_unlock(&csis->lock);
> @@ -1409,30 +1392,9 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
>  	return ret ? -EAGAIN : 0;
>  }
>
> -static int __maybe_unused mipi_csis_suspend(struct device *dev)
> -{
> -	return mipi_csis_pm_suspend(dev, false);
> -}
> -
> -static int __maybe_unused mipi_csis_resume(struct device *dev)
> -{
> -	return mipi_csis_pm_resume(dev, false);
> -}
> -
> -static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> -{
> -	return mipi_csis_pm_suspend(dev, true);
> -}
> -
> -static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> -{
> -	return mipi_csis_pm_resume(dev, true);
> -}
> -
>  static const struct dev_pm_ops mipi_csis_pm_ops = {
>  	SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
>  			   NULL)
> -	SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
>  };
>
>  /* -----------------------------------------------------------------------------
> @@ -1557,7 +1519,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
>  	/* Enable runtime PM. */
>  	pm_runtime_enable(dev);
>  	if (!pm_runtime_enabled(dev)) {
> -		ret = mipi_csis_pm_resume(dev, true);
> +		ret = mipi_csis_runtime_resume(dev);
>  		if (ret < 0)
>  			goto unregister_all;
>  	}
> @@ -1592,7 +1554,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
>  	v4l2_async_unregister_subdev(&csis->sd);
>
>  	pm_runtime_disable(&pdev->dev);
> -	mipi_csis_pm_suspend(&pdev->dev, true);
> +	mipi_csis_runtime_suspend(&pdev->dev);
>  	mipi_csis_clk_disable(csis);
>  	media_entity_cleanup(&csis->sd.entity);
>  	mutex_destroy(&csis->lock);
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time

[Jacopo Mondi]

On Fri, Mar 11, 2022 at 03:55:34PM +0200, Laurent Pinchart wrote:
> Streaming is guaranteed to have been stopped by the time the device gets
> runtime suspended, as pm_runtime_put() is called from .s_stream(0) only.
> Drop the manual stop.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 3bdfe05a6c54..d656b8bfcc33 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -1355,7 +1355,6 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
>
>  	mutex_lock(&csis->lock);
>  	if (csis->state & ST_POWERED) {
> -		mipi_csis_stop_stream(csis);
>  		ret = mipi_csis_phy_disable(csis);
>  		if (ret)
>  			goto unlock;
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Jacopo Mondi]

Hi Laurent,

On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote:
> The runtime PM resume handler is guaranteed to be called on a suspended
> device, and the suspend handler on a resumed device. The implementation
> can thus be simplified.
>
> While at it, rename the mipi_csis_device state field to powered, as the
> now state contains a single flag only.

Can we instead rely on pm_runtime_get_if_in_use() instead of manual
tracking the power state ?

After all, the powered flag is only used in:

static int mipi_csis_log_status(struct v4l2_subdev *sd)
{
	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);

	mutex_lock(&csis->lock);
	mipi_csis_log_counters(csis, true);
	if (csis->debug.enable && csis->powered)
		mipi_csis_dump_regs(csis);
	mutex_unlock(&csis->lock);

	return 0;
}

which could be simplified as

static int mipi_csis_log_status(struct v4l2_subdev *sd)
{
	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);

	mipi_csis_log_counters(csis, true);

	if (!csis->debug.enable)
                return 0;

	mutex_lock(&csis->lock);

        if (!pm_runtime_get_if_in_use())
                goto unlock;

        mipi_csis_dump_regs(csis);

        pm_runtime_put();

unlock:
	mutex_unlock(&csis->lock);

	return 0;
}

Thanks
  j

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index d656b8bfcc33..f6ff8d50843c 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -248,10 +248,6 @@
>  #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
>  #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
>
> -enum {
> -	ST_POWERED	= 1,
> -};
> -
>  struct mipi_csis_event {
>  	bool debug;
>  	u32 mask;
> @@ -331,10 +327,10 @@ struct mipi_csis_device {
>  	u32 hs_settle;
>  	u32 clk_settle;
>
> -	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
> +	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
>  	const struct csis_pix_format *csis_fmt;
>  	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> -	u32 state;
> +	bool powered;
>
>  	spinlock_t slock;	/* Protect events */
>  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
>
>  	mutex_lock(&csis->lock);
>  	mipi_csis_log_counters(csis, true);
> -	if (csis->debug.enable && (csis->state & ST_POWERED))
> +	if (csis->debug.enable && csis->powered)
>  		mipi_csis_dump_regs(csis);
>  	mutex_unlock(&csis->lock);
>
> @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
>  	int ret = 0;
>
>  	mutex_lock(&csis->lock);
> -	if (csis->state & ST_POWERED) {
> -		ret = mipi_csis_phy_disable(csis);
> -		if (ret)
> -			goto unlock;
> -		mipi_csis_clk_disable(csis);
> -		csis->state &= ~ST_POWERED;
> -	}
> +
> +	ret = mipi_csis_phy_disable(csis);
> +	if (ret)
> +		goto unlock;
> +
> +	mipi_csis_clk_disable(csis);
> +
> +	csis->powered = false;
>
>  unlock:
>  	mutex_unlock(&csis->lock);
> @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
>
>  	mutex_lock(&csis->lock);
>
> -	if (!(csis->state & ST_POWERED)) {
> -		ret = mipi_csis_phy_enable(csis);
> -		if (ret)
> -			goto unlock;
> +	ret = mipi_csis_phy_enable(csis);
> +	if (ret)
> +		goto unlock;
>
> -		csis->state |= ST_POWERED;
> -		mipi_csis_clk_enable(csis);
> -	}
> +	mipi_csis_clk_enable(csis);
> +
> +	csis->powered = true;
>
>  unlock:
>  	mutex_unlock(&csis->lock);
> --
> Regards,
>
> Laurent Pinchart
>

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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Mar 11, 2022 at 05:34:43PM +0100, Jacopo Mondi wrote:
> 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;

Additional cleanup is likely possible, I haven't really looked at that.
I was happy anough with a first simplification :-)

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

Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation

[Laurent Pinchart]

Hi Jacopo,

On Fri, Mar 11, 2022 at 05:53:25PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
> 
>    the commit title makes me think runtime_pm has been dropped
> 
> I would
> 
> media: imx: imx-mipi-csis: Drop manual PM tracking

I wrote "system PM" specifically to avoid implying runtime PM was
dropped, but it seems it's not enough ? :-) I can change the subject,
but I find "manual PM tracking" not very descriptive of what the patch
does.

> On Fri, Mar 11, 2022 at 03:55:33PM +0200, Laurent Pinchart wrote:
> > There's no need to implement system suspend/resume manually, as video
> > pipelines are supposed to be suspended in a controlled and ordered
> > manner by the data sink driver at system suspend time (and similarly at
> > resume time). Drop the system suspend/resume handlers.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/imx/imx-mipi-csis.c | 48 +++-------------------
> >  1 file changed, 5 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index 6e06d19c1334..3bdfe05a6c54 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -250,8 +250,6 @@
> >
> >  enum {
> >  	ST_POWERED	= 1,
> > -	ST_STREAMING	= 2,
> > -	ST_SUSPENDED	= 4,
> >  };
> 
> I see csis->state be dropped in the next patches so
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  struct mipi_csis_event {
> > @@ -954,24 +952,17 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> >  	mutex_lock(&csis->lock);
> >
> >  	if (enable) {
> > -		if (csis->state & ST_SUSPENDED) {
> > -			ret = -EBUSY;
> > -			goto unlock;
> > -		}
> > -
> >  		mipi_csis_start_stream(csis);
> >  		ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
> >  		if (ret < 0)
> >  			goto unlock;
> >
> >  		mipi_csis_log_counters(csis, true);
> > -
> > -		csis->state |= ST_STREAMING;
> >  	} else {
> >  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> >
> >  		mipi_csis_stop_stream(csis);
> > -		csis->state &= ~ST_STREAMING;
> > +
> >  		if (csis->debug.enable)
> >  			mipi_csis_log_counters(csis, true);
> >  	}
> > @@ -1356,7 +1347,7 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> >   * Suspend/resume
> >   */
> >
> > -static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> > +static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > @@ -1370,8 +1361,6 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> >  			goto unlock;
> >  		mipi_csis_clk_disable(csis);
> >  		csis->state &= ~ST_POWERED;
> > -		if (!runtime)
> > -			csis->state |= ST_SUSPENDED;
> >  	}
> >
> >  unlock:
> > @@ -1380,15 +1369,13 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> >  	return ret ? -EAGAIN : 0;
> >  }
> >
> > -static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> > +static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> >  	int ret = 0;
> >
> >  	mutex_lock(&csis->lock);
> > -	if (!runtime && !(csis->state & ST_SUSPENDED))
> > -		goto unlock;
> >
> >  	if (!(csis->state & ST_POWERED)) {
> >  		ret = mipi_csis_phy_enable(csis);
> > @@ -1398,10 +1385,6 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> >  		csis->state |= ST_POWERED;
> >  		mipi_csis_clk_enable(csis);
> >  	}
> > -	if (csis->state & ST_STREAMING)
> > -		mipi_csis_start_stream(csis);
> > -
> > -	csis->state &= ~ST_SUSPENDED;
> >
> >  unlock:
> >  	mutex_unlock(&csis->lock);
> > @@ -1409,30 +1392,9 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> >  	return ret ? -EAGAIN : 0;
> >  }
> >
> > -static int __maybe_unused mipi_csis_suspend(struct device *dev)
> > -{
> > -	return mipi_csis_pm_suspend(dev, false);
> > -}
> > -
> > -static int __maybe_unused mipi_csis_resume(struct device *dev)
> > -{
> > -	return mipi_csis_pm_resume(dev, false);
> > -}
> > -
> > -static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> > -{
> > -	return mipi_csis_pm_suspend(dev, true);
> > -}
> > -
> > -static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> > -{
> > -	return mipi_csis_pm_resume(dev, true);
> > -}
> > -
> >  static const struct dev_pm_ops mipi_csis_pm_ops = {
> >  	SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> >  			   NULL)
> > -	SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> >  };
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -1557,7 +1519,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >  	/* Enable runtime PM. */
> >  	pm_runtime_enable(dev);
> >  	if (!pm_runtime_enabled(dev)) {
> > -		ret = mipi_csis_pm_resume(dev, true);
> > +		ret = mipi_csis_runtime_resume(dev);
> >  		if (ret < 0)
> >  			goto unregister_all;
> >  	}
> > @@ -1592,7 +1554,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
> >  	v4l2_async_unregister_subdev(&csis->sd);
> >
> >  	pm_runtime_disable(&pdev->dev);
> > -	mipi_csis_pm_suspend(&pdev->dev, true);
> > +	mipi_csis_runtime_suspend(&pdev->dev);
> >  	mipi_csis_clk_disable(csis);
> >  	media_entity_cleanup(&csis->sd.entity);
> >  	mutex_destroy(&csis->lock);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Laurent Pinchart]

Hi Jacopo,

On Fri, Mar 11, 2022 at 06:00:45PM +0100, Jacopo Mondi wrote:
> On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote:
> > The runtime PM resume handler is guaranteed to be called on a suspended
> > device, and the suspend handler on a resumed device. The implementation
> > can thus be simplified.
> >
> > While at it, rename the mipi_csis_device state field to powered, as the
> > now state contains a single flag only.
> 
> Can we instead rely on pm_runtime_get_if_in_use() instead of manual
> tracking the power state ?
> 
> After all, the powered flag is only used in:
> 
> static int mipi_csis_log_status(struct v4l2_subdev *sd)
> {
> 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> 
> 	mutex_lock(&csis->lock);
> 	mipi_csis_log_counters(csis, true);
> 	if (csis->debug.enable && csis->powered)
> 		mipi_csis_dump_regs(csis);
> 	mutex_unlock(&csis->lock);
> 
> 	return 0;
> }
> 
> which could be simplified as
> 
> static int mipi_csis_log_status(struct v4l2_subdev *sd)
> {
> 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> 
> 	mipi_csis_log_counters(csis, true);
> 
> 	if (!csis->debug.enable)
>                 return 0;
> 
> 	mutex_lock(&csis->lock);
> 
>         if (!pm_runtime_get_if_in_use())
>                 goto unlock;
> 
>         mipi_csis_dump_regs(csis);
> 
>         pm_runtime_put();
> 
> unlock:
> 	mutex_unlock(&csis->lock);
> 
> 	return 0;
> }

That's a good idea. Do you mind if I do so on a patch on top of this
one, to not mix two separate changes ?

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
> >  1 file changed, 17 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index d656b8bfcc33..f6ff8d50843c 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -248,10 +248,6 @@
> >  #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
> >  #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
> >
> > -enum {
> > -	ST_POWERED	= 1,
> > -};
> > -
> >  struct mipi_csis_event {
> >  	bool debug;
> >  	u32 mask;
> > @@ -331,10 +327,10 @@ struct mipi_csis_device {
> >  	u32 hs_settle;
> >  	u32 clk_settle;
> >
> > -	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
> > +	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
> >  	const struct csis_pix_format *csis_fmt;
> >  	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> > -	u32 state;
> > +	bool powered;
> >
> >  	spinlock_t slock;	/* Protect events */
> >  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> > @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >
> >  	mutex_lock(&csis->lock);
> >  	mipi_csis_log_counters(csis, true);
> > -	if (csis->debug.enable && (csis->state & ST_POWERED))
> > +	if (csis->debug.enable && csis->powered)
> >  		mipi_csis_dump_regs(csis);
> >  	mutex_unlock(&csis->lock);
> >
> > @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> >  	int ret = 0;
> >
> >  	mutex_lock(&csis->lock);
> > -	if (csis->state & ST_POWERED) {
> > -		ret = mipi_csis_phy_disable(csis);
> > -		if (ret)
> > -			goto unlock;
> > -		mipi_csis_clk_disable(csis);
> > -		csis->state &= ~ST_POWERED;
> > -	}
> > +
> > +	ret = mipi_csis_phy_disable(csis);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	mipi_csis_clk_disable(csis);
> > +
> > +	csis->powered = false;
> >
> >  unlock:
> >  	mutex_unlock(&csis->lock);
> > @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> >
> >  	mutex_lock(&csis->lock);
> >
> > -	if (!(csis->state & ST_POWERED)) {
> > -		ret = mipi_csis_phy_enable(csis);
> > -		if (ret)
> > -			goto unlock;
> > +	ret = mipi_csis_phy_enable(csis);
> > +	if (ret)
> > +		goto unlock;
> >
> > -		csis->state |= ST_POWERED;
> > -		mipi_csis_clk_enable(csis);
> > -	}
> > +	mipi_csis_clk_enable(csis);
> > +
> > +	csis->powered = true;
> >
> >  unlock:
> >  	mutex_unlock(&csis->lock);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation

[Jacopo Mondi]

Hi LAurent

On Fri, Mar 11, 2022 at 07:07:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Mar 11, 2022 at 05:53:25PM +0100, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> >    the commit title makes me think runtime_pm has been dropped
> >
> > I would
> >
> > media: imx: imx-mipi-csis: Drop manual PM tracking
>
> I wrote "system PM" specifically to avoid implying runtime PM was
> dropped, but it seems it's not enough ? :-) I can change the subject,
> but I find "manual PM tracking" not very descriptive of what the patch
> does.

Fine with what you have then, if the meaning was the same :)

>
> > On Fri, Mar 11, 2022 at 03:55:33PM +0200, Laurent Pinchart wrote:
> > > There's no need to implement system suspend/resume manually, as video
> > > pipelines are supposed to be suspended in a controlled and ordered
> > > manner by the data sink driver at system suspend time (and similarly at
> > > resume time). Drop the system suspend/resume handlers.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/imx/imx-mipi-csis.c | 48 +++-------------------
> > >  1 file changed, 5 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > index 6e06d19c1334..3bdfe05a6c54 100644
> > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > @@ -250,8 +250,6 @@
> > >
> > >  enum {
> > >  	ST_POWERED	= 1,
> > > -	ST_STREAMING	= 2,
> > > -	ST_SUSPENDED	= 4,
> > >  };
> >
> > I see csis->state be dropped in the next patches so
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > >  struct mipi_csis_event {
> > > @@ -954,24 +952,17 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	mutex_lock(&csis->lock);
> > >
> > >  	if (enable) {
> > > -		if (csis->state & ST_SUSPENDED) {
> > > -			ret = -EBUSY;
> > > -			goto unlock;
> > > -		}
> > > -
> > >  		mipi_csis_start_stream(csis);
> > >  		ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
> > >  		if (ret < 0)
> > >  			goto unlock;
> > >
> > >  		mipi_csis_log_counters(csis, true);
> > > -
> > > -		csis->state |= ST_STREAMING;
> > >  	} else {
> > >  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> > >
> > >  		mipi_csis_stop_stream(csis);
> > > -		csis->state &= ~ST_STREAMING;
> > > +
> > >  		if (csis->debug.enable)
> > >  			mipi_csis_log_counters(csis, true);
> > >  	}
> > > @@ -1356,7 +1347,7 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> > >   * Suspend/resume
> > >   */
> > >
> > > -static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> > > +static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> > >  {
> > >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > @@ -1370,8 +1361,6 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> > >  			goto unlock;
> > >  		mipi_csis_clk_disable(csis);
> > >  		csis->state &= ~ST_POWERED;
> > > -		if (!runtime)
> > > -			csis->state |= ST_SUSPENDED;
> > >  	}
> > >
> > >  unlock:
> > > @@ -1380,15 +1369,13 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> > >  	return ret ? -EAGAIN : 0;
> > >  }
> > >
> > > -static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> > > +static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> > >  {
> > >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > >  	int ret = 0;
> > >
> > >  	mutex_lock(&csis->lock);
> > > -	if (!runtime && !(csis->state & ST_SUSPENDED))
> > > -		goto unlock;
> > >
> > >  	if (!(csis->state & ST_POWERED)) {
> > >  		ret = mipi_csis_phy_enable(csis);
> > > @@ -1398,10 +1385,6 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> > >  		csis->state |= ST_POWERED;
> > >  		mipi_csis_clk_enable(csis);
> > >  	}
> > > -	if (csis->state & ST_STREAMING)
> > > -		mipi_csis_start_stream(csis);
> > > -
> > > -	csis->state &= ~ST_SUSPENDED;
> > >
> > >  unlock:
> > >  	mutex_unlock(&csis->lock);
> > > @@ -1409,30 +1392,9 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> > >  	return ret ? -EAGAIN : 0;
> > >  }
> > >
> > > -static int __maybe_unused mipi_csis_suspend(struct device *dev)
> > > -{
> > > -	return mipi_csis_pm_suspend(dev, false);
> > > -}
> > > -
> > > -static int __maybe_unused mipi_csis_resume(struct device *dev)
> > > -{
> > > -	return mipi_csis_pm_resume(dev, false);
> > > -}
> > > -
> > > -static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> > > -{
> > > -	return mipi_csis_pm_suspend(dev, true);
> > > -}
> > > -
> > > -static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> > > -{
> > > -	return mipi_csis_pm_resume(dev, true);
> > > -}
> > > -
> > >  static const struct dev_pm_ops mipi_csis_pm_ops = {
> > >  	SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> > >  			   NULL)
> > > -	SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> > >  };
> > >
> > >  /* -----------------------------------------------------------------------------
> > > @@ -1557,7 +1519,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> > >  	/* Enable runtime PM. */
> > >  	pm_runtime_enable(dev);
> > >  	if (!pm_runtime_enabled(dev)) {
> > > -		ret = mipi_csis_pm_resume(dev, true);
> > > +		ret = mipi_csis_runtime_resume(dev);
> > >  		if (ret < 0)
> > >  			goto unregister_all;
> > >  	}
> > > @@ -1592,7 +1554,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
> > >  	v4l2_async_unregister_subdev(&csis->sd);
> > >
> > >  	pm_runtime_disable(&pdev->dev);
> > > -	mipi_csis_pm_suspend(&pdev->dev, true);
> > > +	mipi_csis_runtime_suspend(&pdev->dev);
> > >  	mipi_csis_clk_disable(csis);
> > >  	media_entity_cleanup(&csis->sd.entity);
> > >  	mutex_destroy(&csis->lock);
>
> --
> Regards,
>
> Laurent Pinchart

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

[Jacopo Mondi]

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..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);
+
+		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);

 	return ret;
 }
--
2.35.1

[PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag

[Jacopo Mondi]

The mipi_csis_device.powered flag only serves for the purpose of
not accessing registers in mipi_csis_log_status() when the interface
is not powered up.

Instead of manually tracking the power state, rely on
pm_runtime_get_if_in_use() and remove the flag.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/imx/imx-mipi-csis.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index fa00b36fc394..505dd373b40c 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -327,10 +327,9 @@ struct mipi_csis_device {
 	u32 hs_settle;
 	u32 clk_settle;

-	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
+	struct mutex lock;	/* Protect csis_fmt and format_mbus */
 	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
-	bool powered;

 	spinlock_t slock;	/* Protect events */
 	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
@@ -1176,8 +1175,15 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)

 	mutex_lock(&csis->lock);
 	mipi_csis_log_counters(csis, true);
-	if (csis->debug.enable && csis->powered)
-		mipi_csis_dump_regs(csis);
+	if (!csis->debug.enable ||
+	    !pm_runtime_get_if_in_use(csis->dev))
+		goto unlock;
+
+	mipi_csis_dump_regs(csis);
+
+	pm_runtime_put(csis->dev);
+
+unlock:
 	mutex_unlock(&csis->lock);

 	return 0;
@@ -1344,8 +1350,6 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)

 	mipi_csis_clk_disable(csis);

-	csis->powered = false;
-
 unlock:
 	mutex_unlock(&csis->lock);

@@ -1366,8 +1370,6 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)

 	mipi_csis_clk_enable(csis);

-	csis->powered = true;
-
 unlock:
 	mutex_unlock(&csis->lock);

--
2.35.1

Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Jacopo Mondi]

Hi Laurent

On Fri, Mar 11, 2022 at 07:09:25PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Mar 11, 2022 at 06:00:45PM +0100, Jacopo Mondi wrote:
> > On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote:
> > > The runtime PM resume handler is guaranteed to be called on a suspended
> > > device, and the suspend handler on a resumed device. The implementation
> > > can thus be simplified.
> > >
> > > While at it, rename the mipi_csis_device state field to powered, as the
> > > now state contains a single flag only.
> >
> > Can we instead rely on pm_runtime_get_if_in_use() instead of manual
> > tracking the power state ?
> >
> > After all, the powered flag is only used in:
> >
> > static int mipi_csis_log_status(struct v4l2_subdev *sd)
> > {
> > 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> >
> > 	mutex_lock(&csis->lock);
> > 	mipi_csis_log_counters(csis, true);
> > 	if (csis->debug.enable && csis->powered)
> > 		mipi_csis_dump_regs(csis);
> > 	mutex_unlock(&csis->lock);
> >
> > 	return 0;
> > }
> >
> > which could be simplified as
> >
> > static int mipi_csis_log_status(struct v4l2_subdev *sd)
> > {
> > 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> >
> > 	mipi_csis_log_counters(csis, true);
> >
> > 	if (!csis->debug.enable)
> >                 return 0;
> >
> > 	mutex_lock(&csis->lock);
> >
> >         if (!pm_runtime_get_if_in_use())
> >                 goto unlock;
> >
> >         mipi_csis_dump_regs(csis);
> >
> >         pm_runtime_put();
> >
> > unlock:
> > 	mutex_unlock(&csis->lock);
> >
> > 	return 0;
> > }
>
> That's a good idea. Do you mind if I do so on a patch on top of this
> one, to not mix two separate changes ?
>

I sent two patches in reply to this series for you to collect on v2 if
desired.

Please add
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
to all patches in v2.

Thanks
   j

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
> > >  1 file changed, 17 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > index d656b8bfcc33..f6ff8d50843c 100644
> > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > @@ -248,10 +248,6 @@
> > >  #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
> > >  #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
> > >
> > > -enum {
> > > -	ST_POWERED	= 1,
> > > -};
> > > -
> > >  struct mipi_csis_event {
> > >  	bool debug;
> > >  	u32 mask;
> > > @@ -331,10 +327,10 @@ struct mipi_csis_device {
> > >  	u32 hs_settle;
> > >  	u32 clk_settle;
> > >
> > > -	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
> > > +	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
> > >  	const struct csis_pix_format *csis_fmt;
> > >  	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> > > -	u32 state;
> > > +	bool powered;
> > >
> > >  	spinlock_t slock;	/* Protect events */
> > >  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> > > @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> > >
> > >  	mutex_lock(&csis->lock);
> > >  	mipi_csis_log_counters(csis, true);
> > > -	if (csis->debug.enable && (csis->state & ST_POWERED))
> > > +	if (csis->debug.enable && csis->powered)
> > >  		mipi_csis_dump_regs(csis);
> > >  	mutex_unlock(&csis->lock);
> > >
> > > @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> > >  	int ret = 0;
> > >
> > >  	mutex_lock(&csis->lock);
> > > -	if (csis->state & ST_POWERED) {
> > > -		ret = mipi_csis_phy_disable(csis);
> > > -		if (ret)
> > > -			goto unlock;
> > > -		mipi_csis_clk_disable(csis);
> > > -		csis->state &= ~ST_POWERED;
> > > -	}
> > > +
> > > +	ret = mipi_csis_phy_disable(csis);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	mipi_csis_clk_disable(csis);
> > > +
> > > +	csis->powered = false;
> > >
> > >  unlock:
> > >  	mutex_unlock(&csis->lock);
> > > @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> > >
> > >  	mutex_lock(&csis->lock);
> > >
> > > -	if (!(csis->state & ST_POWERED)) {
> > > -		ret = mipi_csis_phy_enable(csis);
> > > -		if (ret)
> > > -			goto unlock;
> > > +	ret = mipi_csis_phy_enable(csis);
> > > +	if (ret)
> > > +		goto unlock;
> > >
> > > -		csis->state |= ST_POWERED;
> > > -		mipi_csis_clk_enable(csis);
> > > -	}
> > > +	mipi_csis_clk_enable(csis);
> > > +
> > > +	csis->powered = true;
> > >
> > >  unlock:
> > >  	mutex_unlock(&csis->lock);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Mar 12, 2022 at 04:25:05PM +0100, Jacopo Mondi wrote:
> The mipi_csis_device.powered flag only serves for the purpose of
> not accessing registers in mipi_csis_log_status() when the interface
> is not powered up.
> 
> Instead of manually tracking the power state, rely on
> pm_runtime_get_if_in_use() and remove the flag.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index fa00b36fc394..505dd373b40c 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -327,10 +327,9 @@ struct mipi_csis_device {
>  	u32 hs_settle;
>  	u32 clk_settle;
> 
> -	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
> +	struct mutex lock;	/* Protect csis_fmt and format_mbus */
>  	const struct csis_pix_format *csis_fmt;
>  	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> -	bool powered;
> 
>  	spinlock_t slock;	/* Protect events */
>  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> @@ -1176,8 +1175,15 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> 
>  	mutex_lock(&csis->lock);
>  	mipi_csis_log_counters(csis, true);
> -	if (csis->debug.enable && csis->powered)
> -		mipi_csis_dump_regs(csis);
> +	if (!csis->debug.enable ||
> +	    !pm_runtime_get_if_in_use(csis->dev))
> +		goto unlock;
> +
> +	mipi_csis_dump_regs(csis);
> +
> +	pm_runtime_put(csis->dev);
> +
> +unlock:

I like this change, but I think we can go one step further and drop the
lock here, as there's no powered field to access anymore:

	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);

	mipi_csis_log_counters(csis, true);

	if (csis->debug.enable && pm_runtime_get_if_in_use(csi->dev)) {
		mipi_csis_dump_regs(csis);
		pm_runtime_put(csis->dev);
	}

	return 0;

A third patch could then drop the lock from mipi_csis_runtime_suspend()
and mipi_csis_runtime_resume(). What do you think ?

>  	mutex_unlock(&csis->lock);
> 
>  	return 0;
> @@ -1344,8 +1350,6 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> 
>  	mipi_csis_clk_disable(csis);
> 
> -	csis->powered = false;
> -
>  unlock:
>  	mutex_unlock(&csis->lock);
> 
> @@ -1366,8 +1370,6 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> 
>  	mipi_csis_clk_enable(csis);
> 
> -	csis->powered = true;
> -
>  unlock:
>  	mutex_unlock(&csis->lock);
> 

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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@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..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

Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Sakari Ailus]

Hi Laurent,

On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote:
> The runtime PM resume handler is guaranteed to be called on a suspended
> device, and the suspend handler on a resumed device. The implementation
> can thus be simplified.
> 
> While at it, rename the mipi_csis_device state field to powered, as the
> now state contains a single flag only.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index d656b8bfcc33..f6ff8d50843c 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -248,10 +248,6 @@
>  #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
>  #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
>  
> -enum {
> -	ST_POWERED	= 1,
> -};
> -
>  struct mipi_csis_event {
>  	bool debug;
>  	u32 mask;
> @@ -331,10 +327,10 @@ struct mipi_csis_device {
>  	u32 hs_settle;
>  	u32 clk_settle;
>  
> -	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
> +	struct mutex lock;	/* Protect csis_fmt, format_mbus and powered */
>  	const struct csis_pix_format *csis_fmt;
>  	struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
> -	u32 state;
> +	bool powered;
>  
>  	spinlock_t slock;	/* Protect events */
>  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
>  
>  	mutex_lock(&csis->lock);
>  	mipi_csis_log_counters(csis, true);
> -	if (csis->debug.enable && (csis->state & ST_POWERED))
> +	if (csis->debug.enable && csis->powered)

Could you use instead pm_runtime_get_if_active() instead? You could then
drop the field.

>  		mipi_csis_dump_regs(csis);
>  	mutex_unlock(&csis->lock);
>  
> @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
>  	int ret = 0;
>  
>  	mutex_lock(&csis->lock);
> -	if (csis->state & ST_POWERED) {
> -		ret = mipi_csis_phy_disable(csis);
> -		if (ret)
> -			goto unlock;
> -		mipi_csis_clk_disable(csis);
> -		csis->state &= ~ST_POWERED;
> -	}
> +
> +	ret = mipi_csis_phy_disable(csis);
> +	if (ret)
> +		goto unlock;
> +
> +	mipi_csis_clk_disable(csis);
> +
> +	csis->powered = false;
>  
>  unlock:
>  	mutex_unlock(&csis->lock);
> @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
>  
>  	mutex_lock(&csis->lock);
>  
> -	if (!(csis->state & ST_POWERED)) {
> -		ret = mipi_csis_phy_enable(csis);
> -		if (ret)
> -			goto unlock;
> +	ret = mipi_csis_phy_enable(csis);
> +	if (ret)
> +		goto unlock;
>  
> -		csis->state |= ST_POWERED;
> -		mipi_csis_clk_enable(csis);
> -	}
> +	mipi_csis_clk_enable(csis);
> +
> +	csis->powered = true;
>  
>  unlock:
>  	mutex_unlock(&csis->lock);

-- 
Regards,

Sakari Ailus

Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation

[Sakari Ailus]

On Sun, Mar 13, 2022 at 12:18:39AM +0200, Sakari Ailus wrote:
> > @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >  
> >  	mutex_lock(&csis->lock);
> >  	mipi_csis_log_counters(csis, true);
> > -	if (csis->debug.enable && (csis->state & ST_POWERED))
> > +	if (csis->debug.enable && csis->powered)
> 
> Could you use instead pm_runtime_get_if_active() instead? You could then
> drop the field.

Ah, I see Jacopo already posted a patch to do that. Feel free to ignore.

-- 
Sakari Ailus