Re: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment

[Jonathan Cameron]

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?

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;
>  }
>