| Message ID | 20241203111314.2420473-4-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iio: adc: rzg2l_adc: Add support for RZ/G3S | expand |
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,
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, >
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 --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);