* [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power()
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
@ 2022-03-11 13:55 ` Laurent Pinchart
2022-03-11 16:34 ` Jacopo Mondi
2022-03-11 13:55 ` [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation Laurent Pinchart
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 13:55 UTC (permalink / raw)
To: linux-media
Cc: Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power()
2022-03-11 13:55 ` [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power() Laurent Pinchart
@ 2022-03-11 16:34 ` Jacopo Mondi
2022-03-11 17:05 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-11 16:34 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power()
2022-03-11 16:34 ` Jacopo Mondi
@ 2022-03-11 17:05 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 17:05 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
2022-03-11 13:55 ` [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power() Laurent Pinchart
@ 2022-03-11 13:55 ` Laurent Pinchart
2022-03-11 16:53 ` Jacopo Mondi
2022-03-11 13:55 ` [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time Laurent Pinchart
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 13:55 UTC (permalink / raw)
To: linux-media
Cc: Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation
2022-03-11 13:55 ` [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation Laurent Pinchart
@ 2022-03-11 16:53 ` Jacopo Mondi
2022-03-11 17:07 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-11 16:53 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation
2022-03-11 16:53 ` Jacopo Mondi
@ 2022-03-11 17:07 ` Laurent Pinchart
2022-03-11 17:19 ` Jacopo Mondi
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 17:07 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation
2022-03-11 17:07 ` Laurent Pinchart
@ 2022-03-11 17:19 ` Jacopo Mondi
0 siblings, 0 replies; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-11 17:19 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
2022-03-11 13:55 ` [PATCH 1/4] media: imx: imx-mipi-csis: Don't use .s_power() Laurent Pinchart
2022-03-11 13:55 ` [PATCH 2/4] media: imx: imx-mipi-csis: Drop unneeded system PM implementation Laurent Pinchart
@ 2022-03-11 13:55 ` Laurent Pinchart
2022-03-11 16:55 ` Jacopo Mondi
2022-03-11 13:55 ` [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation Laurent Pinchart
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 13:55 UTC (permalink / raw)
To: linux-media
Cc: Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time
2022-03-11 13:55 ` [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time Laurent Pinchart
@ 2022-03-11 16:55 ` Jacopo Mondi
0 siblings, 0 replies; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-11 16:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
` (2 preceding siblings ...)
2022-03-11 13:55 ` [PATCH 3/4] media: imx: imx-mipi-csis: Don't stop streaming at runtime suspend time Laurent Pinchart
@ 2022-03-11 13:55 ` Laurent Pinchart
2022-03-11 17:00 ` Jacopo Mondi
2022-03-12 22:18 ` Sakari Ailus
2022-03-11 14:00 ` [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
` (2 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 13:55 UTC (permalink / raw)
To: linux-media
Cc: Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-11 13:55 ` [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation Laurent Pinchart
@ 2022-03-11 17:00 ` Jacopo Mondi
2022-03-11 17:09 ` Laurent Pinchart
2022-03-12 22:18 ` Sakari Ailus
1 sibling, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-11 17:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
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.
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-11 17:00 ` Jacopo Mondi
@ 2022-03-11 17:09 ` Laurent Pinchart
2022-03-12 15:30 ` Jacopo Mondi
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 17:09 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-11 17:09 ` Laurent Pinchart
@ 2022-03-12 15:30 ` Jacopo Mondi
0 siblings, 0 replies; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-12 15:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-11 13:55 ` [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation Laurent Pinchart
2022-03-11 17:00 ` Jacopo Mondi
@ 2022-03-12 22:18 ` Sakari Ailus
2022-03-12 22:20 ` Sakari Ailus
1 sibling, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2022-03-12 22:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi,
Paul Elder
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation
2022-03-12 22:18 ` Sakari Ailus
@ 2022-03-12 22:20 ` Sakari Ailus
0 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2022-03-12 22:20 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi,
Paul Elder
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
` (3 preceding siblings ...)
2022-03-11 13:55 ` [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation Laurent Pinchart
@ 2022-03-11 14:00 ` Laurent Pinchart
2022-03-12 15:25 ` [PATCH 1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream() Jacopo Mondi
2022-03-12 15:25 ` [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag Jacopo Mondi
6 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-11 14:00 UTC (permalink / raw)
To: linux-media
Cc: Rui Miguel Silva, kernel, linux-imx, Jacopo Mondi, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream()
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
` (4 preceding siblings ...)
2022-03-11 14:00 ` [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
@ 2022-03-12 15:25 ` Jacopo Mondi
2022-03-12 20:17 ` Laurent Pinchart
2022-03-12 15:25 ` [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag Jacopo Mondi
6 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-12 15:25 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Jacopo Mondi, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream()
2022-03-12 15:25 ` [PATCH 1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream() Jacopo Mondi
@ 2022-03-12 20:17 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-12 20:17 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag
2022-03-11 13:55 [PATCH 0/4] media: imx: imx-mipi-csis: Simplify PM support Laurent Pinchart
` (5 preceding siblings ...)
2022-03-12 15:25 ` [PATCH 1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream() Jacopo Mondi
@ 2022-03-12 15:25 ` Jacopo Mondi
2022-03-12 18:50 ` Laurent Pinchart
6 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2022-03-12 15:25 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Jacopo Mondi, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag
2022-03-12 15:25 ` [PATCH 2/2] media: imx: imx-mipi-csis: Drop powered flag Jacopo Mondi
@ 2022-03-12 18:50 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2022-03-12 18:50 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Rui Miguel Silva, kernel, linux-imx, Paul Elder,
Sakari Ailus
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
^ permalink raw reply [flat|nested] 21+ messages in thread