[PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Rakuram Eswaran]

This patch refactors pxamci_probe() to use devm-managed resource
allocation (e.g. devm_dma_request_chan()) and dev_err_probe() for
improved readability and automatic cleanup on probe failure.

This eliminates redundant NULL assignments and manual release logic.

This issue was originally reported by Smatch:
drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'

The warning occurred because a pointer was set to NULL before using
PTR_ERR(), leading to PTR_ERR(0) and an incorrect 0 return value.
This refactor eliminates that condition while improving overall
error handling robustness.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
---

Changes since v1:
Following Uwe Kleine-König’s suggestion:
- Replaced dma_request_chan() with devm_dma_request_chan() to make DMA
  channel allocation devm-managed and avoid manual release paths.
- Used dev_err_probe() for improved error reporting and consistent
  probe failure handling.
- Removed redundant NULL assignments and obsolete goto-based cleanup logic.
- Updated commit message to better describe the intent of the change.

Testing note:
I do not have access to appropriate hardware for runtime testing. 
Any help verifying on actual hardware would be appreciated.

Build and Analysis:
This patch was compiled against the configuration file reported by
0day CI in the above link (config: s390-randconfig-r071-20251004) using
`s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`. 

Static analysis was performed with Smatch to ensure the reported warning 
no longer reproduces after applying this fix.

Command used for verification:
  ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
  ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c

 drivers/mmc/host/pxamci.c | 57 +++++++++++++++------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 26d03352af63..d03388f64934 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -652,11 +652,14 @@ static int pxamci_probe(struct platform_device *pdev)
 	host->clkrt = CLKRT_OFF;
 
 	host->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(host->clk)) {
-		host->clk = NULL;
-		return PTR_ERR(host->clk);
-	}
+	if (IS_ERR(host->clk))
+		return dev_err_probe(dev, PTR_ERR(host->clk),
+				     "Failed to acquire clock\n");
 
+	/*
+	 * Note that the return value of clk_get_rate() is only valid
+	 * if the clock is enabled.
+	 */
 	host->clkrate = clk_get_rate(host->clk);
 
 	/*
@@ -703,46 +706,36 @@ static int pxamci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mmc);
 
-	host->dma_chan_rx = dma_request_chan(dev, "rx");
-	if (IS_ERR(host->dma_chan_rx)) {
-		host->dma_chan_rx = NULL;
+	host->dma_chan_rx = devm_dma_request_chan(dev, "rx");
+	if (IS_ERR(host->dma_chan_rx))
 		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
 				     "unable to request rx dma channel\n");
-	}
 
-	host->dma_chan_tx = dma_request_chan(dev, "tx");
-	if (IS_ERR(host->dma_chan_tx)) {
-		dev_err(dev, "unable to request tx dma channel\n");
-		ret = PTR_ERR(host->dma_chan_tx);
-		host->dma_chan_tx = NULL;
-		goto out;
-	}
+	host->dma_chan_tx = devm_dma_request_chan(dev, "tx");
+	if (IS_ERR(host->dma_chan_tx))
+		return dev_err_probe(dev, PTR_ERR(host->dma_chan_tx),
+				     "unable to request tx dma channel\n");
 
 	if (host->pdata) {
 		host->detect_delay_ms = host->pdata->detect_delay_ms;
 
 		host->power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
-		if (IS_ERR(host->power)) {
-			ret = PTR_ERR(host->power);
-			dev_err(dev, "Failed requesting gpio_power\n");
-			goto out;
-		}
+		if (IS_ERR(host->power))
+			return dev_err_probe(dev, PTR_ERR(host->power),
+						"Failed requesting gpio_power\n");
 
 		/* FIXME: should we pass detection delay to debounce? */
 		ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
-		if (ret && ret != -ENOENT) {
-			dev_err(dev, "Failed requesting gpio_cd\n");
-			goto out;
-		}
+		if (ret && ret != -ENOENT)
+			return dev_err_probe(dev, ret, "Failed requesting gpio_cd\n");
 
 		if (!host->pdata->gpio_card_ro_invert)
 			mmc->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
 		ret = mmc_gpiod_request_ro(mmc, "wp", 0, 0);
