[PATCH 0/5] media: imx: imx-mipi-csis: Additional cleanups

[PATCH 0/5] media: imx: imx-mipi-csis: Additional cleanups

[Jacopo Mondi]

As a followup of the two patches I sent in respose to Laurent's
[PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support
I have piled a few more cleanups for the CSIS.

Jacopo Mondi (5):
  media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream()
  media: imx: imx-mipi-csis: Drop powered flag
  media: imx: imx-mipi-csis: Remove lock from s_stream
  media: imx: imx-mipi-csis: Remove duplicated check
  media: imx: imx-mipi-csis: Protect mipi_csis_dump_regs()

 drivers/media/platform/imx/imx-mipi-csis.c | 64 ++++++++++------------
 1 file changed, 28 insertions(+), 36 deletions(-)

--
2.35.1

[PATCH 1/5] 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..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:
+	mipi_csis_stop_stream(csis);
+	mutex_unlock(&csis->lock);
+	pm_runtime_put(csis->dev);
 
 	return ret;
 }
-- 
2.35.1

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

[Laurent Pinchart]

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

[PATCH 2/5] 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() to remove the 'powered' flag. Also remove
the locking in the function as runtime_pm() is refcounted and there's no
risk of the interface being powered down behind our backs.

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

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 60e7f0452582..b76bb129a416 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];
@@ -1174,11 +1173,11 @@ 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)
+	if (csis->debug.enable && pm_runtime_get_if_in_use(csis->dev)) {
 		mipi_csis_dump_regs(csis);
-	mutex_unlock(&csis->lock);
+		pm_runtime_put(csis->dev);
+	}
 
 	return 0;
 }
@@ -1344,8 +1343,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 +1363,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 2/5] media: imx: imx-mipi-csis: Drop powered flag

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 11:39:38AM +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() to remove the 'powered' flag. Also remove
> the locking in the function as runtime_pm() is refcounted and there's no
> risk of the interface being powered down behind our backs.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 60e7f0452582..b76bb129a416 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];
> @@ -1174,11 +1173,11 @@ 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)
> +	if (csis->debug.enable && pm_runtime_get_if_in_use(csis->dev)) {
>  		mipi_csis_dump_regs(csis);
> -	mutex_unlock(&csis->lock);
> +		pm_runtime_put(csis->dev);

I was going to write that you could move runtime PM to
mipi_csis_dump_regs(), and then noticed patch 5/5 that does so. Is there
any reason it can't be done here already (essentially squashing this
with 5/5) ?

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

> +	}
>  
>  	return 0;
>  }
> @@ -1344,8 +1343,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 +1363,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

[PATCH 3/5] media: imx: imx-mipi-csis: Remove lock from s_stream

[Jacopo Mondi]

The s_stream() operation implementation was locked to avoid races with
the mipi_csis_log_status() which access the platform registers and needs
the interface to be powered up.

As powering is now handled by runtime_pm which is refcounted, it is
not necessary to manually lock s_stream() anymore.

As the error path is now simplified, we can inline it.

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

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index b76bb129a416..4a6152c13d52 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -912,16 +912,12 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (!enable) {
-		mutex_lock(&csis->lock);
-
 		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
 
 		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;
@@ -937,25 +933,18 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&csis->lock);
-
 	mipi_csis_start_stream(csis);
 	ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
-	if (ret < 0)
-		goto out;
+	if (ret < 0) {
+		mipi_csis_stop_stream(csis);
+		pm_runtime_put(csis->dev);
 
-	mipi_csis_log_counters(csis, true);
+		return ret;
+	}
 
-	mutex_unlock(&csis->lock);
+	mipi_csis_log_counters(csis, true);
 
 	return 0;
-
-out:
-	mipi_csis_stop_stream(csis);
-	mutex_unlock(&csis->lock);
-	pm_runtime_put(csis->dev);
-
-	return ret;
 }
 
 static struct v4l2_mbus_framefmt *
-- 
2.35.1

