Hi Biju,
Thank you for the review.
On Fri, Apr 22, 2022 at 7:52 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > 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
>