-		if (ret && ret != -ENOENT) {
-			dev_err(dev, "Failed requesting gpio_ro\n");
-			goto out;
-		}
+		if (ret && ret != -ENOENT)
+			return dev_err_probe(dev, ret, "Failed requesting gpio_ro\n");
+
 		if (!ret)
 			host->use_ro_gpio = true;
 
@@ -759,16 +752,8 @@ static int pxamci_probe(struct platform_device *pdev)
 	if (ret) {
 		if (host->pdata && host->pdata->exit)
 			host->pdata->exit(dev, mmc);
-		goto out;
 	}
 
-	return 0;
-
-out:
-	if (host->dma_chan_rx)
-		dma_release_channel(host->dma_chan_rx);
-	if (host->dma_chan_tx)
-		dma_release_channel(host->dma_chan_tx);
 	return ret;
 }
 
-- 
2.48.1

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Khalid Aziz]

On 10/14/25 12:46 PM, Rakuram Eswaran wrote:
> This patch refactors pxamci_probe() to use devm-managed resource
> allocation (e.g. devm_dma_request_chan()) and dev_err_probe() for
> improved readability and automatic cleanup on probe failure.
> 
> This eliminates redundant NULL assignments and manual release logic.
> 
> This issue was originally reported by Smatch:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> 
> The warning occurred because a pointer was set to NULL before using
> PTR_ERR(), leading to PTR_ERR(0) and an incorrect 0 return value.
> This refactor eliminates that condition while improving overall
> error handling robustness.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> ---
> 
> Changes since v1:
> Following Uwe Kleine-König’s suggestion:
> - Replaced dma_request_chan() with devm_dma_request_chan() to make DMA
>    channel allocation devm-managed and avoid manual release paths.
> - Used dev_err_probe() for improved error reporting and consistent
>    probe failure handling.
> - Removed redundant NULL assignments and obsolete goto-based cleanup logic.
> - Updated commit message to better describe the intent of the change.
> 
> Testing note:
> I do not have access to appropriate hardware for runtime testing.
> Any help verifying on actual hardware would be appreciated.
> 
> Build and Analysis:
> This patch was compiled against the configuration file reported by
> 0day CI in the above link (config: s390-randconfig-r071-20251004) using
> `s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`.
> 
> Static analysis was performed with Smatch to ensure the reported warning
> no longer reproduces after applying this fix.
> 
> Command used for verification:
>    ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
>    ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c
> 
>   drivers/mmc/host/pxamci.c | 57 +++++++++++++++------------------------
>   1 file changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..d03388f64934 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -652,11 +652,14 @@ static int pxamci_probe(struct platform_device *pdev)
>   	host->clkrt = CLKRT_OFF;
>   
>   	host->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(host->clk)) {
> -		host->clk = NULL;
> -		return PTR_ERR(host->clk);
> -	}
> +	if (IS_ERR(host->clk))
> +		return dev_err_probe(dev, PTR_ERR(host->clk),
> +				     "Failed to acquire clock\n");
>   
> +	/*
> +	 * Note that the return value of clk_get_rate() is only valid
> +	 * if the clock is enabled.
> +	 */
>   	host->clkrate = clk_get_rate(host->clk);
>   
>   	/*
> @@ -703,46 +706,36 @@ static int pxamci_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, mmc);
>   
> -	host->dma_chan_rx = dma_request_chan(dev, "rx");
> -	if (IS_ERR(host->dma_chan_rx)) {
> -		host->dma_chan_rx = NULL;
> +	host->dma_chan_rx = devm_dma_request_chan(dev, "rx");
> +	if (IS_ERR(host->dma_chan_rx))
>   		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
>   				     "unable to request rx dma channel\n");
> -	}
>   
> -	host->dma_chan_tx = dma_request_chan(dev, "tx");
> -	if (IS_ERR(host->dma_chan_tx)) {
> -		dev_err(dev, "unable to request tx dma channel\n");
> -		ret = PTR_ERR(host->dma_chan_tx);
> -		host->dma_chan_tx = NULL;
> -		goto out;
> -	}
> +	host->dma_chan_tx = devm_dma_request_chan(dev, "tx");
> +	if (IS_ERR(host->dma_chan_tx))
> +		return dev_err_probe(dev, PTR_ERR(host->dma_chan_tx),
> +				     "unable to request tx dma channel\n");
>   
>   	if (host->pdata) {
>   		host->detect_delay_ms = host->pdata->detect_delay_ms;
>   
>   		host->power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
> -		if (IS_ERR(host->power)) {
> -			ret = PTR_ERR(host->power);
> -			dev_err(dev, "Failed requesting gpio_power\n");
> -			goto out;
> -		}
> +		if (IS_ERR(host->power))
> +			return dev_err_probe(dev, PTR_ERR(host->power),
> +						"Failed requesting gpio_power\n");
>   
>   		/* FIXME: should we pass detection delay to debounce? */
>   		ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
> -		if (ret && ret != -ENOENT) {
> -			dev_err(dev, "Failed requesting gpio_cd\n");
> -			goto out;
> -		}
> +		if (ret && ret != -ENOENT)
> +			return dev_err_probe(dev, ret, "Failed requesting gpio_cd\n");
>   
>   		if (!host->pdata->gpio_card_ro_invert)
>   			mmc->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>   
>   		ret = mmc_gpiod_request_ro(mmc, "wp", 0, 0);
> -		if (ret && ret != -ENOENT) {
> -			dev_err(dev, "Failed requesting gpio_ro\n");
> -			goto out;
> -		}
> +		if (ret && ret != -ENOENT)
> +			return dev_err_probe(dev, ret, "Failed requesting gpio_ro\n");
> +
>   		if (!ret)
>   			host->use_ro_gpio = true;
>   
> @@ -759,16 +752,8 @@ static int pxamci_probe(struct platform_device *pdev)
>   	if (ret) {
>   		if (host->pdata && host->pdata->exit)
>   			host->pdata->exit(dev, mmc);
> -		goto out;
>   	}
>   
> -	return 0;
> -
> -out:
> -	if (host->dma_chan_rx)
> -		dma_release_channel(host->dma_chan_rx);
> -	if (host->dma_chan_tx)
> -		dma_release_channel(host->dma_chan_tx);
>   	return ret;
>   }
>   

