diff mbox series

[v3,07/46] media: sun6i-csi: Use runtime pm for clocks and reset

Message ID 20220311143532.265091-8-paul.kocialkowski@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Allwinner A31/A83T MIPI CSI-2 and A31 ISP / CSI Rework | expand

Commit Message

Paul Kocialkowski March 11, 2022, 2:34 p.m. UTC 
Wrap the clock and reset preparation into runtime pm functions
for better organization of the code. Also fix the clock and
reset enable order to first deassert reset, as recommended in
Allwinner litterature.

Make the driver depend on PM while at it since runtime pm is
mandatory for the driver to work.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../media/platform/sunxi/sun6i-csi/Kconfig    |  2 +-
 .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 94 ++++++++++++++-----
 2 files changed, 70 insertions(+), 26 deletions(-)

Comments

Jernej Škrabec March 15, 2022, 7:46 p.m. UTC | #1 
Dne petek, 11. marec 2022 ob 15:34:53 CET je Paul Kocialkowski napisal(a):
> Wrap the clock and reset preparation into runtime pm functions
> for better organization of the code. Also fix the clock and
> reset enable order to first deassert reset, as recommended in
> Allwinner litterature.
> 
> Make the driver depend on PM while at it since runtime pm is
> mandatory for the driver to work.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../media/platform/sunxi/sun6i-csi/Kconfig    |  2 +-
>  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 94 ++++++++++++++-----
>  2 files changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/Kconfig b/drivers/media/
platform/sunxi/sun6i-csi/Kconfig
> index 586e3fb3a80d..fd03e48f0c8a 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> +++ b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config VIDEO_SUN6I_CSI
>  	tristate "Allwinner V3s Camera Sensor Interface driver"
> -	depends on VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
> +	depends on PM && VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
>  	depends on ARCH_SUNXI || COMPILE_TEST
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/
media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index 4a0d08e0ac25..7f045a9551a7 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -152,40 +152,18 @@ int sun6i_csi_set_power(struct sun6i_csi_device 
*csi_dev, bool enable)
>  
>  	if (!enable) {
>  		regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
0);
> +		pm_runtime_put(dev);
>  
> -		clk_disable_unprepare(csi_dev->clk_ram);
> -		clk_disable_unprepare(csi_dev->clk_mod);
> -		reset_control_assert(csi_dev->reset);
>  		return 0;
>  	}
>  
> -	ret = clk_prepare_enable(csi_dev->clk_mod);
> -	if (ret) {
> -		dev_err(csi_dev->dev, "Enable csi clk err %d\n", ret);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(csi_dev->clk_ram);
> -	if (ret) {
> -		dev_err(csi_dev->dev, "Enable clk_dram_csi clk err 
%d\n", ret);
> -		goto clk_mod_disable;
> -	}
> -
> -	ret = reset_control_deassert(csi_dev->reset);
> -	if (ret) {
> -		dev_err(csi_dev->dev, "reset err %d\n", ret);
> -		goto clk_ram_disable;
> -	}
>  
>  	regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
CSI_EN_CSI_EN);
>  
>  	return 0;
> -
> -clk_ram_disable:
> -	clk_disable_unprepare(csi_dev->clk_ram);
> -clk_mod_disable:
> -	clk_disable_unprepare(csi_dev->clk_mod);
> -	return ret;
>  }
>  
>  static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device 
*csi_dev,
> @@ -800,6 +778,65 @@ static irqreturn_t sun6i_csi_isr(int irq, void 
*private)
>  	return IRQ_HANDLED;
>  }
>  
> +static int sun6i_csi_suspend(struct device *dev)
> +{
> +	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> +
> +	reset_control_assert(csi_dev->reset);
> +	clk_disable_unprepare(csi_dev->clk_ram);
> +	clk_disable_unprepare(csi_dev->clk_mod);
> +	clk_disable_unprepare(csi_dev->clk_bus);
> +
> +	return 0;
> +}
> +
> +static int sun6i_csi_resume(struct device *dev)
> +{
> +	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = reset_control_deassert(csi_dev->reset);
> +	if (ret) {
> +		dev_err(dev, "failed to deassert reset\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(csi_dev->clk_bus);
> +	if (ret) {
> +		dev_err(dev, "failed to enable bus clock\n");
> +		goto error_reset;
> +	}
> +
> +	ret = clk_prepare_enable(csi_dev->clk_mod);
> +	if (ret) {
> +		dev_err(dev, "failed to enable module clock\n");
> +		goto error_clk_bus;
> +	}
> +
> +	ret = clk_prepare_enable(csi_dev->clk_ram);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ram clock\n");
> +		goto error_clk_mod;
> +	}
> +
> +	return 0;
> +
> +error_clk_mod:
> +	clk_disable_unprepare(csi_dev->clk_mod);
> +
> +error_clk_bus:
> +	clk_disable_unprepare(csi_dev->clk_bus);
> +
> +error_reset:
> +	reset_control_assert(csi_dev->reset);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops sun6i_csi_pm_ops = {
> +	SET_RUNTIME_PM_OPS(sun6i_csi_suspend, sun6i_csi_resume, NULL)

Since driver depends on PM, using SET_RUNTIME_PM_OPS macro doesn't make sense.

> +};
> +
>  static const struct regmap_config sun6i_csi_regmap_config = {
>  	.reg_bits       = 32,
>  	.reg_stride     = 4,
> @@ -886,6 +923,11 @@ static int sun6i_csi_resources_setup(struct 
sun6i_csi_device *csi_dev,
>  		goto error_clk_rate_exclusive;
>  	}
>  
> +	/* Runtime PM */
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_suspended(dev);

pm_runtime_set_suspended() description says: "It is not valid to call this 
function for devices with runtime PM enabled."

Best regards,
Jernej

> +
>  	return 0;
>  
>  error_clk_rate_exclusive:
> @@ -896,6 +938,7 @@ static int sun6i_csi_resources_setup(struct 
sun6i_csi_device *csi_dev,
>  
>  static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev)
>  {
> +	pm_runtime_disable(csi_dev->dev);
>  	clk_rate_exclusive_put(csi_dev->clk_mod);
>  }
>  
> @@ -978,6 +1021,7 @@ static struct platform_driver sun6i_csi_platform_driver 
= {
>  	.driver	= {
>  		.name		= SUN6I_CSI_NAME,
>  		.of_match_table	= 
of_match_ptr(sun6i_csi_of_match),
> +		.pm		= &sun6i_csi_pm_ops,
>  	},
>  };
>  
> -- 
> 2.35.1
> 
>
Paul Kocialkowski March 18, 2022, 4:15 p.m. UTC | #2 
Hi Jernej,

On Tue 15 Mar 22, 20:46, Jernej Škrabec wrote:
> Dne petek, 11. marec 2022 ob 15:34:53 CET je Paul Kocialkowski napisal(a):
> > Wrap the clock and reset preparation into runtime pm functions
> > for better organization of the code. Also fix the clock and
> > reset enable order to first deassert reset, as recommended in
> > Allwinner litterature.
> > 
> > Make the driver depend on PM while at it since runtime pm is
> > mandatory for the driver to work.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  .../media/platform/sunxi/sun6i-csi/Kconfig    |  2 +-
> >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 94 ++++++++++++++-----
> >  2 files changed, 70 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/Kconfig b/drivers/media/
> platform/sunxi/sun6i-csi/Kconfig
> > index 586e3fb3a80d..fd03e48f0c8a 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  config VIDEO_SUN6I_CSI
> >  	tristate "Allwinner V3s Camera Sensor Interface driver"
> > -	depends on VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
> > +	depends on PM && VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
> >  	depends on ARCH_SUNXI || COMPILE_TEST
> >  	select MEDIA_CONTROLLER
> >  	select VIDEO_V4L2_SUBDEV_API
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/
> media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index 4a0d08e0ac25..7f045a9551a7 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -152,40 +152,18 @@ int sun6i_csi_set_power(struct sun6i_csi_device 
> *csi_dev, bool enable)
> >  
> >  	if (!enable) {
> >  		regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
> 0);
> > +		pm_runtime_put(dev);
> >  
> > -		clk_disable_unprepare(csi_dev->clk_ram);
> > -		clk_disable_unprepare(csi_dev->clk_mod);
> > -		reset_control_assert(csi_dev->reset);
> >  		return 0;
> >  	}
> >  
> > -	ret = clk_prepare_enable(csi_dev->clk_mod);
> > -	if (ret) {
> > -		dev_err(csi_dev->dev, "Enable csi clk err %d\n", ret);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> > -
> > -	ret = clk_prepare_enable(csi_dev->clk_ram);
> > -	if (ret) {
> > -		dev_err(csi_dev->dev, "Enable clk_dram_csi clk err 
> %d\n", ret);
> > -		goto clk_mod_disable;
> > -	}
> > -
> > -	ret = reset_control_deassert(csi_dev->reset);
> > -	if (ret) {
> > -		dev_err(csi_dev->dev, "reset err %d\n", ret);
> > -		goto clk_ram_disable;
> > -	}
> >  
> >  	regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
> CSI_EN_CSI_EN);
> >  
> >  	return 0;
> > -
> > -clk_ram_disable:
> > -	clk_disable_unprepare(csi_dev->clk_ram);
> > -clk_mod_disable:
> > -	clk_disable_unprepare(csi_dev->clk_mod);
> > -	return ret;
> >  }
> >  
> >  static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device 
> *csi_dev,
> > @@ -800,6 +778,65 @@ static irqreturn_t sun6i_csi_isr(int irq, void 
> *private)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static int sun6i_csi_suspend(struct device *dev)
> > +{
> > +	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> > +
> > +	reset_control_assert(csi_dev->reset);
> > +	clk_disable_unprepare(csi_dev->clk_ram);
> > +	clk_disable_unprepare(csi_dev->clk_mod);
> > +	clk_disable_unprepare(csi_dev->clk_bus);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_csi_resume(struct device *dev)
> > +{
> > +	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = reset_control_deassert(csi_dev->reset);
> > +	if (ret) {
> > +		dev_err(dev, "failed to deassert reset\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(csi_dev->clk_bus);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable bus clock\n");
> > +		goto error_reset;
> > +	}
> > +
> > +	ret = clk_prepare_enable(csi_dev->clk_mod);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable module clock\n");
> > +		goto error_clk_bus;
> > +	}
> > +
> > +	ret = clk_prepare_enable(csi_dev->clk_ram);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable ram clock\n");
> > +		goto error_clk_mod;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_clk_mod:
> > +	clk_disable_unprepare(csi_dev->clk_mod);
> > +
> > +error_clk_bus:
> > +	clk_disable_unprepare(csi_dev->clk_bus);
> > +
> > +error_reset:
> > +	reset_control_assert(csi_dev->reset);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct dev_pm_ops sun6i_csi_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(sun6i_csi_suspend, sun6i_csi_resume, NULL)
> 
> Since driver depends on PM, using SET_RUNTIME_PM_OPS macro doesn't make sense.

Can you ellaborate why? It's really not obvious why the macro should only be
used when PM is optional. I see that it facilitates that, but I really don't
see any issue using it with PM required too. Is it a common practice?

> > +};
> > +
> >  static const struct regmap_config sun6i_csi_regmap_config = {
> >  	.reg_bits       = 32,
> >  	.reg_stride     = 4,
> > @@ -886,6 +923,11 @@ static int sun6i_csi_resources_setup(struct 
> sun6i_csi_device *csi_dev,
> >  		goto error_clk_rate_exclusive;
> >  	}
> >  
> > +	/* Runtime PM */
> > +
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_set_suspended(dev);
> 
> pm_runtime_set_suspended() description says: "It is not valid to call this 
> function for devices with runtime PM enabled."

Okay I need to revisit this, I think I misunderstood a bunch of things here.

Thanks,

Paul

> Best regards,
> Jernej
> 
> > +
> >  	return 0;
> >  
> >  error_clk_rate_exclusive:
> > @@ -896,6 +938,7 @@ static int sun6i_csi_resources_setup(struct 
> sun6i_csi_device *csi_dev,
> >  
> >  static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev)
> >  {
> > +	pm_runtime_disable(csi_dev->dev);
> >  	clk_rate_exclusive_put(csi_dev->clk_mod);
> >  }
> >  
> > @@ -978,6 +1021,7 @@ static struct platform_driver sun6i_csi_platform_driver 
> = {
> >  	.driver	= {
> >  		.name		= SUN6I_CSI_NAME,
> >  		.of_match_table	= 
> of_match_ptr(sun6i_csi_of_match),
> > +		.pm		= &sun6i_csi_pm_ops,
> >  	},
> >  };
> >  
> > -- 
> > 2.35.1
> > 
> > 
> 
>
diff mbox series

Patch

diff --git a/drivers/media/platform/sunxi/sun6i-csi/Kconfig b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
index 586e3fb3a80d..fd03e48f0c8a 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/Kconfig
+++ b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
@@ -1,7 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 config VIDEO_SUN6I_CSI
 	tristate "Allwinner V3s Camera Sensor Interface driver"
-	depends on VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
+	depends on PM && VIDEO_V4L2 && COMMON_CLK  && HAS_DMA
 	depends on ARCH_SUNXI || COMPILE_TEST
 	select MEDIA_CONTROLLER
 	select VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
index 4a0d08e0ac25..7f045a9551a7 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
@@ -152,40 +152,18 @@  int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable)
 
 	if (!enable) {
 		regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0);
+		pm_runtime_put(dev);
 
-		clk_disable_unprepare(csi_dev->clk_ram);
-		clk_disable_unprepare(csi_dev->clk_mod);
-		reset_control_assert(csi_dev->reset);
 		return 0;
 	}
 
-	ret = clk_prepare_enable(csi_dev->clk_mod);
-	if (ret) {
-		dev_err(csi_dev->dev, "Enable csi clk err %d\n", ret);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
-	}
-
-	ret = clk_prepare_enable(csi_dev->clk_ram);
-	if (ret) {
-		dev_err(csi_dev->dev, "Enable clk_dram_csi clk err %d\n", ret);
-		goto clk_mod_disable;
-	}
-
-	ret = reset_control_deassert(csi_dev->reset);
-	if (ret) {
-		dev_err(csi_dev->dev, "reset err %d\n", ret);
-		goto clk_ram_disable;
-	}
 
 	regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, CSI_EN_CSI_EN);
 
 	return 0;
