Re: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the DMA channels

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Biju,

Thank you for the review.

On Fri, Apr 22, 2022 at 7:52 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Lad Prabhakar,
>
> Thanks for the patch.
>
> > Subject: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the
> > DMA channels
> >
> > DMA channels requested by rz_ssi_dma_request() in rz_ssi_probe() were never
> > released in the error path apart from one place. This patch fixes this
> > issue by adding a devres action to release the DMA channels and dropping
> > the single rz_ssi_release_dma_channels() call which was placed in the error
> > path in case devm_snd_soc_register_component() failed.
> >
> > Fixes: 26ac471c5354 ("ASoC: sh: rz-ssi: Add SSI DMAC support")
> > Reported-by: Pavel Machek <pavel@denx.de>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  sound/soc/sh/rz-ssi.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c index
> > d9a684e71ec3..f04da1bf5680 100644
> > --- a/sound/soc/sh/rz-ssi.c
> > +++ b/sound/soc/sh/rz-ssi.c
> > @@ -912,6 +912,11 @@ static const struct snd_soc_component_driver
> > rz_ssi_soc_component = {
> >       .pcm_construct  = rz_ssi_pcm_new,
> >  };
> >
> > +static void rz_ssi_release_dma_channels_action(void *data) {
> > +     rz_ssi_release_dma_channels(data);
> > +}
> > +
> >  static int rz_ssi_probe(struct platform_device *pdev)  {
> >       struct rz_ssi_priv *ssi;
> > @@ -966,6 +971,11 @@ static int rz_ssi_probe(struct platform_device *pdev)
> >               dev_info(&pdev->dev, "DMA enabled");
> >               ssi->playback.transfer = rz_ssi_dma_transfer;
> >               ssi->capture.transfer = rz_ssi_dma_transfer;
> > +
> > +             ret = devm_add_action_or_reset(&pdev->dev,
> > +                                            rz_ssi_release_dma_channels_action,
> > ssi);
> > +             if (ret)
> > +                     return ret;
> >       }
> >
> >       ssi->playback.priv = ssi;
> > @@ -1027,8 +1037,6 @@ static int rz_ssi_probe(struct platform_device *pdev)
> >                                             rz_ssi_soc_dai,
> >                                             ARRAY_SIZE(rz_ssi_soc_dai));
> >       if (ret < 0) {
> > -             rz_ssi_release_dma_channels(ssi);
> > -
>
> Maybe we need to keep this as it is, otherwise DMA channel release will happen
> after CLK disable and Reset assert. Ideally release the channel, disable the clock and assert
> the reset.
>
Ok will move this call to individual error paths.

> Similarly, you may want to add "rz_ssi_release_dma_channels(ssi)" for error path related to
> Pm_runtime_resume_get.
>
yes this needs to go under all error paths except the pio chunk.

>
> Also with this change there is unbalanced release_dma_channels() one from devres and other from remove.
>
Agreed.

Cheers,
Prabhakar

> Regards,
> Biju
>
>
>
> >               pm_runtime_put(ssi->dev);
> >               pm_runtime_disable(ssi->dev);
> >               reset_control_assert(ssi->rstc);
> > --
> > 2.17.1
>