This looks good now.

Reviewed-by: Khalid Aziz <khalid@kernel.org>

--
Khalid

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]

On Wed, Oct 15, 2025 at 12:16:57AM +0530, Rakuram Eswaran wrote:
> This patch refactors pxamci_probe() to use devm-managed resource
> allocation (e.g. devm_dma_request_chan()) and dev_err_probe() for
> improved readability and automatic cleanup on probe failure.
> 
> This eliminates redundant NULL assignments and manual release logic.
> 
> This issue was originally reported by Smatch:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> 
> The warning occurred because a pointer was set to NULL before using
> PTR_ERR(), leading to PTR_ERR(0) and an incorrect 0 return value.
> This refactor eliminates that condition while improving overall
> error handling robustness.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> ---
> 
> Changes since v1:
> Following Uwe Kleine-König’s suggestion:
> - Replaced dma_request_chan() with devm_dma_request_chan() to make DMA
>   channel allocation devm-managed and avoid manual release paths.
> - Used dev_err_probe() for improved error reporting and consistent
>   probe failure handling.
> - Removed redundant NULL assignments and obsolete goto-based cleanup logic.
> - Updated commit message to better describe the intent of the change.
> 
> Testing note:
> I do not have access to appropriate hardware for runtime testing. 
> Any help verifying on actual hardware would be appreciated.
> 
> Build and Analysis:
> This patch was compiled against the configuration file reported by
> 0day CI in the above link (config: s390-randconfig-r071-20251004) using
> `s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`. 
> 
> Static analysis was performed with Smatch to ensure the reported warning 
> no longer reproduces after applying this fix.
> 
> Command used for verification:
>   ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
>   ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c
> 
>  drivers/mmc/host/pxamci.c | 57 +++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..d03388f64934 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -652,11 +652,14 @@ static int pxamci_probe(struct platform_device *pdev)
>  	host->clkrt = CLKRT_OFF;
>  
>  	host->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(host->clk)) {
> -		host->clk = NULL;
> -		return PTR_ERR(host->clk);
> -	}
> +	if (IS_ERR(host->clk))
> +		return dev_err_probe(dev, PTR_ERR(host->clk),
> +				     "Failed to acquire clock\n");
>  
> +	/*
> +	 * Note that the return value of clk_get_rate() is only valid
> +	 * if the clock is enabled.
> +	 */