Re: [PATCH 3/5] media: imx: imx-mipi-csis: Remove lock from s_stream

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 11:39:39AM +0100, Jacopo Mondi wrote:
> The s_stream() operation implementation was locked to avoid races with
> the mipi_csis_log_status() which access the platform registers and needs
> the interface to be powered up.
> 
> As powering is now handled by runtime_pm which is refcounted, it is
> not necessary to manually lock s_stream() anymore.

The lock is also used to protect against races between .s_stream() and
.set_fmt() both accessing the active format. I don't think we can drop
it.

> As the error path is now simplified, we can inline it.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 23 ++++++----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index b76bb129a416..4a6152c13d52 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -912,16 +912,12 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>  
>  	if (!enable) {
> -		mutex_lock(&csis->lock);
> -
>  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
>  
>  		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;
> @@ -937,25 +933,18 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (ret < 0)
>  		return ret;
>  
> -	mutex_lock(&csis->lock);
> -
>  	mipi_csis_start_stream(csis);
>  	ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
> -	if (ret < 0)
> -		goto out;
> +	if (ret < 0) {
> +		mipi_csis_stop_stream(csis);
> +		pm_runtime_put(csis->dev);
>  
> -	mipi_csis_log_counters(csis, true);
> +		return ret;
> +	}
>  
> -	mutex_unlock(&csis->lock);
> +	mipi_csis_log_counters(csis, true);
>  
>  	return 0;
> -
> -out:
> -	mipi_csis_stop_stream(csis);
> -	mutex_unlock(&csis->lock);
> -	pm_runtime_put(csis->dev);
> -
> -	return ret;
>  }
>  
>  static struct v4l2_mbus_framefmt *

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] media: imx: imx-mipi-csis: Remove duplicated check

[Jacopo Mondi]

The mipi_csis_log_counters() function already checks for
csis->debug.enable, it is not necessary to do the same in the caller.

