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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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;
}
--
Best regards
George