The intention of this comment in my WIP suggestion was to point out
another thing to fix as the precondition for calling clk_get_rate()
isn't asserted. If you don't want to address this (which is fine),
drop the comment (or improve my wording to make it more obvious that
there is something to fix).

>  	host->clkrate = clk_get_rate(host->clk);
>  
>  	/*
> [...]
> @@ -759,16 +752,8 @@ static int pxamci_probe(struct platform_device *pdev)
>  	if (ret) {
>  		if (host->pdata && host->pdata->exit)
>  			host->pdata->exit(dev, mmc);
> -		goto out;
>  	}
>  
> -	return 0;
> -
> -out:
> -	if (host->dma_chan_rx)
> -		dma_release_channel(host->dma_chan_rx);
> -	if (host->dma_chan_tx)
> -		dma_release_channel(host->dma_chan_tx);

I was lazy in my prototype patch and didn't drop the calls to
dma_release_channel() in pxamci_remove(). For a proper patch this is
required though.

To continue the quest: Now that I looked at pxamci_remove(): `mmc` is
always non-NULL, so the respective check can be dropped.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Rakuram Eswaran]

On Thu, 16 Oct 2025 at 14:20, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
>
> On Wed, Oct 15, 2025 at 12:16:57AM +0530, Rakuram Eswaran wrote:
> > This patch refactors pxamci_probe() to use devm-managed resource
> > allocation (e.g. devm_dma_request_chan()) and dev_err_probe() for
> > improved readability and automatic cleanup on probe failure.
> >
> > This eliminates redundant NULL assignments and manual release logic.
> >
> > This issue was originally reported by Smatch:
> > drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> >
> > The warning occurred because a pointer was set to NULL before using
> > PTR_ERR(), leading to PTR_ERR(0) and an incorrect 0 return value.
> > This refactor eliminates that condition while improving overall
> > error handling robustness.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> > Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> > Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> > ---
> >
> > Changes since v1:
> > Following Uwe Kleine-König’s suggestion:
> > - Replaced dma_request_chan() with devm_dma_request_chan() to make DMA
> >   channel allocation devm-managed and avoid manual release paths.
> > - Used dev_err_probe() for improved error reporting and consistent
> >   probe failure handling.
> > - Removed redundant NULL assignments and obsolete goto-based cleanup logic.
> > - Updated commit message to better describe the intent of the change.
> >
> > Testing note:
> > I do not have access to appropriate hardware for runtime testing.
> > Any help verifying on actual hardware would be appreciated.
> >
> > Build and Analysis:
> > This patch was compiled against the configuration file reported by
> > 0day CI in the above link (config: s390-randconfig-r071-20251004) using
> > `s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`.
> >
> > Static analysis was performed with Smatch to ensure the reported warning
> > no longer reproduces after applying this fix.
> >
> > Command used for verification:
> >   ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
> >   ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c
> >
> >  drivers/mmc/host/pxamci.c | 57 +++++++++++++++------------------------
> >  1 file changed, 21 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> > index 26d03352af63..d03388f64934 100644
> > --- a/drivers/mmc/host/pxamci.c
> > +++ b/drivers/mmc/host/pxamci.c
> > @@ -652,11 +652,14 @@ static int pxamci_probe(struct platform_device *pdev)
> >       host->clkrt = CLKRT_OFF;
> > 
> >       host->clk = devm_clk_get(dev, NULL);
> > -     if (IS_ERR(host->clk)) {
> > -             host->clk = NULL;
> > -             return PTR_ERR(host->clk);
> > -     }
> > +     if (IS_ERR(host->clk))
> > +             return dev_err_probe(dev, PTR_ERR(host->clk),
> > +                                  "Failed to acquire clock\n");
> >
> > +     /*
> > +      * Note that the return value of clk_get_rate() is only valid
> > +      * if the clock is enabled.
> > +      */
>
> The intention of this comment in my WIP suggestion was to point out
> another thing to fix as the precondition for calling clk_get_rate()
> isn't asserted. If you don't want to address this (which is fine),
> drop the comment (or improve my wording to make it more obvious that
> there is something to fix).
>