Compatc the code in the caller as well by removing an empty line.

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

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 4a6152c13d52..4bb469fcb6b3 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -913,11 +913,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 
 	if (!enable) {
 		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
-
 		mipi_csis_stop_stream(csis);
-		if (csis->debug.enable)
-			mipi_csis_log_counters(csis, true);
-
+		mipi_csis_log_counters(csis, true);
 		pm_runtime_put(csis->dev);
 
 		return 0;
-- 
2.35.1

Re: [PATCH 4/5] media: imx: imx-mipi-csis: Remove duplicated check

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 11:39:40AM +0100, Jacopo Mondi wrote:
> The mipi_csis_log_counters() function already checks for
> csis->debug.enable, it is not necessary to do the same in the caller.

Does it ? It does only to decide whether or not to print counters that
have a zero value.

> Compatc the code in the caller as well by removing an empty line.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 4a6152c13d52..4bb469fcb6b3 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -913,11 +913,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	if (!enable) {
>  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> -
>  		mipi_csis_stop_stream(csis);
> -		if (csis->debug.enable)
> -			mipi_csis_log_counters(csis, true);
> -
> +		mipi_csis_log_counters(csis, true);
>  		pm_runtime_put(csis->dev);
>  
>  		return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] media: imx: imx-mipi-csis: Remove duplicated check

[Jacopo Mondi]

On Tue, Mar 15, 2022 at 01:30:20PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Mar 14, 2022 at 11:39:40AM +0100, Jacopo Mondi wrote:
> > The mipi_csis_log_counters() function already checks for
> > csis->debug.enable, it is not necessary to do the same in the caller.
>
> Does it ? It does only to decide whether or not to print counters that
> have a zero value.
>

Roght, I mis-read the condition there.

What is the usage of the counters logger ? Should we make it
conditional to debug.enable ?

Thanks
  j

> > Compatc the code in the caller as well by removing an empty line.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/platform/imx/imx-mipi-csis.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index 4a6152c13d52..4bb469fcb6b3 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -913,11 +913,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> >
> >  	if (!enable) {
> >  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> > -
> >  		mipi_csis_stop_stream(csis);
> > -		if (csis->debug.enable)
> > -			mipi_csis_log_counters(csis, true);
> > -
> > +		mipi_csis_log_counters(csis, true);
> >  		pm_runtime_put(csis->dev);
> >
> >  		return 0;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 4/5] media: imx: imx-mipi-csis: Remove duplicated check

[Laurent Pinchart]

Hi Jacopo,

On Tue, Mar 15, 2022 at 01:27:48PM +0100, Jacopo Mondi wrote:
> On Tue, Mar 15, 2022 at 01:30:20PM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 14, 2022 at 11:39:40AM +0100, Jacopo Mondi wrote:
> > > The mipi_csis_log_counters() function already checks for
> > > csis->debug.enable, it is not necessary to do the same in the caller.
> >
> > Does it ? It does only to decide whether or not to print counters that
> > have a zero value.
> 
> Roght, I mis-read the condition there.
> 
> What is the usage of the counters logger ? Should we make it
> conditional to debug.enable ?

It's been there from the very beginning, so I'm not sure what the
expected use cases where. I'd be tempted to actually move it to debugfs,
possibly with a single warning message at stream start if any error is
detected.

Rui, what do you think ?

> > > Compatc the code in the caller as well by removing an empty line.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/platform/imx/imx-mipi-csis.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > index 4a6152c13d52..4bb469fcb6b3 100644
> > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > @@ -913,11 +913,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > >
> > >  	if (!enable) {
> > >  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> > > -
> > >  		mipi_csis_stop_stream(csis);
> > > -		if (csis->debug.enable)
> > > -			mipi_csis_log_counters(csis, true);
> > > -
> > > +		mipi_csis_log_counters(csis, true);
> > >  		pm_runtime_put(csis->dev);
> > >
> > >  		return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] media: imx: imx-mipi-csis: Remove duplicated check

[Rui Miguel Silva]

Hi Laurent,
On Tue Mar 15, 2022 at 1:03 PM WET, Laurent Pinchart wrote:

> Hi Jacopo,
>
> On Tue, Mar 15, 2022 at 01:27:48PM +0100, Jacopo Mondi wrote:
> > On Tue, Mar 15, 2022 at 01:30:20PM +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 14, 2022 at 11:39:40AM +0100, Jacopo Mondi wrote:
> > > > The mipi_csis_log_counters() function already checks for
> > > > csis->debug.enable, it is not necessary to do the same in the caller.
> > >
> > > Does it ? It does only to decide whether or not to print counters that
> > > have a zero value.
> > 
> > Roght, I mis-read the condition there.
> > 
> > What is the usage of the counters logger ? Should we make it
> > conditional to debug.enable ?
>
> It's been there from the very beginning, so I'm not sure what the
> expected use cases where. I'd be tempted to actually move it to debugfs,
> possibly with a single warning message at stream start if any error is
> detected.
>
> Rui, what do you think ?

This were there to trace events when debug is enable or irq_src_errors
where thrown in the irq_handler and report them at start stream.

But, it looks good to move it to debugfs. Thanks for your and Jacopo
work on this to improve it.

Cheers,
     Rui
>
> > > > Compatc the code in the caller as well by removing an empty line.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  drivers/media/platform/imx/imx-mipi-csis.c | 5 +----
> > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > > > index 4a6152c13d52..4bb469fcb6b3 100644
> > > > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > > > @@ -913,11 +913,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > >
> > > >  	if (!enable) {
> > > >  		v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> > > > -
> > > >  		mipi_csis_stop_stream(csis);
> > > > -		if (csis->debug.enable)
> > > > -			mipi_csis_log_counters(csis, true);
> > > > -
> > > > +		mipi_csis_log_counters(csis, true);
> > > >  		pm_runtime_put(csis->dev);
> > > >
> > > >  		return 0;
>
> -- 
> Regards,
>
> Laurent Pinchart

[PATCH 5/5] media: imx: imx-mipi-csis: Protect mipi_csis_dump_regs()

[Jacopo Mondi]

The mipi_csis_dump_regs() function accesses the interface registers
in order to printout their values for debug purposes.

As the function access the registers, it requires the interface to be
powered up. Currently this is only enforced in one of the function's
callers (mipi_csis_log_status)() but not when the function is called by
the debugfs attribute handler.

Make sure to access registers only if the interface is powered up and
remove the same check from the caller.

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

diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
index 4bb469fcb6b3..3437455b9c82 100644
--- a/drivers/media/platform/imx/imx-mipi-csis.c
+++ b/drivers/media/platform/imx/imx-mipi-csis.c
@@ -857,6 +857,9 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
 	unsigned int i;
 	u32 cfg;
 
+	if (!pm_runtime_get_if_in_use(csis->dev))
+		return 0;
+
 	dev_info(csis->dev, "--- REGISTERS ---\n");
 
 	for (i = 0; i < ARRAY_SIZE(registers); i++) {
@@ -864,6 +867,8 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
 		dev_info(csis->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
 	}
 
+	pm_runtime_put(csis->dev);
+
 	return 0;
 }
 
@@ -1160,10 +1165,8 @@ 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 && pm_runtime_get_if_in_use(csis->dev)) {
+	if (csis->debug.enable)
 		mipi_csis_dump_regs(csis);
-		pm_runtime_put(csis->dev);
-	}
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 5/5] media: imx: imx-mipi-csis: Protect mipi_csis_dump_regs()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 11:39:41AM +0100, Jacopo Mondi wrote:
> The mipi_csis_dump_regs() function accesses the interface registers
> in order to printout their values for debug purposes.
> 
> As the function access the registers, it requires the interface to be
> powered up. Currently this is only enforced in one of the function's
> callers (mipi_csis_log_status)() but not when the function is called by

