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

[Rakuram Eswaran <rakuram.e96@gmail.com>]

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