Hi Uwe,

Sorry for the delayed reply as I was in vacation. 

Ah, got it — I’ll drop the clk_get_rate() comment since it was only a reminder
from your WIP suggestion.

Just to confirm, are you referring to adding a call to clk_prepare_enable()
before clk_get_rate()? I can move the clk_get_rate() call after
clk_prepare_enable(), or drop the comment entirely.

If my understanding is correct, I’ll keep v3 focused on the current set of
fixes and handle the clk_get_rate() precondition (by moving it after
clk_prepare_enable()) in a follow-up patch. That should keep the scope of each
change clean and review-friendly.

> > -out:
> > -     if (host->dma_chan_rx)
> > -             dma_release_channel(host->dma_chan_rx);
> > -     if (host->dma_chan_tx)
> > -             dma_release_channel(host->dma_chan_tx);
>
> I was lazy in my prototype patch and didn't drop the calls to
> dma_release_channel() in pxamci_remove(). For a proper patch this is
> required though.
>
> To continue the quest: Now that I looked at pxamci_remove(): `mmc` is
> always non-NULL, so the respective check can be dropped.
>

Understood. Since pxamci_remove() is only called after successful allocation
and initialization in probe(), mmc will always be a valid pointer. I’ll drop
the if (mmc) check in v3 as it can never be NULL in normal operation, and
remove the dma_release_channel() calls as well.

I’ve prepared a preview of the v3 patch incorporating your previous comments.
Before sending it out formally, I wanted to share it with you to confirm that
the updates look good — especially the cleanup changes in pxamci_remove() and
the dropped clk_get_rate() comment.

static void pxamci_remove(struct platform_device *pdev)
{
	struct mmc_host *mmc = platform_get_drvdata(pdev);
	struct pxamci_host *host = mmc_priv(mmc);

	mmc_remove_host(mmc);

	if (host->pdata && host->pdata->exit)
		host->pdata->exit(&pdev->dev, mmc);

	pxamci_stop_clock(host);
	writel(TXFIFO_WR_REQ|RXFIFO_RD_REQ|CLK_IS_OFF|STOP_CMD|
			END_CMD_RES|PRG_DONE|DATA_TRAN_DONE,
			host->base + MMC_I_MASK);

	dmaengine_terminate_all(host->dma_chan_rx);
	dmaengine_terminate_all(host->dma_chan_tx);
}

Please let me know if anything still needs adjustment before v3.

Best Regards,
Rakuram

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]

Hello Rakuram,

On Tue, Oct 21, 2025 at 12:02:07AM +0530, Rakuram Eswaran wrote:
> On Thu, 16 Oct 2025 at 14:20, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> > On Wed, Oct 15, 2025 at 12:16:57AM +0530, Rakuram Eswaran wrote:
> Sorry for the delayed reply as I was in vacation. 

I didn't hold my breath :-O

