linux-kernel - Re: [PATCH 1/4] iio: adc: ti-adc12138: Simplify with devm_clk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250724135450.36f9a65f@jic23-huawei>
Date: Thu, 24 Jul 2025 13:54:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] iio: adc: ti-adc12138: Simplify with
 devm_clk_get_enabled()

On Sun, 13 Jul 2025 17:59:55 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> wrote:

> Driver is getting clock and almost immediately enabling it, with the
> devm_request_irq() as the only relevant code executed between, thus the
> probe path and cleanups can be simplified with devm_clk_get_enabled().
> 
> Move devm_request_irq() earlier, so the interrupt handler will be
> registered before clock is enabled.  This might be important in case
> regulator supplies are enabled by other device driver and this device
> raises interrupt immediately after clock sarts ticking.
> 
> The change does not reverse cleanup paths - first regulator will be
> disabled, then clock and finally interrupt handler freed.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>

Seems no one cares (or is paying attention) about the ordering change
and we both think it is fine so applied to the testing branch of iio.git
for 6.18. I'll rebase on rc1 once available

Thanks,

Jonathan

> 
> ---
> 
> Not tested on hardware.
> ---
>  drivers/iio/adc/ti-adc12138.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc12138.c b/drivers/iio/adc/ti-adc12138.c
> index 9dc465a10ffc8d9f596e34215af685999235d690..e5ec4b073daae33d0e51cf21a3520f0ab2184828 100644
> --- a/drivers/iio/adc/ti-adc12138.c
> +++ b/drivers/iio/adc/ti-adc12138.c
> @@ -38,15 +38,13 @@ enum {
>  struct adc12138 {
>  	struct spi_device *spi;
>  	unsigned int id;
> -	/* conversion clock */
> -	struct clk *cclk;
>  	/* positive analog voltage reference */
>  	struct regulator *vref_p;
>  	/* negative analog voltage reference */
>  	struct regulator *vref_n;
>  	struct mutex lock;
>  	struct completion complete;
> -	/* The number of cclk periods for the S/H's acquisition time */
> +	/* The number of conversion clock periods for the S/H's acquisition time */
>  	unsigned int acquisition_time;
>  	/*
>  	 * Maximum size needed: 16x 2 bytes ADC data + 8 bytes timestamp.
> @@ -400,6 +398,7 @@ static int adc12138_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
>  	struct adc12138 *adc;
> +	struct clk *cclk;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> @@ -435,9 +434,14 @@ static int adc12138_probe(struct spi_device *spi)
>  	if (ret)
>  		adc->acquisition_time = 10;
>  
> -	adc->cclk = devm_clk_get(&spi->dev, NULL);
> -	if (IS_ERR(adc->cclk))
> -		return PTR_ERR(adc->cclk);
> +	ret = devm_request_irq(&spi->dev, spi->irq, adc12138_eoc_handler,
> +			       IRQF_TRIGGER_RISING, indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	cclk = devm_clk_get_enabled(&spi->dev, NULL);
> +	if (IS_ERR(cclk))
> +		return PTR_ERR(cclk);
>  
>  	adc->vref_p = devm_regulator_get(&spi->dev, "vref-p");
>  	if (IS_ERR(adc->vref_p))
> @@ -454,18 +458,9 @@ static int adc12138_probe(struct spi_device *spi)
>  			return ret;
>  	}
>  
> -	ret = devm_request_irq(&spi->dev, spi->irq, adc12138_eoc_handler,
> -			       IRQF_TRIGGER_RISING, indio_dev->name, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(adc->cclk);
> -	if (ret)
> -		return ret;
> -
>  	ret = regulator_enable(adc->vref_p);
>  	if (ret)
> -		goto err_clk_disable;
> +		return ret;
>  
>  	if (!IS_ERR(adc->vref_n)) {
>  		ret = regulator_enable(adc->vref_n);
> @@ -496,8 +491,6 @@ static int adc12138_probe(struct spi_device *spi)
>  		regulator_disable(adc->vref_n);
>  err_vref_p_disable:
>  	regulator_disable(adc->vref_p);
> -err_clk_disable:
> -	clk_disable_unprepare(adc->cclk);
>  
>  	return ret;
>  }
> @@ -512,7 +505,6 @@ static void adc12138_remove(struct spi_device *spi)
>  	if (!IS_ERR(adc->vref_n))
>  		regulator_disable(adc->vref_n);
>  	regulator_disable(adc->vref_p);
> -	clk_disable_unprepare(adc->cclk);
>  }
>  
>  static const struct of_device_id adc12138_dt_ids[] = {
>