diff mbox series

[03/14] iio: adc: rzg2l_adc: Simplify the runtime PM code

Message ID 20241203111314.2420473-4-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Not Applicable, archived
Headers show
Series iio: adc: rzg2l_adc: Add support for RZ/G3S | expand

Commit Message

Claudiu Beznea Dec. 3, 2024, 11:13 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
the state of the clocks. As a result, the code in the rzg2l_adc driver that
explicitly manages ADC clocks can be removed, leading to simpler and
cleaner implementation.

Additionally, replace the use of rzg2l_adc_set_power() with direct PM
runtime API calls to further simplify and clean up the code.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
 1 file changed, 20 insertions(+), 80 deletions(-)

Comments

Paul Barker Dec. 3, 2024, 12:53 p.m. UTC | #1 
Hi Claudiu,

On 03/12/2024 11:13, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
> domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
> the state of the clocks. As a result, the code in the rzg2l_adc driver that
> explicitly manages ADC clocks can be removed, leading to simpler and
> cleaner implementation.
> 
> Additionally, replace the use of rzg2l_adc_set_power() with direct PM
> runtime API calls to further simplify and clean up the code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
>  1 file changed, 20 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 7039949a7554..a17690ecbdc3 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -8,7 +8,6 @@
>   */
>  
>  #include <linux/bitfield.h>
> -#include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/iio/iio.h>
> @@ -69,8 +68,6 @@ struct rzg2l_adc_data {
>  
>  struct rzg2l_adc {
>  	void __iomem *base;
> -	struct clk *pclk;
> -	struct clk *adclk;
>  	struct reset_control *presetn;
>  	struct reset_control *adrstn;
>  	struct completion completion;
> @@ -188,29 +185,18 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>  	return 0;
>  }
>  
> -static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
> -{
> -	struct device *dev = indio_dev->dev.parent;
> -
> -	if (on)
> -		return pm_runtime_resume_and_get(dev);
> -
> -	return pm_runtime_put_sync(dev);
> -}
> -
>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>  {
> +	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = rzg2l_adc_set_power(indio_dev, true);
> +	ret = pm_runtime_resume_and_get(dev);
>  	if (ret)
>  		return ret;

Should we check (ret < 0) here instead of just (ret)? According to the
docs [1], pm_runtime_resume_and_get() can return 1 if the device is
already active.

[1]: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions

Thanks,
Claudiu Beznea Dec. 3, 2024, 1:40 p.m. UTC | #2 
Hi, Paul,

On 03.12.2024 14:53, Paul Barker wrote:
> Hi Claudiu,
> 
> On 03/12/2024 11:13, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
>> domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
>> the state of the clocks. As a result, the code in the rzg2l_adc driver that
>> explicitly manages ADC clocks can be removed, leading to simpler and
>> cleaner implementation.
>>
>> Additionally, replace the use of rzg2l_adc_set_power() with direct PM
>> runtime API calls to further simplify and clean up the code.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
>>  1 file changed, 20 insertions(+), 80 deletions(-)
>>
>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>> index 7039949a7554..a17690ecbdc3 100644
>> --- a/drivers/iio/adc/rzg2l_adc.c
>> +++ b/drivers/iio/adc/rzg2l_adc.c
>> @@ -8,7 +8,6 @@
>>   */
>>  
>>  #include <linux/bitfield.h>
>> -#include <linux/clk.h>
>>  #include <linux/completion.h>
>>  #include <linux/delay.h>
>>  #include <linux/iio/iio.h>
>> @@ -69,8 +68,6 @@ struct rzg2l_adc_data {
>>  
>>  struct rzg2l_adc {
>>  	void __iomem *base;
>> -	struct clk *pclk;
>> -	struct clk *adclk;
>>  	struct reset_control *presetn;
>>  	struct reset_control *adrstn;
>>  	struct completion completion;
>> @@ -188,29 +185,18 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>>  	return 0;
>>  }
>>  
>> -static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
>> -{
>> -	struct device *dev = indio_dev->dev.parent;
>> -
>> -	if (on)
>> -		return pm_runtime_resume_and_get(dev);
>> -
>> -	return pm_runtime_put_sync(dev);
>> -}
>> -
>>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>>  {
>> +	struct device *dev = indio_dev->dev.parent;
>>  	int ret;
>>  
>> -	ret = rzg2l_adc_set_power(indio_dev, true);
>> +	ret = pm_runtime_resume_and_get(dev);
>>  	if (ret)
>>  		return ret;
> 
> Should we check (ret < 0) here instead of just (ret)? According to the
> docs [1], pm_runtime_resume_and_get() can return 1 if the device is
> already active.

The v6.13-rc1 implementation of pm_runtime_resume_and_get() is:

static inline int pm_runtime_resume_and_get(struct device *dev)
{
	int ret;

	ret = __pm_runtime_resume(dev, RPM_GET_PUT);
	if (ret < 0) {
		pm_runtime_put_noidle(dev);
		return ret;
	}

	return 0;
}

It can return zero or negative error number.

Thank you,
Claudiu


> 
> [1]: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions
> 
> Thanks,
>
Paul Barker Dec. 3, 2024, 2:20 p.m. UTC | #3 
Hi Claudiu,

On 03/12/2024 13:40, Claudiu Beznea wrote:
> On 03.12.2024 14:53, Paul Barker wrote:
>> On 03/12/2024 11:13, Claudiu wrote:
>>>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>>>  {
>>> +	struct device *dev = indio_dev->dev.parent;
>>>  	int ret;
>>>  
>>> -	ret = rzg2l_adc_set_power(indio_dev, true);
>>> +	ret = pm_runtime_resume_and_get(dev);
>>>  	if (ret)
>>>  		return ret;
>>
>> Should we check (ret < 0) here instead of just (ret)? According to the
>> docs [1], pm_runtime_resume_and_get() can return 1 if the device is
>> already active.
> 
> The v6.13-rc1 implementation of pm_runtime_resume_and_get() is:
> 
> static inline int pm_runtime_resume_and_get(struct device *dev)
> {
> 	int ret;
> 
> 	ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> 	if (ret < 0) {
> 		pm_runtime_put_noidle(dev);
> 		return ret;
> 	}
> 
> 	return 0;
> }
> 
> It can return zero or negative error number.

Ah, ok. The docs say that pm_runtime_resume_and_get() will "return the
result of pm_runtime_resume" (which can return 1), but it doesn't do
that exactly. I'll send a fix for the docs.

Thanks,
diff mbox series

Patch

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 7039949a7554..a17690ecbdc3 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -8,7 +8,6 @@ 
  */
 
 #include <linux/bitfield.h>
-#include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
@@ -69,8 +68,6 @@  struct rzg2l_adc_data {
 
 struct rzg2l_adc {
 	void __iomem *base;
-	struct clk *pclk;
-	struct clk *adclk;
 	struct reset_control *presetn;
 	struct reset_control *adrstn;
 	struct completion completion;
@@ -188,29 +185,18 @@  static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 	return 0;
 }
 
-static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
-{
-	struct device *dev = indio_dev->dev.parent;
-
-	if (on)
-		return pm_runtime_resume_and_get(dev);
-
-	return pm_runtime_put_sync(dev);
-}
-
 static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
 {
+	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
-	ret = rzg2l_adc_set_power(indio_dev, true);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;
 
 	ret = rzg2l_adc_conversion_setup(adc, ch);
-	if (ret) {
-		rzg2l_adc_set_power(indio_dev, false);
-		return ret;
-	}
+	if (ret)
+		goto rpm_put;
 
 	reinit_completion(&adc->completion);
 
@@ -219,12 +205,14 @@  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
 	if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
 		rzg2l_adc_writel(adc, RZG2L_ADINT,
 				 rzg2l_adc_readl(adc, RZG2L_ADINT) & ~RZG2L_ADINT_INTEN_MASK);
-		rzg2l_adc_start_stop(adc, false);
-		rzg2l_adc_set_power(indio_dev, false);
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
 	}
 
-	return rzg2l_adc_set_power(indio_dev, false);
+	rzg2l_adc_start_stop(adc, false);
+
+rpm_put:
+	pm_runtime_put_sync(dev);
+	return ret;
 }
 
 static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
@@ -352,13 +340,13 @@  static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 	return 0;
 }
 
-static int rzg2l_adc_hw_init(struct rzg2l_adc *adc)
+static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 {
 	int timeout = 5;
 	u32 reg;
 	int ret;
 
-	ret = clk_prepare_enable(adc->pclk);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;
 
@@ -396,25 +384,10 @@  static int rzg2l_adc_hw_init(struct rzg2l_adc *adc)
 	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
 
 exit_hw_init:
-	clk_disable_unprepare(adc->pclk);
-
+	pm_runtime_put_sync(dev);
 	return ret;
 }
 
-static void rzg2l_adc_pm_runtime_disable(void *data)
-{
-	struct device *dev = data;
-
-	pm_runtime_disable(dev->parent);
-}
-
-static void rzg2l_adc_pm_runtime_set_suspended(void *data)
-{
-	struct device *dev = data;
-
-	pm_runtime_set_suspended(dev->parent);
-}
-
 static int rzg2l_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -439,18 +412,6 @@  static int rzg2l_adc_probe(struct platform_device *pdev)
 	if (IS_ERR(adc->base))
 		return PTR_ERR(adc->base);
 
-	adc->pclk = devm_clk_get(dev, "pclk");
-	if (IS_ERR(adc->pclk)) {
-		dev_err(dev, "Failed to get pclk");
-		return PTR_ERR(adc->pclk);
-	}
-
-	adc->adclk = devm_clk_get(dev, "adclk");
-	if (IS_ERR(adc->adclk)) {
-		dev_err(dev, "Failed to get adclk");
-		return PTR_ERR(adc->adclk);
-	}
-
 	adc->adrstn = devm_reset_control_get_exclusive_deasserted(dev, "adrst-n");
 	if (IS_ERR(adc->adrstn)) {
 		dev_err(dev, "failed to get adrstn\n");
@@ -463,7 +424,13 @@  static int rzg2l_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(adc->presetn);
 	}
 
-	ret = rzg2l_adc_hw_init(adc);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = rzg2l_adc_hw_init(dev, adc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret);
 		return ret;
@@ -480,26 +447,12 @@  static int rzg2l_adc_probe(struct platform_device *pdev)
 
 	init_completion(&adc->completion);
 
-	platform_set_drvdata(pdev, indio_dev);
-
 	indio_dev->name = DRIVER_NAME;
 	indio_dev->info = &rzg2l_adc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adc->data->channels;
 	indio_dev->num_channels = adc->data->num_channels;
 
-	pm_runtime_set_suspended(dev);
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_pm_runtime_set_suspended, &indio_dev->dev);
-	if (ret)
-		return ret;
-
-	pm_runtime_enable(dev);
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_pm_runtime_disable, &indio_dev->dev);
-	if (ret)
-		return ret;
-
 	return devm_iio_device_register(dev, indio_dev);
 }
 
@@ -515,8 +468,6 @@  static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
 
 	rzg2l_adc_pwr(adc, false);
-	clk_disable_unprepare(adc->adclk);
-	clk_disable_unprepare(adc->pclk);
 
 	return 0;
 }
@@ -525,17 +476,6 @@  static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
-	int ret;
-
-	ret = clk_prepare_enable(adc->pclk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(adc->adclk);
-	if (ret) {
-		clk_disable_unprepare(adc->pclk);
-		return ret;
-	}
 
 	rzg2l_adc_pwr(adc, true);