> Ah, got it — I’ll drop the clk_get_rate() comment since it was only a reminder
> from your WIP suggestion.
> 
> Just to confirm, are you referring to adding a call to clk_prepare_enable()
> before clk_get_rate()? I can move the clk_get_rate() call after
> clk_prepare_enable(), or drop the comment entirely.
> 
> If my understanding is correct, I’ll keep v3 focused on the current set of
> fixes and handle the clk_get_rate() precondition (by moving it after
> clk_prepare_enable()) in a follow-up patch. That should keep the scope of each
> change clean and review-friendly.
> 
> > > -out:
> > > -     if (host->dma_chan_rx)
> > > -             dma_release_channel(host->dma_chan_rx);
> > > -     if (host->dma_chan_tx)
> > > -             dma_release_channel(host->dma_chan_tx);
> >
> > I was lazy in my prototype patch and didn't drop the calls to
> > dma_release_channel() in pxamci_remove(). For a proper patch this is
> > required though.
> >
> > To continue the quest: Now that I looked at pxamci_remove(): `mmc` is
> > always non-NULL, so the respective check can be dropped.
> >
> 
> Understood. Since pxamci_remove() is only called after successful allocation
> and initialization in probe(), mmc will always be a valid pointer. I’ll drop
> the if (mmc) check in v3 as it can never be NULL in normal operation, and
> remove the dma_release_channel() calls as well.

Yes, I suggest to make restructuring .remote a separate patch. (But
removing dma_release_channel belongs into the patch that introduces devm
to allocate the dma channels.)
 
> I’ve prepared a preview of the v3 patch incorporating your previous comments.
> Before sending it out formally, I wanted to share it with you to confirm that
> the updates look good — especially the cleanup changes in pxamci_remove() and
> the dropped clk_get_rate() comment.
> 
> static void pxamci_remove(struct platform_device *pdev)
> {
> 	struct mmc_host *mmc = platform_get_drvdata(pdev);
> 	struct pxamci_host *host = mmc_priv(mmc);
> 
> 	mmc_remove_host(mmc);
> 
> 	if (host->pdata && host->pdata->exit)
> 		host->pdata->exit(&pdev->dev, mmc);
> 
> 	pxamci_stop_clock(host);
> 	writel(TXFIFO_WR_REQ|RXFIFO_RD_REQ|CLK_IS_OFF|STOP_CMD|
> 			END_CMD_RES|PRG_DONE|DATA_TRAN_DONE,
> 			host->base + MMC_I_MASK);
> 
> 	dmaengine_terminate_all(host->dma_chan_rx);
> 	dmaengine_terminate_all(host->dma_chan_tx);
> }

Looks right.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Rakuram Eswaran]

On Tue, 21 Oct 2025 at 14:01, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
>
> Hello Rakuram,
>
> On Tue, Oct 21, 2025 at 12:02:07AM +0530, Rakuram Eswaran wrote:
> > On Thu, 16 Oct 2025 at 14:20, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> > > On Wed, Oct 15, 2025 at 12:16:57AM +0530, Rakuram Eswaran wrote:
> > Sorry for the delayed reply as I was in vacation.
>
> I didn't hold my breath :-O
>
> > Ah, got it — I’ll drop the clk_get_rate() comment since it was only a reminder
> > from your WIP suggestion.
> >
> > Just to confirm, are you referring to adding a call to clk_prepare_enable()
> > before clk_get_rate()? I can move the clk_get_rate() call after
> > clk_prepare_enable(), or drop the comment entirely.
> >
> > If my understanding is correct, I’ll keep v3 focused on the current set of
> > fixes and handle the clk_get_rate() precondition (by moving it after
> > clk_prepare_enable()) in a follow-up patch. That should keep the scope of each
> > change clean and review-friendly.
> >
> > > > -out:
> > > > -     if (host->dma_chan_rx)
> > > > -             dma_release_channel(host->dma_chan_rx);
> > > > -     if (host->dma_chan_tx)
> > > > -             dma_release_channel(host->dma_chan_tx);
> > >
> > > I was lazy in my prototype patch and didn't drop the calls to
> > > dma_release_channel() in pxamci_remove(). For a proper patch this is
> > > required though.
> > >
> > > To continue the quest: Now that I looked at pxamci_remove(): `mmc` is
> > > always non-NULL, so the respective check can be dropped.
> > >
> >
> > Understood. Since pxamci_remove() is only called after successful allocation
> > and initialization in probe(), mmc will always be a valid pointer. I’ll drop
> > the if (mmc) check in v3 as it can never be NULL in normal operation, and
> > remove the dma_release_channel() calls as well.
>
> Yes, I suggest to make restructuring .remote a separate patch. (But
> removing dma_release_channel belongs into the patch that introduces devm
> to allocate the dma channels.)
>