-
-clk_ram_disable:
-	clk_disable_unprepare(csi_dev->clk_ram);
-clk_mod_disable:
-	clk_disable_unprepare(csi_dev->clk_mod);
-	return ret;
 }
 
 static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device *csi_dev,
@@ -800,6 +778,65 @@  static irqreturn_t sun6i_csi_isr(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static int sun6i_csi_suspend(struct device *dev)
+{
+	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
+
+	reset_control_assert(csi_dev->reset);
+	clk_disable_unprepare(csi_dev->clk_ram);
+	clk_disable_unprepare(csi_dev->clk_mod);
+	clk_disable_unprepare(csi_dev->clk_bus);
+
+	return 0;
+}
+
+static int sun6i_csi_resume(struct device *dev)
+{
+	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
+	int ret;
+
+	ret = reset_control_deassert(csi_dev->reset);
+	if (ret) {
+		dev_err(dev, "failed to deassert reset\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(csi_dev->clk_bus);
+	if (ret) {
+		dev_err(dev, "failed to enable bus clock\n");
+		goto error_reset;
+	}
+
+	ret = clk_prepare_enable(csi_dev->clk_mod);
+	if (ret) {
+		dev_err(dev, "failed to enable module clock\n");
+		goto error_clk_bus;
+	}
+
+	ret = clk_prepare_enable(csi_dev->clk_ram);
+	if (ret) {
+		dev_err(dev, "failed to enable ram clock\n");
+		goto error_clk_mod;
+	}
+
+	return 0;
+
+error_clk_mod:
+	clk_disable_unprepare(csi_dev->clk_mod);
+
+error_clk_bus:
+	clk_disable_unprepare(csi_dev->clk_bus);
+
+error_reset:
+	reset_control_assert(csi_dev->reset);
+
+	return ret;
+}
+
+static const struct dev_pm_ops sun6i_csi_pm_ops = {
+	SET_RUNTIME_PM_OPS(sun6i_csi_suspend, sun6i_csi_resume, NULL)
+};
+
 static const struct regmap_config sun6i_csi_regmap_config = {
 	.reg_bits       = 32,
 	.reg_stride     = 4,
@@ -886,6 +923,11 @@  static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev,
 		goto error_clk_rate_exclusive;
 	}
 
+	/* Runtime PM */
+
+	pm_runtime_enable(dev);
+	pm_runtime_set_suspended(dev);
+
 	return 0;
 
 error_clk_rate_exclusive:
@@ -896,6 +938,7 @@  static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev,
 
 static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev)
 {
+	pm_runtime_disable(csi_dev->dev);
 	clk_rate_exclusive_put(csi_dev->clk_mod);
 }
 
@@ -978,6 +1021,7 @@  static struct platform_driver sun6i_csi_platform_driver = {
 	.driver	= {
 		.name		= SUN6I_CSI_NAME,
 		.of_match_table	= of_match_ptr(sun6i_csi_of_match),
+		.pm		= &sun6i_csi_pm_ops,
 	},
 };