Re: [PATCH 7/7] iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Fri, 7 Jan 2022 15:34:32 +0800
Baolin Wang <baolin.wang7@gmail.com> wrote:

> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > Ump9620 ADC suspend and resume pm optimization, configuration
> > 0x6490_ 0350(PAD_ CLK26M_ SINOUT_ PMIC_ 1P8 ) bit 8.
> >
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
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 <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> >
> >  /* 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
> >  
> 
>