Re: [PATCH v4 23/45] media: sun6i-csi: Move power management to runtime pm in capture

Jernej Škrabec
2022-04-27T19:22:44Z

Dne petek, 15. april 2022 ob 17:27:49 CEST je Paul Kocialkowski napisal(a):
> Let's just enable the module when we start using it (at stream on)
> and benefit from runtime pm instead of enabling it at first open.
>
> Also reorder the call to v4l2_pipeline_pm_get.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
> .../platform/sunxi/sun6i-csi/sun6i_csi.c | 24 -----------
> .../platform/sunxi/sun6i-csi/sun6i_csi.h | 7 ----
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 41 ++++++++++---------
> 3 files changed, 22 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index
> 29892e8c2b9d..7801f5abe47e 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -148,30 +148,6 @@ bool sun6i_csi_is_format_supported(struct
> sun6i_csi_device *csi_dev, return false;
> }
>
> -int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable)
> -{
> - struct device *dev = csi_dev->dev;
> - struct regmap *regmap = csi_dev->regmap;
> - int ret;
> -
> - if (!enable) {
> - regmap_update_bits(regmap, SUN6I_CSI_EN_REG,
> - SUN6I_CSI_EN_CSI_EN, 0);
> - pm_runtime_put(dev);
> -
> - return 0;
> - }
> -
> - ret = pm_runtime_resume_and_get(dev);
> - if (ret < 0)
> - return ret;
> -
> - regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN,
> - SUN6I_CSI_EN_CSI_EN);
> -
> - return 0;
> -}
> -
> static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device
> *csi_dev, u32 mbus_code, u32 pixformat)
> {
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h index
> 155527961280..3a4f2b45d267 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> @@ -77,13 +77,6 @@ struct sun6i_csi_variant {
> bool sun6i_csi_is_format_supported(struct sun6i_csi_device *csi_dev,
> u32 pixformat, u32 mbus_code);
>
> -/**
> - * sun6i_csi_set_power() - power on/off the csi
> - * @csi: pointer to the csi
> - * @enable: on/off
> - */
> -int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable);
> -
> /**
> * sun6i_csi_update_config() - update the csi register settings
> * @csi: pointer to the csi
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c index
> 72fcc60d2695..3e5c4f7df48d 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regmap.h>
>
> #include <media/v4l2-device.h>
> @@ -141,6 +142,9 @@ static void sun6i_csi_capture_enable(struct
> sun6i_csi_device *csi_dev) {
> struct regmap *regmap = csi_dev->regmap;
>
> + regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN,
> + SUN6I_CSI_EN_CSI_EN);
> +
> regmap_update_bits(regmap, SUN6I_CSI_CAP_REG,
SUN6I_CSI_CAP_VCAP_ON,
> SUN6I_CSI_CAP_VCAP_ON);
> }
> @@ -150,6 +154,7 @@ static void sun6i_csi_capture_disable(struct
> sun6i_csi_device *csi_dev) struct regmap *regmap = csi_dev->regmap;
>
> regmap_update_bits(regmap, SUN6I_CSI_CAP_REG,
SUN6I_CSI_CAP_VCAP_ON, 0);
> + regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN,
0);
> }
>
> static void
> @@ -382,6 +387,7 @@ static int sun6i_csi_capture_start_streaming(struct
> vb2_queue *queue, struct sun6i_csi_capture *capture = &csi_dev->capture;
> struct sun6i_csi_capture_state *state = &capture->state;
> struct video_device *video_dev = &capture->video_dev;
> + struct device *dev = csi_dev->dev;
> struct v4l2_subdev *subdev;
> int ret;
>
> @@ -402,6 +408,12 @@ static int sun6i_csi_capture_start_streaming(struct
> vb2_queue *queue, goto error_media_pipeline;
> }
>
> + /* PM */
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0)
> + goto error_media_pipeline;
> +
> /* Clear */
>
> sun6i_csi_capture_irq_clear(csi_dev);
> @@ -429,6 +441,8 @@ static int sun6i_csi_capture_start_streaming(struct
> vb2_queue *queue, sun6i_csi_capture_disable(csi_dev);
> sun6i_csi_capture_irq_disable(csi_dev);
>
> + pm_runtime_put(dev);
> +
> error_media_pipeline:
> media_pipeline_stop(&video_dev->entity);
>
> @@ -442,6 +456,7 @@ static void sun6i_csi_capture_stop_streaming(struct
> vb2_queue *queue) {
> struct sun6i_csi_device *csi_dev = vb2_get_drv_priv(queue);
> struct sun6i_csi_capture *capture = &csi_dev->capture;
> + struct device *dev = csi_dev->dev;
> struct v4l2_subdev *subdev;
>
> subdev = sun6i_csi_capture_remote_subdev(capture, NULL);
> @@ -451,6 +466,8 @@ static void sun6i_csi_capture_stop_streaming(struct
> vb2_queue *queue) sun6i_csi_capture_disable(csi_dev);
> sun6i_csi_capture_irq_disable(csi_dev);
>
> + pm_runtime_put(dev);
> +
> media_pipeline_stop(&capture->video_dev.entity);
>
> sun6i_csi_capture_state_cleanup(csi_dev, true);
> @@ -639,27 +656,20 @@ static int sun6i_csi_capture_open(struct file *file)
> if (mutex_lock_interruptible(&capture->lock))
> return -ERESTARTSYS;
>
> - ret = v4l2_fh_open(file);
> + ret = v4l2_pipeline_pm_get(&capture->video_dev.entity);
> if (ret < 0)
> goto error_lock;
>
> - ret = v4l2_pipeline_pm_get(&capture->video_dev.entity);
> + ret = v4l2_fh_open(file);
> if (ret < 0)
> - goto error_v4l2_fh;
> -
> - /* Power on at first open. */
> - if (v4l2_fh_is_singular_file(file)) {
> - ret = sun6i_csi_set_power(csi_dev, true);
> - if (ret < 0)
> - goto error_v4l2_fh;
> - }
> + goto error_pipeline;
>
> mutex_unlock(&capture->lock);
>
> return 0;
>
> -error_v4l2_fh:
> - v4l2_fh_release(file);
> +error_pipeline:
> + v4l2_pipeline_pm_put(&capture->video_dev.entity);
>
> error_lock:
> mutex_unlock(&capture->lock);
> @@ -671,19 +681,12 @@ static int sun6i_csi_capture_close(struct file *file)
> {
> struct sun6i_csi_device *csi_dev = video_drvdata(file);
> struct sun6i_csi_capture *capture = &csi_dev->capture;
> - bool last_close;
>
> mutex_lock(&capture->lock);
>
> - last_close = v4l2_fh_is_singular_file(file);
> -
> _vb2_fop_release(file, NULL);
> v4l2_pipeline_pm_put(&capture->video_dev.entity);
>
> - /* Power off at last close. */
> - if (last_close)
> - sun6i_csi_set_power(csi_dev, false);
> -
> mutex_unlock(&capture->lock);
>
> return 0;