| Message ID | 20241203111314.2420473-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iio: adc: rzg2l_adc: Add support for RZ/G3S | expand |
On Tue, 3 Dec 2024 13:13:07 +0200 Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Enable runtime PM autosuspend support for the rzg2l_adc driver. With this > change, consecutive conversion requests will no longer cause the device to > be runtime-enabled/disabled after each request. Instead, the device will > transition based on the delay configured by the user. > > This approach reduces the frequency of hardware register access during > runtime PM suspend/resume cycles, thereby saving CPU cycles. The default > autosuspend delay is set to zero to maintain the previous driver behavior. Unless you have a weird user who is polling slow enough to not trigger autosuspend with a non zero period, but is still saving power I'm not convinced anyone will notice if you just enable this for a sensible autosuspend delay. There will of course be a small increase in power usage for each read but hopefully that is trivial. So I'd not go with a default of 0, though what value makes sense depends on the likely usecase + how much power is saved by going to sleep. If you really want to keep 0 I don't mind that much, just seems odd! Jonathan > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/iio/adc/rzg2l_adc.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > index eed2944bd98d..fda8b42ded81 100644 > --- a/drivers/iio/adc/rzg2l_adc.c > +++ b/drivers/iio/adc/rzg2l_adc.c > @@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc > rzg2l_adc_start_stop(adc, false); > > rpm_put: > - pm_runtime_put_sync(dev); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > return ret; > } > > @@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc) > rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); > > exit_hw_init: > - pm_runtime_put_sync(dev); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > return ret; > } > > @@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev) > return PTR_ERR(adc->presetn); > } > > + /* Default 0 for power saving. Can be overridden via sysfs. */ > + pm_runtime_set_autosuspend_delay(dev, 0); > + pm_runtime_use_autosuspend(dev); > ret = devm_pm_runtime_enable(dev); > if (ret) > return ret;
Hi, Jonathan, On 03.12.2024 22:00, Jonathan Cameron wrote: > On Tue, 3 Dec 2024 13:13:07 +0200 > Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Enable runtime PM autosuspend support for the rzg2l_adc driver. With this >> change, consecutive conversion requests will no longer cause the device to >> be runtime-enabled/disabled after each request. Instead, the device will >> transition based on the delay configured by the user. >> >> This approach reduces the frequency of hardware register access during >> runtime PM suspend/resume cycles, thereby saving CPU cycles. The default >> autosuspend delay is set to zero to maintain the previous driver behavior. > > Unless you have a weird user who is polling slow enough to not trigger > autosuspend with a non zero period, but is still saving power I'm not convinced > anyone will notice if you just enable this for a sensible autosuspend delay. > There will of course be a small increase in power usage for each read but > hopefully that is trivial. > > So I'd not go with a default of 0, though what value makes sense depends > on the likely usecase + how much power is saved by going to sleep. > > If you really want to keep 0 I don't mind that much, just seems odd! I agree with you. I chose it like this as I got internal request (on other drivers enabling autosuspend support) to keep the previous behavior in place. Thank you for your review, Claudiu > > Jonathan > >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/iio/adc/rzg2l_adc.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c >> index eed2944bd98d..fda8b42ded81 100644 >> --- a/drivers/iio/adc/rzg2l_adc.c >> +++ b/drivers/iio/adc/rzg2l_adc.c >> @@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc >> rzg2l_adc_start_stop(adc, false); >> >> rpm_put: >> - pm_runtime_put_sync(dev); >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> return ret; >> } >> >> @@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc) >> rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); >> >> exit_hw_init: >> - pm_runtime_put_sync(dev); >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> return ret; >> } >> >> @@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev) >> return PTR_ERR(adc->presetn); >> } >> >> + /* Default 0 for power saving. Can be overridden via sysfs. */ >> + pm_runtime_set_autosuspend_delay(dev, 0); >> + pm_runtime_use_autosuspend(dev); >> ret = devm_pm_runtime_enable(dev); >> if (ret) >> return ret; >
Hi All, > -----Original Message----- > From: Claudiu Beznea <claudiu.beznea@tuxon.dev> > Sent: 04 December 2024 08:32 > Subject: Re: [PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support > > Hi, Jonathan, > > On 03.12.2024 22:00, Jonathan Cameron wrote: > > On Tue, 3 Dec 2024 13:13:07 +0200 > > Claudiu <claudiu.beznea@tuxon.dev> wrote: > > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> Enable runtime PM autosuspend support for the rzg2l_adc driver. With > >> this change, consecutive conversion requests will no longer cause the > >> device to be runtime-enabled/disabled after each request. Instead, > >> the device will transition based on the delay configured by the user. > >> > >> This approach reduces the frequency of hardware register access > >> during runtime PM suspend/resume cycles, thereby saving CPU cycles. > >> The default autosuspend delay is set to zero to maintain the previous driver behavior. > > > > Unless you have a weird user who is polling slow enough to not trigger > > autosuspend with a non zero period, but is still saving power I'm not > > convinced anyone will notice if you just enable this for a sensible autosuspend delay. > > There will of course be a small increase in power usage for each read > > but hopefully that is trivial. > > > > So I'd not go with a default of 0, though what value makes sense > > depends on the likely usecase + how much power is saved by going to sleep. > > > > If you really want to keep 0 I don't mind that much, just seems odd! > > I agree with you. I chose it like this as I got internal request (on other drivers enabling > autosuspend support) to keep the previous behavior in place. On I2C driver after every transfer we turn off the clocks to save power(transaction based). If we introduce autosupend with 100msec, we are consuming power for 100msec. So, to keep previous behaviour, we are setting the value to 0, when auto suspend feature is introduced there. ADC case maybe different, you can have sensible delay between reading the temperature/voltage and next request as it uses polling. Cheers, Biju
diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c index eed2944bd98d..fda8b42ded81 100644 --- a/drivers/iio/adc/rzg2l_adc.c +++ b/drivers/iio/adc/rzg2l_adc.c @@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc rzg2l_adc_start_stop(adc, false); rpm_put: - pm_runtime_put_sync(dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); return ret; } @@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc) rzg2l_adc_writel(adc, RZG2L_ADM(3), reg); exit_hw_init: - pm_runtime_put_sync(dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); return ret; } @@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev) return PTR_ERR(adc->presetn); } + /* Default 0 for power saving. Can be overridden via sysfs. */ + pm_runtime_set_autosuspend_delay(dev, 0); + pm_runtime_use_autosuspend(dev); ret = devm_pm_runtime_enable(dev); if (ret) return ret;