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

Uwe Kleine-König
2025-10-21T08:31:28Z

[-- 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 --]