Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 02361C433F5 for ; Sun, 9 Jan 2022 16:16:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236006AbiAIQQf (ORCPT ); Sun, 9 Jan 2022 11:16:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46818 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231653AbiAIQQe (ORCPT ); Sun, 9 Jan 2022 11:16:34 -0500 Received: from haggis.mythic-beasts.com (haggis.mythic-beasts.com [IPv6:2a00:1098:0:86:1000:0:2:1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 991FAC06173F; Sun, 9 Jan 2022 08:16:33 -0800 (PST) Received: from [81.101.6.87] (port=47172 helo=jic23-huawei) by haggis.mythic-beasts.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92.3) (envelope-from ) id 1n6arq-0002zs-O1; Sun, 09 Jan 2022 16:16:27 +0000 Date: Sun, 9 Jan 2022 16:22:15 +0000 From: Jonathan Cameron To: Baolin Wang Cc: Cixi Geng , Orson Zhai , Chunyan Zhang , Lars-Peter Clausen , Rob Herring , lgirdwood@gmail.com, Mark Brown , yuming.zhu1@unisoc.com, linux-iio@vger.kernel.org, Devicetree List , LKML Subject: Re: [PATCH 7/7] iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support Message-ID: <20220109162215.631b228b@jic23-huawei> In-Reply-To: References: <20220106125947.139523-1-gengcixi@gmail.com> <20220106125947.139523-8-gengcixi@gmail.com> X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BlackCat-Spam-Score: 19 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 7 Jan 2022 15:34:32 +0800 Baolin Wang wrote: > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng wrote: > > > > From: Cixi Geng > > > > Ump9620 ADC suspend and resume pm optimization, configuration > > 0x6490_ 0350(PAD_ CLK26M_ SINOUT_ PMIC_ 1P8 ) bit 8. > > > > Signed-off-by: Cixi Geng > > Signed-off-by: Yuming Zhu A few additional comments from me inline, Thanks, Jonathan > > --- > > drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 102 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > index 68b967f32498..cecda8d53474 100644 > > --- a/drivers/iio/adc/sc27xx_adc.c > > +++ b/drivers/iio/adc/sc27xx_adc.c > > @@ -11,6 +11,7 @@ > > #include > > #include > > #include > > +#include > > > > /* PMIC global registers definition */ > > #define SC2730_MODULE_EN 0x1808 > > @@ -83,6 +84,9 @@ > > /* ADC default channel reference voltage is 2.8V */ > > #define SC27XX_ADC_REFVOL_VDD28 2800000 > > > > +/* 10s delay before suspending the ADC IP */ > > +#define SC27XX_ADC_AUTOSUSPEND_DELAY 10000 > > + > > enum sc27xx_pmic_type { > > SC27XX_ADC, > > SC2721_ADC, > > @@ -618,6 +622,9 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > > return ret; > > } > > > > + if (data->var_data->pmic_type == UMP9620_ADC) > > + pm_runtime_get_sync(data->indio_dev->dev.parent); > > + > > /* > > * According to the sc2721 chip data sheet, the reference voltage of > > * specific channel 30 and channel 31 in ADC module needs to be set from > > @@ -700,6 +707,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > > } > > } > > > > + if (data->var_data->pmic_type == UMP9620_ADC) { > > + pm_runtime_mark_last_busy(data->indio_dev->dev.parent); > > + pm_runtime_put_autosuspend(data->indio_dev->dev.parent); > > + } > > + > > hwspin_unlock_raw(data->hwlock); > > > > if (!ret) > > @@ -947,6 +959,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > > ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0, > > UMP9620_XTL_WAIT_CTRL0_EN, > > UMP9620_XTL_WAIT_CTRL0_EN); > > + if (ret) { > > + dev_err(data->dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n"); > > + goto clean_adc_clk26m_bit8; > > + } > > } > > > > /* Enable ADC work clock */ > > @@ -988,6 +1004,11 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > > regmap_update_bits(data->regmap, data->var_data->module_en, > > SC27XX_MODULE_ADC_EN, 0); > > > > +clean_adc_clk26m_bit8: > > + if (data->var_data->pmic_type == UMP9620_ADC) > > + regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0, > > + UMP9620_XTL_WAIT_CTRL0_EN, 0); > > Can you hide this into the pm runtime callbacks? > > > + > > return ret; > > } > > > > @@ -1086,6 +1107,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > > if (!indio_dev) > > return -ENOMEM; > > > > + platform_set_drvdata(pdev, indio_dev); > > + > > sc27xx_data = iio_priv(indio_dev); > > > > sc27xx_data->regmap = dev_get_regmap(dev->parent, NULL); > > @@ -1126,7 +1149,10 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > > } > > } > > > > + sc27xx_data->dev = dev; > > sc27xx_data->var_data = pdata; > > + sc27xx_data->indio_dev = indio_dev; > > + > > sc27xx_data->var_data->init_scale(sc27xx_data); > > > > ret = sc27xx_adc_enable(sc27xx_data); > > @@ -1137,18 +1163,39 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > > > > ret = devm_add_action_or_reset(dev, sc27xx_adc_disable, sc27xx_data); > > if (ret) { > > + sc27xx_adc_disable(sc27xx_data); No. That's what the _or_reset() bit of the above call is about. It will have already called this if the devm registration failed. > > dev_err(dev, "failed to add ADC disable action\n"); > > return ret; > > } > > > > + indio_dev->dev.parent = dev; > > indio_dev->name = dev_name(dev); > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->info = &sc27xx_info; > > indio_dev->channels = sc27xx_channels; > > indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels); > > + > > + if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) { > > + pm_runtime_set_autosuspend_delay(dev, > > + SC27XX_ADC_AUTOSUSPEND_DELAY); > > + pm_runtime_use_autosuspend(dev); > > + pm_runtime_set_suspended(dev); > > + pm_runtime_enable(dev); > > + } > > + > > ret = devm_iio_device_register(dev, indio_dev); > > - if (ret) > > + if (ret) { > > dev_err(dev, "could not register iio (ADC)"); > > + goto err_iio_register; > > + } > > + > > + return 0; > > + > > +err_iio_register: > > + if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) { > > + pm_runtime_put(dev); > > I don't think the pm_runtime_put() is needed, since you did not get > the counter before, right? Please try to avoid mixing up devm_ managed cleanup and manual cleanup. devm_add_action_or_reset() can be used to ensure the pm_runtime_disable occurs on error and in remove function. > > > + pm_runtime_disable(dev); > > + } > > > > return ret; > > } > > @@ -1163,11 +1210,65 @@ static const struct of_device_id sc27xx_adc_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match); > > > > +static int sc27xx_adc_remove(struct platform_device *pdev) > > +{ > > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > > + struct sc27xx_adc_data *sc27xx_data = iio_priv(indio_dev); > > + > > + if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) { > > + pm_runtime_put(&pdev->dev); > > You did not get the pm count, why put it firstly? > > > + pm_runtime_disable(&pdev->dev); > > + > > + /* set the UMP9620 ADC clk26m bit8 on IP */ > > + regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0, > > + UMP9620_XTL_WAIT_CTRL0_EN, 0); Why is this not called in error path of the probe() function? I suspect because it also doesn't need to be called here as you have it automatically called in the sc27xx_adc_disable() call during device managed cleanup. > > + } > > + > > + return 0; > > +} > > + > > +static int sc27xx_adc_runtime_suspend(struct device *dev) > > +{ > > + struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev)); > > + > > + /* clean the UMP9620 ADC clk26m bit8 on IP */ > > + if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) > > + regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0, > > + UMP9620_XTL_WAIT_CTRL0_EN, 0); > > + > > + return 0; > > +} > > + > > +static int sc27xx_adc_runtime_resume(struct device *dev) > > +{ > > + int ret = 0; > > no need to initialize it. > > > + struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev)); > > + > > + /* set the UMP9620 ADC clk26m bit8 on IP */ > > + if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) { > > + ret = regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0, > > + UMP9620_XTL_WAIT_CTRL0_EN, UMP9620_XTL_WAIT_CTRL0_EN); > > + if (ret) { > > + dev_err(dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n"); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops sc27xx_adc_pm_ops = { > > + .runtime_suspend = &sc27xx_adc_runtime_suspend, > > + .runtime_resume = &sc27xx_adc_runtime_resume, > > +}; > > Please use SET_RUNTIME_PM_OPS macro. > > > + > > static struct platform_driver sc27xx_adc_driver = { > > .probe = sc27xx_adc_probe, > > + .remove = sc27xx_adc_remove, > > .driver = { > > .name = "sc27xx-adc", > > .of_match_table = sc27xx_adc_of_match, > > + .pm = &sc27xx_adc_pm_ops, > > }, > > }; > > > > -- > > 2.25.1 > > > >