s/)()/())/

> the debugfs attribute handler.
> 
> Make sure to access registers only if the interface is powered up and
> remove the same check from the caller.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

But I think we can squash this with 2/5.

> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index 4bb469fcb6b3..3437455b9c82 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -857,6 +857,9 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
>  	unsigned int i;
>  	u32 cfg;
>  
> +	if (!pm_runtime_get_if_in_use(csis->dev))
> +		return 0;
> +
>  	dev_info(csis->dev, "--- REGISTERS ---\n");
>  
>  	for (i = 0; i < ARRAY_SIZE(registers); i++) {
> @@ -864,6 +867,8 @@ static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
>  		dev_info(csis->dev, "%14s: 0x%08x\n", registers[i].name, cfg);
>  	}
>  
> +	pm_runtime_put(csis->dev);
> +
>  	return 0;
>  }
>  
> @@ -1160,10 +1165,8 @@ 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 && pm_runtime_get_if_in_use(csis->dev)) {
> +	if (csis->debug.enable)
>  		mipi_csis_dump_regs(csis);
> -		pm_runtime_put(csis->dev);
> -	}
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] media: imx: imx-mipi-csis: Additional cleanups

[Rui Miguel Silva]

Hi Jacopo,
Many thanks for the series on top of Laurent simplification,

On Mon Mar 14, 2022 at 10:39 AM WET, Jacopo Mondi wrote:

> As a followup of the two patches I sent in respose to Laurent's
> [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support
> I have piled a few more cleanups for the CSIS.
>
> Jacopo Mondi (5):
>   media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream()
>   media: imx: imx-mipi-csis: Drop powered flag
>   media: imx: imx-mipi-csis: Remove lock from s_stream
>   media: imx: imx-mipi-csis: Remove duplicated check
>   media: imx: imx-mipi-csis: Protect mipi_csis_dump_regs()

all look good to me:

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Cheers,
     Rui
>
>  drivers/media/platform/imx/imx-mipi-csis.c | 64 ++++++++++------------
>  1 file changed, 28 insertions(+), 36 deletions(-)
>
> --
> 2.35.1