| Message ID | 20230714190320.308320-2-gnstark@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: adc: meson: fix core clock enable/disable moment | expand |
On Fri, 14 Jul 2023 22:03:15 +0300 George Stark <gnstark@sberdevices.ru> wrote: > Enable core clock at probe stage and disable it at remove stage. > Core clock is responsible for turning on/off the entire SoC module so > it should be on before the first module register is touched and be off > at very last moment. > > Signed-off-by: George Stark <gnstark@sberdevices.ru> This sounds to me like this should perhaps have a fixes tag? Given where it is in the new sequence you can also use devm_clk_get_enabled() though that makes a potential backport of the fix trickier... I'd go with it anyway as it will make this change quite a bit simpler. More comments inline Jonathan > --- > drivers/iio/adc/meson_saradc.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > index fe066c9fab83..5a5bb5cc1320 100644 > --- a/drivers/iio/adc/meson_saradc.c > +++ b/drivers/iio/adc/meson_saradc.c > @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) > goto err_vref; > } > > - ret = clk_prepare_enable(priv->core_clk); > - if (ret) { > - dev_err(dev, "failed to enable core clk\n"); > - goto err_core_clk; > - } > - > regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); > regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, > MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); > @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) > regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, > MESON_SAR_ADC_REG3_ADC_EN, 0); > meson_sar_adc_set_bandgap(indio_dev, false); > - clk_disable_unprepare(priv->core_clk); > -err_core_clk: > regulator_disable(priv->vref); > err_vref: > meson_sar_adc_unlock(indio_dev); > @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) > > meson_sar_adc_set_bandgap(indio_dev, false); > > - clk_disable_unprepare(priv->core_clk); > - > regulator_disable(priv->vref); > > if (!ret) > @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) > ARRAY_SIZE(meson_sar_adc_iio_channels); > } > > + ret = clk_prepare_enable(priv->core_clk); > + if (ret) { > + dev_err(dev, "failed to enable core clk\n"); > + goto err; If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare() > + } > + > ret = meson_sar_adc_init(indio_dev); > if (ret) > goto err; > @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) > err_hw: > meson_sar_adc_hw_disable(indio_dev); > err: > + clk_disable_unprepare(priv->core_clk); Nothing to do with your patch, but this driver previously did some odd mixing of direct returns and going to an empty label which definitely doesn't help make this clear to follow. > return ret; > } > > static int meson_sar_adc_remove(struct platform_device *pdev) > { > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > > meson_sar_adc_hw_disable(indio_dev); > > + clk_disable_unprepare(priv->core_clk); > + > return 0; > } >
Hello Jonathan Thanks for the feedback. On 7/15/23 20:21, Jonathan Cameron wrote: > On Fri, 14 Jul 2023 22:03:15 +0300 > George Stark <gnstark@sberdevices.ru> wrote: > >> Enable core clock at probe stage and disable it at remove stage. >> Core clock is responsible for turning on/off the entire SoC module so >> it should be on before the first module register is touched and be off >> at very last moment. >> >> Signed-off-by: George Stark <gnstark@sberdevices.ru> > > This sounds to me like this should perhaps have a fixes tag? ack > > Given where it is in the new sequence you can also use > devm_clk_get_enabled() though that makes a potential backport of > the fix trickier... > > I'd go with it anyway as it will make this change quite a bit simpler. ack > > More comments inline > > Jonathan > >> --- >> drivers/iio/adc/meson_saradc.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c >> index fe066c9fab83..5a5bb5cc1320 100644 >> --- a/drivers/iio/adc/meson_saradc.c >> +++ b/drivers/iio/adc/meson_saradc.c >> @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) >> goto err_vref; >> } >> >> - ret = clk_prepare_enable(priv->core_clk); >> - if (ret) { >> - dev_err(dev, "failed to enable core clk\n"); >> - goto err_core_clk; >> - } >> - >> regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); >> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, >> MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); >> @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) >> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, >> MESON_SAR_ADC_REG3_ADC_EN, 0); >> meson_sar_adc_set_bandgap(indio_dev, false); >> - clk_disable_unprepare(priv->core_clk); >> -err_core_clk: >> regulator_disable(priv->vref); >> err_vref: >> meson_sar_adc_unlock(indio_dev); >> @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) >> >> meson_sar_adc_set_bandgap(indio_dev, false); >> >> - clk_disable_unprepare(priv->core_clk); >> - >> regulator_disable(priv->vref); >> >> if (!ret) >> @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) >> ARRAY_SIZE(meson_sar_adc_iio_channels); >> } >> >> + ret = clk_prepare_enable(priv->core_clk); >> + if (ret) { >> + dev_err(dev, "failed to enable core clk\n"); >> + goto err; > > If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare() yes, but with devm_clk_get_enabled we dont't need this line anymore > >> + } >> + >> ret = meson_sar_adc_init(indio_dev); >> if (ret) >> goto err; >> @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) >> err_hw: >> meson_sar_adc_hw_disable(indio_dev); >> err: >> + clk_disable_unprepare(priv->core_clk); > > Nothing to do with your patch, but this driver previously did some odd mixing > of direct returns and going to an empty label which definitely doesn't help > make this clear to follow. I made additional patch adding log messages to all errors those can break probe stage > >> return ret; >> } >> >> static int meson_sar_adc_remove(struct platform_device *pdev) >> { >> struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); >> >> iio_device_unregister(indio_dev); >> >> meson_sar_adc_hw_disable(indio_dev); >> >> + clk_disable_unprepare(priv->core_clk); >> + >> return 0; >> } >> >
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c index fe066c9fab83..5a5bb5cc1320 100644 --- a/drivers/iio/adc/meson_saradc.c +++ b/drivers/iio/adc/meson_saradc.c @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) goto err_vref; } - ret = clk_prepare_enable(priv->core_clk); - if (ret) { - dev_err(dev, "failed to enable core clk\n"); - goto err_core_clk; - } - regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, MESON_SAR_ADC_REG3_ADC_EN, 0); meson_sar_adc_set_bandgap(indio_dev, false); - clk_disable_unprepare(priv->core_clk); -err_core_clk: regulator_disable(priv->vref); err_vref: meson_sar_adc_unlock(indio_dev); @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) meson_sar_adc_set_bandgap(indio_dev, false); - clk_disable_unprepare(priv->core_clk); - regulator_disable(priv->vref); if (!ret) @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) ARRAY_SIZE(meson_sar_adc_iio_channels); } + ret = clk_prepare_enable(priv->core_clk); + if (ret) { + dev_err(dev, "failed to enable core clk\n"); + goto err; + } + ret = meson_sar_adc_init(indio_dev); if (ret) goto err; @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) err_hw: meson_sar_adc_hw_disable(indio_dev); err: + clk_disable_unprepare(priv->core_clk); return ret; } static int meson_sar_adc_remove(struct platform_device *pdev) { struct iio_dev *indio_dev = platform_get_drvdata(pdev); + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); iio_device_unregister(indio_dev); meson_sar_adc_hw_disable(indio_dev); + clk_disable_unprepare(priv->core_clk); + return 0; }
Enable core clock at probe stage and disable it at remove stage. Core clock is responsible for turning on/off the entire SoC module so it should be on before the first module register is touched and be off at very last moment. Signed-off-by: George Stark <gnstark@sberdevices.ru> --- drivers/iio/adc/meson_saradc.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)