I believe ".remote" is a typo and you're referring to the _remove() function. 
Removing if(mmc) condition check from pxamci_remove() can be handled in a 
separate cleanup patch, while removing redundant dma_release_channel()
will be included in v3. 

Is my above understanding correct?

> > I’ve prepared a preview of the v3 patch incorporating your previous comments.
> > Before sending it out formally, I wanted to share it with you to confirm that
> > the updates look good — especially the cleanup changes in pxamci_remove() and
> > the dropped clk_get_rate() comment.
> >
> > static void pxamci_remove(struct platform_device *pdev)
> > {
> >       struct mmc_host *mmc = platform_get_drvdata(pdev);
> >       struct pxamci_host *host = mmc_priv(mmc);
> >
> >       mmc_remove_host(mmc);
> >
> >       if (host->pdata && host->pdata->exit)
> >               host->pdata->exit(&pdev->dev, mmc);
> >
> >       pxamci_stop_clock(host);
> >       writel(TXFIFO_WR_REQ|RXFIFO_RD_REQ|CLK_IS_OFF|STOP_CMD|
> >                       END_CMD_RES|PRG_DONE|DATA_TRAN_DONE,
> >                       host->base + MMC_I_MASK);
> >
> >       dmaengine_terminate_all(host->dma_chan_rx);
> >       dmaengine_terminate_all(host->dma_chan_tx);
> > }
>
> Looks right.
>

Thank you for the feedback.

Best Regards,
Rakuram

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

Hello Rakuram,

On Thu, Oct 23, 2025 at 05:28:17PM +0530, Rakuram Eswaran wrote:
> On Tue, 21 Oct 2025 at 14:01, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> > Yes, I suggest to make restructuring .remote a separate patch. (But
> > removing dma_release_channel belongs into the patch that introduces devm
> > to allocate the dma channels.)
> 
> I believe ".remote" is a typo and you're referring to the _remove() function. 
> Removing if(mmc) condition check from pxamci_remove() can be handled in a 
> separate cleanup patch, while removing redundant dma_release_channel()
> will be included in v3. 
> 
> Is my above understanding correct?

ack. remote vs. remove is one of my most-committed typos :-D

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mmc: pxamci: Simplify pxamci_probe() error handling using devm APIs

[Rakuram Eswaran]

On Thu, 23 Oct 2025 at 18:28, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
>
> Hello Rakuram,
>
> On Thu, Oct 23, 2025 at 05:28:17PM +0530, Rakuram Eswaran wrote:
> > On Tue, 21 Oct 2025 at 14:01, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> > > Yes, I suggest to make restructuring .remote a separate patch. (But
> > > removing dma_release_channel belongs into the patch that introduces devm
> > > to allocate the dma channels.)
> >
> > I believe ".remote" is a typo and you're referring to the _remove() function.
> > Removing if(mmc) condition check from pxamci_remove() can be handled in a
> > separate cleanup patch, while removing redundant dma_release_channel()
> > will be included in v3.
> >
> > Is my above understanding correct?
>
> ack. remote vs. remove is one of my most-committed typos :-D
>

Understood, thank you for confirming.

I've just sent the v3 patch. You can find it here:
https://lore.kernel.org/linux-mmc/20251023145432.164696-1-rakuram.e96@gmail.com/T/#u

Best Regards,
Rakuram